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 Vorbis/Ogg Importer based on stb_vorbis #8

Closed
wants to merge 6 commits into
base: master
from

Conversation

2 participants
@Squareys
Contributor

Squareys commented Oct 1, 2015

Hi @mosra !

Stb is really something, this went pretty quick! The importer only supports 16 bit mono/stereo, no streaming, but here is ogg files support for magnum.

stb_vorbis could also support float data, which will have to wait until audio-extension-support is done, though.

It would be awesome if you could have a quick look at this (it's basically only 10 lines of code or so, everything else is pretty much identical to the wav importer).

Greetings,
Squareys

StbVorbisImporter::StbVorbisImporter(PluginManager::AbstractManager& manager, std::string plugin): AbstractImporter(manager, std::move(plugin)) {}
StbVorbisImporter::~StbVorbisImporter() { close(); }

This comment has been minimized.

@mosra

mosra Oct 1, 2015

Owner

The destructor is not needed (equivalent code will be generated implicitly).

(Yeah, I might still have it in WavAudioImporter)

Short* decodedData;
Int samples = stb_vorbis_decode_memory(reinterpret_cast<const UnsignedByte*>(data.data()), data.size(), &numChannels, &frequency, &decodedData);
_data = Containers::Array<char>(reinterpret_cast<char*>(decodedData), samples*numChannels*2);

This comment has been minimized.

@mosra

mosra Oct 1, 2015

Owner

I would save the array into local variable and then move it into _data only at the end when everything succeeded (so you don't have to call _data = nullptr on each error exit).

This comment has been minimized.

@Squareys

Squareys Oct 1, 2015

Contributor

Sometimes there is an error though and decodedData points to valid heap data. This is the case for unsupported channel counts for example. Should I then just delete the data?

This comment has been minimized.

@mosra

mosra Oct 1, 2015

Owner

It will be done implicitly when the local Array instance goes out of scope, no?

If I remember correctly, memory in stb_* is allocated using malloc so you can't use delete. See e.g. fd9726c for solution. Consult the documentation for details (might be done differently in stb_vorbis, I don't know).

This comment has been minimized.

@Squareys

Squareys Oct 1, 2015

Contributor

Ah! Now I get it, I had missunderstood, sorry!

CORRADE_COMPARE(data[0], '\xcd');
CORRADE_COMPARE(data[1], '\x0a');
CORRADE_COMPARE(data[2], '\x2b');
CORRADE_COMPARE(data[3], '\x0a');

This comment has been minimized.

@mosra

mosra Oct 1, 2015

Owner

Including <Corrade/TestSuite/Compare/Container.h> and doing the following should work too, I think:

CORRADE_COMPARE_AS(importer.data(), 
    Containers::Array<char>::from('\xcd', ...), 
    TestSuite::Compare::Container);

This comment has been minimized.

@Squareys

Squareys Oct 1, 2015

Contributor

Alright, of course. I think I followed the WavImporter code too blindly :)

This comment has been minimized.

@mosra

mosra Oct 1, 2015

Owner

And I should clean it up :)

@mosra

This comment has been minimized.

Owner

mosra commented Oct 1, 2015

One important thing re the naming -- the class and file names are okay, it's Audio::StbVorbisImporter, put the plugin (the library and thus also all CMake variables) should be named StbVorbisAudioImporter to differentiate it from scene/image importers from Trade namespace. Sorry about the chaos :)

@Squareys Squareys force-pushed the Squareys:stb-vorbis-importer branch from 2295501 to 435f966 Oct 1, 2015

external: Add stb_vorbis
Base of the future StbVorbisImporter.

Signed-off-by: Squareys <Squareys@googlemail.com>

@Squareys Squareys force-pushed the Squareys:stb-vorbis-importer branch from 435f966 to 33988f7 Oct 1, 2015

@Squareys

This comment has been minimized.

Contributor

Squareys commented Oct 1, 2015

@mosra Alright, I fixed all the aforementioned issues and did a fresh rebase onto master :)

#include "MagnumPlugins/StbVorbisImporter/StbVorbisImporter.h"
CORRADE_PLUGIN_REGISTER(StbVorbisImporter, Magnum::Audio::StbVorbisImporter,

This comment has been minimized.

@mosra

mosra Oct 2, 2015

Owner

This needs to be CORRADE_PLUGIN_REGISTER(StbVorbisAudioImporter, Magnum::Audio::StbVorbisImporter, ... too to make dynamic plugin loading work properly.

This comment has been minimized.

@Squareys

Squareys Oct 3, 2015

Contributor

Oh, and the include is wrong, too, right?

@@ -79,6 +79,8 @@ By default no plugins are built and you need to select them manually:
plugin.
- `WITH_STBPNGIMAGECONVERTER` -- @ref Trade::StbPngImageConverter "StbPngImageConverter"
plugin.
- `WITH_STBVORBISAUDIOIMPORTER` -- @ref Audio::StbVorbisImporter "StbVorbisAudioImporter" plugin.
plugin.

This comment has been minimized.

@mosra

mosra Oct 2, 2015

Owner

Hey, you have plugin. twice in here ;)

include_directories(BEFORE ${CMAKE_CURRENT_BINARY_DIR})
corrade_add_test(StbVorbisImporterTest StbVorbisImporterTest.cpp LIBRARIES MagnumStbVorbisImporterTestLib)

This comment has been minimized.

@mosra

mosra Oct 2, 2015

Owner

Just for consistency with library/plugin name I would name this StbVorbisAudioImporterTest and MagnumStbVorbisAudioImporterTestLib, too.

This comment has been minimized.

@Squareys

Squareys Oct 3, 2015

Contributor

Does this include renaming the StbVorbisImporterTest.cpp file?

Short* decodedData;
Int samples = stb_vorbis_decode_memory(reinterpret_cast<const UnsignedByte*>(data.data()), data.size(), &numChannels, &frequency, &decodedData);
Containers::Array<char> tempData = Containers::Array<char>(reinterpret_cast<char*>(decodedData), samples*numChannels*2);

This comment has been minimized.

@mosra

mosra Oct 2, 2015

Owner

Maybe you missed my earlier comment, the decodedData pointer needs to be freed using free() instead of delete to avoid undefined behavior. Solution is to use custom deleter instead of the default provided in Containers::Array:

Containers::Array<char> tempData{reinterpret_cast<char*>(decodedData), samples*numChannels*2,
    [](char* data, std::size_t) { std::free(data); }};

This comment has been minimized.

@Squareys

Squareys Oct 3, 2015

Contributor

Yeah, I missed that, sorry.

@Squareys

This comment has been minimized.

Contributor

Squareys commented Oct 3, 2015

@mosra Thanks for the feedback! I fixed-up the issues you pointed out. If there isn't anything else, I will rebase.

@mosra

This comment has been minimized.

Owner

mosra commented Oct 5, 2015

Awesome. Will merge as soon as possible. One more thing, could you please enable the new plugin in the CIs (package/ci/appveyor.yml, package/ci/travis.yml)? Thank you.

@Squareys

This comment has been minimized.

Contributor

Squareys commented Oct 5, 2015

Will do as soon as possible (tonight probably).

Squareys added some commits Sep 30, 2015

StbVorbisAudioImporter: Add cmake project files.
Signed-off-by: Squareys <Squareys@googlemail.com>
Doc: Add StbVorbisAudioImporter.
Signed-off-by: Squareys <Squareys@googlemail.com>

@Squareys Squareys force-pushed the Squareys:stb-vorbis-importer branch 2 times, most recently from b49e7ef to e7770b5 Oct 5, 2015

Squareys added some commits Oct 1, 2015

StbVorbisAudioImporter: Add initial implementation.
Signed-off-by: Squareys <Squareys@googlemail.com>
StbVorbisAudioImporter: Add test and test data.
Signed-off-by: Squareys <Squareys@googlemail.com>
ci: Enable StbVorbisAudioImporter for Appveyor and Travis builds.
Signed-off-by: Squareys <Squareys@googlemail.com>

@Squareys Squareys force-pushed the Squareys:stb-vorbis-importer branch from e7770b5 to e0b10a5 Oct 5, 2015

@Squareys

This comment has been minimized.

Contributor

Squareys commented Oct 5, 2015

Alright, everything fixed for CIs, which build properly now. A thing I think might be worth mentioning: I had weird errors on all the CIs, invalid pointers or whatever.

First I thought they were caused by the custom deleter possibly not being moved in the array move constructor, but in the end moving the creation of the decoded data array, after checking for sample counts of -1 and -2 (which would have resulted in negative sizes casted to size_t), solved the issues.

All in all, this is ready for merge from my side once again :)

@mosra

This comment has been minimized.

Owner

mosra commented Oct 10, 2015

Merged, thanks a lot.

(I did a whitespace cleanup in stb_vorbis.c so the following commits were rebased. Hope that's okay.)

@mosra mosra closed this Oct 10, 2015

@Squareys

This comment has been minimized.

Contributor

Squareys commented Oct 10, 2015

so the following commits were rebased. Hope that's okay.

Sure, no problem!

@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