MP3 reader support using minimp3

[eXpl0it3r]

Description

Accidently pushed the wrong changes, closing the other PR #1796
Sorry about that, @lieff
Here's the rebased changes

This PR is related to the issue #1232

How to test this PR?

The sound example was extended and can be executed as a test

[eXpl0it3r]

@Bromeon can you give this another look, as you reviewed the original PR #1796

[Bromeon]

Thanks for the update, @lieff and @eXpl0it3r!
Most looks good now, added a few comments.

Has someone tested this with common MP3 files yet?

---

Accidently pushed the wrong changes, closing the other PR #1796

This happened to me as well in the past; you should be able to reopen it by pushing to the a with same name (and possibly clicking the "Reopen" button). Just for next time 🙂

[eXpl0it3r]

This happened to me as well in the past; you should be able to reopen it by pushing to the a with same name (and possibly clicking the "Reopen" button). Just for next time 🙂

That is true if you own the repository you're pushing to. But I accidentally reset lieff's master branch to the official master branch state and by doing so, the PR got closed and also removed the access to push to their repository.
As I didn't want to "lose" the changes, the only option was to push it on our side. 😅

[eXpl0it3r]

Even though it's currently not a "symmetric" implementation of decoder/encoder it makes sense to also include it in SFML 2.6
We might get more "can't save as MP3" forum posts, but that's small pain to deal with, when on the other hand users can load MP3 files.

But before merging this, we should spend some time, testing it with various mp3. 🙂

[lieff]

For encoder part, https://github.com/cpuimage/tinymp3 can be used, if quality is not the primary concern.

[vittorioromeo]

But before merging this, we should spend some time, testing it with various mp3. 🙂

Anything in particular you'd like to test out?

[eXpl0it3r]

Grab some mp3's from your collection (if that's still a thing) and play them with SFML.
Best also when they're from different source, times, encoders, with and without tags, etc.
One could also try common/well-known audio applications and see whether their output mp3 play fine with SFML.

[vittorioromeo]

Some testing on Windows 10 x64, using MSYS2/MinGW:

• I was able to play the provided ding.mp3 just fine.

• I wasn't able to play minimal.mp3 (attached), with the following error:

  Failed to open sound file "resources/minimal.mp3" (format not supported)

  • The error is pretty unhelpful. Why is the format not supported?
  • I can play the file in any music player just fine.

• I was able to play the T-Range.mp3 file (attached) just fine.

• I was able to play the swap.mp3 file (attached) just fine.

• I was able to play the click.mp3 file (attached) just fine.

This is what VLC reports for minimal.mp3:

This is what VLC reports for ding.mp3:

Test files:
test_mp3_resources.zip

---

EDIT: There is definitely a chance to improve our error reporting mechanism here, possibly by using something like std::optional<std::string> as a first solution, or something nicer like an expected<T> class instead of a plain bool. Then we can propagate that outwards. I think it's outside the scope of the current PR, but it would be a nice improvement for a future one.

---

EDIT2: With some good-ol' printf debugging, these checks both fail in check() for minimal.mp3:

if (hasValidId3Tag(header)) // returns false
    return true;

if (hdr_valid(header))  // returns false
    return true;

@lieff, is this intended?

[lieff]

minimal.mp3 is not regular mp3 file, but mp4 file with mp3 inside:

So yeah, this intended.
I've tested some files too and they all play fine.

[vittorioromeo]

minimal.mp3 is not regular mp3 file, but mp4 file with mp3 inside:

So yeah, this intended. I've tested some files too and they all play fine.

Thanks for the clarification. I am quite ignorant regarding music formats, is it uncommon to have an mp3 inside a mp4 container? Would it be hard to support it in minimp3, if it's reasonably common to encounter?

[lieff]

Not sure how it's common. I've never seen just mp3 audio in mp4 before, aac audio is more native and common to mp4.

Minimp3 is not the place where demuxing should be done. https://github.com/lieff/minimp4 can be used for demuxing.

Note that mp4 container can contain different audio and video codecs and multiple tracks, so such cases should be handled somehow. For example limit to only audio in mp3 and first track, return error otherwise.

Also note that there more popular containers: avi, wmv(wma), mkv, webm, so full proper support for containers is big. Better use ffmpeg or gstreamer in this case.

[vittorioromeo]

From my perspective, I am happy with this PR. I would prefer merging it as it is, get some community testing/feedback, and iterate on any possible issue/improvement that might arise from that.

[vittorioromeo]

@eXpl0it3r: happy to merge?

[eXpl0it3r]

Thanks a lot @lieff and everyone involved, looking forward to adding MP3 as supported audio format to the changelog 🙂

[ChrisThrasher]

Can we close #1232?

[eXpl0it3r]

Since nobody seems to read the issue description, I guess we can close it and open a new issue for implementing a MP3 encoder 😄

See #2679