[Spice-devel,client,1/3] build-sys: Allow simultaneous support for Pulse and GStreamer audio

Submitted by Francois Gouget on Dec. 2, 2015, 1:54 p.m.

Details

Message ID eefa537d9d97f964812bf5834f0e6bd24acb9df2.1449064327.git.fgouget@free.fr
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Francois Gouget Dec. 2, 2015, 1:54 p.m.
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.
Note that --with-audio is kept for backward compatibility.

Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
---
 configure.ac      | 71 +++++++++++++++++++++++++++++--------------------------
 src/Makefile.am   |  4 ++--
 src/spice-audio.c | 11 +++++----
 3 files changed, 46 insertions(+), 40 deletions(-)

Patch hide | download patch | download mbox

diff --git a/configure.ac b/configure.ac
index dea9a30..734b73c 100644
--- a/configure.ac
+++ b/configure.ac
@@ -292,49 +292,53 @@  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@:>@]),
-  [],
-  [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_HELP_STRING([--with-audio=@<:@gstreamer/pulse/auto/no@:>@], [For legacy compatibility only]),
+  [SPICE_WARNING([--with-audio is deprecated. Use --enable-pulse and/or --enable-gstaudio instead])
+   case "$with_audio" in
+   pulse)     enable_pulse="yes"; enable_gstaudio="no" ;;
+   gstreamer) enable_pulse="no";  enable_gstaudio="yes" ;;
+   no)        enable_pulse="no";  enable_gstaudio="no" ;;
+   esac
+])
 
-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])
+AC_ARG_ENABLE([pulse],
+  AS_HELP_STRING([--enable-pulse=@<:@yes/auto/no@:>@], [Enable the PulseAudio backend @<:@default=auto@:>@]),
+  [],
+  [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 +745,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 defined(WITH_GSTAUDIO)
+#ifdef HAVE_GST_AUDIO
 #include "spice-gstaudio.h"
 #endif
 
@@ -261,11 +261,12 @@  SpiceAudio *spice_audio_new(SpiceSession *session, GMainContext *context,
     if (name == NULL)
         name = g_get_application_name();
 
-#ifdef WITH_PULSE
+#ifdef HAVE_PULSE
     self = SPICE_AUDIO(spice_pulse_new(session, context, name));
 #endif
-#if defined(WITH_GSTAUDIO)
-    self = SPICE_AUDIO(spice_gstaudio_new(session, context, name));
+#ifdef HAVE_GST_AUDIO
+    if (!self)
+        self = SPICE_AUDIO(spice_gstaudio_new(session, context, name));
 #endif
     if (!self)
         return NULL;

Comments

Hey,

On Wed, Dec 02, 2015 at 02:54:55PM +0100, Francois Gouget wrote:
>  ])
> -AM_CONDITIONAL([WITH_PULSE], [test "x$have_pulse" = "xyes"])
> +AM_CONDITIONAL([SUPPORT_PULSE], [test "x$enable_pulse" = "xyes"])

I"d prefer HAVE_PULSE rather than SUPPORT_PULSE to be consistent with
the naming used in spice-deps.m4 (I've changed this locally if that's ok
with you, no need to send an updated version if the change is fine with
you).

ACK apart from this.

Christophe
On Thu, 3 Dec 2015, Christophe Fergeau wrote:

> Hey,
> 
> On Wed, Dec 02, 2015 at 02:54:55PM +0100, Francois Gouget wrote:
> >  ])
> > -AM_CONDITIONAL([WITH_PULSE], [test "x$have_pulse" = "xyes"])
> > +AM_CONDITIONAL([SUPPORT_PULSE], [test "x$enable_pulse" = "xyes"])
> 
> I"d prefer HAVE_PULSE rather than SUPPORT_PULSE to be consistent with
> the naming used in spice-deps.m4 (I've changed this locally if that's ok
> with you, no need to send an updated version if the change is fine with
> you).
> 
> ACK apart from this.

Sure. That works for me.
On Thu, Dec 03, 2015 at 05:44:11PM +0100, Francois Gouget wrote:
> On Thu, 3 Dec 2015, Christophe Fergeau wrote:
> 
> > Hey,
> > 
> > On Wed, Dec 02, 2015 at 02:54:55PM +0100, Francois Gouget wrote:
> > >  ])
> > > -AM_CONDITIONAL([WITH_PULSE], [test "x$have_pulse" = "xyes"])
> > > +AM_CONDITIONAL([SUPPORT_PULSE], [test "x$enable_pulse" = "xyes"])
> > 
> > I"d prefer HAVE_PULSE rather than SUPPORT_PULSE to be consistent with
> > the naming used in spice-deps.m4 (I've changed this locally if that's ok
> > with you, no need to send an updated version if the change is fine with
> > you).
> > 
> > ACK apart from this.
> 
> Sure. That works for me.

Ok, I've pushed the 3 patches now.

Christophe
Hi,

On Wed, Dec 02, 2015 at 02:54:55PM +0100, Francois Gouget wrote:
> 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.
> Note that --with-audio is kept for backward compatibility.

Does --with-audio=gstreamer work with you? It doesn't work here.
arrrg. bad day on building, this is.

