[Spice-devel] [PATCH v1 2/2] display-gst: change compile to runtime check of vaapisink element

[Victor Toso]

Hi,

On Sun, Jan 27, 2019 at 06:25:18AM -0500, Frediano Ziglio wrote:
> > 
> > From: Victor Toso <me at victortoso.com>
> > 
> > Runtime check is better fit to enable/disable vaapisink. For that, use
> > spice_check_gst_plugin_version() from previous commit.
> > 
> > Signed-off-by: Victor Toso <victortoso at redhat.com>
> > ---
> >  src/channel-display-gst.c | 25 +++++++++++--------------
> >  1 file changed, 11 insertions(+), 14 deletions(-)
> > 
> > diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> > index 4272ade..ef578db 100644
> > --- a/src/channel-display-gst.c
> > +++ b/src/channel-display-gst.c
> > @@ -21,6 +21,7 @@
> >  #include "spice-common.h"
> >  #include "spice-channel-priv.h"
> >  
> > +#include "spice-session-priv.h"
> >  #include "channel-display-priv.h"
> >  
> >  #include <gst/gst.h>
> > @@ -431,6 +432,7 @@ static gboolean create_pipeline(SpiceGstDecoder *decoder)
> >      GstElement *playbin, *sink;
> >      SpiceGstPlayFlags flags;
> >      GstCaps *caps;
> > +    GError *err = NULL;
> >  
> >      playbin = gst_element_factory_make("playbin", "playbin");
> >      if (playbin == NULL) {
> > @@ -461,29 +463,24 @@ static gboolean create_pipeline(SpiceGstDecoder
> > *decoder)
> >                   NULL);
> >  
> >          decoder->appsink = GST_APP_SINK(sink);
> > -    } else {
> > +    } else if (!spice_check_gst_plugin_version("vaapisink", 1, 14, 0, &err))
> > {
> > +        GstPluginFeature *vaapisink;
> 
> I would update the style and declare and initialise on the same
> statement.
> Time to move to a more C99 style, no reason to stick to C89.

Right, I don't mind.

> > +
> >          /* handle has received, it means playbin will render directly into
> >           * widget using the gstvideooverlay interface instead of app-sink.
> >           */
> >          SPICE_DEBUG("Video is presented using gstreamer's GstVideoOverlay
> >          interface");
> >  
> > -#if !GST_CHECK_VERSION(1,14,0)
> >          /* Avoid using vaapisink if exist since vaapisink could be
> >           * buggy when it is combined with playbin. changing its rank to
> >           * none will make playbin to avoid of using it.
> >           */
> > -        GstRegistry *registry = NULL;
> > -        GstPluginFeature *vaapisink = NULL;
> > -
> > -        registry = gst_registry_get();
> > -        if (registry) {
> > -            vaapisink = gst_registry_lookup_feature(registry, "vaapisink");
> > -        }
> > -        if (vaapisink) {
> > -            gst_plugin_feature_set_rank(vaapisink, GST_RANK_NONE);
> > -            gst_object_unref(vaapisink);
> > -        }
> > -#endif
> > +        vaapisink = gst_registry_lookup_feature(gst_registry_get(),
> > "vaapisink");
> 
> It seems every time you call spice_check_gst_plugin_version you
> are going to use the plugin feature pointer, maybe
> spice_check_gst_plugin_version should return the pointer if
> successful.

Sure, i'll send a v2 series soon.

> Note that assuming that gst_registry_lookup_feature will return
> not NULL is racy and can lead to crashes, this would prevent
> this bug (although rare).

> > +        gst_plugin_feature_set_rank(vaapisink, GST_RANK_NONE);
> > +        g_clear_object(&vaapisink);
> > +    } else if (err != NULL) {
> > +        g_warning("Failure while checking vaapisink version: %s",
> > err->message);
> > +        g_clear_error(&err);
> >      }
> >  
> >      g_signal_connect(playbin, "deep-element-added",
> >      G_CALLBACK(deep_element_added_cb), decoder);
> 
> Frediano

Cheers,
-------------- 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/20190127/29a751de/attachment.sig>