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

"whipper image verify" abends on FLAC having ID3 tags ("TypeError: %x format: a number is required, not NoneType") #176

Closed
AccidentBlackSpot opened this issue Jul 6, 2017 · 6 comments
Labels
Bug Generic bug: can be used together with more specific labels Expected Behaviour Bug report of intended functionality

Comments

@AccidentBlackSpot
Copy link

AccidentBlackSpot commented Jul 6, 2017

Title correction: "most albums" grossly inaccurate. Necessary condition appears to be:
That a ripped FLAC file has been subsequently (and externally) polluted with a (sufficiently lengthy) ID3 tag block.

More detail of test conditions in following comment. I've reduced this original post to concentrate on the symptoms.


whipper 0.5.1

Bottom lines:
File "/usr/local/lib/python2.7/dist-packages/whipper-0.5.1-py2.7.egg/whipper/common/program.py", line 620, in _verifyImageWithChecksums
if "%08x" % csum == r.checksums[i]:
TypeError: %x format: a number is required, not NoneType

Watching the terminal, I see the "Getting length of audio track (n of N)" run similarly in the successful and abending cases. (It does the exercise twice, apparently, 1 to N and then 1 to N again.)

Whereas the successful cases then clearly run through "Calculating (Fast) AccurateRip checksum (n of N)", albeit very quickly, the abending cases print "WARNING:whipper.program.arc:ARC calculation failed: arc return code is non zero", times the number of tracks (I believe), with no noticeable hesitation.

I'm out of my depth, here, but from my limited understanding, I believe whipper.program.arc is failing (probably incorrectly) and the calling routine is (probably incorrectly) treating the error result as non-critical, which eventually results in the unhelpful error message, somewhat later on (when an unassigned variable cannot be formatted into a string).

stderror.txt
stdout-abending.txt
stdout-successful.txt

@AccidentBlackSpot
Copy link
Author

Sorry - I've just been working back through changes I made to the FLAC files and completely forgot to mention. Consequently, I have probably identified what is causing Whipper to barf.

I had added ID3 v2.3 tags to the FLAC files.

Yes, I know that contravenes the FLAC standards. You probably know that Exact Audio Copy provides us with an option to do this (with a choice of ID3 version). You may even know that some players expect tagging to be in ID3 format, in FLAC files, and, by now, you may have guessed that I have such a device (Samsung).

Yes, I know it was unforgivable of me to mention none of that in my original bug report. Simply, I somehow forgot what I'd been doing when I started trying to understand the error message.

From a quick check, Whipper seems to be happy, even giving an affirmative verification result, until I include the cover image in the ID3 tag. (I.e. I've got away with having the textual ID3 tags in the FLAC files.) Probably, this is related to the number of bytes taken up by the ID3 tag, rather than a particular disliking for images.

Of course, Whipper is using some FLAC library to obtain the WAV for checksumming. So, the issue (if we agree it is an issue, and since it's not a clear rejection of the mere presence of an ID3 tag, the behaviour surely cannot be exactly by design) could be external to Whipper.

By the way, I just used avconv to transcode a problem FLAC to MP3. No errors, but the MP3 file has the text of each tag repeated, e.g.:
ID3 v2.4:
title: Crazy for You;Crazy for You artist: Madonna;Madonna
album: Something to Remember;Something to Remember year: 1995
track: 5

So, running the obvious test:
ID3 v2.4:
title: the ID3 title;Crazy for You artist: the ID3 artist;Madonna
album: the ID3 album;Something to Remember year: 1995
track: 5

Yep, avconv will process both the genuine FLAC tags and the (naughty) ID3 tags, and, when it finds both types, concatenates them.

So, standards or no standards, choking on an ID3 tag in a FLAC file appears to be regarded as bad manners. (Not that I should really be raising the subject of manners at the moment.)

@AccidentBlackSpot AccidentBlackSpot changed the title "whipper image verify" abends on most albums ("TypeError: %x format: a number is required, not NoneType") "whipper image verify" abends on FLAC having ID3 tags ("TypeError: %x format: a number is required, not NoneType") Jul 6, 2017
@MerlijnWajer
Copy link
Collaborator

I believe the verify image errors are not related to any metadata in the FLAC files. It seems related to ARC calculation.

@RecursiveForest
Copy link
Contributor

if you run the binary accuraterip-checksum that you installed alongside whipper against a flac file with ID3 tags, what does it return? Both std{out,err} and $?, please.

@AccidentBlackSpot
Copy link
Author

Wow! Was completely ignorant of that binary. It certainly makes it more easy to create a manageable test case. (I'd been struggling at the "whipper image verify" level, as it seemed to require something quite closely matching a genuine rip in order to get started. I'd got as far as guessing I needed to keep the track lengths correct, but had not got round to trying that.) So, thanks for alerting me.

$ accuraterip-checksum "01_American_Life_elision_withImage_id3v23.flac" 1 11
sf_open failed! sf_error==2

Second line is from stderr. I did a run with redirection. Stdout was a zero-length file.

I do not understand what you meant by the "$" of the return, sorry.

I cannot vouch for the technical correctness of my test files. I have been using the eyed3 Python library to enhance the FLAC files with ID3 tags and assume that library produces valid files (to the full extent any FLAC with ID3 could be considered valid). (Somewhere in that assumption is the hope that, were I misusing the library - which I do not dismiss on grounds of my competence / expertise, an error of some sort would have been coming back from it.)

Anyway, the error occurs when there are ID3 (version 2.3) tags present and a "CD Front Cover" JPG is one of them. If there is not the "CD Front Cover" JPG, the error is not reported:

$ accuraterip-checksum "01_American_Life_elision_noImage_id3v23.flac" 1 11
9D5AFEC4

I have not exhaustively tested all cases, such as image tag but none of the plain text tags, images of different lengths, no image but War And Peace as a plain text tag, other versions of ID3 tags. Were I competent, I would dive into the debugger, at this stage, and hope that presented an easy route.

All that said, as I sort of implied in my mea culpa comment, getting Whipper to operate on non-compliant files is never going to be the best use of any programmer's time. Rather than addressing my use case, though, there may be a robustness point to come out of this little incident.

If the test files are not available through the site, let me know and I'll publish them through some cloud or other.

01_American_Life_elision.zip

@JoeLametta JoeLametta added the Bug Generic bug: can be used together with more specific labels label Mar 8, 2018
@JoeLametta
Copy link
Collaborator

@AccidentBlackSpot Mystery solved! 😜

sf_open failed! sf_error==2

This error comes from libsndfile.
I've just tested and the file 01_American_Life_elision_withImage_id3v23.flac is correctly processed if using accuraterip-checksum linked against libsndfile 1.0.28: in case it has been linked against previous versions (like 1.0.27) it seems to fail with: sf_open failed! (and various error codes).

Changelog for libsndfile 1.0.28.

TL;DR This isn't a bug in whipper. It affects libsndfile's versions before 1.0.28 and it's not really a bug (section from FLAC's FAQ on Xiph.org):

you should not expect any tags beside FLAC tags to be supported in applications; some implementations may not even be able to decode a FLAC file with ID3 tags.

@JoeLametta JoeLametta added the Expected Behaviour Bug report of intended functionality label Apr 6, 2018
@AccidentBlackSpot
Copy link
Author

@JoeLametta

I am really grateful for your effort, and also sorry I reported the issue, since it was one of those things that was going to go away on its own, without anyone worrying about it (at least from the perspective of the whipper project). Still, at least I remembered there were ID3 tags not too long after I'd cried "bug"!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Generic bug: can be used together with more specific labels Expected Behaviour Bug report of intended functionality
Projects
None yet
Development

No branches or pull requests

4 participants