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

Tue Jan 8 13:25:09 UTC 2019

> 
> Hi
> 
> On Mon, Jan 7, 2019 at 5:46 PM Frediano Ziglio <fziglio at redhat.com> wrote:
> >
> > > 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.
> > >
> >
> > It seems you are speaking of the future using a past tense. I opened
> > some programs under Fedora 29 and they are using pulseaudio or alsa.
> >
> > > > > - 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.
> > >
> >
> > Agree, or any other framework suitable.
> >
> > > There is no need to keep modularity if we have single backend.
> > >
> >
> > But as we already have it I don't see the reason to remove it.
> >
> > > It is fairly easy to bring back modularity when we have it again.
> > >
> >
> > Yes, at the present, but I would like to keep it keeping the
> > modularity.
> >
> > > >
> > > > >
> > > > > 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 disagree. You are already talking about different 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"
> 
> 
> We have only GStreamer support for video, so let's make it required.

There's also mjpeg for video.

> There is no modularity needed at this point, it can be added when
> needed (although I would rather have less modularity in general,
> because of testing matrix, maintainance..).
> 

Modularity is already present and you are removing it.
Usually modularity is used to reduce test matrix making sure all
implementations of a given interface behave as expected.

> We would like to remove PulseAudio audio backend in the future, as it
> will be replaced by pipewire, and unnecessarily make us maintain
> different audio backends: GStreamer handles various audio backends for
> us, and we know it is quite solid since it is being used with the
> Windows build for a long while.
> 
> Since we would like to release v0.36 soonish, and there are valid
> concerns that such a change now is a bit too risky, I propose to:
> 
> - make GStreamer a blessed dependency in 0.36 for audio & video (since
> it is required for video decoding/overlay etc)
> - keep pulse audio backend as default, but add a warning at compile
> time that it will be deprecated in future releases
> - make GStreamer audio backend lower the rank of pulsesrc < 1.15 so
> alsa or other backends are chosen with higher priority
> - after 0.36, make GStreamer audo backend the default, and disable the
> pulse backend (unless explicetly --enable-pulse).
> - later on, remove the pulse audio backend
> 
> Any comments?
> 

It sounds fine.
I'm noting however that this is the forth version in less then a week,
hope you are not changing your mind again tomorrow.

Frediano