IMHO, it is safe to completly remove --with-audio but error when used.

cheers,

>
> Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
> ---
>  configure.ac      | 71 +++++++++++++++++++++++++++++--------------------------
>  src/Makefile.am   |  4 ++--
>  src/spice-audio.c | 11 +++++----
>  3 files changed, 46 insertions(+), 40 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index dea9a30..734b73c 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -292,49 +292,53 @@ 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@:>@]),
> -  [],
> -  [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_HELP_STRING([--with-audio=@<:@gstreamer/pulse/auto/no@:>@], [For legacy compatibility only]),
> +  [SPICE_WARNING([--with-audio is deprecated. Use --enable-pulse and/or --enable-gstaudio instead])
> +   case "$with_audio" in
> +   pulse)     enable_pulse="yes"; enable_gstaudio="no" ;;
> +   gstreamer) enable_pulse="no";  enable_gstaudio="yes" ;;
> +   no)        enable_pulse="no";  enable_gstaudio="no" ;;
> +   esac
> +])
>  
> -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])
> +AC_ARG_ENABLE([pulse],
> +  AS_HELP_STRING([--enable-pulse=@<:@yes/auto/no@:>@], [Enable the PulseAudio backend @<:@default=auto@:>@]),
> +  [],
> +  [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 +745,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 defined(WITH_GSTAUDIO)
> +#ifdef HAVE_GST_AUDIO
>  #include "spice-gstaudio.h"
>  #endif
>  
> @@ -261,11 +261,12 @@ SpiceAudio *spice_audio_new(SpiceSession *session, GMainContext *context,
>      if (name == NULL)
>          name = g_get_application_name();
>  
> -#ifdef WITH_PULSE
> +#ifdef HAVE_PULSE
>      self = SPICE_AUDIO(spice_pulse_new(session, context, name));
>  #endif
> -#if defined(WITH_GSTAUDIO)
> -    self = SPICE_AUDIO(spice_gstaudio_new(session, context, name));
> +#ifdef HAVE_GST_AUDIO
> +    if (!self)
> +        self = SPICE_AUDIO(spice_gstaudio_new(session, context, name));
>  #endif
>      if (!self)
>          return NULL;
> -- 
> 2.6.2
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
On Wed, Dec 09, 2015 at 10:10:51AM +0100, Victor Toso wrote:
> Hi,
> 
> On Wed, Dec 02, 2015 at 02:54:55PM +0100, Francois Gouget wrote:
> > 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.
> > Note that --with-audio is kept for backward compatibility.
> 
> Does --with-audio=gstreamer work with you? It doesn't work here.
> arrrg. bad day on building, this is.

Any more details on that? Seems to be working here (configure reports
it's going to build with gstreamer support, and build succeeds).

> IMHO, it is safe to completly remove --with-audio but error when used.

The initial patch was removing it (but without erroring out I think). I
prefer that we have a transitional phase to leave a bit of time to
distros to adjust to the new option names.

Christophe
Hi,

On Wed, Dec 09, 2015 at 11:48:11AM +0100, Christophe Fergeau wrote:
> On Wed, Dec 09, 2015 at 10:10:51AM +0100, Victor Toso wrote:
> > Hi,
> >
> > On Wed, Dec 02, 2015 at 02:54:55PM +0100, Francois Gouget wrote:
> > > 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.
> > > Note that --with-audio is kept for backward compatibility.
> >
> > Does --with-audio=gstreamer work with you? It doesn't work here.
> > arrrg. bad day on building, this is.
>
> Any more details on that? Seems to be working here (configure reports
> it's going to build with gstreamer support, and build succeeds).

Sure. Exactly that! It says that it will build with gstreamer but VM is
without audio. I tried without the patches and it worked.

Sorry, I did not look into them to see what is going on but I can do it
later on.

>
> > IMHO, it is safe to completly remove --with-audio but error when used.
>
> The initial patch was removing it (but without erroring out I think). I
> prefer that we have a transitional phase to leave a bit of time to
> distros to adjust to the new option names.
>
> Christophe

Sure! :)
On Wed, Dec 09, 2015 at 12:00:25PM +0100, Victor Toso wrote:
> Hi,
> 
> On Wed, Dec 09, 2015 at 11:48:11AM +0100, Christophe Fergeau wrote:
> > On Wed, Dec 09, 2015 at 10:10:51AM +0100, Victor Toso wrote:
> > > Hi,
> > >
> > > On Wed, Dec 02, 2015 at 02:54:55PM +0100, Francois Gouget wrote:
> > > > 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.
> > > > Note that --with-audio is kept for backward compatibility.
> > >
> > > Does --with-audio=gstreamer work with you? It doesn't work here.
> > > arrrg. bad day on building, this is.
> >
> > Any more details on that? Seems to be working here (configure reports
> > it's going to build with gstreamer support, and build succeeds).
> 
> Sure. Exactly that! It says that it will build with gstreamer but VM is
> without audio. I tried without the patches and it worked.
> 
> Sorry, I did not look into them to see what is going on but I can do it
> later on.

Just sent a patch for it.

Christophe