fix inconsistent handling of 8-bit audio #439
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
It was not actually checking anything.
tolerance=0 should mean they have to exactly equal.
It was just a bit more than 1% off.
Since AudioSegment.dBFS calculates the value based on the amplitude, we should compare against that in our tests. The expected value for the 8-bit test had to be changed because audioop.rms truncates its result to an integer, and so the dBFS does not precisely match the value for the 16-bit version.
The RMS values for 8-bit AudioSegments created a variety of ways are compared.
This solves the issue where 8-bit audio is inconsistently treated as unsigned and signed integers. The audioop library only supports signed integers, so this fix converts to and from signed integers where necessary. It removes places where the values are treated like unsigned as these partial fixes are no longer necessary. This includes special handling in the rms and set_sample_width methods of the AudioSegment class. It changes the codec used for reading from 8-bit wav files to pcm_u8 from pcm_s8 since pcm_u8 is the only valid codec for 8-bit wav files.
|
This looks great - would you be able to address the failing unit tests? |
The test was requiring more precision than we can expect from mp3.
The tolerance is changed to 1.5dB as used elsewhere in the code.
|
Okay, i've fixed the tests. |
|
This also fixes issue #409. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This solves the problem where 8-bit audio is inconsistently treated as unsigned and signed integers.
This fix converts to and from signed integers when reading or writing 8-bit wav files.
The rms and set_sample_width methods of the AudioSegment class tried to handle the data as unsigned,
but this would not be enough, since audioop's methods all treat the audio like signed integers.
The codec used for reading from 8-bit wav files is changed to pcm_u8 from pcm_s8.
This will fix issues #417. #367, and the garbled audio problem mentioned in #381.
Tests were added to compare the RMS values for 8-bit audio segments created from a variety of ways to ensure they are all the same.