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

[Frediano Ziglio]

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

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