| Message ID | 20170601203930.1069-2-bernd.kuhls@t-online.de |
|---|---|
| State | Changes Requested |
| Headers | show |
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" == 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
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" == 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?
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" == 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..
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
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(+)