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

Add FLAC importing via dr_flac #18

Closed
wants to merge 6 commits into
base: master
from

Conversation

4 participants
@ghost

ghost commented Sep 10, 2016

Exactly as it says on the tin, very similar to StbVorbisImporter, provides FLAC loading via dr_flac.

dr_flac can also load FLAC that is stored in an OGG container, but I don't know how to provide that rather complex plugin dependency, so I've only added support if the file extension is ".flac".

While WAV provides the same use case of lossless audio, FLAC is considerably smaller and faster to decode, which could be important on mobile devices.

@coveralls

This comment has been minimized.

coveralls commented Sep 10, 2016

Coverage Status

Coverage decreased (-0.03%) to 88.889% when pulling f2f0f53 on Alicemargatroid:master into ac7a9e3 on mosra:master.

@coveralls

This comment has been minimized.

coveralls commented Sep 10, 2016

Coverage Status

Coverage decreased (-0.03%) to 88.889% when pulling 47025e0 on Alicemargatroid:master into ac7a9e3 on mosra:master.

@@ -53,6 +53,8 @@ void AnyImporter::doOpenFile(const std::string& filename) {
plugin = "VorbisAudioImporter";
else if(Utility::String::endsWith(filename, ".wav"))
plugin = "WavAudioImporter";
else if(Utility::String::endsWith(filename, ".flac"))
plugin = "FlacAudioImporter";

This comment has been minimized.

@mosra

This comment has been minimized.

@LB--

LB-- Sep 11, 2016

Contributor

Looks like the indentation is mismatched

This comment has been minimized.

@ghost

ghost Sep 11, 2016

@LB-- Looks like that happened in a few locations, I'll patch it shortly. My nano instance isn't properly configured yet, and my eyes aren't what they used to be!

return;
}
_handle = (void*)handle;

This comment has been minimized.

@mosra

mosra Sep 11, 2016

Owner

If the opening fails on "unsupported channel count" below, the file is not considered being opened (doIsOpened() returns false) and thus doClose() would never get called, causing the _handle to leak.

I would save the handle only after everything is successful (i.e. along with saving the _data below) and not here. Then also call drflac_close() explicitly after the Error() call below.

This plugins provides `VorbisAudioImporter`, but note that this plugin doesn't
have complete support for all format quirks and the performance might be worse
than when using plugin dedicated for given format.

This comment has been minimized.

@mosra

mosra Sep 11, 2016

Owner

I think you forgot to update some parts here (still referencing OGG/vorbis/stb in some parts).

Containers::Array<char> _data;
Buffer::Format _format;
UnsignedInt _frequency;
void* _handle;

This comment has been minimized.

@mosra

mosra Sep 11, 2016

Owner

Could you say

struct drflac* _handle;

here? It's just a pointer so a forward declaration is enough, and then you also avoid those ugly void* casts in the implementation. Also the indentation is a tiny bit off ;)

@mosra

This comment has been minimized.

Owner

mosra commented Sep 11, 2016

Oh cool! This is great :)

Regarding test coverage -- would it be possible for you to create also some minimal test case (just some minimal one-second file, only verifying that the third party code is able to execute on all platforms without crashing, nothing fancy) and then enable the plugin in all package/ci/travis-*.sh and package/ci/appveyor-*.bat CI configurations?

I would be very happy to merge it afterwards.

@mosra mosra added feature labels Sep 11, 2016

_frequency = frequency;
Containers::Array<char> tempData{reinterpret_cast<char*>(decodedData), size_t(samples*numChannels*2)};

This comment has been minimized.

@ghost

ghost Sep 11, 2016

@mosra Question I wasn't easily able to resolve myself; does Containers::Array<char> copy decodedData, or is there a way to make it do so? Then we could get rid of the entire _handle, and I wouldn't have to manage it at all.

This comment has been minimized.

@mosra

mosra Sep 11, 2016

Owner

Yeah, that would be even better! Containers::Array doesn't copy the data in any way (design decision), so you need to use std::copy() or something like in the doData() below.

This comment has been minimized.

@ghost

ghost Sep 11, 2016

Just as I submit different changes! Let me rewrite it that way.

@coveralls

This comment has been minimized.

coveralls commented Sep 11, 2016

Coverage Status

Coverage decreased (-0.03%) to 88.889% when pulling 24bc693 on Alicemargatroid:master into ac7a9e3 on mosra:master.

@ghost

This comment has been minimized.

ghost commented Sep 11, 2016

@mosra I think the last push should resolve all issues besides test cases.

I will attempt to write some small test cases as soon as possible, hopefully by the end of the day.

@coveralls

This comment has been minimized.

coveralls commented Sep 11, 2016

Coverage Status

Coverage decreased (-0.03%) to 88.889% when pulling cba090c on Alicemargatroid:master into ac7a9e3 on mosra:master.

@ghost

This comment has been minimized.

ghost commented Sep 12, 2016

@mosra Three tests added, test FLAC's are licensed CC0/public domain.

How do I enable the tests in our CI? Think that is the last step.

@coveralls

This comment has been minimized.

coveralls commented Sep 12, 2016

Coverage Status

