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.

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

Make mpg123, fluidsynth and sndfile not link directly to lower-level audio playback libs #2471

Merged
merged 1 commit into from
Oct 26, 2023

Conversation

ankith26
Copy link
Member

@ankith26 ankith26 commented Sep 27, 2023

Depends on #2470 (it should be merged before this)

EDITED description to what this PR does now (taking starbucks suggestions):

Previously, mpg123, fluidsynth and sndfile would link directly to lower-level libraries alsa, pulseaudio and others and all these would get included in the wheels. But these links aren't required for audio playback

As a side effect of these changes, our manylinux wheels should get a bit lighter (maybe 1-2 mb)

To test this PR, we need to make sure that fluidsynth-based music and mp3 music still work as before on linux (which I can test) and mac (for which I need a volunteers help)

@ankith26 ankith26 force-pushed the ankith26-deps-updates-2 branch 5 times, most recently from 9419211 to 10783c3 Compare September 30, 2023 04:05
@ankith26 ankith26 marked this pull request as ready for review September 30, 2023 04:32
@ankith26 ankith26 requested a review from a team as a code owner September 30, 2023 04:32
@ankith26
Copy link
Member Author

ankith26 commented Sep 30, 2023

Here is the diff of the so files bundled in the wheels before and after this PR

libasound-e2127b4b.so.2.0.0
- libattr-4f2a9577.so.1.1.0
libbrotlicommon-c828db13.so.1.1.0
libbrotlidec-3b276a64.so.1.1.0
- libbz2-a273e504.so.1.0.6
- libcap-79733b59.so.2.22
- libdbus-1-5d678da9.so.3.14.14
- libdw-0-780627ce.176.so
- libelf-0-3d9de92a.176.so
- libFLAC-9c16284b.so.12.1.0
+ libFLAC-11b10a77.so.12.1.0
- libfluidsynth-78c3029a.so.3.2.2
+ libfluidsynth-13e12a66.so.3.2.2
libfreetype-ce7af30f.so.6.20.1
- libgcrypt-18957bae.so.11.8.2
libgomp-a34b3233.so.1.0.0
- libgpg-error-3f09c3c7.so.0.10.0
libharfbuzz-b5bd39f9.so.0.60821.0
- libjpeg-1944207f.so.62.4.0
+ libjpeg-034e96d7.so.62.4.0
- liblz4-af1653fb.so.1.8.3
- liblzma-004595ca.so.5.2.2
libmodplug-8c1b3b53.so.1.0.0
libmpg123-7a05540a.so.0.48.0
libogg-3f4a1572.so.0.8.5
libopus-4c85b165.so.0.9.0
libopusfile-df4005ff.so.0.4.5
- libpcre-9513aab5.so.1.2.0
libpng16-92b73642.so.16.40.0
libportmidi-bc2a3536.so.2.0.3
- libpulse-5b5d1e90.so.0.23.0
- libpulsecommon-14-9b36c523.2.so
- libpulse-simple-4ba3beb5.so.0.1.1
libSDL2-2-cdcf49c3.0.so.0.2600.5
libSDL2_image-2-d19463c5.0.so.0.2.3
libSDL2_mixer-2-705164cf.0.so.0.600.3
libSDL2_ttf-2-e58b8489.0.so.0.2000.2
- libselinux-0922c95c.so.1
libsharpyuv-5ed6246f.so.0.0.1
libsndfile-bfa200bd.so.1.0.37
- libsystemd-9be61d21.so.0.6.0
libtiff-61e60d59.so.6.0.2
libvorbis-ac399090.so.0.4.9
libvorbisenc-bb396c4a.so.2.0.12
libvorbisfile-8aced0b7.so.3.3.8
libwebp-3bee5bfa.so.7.1.8

@ankith26 ankith26 closed this Sep 30, 2023
@ankith26 ankith26 reopened this Sep 30, 2023
Copy link
Member

@MyreMylar MyreMylar left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@oddbookworm
Copy link
Member

Tested on mac
flac, ogg, mp3, and tests ran fine
ogx did not load
midi loaded as long as I specified a soundfont with SDL_SOUNDFONT, but crashed when I tried to restart the sound from the beginning
(see my reply to @ankith26 in the discord contributing channel for error logs)

@Starbuck5
Copy link
Member

Starbuck5 commented Oct 15, 2023

ogx did not load

Is this okay?

@ankith26 I believe all the audio output backends of mpg123 and fluidsynth should be turned off, because they're not relevant to SDL_mixer. This also means the SDL + libs build order doesn't need to be rearranged, so this PR will get simpler and shave even more off of dependency size?

@ankith26
Copy link
Member Author

Regarding flac-in-ogg support, I realised that SDL_mixer didn't support these on their end, so I have opened a PR there, lets see how it goes. If they do merge that PR, then the fix in this PR will start to work.

I will take starbucks suggestions to simplify the diffs in this PR, and move this flac fix stuff out too in the process

@ankith26 ankith26 changed the title Make mpg123 and fluidsynth use sdl2, fix flac-ogg Make mpg123, fluidsynth and sndfile not link directly to lower-level audio playback libs Oct 15, 2023
Copy link
Member

@Starbuck5 Starbuck5 left a comment

Choose a reason for hiding this comment

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

Looks good to me.

I really like how this turned out, better buildconfig -> smaller wheels.

I just checked and it looks like this takes the Linux wheels down 1.4 mb each
Much less significant effect on the Mac wheels, only down about 2 kb each

@MyreMylar MyreMylar added this to the 2.4.0 milestone Oct 26, 2023
@MyreMylar MyreMylar merged commit 049bbdb into main Oct 26, 2023
28 checks passed
@ankith26 ankith26 deleted the ankith26-deps-updates-2 branch October 28, 2023 03:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants