WIP: Implement HTMLMediaElement.loop attribute #22321

kkpoon · 2018-11-30T05:41:09Z

Implement HTMLMediaElement.loop attribute

enabled loop attribute in HTMLMediaElement
initialised loop value as Cell::new(false)
set playback_position to default_playback_start_position in PlayerEvent::EndOfStream event when loop is set

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

There are tests for these changes OR

This change is

highfive · 2018-11-30T05:41:15Z

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

highfive · 2018-11-30T05:41:16Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/htmlmediaelement.rs
@KiChjang: components/script/dom/htmlmediaelement.rs

highfive · 2018-11-30T05:41:18Z

Warning

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

KiChjang

Thanks for working on this! I have some minor nits and a general question regarding the playback ended handler.

KiChjang · 2018-11-30T07:10:22Z

components/script/dom/htmlmediaelement.rs

@@ -163,6 +163,8 @@ pub struct HTMLMediaElement {
    paused: Cell<bool>,
    /// <https://html.spec.whatwg.org/multipage/#attr-media-autoplay>
    autoplaying: Cell<bool>,
+    /// <https://html.spec.whatwg.org/multipage/media.html#attr-media-loop>


Similar to the doc comments above, you'll need to remove media.html from the URL.

KiChjang · 2018-11-30T07:10:22Z

components/script/dom/htmlmediaelement.rs

@@ -163,6 +163,8 @@ pub struct HTMLMediaElement {
    paused: Cell<bool>,
    /// <https://html.spec.whatwg.org/multipage/#attr-media-autoplay>
    autoplaying: Cell<bool>,
+    /// <https://html.spec.whatwg.org/multipage/media.html#attr-media-loop>
+    loop: Cell<bool>,


loop is a reserved keyword in Rust, so I would be very surprised if this passed compilation.

I believe we don't need to add the attribute here in this case. We can use the make_bool_getter! and make_bool_setter! macros instead and use self.Loop() to check the attribute's value. You can take the autoplay attribute as an example.

KiChjang · 2018-11-30T07:10:22Z

components/script/dom/htmlmediaelement.rs

@@ -1218,6 +1221,12 @@ impl HTMLMediaElement {
                if self.ready_state.get() < ReadyState::HaveMetadata {
                    self.queue_dedicated_media_source_failure_steps();
                }
+                if self.loop.get() {                    


The step annotations above indicate that this may be the incorrect location to add the looping functionality. I suspect the concept of the end of the media resource may not even be implemented yet. @ferjm Am I correct in claiming so? If not, where do we handle the playback ended event?

The concept of the end of the media resource is not implemented yet. It should be done as part of #22294.

I do believe this is the right place to add the looping functionality. But apart from seeking to the start position we need to do a couple of things more:

Do an early return if the previous condition is true, as we don't want to loop if there are errors.

Add a XXX comment saying that we need to identify if the stream is seekable or not to decide if we can loop. Please, reference #22297 in the comment.

Set playback_position to default_playback_start_position here.

ferjm

Thanks for the PR! This is a good start, but we need to make some changes. I left some comments in the code.

We need to add the loop attribute to the HTMLMediaElement WebIDL file. It's already there, but commented out.

We have a Web Platform Test at tests/wpt/web-platform-tests/html/semantics/embedded-content/media-elements/audio_loop_base.html to test this functionality. You will likely need to update the tests expectations.

Finally, please, test that the code builds and works as expected locally. Note that the looping step may take some time as we don't have a media cache yet, so we discard everything that we play and seeking to the start will require fetching the data again from the network.

ferjm · 2018-11-30T13:45:31Z

components/script/dom/htmlmediaelement.rs

@@ -163,6 +163,8 @@ pub struct HTMLMediaElement {
    paused: Cell<bool>,
    /// <https://html.spec.whatwg.org/multipage/#attr-media-autoplay>
    autoplaying: Cell<bool>,
+    /// <https://html.spec.whatwg.org/multipage/media.html#attr-media-loop>
+    loop: Cell<bool>,


I believe we don't need to add the attribute here in this case. We can use the make_bool_getter! and make_bool_setter! macros instead and use self.Loop() to check the attribute's value. You can take the autoplay attribute as an example.

ferjm · 2018-11-30T13:45:31Z

components/script/dom/htmlmediaelement.rs

@@ -1218,6 +1221,12 @@ impl HTMLMediaElement {
                if self.ready_state.get() < ReadyState::HaveMetadata {
                    self.queue_dedicated_media_source_failure_steps();
                }
+                if self.loop.get() {                    


The concept of the end of the media resource is not implemented yet. It should be done as part of #22294.

I do believe this is the right place to add the looping functionality. But apart from seeking to the start position we need to do a couple of things more:

Do an early return if the previous condition is true, as we don't want to loop if there are errors.

Add a XXX comment saying that we need to identify if the stream is seekable or not to decide if we can loop. Please, reference #22297 in the comment.

Set playback_position to default_playback_start_position here.

kkpoon · 2018-11-30T16:52:30Z

Thanks for the comments. Working on it :D

kkpoon · 2018-12-03T14:08:41Z

Working hard to compile it locally...


        Implement HTMLMediaElement.loop attribute


        Add update-wpt output

kkpoon · 2018-12-06T09:14:38Z

Hi @ferjm, test-wpt and update-wpt have been run and after checking the tests updated by update-wpt, seem only tests/wpt/metadata/html/dom/interfaces.https.html.ini is related. Yet, it removes the testes about loop, is this expected?

ferjm · 2018-12-06T15:07:10Z

Yes, that's expected. That test is checking the interfaces exposed to web content. We weren't exposing the loop attribute before this patch, that's why the test was failing in the loop related checks and why we needed to whitelist the failures. Now we are exposing this attribute, so the test shouldn't fail anymore.

Did you check if https://github.com/servo/servo/blob/master/tests/wpt/web-platform-tests/html/semantics/embedded-content/media-elements/audio_loop_base.html is passing?


        Merge branch 'master' of github.com:servo/servo into impl-HTMLMediaEl…

…ement-loop


        Auto merge of #22348 - germangb:html_media_ended, r=<try>

Implement Ended media attribute  This PR should implement: * New method `HTMLMediaElement::earliest_possible_position()` for the [earliest possible position](https://html.spec.whatwg.org/multipage/media.html#earliest-possible-position) * `Ended` attribute following https://html.spec.whatwg.org/multipage/media.html#ended-playback * Queue steps for when the playback position reaches the end This PR contains placeholders for the following issues (I can rebase changes after the corresponding PRs get merged) - #22321 (Define the Loop attribute) - #22293 (To identify playback direction. Either forwards or backwards) ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #22294.    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22348)

kkpoon · 2018-12-08T01:58:44Z

Yes, that's expected. That test is checking the interfaces exposed to web content. We weren't exposing the loop attribute before this patch, that's why the test was failing in the loop related checks and why we needed to whitelist the failures. Now we are exposing this attribute, so the test shouldn't fail anymore.

Did you check if https://github.com/servo/servo/blob/master/tests/wpt/web-platform-tests/html/semantics/embedded-content/media-elements/audio_loop_base.html is passing?

Hi @ferjm

I check the servo.log from test-wtp,

{"source": "web-platform-tests", "test": "/html/semantics/embedded-content/media-elements/audio_loop_base.html", "thread": "MainThread", "time": 1544206279706, "action": "test_start", "pid": 2564}
{"status": "SKIP", "source": "web-platform-tests", "test": "/html/semantics/embedded-content/media-elements/audio_loop_base.html", "thread": "MainThread", "time": 1544206279706, "action": "test_end", "pid": 2564}

It seems being skipped. Any idea about this?, I run it on a machine in DigitalOcean. ubuntu-1804 8ram4vcpu.

Thanks

jdm · 2018-12-08T10:43:23Z

https://github.com/servo/servo/blob/master/tests/wpt/metadata/html/semantics/embedded-content/media-elements/audio_loop_base.html.ini#L3 shows that this test has been disabled. Try removing that line!


        Auto merge of #22348 - germangb:html_media_ended, r=ferjm

Implement Ended media attribute  This PR should implement: * New method `HTMLMediaElement::earliest_possible_position()` for the [earliest possible position](https://html.spec.whatwg.org/multipage/media.html#earliest-possible-position) * `Ended` attribute following https://html.spec.whatwg.org/multipage/media.html#ended-playback * Queue steps for when the playback position reaches the end This PR contains placeholders for the following issues (I can rebase changes after the corresponding PRs get merged) - #22321 (Define the Loop attribute) - #22293 (To identify playback direction. Either forwards or backwards) ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [x] These changes fix #22294.    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22348)


        Auto merge of #22348 - germangb:html_media_ended, r=ferjm

Implement Ended media attribute  This PR should implement: * New method `HTMLMediaElement::earliest_possible_position()` for the [earliest possible position](https://html.spec.whatwg.org/multipage/media.html#earliest-possible-position) * `Ended` attribute following https://html.spec.whatwg.org/multipage/media.html#ended-playback * Queue steps for when the playback position reaches the end This PR contains placeholders for the following issues (I can rebase changes after the corresponding PRs get merged) - #22321 (Define the Loop attribute) - #22293 (To identify playback direction. Either forwards or backwards) ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [x] These changes fix #22294.    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22348)


        Auto merge of #22348 - germangb:html_media_ended, r=ferjm

Implement Ended media attribute  This PR should implement: * New method `HTMLMediaElement::earliest_possible_position()` for the [earliest possible position](https://html.spec.whatwg.org/multipage/media.html#earliest-possible-position) * `Ended` attribute following https://html.spec.whatwg.org/multipage/media.html#ended-playback * Queue steps for when the playback position reaches the end This PR contains placeholders for the following issues (I can rebase changes after the corresponding PRs get merged) - #22321 (Define the Loop attribute) - #22293 (To identify playback direction. Either forwards or backwards) ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [x] These changes fix #22294.    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22348)

ferjm · 2018-12-18T09:35:50Z

@bors-servo try=wpt

bors-servo · 2018-12-18T09:35:56Z

⌛ Trying commit ac71d3d with merge 2840685...


        Auto merge of #22321 - kkpoon:impl-HTMLMediaElement-loop, r=<try>

WIP: Implement HTMLMediaElement.loop attribute Implement HTMLMediaElement.loop attribute - enabled `loop` attribute in `HTMLMediaElement` - initialised `loop` value as `Cell::new(false)` - set `playback_position` to `default_playback_start_position` in `PlayerEvent::EndOfStream` event when `loop` is set --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #22290 (github issue number if applicable). --- - [ ] There are tests for these changes OR  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22321)


        Merge branch 'master' of github.com:servo/servo into impl-HTMLMediaEl…

…ement-loop


        Seek to default playback position

kkpoon · 2018-12-22T03:42:56Z

I get the test fail about reached the unreachable code. I think it is the media error event at here https://github.com/servo/servo/blob/master/tests/wpt/web-platform-tests/html/semantics/embedded-content/media-elements/audio_loop_base.html#L35

Any way to check about the detail of that error?


        Auto merge of #22348 - germangb:html_media_ended, r=ferjm

Implement Ended media attribute  This PR should implement: * New method `HTMLMediaElement::earliest_possible_position()` for the [earliest possible position](https://html.spec.whatwg.org/multipage/media.html#earliest-possible-position) * `Ended` attribute following https://html.spec.whatwg.org/multipage/media.html#ended-playback * Queue steps for when the playback position reaches the end This PR contains placeholders for the following issues (I can rebase changes after the corresponding PRs get merged) - #22321 (Define the Loop attribute) - #22293 (To identify playback direction. Either forwards or backwards) ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [x] These changes fix #22294.    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22348)

ferjm · 2018-12-26T15:23:06Z

You should be able to get the error from the error attribute of the media element. However, I suspect that these tests may give you intermittent results until I manage to land #22477.

ferjm · 2019-01-10T14:06:57Z

@kkpoon #22477 has just landed, so you should experience more stability with the WPTs after rebasing on top of master.

jdm · 2019-01-10T15:42:03Z

@bors-servo try=wpt


        Auto merge of #22321 - kkpoon:impl-HTMLMediaElement-loop, r=<try>

WIP: Implement HTMLMediaElement.loop attribute Implement HTMLMediaElement.loop attribute - enabled `loop` attribute in `HTMLMediaElement` - initialised `loop` value as `Cell::new(false)` - set `playback_position` to `default_playback_start_position` in `PlayerEvent::EndOfStream` event when `loop` is set --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #22290 (github issue number if applicable). --- - [ ] There are tests for these changes OR  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22321)

bors-servo · 2019-01-10T15:42:12Z

⌛ Trying commit 3dccbdd with merge 9b574db...

bors-servo · 2019-01-10T15:48:59Z

💔 Test failed - linux-rel-css


        Auto merge of #22348 - germangb:html_media_ended, r=<try>

Implement Ended media attribute  This PR should implement: * New method `HTMLMediaElement::earliest_possible_position()` for the [earliest possible position](https://html.spec.whatwg.org/multipage/media.html#earliest-possible-position) * `Ended` attribute following https://html.spec.whatwg.org/multipage/media.html#ended-playback * Queue steps for when the playback position reaches the end This PR contains placeholders for the following issues (I can rebase changes after the corresponding PRs get merged) - #22321 (Define the Loop attribute) - #22293 (To identify playback direction. Either forwards or backwards) ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [x] These changes fix #22294.    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22348)

bors-servo · 2019-01-11T15:44:42Z

☔ The latest upstream changes (presumably #22522) made this pull request unmergeable. Please resolve the merge conflicts.


        Auto merge of #22348 - germangb:html_media_ended, r=<try>

Implement Ended media attribute  This PR should implement: * New method `HTMLMediaElement::earliest_possible_position()` for the [earliest possible position](https://html.spec.whatwg.org/multipage/media.html#earliest-possible-position) * `Ended` attribute following https://html.spec.whatwg.org/multipage/media.html#ended-playback * Queue steps for when the playback position reaches the end This PR contains placeholders for the following issues (I can rebase changes after the corresponding PRs get merged) - #22321 (Define the Loop attribute) - #22293 (To identify playback direction. Either forwards or backwards) ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [x] These changes fix #22294.    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22348)


        Auto merge of #22348 - germangb:html_media_ended, r=ferjm

Implement Ended media attribute  This PR should implement: * New method `HTMLMediaElement::earliest_possible_position()` for the [earliest possible position](https://html.spec.whatwg.org/multipage/media.html#earliest-possible-position) * `Ended` attribute following https://html.spec.whatwg.org/multipage/media.html#ended-playback * Queue steps for when the playback position reaches the end This PR contains placeholders for the following issues (I can rebase changes after the corresponding PRs get merged) - #22321 (Define the Loop attribute) - #22293 (To identify playback direction. Either forwards or backwards) ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [x] These changes fix #22294.    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22348)

ferjm · 2019-02-01T09:16:50Z

@kkpoon sorry, I mistakenly closed #22290. Are you still working on this?

kkpoon · 2019-02-01T09:18:48Z

Oh, I just though it is done.
Yes, I still want to try on this. Just almost finish one in PerformanceResourceTiming.

ferjm · 2019-03-11T17:30:17Z

@kkpoon how are things going? Are you still interested in working on this issue?

kkpoon · 2019-03-12T07:06:53Z

@ferjm Sorry that I am not able to work on this at this moment. You may take over this PR or close it for other to work on this.

Haven't update the status and I am sorry about that.

ferjm · 2019-03-12T12:11:02Z

No worries! Let's close this for now. Feel free to reopen if you find the time to work on it again. Thanks!

highfive assigned avadacatavra Nov 30, 2018

highfive added the S-awaiting-review label Nov 30, 2018

KiChjang requested changes Nov 30, 2018

View changes

ferjm requested changes Nov 30, 2018

View changes

kkpoon changed the title ~~Implement HTMLMediaElement.loop attribute~~ WIP: Implement HTMLMediaElement.loop attribute Nov 30, 2018

kkpoon force-pushed the kkpoon:impl-HTMLMediaElement-loop branch 2 times, most recently from aa0cbad to 0f09af3 Dec 1, 2018

germangb mentioned this pull request Dec 1, 2018

Implement Ended media attribute #22348

Merged

3 of 3 tasks complete

Implement HTMLMediaElement.loop attribute

Loading status checks…

1ca5944

kkpoon force-pushed the kkpoon:impl-HTMLMediaElement-loop branch from 0f09af3 to 1ca5944 Dec 5, 2018

Add update-wpt output

Loading status checks…

1852bb3

Merge branch 'master' of github.com:servo/servo into impl-HTMLMediaEl…

Loading status checks…

ac71d3d

…ement-loop

jdm assigned ferjm and unassigned avadacatavra Dec 17, 2018

ferjm added S-needs-code-changes and removed S-awaiting-review labels Dec 18, 2018

kkpoon added 2 commits Dec 21, 2018

Merge branch 'master' of github.com:servo/servo into impl-HTMLMediaEl…

17db38b

…ement-loop

Seek to default playback position

Loading status checks…

3dccbdd

highfive added S-awaiting-review and removed S-tests-failed S-needs-code-changes labels Dec 22, 2018

highfive added the S-tests-failed label Jan 10, 2019

highfive added the S-needs-rebase label Jan 11, 2019

ferjm mentioned this pull request Jan 30, 2019

Implement HTMLMediaElement.loop attribute #22290

Closed

kkpoon closed this Feb 1, 2019

kkpoon reopened this Feb 1, 2019

ferjm closed this Mar 12, 2019

servo / servo

WIP: Implement HTMLMediaElement.loop attribute #22321

WIP: Implement HTMLMediaElement.loop attribute #22321

kkpoon commented Nov 30, 2018 •

edited

highfive commented Nov 30, 2018

highfive commented Nov 30, 2018

highfive commented Nov 30, 2018

KiChjang 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.

ferjm left a comment

This comment has been minimized.

This comment has been minimized.

kkpoon commented Nov 30, 2018 •

edited

kkpoon commented Dec 3, 2018

kkpoon commented Dec 6, 2018

ferjm commented Dec 6, 2018

kkpoon commented Dec 8, 2018

jdm commented Dec 8, 2018

ferjm commented Dec 18, 2018

bors-servo commented Dec 18, 2018

kkpoon commented Dec 22, 2018

ferjm commented Dec 26, 2018

ferjm commented Jan 10, 2019

jdm commented Jan 10, 2019

bors-servo commented Jan 10, 2019

bors-servo commented Jan 10, 2019

bors-servo commented Jan 11, 2019

ferjm commented Feb 1, 2019

kkpoon commented Feb 1, 2019

ferjm commented Mar 11, 2019

kkpoon commented Mar 12, 2019

ferjm commented Mar 12, 2019

servo / servo

Join GitHub today

WIP: Implement HTMLMediaElement.loop attribute #22321

WIP: Implement HTMLMediaElement.loop attribute #22321

Conversation

kkpoon commented Nov 30, 2018 • edited

highfive commented Nov 30, 2018

highfive commented Nov 30, 2018

highfive commented Nov 30, 2018

KiChjang left a comment

This comment has been minimized.

KiChjang Nov 30, 2018

This comment has been minimized.

KiChjang Nov 30, 2018

This comment has been minimized.

ferjm Nov 30, 2018

This comment has been minimized.

KiChjang Nov 30, 2018

This comment has been minimized.

ferjm Nov 30, 2018

ferjm left a comment

This comment has been minimized.

ferjm Nov 30, 2018

This comment has been minimized.

ferjm Nov 30, 2018

kkpoon commented Nov 30, 2018 • edited

kkpoon commented Dec 3, 2018

kkpoon commented Dec 6, 2018

ferjm commented Dec 6, 2018

kkpoon commented Dec 8, 2018

jdm commented Dec 8, 2018

ferjm commented Dec 18, 2018

bors-servo commented Dec 18, 2018

kkpoon commented Dec 22, 2018

ferjm commented Dec 26, 2018

ferjm commented Jan 10, 2019

jdm commented Jan 10, 2019

bors-servo commented Jan 10, 2019

bors-servo commented Jan 10, 2019

bors-servo commented Jan 11, 2019

ferjm commented Feb 1, 2019

kkpoon commented Feb 1, 2019

ferjm commented Mar 11, 2019

kkpoon commented Mar 12, 2019

ferjm commented Mar 12, 2019

kkpoon commented Nov 30, 2018 •

edited

kkpoon commented Nov 30, 2018 •

edited