Coverage decreased (-0.03%) to 88.889% when pulling f605c93 on Alicemargatroid:master into ac7a9e3 on mosra:master.

@mosra

This comment has been minimized.

Owner

mosra commented Sep 12, 2016

Like this:

diff --git a/package/ci/appveyor-desktop-mingw.bat b/package/ci/appveyor-desktop-mingw.bat
index 473377a..3f7212a 100644
--- a/package/ci/appveyor-desktop-mingw.bat
+++ b/package/ci/appveyor-desktop-mingw.bat
@@ -67,6 +67,7 @@ cmake .. ^
     -DWITH_ANYIMAGEIMPORTER=ON ^
     -DWITH_ANYSCENEIMPORTER=ON ^
     -DWITH_DDSIMPORTER=ON ^
+    -DWITH_DRFLACAUDIOIMPORTER=ON ^
     -DWITH_FREETYPEFONT=OFF ^
     -DWITH_HARFBUZZFONT=OFF ^
     -DWITH_JPEGIMPORTER=ON ^
diff --git a/package/ci/appveyor-desktop.bat b/package/ci/appveyor-desktop.bat
index fe71d50..24fa4a1 100644
--- a/package/ci/appveyor-desktop.bat
+++ b/package/ci/appveyor-desktop.bat
@@ -66,6 +66,7 @@ cmake .. ^
     -DWITH_ANYIMAGEIMPORTER=ON ^
     -DWITH_ANYSCENEIMPORTER=ON ^
     -DWITH_DDSIMPORTER=ON ^
+    -DWITH_DRFLACAUDIOIMPORTER=ON ^
     -DWITH_FREETYPEFONT=OFF ^
     -DWITH_HARFBUZZFONT=OFF ^
     -DWITH_JPEGIMPORTER=ON ^
diff --git a/package/ci/appveyor-rt.bat b/package/ci/appveyor-rt.bat
index 4dfc306..ea3b44a 100644
--- a/package/ci/appveyor-rt.bat
+++ b/package/ci/appveyor-rt.bat
@@ -78,6 +78,7 @@ cmake .. ^
     -DWITH_ANYIMAGEIMPORTER=ON ^
     -DWITH_ANYSCENEIMPORTER=ON ^
     -DWITH_DDSIMPORTER=ON ^
+    -DWITH_DRFLACAUDIOIMPORTER=OFF ^
     -DWITH_FREETYPEFONT=OFF ^
     -DWITH_HARFBUZZFONT=OFF ^
     -DWITH_JPEGIMPORTER=OFF ^
diff --git a/package/ci/travis-desktop.sh b/package/ci/travis-desktop.sh
index 06cc278..405203c 100755
--- a/package/ci/travis-desktop.sh
+++ b/package/ci/travis-desktop.sh
@@ -50,6 +50,7 @@ cmake .. \
     -DWITH_ANYSCENEIMPORTER=ON \
     -DWITH_COLLADAIMPORTER=$WITH_COLLADAIMPORTER \
     -DWITH_DDSIMPORTER=ON \
+    -DWITH_DRFLACAUDIOIMPORTER=ON \
     -DWITH_FREETYPEFONT=ON \
     -DWITH_HARFBUZZFONT=ON \
     -DWITH_JPEGIMPORTER=ON \
diff --git a/package/ci/travis-emscripten.sh b/package/ci/travis-emscripten.sh
index 014a7ec..c85967c 100755
--- a/package/ci/travis-emscripten.sh
+++ b/package/ci/travis-emscripten.sh
@@ -75,6 +75,7 @@ cmake .. \
     -DWITH_ANYSCENEIMPORTER=ON \
     -DWITH_COLLADAIMPORTER=OFF \
     -DWITH_DDSIMPORTER=ON \
+    -DWITH_DRFLACAUDIOIMPORTER=ON \
     -DWITH_FREETYPEFONT=OFF \
     -DWITH_HARFBUZZFONT=OFF \
     -DWITH_JPEGIMPORTER=OFF \
diff --git a/package/ci/travis-ios-simulator.sh b/package/ci/travis-ios-simulator.sh
index 6694827..d12647b 100755
--- a/package/ci/travis-ios-simulator.sh
+++ b/package/ci/travis-ios-simulator.sh
@@ -78,6 +78,7 @@ cmake .. \
     -DWITH_ANYSCENEIMPORTER=ON \
     -DWITH_COLLADAIMPORTER=OFF \
     -DWITH_DDSIMPORTER=ON \
+    -DWITH_DRFLACAUDIOIMPORTER=ON \
     -DWITH_FREETYPEFONT=OFF \
     -DWITH_HARFBUZZFONT=OFF \
     -DWITH_JPEGIMPORTER=OFF \
@coveralls

This comment has been minimized.

coveralls commented Sep 13, 2016

Coverage Status

Coverage decreased (-0.4%) to 88.547% when pulling 45774a1 on Alicemargatroid:master into ac7a9e3 on mosra:master.

@mosra

This comment has been minimized.

Owner

mosra commented Sep 13, 2016

Squased into one commit and merged into master in 87d8160. I did some additional boilerplate updates so it is now also available through FindMagnumPlugins.cmake.

Thanks a lot for this :)

@mosra mosra closed this Sep 13, 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