[Spice-devel] [PATCH spice-gtk] gst: Fix detection of h264 stream decoder
Victor Toso
victortoso at redhat.com
Wed Jul 19 14:15:54 UTC 2017
Hi,
On Wed, Jul 19, 2017 at 04:01:21PM +0200, Pavel Grunt wrote:
> On Wed, 2017-07-19 at 15:52 +0200, Victor Toso wrote:
> > 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
> >
> It is simple, because it is incorrect. Remove h264parse and vaapi plugins and
> keep avdec_h264, then try to connect to h264 streaming vm...
The pipeline will fail and that's fine. You shouldn't do that in the
first place.
My point is to check for decoders only. If you want to check for
parsers, this patch of yours will do it solely for hard coded elements.
You would need to do something nicer using GStreamer API to be agnostic
of specific parser elements.
And yet, we could miss something else now or in one year.
Yes, my take is to check for decoders and avoid this workarounds.
>
>
> > >
> > >
> > > > > >
> > > > > > 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/7f6673ca/attachment-0001.sig>
More information about the Spice-devel
mailing list