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 a basic Wav decoder. #984

Merged
merged 4 commits into from Aug 13, 2016

Conversation

Projects
None yet
4 participants
@Rinnegatamante
Member

Rinnegatamante commented Aug 9, 2016

This fix the HUUUUGE loading time for WAV tracks on embedded systems
using libsndfile decoder. In the future when 3ds_decoder will be
removed, this should be used even on 3DS port probably.

Show outdated Hide outdated src/decoder_wav.h
#include <memory>
/**
* Audio decoder for WAV powered by libsndfile

This comment has been minimized.

@Ghabry

Ghabry Aug 10, 2016

Member

Update that comment

@Ghabry

Ghabry Aug 10, 2016

Member

Update that comment

@Ghabry

This comment has been minimized.

Show comment
Hide comment
@Ghabry

Ghabry Aug 10, 2016

Member

This is in general useful also for other systems.

To the other devs: Should this have some optional opt-in from configure.ac? Default on makes no sense to me on PC, because the IO is fast, but manual enabling could be useful for testing.
Also about the macro. Which one would you suggest? WANT_BUILTIN_WAV?

@Rinnegatamante
Please add these 2 new files to the "Makefile.am" in the root directory and to "builds/vs2015/PlayerLib.vcxproj" and "builds/vs2015/PlayerLib.vcxproj.filters"
In the filter file place them in the audio filter (<Filter>Source Files\Backend\Audio</Filter>)
The file formats should easy to understand.

Otherwise minor style issues:
Add spaces between "if" and "(", also between ")" and "{"
Add spaces between "=" and the variables: int decoded = fread
ifs followed by single statements: Move the statement on the next line

if (a > b)
    return 0;
Member

Ghabry commented Aug 10, 2016

This is in general useful also for other systems.

To the other devs: Should this have some optional opt-in from configure.ac? Default on makes no sense to me on PC, because the IO is fast, but manual enabling could be useful for testing.
Also about the macro. Which one would you suggest? WANT_BUILTIN_WAV?

@Rinnegatamante
Please add these 2 new files to the "Makefile.am" in the root directory and to "builds/vs2015/PlayerLib.vcxproj" and "builds/vs2015/PlayerLib.vcxproj.filters"
In the filter file place them in the audio filter (<Filter>Source Files\Backend\Audio</Filter>)
The file formats should easy to understand.

Otherwise minor style issues:
Add spaces between "if" and "(", also between ")" and "{"
Add spaces between "=" and the variables: int decoded = fread
ifs followed by single statements: Move the statement on the next line

if (a > b)
    return 0;
Show outdated Hide outdated src/decoder_wav.cpp
return file_!=NULL;
}
bool WavDecoder::Seek(size_t offset, Origin origin) {

This comment has been minimized.

@Ghabry

Ghabry Aug 10, 2016

Member

The Seek function completely ignores the "origin" argument (pass it through to fseek)

@Ghabry

Ghabry Aug 10, 2016

Member

The Seek function completely ignores the "origin" argument (pass it through to fseek)

Show outdated Hide outdated builds/psvita/Makefile
@@ -27,7 +27,7 @@ PREFIX = arm-vita-eabi
CC = $(PREFIX)-gcc
CXX = $(PREFIX)-g++
CFLAGS = -Wl,-q -O3 -DPSP2 -DHAVE_LIBSPEEXDSP \
-DHAVE_SLOW_CPU -DSUPPORT_AUDIO -DHAVE_LIBSNDFILE \
-DWANT_FASTWAV=1 -DSUPPORT_AUDIO -DHAVE_LIBSNDFILE \

This comment has been minimized.

@Ghabry

Ghabry Aug 10, 2016

Member

That =1 thing was in FMMIDI because we offer 2 options (1 and 2). For your case this is not needed, you can remove the "=1"

@Ghabry

Ghabry Aug 10, 2016

Member

That =1 thing was in FMMIDI because we offer 2 options (1 and 2). For your case this is not needed, you can remove the "=1"

Show outdated Hide outdated src/audio_decoder.cpp
@@ -168,14 +168,14 @@ std::unique_ptr<AudioDecoder> AudioDecoder::Create(FILE* file, const std::string
#endif
}
#ifdef HAVE_SLOW_CPU
#if WANT_FASTWAV == 1

This comment has been minimized.

@Ghabry

Ghabry Aug 10, 2016

Member

dito

Show outdated Hide outdated src/system.h
# if WANT_FASTWAV != 1
# error "WANT_FASTWAV must be set to 1"
# endif
#endif

This comment has been minimized.

@Ghabry

Ghabry Aug 10, 2016

Member

also not needed g

@Ghabry

Ghabry Aug 10, 2016

Member

also not needed g

Show outdated Hide outdated src/decoder_wav.cpp
#include "system.h"
#ifdef HAVE_SLOW_CPU

This comment has been minimized.

@carstene1ns

carstene1ns Aug 10, 2016

Member

WANT_FASTWAV

@carstene1ns

carstene1ns Aug 10, 2016

Member

WANT_FASTWAV

@Ghabry

This comment has been minimized.

Show comment
Hide comment
@Ghabry

Ghabry Aug 10, 2016

Member

I think we can squash the last 4 commits into one, because is just typo and style.
Next git lesson :P

git rebase -i HEAD~4

shows an editor of the last 4 commits (newest at the bottom)

Set the 3 last commits to "squash" (from pick) and keep the first line on "pick".

Squash merges the commit in the one above until it encounters a picked commit.

Save and quit, asks now for the commit message. Write a nice one (suggests all 4) and
git push -f origin as usual

Member

Ghabry commented Aug 10, 2016

I think we can squash the last 4 commits into one, because is just typo and style.
Next git lesson :P

git rebase -i HEAD~4

shows an editor of the last 4 commits (newest at the bottom)

Set the 3 last commits to "squash" (from pick) and keep the first line on "pick".

Squash merges the commit in the one above until it encounters a picked commit.

Save and quit, asks now for the commit message. Write a nice one (suggests all 4) and
git push -f origin as usual

@Ghabry

This comment has been minimized.

Show comment
Hide comment
@Ghabry

Ghabry Aug 12, 2016

Member

@fdelapena @carstene1ns
Do you think an entry to configure.ac /CMakeLists makes sense for this?

Probably not of much use on the desktop, has already libsndfile and SDL fallback and is fast enough.

Member

Ghabry commented Aug 12, 2016

@fdelapena @carstene1ns
Do you think an entry to configure.ac /CMakeLists makes sense for this?

Probably not of much use on the desktop, has already libsndfile and SDL fallback and is fast enough.

@fdelapena

This comment has been minimized.

Show comment
Hide comment
@fdelapena

fdelapena Aug 12, 2016

Contributor

IMHO it might be worth to fix libsndfile loading code to try preventing this behaviour, as several games use ADPCM and 24 bit PCM. Not sure how long will happen, but so far I don't know other ports with significant WAV slowdowns using autotools/cmake. Because 3DS is using its own Makefile, this could be deferred until at least confirming a possible libsndfile fix.

Contributor

fdelapena commented Aug 12, 2016

IMHO it might be worth to fix libsndfile loading code to try preventing this behaviour, as several games use ADPCM and 24 bit PCM. Not sure how long will happen, but so far I don't know other ports with significant WAV slowdowns using autotools/cmake. Because 3DS is using its own Makefile, this could be deferred until at least confirming a possible libsndfile fix.

@Ghabry Ghabry self-assigned this Aug 12, 2016

@carstene1ns

This comment has been minimized.

Show comment
Hide comment
@carstene1ns

carstene1ns Aug 12, 2016

Member

I think that libsndfile behaviour cannot be changed, as it will always try to fill in some information structure and therefore needs to know how much data the file has. Therefore this is a good addition. However, only for non-desktop systems, so not needed for autotools/cmake.

Btw. looking at the sdl/mixer/sound wave readers, there are some more formats that could be added to this, no idea if worth, as I do not know if used by a game.

Member

carstene1ns commented Aug 12, 2016

I think that libsndfile behaviour cannot be changed, as it will always try to fill in some information structure and therefore needs to know how much data the file has. Therefore this is a good addition. However, only for non-desktop systems, so not needed for autotools/cmake.

Btw. looking at the sdl/mixer/sound wave readers, there are some more formats that could be added to this, no idea if worth, as I do not know if used by a game.

fread(&samplerate, 1, 4, file_);
uint16_t bitspersample;
fread(&bitspersample, 1, 2, file_);
switch (bitspersample) {

This comment has been minimized.

@carstene1ns

carstene1ns Aug 12, 2016

Member

One thing to note here is that there are no error checks for invalid wave files (i.e. bad sample rate). But given the games have mostly been tested before, this problem should be minimal.

@carstene1ns

carstene1ns Aug 12, 2016

Member

One thing to note here is that there are no error checks for invalid wave files (i.e. bad sample rate). But given the games have mostly been tested before, this problem should be minimal.

This comment has been minimized.

@Ghabry

Ghabry Aug 13, 2016

Member

Does this handle 24bit PCMs?

@Ghabry

Ghabry Aug 13, 2016

Member

Does this handle 24bit PCMs?

This comment has been minimized.

@carstene1ns

carstene1ns Aug 13, 2016

Member

No this will fail this switch and likely crash later, because output_format is not defined.
We could add a default: condition to this switch and return false; directly for 24 bit and broken files.

@carstene1ns

carstene1ns Aug 13, 2016

Member

No this will fail this switch and likely crash later, because output_format is not defined.
We could add a default: condition to this switch and return false; directly for 24 bit and broken files.

uint16_t raw_enc;
fread(&raw_enc, 2, 1, file);
fseek(file, 0, SEEK_SET);
if (raw_enc == 0x01) { // Codec is normal PCM

This comment has been minimized.

@Ghabry

Ghabry Aug 13, 2016

Member

Or does 24bit fail that check? (then is no problem)

@Ghabry

Ghabry Aug 13, 2016

Member

Or does 24bit fail that check? (then is no problem)

@Rinnegatamante

This comment has been minimized.

Show comment
Hide comment
@Rinnegatamante

Rinnegatamante Aug 13, 2016

Member

24 Bits are not supported since apparently there isn't a proper output format: https://github.com/Ghabry/easyrpg-player/blob/audio/src/audio_decoder.h#L38-L46

Member

Rinnegatamante commented Aug 13, 2016

24 Bits are not supported since apparently there isn't a proper output format: https://github.com/Ghabry/easyrpg-player/blob/audio/src/audio_decoder.h#L38-L46

@carstene1ns

This comment has been minimized.

Show comment
Hide comment
@carstene1ns

carstene1ns Aug 13, 2016

Member

Yes, but you should add it to the switch and return false then.

Member

carstene1ns commented Aug 13, 2016

Yes, but you should add it to the switch and return false then.

Rinnegatamante added some commits Aug 9, 2016

Added a basic Wav decoder.
This fix the HUUUUGE loading time for WAV tracks on embedded systems
using libsndfile decoder. In the future when 3ds_decoder will be
removed, this should be used even on 3DS port probably.
Added WANT_FASTWAV to WII/3DS & 3DS Decoder cleanup
Added WANT_FASTWAV to WII/3DS & 3DS Decoder cleanup
Correctly handling uncommon formats.
Correctly handling uncommon formats.

@carstene1ns carstene1ns merged commit 9111417 into EasyRPG:master Aug 13, 2016

6 checks passed

Android (armeabi-v7a) Build finished.
Details
GNU/Linux Build finished.
Details
OSX Build finished.
Details
Windows (x64) Build finished.
Details
Windows (x86) Build finished.
Details
web Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment