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

[Victor Toso]

Hi,

On Tue, Jun 06, 2017 at 05:30:41PM +0200, Christophe Fergeau wrote:
> 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"

Fixed

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

From what was explained to me, since 1.9.0 the GStreamer got smarter to
deal with vaapi elements first. That matters on the playbin patch but
not here.

Here we list the elements based on GstCaps. That should work prior 1.9.0
just fine.

So, to answer your question, the only need for checking
GST_CHECK_VERSION(1,9,0) is due the ifdef in the array of capabilities
itself - gst_opts[].

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

Sure, will do

>
> > +        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);

Right, I'll change as well.

> > +        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)?

Yes, but we have a check at configure/build time for all major elements,
such as:

gst-plugins-base 1.0 : [appsrc videoconvert appsink]
gst-plugins-good 1.0 : [jpegdec vp8dec vp9dec]
gst-plugins-bad 1.0  : [h264parse]
gstreamer-libav 1.0  : [avdec_h264]

> If appsrc or videoconvert are missing, are we going to catch this
> after your changes?
>
> Christophe

Nops. So, my take is that if the user does not have appsrc, videoconvert
or appsink it shouldn't have --enable-gstvideo as that is the bare
minimum for our video pipeline to work. I'll try to patch configure.ac
for this.

This patch only checks the decoding elements. It is possible to have
more then one per video-codec and each of them could have extra elements
as requirement and definitely they will have different impact on
performance.

I think that checking that we have vp8dec should be enough to state that
we have what is needed to do decoding on vp8 video streams.

Cheers,
    toso
-------------- 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/20170627/f2b82aad/attachment.sig>