[Spice-devel] [PATCH spice-gtk v3 5/6] display-gst: check GstRegistry for decoding elements

[Christophe Fergeau]

On Tue, May 16, 2017 at 04:48:17PM +0200, Victor Toso wrote:
> From: Victor Toso <me at victortoso.com>
> 
> With this patch, we can find all the elements in the registry that are
> video decoders which can handle the predefined GstCaps.
> 
> Main benefits are:
> - We don't rely on predefined list of GstElements. We don't need to
>   update them;

"on a predefined list of GstElements"

> - debugging: It does help to know what the registry has at runtime;
> 
> Signed-off-by: Victor Toso <victortoso at redhat.com>
> Acked-by: Frediano Ziglio <fziglio at redhat.com>
> Signed-off-by: Victor Toso <me at victortoso.com>
> ---
>  src/channel-display-gst.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> index fa0ec90..e675062 100644
> --- a/src/channel-display-gst.c
> +++ b/src/channel-display-gst.c
> @@ -524,6 +524,54 @@ VideoDecoder* create_gstreamer_decoder(int codec_type, display_stream *stream)
>  G_GNUC_INTERNAL
>  gboolean gstvideo_has_codec(int codec_type)
>  {
> +#if GST_CHECK_VERSION(1,9,0)

Why is this limited to gstreamer 1.9 and newer? (a small note in the
comit log could probably be helpful)

> +    GList *all_decoders, *codec_decoders;
> +    GstCaps *caps;
> +    GstElementFactoryListType type;
> +
> +    g_return_val_if_fail(gstvideo_init(), FALSE);
> +    g_return_val_if_fail(VALID_VIDEO_CODEC_TYPE(codec_type), FALSE);
> +
> +    type = GST_ELEMENT_FACTORY_TYPE_DECODER | GST_ELEMENT_FACTORY_TYPE_MEDIA_VIDEO;
> +    all_decoders = gst_element_factory_list_get_elements(type, GST_RANK_NONE);
> +    if (all_decoders == NULL) {
> +        spice_warning("No video decoders from GStreamer were found");
> +        return FALSE;
> +    }
> +
> +    caps = gst_caps_from_string(gst_opts[codec_type].dec_caps);
> +    codec_decoders = gst_element_factory_list_filter(all_decoders, caps, GST_PAD_SINK, TRUE);
> +    gst_caps_unref(caps);
> +
> +    if (codec_decoders == NULL) {
> +        spice_debug("From %u decoders, none can handle '%s'",
> +                    g_list_length(all_decoders), gst_opts[codec_type].dec_caps);
> +        gst_plugin_feature_list_free(all_decoders);
> +        return FALSE;
> +    }
> +
> +    if (spice_util_get_debug()) {

I would split this debugging code in a separate helper, it's as long as
the main code.

> +        GList *l;
> +        GString *msg = g_string_new(NULL);
> +        /* Print list of available decoders to make debugging easier */
> +        g_string_printf(msg, "From %3u video decoder elements, %2u can handle caps %12s: ",
> +                        g_list_length(all_decoders), g_list_length(codec_decoders),
> +                        gst_opts[codec_type].dec_caps);
> +
> +        for (l = codec_decoders; l != NULL; l = l->next) {
> +            GstPluginFeature *pfeat = GST_PLUGIN_FEATURE(l->data);
> +            g_string_append_printf(msg, "%s, ", gst_plugin_feature_get_name(pfeat));
> +        }
> +
> +        msg->str[msg->len - 2] = '\0';

I would replace this with
/* Drop trailing ", " */
g_string_truncate(msg, msg->len - 2);


> +        spice_debug("%s", msg->str);
> +        g_string_free(msg, TRUE);
> +    }
> +
> +    gst_plugin_feature_list_free(codec_decoders);
> +    gst_plugin_feature_list_free(all_decoders);
> +    return TRUE;
> +#else
>      gboolean has_codec = FALSE;
>  
>      VideoDecoder *decoder = create_gstreamer_decoder(codec_type, NULL);

If we don't attempt to call create_gstreamer_decoder, aren't we
restricting our testing to a smaller subset of what we used to be doing
(ie test if we can create a pipeline like appsrc ! $caps ! $decoder !
videoconvert ! appsink)? If appsrc or videoconvert are missing, are we
going to catch this after your changes?

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170606/8b81935c/attachment-0001.sig>