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

riff: fix parsing ffmpeg wave stdout output #327

Closed

Conversation

sscobici
Copy link

Adding back 4 bytes for parent length comparison with u32:MAX as it was reduced by 4 bytes in commit f72ac41

This fixes symphonia-check some.mp3 which currently returns error:

Test interrupted by error: malformed stream: riff: chunk length exceeds parent (list) chunk length

@pdeljanov
Copy link
Owner

Hmm, I see the issue, and this would be a workaround, but I don't think it is a correct fix. Subtracting 4 was just one possible case for AIFF and WAVE, however, it may be different for other RIFF-based formats like AVI which would also be implemented in this crate.

I got hit with a similar issue with MkvReader. I'm rewriting it now so that EbmlIterator will take the the length as an option since these magic values always seem to cause bugs in the end.

So, I think the ideal fix here too would be to make the length passed to the chunk reader an Option<u64>. In the case where the length is unknown, we'd just pass None.

@sscobici sscobici marked this pull request as draft December 28, 2024 16:39
@sscobici
Copy link
Author

sscobici commented Jan 5, 2025

Cancelling this workaround PR. Will wait for your refactoring.

@sscobici sscobici closed this Jan 5, 2025
@pdeljanov
Copy link
Owner

I fixed this issue in 5a9bc17 by passing an Option<u32> to the ChunkReader and detecting when FFmpeg sets it to 0xffff'ffff.

Please help test both AIFF and WAVE with this new change.

@sscobici
Copy link
Author

I don't have too many files but ffmpeg output comparison now works with this commit, while previously it was failing. Thanks

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.

2 participants