-
Notifications
You must be signed in to change notification settings - Fork 326
Skip ID3v2 tags when decode through errors is enabled #859
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
base: master
Are you sure you want to change the base?
Conversation
|
Can you please share what error you get without this patch? Skipping ID3v2 tags has been in flac for a long time, so I am unsure why this PR would be necessary. |
|
Sure, if you attach an ID3v2 tag to any valid flac file, it will place it at the start of the file. The You can see this behavior in action by asking flac to process a file with an ID3v2 tag: Despite the I believe this is a bug. In my opinion, the Let me know if you need any more info or clarification. Thanks! |
|
You're trying to re-encode through errors. Functionality is indeed more limited than pure decoding, and this is not the only example. I'll need some time to review, but following the xz utils backdoor, PRs with binary files need more scrutiny. |
|
For the record, the binary file I included here is derived from:
I simply copied this file and added an ID3v2 flag to it using the id3v2 program: After doing this you will see the SHA256 hash is identical to the test file I provided:
Those are the steps taken to create the binary test file. Of course, if you don't trust the If you have a tool that you trust I would be happy to use that to craft an ID3v2 file using the resources already present in this project. Also, if you want to provide an ID3v2 flac file that you trust, I would be more than happy to alter the tests to use yours instead. Just let me know if there is anything I can do on my end. Thanks! |
|
Hello, Any updates on this PR? I'd be happy to implement any suggestions. |
Hello,
The
flaccommand issues an error when a flac file starts with an ID3v2 tag. However, it still emits an error even when decode through errors is enabled. This patch changes this behavior with this option. Instead of quitting with an error, the command skips guessing the filetype from the first 12 bytes. We instead use the type guess from the file extension. When decoding occurs, it correctly skips this chunk and is able to process the rest of the flac file as expected. I have added a test to make sure this functionality is correct.Let me know if there are any problems! All in all I am very unfamiliar with the flac file format (outside of a quick skim read of the RFC), so I am sure there is something I am missing. I'd be happy to correct any issues.
Thanks,
Owen Cochell