Have you seen #493 by @jpffitch ?

Yes, I have seen #493, it's what inspired to put this up, having sat on it for a while. I hope that this and #493 can be merged into one another.

There are a few differences between this and the approach of #493. Firstly, as encoding and decoding use completely orthogonal libraries, encoding and decoding are split into separate files.

The subformat types introduced differ. #493 introduces the subtypes SF_FORMAT_MP3_CBR and SF_FORMAT_MP3_VBR. #499 (this) introduces SF_FORMAT_MPEG_I through SF_FORMAT_MPEG_III.

Otherwise, it is a measure of completeness in different areas comparing the two, and much of the code from two could be merged into one another (id3 support, xing tag support, normalization support.)

My attempt has got bogged down in reading. I failed to get mpg123 to work and I was warned off the HIP part of lame. Currently I have been experimenting with a different library -- I seem to loose half the input but reading ints otherwise works (for some value of work...) If you have got further I am happy to withdraw. I will look at your PR

I was hoping you would take this over from me :) I too got bogged down in this. MP3 as a file-format is under-specified.

I looked at HIP from liblame too. No seek support, and questions about what it would do with non-vanilla files.

What other library are you looking at? I looked at libmad too, but again, no seek support. mpg123 sometimes seems a bit heavy.

First task is the rebase this against current master to remove the merge commits.

Does this include tests?

This version runs the mpeg_test code (simple edit from ogg_test)

mpeg_short_test                : mpeg_short.mp3 .......... ok
mpeg_seek_test                 : mpeg_short.mp3 .......... ok
mpeg_int_test                  : mpeg_int.mp3 ............ ok
mpeg_seek_test                 : mpeg_int.mp3 ............ ok
mpeg_float_test                : mpeg_float.mp3 .......... ok
mpeg_seek_test                 : mpeg_float.mp3 .......... ok
mpeg_double_test               : mpeg_double.mp3 ......... ok
mpeg_seek_test                 : mpeg_double.mp3 ......... ok
mpeg_stereo_seek_test          : mpeg_seek.mpeg .......... ok

Will do some more tests but so far looks good.

Thank you @jpffitch! I have included your test program, and thanks again for finding the float decoding bug.

In this pull, the major format specifier is named SF_FORMAT_MPEG and there are three minors: SF_FORMAT_MPEG_I through SF_FORMAT_MPEG_III. Support for layers other than just three was added, because mpg123 supports all three layers, we would have to do work to exclude them.

(Only layer III is supported for writing. Nothing precludes using twolame for layer II writing support at some future point.)

The more I think about it, I wonder if renaming the major format from SF_FORMAT_MPEG back to SF_FORMAT_MP3 that it was before might be a good idea.

So-called mp3 files aren't actually a thing, there is no specification, just conventions on headers and trailers added to a MPEG audio layer III stream shoved in a file. SF_FORMAT_MP3 has utility in that it makes common assumptions explicit, and models common conception about 'mp3' files.

I still think the subformat types should model MPEG audio layers.

One case to look out for is SF_FORMAT_MP3 | SF_FORMAT_MPEG_III  vs SF_FORMAT_RAW | SF_FORMAT_MPEG_III. If one saves a RAW|MPEG_III file, then opens it again, it should be detected as RAW|MPEG_III not MP3|MPEG_III, as the tests would consider that an error. I think that it should be easy to differentiate, based on the lack of and Lame/Xing/Info header and/or ID3v1 trailer. This does require forcing SF_FORMAT_MP3 to cause a Lame info header to be written even in CBR files, but I would suggest that if a user wants a CBR MP3 with no Lame/Xing/Info header, they should use RAW|MPEG_III.

Is this going to be merged? MP3 support would be great.

Needs more work at this time sadly, and I need more time (or money, can contract, call me.) 'MP3 files' are an ad-hoc dirty standard, based on MPEG-1 and MPEG-2 sure, but there's a whole lot of fuzzy crap around the edges. Add to that the fact that the leading libraries for them, lame (lackluster API) and mpg123 (not threadsafe), and well, you have where we are now.

lame (lackluster API) and mpg123 (not threadsafe)

Why, is libsndfile thread safe? And libmp3lame uses libmpg123 for decoding anyway. However.

If its issues are listed anywhere, I may be able to help, not because i'm rich yet :) but I have some time and ability

What do you mean by "needs more work"?

@martinwguy

Why, is libsndfile thread safe?

Yes, it is mostly thread safe. The only exception is in error handling and reporting.

What do you mean by "needs more work"?

According to #499 (comment) by the MP3 branch's current maintainer

What kind of work, though?

What kind of work, though?

I'm trying to find that out.

Why, is libsndfile thread safe? And libmp3lame uses libmpg123 for decoding anyway.

mpg123 has non-thread safe global init and cleanup functions. It's a problem that can be ignored for now, it's some table precalculation stuff, so threads could race at writing the same data, but it feels dirty, and who knows, maybe some future version of mpg123 will use the global functions for more.

