Bug 307343

Summary:	audiocd-kio 4.9.1 does not supply vorbis support
Product:	[Unmaintained] kdemultimedia	Reporter:	Felix Tiede <kde>
Component:	general	Assignee:	Multimedia Developers <kde-multimedia>
Status:	RESOLVED FIXED
Severity:	major	CC:	mpyne
Priority:	NOR
Version:	unspecified
Target Milestone:	---
Platform:	Gentoo Packages
OS:	Linux
Latest Commit:	http://commits.kde.org/audiocd-kio/c2baf636be4cb17400669b989d0215307c4ce1ec	Version Fixed In:	4.9.2
Sentry Crash Report:
Attachments:	Fix audiocd-kio plugins/vorbis/CMakeLists.txt to actually build vorbis encoder plugin Fix plugins/vorbis/encodervorbis.* to not use HAVE_VORBIS anymore

Description Felix Tiede 2012-09-24 20:47:50 UTC

Though libvorbis-1.3.3 is provided and found by audiocd-kio's build system and libaudiocd_encoder_vorbis.so is built, there is no vorbis support for audio CDs.

There is no tab in systemsettings to allow setup and ogg/vorbis files are not provided if URL audiocd:/ is opened in dolphin, konqueror or amarok.

Reproducible: Always

Steps to Reproduce:
1. Open URL audiocd:/ in any kioslave-supporting application
Actual Results:  
No folder with Ogg/Vorbis files is provided.

Expected Results:  
Similar to wav there should be a folder providing the same files encoded as Ogg/Vorbis.

KDE-4.9.1, compiled from sources;
libvorbis-1.3.3, compiled from sources.

Digging through the code it seems the problem is caused by a number of #ifdef HAVE_VORBIS statements in source files in plugins/vorbis while HAVE_VORBIS is never defined, thus switching off the entire sourcecode of the plugin. 

I'll test if adding a line 'add_definitions( -DHAVE_VORBIS )' (similar to the matching line in plugins/flac/CMakeLists.txt) to plugins/vorbis/CMakeLists.txt solves the problem and provide a matching patch for it.

Comment 1 Felix Tiede 2012-09-24 21:02:18 UTC

Created attachment 74146 [details]
Fix audiocd-kio plugins/vorbis/CMakeLists.txt to actually build vorbis encoder plugin

Comment 2 Felix Tiede 2012-09-25 07:45:57 UTC

Created attachment 74156 [details]
Fix plugins/vorbis/encodervorbis.* to not use HAVE_VORBIS anymore

This is my other - IMHO cleaner - approach to the bug. It ditches conditional compiling completely but requires libvorbis version 2 (package version >RC2) at compiletime. As far as I understand this version of libvorbis is fairly old itself (according to xiph.org's source control: more than 10 years) so it should not pose any problems with reasonable recent installations.

This patch also makes use of the newer features of libvorbis, whih the other patch did not.

Comment 3 Michael Pyne 2012-09-27 03:23:20 UTC

Git commit c2baf636be4cb17400669b989d0215307c4ce1ec by Michael Pyne.
Committed on 27/09/2012 at 05:16.
Pushed by mpyne into branch 'KDE/4.9'.

audiocd-kio: Compile vorbis support when detected.

Apply a patch from Felix Tiede to re-enable vorbis plugin support for
the audio CD KIOSlave. (It can be verified that support was not
previously compiled by running kcmshell4 audiocd).

Felix has ideas for further improvement but I wanted to ensure that KDE
SC 4.9.2 contained the fix. Thanks for the bug report, issue
investigation, and fix(es)!

FIXED-IN:4.9.2

M  +2    -0    plugins/vorbis/CMakeLists.txt

http://commits.kde.org/audiocd-kio/c2baf636be4cb17400669b989d0215307c4ce1ec

Comment 4 Michael Pyne 2012-09-27 03:38:22 UTC

Felix:

I like the idea of your second patch to remove HAVE_VORBIS entirely. It would require the CMake check to be modified so that we don't try to compile if the correct version of the library isn't present (my version of CMake appears to already export data about which version of libvorbis was found, so this should hopefully be easy). In any event there's no emergency here so we can hold off until 4.10 before we change the required version of libvorbis.

If you could please send the code change patch along to the kde-multimedia mailing list for discussion/development I would appreciate it, Bugzilla isn't really the best place for code review. ;-)