[Spice-devel] [PATCH spice-gtk v2 4/5] display-gst: remove SPICE_GSTVIDEO_AUTO check

[Victor Toso]

Hi,

On Fri, May 12, 2017 at 08:45:39AM -0400, Frediano Ziglio wrote:
> > 
> > From: Victor Toso <me at victortoso.com>
> > 
> > By using this environment variable, we could use decodebin to let
> > GStreamer automatically find the best elements to get the streaming
> > decoded. It was disable by default, in an attempt to have a easy way
> > to test it.
> > 
> > Follow up patch will use Playbin to create the pipeline which does the
> > similar behavior but with less work to maintain the pipeline.
> > 
> > Remove this in a separated patch to reduce the code changes.
> > 
> > Signed-off-by: Victor Toso <victortoso at redhat.com>
> > Signed-off-by: Victor Toso <me at victortoso.com>
> > ---
> >  src/channel-display-gst.c | 22 ++++------------------
> >  1 file changed, 4 insertions(+), 18 deletions(-)
> > 
> > diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> > index a9c45f6..dbdcef8 100644
> > --- a/src/channel-display-gst.c
> > +++ b/src/channel-display-gst.c
> > @@ -54,12 +54,8 @@ static struct {
> >      const gchar *dec_name;
> >      const gchar *dec_caps;
> >  } gst_opts[] = {
> > -    /* decodebin will use vaapi if installed, which for a time could
> > -     * intentionally crash the application. So only use decodebin as a
> > -     * fallback or when SPICE_GSTVIDEO_AUTO is set.
> > -     * See: https://bugs.freedesktop.org/show_bug.cgi?id=90884
> > -     */
> > -    { "decodebin", "" },
> > +    /* Spice' video codec type starts on index 1 */
> > +    { NULL, NULL },
> >  
> >      /* SPICE_VIDEO_CODEC_TYPE_MJPEG */
> >      { "jpegdec", "image/jpeg" },
> > @@ -304,21 +300,10 @@ static gboolean handle_pipeline_message(GstBus *bus,
> > GstMessage *msg, gpointer v
> >  static gboolean create_pipeline(SpiceGstDecoder *decoder)
> >  {
> >      gchar *desc;
> > -    gboolean auto_enabled;
> > -    guint opt;
> >      GstAppSinkCallbacks appsink_cbs = { NULL };
> >      GError *err = NULL;
> >      GstBus *bus;
> >  
> > -    auto_enabled = (g_getenv("SPICE_GSTVIDEO_AUTO") != NULL);
> > -    if (auto_enabled || !VALID_VIDEO_CODEC_TYPE(decoder->base.codec_type)) {
> > -        SPICE_DEBUG("Trying %s for codec type %d %s",
> > -                    gst_opts[0].dec_name, decoder->base.codec_type,
> > -                    (auto_enabled) ? "(SPICE_GSTVIDEO_AUTO is set)" : "");
> > -        opt = 0;
> > -    } else {
> > -        opt = decoder->base.codec_type;
> > -    }
> 
> You removed the check for invalid codec type.
> This will result in a buffer overflow as codec_type at the end came
> directly from the remote end sending a StreamCreate message.
> Would be better to check this in create_gstreamer_decoder.
> A check is already present in gstvideo_has_codec in your newer code.

IMHO decoder->base.codec_type should be set only with a valid
codec_type. This happens at create_gstreamer_decoder() which is called
by gstvideo_has_codec() and neither of them does check if the codec_type
is valid (unless with the new code, as you noted).

Also when the stream is created, as you said, we don't check it too
because create_gstreamer_decoder() does not check it.

Better is to include the check in create_gstreamer_decoder() as we
should not create a decoder with invalid codec_type.

I'll fix it, thanks for the review!


> 
> >  
> >      /* - We schedule the frame display ourselves so set sync=false on
> >      appsink
> >       *   so the pipeline decodes them as fast as possible. This will also
> > @@ -330,7 +315,8 @@ static gboolean create_pipeline(SpiceGstDecoder *decoder)
> >      desc = g_strdup_printf("appsrc name=src is-live=true format=time
> >      max-bytes=0 block=true "
> >                             "caps=%s ! %s ! videoconvert ! appsink name=sink
> >                             "
> >                             "caps=video/x-raw,format=BGRx sync=false
> >                             drop=false",
> > -                           gst_opts[opt].dec_caps, gst_opts[opt].dec_name);
> > +                           gst_opts[decoder->base.codec_type].dec_caps,
> > +                           gst_opts[decoder->base.codec_type].dec_name);
> >      SPICE_DEBUG("GStreamer pipeline: %s", desc);
> >  
> >      decoder->pipeline = gst_parse_launch_full(desc, NULL,
> >      GST_PARSE_FLAG_FATAL_ERRORS, &err);
> 
> Frediano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170512/85c2a22b/attachment.sig>