Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add MP3 import and playback support #43007

Merged
merged 1 commit into from
Dec 7, 2020
Merged

Add MP3 import and playback support #43007

merged 1 commit into from
Dec 7, 2020

Conversation

DeleteSystem32
Copy link
Contributor

@DeleteSystem32 DeleteSystem32 commented Oct 22, 2020 •
edited
Loading

Support for MP3 audio streams, using the minimp3 decoder. Adresses this feature proposal: godotengine/godot-proposals#85

Tested to be working with mono, stereo, and joint stereo encoded files.

Tested and working on both Windows and Linux.

@Chaosus Chaosus added this to the 4.0 milestone Oct 22, 2020
@akien-mga akien-mga requested a review from reduz October 22, 2020 14:43
@DeleteSystem32
Copy link
Contributor Author

DeleteSystem32 commented Oct 22, 2020 •
edited
Loading

Turns out, I should have tested this on other platforms than Windows. I'm fixing it ASAP. Done.

EDIT: Minimp3 uses variable shadowing, which needs to be specifically allowed through a compiler flag when compiling for Linux. I'm not sure if that's a good idea, but it seems to be working.

@DeleteSystem32 DeleteSystem32 marked this pull request as draft October 22, 2020 15:22
@akien-mga akien-mga changed the title Mp3 support MP3 support Oct 22, 2020
@akien-mga akien-mga changed the title MP3 support Add MP3 import and playback support Oct 22, 2020
@DeleteSystem32 DeleteSystem32 force-pushed the mp3-support branch 6 times, most recently from 585fe0a to 18f811c Compare October 23, 2020 00:09
@DeleteSystem32 DeleteSystem32 marked this pull request as ready for review October 23, 2020 00:22
@DeleteSystem32 DeleteSystem32 marked this pull request as draft October 29, 2020 14:32
@DeleteSystem32 DeleteSystem32 marked this pull request as ready for review October 29, 2020 15:02
akien-mga
akien-mga reviewed Dec 3, 2020
View reviewed changes
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good to me, just some nitpicks, and a rebase is necessary as some core headers were moved recently in the master branch (so you'll have to propagate this in your own includes).

modules/minimp3/resource_importer_mp3.cpp Outdated Show resolved Hide resolved
modules/minimp3/resource_importer_mp3.cpp Outdated Show resolved Hide resolved
modules/minimp3/resource_importer_mp3.h Outdated Show resolved Hide resolved
modules/minimp3/SCsub Outdated

# Godot's own source files
env_minimp3.add_source_files(env.modules_sources, "*.cpp")
env_minimp3.disable_warnings()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The warnings should only be disabled for thirdparty code, not for Godot files.
Here the thirdparty code is only headers so it's a bit tricky. I'd suggest trying without disabling warnings, and if some warnings are raised in the headers, then we'll have to use -isystem to include them to the include path.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I wasn't sure about that one. I changed it to use -isystem now.

modules/minimp3/audio_stream_mp3.cpp Outdated Show resolved Hide resolved
modules/minimp3/audio_stream_mp3.h Outdated Show resolved Hide resolved
@DeleteSystem32 DeleteSystem32 force-pushed the mp3-support branch 3 times, most recently from dc4ef11 to 8e6d6c7 Compare December 3, 2020 16:44
@akien-mga akien-mga merged commit d32878b into godotengine:master Dec 7, 2020
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

If you want to prepare a backport of this feature to the 3.2 branch, I think that'd be useful for many Godot users.

@akien-mga akien-mga mentioned this pull request Dec 7, 2020
Closed
@DeleteSystem32 DeleteSystem32 deleted the mp3-support branch December 7, 2020 12:01
@Calinou Calinou mentioned this pull request Dec 7, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Xrayez Xrayez Xrayez left review comments

@akien-mga akien-mga akien-mga approved these changes

@reduz reduz reduz approved these changes

Assignees
No one assigned
Labels
feature proposal topic:audio topic:import
Projects
None yet
Milestone
4.0
Development

Successfully merging this pull request may close these issues.
5 participants
@DeleteSystem32 @akien-mga @reduz @Xrayez @Chaosus