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

Calculate correct mp4 header length. #9618

Merged
merged 1 commit into from Feb 12, 2016
Merged

Conversation

@jongiddy
Copy link
Contributor

jongiddy commented Feb 12, 2016

The calculation of MP4 frame length is incorrect, shifting values by 1 bit instead of 8 bits. It works for the test MP4 file because the length of the frame is less than 256 bytes, so the shifted values are all zero.

This PR changes the code to calculate the length correctly. It adds a test to check a fake long stream. Still not long enough to test completely, but at least detects the problem with the original code.

Review on Reviewable

@highfive
Copy link

highfive commented Feb 12, 2016

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @pcwalton (or someone else) soon.

&[0x00, 0x00, 0x01, 0x04, 0x66, 0x74, 0x79, 0x70, 0x6D, 0x70, 0x34]
);

println!("Data Length {:?}", data.len());

This comment has been minimized.

@jdm

jdm Feb 12, 2016

Member

Let's remove this.

@mbrubeck
Copy link
Contributor

mbrubeck commented Feb 12, 2016

@bors-servo r+

Thanks!

@bors-servo
Copy link
Contributor

bors-servo commented Feb 12, 2016

📌 Commit 4819d9a has been approved by mbrubeck

);

println!("Data Length {:?}", data.len());
if !matcher.matches(&data) {

This comment has been minimized.

@jdm

jdm Feb 12, 2016

Member

assert!(matcher.matches(&data));

@mbrubeck
Copy link
Contributor

mbrubeck commented Feb 12, 2016

@bors-servo r- Sorry, didn't see @jdm's comment

@jdm
Copy link
Member

jdm commented Feb 12, 2016

This change looks great, and thank you for including a test! How did you notice this? :)

@jongiddy
Copy link
Contributor Author

jongiddy commented Feb 12, 2016

@jdm Just reading code to get more familiar with Rust.

@jdm
Copy link
Member

jdm commented Feb 12, 2016

Please squash the commits into one, and then I'll merge it. Thanks!

@jongiddy jongiddy force-pushed the jongiddy:fix-mp4-size branch from 0e5d8b9 to ec0ea46 Feb 12, 2016
@jongiddy
Copy link
Contributor Author

jongiddy commented Feb 12, 2016

Squashed and force pushed. Hope that's right. Thanks for the impressively quick response.

@jdm
Copy link
Member

jdm commented Feb 12, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Feb 12, 2016

📌 Commit ec0ea46 has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Feb 12, 2016

Testing commit ec0ea46 with merge 331ba84...

bors-servo added a commit that referenced this pull request Feb 12, 2016
Calculate correct mp4 header length.

The calculation of MP4 frame length is incorrect, shifting values by 1 bit instead of 8 bits.  It works for the test MP4 file because the length of the frame is less than 256 bytes, so the shifted values are all zero.

This PR changes the code to calculate the length correctly. It adds a test to check a fake long stream. Still not long enough to test completely, but at least detects the problem with the original code.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9618)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Feb 12, 2016

💔 Test failed - linux-rel

@jdm
Copy link
Member

jdm commented Feb 12, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Feb 12, 2016

Testing commit ec0ea46 with merge 520ca25...

bors-servo added a commit that referenced this pull request Feb 12, 2016
Calculate correct mp4 header length.

The calculation of MP4 frame length is incorrect, shifting values by 1 bit instead of 8 bits.  It works for the test MP4 file because the length of the frame is less than 256 bytes, so the shifted values are all zero.

This PR changes the code to calculate the length correctly. It adds a test to check a fake long stream. Still not long enough to test completely, but at least detects the problem with the original code.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9618)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Feb 12, 2016

💔 Test failed - mac-rel-wpt

@KiChjang
Copy link
Member

KiChjang commented Feb 12, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Feb 12, 2016

Previous build results for android, gonk, linux-dev, linux-rel, mac-dev-unit are reusable. Rebuilding only mac-rel-css, mac-rel-wpt...

@bors-servo
Copy link
Contributor

bors-servo commented Feb 12, 2016

@bors-servo bors-servo merged commit ec0ea46 into servo:master Feb 12, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.