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

Fix truncated decode of mp3 files on import #175

Merged
merged 1 commit into from Feb 24, 2017

Conversation

Projects
None yet
3 participants
@cannam
Contributor

cannam commented Nov 26, 2016

The MAD decoder will not decode the final frame in an mp3 stream unless
it has a small amount of padding afterwards (MAD_BUFFER_GUARD bytes,
actually 8). Without this, it loses sync before returning any decoded
data from the final frame. The result is that the imported audio is
truncated by up to 1152 samples.

This commit addresses that by using a final round of input callback
calls to provide the last MAD_BUFFER_GUARD bytes after the underlying
file has reached eof. The logic is based on madplay.

Fix truncated decode of mp3 files on import
The MAD decoder will not decode the final frame in an mp3 stream unless
it has a small amount of padding afterwards (MAD_BUFFER_GUARD bytes,
actually 8). Without this, it loses sync before returning any decoded
data from the final frame. The result is that the imported audio is
truncated by up to 1152 samples.

This commit addresses that by using a final round of input callback
calls to provide the last MAD_BUFFER_GUARD bytes after the underlying
file has reached eof. The logic is based on madplay.
@cannam

This comment has been minimized.

Show comment
Hide comment
@cannam

cannam Nov 26, 2016

Contributor

You can find a test file at http://all-day-breakfast.com/m/ticks.mp3. It contains two short bursts of noise, one right at the start of the file and the other right at the end, separated by a couple of seconds of silence. If you open this in the Audacity master branch you will find that the second burst is missing; this commit fixes that.

The logic here is surprisingly fiddly, and would benefit from careful review.

(I chanced on this after finding the same problem in my own use of the MAD library in Sonic Visualiser. I suspect that hardly anyone is using this library completely correctly, since everyone copies the minimad example code and that has this problem as well. The official madplay application handles this case and that's what the fix is based on.)

Contributor

cannam commented Nov 26, 2016

You can find a test file at http://all-day-breakfast.com/m/ticks.mp3. It contains two short bursts of noise, one right at the start of the file and the other right at the end, separated by a couple of seconds of silence. If you open this in the Audacity master branch you will find that the second burst is missing; this commit fixes that.

The logic here is surprisingly fiddly, and would benefit from careful review.

(I chanced on this after finding the same problem in my own use of the MAD library in Sonic Visualiser. I suspect that hardly anyone is using this library completely correctly, since everyone copies the minimad example code and that has this problem as well. The official madplay application handles this case and that's what the fix is based on.)

@SteveDaulton

This comment has been minimized.

Show comment
Hide comment
@SteveDaulton

SteveDaulton Nov 27, 2016

Member

Thanks for this Chris.

I can confirm that Audacity does have the problem that you describe, and that your patch does allow Audacity to import the final frame, but it does not seem to be quite right yet. With your patched version I observe that the imported file is prepended with 1152 additional silent samples.

Audacity has just gone into freeze in preparation for releasing 2.1.3, so this can't be merged right now. If you can sort out the residual problem (the extra samples at the start) then I'd be keen to see this reviewed and committed when the freeze is ended.

Member

SteveDaulton commented Nov 27, 2016

Thanks for this Chris.

I can confirm that Audacity does have the problem that you describe, and that your patch does allow Audacity to import the final frame, but it does not seem to be quite right yet. With your patched version I observe that the imported file is prepended with 1152 additional silent samples.

Audacity has just gone into freeze in preparation for releasing 2.1.3, so this can't be merged right now. If you can sort out the residual problem (the extra samples at the start) then I'd be keen to see this reviewed and committed when the freeze is ended.

@cannam

This comment has been minimized.

Show comment
Hide comment
@cannam

cannam Nov 27, 2016

Contributor
Contributor

cannam commented Nov 27, 2016

@SteveDaulton

This comment has been minimized.

Show comment
Hide comment
@SteveDaulton

SteveDaulton Nov 27, 2016

Member

Thanks for the additional detail. Yes I see what you're saying about the leading samples. I'll forward your link to the developer's mailing list and log both of these issues on our bug-tracker.
I'd be happy to assign these bugs to you if you'd care to join us on the developer's mailing list (https://lists.sourceforge.net/lists/listinfo/audacity-devel).
As I wrote previously, we're in freeze so we can't merge this right now, but please do come and say hello on-list (the GitHub comments are not heavily monitored - the primary communication channel for the Audacity developers is the mailing list).
Thanks again for your contribution (and by the way, I'm a big fan of Sonic Visualiser :-)

Member

SteveDaulton commented Nov 27, 2016

Thanks for the additional detail. Yes I see what you're saying about the leading samples. I'll forward your link to the developer's mailing list and log both of these issues on our bug-tracker.
I'd be happy to assign these bugs to you if you'd care to join us on the developer's mailing list (https://lists.sourceforge.net/lists/listinfo/audacity-devel).
As I wrote previously, we're in freeze so we can't merge this right now, but please do come and say hello on-list (the GitHub comments are not heavily monitored - the primary communication channel for the Audacity developers is the mailing list).
Thanks again for your contribution (and by the way, I'm a big fan of Sonic Visualiser :-)

@JamesCrook JamesCrook merged commit 9234973 into audacity:master Feb 24, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment