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

Hotfix Moov parser #178

Merged
merged 4 commits into from
Oct 5, 2020
Merged

Hotfix Moov parser #178

merged 4 commits into from
Oct 5, 2020

Conversation

lorenzograndi4
Copy link
Contributor

@lorenzograndi4 lorenzograndi4 commented Oct 5, 2020

Closes #177

This is blocking the nu backend release, since there are errors on spaceship which deployed 0.25.1 first

@@ -36,7 +36,7 @@ def as_json(*a)
# matches the type, drilling down if a list of atom names is given
def find_first_atom_by_path(atoms, *atom_types)
type_to_find = atom_types.shift
requisite = atoms.find { |e| e.atom_type == type_to_find }
requisite = atoms.find { |e| e.is_a?(Atom) && e.atom_type == type_to_find }
Copy link
Contributor

Choose a reason for hiding this comment

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

I still not understand why the commit 2c8adbf broke it, but this change fix the problem!

@fabioperrella
Copy link
Contributor

I think I found the real problem..

The method find_video_trak_atom sometimes does not find the video trak and it's returning an empty array:

def find_video_trak_atom(atoms)
  trak_atoms = find_atoms_by_path(atoms, ['moov', 'trak'])

  return [] if trak_atoms.empty? # <---- here

Then, the method parse_dimensions assumes the empty array is a found video trak and it shouldn't!

def parse_dimensions(decoder, atom_tree)
  video_trak_atom = decoder.find_video_trak_atom(atom_tree)

  tkhd = begin
    if video_trak_atom # <----- here an empty array is considered TRUE
      decoder.find_first_atom_by_path([video_trak_atom], 'trak', 'tkhd')
    else

I think the fix should be in method find_video_trak_atom to return nil when it does not find it:

def find_video_trak_atom(atoms)
  trak_atoms = find_atoms_by_path(atoms, ['moov', 'trak'])

  return nil if trak_atoms.empty? # <---- here

With that modification, all the tests passed and I tested a file from production having this new issue and it also worked!

wdyt?

Copy link
Contributor

@fabioperrella fabioperrella left a comment

Choose a reason for hiding this comment

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

I left a comment before about the problem that I found

@lorenzograndi4
Copy link
Contributor Author

Good catch @fabioperrella :) the empty array played us.
I'd still leave the class check in the other line, it shouldn't hurt?

Copy link
Contributor

@fabioperrella fabioperrella left a comment

Choose a reason for hiding this comment

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

nice!

@fabioperrella
Copy link
Contributor

fabioperrella commented Oct 5, 2020

I'd still leave the class check in the other line, it shouldn't hurt?

I don't think it's a good choice, it would have hidden this problem, don't you think?

@lorenzograndi4 lorenzograndi4 merged commit 5d94ecb into master Oct 5, 2020
@lorenzograndi4 lorenzograndi4 deleted the hotfix-moov-parser branch October 5, 2020 12:51
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.

Fix NoMethodError in Moov parser
2 participants