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

Mon Jan 7 12:42:01 UTC 2019

Hi

On Mon, Jan 7, 2019 at 3:44 PM Frediano Ziglio <fziglio at redhat.com> wrote:
>
> > 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
>
> It's not true, plugins are written because is important to interact, it's
> not a proof that one technology us solid, just that is used.

It has to be solid to be the core audio stack of Gnome & Fedora, and
replace pulseaudio.

> > - spice-gtk audio should use pipewire, and GStreamer backend is a good
> > to do that
>
> I don't understand what you mean here. Your patch is embedding gstreamer
> as a requirement removing the possibility to switch multimedia framework
> in the future (will require to undo part of your patches).
> The current code allow multiple implementation, I would like to keep this
> possibility. I'm not saying that we cannot require gstreamer, just that
> should not became a design requirement.

spice-gtk should use pipewire when available, and the GStreamer
backend is the way to to that.

There is no need to keep modularity if we have single backend.

It is fairly easy to bring back modularity when we have it again.

>
> >
> > Don't get me wrong, pipewire is not going to be a spice-gtk dependency.
> >
>
> Than what do you mean with "spice-gtk audio should use pipewire"?
> If it should use pipewire through gstreamer should be up to gstreamer
> to choose the best output.

yes

>
> > >
> > > 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.
> >
>
> Not saying to have a switch to disable, but is good to keep the
> possibility to switch implementation in the future.

Again, it's really not that big of a deal. Let's not have unnecessary
fake modularity for a potential future backend.

>
> > >
> > > > 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.
> >
>
> We can keep pulseaudio support. We have to consider also the various
> distro. RHEL 7, for instance is using gstreamer 1.10.

Yes, as discussed, we keep the pulse backend for a while and detect
broken GStreamer.

>
> > I propose to make GStreamer audio backend the default, and deprecate
> > the pulse backend.
> >
>
> "I'll work on a patch to remove pulse" is not deprecating, is removing it.
>
> > If the gst pulseaudio plugin is too old (<1.15), then fallback on
> > spice-gtk pulse backend for now.
> >
>
> So it's not clear if you want to remove or not.

In the future, yes! For now, use GStreamer by default if possible, and
fallback on pulse.

See also proposed implementation:
"[PATCH spice-gtk 32/34] gst: check pulseaudio plugin version >= 1.15"
"[PATCH spice-gtk 33/34] audio: use gstreamer by default"

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

-- 
Marc-André Lureau