MP3 reader support #1796

lieff · 2021-05-10T19:45:27Z

This PR is related to the issue #1232 and adds mp3 reading support.
Also discussed on the forum
Example SFML/examples/sound/Sound.cpp is extended to play an mp3 file.

eXpl0it3r

You should update the comments on the these three openFrom* functions in the InputSoundFile header.
You may also add a comment regarding APEv2 causing potential issues.

SFML/include/SFML/Audio/InputSoundFile.hpp

Lines 90 to 101 in 43b2e9d

    
           //////////////////////////////////////////////////////////// 
        
           /// \brief Open a sound file from a custom stream for reading 
        
           /// 
        
           /// The supported audio formats are: WAV (PCM only), OGG/Vorbis, FLAC. 
        
           /// The supported sample sizes for FLAC and WAV are 8, 16, 24 and 32 bit. 
        
           /// 
        
           /// \param stream Source stream to read from 
        
           /// 
        
           /// \return True if the file was successfully opened 
        
           /// 
        
           //////////////////////////////////////////////////////////// 
        
           bool openFromStream(InputStream& stream);

cmake/Modules/FindMinimp3.cmake

examples/sound/Sound.cpp

src/SFML/Audio/CMakeLists.txt

src/SFML/Audio/SoundFileReaderMp3.hpp

src/SFML/Audio/SoundFileReaderMp3.cpp

eXpl0it3r · 2021-05-11T09:30:13Z

You should also update the license.md to include the license for minimp3

include/SFML/Audio/InputSoundFile.hpp

eXpl0it3r

See the two comments regarding use system deps and the windows.h conflict

Bromeon

Thanks a lot for the contribution!

I noticed a few smaller issues, but overall it looks good 🙂

CMakeLists.txt

examples/sound/Sound.cpp

include/SFML/Audio/InputSoundFile.hpp

src/SFML/Audio/SoundFileReaderMp3.cpp

examples/sound/Sound.cpp

src/SFML/Audio/SoundFileReaderMp3.cpp

Bromeon · 2021-05-12T10:37:48Z

src/SFML/Audio/SoundFileReaderMp3.cpp

+    m_io.read = readCallback;
+    m_io.seek = seekCallback;


The rest of m_io remains uninitialized, since its type is a POD (plain old data) without constructor. This might lead to UB if one of the SoundFileReaderMp3 methods is called without a prior open() call, where m_io is fully initialized.

Also, m_decoder is not initialized.

Added memset initialize. Updated the PR.

When the memory is zeroed, seek() and other operations are safe (i.e. fail properly)?
I don't know the implementation and assumptions of the underlying library.

It should be fine, added tests for such case lieff/minimp3@b18d274 .
But calling open() method twice will lead to mem leak. Same for SoundFileReaderOgg as I can see. So calling it twice is forbidden? Or may be add protection against it?

src/SFML/Audio/SoundFileReaderMp3.cpp

eXpl0it3r · 2021-11-23T09:25:39Z

Woops, I screwed up, while trying to update the branch. Had to opened a new PR #1851 with the changes...
Sorry about that. 😓

lieff · 2021-11-23T13:59:50Z

Is something needed from my side? Looks like PR stuck on email delivery or something.

eXpl0it3r · 2021-11-23T15:09:30Z

Nope, yeah seems like it somehow got lost, sorry about that.
I'll see to fix the typo as mentioned on the other PR and get the unity (not the engine) build to work.
If you want to take ownership of the PR again, then feel free to open one again, I messed up with updating the PR. 😓

lieff mentioned this pull request May 10, 2021

Add support for MP3 #1232

Closed

2 tasks

eXpl0it3r requested changes May 11, 2021

View reviewed changes

eXpl0it3r added feature m:sfml-audio s:accepted labels May 11, 2021

lieff force-pushed the master branch 4 times, most recently from f50c7f1 to 8371a4c Compare May 11, 2021 11:06

lieff requested a review from eXpl0it3r May 11, 2021 11:59

eXpl0it3r reviewed May 11, 2021

View reviewed changes

include/SFML/Audio/InputSoundFile.hpp Outdated Show resolved Hide resolved

lieff force-pushed the master branch from 8371a4c to 8a95fc8 Compare May 11, 2021 12:12

lieff requested a review from eXpl0it3r May 11, 2021 12:14

eXpl0it3r requested changes May 11, 2021

View reviewed changes

lieff force-pushed the master branch 2 times, most recently from 4682f3a to 65293e7 Compare May 11, 2021 13:31

lieff requested a review from eXpl0it3r May 11, 2021 13:32

eXpl0it3r approved these changes May 11, 2021

View reviewed changes

eXpl0it3r requested a review from Bromeon May 11, 2021 13:47

Bromeon requested changes May 12, 2021

View reviewed changes

lieff force-pushed the master branch 3 times, most recently from b58c4a8 to 5f46044 Compare May 12, 2021 08:43

lieff requested a review from Bromeon May 12, 2021 08:43

lieff force-pushed the master branch from 5f46044 to c2e67f9 Compare May 12, 2021 10:50

Bromeon requested changes May 12, 2021

View reviewed changes

lieff force-pushed the master branch from c2e67f9 to 16a4139 Compare May 12, 2021 11:07

lieff requested a review from Bromeon May 12, 2021 11:11

lieff force-pushed the master branch from 16a4139 to 2390b5f Compare May 12, 2021 12:05

eXpl0it3r closed this Nov 23, 2021

eXpl0it3r force-pushed the master branch from 2390b5f to dc88cbd Compare November 23, 2021 09:18

eXpl0it3r mentioned this pull request Nov 23, 2021

MP3 reader support using minimp3 #1851

Merged

eXpl0it3r added s:superseded and removed s:accepted labels Nov 23, 2021

eXpl0it3r added this to Discussion in SFML 2.6.0 via automation Dec 6, 2021

eXpl0it3r added this to the 2.6 milestone Dec 6, 2021

eXpl0it3r moved this from Discussion to Done in SFML 2.6.0 Dec 6, 2021

dogunbound mentioned this pull request May 17, 2023

Pull request for 2.6.x CSFML SFML/CSFML#186

Merged

ChrisThrasher mentioned this pull request Jun 23, 2023

Implement remaining SFML 2.6.0 changes SFML/CSFML#187

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MP3 reader support #1796

MP3 reader support #1796

lieff commented May 10, 2021

eXpl0it3r left a comment

eXpl0it3r commented May 11, 2021

eXpl0it3r left a comment

Bromeon left a comment

Bromeon May 12, 2021

lieff May 12, 2021 •

edited

Bromeon May 12, 2021

lieff May 12, 2021 •

edited

eXpl0it3r commented Nov 23, 2021 •

edited

lieff commented Nov 23, 2021

eXpl0it3r commented Nov 23, 2021

	////////////////////////////////////////////////////////////
	/// \brief Open a sound file from a custom stream for reading
	///
	/// The supported audio formats are: WAV (PCM only), OGG/Vorbis, FLAC.
	/// The supported sample sizes for FLAC and WAV are 8, 16, 24 and 32 bit.
	///
	/// \param stream Source stream to read from
	///
	/// \return True if the file was successfully opened
	///
	////////////////////////////////////////////////////////////
	bool openFromStream(InputStream& stream);

MP3 reader support #1796

MP3 reader support #1796

Conversation

lieff commented May 10, 2021

eXpl0it3r left a comment

Choose a reason for hiding this comment

eXpl0it3r commented May 11, 2021

eXpl0it3r left a comment

Choose a reason for hiding this comment

Bromeon left a comment

Choose a reason for hiding this comment

Bromeon May 12, 2021

Choose a reason for hiding this comment

lieff May 12, 2021 • edited

Choose a reason for hiding this comment

Bromeon May 12, 2021

Choose a reason for hiding this comment

lieff May 12, 2021 • edited

Choose a reason for hiding this comment

eXpl0it3r commented Nov 23, 2021 • edited

lieff commented Nov 23, 2021

eXpl0it3r commented Nov 23, 2021

lieff May 12, 2021 •

edited

lieff May 12, 2021 •

edited

eXpl0it3r commented Nov 23, 2021 •

edited