search

moumar / ruby-mp3info

ruby-mp3info read low-level informations and manipulate tags on mp3 files.
http://rdoc.info/github/moumar/ruby-mp3info/master/frames
223 stars 57 forks source link

First check for pngs, before checking of jpgs #62

Closed RStankov closed 9 years ago

RStankov commented 9 years ago

I'm working on podcasting service and I need to be able to extract artwork from mp3. The gem works great most of the time. But for some mp3, like the following, it can't get the correct image.

http://www.podtrac.com/pts/redirect.mp3/audio.wnyc.org/notetoself/notetoself092315_cms532751_pod.mp3

I figured out that the issue was that when images parsed, the gem first checks for a jpg and then for png. For almost all of the troubling mp3 the fix attached in the PR. fixes this.

I made this sample project as an additional test case.

RStankov commented 9 years ago

(travis fails for jruby on master as well)

moumar commented 9 years ago

hello, thanks for the PR. Can you send me a problematic mp3 in order to look at the bug since your patch look like magic to me ...

RStankov commented 9 years ago

Hi,

This mp3 is problematic -

http://www.podtrac.com/pts/redirect.mp3/audio.wnyc.org/notetoself/notetoself092315_cms532751_pod.mp3

Also you can check https://github.com/RStankov/ruby-mp3info-test it contains a test suit with 27 files, 3 of which are failing with current version of mp3info.

moumar commented 9 years ago

thanks, i'll take a look at this

moumar commented 9 years ago

hello, i just pushed a fix on master, and your tests pass. Can you check it please ?

RStankov commented 9 years ago

Unfortunately it doesn't work, when I switch to at my test repo:

gem 'ruby-mp3info', github: 'moumar/ruby-mp3info', branch: 'master'

And I run:

ruby coverart.rb

I get:

file_000.mp3: pass
/Users/rstankov/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/bundler/gems/ruby-mp3info-5f5a03b17e36/lib/mp3info/id3v2.rb:305:in `block in pictures': undefined method `<' for nil:NilClass (NoMethodError)
    from /Users/rstankov/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/bundler/gems/ruby-mp3info-5f5a03b17e36/lib/mp3info/id3v2.rb:282:in `each_index'
    from /Users/rstankov/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/bundler/gems/ruby-mp3info-5f5a03b17e36/lib/mp3info/id3v2.rb:282:in `pictures'
    from coverart.rb:21:in `block in image_from_io'
    from /Users/rstankov/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/bundler/gems/ruby-mp3info-5f5a03b17e36/lib/mp3info.rb:304:in `open'
    from coverart.rb:16:in `image_from_io'
    from coverart.rb:77:in `block in <main>'
    from coverart.rb:76:in `each'
    from coverart.rb:76:in `<main>'

The check for the trailing null byte returns nil here - https://github.com/moumar/ruby-mp3info/blob/master/lib/mp3info/id3v2.rb#L305-L307

When I change the code to:

--  if (data =~ trailing_null_byte) < 10
-- data.gsub!(trailing_null_byte, "\xff".force_encoding('BINARY'))
-- end 
++ data =~ trailing_null_byte
++ data.gsub!(trailing_null_byte, "\xff".force_encoding('BINARY')) if data && data < 10

It generates broken image files for the following:

And file_021 crashes with Encoding::InvalidByteSequenceError.

$ ruby coverart.rb
file_000.mp3: pass
file_001.mp3: pass
file_002.mp3: pass
file_003.mp3: pass
file_004.mp3: pass
file_005.mp3: pass
file_006.mp3: pass
file_007.mp3: pass
file_008.mp3: pass
file_009.mp3: pass
file_010.mp3: pass
file_011.mp3: pass
file_012.mp3: pass
file_013.mp3: pass
file_014.mp3: pass
file_015.mp3: pass
file_016.mp3: pass
file_017.mp3: pass
file_018.mp3: pass
file_019.mp3: pass
file_020.mp3: pass
/Users/rstankov/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/bundler/gems/ruby-mp3info-5f5a03b17e36/lib/mp3info/id3v2.rb:489:in `encode!': incomplete "\x00" on UTF-16LE (Encoding::InvalidByteSequenceError)
    from /Users/rstankov/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/bundler/gems/ruby-mp3info-5f5a03b17e36/lib/mp3info/id3v2.rb:489:in `decode_tag'
    from /Users/rstankov/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/bundler/gems/ruby-mp3info-5f5a03b17e36/lib/mp3info/id3v2.rb:561:in `add_value_to_tag2'
    from /Users/rstankov/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/bundler/gems/ruby-mp3info-5f5a03b17e36/lib/mp3info/id3v2.rb:527:in `block in read_id3v2_3_frames'
    from /Users/rstankov/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/bundler/gems/ruby-mp3info-5f5a03b17e36/lib/mp3info/id3v2.rb:513:in `loop'
    from /Users/rstankov/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/bundler/gems/ruby-mp3info-5f5a03b17e36/lib/mp3info/id3v2.rb:513:in `read_id3v2_3_frames'
    from /Users/rstankov/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/bundler/gems/ruby-mp3info-5f5a03b17e36/lib/mp3info/id3v2.rb:372:in `from_io'
    from /Users/rstankov/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/bundler/gems/ruby-mp3info-5f5a03b17e36/lib/mp3info.rb:554:in `parse_tags'
    from /Users/rstankov/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/bundler/gems/ruby-mp3info-5f5a03b17e36/lib/mp3info.rb:259:in `reload'
    from /Users/rstankov/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/bundler/gems/ruby-mp3info-5f5a03b17e36/lib/mp3info.rb:230:in `initialize'
    from /Users/rstankov/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/bundler/gems/ruby-mp3info-5f5a03b17e36/lib/mp3info.rb:300:in `new'
    from /Users/rstankov/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/bundler/gems/ruby-mp3info-5f5a03b17e36/lib/mp3info.rb:300:in `open'
    from coverart.rb:16:in `image_from_io'
    from coverart.rb:77:in `block in <main>'
    from coverart.rb:76:in `each'
    from coverart.rb:76:in `<main>'
moumar commented 9 years ago

ok, thanks for the test repo, i'll take a look at it

RStankov commented 9 years ago

I'm closing the PR. Since it is cluttering in my "Open Pull requests" section on github and there doesn't seems to be any progress here :)

moumar commented 8 years ago

hello Rstankov,

can you look at the tip ? I just fixed this problem, all your tests at https://github.com/RStankov/ruby-mp3info-test pass ;)

RStankov commented 8 years ago

Hi,

It still fails for me on:

http://traffic.libsyn.com/themoment/TMBK15070701_Godin.mp3 http://traffic.libsyn.com/entreprogrammers/Episode-69-.mp3

/Users/rstankov/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/bundler/gems/ruby-mp3info-f7abc2f57a9f/lib/mp3info/id3v2.rb:491:in `encode!': incomplete "\x00" on UTF-16LE (Encoding::InvalidByteSequenceError)
    from /Users/rstankov/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/bundler/gems/ruby-mp3info-f7abc2f57a9f/lib/mp3info/id3v2.rb:491:in `decode_tag'
    from /Users/rstankov/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/bundler/gems/ruby-mp3info-f7abc2f57a9f/lib/mp3info/id3v2.rb:563:in `add_value_to_tag2'
    from /Users/rstankov/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/bundler/gems/ruby-mp3info-f7abc2f57a9f/lib/mp3info/id3v2.rb:529:in `block in read_id3v2_3_frames'
    from /Users/rstankov/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/bundler/gems/ruby-mp3info-f7abc2f57a9f/lib/mp3info/id3v2.rb:515:in `loop'
    from /Users/rstankov/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/bundler/gems/ruby-mp3info-f7abc2f57a9f/lib/mp3info/id3v2.rb:515:in `read_id3v2_3_frames'
    from /Users/rstankov/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/bundler/gems/ruby-mp3info-f7abc2f57a9f/lib/mp3info/id3v2.rb:374:in `from_io'
    from /Users/rstankov/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/bundler/gems/ruby-mp3info-f7abc2f57a9f/lib/mp3info.rb:554:in `parse_tags'
    from /Users/rstankov/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/bundler/gems/ruby-mp3info-f7abc2f57a9f/lib/mp3info.rb:259:in `reload'
    from /Users/rstankov/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/bundler/gems/ruby-mp3info-f7abc2f57a9f/lib/mp3info.rb:230:in `initialize'
    from /Users/rstankov/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/bundler/gems/ruby-mp3info-f7abc2f57a9f/lib/mp3info.rb:300:in `new'
    from /Users/rstankov/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/bundler/gems/ruby-mp3info-f7abc2f57a9f/lib/mp3info.rb:300:in `open'
    from coverart.rb:16:in `image_from_io'
    from coverart.rb:77:in `block in <main>'
    from coverart.rb:76:in `each'
    from coverart.rb:76:in `<main>'