[Spice-devel] [PATCH spice-gtk v3] gst: check pulsesrc version >= 1.14.5

[Frediano Ziglio]

> 
> Hi
> 
> On Wed, Jan 16, 2019 at 3:38 PM Frediano Ziglio <fziglio at redhat.com> wrote:
> >
> > >
> > > Hi
> > >
> > > On Wed, Jan 16, 2019 at 3:14 PM Frediano Ziglio <fziglio at redhat.com>
> > > wrote:
> > > >
> > > > >
> > > > > Hi,
> > > > >
> > > > > On Wed, Jan 16, 2019 at 12:43:06PM +0400, Marc-André Lureau wrote:
> > > > > > Hi
> > > > > >
> > > > > >
> > > > > > On Fri, Jan 11, 2019 at 3:03 PM <marcandre.lureau at redhat.com>
> > > > > > wrote:
> > > > > > >
> > > > > > > From: Marc-André Lureau <marcandre.lureau at redhat.com>
> > > > > > >
> > > > > > > There is a racy bug in pulsesrc that we can't easily workaround:
> > > > > > > https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/merge_requests/69
> > > > > > >
> > > > > > > It is fixed with 1.15, and will be backported to upcoming 1.14.5.
> > > > > > >
> > > > > > > PulseAudio may not be picked by autoaudiosrc, but looking up the
> > > > > > > actual source or mimicking GstAutoDetect is unnecessarily
> > > > > > > complicated.
> > > > > > >
> > > > > > > When pulsesrc < 1.14.5 is detected, let's drop its rank, so it
> > > > > > > won't
> > > > > > > be
> > > > > > > picked.
> > > > > > >
> > > > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau at redhat.com>
> > > > > >
> > > > > > I think we agreed that this is the way to go.
> > > > > >
> > > > > > ack for v0.36, or should we continue the discussion for v0.37?
> > > > >
> > > > > ack for v0.36
> > > > >
> > > > > >
> > > > > > >  src/spice-gstaudio.c | 28 ++++++++++++++++++++++++++++
> > > > > > >  1 file changed, 28 insertions(+)
> > > > > > >
> > > > > > > diff --git a/src/spice-gstaudio.c b/src/spice-gstaudio.c
> > > > > > > index d0cfbc6..49e9ae6 100644
> > > > > > > --- a/src/spice-gstaudio.c
> > > > > > > +++ b/src/spice-gstaudio.c
> > > > > > > @@ -527,7 +527,35 @@ SpiceGstaudio
> > > > > > > *spice_gstaudio_new(SpiceSession
> > > > > > > *session, GMainContext *context,
> > > > > > >                                    const char *name)
> > > > > > >  {
> > > > > > >      GError *err = NULL;
> > > > > > > +
> > > > > > >      if (gst_init_check(NULL, NULL, &err)) {
> > > > > > > +        GstPluginFeature *pulsesrc;
> > > > > > > +
> > > > > > > +        pulsesrc =
> > > > > > > gst_registry_lookup_feature(gst_registry_get(),
> > > > > > > "pulsesrc");
> > > > > > > +        if (pulsesrc) {
> > > > > > > +            unsigned major, minor, micro;
> > > > > > > +            GstPlugin *plugin =
> > > > > > > gst_plugin_feature_get_plugin(pulsesrc);
> > > > > > > +
> > > > > > > +            if (sscanf(gst_plugin_get_version(plugin),
> > > > > > > "%u.%u.%u",
> > > > > > > +                       &major, &minor, &micro) != 3) {
> >
> > I think this test will fail with versions like 1.15 too.
> > Maybe a
> >
> >     unsigned major, minor = 0, micro = 0;
> >
> >     if (sscanf(gst_plugin_get_version(plugin), "%u.%u.%u", &major, &minor,
> >     &micro) < 2) {
> >         ....
> 
> GStreamer versions are always at least 3 digits afaik. Other versions,
> we can just ignore.
> 

Not much documentation at
https://gstreamer.freedesktop.org/data/doc/gstreamer/head/gstreamer/html/GstPlugin.html#GST-PLUGIN-DEFINE:CAPS

The current code is not actually ignoring, it would return NULL so disabling
GstAudio.

> >
> > OT: is it worth this code on Windows ?
> 
> PA used to build on windows. I don't know today.
> 
> >
> > > > > > > +                g_warn_if_reached();
> > > > > > > +                gst_object_unref(plugin);
> > > > > > > +                gst_object_unref(pulsesrc);
> > > > > > > +                return NULL;
> > > > > > > +            }
> > > > > > > +
> > > > > > > +            if (major < 1 ||
> > > > > > > +                (major == 1 && minor < 14) ||
> > > > > > > +                (major == 1 && minor == 14 && micro < 5)) {
> > > > > > > +                g_warning("Bad pulsesrc version %s, lowering its
> > > > > > > rank",
> > > > > > > +                          gst_plugin_get_version(plugin));
> > > > > > > +                gst_plugin_feature_set_rank(pulsesrc,
> > > > > > > GST_RANK_NONE);
> > > > > > > +            }
> > > > > > > +
> > > > > > > +            gst_object_unref(plugin);
> > > > > > > +            gst_object_unref(pulsesrc);
> > > > > > > +        }
> > > > > > > +
> > > > > > >          return g_object_new(SPICE_TYPE_GSTAUDIO,
> > > > > > >                              "session", session,
> > > > > > >                              "main-context", context,
> > > >
> > > > Looks like the version merged in master is quite different from this
> > > > version,
> > > > in particular:
> > > >
> > > >             if (maj < 1 || min < 15) {
> > > >                 g_warning("Bad pulsesrc version, lowering its rank");
> > > >                 gst_plugin_feature_set_rank(pulsesrc, GST_RANK_NONE);
> > > >             }
> > > >
> > > > will lower the rank even if the version is 2.0 which probably is higher
> > > > than 1.15.
> > > >
> > > > By the way, it looks like you pushed the wrong patch, like this old one
> > > > https://lists.freedesktop.org/archives/spice-devel/2019-January/047138.html
> > > > which was not acked.
> > >
> > > yes, wrong version, my bad!
> > >
> > > Thanks for noticing!!
> > >
> >
> > Maybe more clean to revert the pushed one and merge an updated one?
> >
> > Frediano
> 
> 
> 
> --
> Marc-André Lureau
>