Hi,
On Sun, Jan 27, 2019 at 06:25:18AM -0500, Frediano Ziglio wrote:
> >
> > From: Victor Toso <me@xxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
> > 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,
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel