[Buildroot] [PATCH 1/2] package/ffmpeg: add optional support for alsa-lib

[Buildroot] [PATCH 1/2] package/ffmpeg: add optional support for alsa-lib

[Bernd Kuhls]

ffmpeg has optional support for alsa as input and/or output device:
http://git.videolan.org/?p=ffmpeg.git;a=blob;f=configure;h=23823e3b7012d847b614bd43316fb614676bedb2;hb=refs/heads/release/3.3#l2987

Problem was found while fixing
http://autobuild.buildroot.net/results/7ba/7ba485532fcab74928246a8f95dba7e5eea9d4a5/

Signed-off-by: Bernd Kuhls <bernd.kuhls@t-online.de>
---
 package/ffmpeg/ffmpeg.mk | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/package/ffmpeg/ffmpeg.mk b/package/ffmpeg/ffmpeg.mk
index d69923b84..86bf5458c 100644
--- a/package/ffmpeg/ffmpeg.mk
+++ b/package/ffmpeg/ffmpeg.mk
@@ -157,12 +157,18 @@ endif
 
 ifeq ($(BR2_PACKAGE_FFMPEG_INDEVS),y)
 FFMPEG_CONF_OPTS += --enable-indevs
+ifeq ($(BR2_PACKAGE_ALSA_LIB),y)
+FFMPEG_DEPENDENCIES += alsa-lib
+endif
 else
 FFMPEG_CONF_OPTS += --disable-indevs
 endif
 
 ifeq ($(BR2_PACKAGE_FFMPEG_OUTDEVS),y)
 FFMPEG_CONF_OPTS += --enable-outdevs
+ifeq ($(BR2_PACKAGE_ALSA_LIB),y)
+FFMPEG_DEPENDENCIES += alsa-lib
+endif
 else
 FFMPEG_CONF_OPTS += --disable-outdevs
 endif
-- 
2.11.0

[Buildroot] [PATCH 2/2] package/ffmpeg: fix static linking with alsa

[Bernd Kuhls]

If BR2_PACKAGE_ALSA_LIB_RAWMIDI is enabled ffmpeg needs also
BR2_PACKAGE_ALSA_LIB_SEQ to link against alsa.

A similar patch was committed to alsa-utils:
https://git.buildroot.net/buildroot/commit/package/alsa-utils?id=c69088b8c35177cecdd0f1f385c13f5b2c509f1d

Fixes
http://autobuild.buildroot.net/results/7ba/7ba485532fcab74928246a8f95dba7e5eea9d4a5/

Signed-off-by: Bernd Kuhls <bernd.kuhls@t-online.de>
---
 package/ffmpeg/Config.in | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/package/ffmpeg/Config.in b/package/ffmpeg/Config.in
index 279f94f0d..da1a47e86 100644
--- a/package/ffmpeg/Config.in
+++ b/package/ffmpeg/Config.in
@@ -167,10 +167,12 @@ config BR2_PACKAGE_FFMPEG_FILTERS
 
 config BR2_PACKAGE_FFMPEG_INDEVS
 	bool "Enable input devices"
+	select BR2_PACKAGE_ALSA_LIB_SEQ if BR2_PACKAGE_ALSA_LIB_RAWMIDI
 	default y
 
 config BR2_PACKAGE_FFMPEG_OUTDEVS
 	bool "Enable output devices"
+	select BR2_PACKAGE_ALSA_LIB_SEQ if BR2_PACKAGE_ALSA_LIB_RAWMIDI
 	default y
 
 config BR2_PACKAGE_FFMPEG_EXTRACONF
-- 
2.11.0

[Buildroot] [PATCH 2/2] package/ffmpeg: fix static linking with alsa

[Thomas Petazzoni]

Hello,

On Thu,  1 Jun 2017 22:39:30 +0200, Bernd Kuhls wrote:
> If BR2_PACKAGE_ALSA_LIB_RAWMIDI is enabled ffmpeg needs also
> BR2_PACKAGE_ALSA_LIB_SEQ to link against alsa.
> 
> A similar patch was committed to alsa-utils:
> https://git.buildroot.net/buildroot/commit/package/alsa-utils?id=c69088b8c35177cecdd0f1f385c13f5b2c509f1d
> 
> Fixes
> http://autobuild.buildroot.net/results/7ba/7ba485532fcab74928246a8f95dba7e5eea9d4a5/
> 
> Signed-off-by: Bernd Kuhls <bernd.kuhls@t-online.de>

I think this is a bogus change, and that the commit
c69088b8c35177cecdd0f1f385c13f5b2c509f1d from Peter is equally bogus.

Indeed, src/rawmidi/Makefile.am contains:

librawmidi_la_SOURCES = rawmidi.c rawmidi_hw.c rawmidi_symbols.c
if BUILD_SEQ
librawmidi_la_SOURCES += rawmidi_virt.c
endif

So the rawmidi_virt.c code is not compiled in when --disable-seq is
passed. So, IMO, instead of forcing seq support, we should instead
adjust src/rawmidi/rawmidi_symbols.c as-is:

static const char **snd_rawmidi_open_objects[] = {
        &_snd_module_rawmidi_hw,
#ifdef BUILD_SEQ
        &_snd_module_rawmidi_virt
#endif
};

And I believe this patch would be upstreamable. Peter, what do you
think?

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[Buildroot] [PATCH 2/2] package/ffmpeg: fix static linking with alsa

[Peter Korsgaard]

>>>>> "Thomas" == Thomas Petazzoni <thomas.petazzoni@free-electrons.com> writes:

 > Hello,
 > On Thu,  1 Jun 2017 22:39:30 +0200, Bernd Kuhls wrote:
 >> If BR2_PACKAGE_ALSA_LIB_RAWMIDI is enabled ffmpeg needs also
 >> BR2_PACKAGE_ALSA_LIB_SEQ to link against alsa.
 >> 
 >> A similar patch was committed to alsa-utils:
 >> https://git.buildroot.net/buildroot/commit/package/alsa-utils?id=c69088b8c35177cecdd0f1f385c13f5b2c509f1d
 >> 
 >> Fixes
 >> http://autobuild.buildroot.net/results/7ba/7ba485532fcab74928246a8f95dba7e5eea9d4a5/
 >> 
 >> Signed-off-by: Bernd Kuhls <bernd.kuhls@t-online.de>

 > I think this is a bogus change, and that the commit
 > c69088b8c35177cecdd0f1f385c13f5b2c509f1d from Peter is equally bogus.

 > Indeed, src/rawmidi/Makefile.am contains:

 > librawmidi_la_SOURCES = rawmidi.c rawmidi_hw.c rawmidi_symbols.c
 > if BUILD_SEQ
 > librawmidi_la_SOURCES += rawmidi_virt.c
 > endif

 > So the rawmidi_virt.c code is not compiled in when --disable-seq is
 > passed. So, IMO, instead of forcing seq support, we should instead
 > adjust src/rawmidi/rawmidi_symbols.c as-is:

 > static const char **snd_rawmidi_open_objects[] = {
 >         &_snd_module_rawmidi_hw,
 > #ifdef BUILD_SEQ
 >         &_snd_module_rawmidi_virt
 > #endif
 > };

 > And I believe this patch would be upstreamable. Peter, what do you
 > think?

Calling it bogus is imho a bit harsh, but yeah - Getting it fixed
upstream so it works without seq would be nicer.

The patch was discussed and applied during the 2015.05 stablization
phase, and I chose to fix it by forcing SEQ on instead of patching the
code to work without it as that seemed the safest option. We've had a
number of issues with alsa-lib and disabled optional components in the
past (E.G. the logic we have to force PCM on):

http://lists.busybox.net/pipermail/buildroot/2015-May/128261.html

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH 2/2] package/ffmpeg: fix static linking with alsa

[Thomas Petazzoni]

Hello,

On Tue, 06 Jun 2017 14:27:44 +0200, Peter Korsgaard wrote:

>  > And I believe this patch would be upstreamable. Peter, what do you
>  > think?  
> 
> Calling it bogus is imho a bit harsh, but yeah - Getting it fixed
> upstream so it works without seq would be nicer.

I think it's bogus because the root of the problem is in alsa-lib, but
the proposed fix requires to touch potentially all users of alsa-lib.

So even if we decide not to patch alsa-lib, the proper fix would have
been:

config BR2_PACKAGE_ALSA_LIB_RAWMIDI
        bool "rawmidi"
+	BR2_PACKAGE_ALSA_LIB_SEQ
        default y

Because right now, the library produced by alsa-lib is broken, and this
is worked around in packages using alsa-lib rather than alsa-lib itself.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[Buildroot] [PATCH 2/2] package/ffmpeg: fix static linking with alsa

[Peter Korsgaard]

>>>>> "Thomas" == Thomas Petazzoni <thomas.petazzoni@free-electrons.com> writes:

Hi,

 >> Calling it bogus is imho a bit harsh, but yeah - Getting it fixed
 >> upstream so it works without seq would be nicer.

 > I think it's bogus because the root of the problem is in alsa-lib, but
 > the proposed fix requires to touch potentially all users of alsa-lib.

 > So even if we decide not to patch alsa-lib, the proper fix would have
 > been:

 > config BR2_PACKAGE_ALSA_LIB_RAWMIDI
 >         bool "rawmidi"
 > +	BR2_PACKAGE_ALSA_LIB_SEQ
 >         default y

 > Because right now, the library produced by alsa-lib is broken, and this
 > is worked around in packages using alsa-lib rather than alsa-lib itself.

True. Will you fix it or do you want me to send a patch?

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH 2/2] package/ffmpeg: fix static linking with alsa

[Thomas Petazzoni]

Hello,

On Tue, 06 Jun 2017 15:26:06 +0200, Peter Korsgaard wrote:

>  > Because right now, the library produced by alsa-lib is broken, and this
>  > is worked around in packages using alsa-lib rather than alsa-lib itself.  
> 
> True. Will you fix it or do you want me to send a patch?

Depends what fix we're talking about. Work around in
alsa-lib/Config.in, or proper fix in alsa-lib source code?

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[Buildroot] [PATCH 2/2] package/ffmpeg: fix static linking with alsa

[Peter Korsgaard]

>>>>> "Thomas" == Thomas Petazzoni <thomas.petazzoni@free-electrons.com> writes:

 > Hello,
 > On Tue, 06 Jun 2017 15:26:06 +0200, Peter Korsgaard wrote:

 >> > Because right now, the library produced by alsa-lib is broken, and this
 >> > is worked around in packages using alsa-lib rather than alsa-lib itself.  
 >> 
 >> True. Will you fix it or do you want me to send a patch?

 > Depends what fix we're talking about. Work around in
 > alsa-lib/Config.in, or proper fix in alsa-lib source code?

Either ;) The source code change is imho more of a nice-to-have thing as
nobody has brought it up in the ~two years we had the workaround, but if
you feel up to it..

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH 1/2] package/ffmpeg: add optional support for alsa-lib

[Thomas Petazzoni]

Hello,

On Thu,  1 Jun 2017 22:39:29 +0200, Bernd Kuhls wrote:
> ffmpeg has optional support for alsa as input and/or output device:
> http://git.videolan.org/?p=ffmpeg.git;a=blob;f=configure;h=23823e3b7012d847b614bd43316fb614676bedb2;hb=refs/heads/release/3.3#l2987
> 
> Problem was found while fixing
> http://autobuild.buildroot.net/results/7ba/7ba485532fcab74928246a8f95dba7e5eea9d4a5/
> 
> Signed-off-by: Bernd Kuhls <bernd.kuhls@t-online.de>
> ---
>  package/ffmpeg/ffmpeg.mk | 6 ++++++
>  1 file changed, 6 insertions(+)

Applied to master, thanks. To be honest, I wasn't sure it was necessary
to repeat the alsa-lib logic twice. Maybe having it outside any
conditional would be sufficient.

But OK, since it's only repeated two times, I guess it's OK, and more
precise to have it done the way you proposed, which is why I applied.

Thanks,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com