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 support for 24-bit .wav files. #958

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@Foaly
Contributor

Foaly commented Aug 31, 2015

This is a fix for #955.

Since the decode() overloading works with different types for the value parameter and there is no 24-bit integer type, I named the function decode24bit(). It seemed more appropriate then implementing a new type just for that. Of course I am open to suggestions.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Aug 31, 2015

Member

The right-shift sample >> 16 discards the 8 lower bits of the 24 bit sample, is that intended?

And why not

value = bytes[0] | (bytes[1] << 8) | (bytes[2] << 16); 

with less right-shifting, instead of

value = (bytes[0] << 8) | (bytes[1] << 16) | (bytes[2] << 24); 

Generally, I'm not sure if using signed types for bit-shifting is a good idea, as some operations involving negative values are implementation-defined.

Furthermore, the if(...) without space violates the coding convention.

Member

Bromeon commented Aug 31, 2015

The right-shift sample >> 16 discards the 8 lower bits of the 24 bit sample, is that intended?

And why not

value = bytes[0] | (bytes[1] << 8) | (bytes[2] << 16); 

with less right-shifting, instead of

value = (bytes[0] << 8) | (bytes[1] << 16) | (bytes[2] << 24); 

Generally, I'm not sure if using signed types for bit-shifting is a good idea, as some operations involving negative values are implementation-defined.

Furthermore, the if(...) without space violates the coding convention.

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Aug 31, 2015

Contributor

I was wondering about the sample >> 16 too, but I was following the implementation for 32-bit (case 4) which also truncates the lower 16 bits. Of course I could change the implementation to scale from 24/32 to 16 bits (I think that would be the correct way)

Of course I could use

value = bytes[0] | (bytes[1] << 8) | (bytes[2] << 16);

the I would just have to shift by 8 bit 😄 or apply the scaling accordingly.

So are you suggesting I should use a sf::UInt32 instead of sf::Int32?

I'll change the if, sorry about that.

Thank you for the feedback!

edit: Also we should maybe add a default branch to the switch to inform the user that he is using an unsupported bit format. Like we currently kind of do in the FLAC reader.

Contributor

Foaly commented Aug 31, 2015

I was wondering about the sample >> 16 too, but I was following the implementation for 32-bit (case 4) which also truncates the lower 16 bits. Of course I could change the implementation to scale from 24/32 to 16 bits (I think that would be the correct way)

Of course I could use

value = bytes[0] | (bytes[1] << 8) | (bytes[2] << 16);

the I would just have to shift by 8 bit 😄 or apply the scaling accordingly.

So are you suggesting I should use a sf::UInt32 instead of sf::Int32?

I'll change the if, sorry about that.

Thank you for the feedback!

edit: Also we should maybe add a default branch to the switch to inform the user that he is using an unsupported bit format. Like we currently kind of do in the FLAC reader.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Aug 31, 2015

Member

I agree with @Bromeon, I see no reason to put the 24 bits in the MSB of the 32-bits integer, and then shift them back by 8 additional bits. Just put them in the LSB, then left-shift by 8 to get a 16-bits sample.

Of course I could change the implementation to scale from 24/32 to 16 bits (I think that would be the correct way)

How would that be different? Scaling from 24 bits to 16 bits means dividing by 256, which is exactly what the left-shift by 8 bits does.

Also we should maybe add a default branch to the switch to inform the user that he is using an unsupported bit format

👍

By the way, just to be sure, this code is tested and works with the WAV files given in #955, right?

Member

LaurentGomila commented Aug 31, 2015

I agree with @Bromeon, I see no reason to put the 24 bits in the MSB of the 32-bits integer, and then shift them back by 8 additional bits. Just put them in the LSB, then left-shift by 8 to get a 16-bits sample.

Of course I could change the implementation to scale from 24/32 to 16 bits (I think that would be the correct way)

How would that be different? Scaling from 24 bits to 16 bits means dividing by 256, which is exactly what the left-shift by 8 bits does.

Also we should maybe add a default branch to the switch to inform the user that he is using an unsupported bit format

👍

By the way, just to be sure, this code is tested and works with the WAV files given in #955, right?

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Sep 1, 2015

Contributor

Scaling from 24 bits to 16 bits means dividing by 256, which is exactly what the left-shift by 8 bits does.

Of course you are right. Now that I think about it, it comes out to be the same thing.

Changed the implementation to avoid unnecessary shifting and added a default branch with an error message. I read a bit about implementation-defined behaviour when shifting signed integers, so I am using sf::Uint32 now, to rely on standart-defined behaviour. Should we maybe use sf::Uint32 for the 32 bit case as well?

@LaurentGomila Of course I tested the code with a 24-bit .wav file. I tryed downloading the files form the original issue, but the link is dead. Do you have them? Otherwise I would ask the OP to upload the files again.

Contributor

Foaly commented Sep 1, 2015

Scaling from 24 bits to 16 bits means dividing by 256, which is exactly what the left-shift by 8 bits does.

Of course you are right. Now that I think about it, it comes out to be the same thing.

Changed the implementation to avoid unnecessary shifting and added a default branch with an error message. I read a bit about implementation-defined behaviour when shifting signed integers, so I am using sf::Uint32 now, to rely on standart-defined behaviour. Should we maybe use sf::Uint32 for the 32 bit case as well?

@LaurentGomila Of course I tested the code with a 24-bit .wav file. I tryed downloading the files form the original issue, but the link is dead. Do you have them? Otherwise I would ask the OP to upload the files again.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Sep 1, 2015

Member

added a default branch with an error message

This function will be called a lot, if it prints a message it will flood the error output. Either put an assert, like in FLAC reader, or put the check when decoding the WAV header.

I read a bit about implementation-defined behaviour when shifting signed integers, so I am using sf::Uint32 now, to rely on standart-defined behaviour. Should we maybe use sf::Uint32 for the 32 bit case as well?

Can you give more details about this behaviour?

Do you have them?

Nop.

Otherwise I would ask the OP to upload the files again.

Yep.

Member

LaurentGomila commented Sep 1, 2015

added a default branch with an error message

This function will be called a lot, if it prints a message it will flood the error output. Either put an assert, like in FLAC reader, or put the check when decoding the WAV header.

I read a bit about implementation-defined behaviour when shifting signed integers, so I am using sf::Uint32 now, to rely on standart-defined behaviour. Should we maybe use sf::Uint32 for the 32 bit case as well?

Can you give more details about this behaviour?

Do you have them?

Nop.

Otherwise I would ask the OP to upload the files again.

Yep.

@minirop

This comment has been minimized.

Show comment
Hide comment
@minirop

minirop Sep 1, 2015

@LaurentGomila he probably speaks about:

EXAMPLE An example of implementation-defined behavior is the propagation of the high-order bit
when a signed integer is shifted right.

minirop commented Sep 1, 2015

@LaurentGomila he probably speaks about:

EXAMPLE An example of implementation-defined behavior is the propagation of the high-order bit
when a signed integer is shifted right.

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Sep 1, 2015

Contributor

... it will flood the error output ...

No it won't. For sf::SoundBuffer sf::SoundFileReaderWav::read() is only called once. Since I return 0 after the error message is printed this if in sf::SoundBuffer::initialize() fails. That leads to loadFromFile returning false. I tested this.
sf::Music behaves a little different. It silently opens the file, even if the format is not supported. read() is not called until the user calls play() and sf::Music starts streaming the data. But I guess there is nothing we can do about that. Still the error message is only displayed once, since sf::Music::onGetData() returns false and it stops streaming.

About the shifting behaviour of signed integer types. This SO answer explains it very well. Apparently it's up to the compiler to decide if he wants to pad a negativ integer with 0's or 1's if its shifted to the right. If I understand it correctly, this doesn't affect us in this situation, since we are only using the lower part of the integer anyway, but I'd say it's best practice to use data types where the operations that you use are well-defined.

I left a comment in the original issue, about the reuploading.

Contributor

Foaly commented Sep 1, 2015

... it will flood the error output ...

No it won't. For sf::SoundBuffer sf::SoundFileReaderWav::read() is only called once. Since I return 0 after the error message is printed this if in sf::SoundBuffer::initialize() fails. That leads to loadFromFile returning false. I tested this.
sf::Music behaves a little different. It silently opens the file, even if the format is not supported. read() is not called until the user calls play() and sf::Music starts streaming the data. But I guess there is nothing we can do about that. Still the error message is only displayed once, since sf::Music::onGetData() returns false and it stops streaming.

About the shifting behaviour of signed integer types. This SO answer explains it very well. Apparently it's up to the compiler to decide if he wants to pad a negativ integer with 0's or 1's if its shifted to the right. If I understand it correctly, this doesn't affect us in this situation, since we are only using the lower part of the integer anyway, but I'd say it's best practice to use data types where the operations that you use are well-defined.

I left a comment in the original issue, about the reuploading.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Sep 1, 2015

Member

No it won't

Right 😛. But we don't know how sound files will be used in the future, so it can't hurt to do things right.

It silently opens the file, even if the format is not supported

