Compile and copy all vorbis libraries, fixes #7879 #9849
Conversation
|
Thank you for submitting a pull request! If this is your first PR, make sure to add yourself to AUTHORS. |
| @@ -37,6 +37,8 @@ int main(int argc, char* argv[]){ | |||
| return -1; | |||
| if (Mix_PlayChannel(-1, wave, 0) == -1) | |||
| return -1; | |||
| if (Mix_Init(MIX_INIT_OGG) == -1) | |||
| return -1; | |||
There was a problem hiding this comment.
Do this this test simply not work without this?
There was a problem hiding this comment.
Without this, the specific problem, namely that ogg support is not compiled into SDL2_Mixer, is never triggered. SDL2_Mixer initialises just fine without having ogg compiled in. This line of code forces an error if it is missing.
There was a problem hiding this comment.
But did the example just not play sound before?
There was a problem hiding this comment.
Oh, it's an example? I thought it was a test. The sound plays fine without this addition. Would you rather I create a new test specifically for ogg files?
There was a problem hiding this comment.
No it is a test. I just wonder what the purpose of the Mix_Init(MIX_INIT_OGG) line is if the code can play an ogg file even without this line? Is it just a way to verify that ogg support is built in? And if this code could play ogg files before this change, then surely ogg support was builtin previously?
I'm just trying to understand the state before this change. We could play ogg files? But this Mix_Init(MIX_INIT_OGG) would fail?
There was a problem hiding this comment.
I am sorry for the miscommunication. Let me try to clarify it:
- No, playing ogg files is impossible without
Mix_Init(MIX_INIT_OGG) - No, playing ogg files was impossible before, even with
Mix_Init(MIX_INIT_OGG), it would return -1.SDL_GetError()would then return a message saying that ogg support was not compiled in. - Playing wave files without
Mix_Init(MIX_INIT_OGG)has worked before and continues to work - With the changes made in this PR, this test, while not containing ogg files nor any attempt to play ogg files, will error when ogg support is not compiled in.
- The changes made in this PR ensure that ogg support is compiled in and that this test succeeds in its new form.
Does this clarify things? If you have more questions, I'll be happy to answer them.
There was a problem hiding this comment.
Got it. Sorry I thought this test played an ogg file.
There was a problem hiding this comment.
Sounds good, but please add a comment, "this should error because OGG support was not compiled in".
tools/ports/vorbis.py
Outdated
|
|
||
| return [shared.Cache.get(libname, create)] | ||
| libnamefile = ports.get_lib_name('libvorbisfile') | ||
| libnameenc = ports.get_lib_name('libvorbisenc') |
There was a problem hiding this comment.
in general we use _ to split out parts of local names (see elsewhere in this file, like source_path), please follow those conventions
| @@ -37,6 +37,8 @@ int main(int argc, char* argv[]){ | |||
| return -1; | |||
| if (Mix_PlayChannel(-1, wave, 0) == -1) | |||
| return -1; | |||
| if (Mix_Init(MIX_INIT_OGG) == -1) | |||
| return -1; | |||
There was a problem hiding this comment.
Sounds good, but please add a comment, "this should error because OGG support was not compiled in".
|
@kripken I could use some help with debugging the CI build. I cannot reproduce the error locally so I do not know what is going on there. Is it possible to get the config.log from the ci server somehow? |
|
One option is to do "re-run in ssh" (an option next to "rerun workflow" on the circle page). I did that, and there is an ssh link here: https://circleci.com/gh/emscripten-core/emscripten/205838 (however, not sure how permissions work there, so you may not be able to access it ). Here is that The error seems to be And it happens in the asm.js build, where linking is different. Perhaps you can reproduce this locally with |
|
Regarding the re-running of builds, I lack write permission, so I cannot use that functionality. Thank you for providing the config.log, my hunch is that the floor1.c file is included multiple times. But I do not know how. Regarding the fastcomp thing, I do not understand how I can use it while pointing at my own branch. Is it the emscripten/emscripten-fastcomp branch? If so, why does it not contain a tools/ports directory? Can you explain to me how I can find the source code to emscript-fastcomp and then modify it similarly to how I did with emscripten-core? |
tools/ports/vorbis.py
Outdated
| if library == lib_name_file or library == lib_name: | ||
| excluded_files.append('vorbisenc') | ||
| if library == lib_name_enc or library == lib_name: | ||
| excluded_files.append('vorbisfile') |
There was a problem hiding this comment.
I think the original code include the enc and the file stuff in once big library.
If you want to split those out then don't you need to exlude both the enc and the file sources from the core library? and also exclude the core library files from both the enc and the file libraries.
I'm not sure if switching to some kind of whitelist might help?
There was a problem hiding this comment.
I am unsure what you mean here.
When compiling vorbis linux usage, the following libraries are compiled from the following files:
- vorbis.a: mdct.c smallft.c block.c envelope.c window.c lsp.c
lpc.c analysis.c synthesis.c psy.c info.c
floor1.c floor0.c
res0.c mapping0.c registry.c codebook.c sharedbook.c
lookup.c bitrate.c
envelope.h lpc.h lsp.h codebook.h misc.h psy.h
masking.h os.h mdct.h smallft.h highlevel.h
registry.h scales.h window.h lookup.h lookup_data.h
codec_internal.h backends.h bitrate.h - vorbisenc.a: links vorbis.a and adds vorbisenc.c.
- vorbisfile.a: links vorbis.a and adds vorbisfile.c
Compiling all the source files used in vorbis again for vorbisenc/vorbisfile has the same end-result as linking vorbis.a.
There was a problem hiding this comment.
Normally each library contains a unique set of files without overlap. Indeed this seems to be how vorbis works, at least on my desktop system:
$ ar t /usr/lib/x86_64-linux-gnu/libvorbis.a
mdct.o
smallft.o
block.o
envelope.o
window.o
lsp.o
lpc.o
analysis.o
synthesis.o
psy.o
info.o
floor1.o
floor0.o
res0.o
mapping0.o
registry.o
codebook.o
sharedbook.o
lookup.o
bitrate.o
$ ar t /usr/lib/x86_64-linux-gnu/libvorbisfile.a
vorbisfile.o
$ ar t /usr/lib/x86_64-linux-gnu/libvorbisenc.a
vorbisenc.o
The good news is that those second two libraries only have a single file in them, so it should be trivial to build them without the need for whitelists/blacklists.
Why this matters for fastcomp: Normally duplicating object between libraries is fine due to the way .a files are linked, but the the ports system is setup to create giant object files rather than archive files when building with fastcomp. Basically it builds libvorbis.bc not libvorbis.a.
See:
emscripten/tools/system_libs.py
Line 86 in 0d5dc73
There was a problem hiding this comment.
So it seems. I must've made an oversight somewhere. I'll correct the PR.
|
The latest changes did not seem to change the CI output. If the config.log file is the same as last time, I am stumped. |
|
So on deeper inspection we have now discovered the current status quo is that all three vorbis libraries are compiled as one under emscripten. This can be easily seen via: So if a user includes libvorbis via If you are ok using the |
|
The problem with the status quo is that sdl2_mixer requires libvorbisfile to exist, alongside libvorbis itself. This can be seen in bug #7879, as mentioned in the title of the PR. Personally, now that I have a fork of emscripten that compiles sdl2_mixer with ogg support, I can continue with my personal project. However, I am not the only one running into the problem. As such, having one fat binary/maintaining the status quo, is IMO not the way forward. |
|
Maybe a better approach without modifying the emscripten source code too much, is to create a separate tools/ports/vorbisfile.py and tools/ports/vorbisenc.py? Adding the USE_VORBISFILE and USE_VORBISENC flags? |
|
Oh, I see. Its becaus we are using configure to build SDL_mixer. Lets stop doing that. For one thing its can't work on windows so its not portable. |
|
That would be another way, yes. I don't quite understand the approach taken so far gives errors, but I can understand the portability problems. I'll modify sdl2_mixer instead. |
|
The only outstanding things for sdl2_mixer are the HAVE_SIGNAL_H and HAVE_SETBUF defines. Currently, those are not set and yet it still appears to function correctly under linux. Aside from that, it seems that asmjs once again rears its head :groan: |
|
It is getting late here, I will continue this tomorrow. |
|
Nice work! If fastcomp is a significant burden here, one option is to ifdef on the backend, so that we run the old code for fastcomp, and the new for upstream. Then when we remove fastcomp, we'd just remove that old code. |
|
I don't know what you mean with removing a flag, as the entire configure script is not used anymore. Morever, native_midi is on the exclude_dir list, so it is not included at all. This is the output of Which reminded me that I need to add the timidity directory to the excludes. After which the output becomes |
|
I see, thanks! So there's no expected change for users, aside from properly including all the necessary files now so that SDL2_mixer can use OGG? Just making sure we don't need to mention anything in the changelog - I'll land this and mention that. |
|
That is correct! |
…scripten-core#9849) Vorbis actually consists of three libraries: vorbis.a, vorbisfile.a and vorbisenc.a. This pull request modifies the vorbis port to build and cache these files. Fixes emscripten-core#7879
Vorbis actually consists of three libraries: vorbis.a, vorbisfile.a and vorbisenc.a.
This pull request modifies the vorbis port to build and cache these files.
Fixes #7879