Skip to content
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 S24_LE wav files and tests #41

Merged
merged 3 commits into from Feb 2, 2020
Merged

Add S24_LE wav files and tests #41

merged 3 commits into from Feb 2, 2020

Conversation

fletcherw
Copy link
Contributor

The second patch in the series gets read_wav_wave_format_extensible_pcm_24bit_4byte() to pass, but the other test will need some deeper changes to work (potentially adding a frame size field which is block_align / num_channels ?)

@ruuda
Copy link
Owner

ruuda commented Jan 30, 2020

Thanks! This looks good to me so far.

the other test will need some deeper changes to work (potentially adding a frame size field which is block_align / num_channels ?)

WavSpecEx already has a bytes_per_sample field, and as you show that works out with WAVEFORMATEXTENSIBLE. I think what we can do is relax the check you mentioned earlier, and then pass the number of bytes per sample down into these methods that construct the WavSpecExc:

hound/src/read.rs

Lines 511 to 514 in 10673f8

PCM => self.read_wave_format_pcm(chunk_len, spec),
ADPCM => Err(Error::Unsupported),
IEEE_FLOAT => self.read_wave_format_ieee_float(chunk_len, spec),
EXTENSIBLE => self.read_wave_format_extensible(chunk_len, spec),

Or we could even already construct the WavSpecEx in read_fmt_chunk; all three read_wave_format_* methods set bytes_per_sample to spec.bits_per_sample / 8 anyway. We would set it to block_align / n_channels instead now.

Do you want to add that to this pull request?

Add function read_le_i24_4 which reads a little-endian i24 sample stored
within a 4 byte chunk. This is used for decoding S24_LE wav files.
@fletcherw
Copy link
Contributor Author

Got both tests passing. I wasn't sure what your format preferences were, since it seems like you're not using cargo fmt generally. I just ran it on the lines that I modified, hopefully that looks good to you. I'm going to take a look at writing relatively soon as well.

Loosen the restriction that bitsPerSample exactly equals blockAlign /
numChannels * 8. In doing so, we allow playback of wav files that, while
standards non-conforming, are common in the wild.
Add two test files, one using pcmwaveformat with bitsPerSample == 24,
and one using waveformatextensible with validBitsPerSample == 24. Also
add tests that verify these two files are properly read.

According to the standard, the first sort of file should not be
permitted, but it is created by arecord, so it is a good type of file to
try and support.
@fletcherw
Copy link
Contributor Author

Fixed some build errors on old rust.

@ruuda
Copy link
Owner

ruuda commented Feb 2, 2020

Got both tests passing. I wasn't sure what your format preferences were, since it seems like you're not using cargo fmt generally. I just ran it on the lines that I modified, hopefully that looks good to you.

I don’t have a strong preference on formatting, it looks good to me. Thanks for only formatting the modified lines and not the entire file.

Copy link
Owner

@ruuda ruuda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks for implementing this!

@ruuda ruuda merged commit 99c120b into ruuda:master Feb 2, 2020
@fletcherw fletcherw deleted the s24le_test branch February 5, 2020 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants