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

[Victor Toso]

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.

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. 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 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.

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 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.... 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/7631cd5b/attachment.sig>