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

DrFlacAudioImporter: Add 24/32 bit support #20

Closed
wants to merge 4 commits into
base: master
from

Conversation

3 participants
@ghost

ghost commented Oct 5, 2016

DrFlacAudioImporter can now load any format supported by the reference FLAC encoder, and correctly loads them to the same specification as WavAudioImporter.

@coveralls

This comment has been minimized.

coveralls commented Oct 5, 2016

Coverage Status

Coverage decreased (-0.4%) to 88.188% when pulling 60ee64e on Alicemargatroid:drflac into de1fb52 on mosra:master.

@mosra

I'm happy to merge once the Emscripten tests are fixed. I could also investigate those errors myself, but since I'm extremely busy right now, it may take a bit more time...

@ghost

This comment has been minimized.

ghost commented Oct 6, 2016

@mosra It looks like they are caused by my FLAC files being too big? Any idea on that?

@mosra

This comment has been minimized.

Owner

mosra commented Oct 6, 2016

It fails on the quad16 test for which the test file is 60 kB, I doubt that the decompression would inflate the file to over 16 MB... I also doubt this would be due to some endianness issue (a value interpreted as some gigabytes instead of kilobytes) -- that would blow up also on desktop.

I suspect this might be caused by unaligned reads/writes (yes, of all places, that's actually a thing here) -- you are allocating a char array that theoretically might get allocated on a odd address and then reinterpreting it as an int so then there might be some suspicious behavior. It might be also possible that the DrFlac is doing some unaligned reads -- majority of platforms does not care about unaligned reads anymore so such bug might get unnoticed until now.

Can you try e.g. verifying that the allocated array is aligned on 4 byte boundary or that the sample count is sane?

Also, maybe for better sleep it might be wise to actually check the returned array sizes in the tests -- now I see just checking the first N bytes, but not caring how the returned array is actually big. (Yes, I'm also guilty in this, my poor initial WavAudoImporter tests also didn't check for that..)

@ghost

This comment has been minimized.

ghost commented Oct 6, 2016

@mosra I'll give it a look, unaligned looks like a good place to start.

As well, I think a lot of Magnum code is not compatible with strict aliasing; you aren't really supposed to reinterpret_cast pointers to anything but char*. Worth looking into as a possible problem throughout the source code: http://blog.regehr.org/archives/1307

@coveralls

This comment has been minimized.

coveralls commented Oct 7, 2016

Coverage Status

Coverage decreased (-0.4%) to 88.188% when pulling 2ae27c4 on Alicemargatroid:drflac into de1fb52 on mosra:master.

@coveralls

This comment has been minimized.

coveralls commented Oct 7, 2016

Coverage Status

Coverage decreased (-0.4%) to 88.188% when pulling 1167f26 on Alicemargatroid:drflac into de1fb52 on mosra:master.

@mosra

This comment has been minimized.

Owner

mosra commented Oct 7, 2016

Strict aliasing -- yes, I know about it. I tried to fight it with Corrade::Utility::bitCast() at some point (inspired by some utility function that Chromium uses to avoid strict aliasing issues), but that's such a huge workaround (it involves memcpy) that I gave up on it. Another "well-known" workaround is to have an union, save to one field and then read from another. But The Standard says it's undefined behavior, so ...

Counter-argument from Linus: https://lkml.org/lkml/2003/2/26/158 ;)

Also, a lot of third-party code that is used here (e.g. the stb_* libraries) explicitly claims that it doesn't work with strict aliasing and they have no plans in supporting that.

So maybe I should just document whether the code supports it or not. It's just a problem of GCC if I remember correctly, no other compiler (even Clang) is doing such crazy optimizations.

@ghost

This comment has been minimized.

ghost commented Oct 7, 2016

@mosra You actually have it backwards; every compiler except Clang is doing crazy optimizations with it! LLVM team wrote a little blog post about it and other UB that might be informative: http://blog.llvm.org/2011/05/what-every-c-programmer-should-know.html

Ironically, the union trick while non-standard is supported everywhere, while the memcpy fix while standard is pretty awful, I agree. I think because the union trick is so well tolerated because of Linus (it is used all the time in Linux). I'd use that for bitcast; it's also well used by a very popular and archetypical library: fastest possible delegates.

There is a flag in most compilers to disable strict aliasing; might be worth looking into. Looks like this problem with Travis is due to too large of file sizes though; DrFlac increases the size of the binaries, because it loads everything as 32-bit PCM.

@mosra

This comment has been minimized.

Owner

mosra commented Oct 7, 2016

Ah, okay, so I did not remembered correctly :) In my limited world view, I see just GCC doing it, Clang not and MSVC also not, no clue about other platforms. GCC has -fno-strict-aliasing that I desperately wanted to enable when fighting some annoying bugs, but didn't do it yet.

Re the too large files: so if you pass the suggested flag to Emscripten, do the test pass for you at least locally? If yes, then I might just enable that this for the particular test and be done with it.

@mosra mosra added the improvement label Oct 7, 2016

@coveralls

This comment has been minimized.

coveralls commented Oct 8, 2016

Coverage Status

Coverage decreased (-0.2%) to 88.311% when pulling 5f7f024 on Alicemargatroid:drflac into de1fb52 on mosra:master.

@ghost

This comment has been minimized.

ghost commented Oct 8, 2016

@mosra Yeah, MSVC only recently started taking advantage of some of those rules, so you probably haven't hit them too much yet.

The tests pass for me on Emscripten; as well, I have worked to make the files smaller, and can shrink the size of DrWavAudioImporter's files once this is accepted. We could also just disable some of the tests for DrFlac to avoid the issue.

@mosra

This comment has been minimized.

Owner

mosra commented Oct 15, 2016

Cherry-picked in 9db2922. Sorry it took so long.

I stripped the quad16 and surround51Channel24 files even more, limiting them to 50k samples. The quad16 file had originally around 5M samples per channel, which after expansion to 32bit was almost 100 MB of memory and I didn't want to increase the Emscripten memory heap size that high just for that one test case.

@mosra mosra closed this Oct 15, 2016

@mosra mosra added this to the 2018.02 milestone Feb 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment