Put back video metadata #13094

GuillaumeGomez · 2016-08-28T13:17:32Z

I updated the video-metadata-rs crate: now, no more ffmpeg, just pure rust. The webm format isn't checked yet.

This change is

highfive · 2016-08-28T13:17:35Z

Heads up! This PR modifies the following files:

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

KiChjang · 2016-08-28T14:03:48Z

Does this require additional libraries to be installed on users' system?

GuillaumeGomez · 2016-08-28T14:08:48Z

No. That's the whole point and the great improvement.

jdm · 2016-08-28T15:02:29Z

cc @splav This will impact the plans for using mp4parse.

jdm · 2016-08-28T15:02:54Z

r? @Manishearth
By random selection, since I don't intend to review code this week.

GuillaumeGomez · 2016-08-28T15:03:19Z

@jdm: I'll put @Ms2ger on it then.

r? @Ms2ger

est31 · 2016-08-28T18:28:53Z

Related : #13024, #12769.

splav · 2016-08-28T19:15:02Z

First: The current metadata structure seems to be suitable only for video content. Are there any plans to support audio content as well? May we should make it more general to support both?
Second: mp4_parse essentially does not have rust API yet. So the crate uses own patched version that makes some structs public. I saw the PR for mp4_parse, but I don't think it's a good long-term solution and that it will be used.

GuillaumeGomez · 2016-08-28T19:16:46Z

I can make it more general (by providing a higher crate maybe? The name is video-metadata after all). And as for mp4parse, as long as it doesn't need strange dependencies, it's fine for the moment from my point of view.

splav · 2016-08-28T19:18:38Z

@GuillaumeGomez I was talking about Servo's structure. But sure, this can be changed later.

splav · 2016-08-28T19:19:50Z

As for video-metadata - may be it's ok, but ogg can contain audio as well and checking it twice in two crated seems a bit strange.

GuillaumeGomez · 2016-08-28T19:44:33Z

@splav: For the moment, the thing is to target video to get their size. But if there are pure crates to get audio metadata, I can update video-metadata to add them (I still wonder if I should rename it...).

est31 · 2016-08-28T20:56:29Z

My ogg_metadata crate which video-metadata-rs uses supports audio metadata parsing as well as video metadata parsing: it returns the duration and the sample frequency (at least for vorbis, opus has no fixed sampling frequency).

splav · 2016-08-28T21:20:48Z

@est31, @GuillaumeGomez my point that it should be one metadata wrapper crate for servo for both audio and video containers. The other option is to have one crate for audio and one for video leads to parsing ogg container twice: in both video and audio crates. It seems a bit strange for me.

And IMO the crate rename is the thing that should be done the earlier the better, in case the crate will support both audio and video metadata parsing.

GuillaumeGomez · 2016-08-29T00:16:42Z

I created the new crate (which handles both video and audio metadata) and updated this PR.

est31 · 2016-08-29T00:19:35Z

components/script/dom/htmlmediaelement.rs

+                let dur = meta.audio.duration.unwrap_or(::std::time::Duration::new(0, 0));
+                *elem.video.borrow_mut() = Some(VideoMedia {
+                    format: format!("{:?}", meta.format),
+                    duration: Duration::seconds(dur.as_secs() as i64) +


Maybe use Duration::from_std() here?

And why not just clone()? Or at least new()

@splav dur is of type std::time::Duration, while the VideoMedia struct requires time::Duration from the time crate.

nox · 2016-08-29T09:18:03Z

Think you could publish audio_video_metadata? Would be great to avoid yet another Git dependency.

GuillaumeGomez · 2016-08-29T09:48:03Z

@nox: No, I have mixed origin for this crate (because mp4rust didn't merge my PR yet) so I can't publish it for the moment.

bors-servo · 2016-08-29T11:49:22Z

⌛ Trying commit 04a9f1b with merge d9c324d...


        Auto merge of #13094 - GuillaumeGomez:the_comeback, r=<try>

Put back video metadata I updated the `video-metadata-rs` crate: now, no more ffmpeg, just pure rust. The webm format isn't checked yet.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13094)

GuillaumeGomez · 2016-08-29T11:49:23Z

@bors-servo: try retry

bors-servo · 2016-08-29T12:16:09Z

⌛ Trying commit 04a9f1b with merge 14c1a39...


        Auto merge of #13094 - GuillaumeGomez:the_comeback, r=<try>

Put back video metadata I updated the `video-metadata-rs` crate: now, no more ffmpeg, just pure rust. The webm format isn't checked yet.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13094)

bors-servo · 2016-08-29T12:26:18Z

💔 Test failed - linux-dev

nox · 2016-09-09T12:00:54Z

Not only this PR introduces timeouts, but it doesn't seem to make any new test pass.


        Put back video metadata

GuillaumeGomez · 2016-09-09T12:06:25Z

Updated.

jdm · 2016-09-09T13:01:24Z

The timeouts are fine, since the reflect the actual state of the world when it comes to audio elements. The current HTMLMediaElement code just lies about understanding audio metadata.

nox · 2016-09-09T13:02:50Z

@bors-servo r+

bors-servo · 2016-09-09T13:02:51Z

📌 Commit 94379bf has been approved by nox

bors-servo · 2016-09-09T15:46:48Z

⌛ Testing commit 94379bf with merge 60d4c03...


        Auto merge of #13094 - GuillaumeGomez:the_comeback, r=nox

Put back video metadata I updated the `video-metadata-rs` crate: now, no more ffmpeg, just pure rust. The webm format isn't checked yet.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13094)

bors-servo · 2016-09-09T15:47:00Z

💔 Test failed - arm64

KiChjang · 2016-09-09T19:27:38Z

@bors-servo r+

Little PIP-y boy is annoying

bors-servo · 2016-09-09T19:27:39Z

💡 This pull request was already approved, no need to approve it again.

This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.
There's another pull request that is currently being tested, blocking this pull request: #13208

bors-servo · 2016-09-09T19:27:40Z

📌 Commit 94379bf has been approved by KiChjang

KiChjang · 2016-09-09T19:28:20Z

@bors-servo retry

bors-servo · 2016-09-09T21:28:35Z

⌛ Testing commit 94379bf with merge 142578f...


        Auto merge of #13094 - GuillaumeGomez:the_comeback, r=KiChjang

Put back video metadata I updated the `video-metadata-rs` crate: now, no more ffmpeg, just pure rust. The webm format isn't checked yet.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13094)

bors-servo · 2016-09-09T22:41:09Z

☀️ Test successful - arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows-dev

highfive assigned jdm Aug 28, 2016

highfive added the S-awaiting-review label Aug 28, 2016

highfive assigned Manishearth and unassigned jdm Aug 28, 2016

highfive assigned Ms2ger and unassigned Manishearth Aug 28, 2016

est31 reviewed Aug 29, 2016
View changes

Put back video metadata

Loading status checks…

94379bf

nox removed the S-needs-rebase label Sep 9, 2016

highfive assigned nox and unassigned Ms2ger Sep 9, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Sep 9, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Sep 9, 2016

highfive assigned KiChjang and unassigned nox Sep 9, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels Sep 9, 2016

bors-servo merged commit 94379bf into servo:master Sep 9, 2016
3 checks passed

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

highfive removed the S-awaiting-merge label Sep 9, 2016

GuillaumeGomez deleted the GuillaumeGomez:the_comeback branch Sep 9, 2016

servo / servo

Join GitHub today

Put back video metadata #13094

Put back video metadata #13094

Conversation

GuillaumeGomez commented Aug 28, 2016 • edited by larsbergstrom

highfive commented Aug 28, 2016

KiChjang commented Aug 28, 2016

GuillaumeGomez commented Aug 28, 2016

jdm commented Aug 28, 2016

jdm commented Aug 28, 2016

GuillaumeGomez commented Aug 28, 2016

est31 commented Aug 28, 2016 • edited

splav commented Aug 28, 2016

GuillaumeGomez commented Aug 28, 2016

splav commented Aug 28, 2016

splav commented Aug 28, 2016

GuillaumeGomez commented Aug 28, 2016

est31 commented Aug 28, 2016

splav commented Aug 28, 2016

GuillaumeGomez commented Aug 29, 2016

This comment has been minimized.

est31 Aug 29, 2016

This comment has been minimized.

splav Aug 29, 2016 • edited

This comment has been minimized.

est31 Aug 29, 2016

This comment has been minimized.

splav Aug 29, 2016

nox commented Aug 29, 2016

GuillaumeGomez commented Aug 29, 2016

bors-servo commented Aug 29, 2016

GuillaumeGomez commented Aug 29, 2016 • edited

bors-servo commented Aug 29, 2016

bors-servo commented Aug 29, 2016

nox commented Sep 9, 2016

GuillaumeGomez commented Sep 9, 2016

jdm commented Sep 9, 2016

nox commented Sep 9, 2016

bors-servo commented Sep 9, 2016

bors-servo commented Sep 9, 2016

bors-servo commented Sep 9, 2016

KiChjang commented Sep 9, 2016

bors-servo commented Sep 9, 2016

bors-servo commented Sep 9, 2016

KiChjang commented Sep 9, 2016

bors-servo commented Sep 9, 2016

bors-servo commented Sep 9, 2016

GuillaumeGomez commented Aug 28, 2016 •

edited by larsbergstrom

est31 commented Aug 28, 2016 •

edited

splav Aug 29, 2016 •

edited

GuillaumeGomez commented Aug 29, 2016 •

edited