Add AudioStreamMP3 load_from_file/load_from_buffer and harmonize other audio streams
#100307
Conversation
akien-mga
force-pushed
the
mp3-load_from_file
branch
from
d8a45da to
067fdac
Compare
| void AudioStreamWAV::_bind_methods() { | ||
| ClassDB::bind_static_method("AudioStreamWAV", D_METHOD("load_from_file", "path", "options"), &AudioStreamWAV::load_from_file, DEFVAL(Dictionary())); | ||
| ClassDB::bind_static_method("AudioStreamWAV", D_METHOD("load_from_buffer", "buffer", "options"), &AudioStreamWAV::load_from_buffer, DEFVAL(Dictionary())); | ||
|
|
||
| ClassDB::bind_method(D_METHOD("set_data", "data"), &AudioStreamWAV::set_data); | ||
| ClassDB::bind_method(D_METHOD("get_data"), &AudioStreamWAV::get_data); |
There was a problem hiding this comment.
This is just being moved further down to follow the usual structure:
- Public methods
_bind_methods- Constructor
- Destructor
| Ref<AudioStreamOggVorbis> ResourceImporterOggVorbis::load_from_buffer(const Vector<uint8_t> &file_data) { | ||
| Ref<AudioStreamOggVorbis> ogg_vorbis_stream; | ||
| ogg_vorbis_stream.instantiate(); | ||
|
|
||
| Ref<OggPacketSequence> ogg_packet_sequence; | ||
| ogg_packet_sequence.instantiate(); |
There was a problem hiding this comment.
This was moved as is to AudioStreamOggVorbis::load_from_buffer (with just the parameter renamed to p_stream_data as it doesn't necessarily come from an actual file on disk).
akien-mga
force-pushed
the
mp3-load_from_file
branch
2 times, most recently
from
a31fdde to
12f5b51
Compare
There was a problem hiding this comment.
Tested locally, it works as expected. This can load and play my entire MP3 music collection (3,000+ tracks) without any errors.
Code looks good to me.
Testing project: test_pr_100307.zip
|
#97855 should be closed too? But it seems to me that error propagation suggested by @adamscott in that PR would be good |
There was a problem hiding this comment.
Looks good. Thank you @akien-mga to have taken the time to harmonize this mess. About #97855 and my suggestion of error propagation, I think it still stands, but I wouldn't not merge this PR for that reason.
…other audio streams Move OggVorbis and MP3 loading code to their AudioStream class, matching how it's done for WAV. The duplicate functions in ResourceImporterOggVorbis are now deprecated. Co-authored-by: MaxIsJoe <34368774+MaxIsJoe@users.noreply.github.com>
|
I finally took time to look into #97855. The added error propagation seems interesting, but as is I don't see the immediate benefit, since it doesn't seem to actually be used anywhere to change behavior. So I would leave that to future work, which should also include WAV and likely non-audio formats if we want to change the logic of resource format loaders to do better error handling. |
akien-mga
force-pushed
the
mp3-load_from_file
branch
from
12f5b51 to
4396f8f
Compare
Move OggVorbis and MP3 loading code to their AudioStream class, matching how it's done for WAV.
The duplicate functions in ResourceImporterOggVorbis are now deprecated.
load_from_file()toAudioStreamMP3#97849.AudioStreamWAV#93831, harmonizing runtime loading code for all three audio formats (WAV, Ogg Vorbis, MP3).Tested with the following project (credits to @StayAtHomeDev for the guest appearance):
test-audiostream-loading.zip
gouda.mp4