Media fragment parser #23774

sreeise · 2019-07-15T06:46:00Z

Media fragment parser for audio and video.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #22366 (GitHub issue number if applicable)

There are tests for these changes OR
These changes do not require tests because ___

This change is

highfive · 2019-07-15T06:46:07Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/audiotrack.rs, components/script/dom/mod.rs, components/script/dom/htmlmediaelement.rs, components/script/dom/mediafragmentparser.rs, components/script/dom/videotrack.rs
@KiChjang: components/script/dom/audiotrack.rs, components/script/dom/mod.rs, components/script/dom/htmlmediaelement.rs, components/script/dom/mediafragmentparser.rs, components/script/dom/videotrack.rs

sreeise · 2019-07-15T07:05:44Z

Hey guys. There are some odd parts about the specification that I wanted to note as it relates to the changes in the PR.

First, when a video track is added to the HTML media element the spec says:

If either the media resource or the URL of the current media resource indicate a particular set of video tracks to enable, or if the user agent has information that would facilitate the selection of specific video tracks to improve the user's experience, then: if this video track is the first such video track, then set enable to true, otherwise, set enable to false.

When it describes the track as the first such track I assumed this to mean the video track at the zeroth index in the video track list but I can see how this could also mean if this track is the first such track with that specific media fragment.

Second, I didn't include SMPTE for temporal fragments. I wasn't sure if Servo would use the frames portion of the fragment which is the only difference between SMPTE and NPT. This can be included rather easy if needed.

Last, the spec for HTML media element is not clear on using the end time. I am thinking that somehow the duration could be set to the end time and the player would need to stop at that time. Or, the start and end time could be used to request only that portion of the resource.

sreeise · 2019-07-15T07:13:42Z

For real world clock time I followed RFC 2326 section 3.7.

ferjm

Beautiful work!

ferjm · 2019-07-18T11:05:59Z

tests/wpt/mozilla/tests/mozilla/media_fragment_seek.html

@@ -0,0 +1,39 @@
+<!doctype html>


This is a nice test. I would move it to web-platform-tests/html/semantics/embedded-content/media-elements so it is pushed upstream.

FYI, there is also a very complete set of tests at https://www.w3.org/2008/WebVideo/Fragments/TC/ua-test-cases in case that you want to port some of them here.

Ahh thats good. I incorporated some of those in the test case. I can add more if you would like or another test for invalid semantics.

Thanks. Up to you :) More tests can be added as a follow-up.

ferjm · 2019-07-18T11:05:59Z

components/script/dom/mediafragmentparser.rs

@@ -0,0 +1,355 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public


This can probably be tweaked a little bit (removing the DOMString dependency) so it can be published as a standalone crate. File a follow-up issue for this, please.

Sounds good.

ferjm · 2019-07-18T11:07:22Z

I am thinking that somehow the duration could be set to the end time and the player would need to stop at that time. Or, the start and end time could be used to request only that portion of the resource.

The former looks like a better option to me, as the latter likely depends on byte-range requests support.

ferjm · 2019-07-18T11:07:31Z

@bors-servo try=wpt

bors-servo · 2019-07-18T11:07:36Z

⌛ Trying commit 6fea137 with merge 4a74194...


        Auto merge of #23774 - sreeise:media_fragment_parser, r=<try>

Media fragment parser  Media fragment parser for audio and video. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #22366 (GitHub issue number if applicable)  - [X] There are tests for these changes OR - [ ] These changes do not require tests because ___    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23774)

bors-servo · 2019-07-18T11:57:33Z

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
State: approved= try=True


        Media fragment parser

servo-wpt-sync · 2019-07-23T01:44:45Z

Opened new PR for upstreamable changes.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#17987.

ferjm · 2019-07-23T08:08:16Z

Thanks!
@bors-servo r+

bors-servo · 2019-07-23T08:08:17Z

📌 Commit dc11219 has been approved by ferjm

bors-servo · 2019-07-23T09:00:05Z

⌛ Testing commit dc11219 with merge fdbb317...


        Auto merge of #23774 - sreeise:media_fragment_parser, r=ferjm

Media fragment parser  Media fragment parser for audio and video. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #22366 (GitHub issue number if applicable)  - [X] There are tests for these changes OR - [ ] These changes do not require tests because ___    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23774)

bors-servo · 2019-07-23T11:15:30Z

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
Approved by: ferjm
Pushing fdbb317 to master...

nox · 2019-07-25T17:56:41Z

components/script/dom/mediafragmentparser.rs

+}
+
+// Parse a full URL or a relative URL without a base retaining the query and/or fragment.
+fn split_url(s: &str) -> (DOMString, DOMString) {


Shouldn't this and other parts of this module be using the url crate?

This is mostly for relative URL's which could be passed to the parser from HTMLMediaElement for the video/audio src. These may have a relative file location with a query or fragment such as: resources/video.mp4#t=10

highfive assigned nox Jul 15, 2019

highfive added the S-awaiting-review label Jul 15, 2019

ferjm assigned ferjm and unassigned nox Jul 15, 2019

ferjm approved these changes Jul 18, 2019

View changes

ferjm added S-needs-code-changes and removed S-awaiting-review labels Jul 18, 2019

Media fragment parser

Loading status checks…

dc11219

sreeise force-pushed the sreeise:media_fragment_parser branch from 6fea137 to dc11219 Jul 23, 2019

highfive added S-awaiting-review and removed S-needs-code-changes labels Jul 23, 2019

servo-wpt-sync mentioned this pull request Jul 23, 2019

Media fragment parser web-platform-tests/wpt#17987

Merged

highfive added S-awaiting-merge and removed S-awaiting-review labels Jul 23, 2019

bors-servo merged commit dc11219 into servo:master Jul 23, 2019
3 checks passed

3 checks passed

Taskcluster (pull_request) TaskGroup: success
Details

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Jul 23, 2019

sreeise mentioned this pull request Jul 23, 2019

Change uses of DOMString in MediaFragmentParser #23834

Closed

nox reviewed Jul 25, 2019

View changes

servo / servo

Media fragment parser #23774

Media fragment parser #23774

sreeise commented Jul 15, 2019 •

edited by SimonSapin

highfive commented Jul 15, 2019

sreeise commented Jul 15, 2019

sreeise commented Jul 15, 2019

ferjm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ferjm commented Jul 18, 2019

ferjm commented Jul 18, 2019

bors-servo commented Jul 18, 2019

bors-servo commented Jul 18, 2019

servo-wpt-sync commented Jul 23, 2019

ferjm commented Jul 23, 2019

bors-servo commented Jul 23, 2019

bors-servo commented Jul 23, 2019

bors-servo commented Jul 23, 2019

This comment has been minimized.

This comment has been minimized.

		@@ -0,0 +1,355 @@
		/* This Source Code Form is subject to the terms of the Mozilla Public

servo / servo

Join GitHub today

Media fragment parser #23774

Media fragment parser #23774

Conversation

sreeise commented Jul 15, 2019 • edited by SimonSapin

highfive commented Jul 15, 2019

sreeise commented Jul 15, 2019

sreeise commented Jul 15, 2019

ferjm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ferjm commented Jul 18, 2019

ferjm commented Jul 18, 2019

bors-servo commented Jul 18, 2019

bors-servo commented Jul 18, 2019

servo-wpt-sync commented Jul 23, 2019

ferjm commented Jul 23, 2019

bors-servo commented Jul 23, 2019

bors-servo commented Jul 23, 2019

bors-servo commented Jul 23, 2019

This comment has been minimized.

This comment has been minimized.

sreeise commented Jul 15, 2019 •

edited by SimonSapin