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

Fix mp3 frames reading to jump correctly to the next bytes #165

Merged
merged 5 commits into from
Sep 16, 2020

Conversation

fabioperrella
Copy link
Contributor

@fabioperrella fabioperrella commented Sep 8, 2020

closes #163

In MP3Parser, I noticed the logic to jump to the next bytes was wrong (99% sure of that).

To debug it, I added a puts in the method parse_mpeg_frames:

seek_jmp = sync_bytes_offset_in_4_byte_seq(four_bytes)
puts "frame: #{frame_i}, pos: #{io.pos-4}, pos(h): #{(io.pos-4).to_s(16)} four: #{four_bytes}, hexa: #{four_bytes.map {|ii| ii.to_s(16)} }, jump: #{seek_jmp}"
if seek_jmp > 0
  io.seek(io.pos - 0 + seek_jmp)
  next
end

Then I got this:

frame: 183, pos: 1474, pos(h): 5c2 four: [0, 255, 251, 148], hexa: ["0", "ff", "fb", "94"], jump: 1
frame: 184, pos: 1479, pos(h): 5c7 four: [0, 0, 0, 0], hexa: ["0", "0", "0", "0"], jump: 4

The correct would be jumping to 1475, but it jumped +4 because it didn't take into account the 4 bytes that were already read at the beginning of this method:

# lib/parsers/mp3_parser.rb:139
data = io.read(4)

I fixed to subtract this 4 bytes, like this:

if seek_jmp > 0
  io.seek(io.pos - 4 + seek_jmp)
  next
end

And the result was:

frame: 366, pos: 1474, pos(h): 5c2 four: [0, 255, 251, 148], hexa: ["0", "ff", "fb", "94"], jump: 1
frame: 367, pos: 1475, pos(h): 5c3 four: [255, 251, 148, 196], hexa: ["ff", "fb", "94", "c4"], jump: 0

Because of that, it was possible to read the frames correctly!

I also needed to change MAX_FRAMES_TO_SCAN to a greater value, otherwise, it wouldn't reach this point.

Attention!

With this modification, the test #parse_http is called with hash options in spec/remote_fetching_spec.rb started to fail because it started to recognize the file PNG/anim.png as an mp3 as well!

I'm not sure if this behaviour can cause new bugs, wdyt?

Question

This file was authorized by the owner to use as an example. Should I mention his name in somewhere?` (I created a fixture file by myself)

Copy link
Contributor

@linkyndy linkyndy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice find!

Though, please remove the file, as mentioned in my DM to you ✌️

@fabioperrella
Copy link
Contributor Author

Though, please remove the file, as mentioned in my DM to you

I think I can create another file to reproduce the same problem, I will try it!

@fabioperrella
Copy link
Contributor Author

@linkyndy I replaced the file by other one created by myself!

@martijnvermaat
Copy link

I won't be able to review this until after my holiday, so please proceed without me

@martijnvermaat martijnvermaat removed their request for review September 9, 2020 15:59
Copy link
Contributor

@julik julik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lovely. I have some concerns about the offset arithmetic but let's see how the errors / detection rate look like.

@julik
Copy link
Contributor

julik commented Sep 11, 2020

@fabioperrella I will take a look at the spec

@julik julik self-assigned this Sep 11, 2020
@fabioperrella fabioperrella merged commit 2c1ddd8 into master Sep 16, 2020
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.

Does not return metadata for some mp3 files
5 participants