add mp3 support #782
add mp3 support #782
Conversation
* provide unified interface for reading streamed memory * reuse single filehandler buffer from MusicStream
drmp3 is a minimalistic mp3 decoder known from SDL2 mixer library
* reuse code from ogg decoder * change interface to expose number of channels instead format * code cleanup and const correctness
replace common buffer type with uint8_t
| class Mp3Decoder | ||
| { | ||
| public: | ||
| long Read(uint8_t *buffer, int length, int , int , int , int *); |
There was a problem hiding this comment.
Consider naming the arguments, especially since it's not obvious what they are from context
| long Read(uint8_t *buffer, int length, int , int , int , int *); | |
| long Read(uint8_t *buffer, int length, int bigendianp, int word, int sgned, int *bitstream); |
There was a problem hiding this comment.
this is the common interface for all decoders. mp3 decoder does not use unnamed variables.
| // this buffer gets recycled by each new stream, across *all* audio-channels | ||
| // as a result streams are limited to only ever being played within a single | ||
| // channel (currently BGMusic), but cause far less memory fragmentation |
There was a problem hiding this comment.
this pre-existing limitation is pretty terrible, i have no idea to what extent this PR relies on it (so this is mostly an opportunistic remark) but regardless it would be good to gradually remove it
There was a problem hiding this comment.
I had this in mind but I did not want the scope of this PR outgrow the initial objective of adding mp3 support. I'm happy to work on it in separate issue if that's ok.
| return; | ||
| } | ||
|
|
||
| if (fileBuffer.GetFileExt() == std::string_view{"mp3"}) { |
There was a problem hiding this comment.
does the string_view bring any benefit here over just fileBuffer.GetFileExt() == "mp3"?
There was a problem hiding this comment.
Defensive programming. I wanted to avoid a case where we compare raw const char* without strcmp. Now GetFileExt is already a std::string but in case the return type changes to const char* the code without explicit string_view conversion would start failing.
| } , decoder); | ||
| if (!loaded) { | ||
| LOG_L(L_ERROR, "[MusicStream::Play] Could not load file: %s", path.c_str()); | ||
| source = 0; // invalidate |
There was a problem hiding this comment.
perhaps there could be a well-named Invalidate() that would abstract the source = 0 underneath?
There was a problem hiding this comment.
this definitely should be changed. the class kind of reimplements std::optional within self but this can be fixed in next PR that separates MusicStream from SoundSource.
Co-authored-by: sprunk <[email protected]>
Co-authored-by: sprunk <[email protected]>
Co-authored-by: sprunk <[email protected]>
Co-authored-by: sprunk <[email protected]>
|
Please add |
|
Out of curiosity: what is the motivation of adding old mp3 format at this time, when there are formats like opus that beat it at all dimensions and vorbis that is also AFAIK better than mp3 is already part of the engine? |
|
Are these formats better in availability of actual files? MP3's are still everywhere and it's good if people making games on the engine can just use them without having to convert or do any other extra steps. |
|
I thought that encoding to a good format is a very minor step in overall sound creation. Motivation of "we just want to allow people to drop in mp3s they already have to make it even easier" is fine. (The motivation is basically social and not technical) |
done |
|
I second what @sprunk said above. mp3 might not be the best, but it's pretty common and having it covered up should not be redundant. Let's target this one after the stable version has been released. |
|
Sorry for the long wait, is it ready for merge? |
looks good |
Summary of PR content: