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

Implement video-metadata check #12186

Merged
merged 10 commits into from Jul 30, 2016

Conversation

Projects
None yet
@GuillaumeGomez
Contributor

GuillaumeGomez commented Jul 2, 2016

This change is Reviewable

@highfive

This comment has been minimized.

highfive commented Jul 2, 2016

Heads up! This PR modifies the following files:

  • @KiChjang: components/script/lib.rs, components/script/Cargo.toml, components/script/dom/htmlmediaelement.rs
@highfive

This comment has been minimized.

highfive commented Jul 2, 2016

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@GuillaumeGomez

This comment has been minimized.

Contributor

GuillaumeGomez commented Jul 2, 2016

r? @jdm

@highfive highfive assigned jdm and unassigned larsbergstrom Jul 2, 2016

@@ -167,6 +181,16 @@ impl HTMLMediaElementContext {
}
#[dom_struct]
pub struct VideoMedia {

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Jul 2, 2016

Contributor

The error happens here.

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:video-metadata branch from c50e49d to 0379f33 Jul 2, 2016

@GuillaumeGomez

This comment has been minimized.

Contributor

GuillaumeGomez commented Jul 2, 2016

I now need to add libraries for tests and build. Anywhere I could start to look at?

@emilio

This comment has been minimized.

Member

emilio commented Jul 3, 2016

For travis, adding the corresponding -dev sources to APT should work. For homu, you probably need to get a PR merged in servo/saltfs.

@emilio

This comment has been minimized.

Member

emilio commented Jul 3, 2016

I'd like to review the new dependency carefully, since it seems it contains kind-of-sensible FFI code.

@emilio

This comment has been minimized.

Member

emilio commented Jul 3, 2016

Oh, and also, please update the dependency section at the README.
Thanks btw! :)

@emilio

This comment has been minimized.

Member

emilio commented Jul 3, 2016

Yeah, I'm reviewing the dependency right now, @GuillaumeGomez, would you accept some patches? :)

@GuillaumeGomez

This comment has been minimized.

Contributor

GuillaumeGomez commented Jul 3, 2016

For travis, adding the corresponding -dev sources to APT should work. For homu, you probably need to get a PR merged in servo/saltfs.

I'm using version 3.x of libraries and travis only provides 2.x (like every other linux package managers as it seems).

Yeah, I'm reviewing the dependency right now, @GuillaumeGomez, would you accept some patches? :)

I would very gladly! :)

@nox

This comment has been minimized.

Member

nox commented Jul 3, 2016

You probably need to run ./mach cargo-update -p script, btw.

@@ -166,6 +180,19 @@ impl HTMLMediaElementContext {
}
}
no_jsmanaged_fields!(Duration);

This comment has been minimized.

@izgzhen

izgzhen Jul 4, 2016

Contributor

Adding this to script/dom/bindings/trace.rs would be easier to maintain ;)

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Jul 4, 2016

Contributor

Sure.

@notriddle notriddle assigned notriddle and unassigned jdm Jul 4, 2016

@GuillaumeGomez

This comment has been minimized.

Contributor

GuillaumeGomez commented Jul 5, 2016

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:video-metadata branch from 4de77d2 to fb9e80a Jul 5, 2016

@notriddle

This comment has been minimized.

Contributor

notriddle commented Jul 5, 2016

Could you squash all the commits down into one?

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:video-metadata branch from fb9e80a to de98063 Jul 5, 2016

@notriddle

This comment has been minimized.

Contributor

notriddle commented Jul 5, 2016

components/script/dom/htmlmediaelement.rs, line 110 [r1] (raw file):

            // Step 6
            elem.change_ready_state(HAVE_METADATA);
            self.have_metadata = true;

If we only got a partial video header, get_format_from_slice will fail, elem.video will be None, but have_metadata will still be set. Then we'll get the rest of it, but we won't parse it.


Comments from Reviewable

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:video-metadata branch from de98063 to 39c5fc6 Jul 5, 2016

@GuillaumeGomez

This comment has been minimized.

Contributor

GuillaumeGomez commented Jul 5, 2016

Review status: 0 of 8 files reviewed at latest revision, 3 unresolved discussions.


components/script/dom/htmlmediaelement.rs, line 110 [r1] (raw file):

Previously, notriddle (Michael Howell) wrote…

If we only got a partial video header, get_format_from_slice will fail, elem.video will be None, but have_metadata will still be set. Then we'll get the rest of it, but we won't parse it.

Done.

Comments from Reviewable

@notriddle

This comment has been minimized.

Contributor

notriddle commented Jul 5, 2016

a discussion (no related file):
That leaves us with only two problems:

  • We need the dependencies on the buildslaves.
  • We need Servo-level tests. WPT might already have some: I'll run bors try as soon as possible.

Comments from Reviewable

@notriddle

This comment has been minimized.

Contributor

notriddle commented Jul 5, 2016

Reviewed 4 of 4 files at r1, 1 of 1 files at r2, 6 of 6 files at r3, 1 of 1 files at r4, 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:video-metadata branch from 39c5fc6 to 7b9d232 Jul 5, 2016

@KiChjang

This comment has been minimized.

Member

KiChjang commented Jul 29, 2016

@KiChjang

This comment has been minimized.

Member

KiChjang commented Jul 29, 2016

@bors-servo r=larsbergstrom,jdm,KiChjang

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Jul 29, 2016

📌 Commit e1bf3c1 has been approved by larsbergstrom,jdm,KiChjang

@highfive highfive assigned larsbergstrom and unassigned jdm Jul 29, 2016

bors-servo added a commit that referenced this pull request Jul 29, 2016

Auto merge of #12186 - GuillaumeGomez:video-metadata, r=larsbergstrom…
…,jdm,KiChjang

Implement video-metadata check

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12186)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Jul 29, 2016

⌛️ Testing commit e1bf3c1 with merge 5be08b7...

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:video-metadata branch from e1bf3c1 to 44a86c6 Jul 29, 2016

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:video-metadata branch from 44a86c6 to 34cb551 Jul 29, 2016

@KiChjang

This comment has been minimized.

Member

KiChjang commented Jul 29, 2016

@bors-servo r=larsbergstrom,jdm,KiChjang

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Jul 29, 2016

📌 Commit 34cb551 has been approved by larsbergstrom,jdm,KiChjang

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Jul 29, 2016

⌛️ Testing commit 34cb551 with merge d053fb1...

bors-servo added a commit that referenced this pull request Jul 29, 2016

Auto merge of #12186 - GuillaumeGomez:video-metadata, r=larsbergstrom…
…,jdm,KiChjang

Implement video-metadata check

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12186)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Jul 29, 2016

💔 Test failed - mac-rel-wpt

@larsbergstrom

This comment has been minimized.

Contributor

larsbergstrom commented Jul 29, 2016

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Jul 29, 2016

⚡️ Previous build results for arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, windows-dev are reusable. Rebuilding only mac-rel-wpt...

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Jul 30, 2016

@bors-servo bors-servo merged commit 34cb551 into servo:master Jul 30, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@GuillaumeGomez GuillaumeGomez deleted the GuillaumeGomez:video-metadata branch Jul 30, 2016

bors-servo added a commit that referenced this pull request Aug 4, 2016

Auto merge of #12734 - servo:jdm-patch-1, r=ConnorGBrewster
Update mac prerequisites for libavformat requirements

These were missed in #12186.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12734)
<!-- Reviewable:end -->

@jdm jdm referenced this pull request Aug 24, 2016

Closed

Integrate mp4parse #13024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment