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

Embedart: fails on some jpegs because imghdr doesn't recognize the mimetype #1545

Merged
merged 10 commits into from
Jul 27, 2015
Merged

Embedart: fails on some jpegs because imghdr doesn't recognize the mimetype #1545

merged 10 commits into from
Jul 27, 2015

Conversation

nathdwek
Copy link
Member

This errors pops up

embedart: Embedding album art into Queen - Live at Wembley ’86

embedart: not embedding image of unsupported type: image/x-None

with this cover (found by beets-fetchart).

This is due to this issue. Fortunately, imghdr provides the list imghdr.tests to add extra mimetype tests. I added a test which identifies jpeg files based on their magic bytes, which is the method used by file.

This is my first time contributing to beets, so I think some code style guidance is probably needed (mainly regarding the modification of imghdr.tests, which should only happen once). I also absolutely need some help with adding the right tests. There is some testing code in this PR, which is very naively based on the rest of test_mediafile.py, but the test doesn't fail without the patch, and I think it actually never runs. I should probably also find/build a smaller cover which can only be recognized based on the magic bytes.

nath@home added 6 commits June 26, 2015 12:28
@nathdwek
Copy link
Member Author

I added a commit to fix the code style issues.

@sampsyo
Copy link
Member

sampsyo commented Jul 21, 2015

Awesome! Thanks for sorting this out. Looks great so far.

  • I think the test belongs in a standalone test function, probably in the simpler test_mediafile_edge.py. The test can just call _image_mime_type() directly rather than using the automated test-battery machinery. Does that make sense?
  • I would love to avoid modifying imghdr.tests is possible. Could we just run your new magic-number check ourselves if imghdr gives us None? (Avoiding changes to global state help make things more predictable down the road.)
  • And finally, just to check: do you have reason to believe this check is fairly precise, i.e., there aren't a bunch of other formats that happen to use the same magic number? A link to the file policy would totally suffice.

@nathdwek
Copy link
Member Author

  • I'll have a look at test_mediafile_edge.py and see if I can get really started with tests without any external indication.
  • I also felt kinda uncomfortable modifying imghdr.tests like that but it had the advantage of really being a "pluggable" solution. I'll look into alternatives.
  • https://github.com/file/file/blob/master/magic/Magdir/jpeg#L12

@nathdwek
Copy link
Member Author

I added a commit re:testing. The test now passes and fails adequately.

@sampsyo
Copy link
Member

sampsyo commented Jul 21, 2015

Nice! The test looks great.

Would you mind adding a comment to the code with that link? Could help us stay less confused later on. 😃

@nathdwek
Copy link
Member Author

I added two commits with a more elegant implementation which doesn't require to modify imghdr.tests and also comments for the test.

Should I try to build a smaller image which fits the edge case or is the 1000x1000 image acceptable in the test ressources?

@sampsyo
Copy link
Member

sampsyo commented Jul 21, 2015

This is looking great. Thanks for the extra comments.

A smaller image would be wonderful if it's not too difficult -- it seems a little suboptimal to add a 0.5 MB file to the beets distribution for just one test. (And avoiding copyrighted images is also a plus.)

@nathdwek
Copy link
Member Author

Yeah I don't know I never tried but I guess it shouldn't be too difficult :)

@nathdwek
Copy link
Member Author

I added a commit with a smaller test image.

Anything else to be done?

@nathdwek nathdwek changed the title [WIP] Embedart: fails on some jpegs because imghdr doesn't recognize the mimetype Embedart: fails on some jpegs because imghdr doesn't recognize the mimetype Jul 21, 2015
@nathdwek
Copy link
Member Author

Do I need to do anything else before this can be merged?

@sampsyo
Copy link
Member

sampsyo commented Jul 27, 2015

Nope, I was just being slow. Thanks for your patience.

@sampsyo sampsyo merged commit 68ec829 into beetbox:master Jul 27, 2015
sampsyo added a commit that referenced this pull request Jul 27, 2015
Embedart: fails on some jpegs because imghdr doesn't recognize the mimetype
sampsyo added a commit that referenced this pull request Jul 27, 2015
@sampsyo
Copy link
Member

sampsyo commented Jul 27, 2015

All merged up! Thank you again! ✨

@nathdwek
Copy link
Member Author

Thank you very much for your responsiveness 😃

@nathdwek nathdwek deleted the fix-jpeg-detection branch August 2, 2015 12:28
nathdwek pushed a commit that referenced this pull request Nov 7, 2016
Apply #1545 to a public function used everywhere
nathdwek pushed a commit that referenced this pull request Nov 8, 2016
Apply #1545 to a public function used everywhere
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