That's what I said: we should check this when opening the file (ie. return false in parseHeader) rather than succeeding to open and throwing an error when reading any samples.

Member

LaurentGomila commented Sep 1, 2015

No it won't

Right 😛. But we don't know how sound files will be used in the future, so it can't hurt to do things right.

It silently opens the file, even if the format is not supported

That's what I said: we should check this when opening the file (ie. return false in parseHeader) rather than succeeding to open and throwing an error when reading any samples.

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Sep 2, 2015

Contributor

we should check this when opening the file

Like I said that is already the case for sf::SoundBuffer, but I added a check to the parseHeader() to make the behaviour consistent.
The error looks like this now:

Unsupported bit rate: xx bit (Supported bit rates are 8/16/24/32 bit)
Failed to open WAV sound file (invalid or unsupported file)

I also added a note in the documentation of sf::InputSoundFile stating which bit formats are supported. I don't know if other bit formats are in actual use, but I thought it would be better to be expressive.

Contributor

Foaly commented Sep 2, 2015

we should check this when opening the file

Like I said that is already the case for sf::SoundBuffer, but I added a check to the parseHeader() to make the behaviour consistent.
The error looks like this now:

Unsupported bit rate: xx bit (Supported bit rates are 8/16/24/32 bit)
Failed to open WAV sound file (invalid or unsupported file)

I also added a note in the documentation of sf::InputSoundFile stating which bit formats are supported. I don't know if other bit formats are in actual use, but I thought it would be better to be expressive.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Sep 2, 2015

Member

Like I said that is already the case for sf::SoundBuffer

InputSoundFile is public, it must be able to handle errors properly on its own. What other classes do with it doesn't matter.

The error looks like this now

Great. Except that... it's not the bit rate, it's the sample size.
And... now that we handle 8/16/24/32 bits samples, is there any other sample size that the WAV format supports that we don't handle?

I also added a note in the documentation

Same here, replace "bit rate" with "sample size" :p

Member

LaurentGomila commented Sep 2, 2015

Like I said that is already the case for sf::SoundBuffer

InputSoundFile is public, it must be able to handle errors properly on its own. What other classes do with it doesn't matter.

The error looks like this now

Great. Except that... it's not the bit rate, it's the sample size.
And... now that we handle 8/16/24/32 bits samples, is there any other sample size that the WAV format supports that we don't handle?

I also added a note in the documentation

Same here, replace "bit rate" with "sample size" :p

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Sep 2, 2015

Contributor

it's not the bit rate, it's the sample size.

Riiight haha 😄 I changed it.

I search around, but I couldn't find anything that defines what sample size is valid for WAV. The format specification seems to leave that open. But from the overview I got I'd say that 8/16/24/32 bit are the only formats actually in use.

Contributor

Foaly commented Sep 2, 2015

it's not the bit rate, it's the sample size.

Riiight haha 😄 I changed it.

I search around, but I couldn't find anything that defines what sample size is valid for WAV. The format specification seems to leave that open. But from the overview I got I'd say that 8/16/24/32 bit are the only formats actually in use.

@zsbzsb

This comment has been minimized.

Show comment
Hide comment
@zsbzsb

zsbzsb Sep 2, 2015

Member

According to Audacity only 8/16/24/32 are used. However Audacity also gives us a 32/64 float option too, but I doubt it it widely used. Here is an article on wikipedia talking about different sample sizes.

http://i.imgur.com/kIhjumH.png

Member

zsbzsb commented Sep 2, 2015

According to Audacity only 8/16/24/32 are used. However Audacity also gives us a 32/64 float option too, but I doubt it it widely used. Here is an article on wikipedia talking about different sample sizes.

http://i.imgur.com/kIhjumH.png

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Sep 2, 2015

Member

In fact WAV is just a container, there are many different formats that you can embed inside (even MP3). Since we limit the implementation to PCM format, and that's 99% of what WAV files contain, 8/16/24/32 bits samples should be all that we need.

So I'd say that we can remove the check. As long as we support only PCM (and this is explicitly checked), we are safe.

Therefore if the doc mentions a limitation, it should only talk about PCM format.

Member

LaurentGomila commented Sep 2, 2015

In fact WAV is just a container, there are many different formats that you can embed inside (even MP3). Since we limit the implementation to PCM format, and that's 99% of what WAV files contain, 8/16/24/32 bits samples should be all that we need.

So I'd say that we can remove the check. As long as we support only PCM (and this is explicitly checked), we are safe.

Therefore if the doc mentions a limitation, it should only talk about PCM format.

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Sep 2, 2015

Contributor

I don't know I'd still say leave the check in. Nothing stops you from using 12-bit (Amiga used those), 18-bit or maybe 64-bit files. I know all of this is highly unlikly and the user is using SFML wrong in that case, but at least they get a clear and consistent error message when trying to load the file (sf::SoundBuffer vs. sf::Music/sf::InputSoundFile). Performance-wise this check is not gonna hurt anybody. Your call though.

I changed the doc to mention that WAV only supports PCM.

Contributor

Foaly commented Sep 2, 2015

I don't know I'd still say leave the check in. Nothing stops you from using 12-bit (Amiga used those), 18-bit or maybe 64-bit files. I know all of this is highly unlikly and the user is using SFML wrong in that case, but at least they get a clear and consistent error message when trying to load the file (sf::SoundBuffer vs. sf::Music/sf::InputSoundFile). Performance-wise this check is not gonna hurt anybody. Your call though.

I changed the doc to mention that WAV only supports PCM.

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Sep 10, 2015

Contributor

So if nobody has any concerns I think this is ready to be merged.

Contributor

Foaly commented Sep 10, 2015

So if nobody has any concerns I think this is ready to be merged.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Sep 12, 2015

Member

It would be good if a few more people could test this before merging. Do you have a minimal testing example and some wave files with different sample sizes?

Member

Bromeon commented Sep 12, 2015

It would be good if a few more people could test this before merging. Do you have a minimal testing example and some wave files with different sample sizes?

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Sep 12, 2015

Contributor

I uploaded the files I used for testing here: https://www.dropbox.com/sh/0aa1y8vo8a5ygml/AACm_nj-LeCECf0hOsmYvDola?dl=0

This is the code I used for testing:

#include <SFML/Audio.hpp>
#include <iostream>

int main() {
    sf::SoundBuffer buffer;
    if (!buffer.loadFromFile("fail_24.wav")) {
        std::cerr << "Fatal error: Could not load sound file.";
        return 1;
    }
    sf::Sound sound(buffer);
    sound.play();

    sf::sleep(sf::seconds(2));

    sf::Music music;
    if(!music.openFromFile("fail_24.wav"))
    {
        std::cerr << "Fatal error: Could not load music sound file.";
        return 1;
    }
    music.play();

    sf::sleep(sf::seconds(2));

    return 0;
}

edit: also I updated the default branch in the code

Contributor

Foaly commented Sep 12, 2015

I uploaded the files I used for testing here: https://www.dropbox.com/sh/0aa1y8vo8a5ygml/AACm_nj-LeCECf0hOsmYvDola?dl=0

This is the code I used for testing:

#include <SFML/Audio.hpp>
#include <iostream>

int main() {
    sf::SoundBuffer buffer;
    if (!buffer.loadFromFile("fail_24.wav")) {
        std::cerr << "Fatal error: Could not load sound file.";
        return 1;
    }
    sf::Sound sound(buffer);
    sound.play();

    sf::sleep(sf::seconds(2));

    sf::Music music;
    if(!music.openFromFile("fail_24.wav"))
    {
        std::cerr << "Fatal error: Could not load music sound file.";
        return 1;
    }
    music.play();

    sf::sleep(sf::seconds(2));

    return 0;
}

edit: also I updated the default branch in the code

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Sep 12, 2015

Member

Works well on OS X. 👍

Member

mantognini commented Sep 12, 2015

Works well on OS X. 👍

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Sep 12, 2015

Member

Seems to work fine on Windows 8.1.

Member

eXpl0it3r commented Sep 12, 2015

Seems to work fine on Windows 8.1.

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Sep 12, 2015

Contributor

Thanks for testing guys! I just tried it on a Ubuntu 14.04 LTS and it works fine.

Contributor

Foaly commented Sep 12, 2015

Thanks for testing guys! I just tried it on a Ubuntu 14.04 LTS and it works fine.

@eXpl0it3r eXpl0it3r added s:accepted and removed s:undecided labels Sep 16, 2015

@eXpl0it3r eXpl0it3r added this to the 2.4 milestone Sep 16, 2015

@eXpl0it3r eXpl0it3r self-assigned this Sep 16, 2015

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Sep 16, 2015

Member

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

Member

eXpl0it3r commented Sep 16, 2015

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Sep 20, 2015

Member

Merged in b7d7ac4

Member

eXpl0it3r commented Sep 20, 2015

Merged in b7d7ac4

@eXpl0it3r eXpl0it3r closed this Sep 20, 2015

@Foaly Foaly deleted the Foaly:24-bit_sound branch Feb 10, 2018

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