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

[Victor Toso]

On Wed, Jul 19, 2017 at 03:46:36PM +0200, Pavel Grunt wrote:
> On Wed, 2017-07-19 at 15:40 +0200, Victor Toso wrote:
> > On Wed, Jul 19, 2017 at 03:30:49PM +0200, Pavel Grunt wrote:
> > > On Wed, 2017-07-19 at 15:23 +0200, Victor Toso wrote:
> > > > Hi,
> > > > 
> > > > 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.
> > > > 
> > > > As I mentioned before, given the extra complexity that this is
> > > > requiring, I'm not convinced that it is a good fix because it relies on
> > > > the fact that we know the elements that we need (hard coded).
> > > > 
> > > > 1-) An user might have a different subset of plugins, like the fluendo
> > > >     ones or a different 3rd party ones. Do that work when we check for
> > > >     h264parse avdec_h264? I don't think so as it should be different
> > > >     plugins (one that people pay to have in their system)
> > > 
> > > This checks for presence of all the requirred plugins, for both
> > > h264parse and avdec_h264. the user must have both to detect it.
> > 
> > I can have h264 stream decoded without avdec_h264 -> Not required
> > 
> > I _might_ _still_ have failures due lack of plugins on runtime even with
> > h264parse -> Not enough
> > 
> > > It fixes the "regression" caused by the GstRegistry patch by checking for
> > > the
> > > elements like it was done before (but without parsing the full pipeline)
> > 
> > It does fix for this case. I would prefer to be agnostic about plugins,
> > really.
> > 
> > > > 
> > > > 2-) If playbin just needs an extra plugin, we don't check it and that
> > > >     means the pipeline will fail even if the caps were enabled.
> > > 
> > > It is about the plugin combinations already tested in the past. We have
> > > their
> > > name (and their usage in a pipeline) in the header file
> > 
> > That's correct but it doesn't mean that with playbin it'll be always
> > correct as we are trying to increase the possibilities (i.e not depend
> > solely on avdec_h264)
> > 
> > I don't think we will agree because you want to check for h264parse and
> > that's one thing I want to avoid.
> > 
> I do the check for the plugins defined in the header, if you remove h264parse
> from the header, it won't check for it

Which would mean that [0] is much simpler, no?

[0] https://lists.freedesktop.org/archives/spice-devel/2017-July/038634.html

> 
> 
> > > > 
> > > > I do think the right approach is as I mentioned before, fix the
> > > > filtering to show all decoders for h264 and that's it. If the pipeline
> > > > fails, that's fine because it is *hard* to *ensure* that it'll work
> > > > anyway without an actual video stream.
> > > > 
> > > > > ---
> > > > >  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
-------------- 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/20170719/fd002916/attachment.sig>