Yes, lame contains a mp3 decoder in it's API which is mpg123, but it is an old version which has been copied into the lame codebase, and while it doesn't contain the non-threadsafe parts, also is very much lacking. (Been there, tried it.)

OK, I've run some tests on this and got help privately from arthurt, so I'm forwarding the salient parts of that in case they're userful to other testers (I know, I should have kept it on-list)

Me:

I'm trying to test the new code, but with all MP3 files, sf_open
fails saying "File contains data in an unimplemented format." or
"Format not recognised.", using:

git clone http://github.com/arthurt/libsndfile libsndfile-arthurt
cd libsndfile-arthurt
git checkout mpeg-support
./autogen.sh
./configure --enable-mpeg
make
sudo make install

Is there something obvious I'm missing here?

Arthur:

To compile with MPEG support not only do you have to pass --enable-mpeg, you also have to pass --enable-experimental.

Me:

OK! It was saying

External MPEG Lame/MPG123 : ........... yes

anyway, so maybe that could be protected by some kind of &&
EXPERIMENTAL, so as not to fool any other fools.

Single-threaded MP3 decoding seems to work flawlessly for me. I'm
hacking my app to make the multiple FFT threads open the sound file
once each and beat libsndfile to multi-threaded death. I'll let you
know the results

Results:

The app has five threads doing essentially random seek-reads on five separate SNDFILE*'s open to the same file (one for the audio player and four for FFTs). It works fine on WAV files but, after a few seconds, on OGG files the seek fails and then it dumps core when ogg_sync_buffers() calls realloc() with an invalid pointer and with FLAC files, either the seeks start failing, or it segmentation faults or (apparently) the calls to sf_read() stop returning any data.

With multiple threads, the mpeg code wins the prize though:

Note: Illegal Audio-MPEG-Header 0xe6000c6c at offset 8154.
Note: Trying to resync...
Note: Skipped 1024 bytes in input.
[src/libmpg123/parse.c:1273] error: Giving up resync after 1024 bytes - your stream is not nice... (maybe increasing resync limit could help).
*** stack smashing detected ***: <unknown> terminated
Segmentation fault (core dumped)

Putting a lock()-unlock() pair around each call to sf_*() functions, to ensure that only one call is in flight at a time, cures all of these problems and everything works as it should.

I can only conclude that libsndfile's WAV reader seems thread safe, but that the ogg- and flac-reading libraries. or the way libsndfile calls them, are not, even though libflac itself does claim to be so.

One oddity: though the MP3 reader works on what "file" calls "MPEG ADTS, layer II, and layer III, v1 and v2", it says "Format not recognized" on what "file" calls "Audio file with ID3 version 2.2.0, contains:MPEG ADTS, layer III, v1". snd-file-play fails similarly but the mpg123 command plays it OK.

The MP3 reading in arthurt's mpeg-support branch seems to be stable. I haven't tested its encoding side.

Putting a lock()-unlock() pair around each call to sf_*() functions, to ensure that only one call is in flight at a time, cures all of these problems and everything works as it should.

Are you sharing and simultaneously using a SNDFILE* context between threads? That is very much not supported. libsndfile is a thread safe library, as in multiple threads can use the library within one process. SNDFILE contexts are not threadsafe, a context may only be used by one thread at a time.

One oddity: though the MP3 reader works on what "file" calls "MPEG ADTS, layer II, and layer III, v1 and v2", it says "Format not recognized" on what "file" calls "Audio file with ID3 version 2.2.0, contains:MPEG ADTS, layer III, v1". snd-file-play fails similarly but the mpg123 command plays it OK.

That is probably an existing bug with very large ID3v2 headers in libsndfile (like such headers containing album art.) It's outstanding but not in the scope of this pull request. Check the output of sndfile-info:

Error : Not able to open input file blah.mp3.
File : blah.mp3
Length : 5137651
Found 'ID3' marker.
ID3 length : 97587
--------------------
Request for header allocation of 195194 denied.

Are you sharing and simultaneously using a SNDFILE* context between threads?

I don't think so but will run further checks.

One oddity: it says "Format not recognized" on what "file" calls
"Audio file with ID3 version 2.2.0, contains:MPEG ADTS, layer III, v1".
That is probably an existing bug with very large ID3v2 headers in libsndfile
Check the output of sndfile-info:

Error : Not able to open input file test/Razzle.mp3.
File : test/Razzle.mp3
Length : 4581125

Format not recognised.

Are you sharing and simultaneously using a SNDFILE* context between threads?

I don't think so but will run further checks.

Yes, I'd missed one case; the audio player was using the last-opened audio file instead of its own handle. With that fixed, it works fine for WAV, OGG, FLAC and MP3 too, so libmpg123 (1.25.10) and mpeg-support appear to be thread safe. And libsndfile too of course - maybe that could be mentioned in the Features page.

