[Spice-devel] [PATCH 1/2] gstreamer-encoder: Use NV12 as the default vpp conversion format

[Frediano Ziglio]

Il giorno gio 21 set 2023 alle ore 22:19 Kasireddy, Vivek
<vivek.kasireddy at intel.com> ha scritto:
>
> Hi Frediano,
>
> > >
> > > > > From: Hazwan Arif Mazlan <hazwan.arif.mazlan at intel.com>
> > > > >
> > > > > Using NV12 as the output format for the videoconvert element would
> > > > > allow us to pair a s/w based encoder with a h/w based decoder for
> > > > > decoding the stream as most h/w based decoders only accept NV12 as
> > > > > the input format given its popularity.
> > > > >
> > > >
> > > > I don't fully understand the rationale.
> > > Yeah, I should have added more details.
> > >
> > > > Yes, the h/w codecs usually
> > > > would convert this to NV12 however should not this be done by
> > > > gstreamer instead?
> > > > Surely YUV conversion is useful but what if a software conversion
> > > > would like to use Y444 instead? With NV12 you would lose image
> > > > quality.
> > > > Isn't gstreamer supposed to come out with the best combination?
> > > > Maybe it would be easier to have a more complete pipeline string
> > > > instead specified for each codec?
> > > My understanding is that the goal with this patch is to ensure uniformity
> > > of the format and specifically address the case where we use x264enc
> > > on the encode side and msdkh264dec on the decode side. Since Y444
> > > is the default input format for x264enc, videoconvert converts the BGRx
> > > data into Y444. However, on the decode side, msdkh264dec can only work
> > > with NV12; so this patch is ensuring that we start with NV12 on the encode
> > > side as well. Jin Chung, feel free to add more details if I missed anything.
> > >
> > > Thanks,
> > > Vivek
> > >
> >
> > The problem is compatibility. A client with h/w h264 support should be
> > able to talk to a server without any changes.
> > This patch is not fixing this, unless you use a time machine and you
> > apply it at the time h264 and x264enc were introduced.
> > So, basically the client should be able to decode y444 produced by x264enc.
> > Either keep using x264enc if the server could send it or be able to
> > detect from the first data frame (it should be possible,
> > maybe with a fail and try) and use the correct pipeline.
> What I have learnt is that some Intel h/w cannot decode the default format
> used by x264enc (h264 (High 4:4:4 Predictive), yuv444p). Therefore, we'd have to
> use NV12 in these cases. However, I believe this choice has to be left to the user.
>
> In order to do this, I think it makes sense to have an environment variable
> (SPICE_CONVERTER_PREFERRED_FORMAT?) to override the default format used
> by x264enc.
>

That makes sense. What about making the change a bit more detailed, I
would extent the message
to something like

"Using NV12 as the output format for the videoconvert element would
allow us to pair a s/w based encoder with a h/w based decoder for
decoding the stream as most h/w based decoders only accept NV12 as
the input format given its popularity.

Although this restricts the formats used and potentially decrease video
quality the current capabilities exchange does not allow fine H264 tuning
so the server is not able to select a format better suitable for the
client. So use a more compatible, but potentially limited, format."

And in code

diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
index d8af91f1..057509b5 100644
--- a/server/gstreamer-encoder.c
+++ b/server/gstreamer-encoder.c
@@ -918,7 +918,8 @@ static gboolean create_pipeline(SpiceGstEncode
r *encoder)
 #ifdef HAVE_GSTREAMER_0_10
     const gchar *converter = "ffmpegcolorspace";
 #else
-    const gchar *converter = "videoconvert";
+    // Limit for compatibility, see "gstreamer-encoder: Use NV12 as
the default vpp conversion format" commit
+    const gchar *converter = "videoconvert ! video/x-raw,format=NV12";
 #endif
     const gchar* gstenc_name = get_gst_codec_name(encoder);
     if (!gstenc_name) {


Frediano