[Spice-devel] [PATCH spice-gtk] gst: Fix detection of h264 stream decoder
Victor Toso
victortoso at redhat.com
Thu Jul 20 10:04:35 UTC 2017
Hi,
On Thu, Jul 20, 2017 at 10:03:37AM +0200, Pavel Grunt wrote:
> Victor,
>
> it is faster to write NACK
You don't want feedback and rationale on the NACK?
I wish you had discussed with me on my proposal as well as I don't agree
that's wrong.
>
> 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
<quote>
Wouldn't that make it possible to find a parser but not a decoder and
evaluate the _has_codec() to true?
</quote>
Because, for sure, we know that we need decoders. Having the decoder and
not a parser might produce a failure, that's one thing that I'm
considering a okay-failure which should have proper fallback, etc.
Having a false positive without decoders, that's bad (and you agreed).
>
> > 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
Yes, there is room for improvement there! I do think we should check at
build time elements that we require (mainly from -plugins-base and
plugins-good). We do check for decoders but that's only evaluate to a
warning.
Cheers,
>
> > 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
-------------- 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/20170720/ed0fe670/attachment-0001.sig>
More information about the Spice-devel
mailing list