[Spice-devel] [PATCH spice-gtk] gst: Fix detection of h264 stream decoder

Thu Jul 20 08:03:37 UTC 2017

Victor,

it is faster to write NACK

On Thu, 2017-07-20 at 07:26 +0200, Victor Toso wrote:
> A new day, a new thread
> 
> On Wed, Jul 19, 2017 at 02:59:00PM +0200, Pavel Grunt wrote:
> > GStreamer's avdec_h264 needs h264parse to be able to process H264 video
> > streams. However the check for elements through GstRegistry does not
> > take into account the possible pipeline of elements (like "h264parse !
> > avdec_h264").
> > 
> > Fix that by checking for the elements by their name.
> 
> By reading your commit log, your are fixing a single use case by
> checking element's names that are hard coded to our codebase.
> 
> The reason for the following two patches were to remove the usage of
> hard coded names to check and play the stream.
> 
> Shortlog: display-gst: Use Playbin for GStreamer 1.9.0 onwards
> Commit  : ed697dd7fa165632bd0cbdb34c971c8001c433fa
> Victor Toso on Mon, 10 Jul 2017 17:34:23 +0200
> 
> Shortlog: display-gst: check GstRegistry for decoding elements
> Commit  : c9209aef946b3ad517e7794d2e5648caf5ee2bf9
> Victor Toso on Mon, 10 Jul 2017 17:33:10 +0200
> 
> So, this patch is going backwards in regard to one of the objectives of
> playbin's introducing besides the fact that it does not guarantee a
> solution for 3rd party plugins that might be filtered out.

if going backwards means to not break compatibility with the previous release
then yes, it goes backwards.

> 
> So, Let's try to be clear about what's going on:
> 
> The regression introduced by c9209aef946b3 is that avdec_h264 is filter
> out in gstvideo_has_codec() and for that reason, for systems that do not
> have any other decoder, our caps for decoding h264 will be disabled,
> which is a regression.
> 
> To fix the issue in a way that follows the objective of current code, we
> should simply fix the filter as I've suggested at [0]. By the way, I
> went to the GStreamer folks about this filtering issue and I understood
> that my code in regard to the filtering is wrong due the assumption of
> what is a subset. That's a bug alright.
> 
> https://lists.freedesktop.org/archives/spice-devel/2017-July/038634.html
> 
> Pavel, you saying that patch is incorrect is wrong.

Victor, I cannot say it is correct when it does something incorrectly...

>  If you want, you can
> say _incomplete_, but that's a fix.
> 
> Incomplete you may say, because you think it is important to check for
> h264parse element. I'm not against that
Aren't you?

>  but I do think we, again, need
> to approach #gstreamer folks and check with them that we want a more
> robust check for enabling or not a streaming given a video-codec. If we
> go this way and checking for parses seem important, we should do it in a
> more generic fashion.
> 
> Again, I would go with my patch that fixes an issue and see from now on
> how to make our check more robust and I would not be surprise if there
> is no way to guarantee 100% that checking every single element in a
> pipeline would in fact decode a stream and that's because we don't have
> the parameters for the stream itself embedded in our protocol besides
> other issues that may occur.

Victor, it is checking for the elements used for GST < 1.9 (consider el7,
etc...)

> 
> So, yes, if you remove h264parse from registry, we will say that the
> stream can be decoded with avdec_h264 while it can't. I think that's a
> weak argument

Victor, you have used the same argument for my oneliner
https://lists.freedesktop.org/archives/spice-devel/2017-July/038629.html

>  in favor of your patch and as I'm saying a few times now,
> we should consider failure an option, fallback to image compression or
> different streams with different video-codec types....

then you should remove the configure checks as well... (but it wasn't an
intention of your playbin series)

Have a nice day,
Pavel

>  if the fallback
> does not work, it is a bug even with your patch.
> 
> Cheers,
> 
> 
> > ---
> >  src/channel-display-gst.c | 25 +++++++++++++++++++++++++
> >  1 file changed, 25 insertions(+)
> > 
> > diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> > index 5042fc8..a9a044a 100644
> > --- a/src/channel-display-gst.c
> > +++ b/src/channel-display-gst.c
> > @@ -666,6 +666,31 @@ gboolean gstvideo_has_codec(int codec_type)
> >      codec_decoders = gst_element_factory_list_filter(all_decoders, caps,
> > GST_PAD_SINK, TRUE);
> >      gst_caps_unref(caps);
> >  
> > +    /* Check for the presence of decoding elements that could be filter
> > out.
> > +     * TODO: Improve filtering to avoid this...
> > +     */
> > +    {
> > +        GList *decoder_elements = NULL;
> > +        gchar **decoders_by_names;
> > +        guint i = 0;
> > +        decoders_by_names = g_strsplit(gst_opts[codec_type].dec_name, "!",
> > -1);
> > +        for (i = 0; decoders_by_names[i] != NULL; i++) {
> > +            GstElementFactory *element =
> > gst_element_factory_find(g_strstrip(decoders_by_names[i]));
> > +            if (element == NULL) {
> > +                gst_plugin_feature_list_free(decoder_elements);
> > +                decoder_elements = NULL;
> > +                break;
> > +            }
> > +            if (g_list_find(codec_decoders, element)) {
> > +                gst_object_unref(element);
> > +            } else {
> > +                decoder_elements = g_list_append(decoder_elements,
> > element);
> > +            }
> > +        }
> > +        codec_decoders = g_list_concat(codec_decoders, decoder_elements);
> > +        g_strfreev(decoders_by_names);
> > +    }
> > +
> >      if (codec_decoders == NULL) {
> >          spice_debug("From %u decoders, none can handle '%s'",
> >                      g_list_length(all_decoders),
> > gst_opts[codec_type].dec_caps);
> > -- 
> > 2.13.3
> > 
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel