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

Added optional dependency to libsndfile and libspeexdsp #908

Merged
merged 10 commits into from Jun 17, 2016

Conversation

Projects
None yet
3 participants
@ChristianBreitwieser
Member

ChristianBreitwieser commented Jun 11, 2016

Hello,

As explained before in the forum these are two additions to the audio layer of the player:

  • decoder_libsndfile to enable wav playback without sdl (depends on libsndfile)
  • audio_resampler to enable arbitrary resampling (depends on libspeexdsp, or libsamplerate)

The AudioResampler is meant to wrap any AudioDecoder and intersects if SetFormat or SetPitch are called.

I haven't changed anything in the sdl audio files so these changes only affect background music as sound effects don't use the AudioDecoder class.

@Ghabry

This comment has been minimized.

Show comment
Hide comment
@Ghabry

Ghabry Jun 11, 2016

Member

Jenkins: Test this please

For Sound effects it's fine. SDL is able to play most of them. We only get resampler issues when streaming data because sounds are played in one go but no music.

Thanks, looks really great. Much better than the code I have to review at my company 👍 Going to test it :)

This will need adjustments for Windows (Visual Studio) but I will do this with another pull request after merging this one because it's ugly ;)

Member

Ghabry commented Jun 11, 2016

Jenkins: Test this please

For Sound effects it's fine. SDL is able to play most of them. We only get resampler issues when streaming data because sounds are played in one go but no music.

Thanks, looks really great. Much better than the code I have to review at my company 👍 Going to test it :)

This will need adjustments for Windows (Visual Studio) but I will do this with another pull request after merging this one because it's ugly ;)

@Ghabry

This comment has been minimized.

Show comment
Hide comment
@Ghabry

Ghabry Jun 11, 2016

Member

Works perfect in SDL, even with nonsense rates like 10000 or 23456

Only one thing: In AudioResampler.cpp you have

/************************
*                                                                *
*************************/

and

//--------------------------/

blocks. This doesn't match the coding style (and you only use it in one of your files)

Member

Ghabry commented Jun 11, 2016

Works perfect in SDL, even with nonsense rates like 10000 or 23456

Only one thing: In AudioResampler.cpp you have

/************************
*                                                                *
*************************/

and

//--------------------------/

blocks. This doesn't match the coding style (and you only use it in one of your files)

@ChristianBreitwieser

This comment has been minimized.

Show comment
Hide comment
@ChristianBreitwieser

ChristianBreitwieser Jun 12, 2016

Member

Thanks for the kind reply!
The block seperating comments are removed.

Member

ChristianBreitwieser commented Jun 12, 2016

Thanks for the kind reply!
The block seperating comments are removed.

@carstene1ns

This comment has been minimized.

Show comment
Hide comment
@carstene1ns

carstene1ns Jun 14, 2016

Member

btw. is there a reason you guarded only the internal stuff from the libraries in audio_resampler.h and decoder_libsndfile.h? It will not link without the libs (if using the functions) anyway, so it would make sense to empty the files when the preprocessor macros are not defined imo.

Member

carstene1ns commented Jun 14, 2016

btw. is there a reason you guarded only the internal stuff from the libraries in audio_resampler.h and decoder_libsndfile.h? It will not link without the libs (if using the functions) anyway, so it would make sense to empty the files when the preprocessor macros are not defined imo.

@ChristianBreitwieser

This comment has been minimized.

Show comment
Hide comment
@ChristianBreitwieser

ChristianBreitwieser Jun 14, 2016

Member

No there is no "real" reason - I just tried to follow the style of the two existing decoders (fmmidi and mpg123).

In fact the header files aren't used anyway if the macros aren't defined. The inclusion in audio_decoder.cpp is macro dependent as well.

Should i change it?

Member

ChristianBreitwieser commented Jun 14, 2016

No there is no "real" reason - I just tried to follow the style of the two existing decoders (fmmidi and mpg123).

In fact the header files aren't used anyway if the macros aren't defined. The inclusion in audio_decoder.cpp is macro dependent as well.

Should i change it?

@Ghabry

This comment has been minimized.

Show comment
Hide comment
@Ghabry

Ghabry Jun 17, 2016

Member

Finnally found time to test the tempo. Works great 👍

Member

Ghabry commented Jun 17, 2016

Finnally found time to test the tempo. Works great 👍

@Ghabry Ghabry merged commit 306992e into EasyRPG:master Jun 17, 2016

5 checks passed

Android Build finished.
Details
Linux Build finished.
Details
OSX Build finished.
Details
Windows Build finished.
Details
web Build finished.
Details

@ChristianBreitwieser ChristianBreitwieser deleted the ChristianBreitwieser:audio branch Jun 17, 2016

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