Re: [Spice-devel] [client 07/11] build-sys: Allow simultaneous support for Pulse and GStreamer audio

[Christophe Fergeau]

On Sun, Nov 08, 2015 at 07:39:11PM +0100, Francois Gouget wrote:
> On Fri, 6 Nov 2015, Christophe Fergeau wrote:
> [...]
> > > -AC_ARG_WITH([audio],
> > > -  AS_HELP_STRING([--with-audio=@<:@gstreamer/pulse/auto/no@:>@], [Select 
> > > audio backend @<:@default=auto@:>@]),
> > > +AC_ARG_ENABLE([pulse],
> > > +  AS_HELP_STRING([--enable-pulse=@<:@yes/auto/no@:>@], [Enable the 
> > > PulseAudio backend @<:@default=auto@:>@]),
> > 
> > Imo it would be a less disruptive change if we changed '--with-audio=auto' 
> > to
> > enable both GStreamer and PulseAudio if the needed .pc files are
> > available. Removing --with-audio and replacing it with
> > --enable-pulseaudio/--enable-gstreamer means anyone using --with-audio
> > will need to update its build scripts.
> 
> The drawback of --with-audio=auto is that it makes it impossible to 
> require having support for both PulseAudio and GStreamer. That is unlike 
> './configure --enable-pulse --enable-gstaudio' it will not print an 
> error if one of them is not available.

Yup, good point.

> 
> (and something like --with-audio=pulse,gstreamer feels wrong and would 
> be very non standard)

--with-audio=all could work...

> 
> Would keeping --with-audio as a temporary frontend for the two enable 
> options be ok? It could print a warning to remind developers it's 
> deprecated?

...but this works for me too.

Christophe


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [client 07/11] build-sys: Allow simultaneous support for Pulse and GStreamer audio

[Francois Gouget]

On Fri, 6 Nov 2015, Christophe Fergeau wrote:
[...]
> > -AC_ARG_WITH([audio],
> > -  AS_HELP_STRING([--with-audio=@<:@gstreamer/pulse/auto/no@:>@], [Select 
> > audio backend @<:@default=auto@:>@]),
> > +AC_ARG_ENABLE([pulse],
> > +  AS_HELP_STRING([--enable-pulse=@<:@yes/auto/no@:>@], [Enable the 
> > PulseAudio backend @<:@default=auto@:>@]),
> 
> Imo it would be a less disruptive change if we changed '--with-audio=auto' to
> enable both GStreamer and PulseAudio if the needed .pc files are
> available. Removing --with-audio and replacing it with
> --enable-pulseaudio/--enable-gstreamer means anyone using --with-audio
> will need to update its build scripts.

The drawback of --with-audio=auto is that it makes it impossible to 
require having support for both PulseAudio and GStreamer. That is unlike 
'./configure --enable-pulse --enable-gstaudio' it will not print an 
error if one of them is not available.

(and something like --with-audio=pulse,gstreamer feels wrong and would 
be very non standard)

Would keeping --with-audio as a temporary frontend for the two enable 
options be ok? It could print a warning to remind developers it's 
deprecated?

-- 
Francois Gouget 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [client 07/11] build-sys: Allow simultaneous support for Pulse and GStreamer audio

[Christophe Fergeau]

On Thu, Nov 05, 2015 at 12:17:19AM +0100, Francois Gouget wrote:
> On Wed, 4 Nov 2015, Christophe Fergeau wrote:
> 
> > Can you explain in the commit log why this is a good thing/why you need
> > to do that?
> 
> Not sure how to explain it in the commit log.
> 
> It's essentially like the video decoder support: we don't force the 
> developer to choose between the builtin MJPEG decoder and the GStreamer 
> decoder. You can compile both in and one can serve as the fallback for 
> the other. I see no reason why it would be different for the audio 
> backend.
> 
> Of course, ideally all such cases should be able to pick the backend 
> from the command line, a configuration file, and through the GUI. Also 
> the PulseAudio / GStreamer libraries should be loaded dynamically to not 
> force bringing in dependencies users may not care about. But I think 
> that's best left for later.

Ok, then this could be summarized as "Rather than GStreamer/PulseAudio
backend being mutually exclusive at compile-time, this commit allows to
enable both at the same time. PulseAudio will then be favoured, with a
fallback to GStreamer if it's not available."
One benefit to this approach is that this would kill the occasional
complaint we get that people using distro packages stop getting audio
when they disabled pulseaudio because they don't like it.

> Another reason for the change is that the SPICE_CHECK_GSTREAMER() 
> autoconf macro I introduce in the following patch defines 'HAVE_XXX' 
> macros instead of the WITH_PULSEAUDIO / WITH_GST macros that were used.
> 
> I also think having 'WITH_XXX' C macros is wrong as this essentially 
> means the C code to depends on whether autoconf takes a --with-xxx or 
> --enable-xxx option. Any optional features should depend on HAVE_XXX 
> macros regardless of whether that's from a regular compatibility check, 
> an --enable-xxx option or a --with-xxx one.


These are good points (and I'll actually try to remember this rule of
thumb as I never now what to pick between WITH/ENABLE/HAVE when naming
autoconf variables :), but I don't think fixing the issues you describe
require allowing to enable both GStreamer and PulseAudio at the same
time ;)

Christophe


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [client 07/11] build-sys: Allow simultaneous support for Pulse and GStreamer audio

[Christophe Fergeau]

On Tue, Nov 03, 2015 at 01:06:10PM +0100, Francois Gouget wrote:
> Signed-off-by: Francois Gouget 
> ---
>  configure.ac  | 63 
> +--
>  src/Makefile.am   |  4 ++--
>  src/spice-audio.c | 11 +-
>  3 files changed, 37 insertions(+), 41 deletions(-)
> 
> This patch only depends on [client 07/11].
> 
> diff --git a/configure.ac b/configure.ac
> index dea9a30..ddaab2a 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -291,50 +291,44 @@ AS_IF([test "x$have_phodav" = "xyes"],
>  
>  AM_CONDITIONAL([WITH_PHODAV], [test "x$have_phodav" = "xyes"])
>  
> -AC_ARG_WITH([audio],
> -  AS_HELP_STRING([--with-audio=@<:@gstreamer/pulse/auto/no@:>@], [Select 
> audio backend @<:@default=auto@:>@]),
> +AC_ARG_ENABLE([pulse],
> +  AS_HELP_STRING([--enable-pulse=@<:@yes/auto/no@:>@], [Enable the 
> PulseAudio backend @<:@default=auto@:>@]),

Imo it would be a less disruptive change if we changed '--with-audio=auto' to
enable both GStreamer and PulseAudio if the needed .pc files are
available. Removing --with-audio and replacing it with
--enable-pulseaudio/--enable-gstreamer means anyone using --with-audio
will need to update its build scripts.

Christophe


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [client 07/11] build-sys: Allow simultaneous support for Pulse and GStreamer audio

[Francois Gouget]

On Wed, 4 Nov 2015, Christophe Fergeau wrote:

> Can you explain in the commit log why this is a good thing/why you need
> to do that?

Not sure how to explain it in the commit log.

It's essentially like the video decoder support: we don't force the 
developer to choose between the builtin MJPEG decoder and the GStreamer 
decoder. You can compile both in and one can serve as the fallback for 
the other. I see no reason why it would be different for the audio 
backend.

Of course, ideally all such cases should be able to pick the backend 
from the command line, a configuration file, and through the GUI. Also 
the PulseAudio / GStreamer libraries should be loaded dynamically to not 
force bringing in dependencies users may not care about. But I think 
that's best left for later.

Another reason for the change is that the SPICE_CHECK_GSTREAMER() 
autoconf macro I introduce in the following patch defines 'HAVE_XXX' 
macros instead of the WITH_PULSEAUDIO / WITH_GST macros that were used.

I also think having 'WITH_XXX' C macros is wrong as this essentially 
means the C code to depends on whether autoconf takes a --with-xxx or 
--enable-xxx option. Any optional features should depend on HAVE_XXX 
macros regardless of whether that's from a regular compatibility check, 
an --enable-xxx option or a --with-xxx one.


-- 
Francois Gouget 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [client 07/11] build-sys: Allow simultaneous support for Pulse and GStreamer audio

[Christophe Fergeau]

Can you explain in the commit log why this is a good thing/why you need
to do that?

Thanks,

Christophe

On Tue, Nov 03, 2015 at 01:06:10PM +0100, Francois Gouget wrote:
> Signed-off-by: Francois Gouget 
> ---
>  configure.ac  | 63 
> +--
>  src/Makefile.am   |  4 ++--
>  src/spice-audio.c | 11 +-
>  3 files changed, 37 insertions(+), 41 deletions(-)
> 
> This patch only depends on [client 07/11].
> 
> diff --git a/configure.ac b/configure.ac
> index dea9a30..ddaab2a 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -291,50 +291,44 @@ AS_IF([test "x$have_phodav" = "xyes"],
>  
>  AM_CONDITIONAL([WITH_PHODAV], [test "x$have_phodav" = "xyes"])
>  
> -AC_ARG_WITH([audio],
> -  AS_HELP_STRING([--with-audio=@<:@gstreamer/pulse/auto/no@:>@], [Select 
> audio backend @<:@default=auto@:>@]),
> +AC_ARG_ENABLE([pulse],
> +  AS_HELP_STRING([--enable-pulse=@<:@yes/auto/no@:>@], [Enable the 
> PulseAudio backend @<:@default=auto@:>@]),
>[],
> -  [with_audio="auto"])
> -
> -case "$with_audio" in
> -  gstreamer|pulse|auto*)
> -PKG_CHECK_MODULES(GSTAUDIO, gstreamer-1.0 gstreamer-base-1.0 
> gstreamer-app-1.0 gstreamer-audio-1.0, [have_gstaudio=yes], 
> [have_gstaudio=no])
> -PKG_CHECK_MODULES(PULSE, libpulse libpulse-mainloop-glib, 
> [have_pulse=yes], [have_pulse=no])
> -;;
> -  no*)
> -;;
> -  *) AC_MSG_ERROR(Unsupported audio backend)
> -esac
> -
> -AS_IF([test "x$with_audio" = "xauto" && test "x$have_pulse" = "xyes"],
> -  [with_audio=pulse])
> -
> -AS_IF([test "x$with_audio" = "xauto" && test "x$have_gstaudio" = "xyes"],
> -  [with_audio=gstreamer])
> -
> -AS_IF([test "x$with_audio" = "xauto"],
> -  [with_audio=no])
> -
> -AS_IF([test "x$with_audio" = "xpulse"],
> -  [AS_IF([test "x$have_pulse" = "xyes"],
> - [AC_DEFINE([WITH_PULSE], 1, [Have pulseaudio?])],
> - [AC_MSG_ERROR([PulseAudio requested but not found])
> +  [enable_pulse="auto"])
> +AS_IF([test "x$enable_pulse" != "xno"],
> +  [PKG_CHECK_MODULES(PULSE, [libpulse libpulse-mainloop-glib],
> + [AC_DEFINE([HAVE_PULSE], 1, [Have PulseAudio support?])
> +  enable_pulse="yes"],
> + [AS_IF([test "x$enable_pulse" = "xyes"],
> +AC_MSG_ERROR([PulseAudio requested but not found]))
> +  enable_pulse="no"
>])
>  ])
> -AM_CONDITIONAL([WITH_PULSE], [test "x$have_pulse" = "xyes"])
> +AM_CONDITIONAL([SUPPORT_PULSE], [test "x$enable_pulse" = "xyes"])
>  AC_SUBST(PULSE_CFLAGS)
>  AC_SUBST(PULSE_LIBS)
>  
> -AS_IF([test "x$with_audio" = "xgstreamer"],
> -  [AS_IF([test "x$have_gstaudio" = "xyes"],
> - [AC_DEFINE([WITH_GSTAUDIO], 1, [Have GStreamer 1.0 audio?])],
> - [AC_MSG_ERROR([GStreamer 1.0 audio requested but not found])
> +AC_ARG_ENABLE([gstaudio],
> +  AS_HELP_STRING([--enable-gstaudio=@<:@yes/auto/no@:>@], [Enable the 
> GStreamer 1.0 audio backend @<:@default=auto@:>@]),
> +  [],
> +  [enable_gstaudio="auto"])
> +AS_IF([test "x$enable_gstaudio" != "xno"],
> +  [PKG_CHECK_MODULES(GSTAUDIO, [gstreamer-1.0 gstreamer-base-1.0 
> gstreamer-app-1.0 gstreamer-audio-1.0],
> + [AC_DEFINE([HAVE_GSTAUDIO], 1, [Have GStreamer 1.0 audio support?])
> +  enable_gstaudio="yes"],
> + [AS_IF([test "x$enable_gstaudio" = "xyes"],
> +AC_MSG_ERROR([GStreamer 1.0 audio requested but not found]))
> +  enable_gstaudio="no"
>])
>  ])
> -AM_CONDITIONAL([WITH_GSTAUDIO], [test "x$have_gstaudio" = "xyes"])
> +AM_CONDITIONAL([SUPPORT_GSTAUDIO], [test "x$enable_gstaudio" = "xyes"])
>  AC_SUBST(GSTAUDIO_CFLAGS)
>  AC_SUBST(GSTAUDIO_LIBS)
>  
> +AS_IF([test "x$enable_pulse$enable_gstaudio" = "xnono"],
> +  [SPICE_WARNING([No PulseAudio or GStreamer 1.0 audio decoder, audio 
> will not be streamed])
> +])
> +
>  AC_CHECK_LIB(jpeg, jpeg_destroy_decompress,
>  AC_MSG_CHECKING([for jpeglib.h])
>  AC_TRY_CPP(
> @@ -741,7 +735,8 @@ AC_MSG_NOTICE([
>  
>  Gtk:  ${with_gtk}
>  Coroutine:${with_coroutine}
> -Audio:${with_audio}
> +PulseAudio:   ${enable_pulse}
> +GStreamer Audio:  ${enable_gstaudio}
>  SASL support: ${enable_sasl}
>  Smartcard support:${have_smartcard}
>  USB redirection support:  ${have_usbredir} ${with_usbredir_hotplug}
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 99a8a1f..af95562 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -330,14 +330,14 @@ dist_libspice_client_glibinclude_DATA = \
>   spice-channel-enums.h   \
>   $(NULL)
>  
> -if WITH_PULSE
> +if SUPPORT_PULSE
>  libspice_client_glib_2_0_la_SOURCES +=   \
>   spice-pulse.c   \
>   spice-pulse.h   \
>   $(NULL)
>  endif
>  
> -if WITH_GSTAUDIO
> +if SUPPORT_GSTAUDIO
>  

[Spice-devel] [client 07/11] build-sys: Allow simultaneous support for Pulse and GStreamer audio

[Francois Gouget]

Signed-off-by: Francois Gouget 
---
 configure.ac  | 63 +--
 src/Makefile.am   |  4 ++--
 src/spice-audio.c | 11 +-
 3 files changed, 37 insertions(+), 41 deletions(-)

This patch only depends on [client 07/11].

diff --git a/configure.ac b/configure.ac
index dea9a30..ddaab2a 100644
--- a/configure.ac
+++ b/configure.ac
@@ -291,50 +291,44 @@ AS_IF([test "x$have_phodav" = "xyes"],
 
 AM_CONDITIONAL([WITH_PHODAV], [test "x$have_phodav" = "xyes"])
 
-AC_ARG_WITH([audio],
-  AS_HELP_STRING([--with-audio=@<:@gstreamer/pulse/auto/no@:>@], [Select audio 
backend @<:@default=auto@:>@]),
+AC_ARG_ENABLE([pulse],
+  AS_HELP_STRING([--enable-pulse=@<:@yes/auto/no@:>@], [Enable the PulseAudio 
backend @<:@default=auto@:>@]),
   [],
-  [with_audio="auto"])
-
-case "$with_audio" in
-  gstreamer|pulse|auto*)
-PKG_CHECK_MODULES(GSTAUDIO, gstreamer-1.0 gstreamer-base-1.0 
gstreamer-app-1.0 gstreamer-audio-1.0, [have_gstaudio=yes], [have_gstaudio=no])
-PKG_CHECK_MODULES(PULSE, libpulse libpulse-mainloop-glib, 
[have_pulse=yes], [have_pulse=no])
-;;
-  no*)
-;;
-  *) AC_MSG_ERROR(Unsupported audio backend)
-esac
-
-AS_IF([test "x$with_audio" = "xauto" && test "x$have_pulse" = "xyes"],
-  [with_audio=pulse])
-
-AS_IF([test "x$with_audio" = "xauto" && test "x$have_gstaudio" = "xyes"],
-  [with_audio=gstreamer])
-
-AS_IF([test "x$with_audio" = "xauto"],
-  [with_audio=no])
-
-AS_IF([test "x$with_audio" = "xpulse"],
-  [AS_IF([test "x$have_pulse" = "xyes"],
- [AC_DEFINE([WITH_PULSE], 1, [Have pulseaudio?])],
- [AC_MSG_ERROR([PulseAudio requested but not found])
+  [enable_pulse="auto"])
+AS_IF([test "x$enable_pulse" != "xno"],
+  [PKG_CHECK_MODULES(PULSE, [libpulse libpulse-mainloop-glib],
+ [AC_DEFINE([HAVE_PULSE], 1, [Have PulseAudio support?])
+  enable_pulse="yes"],
+ [AS_IF([test "x$enable_pulse" = "xyes"],
+AC_MSG_ERROR([PulseAudio requested but not found]))
+  enable_pulse="no"
   ])
 ])
-AM_CONDITIONAL([WITH_PULSE], [test "x$have_pulse" = "xyes"])
+AM_CONDITIONAL([SUPPORT_PULSE], [test "x$enable_pulse" = "xyes"])
 AC_SUBST(PULSE_CFLAGS)
 AC_SUBST(PULSE_LIBS)
 
-AS_IF([test "x$with_audio" = "xgstreamer"],
-  [AS_IF([test "x$have_gstaudio" = "xyes"],
- [AC_DEFINE([WITH_GSTAUDIO], 1, [Have GStreamer 1.0 audio?])],
- [AC_MSG_ERROR([GStreamer 1.0 audio requested but not found])
+AC_ARG_ENABLE([gstaudio],
+  AS_HELP_STRING([--enable-gstaudio=@<:@yes/auto/no@:>@], [Enable the 
GStreamer 1.0 audio backend @<:@default=auto@:>@]),
+  [],
+  [enable_gstaudio="auto"])
+AS_IF([test "x$enable_gstaudio" != "xno"],
+  [PKG_CHECK_MODULES(GSTAUDIO, [gstreamer-1.0 gstreamer-base-1.0 
gstreamer-app-1.0 gstreamer-audio-1.0],
+ [AC_DEFINE([HAVE_GSTAUDIO], 1, [Have GStreamer 1.0 audio support?])
+  enable_gstaudio="yes"],
+ [AS_IF([test "x$enable_gstaudio" = "xyes"],
+AC_MSG_ERROR([GStreamer 1.0 audio requested but not found]))
+  enable_gstaudio="no"
   ])
 ])
-AM_CONDITIONAL([WITH_GSTAUDIO], [test "x$have_gstaudio" = "xyes"])
+AM_CONDITIONAL([SUPPORT_GSTAUDIO], [test "x$enable_gstaudio" = "xyes"])
 AC_SUBST(GSTAUDIO_CFLAGS)
 AC_SUBST(GSTAUDIO_LIBS)
 
+AS_IF([test "x$enable_pulse$enable_gstaudio" = "xnono"],
+  [SPICE_WARNING([No PulseAudio or GStreamer 1.0 audio decoder, audio will 
not be streamed])
+])
+
 AC_CHECK_LIB(jpeg, jpeg_destroy_decompress,
 AC_MSG_CHECKING([for jpeglib.h])
 AC_TRY_CPP(
@@ -741,7 +735,8 @@ AC_MSG_NOTICE([
 
 Gtk:  ${with_gtk}
 Coroutine:${with_coroutine}
-Audio:${with_audio}
+PulseAudio:   ${enable_pulse}
+GStreamer Audio:  ${enable_gstaudio}
 SASL support: ${enable_sasl}
 Smartcard support:${have_smartcard}
 USB redirection support:  ${have_usbredir} ${with_usbredir_hotplug}
diff --git a/src/Makefile.am b/src/Makefile.am
index 99a8a1f..af95562 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -330,14 +330,14 @@ dist_libspice_client_glibinclude_DATA =   \
spice-channel-enums.h   \
$(NULL)
 
-if WITH_PULSE
+if SUPPORT_PULSE
 libspice_client_glib_2_0_la_SOURCES += \
spice-pulse.c   \
spice-pulse.h   \
$(NULL)
 endif
 
-if WITH_GSTAUDIO
+if SUPPORT_GSTAUDIO
 libspice_client_glib_2_0_la_SOURCES += \
spice-gstaudio.c\
spice-gstaudio.h\
diff --git a/src/spice-audio.c b/src/spice-audio.c
index 75742d7..550d02a 100644
--- a/src/spice-audio.c
+++ b/src/spice-audio.c
@@ -42,10 +42,10 @@
 #include "spice-channel-priv.h"
 #include "spice-audio-priv.h"
 
-#ifdef WITH_PULSE
+#ifdef HAVE_PULSE
 #include "spice-pulse.h"
 #endif
-#if