Re: the oddity, calling libmpg123 does decode it correctly, but libsndfile-mpeg-support doesn't recognise it. If you're interested in looking at that, the file is under martinwguy.net/test

Re: the oddity, calling libmpg123 does decode it correctly, but libsndfile-mpeg-support doesn't recognise it. If you're interested in looking at that, the file is under martinwguy.net/test

Thanks! Turns out it was because the file has a ID3v2.2 header, whereas libsndfile would only recognise ID3v2.3. Fixed now.

I think X.org uses unstable releases as odd minors, which we aren't doing. So the prelease is x..900. Humm.

I'm not sure: https://en.wikipedia.org/wiki/X_Window_System#Release_history.

@SoapGentoo , what do you think?

That's the entire "X Window System", which is not released monolithically anymore.

https://www.x.org/releases/individual/xserver/

I see odd and even stable minor versions:

xorg-server-1.1.0.tar.bz2
xorg-server-1.2.0.tar.bz2
xorg-server-1.3.0.0.tar.bz2
xorg-server-1.4.tar.bz2
xorg-server-1.5.0.tar.bz2
xorg-server-1.6.0.tar.bz2

To be honest, I'm completely confused with this XOrg versioning scheme.

@SoapGentoo

Our next minor prerelease version using the XOrg scheme is 1.0.99.1 or 1.0.900?

X.org uses unstable releases as odd minors, which we aren't doing. So the prelease is x..900. Humm.

Is this correct?

In that case, I'd consider 1.0.99.1 to be more suitable

semantic versioning should be really considered (https://semver.org/)

FYI, Xorg versioning is perfectly in line with SemVer.

So, to be precise, SemVer would be 1.1.0-beta.1, right? I'm following this as interested outsider. The extra dot irritates me a bit, but I recognize that it eases sorting. personally, I used the scheme 1.1.0-beta1 before and think that it also shouldn't be troublesome to do this without the dot between beta and 1 — as long as you don't do more than 9 rounds of beta/release candidate, which is sensible, IMHO.

I also used 1.2.3-somepatch in the past to indicate an addition to the base version and relied on stuff parsing that as equivalent to 1.2.3 at least… but did not expect any automatism to decide that 1.2.3-foo is newer or older than 1.2.3.

So, to be precise, SemVer would be 1.1.0-beta.1, right?

Correct.

I also used 1.2.3-somepatch in the past to indicate an addition to the base version and relied on stuff parsing that as equivalent to 1.2.3 at least… but did not expect any automatism to decide that 1.2.3-foo is newer or older than 1.2.3.

SemVer supports prereleases only, so any 1.2.3-foo is older than 1.2.3.

@SoapGentoo

/usr/lib/gcc/x86_64-pc-linux-gnu/9.3.0/../../../../x86_64-pc-linux-gnu/bin/ld:../../src/Symbols.gnu-binutils:3: ignoring invalid character `-' in script

I'm pretty sure it is bug in our create_symbols_file.py symbol generation script.

FYI, Xorg versioning is perfectly in line with SemVer.

It is not, check here https://jubianchi.github.io/semver-check

Funnily enough, such a number does not appear to be incorrect, but parsing into separate components fails. Check 1.1.0-beta.1 to see how it works.

To begin with, SemVer does not use the fourth digit of the version at all. Something similar is 'prerelease': 1.1.0-1.

New feature requires minor bump. XOrg's 1.0.99.1 doesn't reflect it,

I am not saying that I am not ready to accept XOrg versioning. But this is not SemVer. And we can even say that this is not entirely semantic versioning in cases of prereleases.

Honestly, just decide on something, I'm kinda tired of this debate.

use 1.0.99.1

Sorry, sometimes I can be very boring. Remind me about it 😄

We choose the XOrg versioning scheme with the majority of votes.

@SoapGentoo, it's your turn now.

so 1.0.900 for SemVer compatibility (consider how you want to handle the versioned library SONAME)?

so 1.0.900 for SemVer compatibility (consider how you want to handle the versioned library SONAME)?

If I understand correctly, you are asking about the ABI version? If so, it has its own versioning scheme.

1.0.31 -> 1.0.32.

@arthurt , i've fixed mpg123 Vcpkg port and MPG123_NO_FRANKENSTEIN is now restored by #725.

Would 1.1.0beta1 be the first official release with MP3 support?

Would 1.1.0beta1 be the first official release with MP3 support?

No, it is pre-release. Not all of the planned features of official 1.1.0 release have been implemented, but it is quite stable.

@arthurt, is there an approximate date?

Just to confirm: does the beta version https://github.com/libsndfile/libsndfile/releases/tag/1.1.0beta1 have MP3 read/write support?

Just to confirm: does the beta version https://github.com/libsndfile/libsndfile/releases/tag/1.1.0beta1 have MP3 read/write support?

Yes.

Just to confirm: does the beta version https://github.com/libsndfile/libsndfile/releases/tag/1.1.0beta1 have MP3 read/write support?

Yes.

Thanks @evpobr!