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

Skip padding data #31

Merged
merged 1 commit into from
Sep 30, 2016
Merged

Skip padding data #31

merged 1 commit into from
Sep 30, 2016

Conversation

alfredoyang
Copy link
Contributor

This is from https://bugzilla.mozilla.org/show_bug.cgi?id=1306236.

Some contents may have extra padding data in the boxes of stbl. For example, stsc, stsz, stsd...etc.
This patch skips these paddings so parser keeps parsing the file.

Padding could be added in box inside stbl in some contents.
Copy link
Contributor

@rillian rillian left a comment

Choose a reason for hiding this comment

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

Is this described in a spec anywhere? I confirmed that stagefright skips everywhere this commit adds skip, but I didn't verify that was all the boxes where it does.

What about tests? Verifying this is especially important for undocumented parsing requirements.

let to_skip = b.bytes_left();
try!(skip(&mut b, to_skip));
SampleEntry::Unknown
{
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume the extra indent so iter releases it's reference to src for the subsequent skip? I don't see an alternate solution besides moving this code into a read_description helper function. That might not be a bad idea for readability anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it releases the borrow for the following skip.

}
}

// Padding could be added in some contents.
try!(skip_box_remain(src));
Copy link
Contributor

Choose a reason for hiding this comment

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

This will skip additional entries where description_count is short. That's probably fine.

@rillian
Copy link
Contributor

rillian commented Sep 30, 2016

Test failure seems to be an infrastructure problem. I'll go ahead and merge; please file new PRs for followup issues.

@rillian rillian merged commit 63212d9 into mozilla:master Sep 30, 2016
@alfredoyang
Copy link
Contributor Author

I'll add test at another PR.

I can't find any statement about the extra padding in 14496-12; however, there are 'free' and 'skip' box defined in spec. I think it should be a special case.

@rillian
Copy link
Contributor

rillian commented Oct 3, 2016

Ok, thanks.

@alfredoyang
Copy link
Contributor Author

alfredoyang commented Oct 5, 2016

I check the bitstream again. These paddings are 'skip' boxes actually.
The layout is:

stsd box
|- stsd data
|- skip box
|- skip box

stsc box
|- stsc data
|- skip box

stsz box
|- stsz data
|- skip box

I don't think it is correct behaviour because 'stsd' is not a container box. My dump tool MP4Box can't parse this kind of content correctly neither.

There are 2 ways in my mind currently.

  1. Add a function to jump over 'skip' box at the end of read_stsd, read_stsc...etc. Just like this pull.
  2. Jump over 'skip' box in check_parser_state(). So all the 'skip' boxes will be omitted in read_stbl.

Any idea?

@rillian
Copy link
Contributor

rillian commented Oct 6, 2016

Hmm. I think this is just broken software. These (stsd, stsc, stsz) are all variable-length boxes. Maybe it leaves some padding while writing the header and then fills in skip boxes later?

Do we need to do more than we did with this PR? Skipping to the end of these boxes, regardless of the contents seems safer than trying to interpret them, when we want compatibility with what stagefright accepts.

@alfredoyang
Copy link
Contributor Author

Ok, we don't need to do anything for these contents. I'll submit another PR for test later.

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