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

[Kasireddy, Vivek]

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.

Thanks,
Vivek

> 
> Regards,
>   Frediano
> 
> > >
> > > > Cc: Frediano Ziglio <freddy77 at gmail.com>
> > > > Cc: Dongwon Kim <dongwon.kim at intel.com>
> > > > Signed-off-by: Hazwan Arif Mazlan <hazwan.arif.mazlan at intel.com>
> > > > Signed-off-by: Jin Chung Teng <jin.chung.teng at intel.com>
> > > > Signed-off-by: Vivek Kasireddy <vivek.kasireddy at intel.com>
> > > > ---
> > > >  server/gstreamer-encoder.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > 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,7 @@ static gboolean
> create_pipeline(SpiceGstEncoder
> > > *encoder)
> > > >  #ifdef HAVE_GSTREAMER_0_10
> > > >      const gchar *converter = "ffmpegcolorspace";
> > > >  #else
> > > > -    const gchar *converter = "videoconvert";
> > > > +    const gchar *converter = "videoconvert ! video/x-
> raw,format=NV12";
> > > >  #endif
> > > >      const gchar* gstenc_name = get_gst_codec_name(encoder);
> > > >      if (!gstenc_name) {
> > >
> > >
> > > Frediano