FFMPEG cover art support #368

jobermayr · 2014-11-05T22:30:14Z

Otherwise build fails with:

lin64_build/soundsourceffmpeg.o:(.rodata._ZTV17SoundSourceFFmpeg[vtable for SoundSourceFFmpeg]+0x48): undefined reference to `SoundSourceFFmpeg::parseCoverArt()'
collect2: ld returned 1 exit status
scons: *** [lin64_build/mixxx] Error 1

mixxx-buildbot · 2014-11-05T22:30:15Z

safe to test?

jobermayr · 2014-11-05T22:32:15Z

buildbot: test this please

kain88-de · 2014-11-06T11:12:12Z

src/soundsourceffmpeg.cpp

+
+    AVFormatContext *FmtCtx = avformat_alloc_context();
+
+    if (avformat_open_input(&FmtCtx, qBAFilename.constData(), NULL, NULL)) {


sure you are not missing a ! here? Usually open functions return true on success.

See line 544: != 0 means we need false to success?

A return value of 0 means that the function call succeeded

https://www.ffmpeg.org/doxygen/2.3/group__lavf__decoding.html#ga10a404346c646e4ab58f4ed798baca32

The standard way to check for this in C is if (func() != 0). It would be nice if you stick with the standard coding style as this makes it easier to read and maintain in the future.

Any idea why it fails:
Run till exit from #0 avformat_open_input (ps=0x7fffffffd440, filename=0x1bfdb18 "/dev/shm/osc/home/abuild/rpmbuild/BUILD/mixxx/src/test/id3-test-data/cover-test.mp3", fmt=0x0, options=0x0)
at libavformat/utils.c:543
No URL Protocols are registered. Missing call to av_register_all()?
0x00000000009d6fba in SoundSourceFFmpeg::parseCoverArt (this=0x1b22a20) at src/soundsourceffmpeg.cpp:645
645 if (avformat_open_input(&FmtCtx, m_qFilename.toLocal8Bit().constData(), NULL, NULL) != 0) {
Value returned is $1 = -1330794744

Which version of FFMPEG do you run?

2.3.3 from https://pmbs.links2linux.de/package/show/Essentials/ffmpeg
-> libavformat.so.55.48.100

This is called main.cpp:309 and that error tries to tell you ParseCoverArt is called before av_register_all() function is called. Do you get it often?

kain88-de · 2014-11-06T11:18:59Z

@jobermayr thank you for your contribution.

Besides the comments. The CoverArtUtilTest.extractEmbeddedCover and CoverArtUtilTest.searchImage are failing.

Btw what compile options are you using. Wreturn-type is not enabled by default in our scons scripts.

jobermayr · 2014-11-06T16:22:43Z

scons -j1 prefix=/usr optimize=9 djconsole=1 djconsole_legacy=0 hifieq=1 experimentalrecord=1 cmetrics=0 ladspa=0 vinylcontrol=1 ffmpeg=1 shoutcast=1 ipod=0 script=1 midiscript=1 tonal=1 m4a=1 faad=1 mad=1 hss1394=0 wv=1 asmlib=0 portmidi=1 shoutcast=1 modplug=1 qdebug=0 perftools=1
[...]
Building with CCFLAGS: -pipe -Wall -Wextra -g -pthread -O3 -ffast-math -funroll-loops -fomit-frame-pointer -mtune=generic -mmmx -msse -mfpmath=sse -msse2 -D__STDC_CONSTANT_MACROS -D__STDC_LIMIT_MACROS -D__STDC_FORMAT_MACROS
[...]
g++ -o lin64_build/library/coverart.o -c -pipe -Wall -Wextra -g -pthread -O3 -ffast-math -funroll-loops -fomit-frame-pointer -mtune=generic -mmmx -msse -mfpmath=sse -msse2 -D__STDC_CONSTANT_MACROS -D__STDC_LIMIT_MACROS -D__STDC_FORMAT_MACROS -Dx86_64 -D__LINUX__ -D__UNIX__ -DSETTINGS_PATH=\".mixxx/\" -DSETTINGS_FILE=\"mixxx.cfg\" -DUNIX_SHARE_PATH=\"/usr/share/mixxx\" -DUNIX_LIB_PATH=\"/usr/lib/mixxx\" -DSOUNDTOUCH_ALLOW_X86_OPTIMIZATIONS -D__PORTAUDIO__ -DQT_SHARED -DQT_TABLET_SUPPORT -DQT_CORE_LIB -DQT_GUI_LIB -DQT_OPENGL_LIB -DQT_XML_LIB -DQT_SVG_LIB -DQT_SQL_LIB -DQT_SCRIPT_LIB -DQT_NETWORK_LIB -DQT_SHARED -D__SNDFILE__ -D__MAD__ -D__HID__ -D__BULK__ -D__VINYLCONTROL__ -D__SHOUTCAST__ -DQT_NO_DEBUG_OUTPUT -D__SSE__ -D__SSE2__ -D__MODPLUG__ -DHAVE_FFTW3 -D__AUTODJCRATES__ -D__SQLITE3__ -D__FFMPEGFILE__ -D__LIBAVRESAMPLE__ -Ilin64_build -Isrc -Ilib/soundtouch-1.6.0 -Ilib/replaygain -I/usr/include/QtOpenGL -I/usr/include/QtXml -I/usr/include/QtSvg -I/usr/include/QtGui -I/usr/include/QtSql -I/usr/include/QtScript -I/usr/include/QtXmlPatterns -I/usr/include/QtNetwork -I/usr/include/QtCore -I/usr/include/QtTest -Ilib/gtest-1.7.0/include -Ilib/fidlib-0.9.10 -I/usr/include/taglib -Ilib/hidapi-0.8.0-pre/hidapi -I/usr/include/libusb-1.0 -Ilib/xwax -Ilib/scratchlib src/library/coverart.cpp [ 635s] In file included from src/library/coverart.cpp:4:0: [ 635s] src/util/debug.h: In instantiation of 'QString toDebugString(const T&) [with T = QSize]': [ 635s] src/library/coverart.cpp:41:48: required from here [ 635s] src/util/debug.h:10:9: warning: unused parameter 'object' [-Wunused-parameter] [ 635s] QString toDebugString(const T& object) { [ 635s] ^ [ 635s] src/util/debug.h: In instantiation of 'QString toDebugString(const T&) [with T = CoverInfo]': [ 635s] src/library/coverart.cpp:42:40: required from here [ 635s] src/util/debug.h:10:9: warning: unused parameter 'object' [-Wunused-parameter]

kain88-de · 2014-11-07T13:05:41Z

src/library/coverart.cpp

    }
+    return "INVALID INFO VALUE";


This is no issue for me. We never use the Wreturn-type flag in our build scripts. Besides the warning shouldn't break the build. You should check if OpenSuse has some defaults set with these compilation types.

It is a reason for openSUSE to let builds fail on their infrastructure:
https://github.com/openSUSE/post-build-checks/blob/master/checks-data/check_gcc_output#L92

We build with -Wall and according to the GCC manual, that implies -Wreturn-type.

kain88-de · 2014-11-07T13:12:27Z

Can you revert the last commit since it seems to be an issue with OpenSuse rather then mixxx.

If I run mixxx with your ffmpeg now the covers are not being scanned, you can check this by running the tests they should all pass. Scrolling crashes mixxx reproducible with a GLib-ERROR that to many files are open.

kain88-de · 2014-11-07T13:21:04Z

src/soundsourceffmpeg.cpp

+            break;
+        }
+
+    av_free(FmtCtx);


https://www.ffmpeg.org/doxygen/2.3/group__lavf__decoding.html#gae804b99aec044690162b8b9b110236a4

You should close the files after you have parsed them.

Yes you should defenetly do

avformat_close_input(&FmtCtx); av_free(FmtCtx);

If you don't then you run these odd too much files open stuff.

illuusio · 2014-11-14T09:51:05Z

Fixed like this I can import my collection (Several gigs of mp3/ogg) and mixxx doesn't crash

QImage SoundSourceFFmpeg::parseCoverArt() {
    qDebug() << "ffmpeg: SoundSourceFFmpeg::parseCoverArt" << m_qFilename;
    QByteArray qBAFilename = m_qFilename.toLocal8Bit();
    QImage coverArt;
    setType("ffmpeg");

    AVFormatContext *FmtCtx = avformat_alloc_context();

    if (avformat_open_input(&FmtCtx, qBAFilename.constData(), NULL, NULL)) {
        qDebug() << "av_open_input_file: cannot open" << qBAFilename.constData();
        av_free(FmtCtx);
        return QImage();
    }

    if (FmtCtx->iformat->read_header(FmtCtx) < 0) {
        qDebug() << "AVFormatContext: cannot read format header";
        avformat_close_input(&FmtCtx);
        av_free(FmtCtx);
        return QImage();
    }

    // find the first attached picture
    for (unsigned int i = 0; i < FmtCtx->nb_streams; i++)
        if (FmtCtx->streams[i]->disposition & AV_DISPOSITION_ATTACHED_PIC) {
            coverArt.loadFromData(FmtCtx->streams[i]->attached_pic.data,
                                  FmtCtx->streams[i]->attached_pic.size, 0);
            break;
        }

    avformat_close_input(&FmtCtx);
    av_free(FmtCtx);
    return coverArt;
}

No much but some fixes.. I also push this to my repo..

illuusio · 2014-11-26T12:02:58Z

Hmm.. seems pretty Ok to me. I'll test this more roughly to get better opinion..

jobermayr · 2014-12-01T22:24:00Z

Isn't it good enough to get merged?

rryan · 2014-12-01T22:43:52Z

src/test/coverartutils_test.cpp

@@ -46,9 +46,11 @@ TEST_F(CoverArtUtilTest, extractEmbeddedCover) {
    // them in a sandboxed environment.
    SecurityTokenPointer pToken;
    // aiff
+#ifndef __FFMPEGFILE__


Why do you need to disable these?

They are not supported? kain88-de bitched about failing tests ...

FFMPEG doesn't support AIFF or WAV?

Or do you mean FFMPEG doesn't support AIFF or WAV cover art?

Cover art support for AIFF and WAV isn't implemented in FFMPEG - at least on my version ...

Hmm, that's unfortunate. We should use TagLib for cover art for SoundSourceFFMPEG then if FFMPEG does not support everything we need.

rryan · 2014-12-01T22:50:17Z

Please revert 34c2851.

If you'd like you could make every codec a build "feature" (see build/features.py) and then openSUSE could build Mixxx like: scons mad=0 ogg=0 flac=0 sndfile=0 opus=0 ffmpeg=1.

rryan · 2014-12-01T22:51:53Z

Apologies for the delay @jobermayr -- we're all focused on getting our next major release out so we are not paying much attention to experimental features like FFMPEG support at the moment.

jobermayr · 2014-12-03T22:15:15Z

Reverting 34c2851 would mean it is possible to use ffmpeg and other codecs together. This isn't the case:

#ifdef __FFMPEGFILE__
    return Mixxx::SoundSourcePointer(new SoundSourceFFmpeg(qFilename));

On the other side it would better if build system fails when ffmpeg is set together with other codecs ...

rryan · 2014-12-03T22:28:55Z

Reverting 34c2851 would mean it is possible to use ffmpeg and other codecs together. This isn't the case:

That's up to the person building Mixxx :). Let's not make them mutually exclusive.

jobermayr · 2014-12-03T22:36:25Z

Am Mittwoch, 3. Dezember 2014, 14:28:58 schrieb RJ Ryan:

Reverting 34c2851 would mean it is possible to use ffmpeg and other codecs together. This isn't the case:

That's up to the person building Mixxx :). Let's not make them mutually exclusive.

Hmm. I know a lot of projects which doesn't make it possible to mix(xx) senseless build options.

Nonetheless it doesn't make sense here to build never reachable code in ...

illuusio · 2014-12-04T12:38:32Z

I think this one is worth of another pull request.. I've been also wondering this by myself but it not fight that should be done here. Main thing is Mixxx is not currently compiling from the GIT when compiling with FFMPEG and with this it works and I can import + 5000 MP3/OGG/M4A files with or without images.. I don't like goto's (they are so Commodore 64) but what you can do.. so let's sort this one in and then start struggling with Scons (I just feel it should be delayed to 1.13 because this Scons script is working).
And I'm 👍 with this because I'm fellow openSUSE dev..

rryan · 2014-12-04T15:21:19Z

I think this one is worth of another pull request.. I've been also wondering this by myself but it not fight that should be done here. Main thing is Mixxx is not currently compiling from the GIT when compiling with FFMPEG and with this it works and I can import + 5000 MP3/OGG/M4A files with or without images..

I added a stub implementation in 39a57a1 so the build is now fixed.

From @jobermayr's comments, it sounds like the FFMPEG cover art API does not meet Mixxx's needs -- so we should use TagLib instead.

SoundSourceFFMPEG::parseHeader should also use TagLib but that's a topic for another PR.

I don't like goto's (they are so Commodore 64) but what you can do..

They didn't come from out of the sky -- @jobermayr typed them.

rryan · 2014-12-04T15:34:44Z

They are not supported? kain88-de bitched about failing tests ...

@jobermayr -- @kain88-de is a valued Mixxx contributor and he took the time out of his day to give you review comments. Pointing out that the tests don't pass isn't "bitching" -- please be more polite.

illuusio · 2014-12-04T17:33:49Z

BTW being bit n00b. How can I find this PR build from Jenkins? @rryan One to rule them all Taglib one choice. But in my opinion FFMpeg is good enough for the work. As said this works very effient for me.

rryan · 2014-12-09T20:32:48Z

BTW being bit n00b. How can I find this PR build from Jenkins?

PRs don't get packaged by the build server currently -- at some point I'd like to add this.

@rryan One to rule them all Taglib one choice. But in my opinion FFMpeg is good enough for the work. As said this works very effient for me.

Using 2 different systems for reading file metadata means double the bugs! From a maintenance perspective that's not acceptable.

Also, SoundSourceFFMPEG::parseHeader doesn't currently support BPM, musical key, grouping, or composer. So it doesn't work for a lot of users (the BPM one in particular is very important) in its current state.

Of course, you could add support for all of this. But why repeat what is already handled well by an existing and production-tested (by Traktor and Mixxx) library?

illuusio · 2014-12-12T08:34:32Z

Just wondering FFMPEG plays far more file formats than off the self Mixxx. Does taglib have to know what format it queries or does it do it by itself..

rryan · 2014-12-26T20:28:41Z

Just wondering FFMPEG plays far more file formats than off the self Mixxx. Does taglib have to know what format it queries or does it do it by itself..

That's a good point -- for formats we don't support with TagLib we could fall back on FFMPEG. I think having consistency in Mixxx's core supported formats makes sense.

illuusio · 2015-03-06T05:56:02Z

Hello, now that Taglib based stuff for FFmpeg is in should we drop this or impment this as ultimate backup for FFmpeg tag and covert art reading?

kain88-de · 2015-03-08T17:02:12Z

yeah we can close this now. The original issue has been solved by using taglib parsing indepdent of ffmpeg.

kain88-de reviewed Nov 6, 2014
View reviewed changes

kain88-de reviewed Nov 7, 2014
View reviewed changes

rryan reviewed Dec 1, 2014
View reviewed changes

rryan changed the title ~~[Build Fix] Implement SoundSourceFFmpeg::parseCoverArt().~~ FFMPEG cover art support Dec 4, 2014

jobermayr added 3 commits December 5, 2014 22:20

Implement SoundSourceFFmpeg::parseCoverArt().

e7ec22c

[Build Fix] openSUSE fails due to -Wreturn-type.

c10ba42

FFMPEG doesn't support other codecs.

d007588

kain88-de closed this Mar 8, 2015


		AVFormatContext *FmtCtx = avformat_alloc_context();

		if (avformat_open_input(&FmtCtx, qBAFilename.constData(), NULL, NULL)) {

FFMPEG cover art support #368

FFMPEG cover art support #368

Conversation

jobermayr commented Nov 5, 2014

mixxx-buildbot commented Nov 5, 2014

jobermayr commented Nov 5, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kain88-de commented Nov 6, 2014

jobermayr commented Nov 6, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kain88-de commented Nov 7, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

illuusio commented Nov 14, 2014

illuusio commented Nov 26, 2014

jobermayr commented Dec 1, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rryan commented Dec 1, 2014

rryan commented Dec 1, 2014

jobermayr commented Dec 3, 2014

rryan commented Dec 3, 2014

jobermayr commented Dec 3, 2014

illuusio commented Dec 4, 2014

rryan commented Dec 4, 2014

rryan commented Dec 4, 2014

illuusio commented Dec 4, 2014

rryan commented Dec 9, 2014

illuusio commented Dec 12, 2014

rryan commented Dec 26, 2014

illuusio commented Mar 6, 2015

kain88-de commented Mar 8, 2015