[Spice-devel] [PATCH spice-gtk 0/5] Require GStreamer, fix build warnings

[Marc-André Lureau]

Hi

On Sat, Jan 5, 2019 at 9:00 PM Frediano Ziglio <fziglio at redhat.com> wrote:
>
> >
> > Hi
> >
> > On Sat, Jan 5, 2019 at 1:59 PM Victor Toso <victortoso at redhat.com> wrote:
> > >
> > > Hi,
> > >
> > > On Fri, Jan 04, 2019 at 04:48:21PM +0400, marcandre.lureau at redhat.com
> > > wrote:
> > > > From: Marc-André Lureau <marcandre.lureau at redhat.com>
> > > >
> > > > Hi,
> > > >
> > > > The series drops support for optional GStreamer dependency. Spice
> > > > increasingly require GStreamer for audio and video support. GStreamer
> > > > is widely available, including in embedded space.
> > >
> > > I think this is fine indeed, not 100% sure if we shouldn't do a
> > > last release without this first. Another question is about
> >
> > As long as we don't require a too recent GStreamer (only 1.0
> > apparently, the rest is checked with GST_CHECK_VERSION), I think it
> > should be fine.
> >
> > > pulseaudio backend. If gstreamer is always enabled, I don't see
> > > why we need pulseaudio..
> >
> > Good point, I thought the the gst backend was inferior to the pulse
> > backend wrt volume handling, but you pointed out in commit message
> > 808ac1d9b3fd926f660231776d5c66946fda5664 that gst now implements it
> > for various elements.
> >
> > And with pipewire plan to replace pulseaudio (and using the gstreamer
> > elements), I think this is the path forward.
> >
>
> Is pipewire stable and cross platform? From the website I cannot find these
> information. Before removing some support we should make sure we have a good
> replacement.

There are several reasons I mentionned pipewire:
- it uses the gstreamer audio elements, which means they have to be quite solid
- spice-gtk audio should use pipewire, and GStreamer backend is a good
to do that

Don't get me wrong, pipewire is not going to be a spice-gtk dependency.

>
> Honestly I'm not really fun of needing GStreamer, if any platform have
> a different streaming library I'd like to be able to use it, making
> GStreamer a need is going the other direction.

As long as we support only GStreamer for audio/video, I don't see the
point in having a switch to disable it.

>
> > I'll work on a patch to remove pulse.
> >
>
> It seems too soon.

yes, we haven't done enough testing of the gstreamer backend on Linux.
The recording path at least fails very often for me due to a race:
https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/merge_requests/69

After fixing the pulsesrc element, I added a check in spice-gtk for
pulseaudio plugin version. I don't see what else I could do.

I propose to make GStreamer audio backend the default, and deprecate
the pulse backend.

If the gst pulseaudio plugin is too old (<1.15), then fallback on
spice-gtk pulse backend for now.

>
> > >
> > > Thanks,
> > >
> > > > Marc-André Lureau (5):
> > > >   build-sys: remove autoconf --with-audio=..
> > > >   build-sys: drop gstaudio option, make GStreamer a requirement
> > > >   build-sys: drop gstvideo option, make it required
> > > >   widget: gst_size_allocate() is static
> > > >   build-sys: fix gir/vapi warnings with GstPipeline
> > > >
> > > >  .gitlab-ci.yml                  |  4 --
> > > >  configure.ac                    | 85 ++++++++++-----------------------
> > > >  meson.build                     | 33 ++-----------
> > > >  meson_options.txt               | 10 ----
> > > >  src/Makefile.am                 | 18 ++-----
> > > >  src/channel-display-priv.h      | 10 +---
> > > >  src/channel-display.c           |  7 ---
> > > >  src/meson.build                 | 14 ++----
> > > >  src/spice-audio.c               |  4 --
> > > >  src/spice-widget-priv.h         |  4 --
> > > >  src/spice-widget.c              | 14 ++----
> > > >  tools/spicy.c                   |  8 ----
> > > >  vapi/Makefile.am                |  2 +
> > > >  vapi/meson.build                |  4 +-
> > > >  vapi/spice-client-glib-2.0.deps |  1 +
> > > >  15 files changed, 47 insertions(+), 171 deletions(-)
> > > >
>
> Frediano



-- 
Marc-André Lureau