[webvtt] Implement HTMLTrackElement src attribute.

RESOLVED FIXED in Firefox 50

Status

()

Core
Audio/Video: Playback
P3
normal
RESOLVED FIXED
2 years ago
5 months ago

People

(Reporter: bechen, Assigned: bechen)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs, {dev-doc-complete})

unspecified
mozilla50
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
When the src attribute is changed, we need to release the previous resource and reload the new src. Like the HMTLMediaElement::LoadResource HMTLMediaElement::AbortExistingLoads etc...
Keywords: dev-doc-needed
Priority: -- → P2
Mass change P2 -> P3
Priority: P2 → P3
(Assignee)

Comment 2

2 years ago
Created attachment 8768673 [details]
Bug 1281418 - Add testcase for changing the src of TrackElement.

Review commit: https://reviewboard.mozilla.org/r/62794/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62794/
Attachment #8768673 - Flags: review?(giles)
Attachment #8768674 - Flags: review?(giles)
(Assignee)

Comment 3

2 years ago
Created attachment 8768674 [details]
Bug 1281418 - Release and reload the vtt resource when the src attribute of TrackElement changed.

Review commit: https://reviewboard.mozilla.org/r/62796/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62796/
Comment on attachment 8768673 [details]
Bug 1281418 - Add testcase for changing the src of TrackElement.

https://reviewboard.mozilla.org/r/62794/#review59760

r=me with comments addressed.

::: dom/media/test/test_trackelementsrc.html:3
(Diff revision 1)
> +<!DOCTYPE HTML>
> +<html>
> +<head>

Please add a charset to the header to silence encoding warnings.

```
  <meta charset="utf-8">
```

::: dom/media/test/test_trackelementsrc.html:18
(Diff revision 1)
> +<pre id="test">
> +<script class="testbody" type="text/javascript">
> +SimpleTest.waitForExplicitFinish();
> +
> +SpecialPowers.pushPrefEnv({"set": [["media.webvtt.regions.enabled", true]]});
> +SpecialPowers.pushPrefEnv({"set": [["media.webvtt.enabled", true]]}, function() {

As I understand the new implementation, pushPrefEnv only applies the given set of permissions while the callback is active, so this line, which has no callback, doesn't do anything.

We don't care about region support, so try just removing this line. If the test works despite the region codes in basic.vtt, that's better. If the text actually depends on region support, add it to the list in the second pushPrefEnv call which runs the test.

::: dom/media/test/test_trackelementsrc.html:39
(Diff revision 1)
> +  is(video.textTracks.length, 1, "Length should be 1.");
> +  is(video.textTracks[0].cues.length, 6, "Cue length should be 6.");
> +
> +  trackElement.src = "sequential.vtt";
> +  video.play();
> +});

Would it be safer to move video.play(); up to immediately after video.appendChild()? That makes sure play() is called even if the loadedmetadata listener isn't called. I don't remember if a race is possible in this case though.

::: dom/media/test/test_trackelementsrc.html:44
(Diff revision 1)
> +});
> +
> +video.addEventListener("ended", function end() {
> +  if (trackElement.readyState <= 1) {
> +    return setTimeout(end, 0);
> +  }

Can you remove this respin, or convert it to an an is() check?

This seems like it could loop forever if there's some playback problem, causing a timeout and slowing down the tests.

What is this protecting against? If 'ended' fires with the wrong ready state the video.textTracks checks below will probably still fail the tests, so we would still catch the problem.
Attachment #8768673 - Flags: review?(giles) → review+
Comment on attachment 8768674 [details]
Bug 1281418 - Release and reload the vtt resource when the src attribute of TrackElement changed.

https://reviewboard.mozilla.org/r/62796/#review59774

::: dom/html/HTMLTrackElement.cpp:201
(Diff revision 1)
> +    mMediaParent->RemoveTextTrack(mTrack);
> +    // Recreate mTrack.
> +    TextTrackMode oldMode = mTrack->Mode();
> +    CreateTextTrack();
> +    mTrack->SetMode(oldMode);
> +  }

It seems odd to preserve the mode here. Is this a proxy for the HTML attribute? Or does the spec say it should be preserved if set?

A WebPlatformTest should probably check this.

::: dom/html/HTMLTrackElement.cpp:214
(Diff revision 1)
> +  if (!mLoadResourceDispatched) {
> +    RefPtr<Runnable> r = NewRunnableMethod(this, &HTMLTrackElement::LoadResource);
> +    nsContentUtils::RunInStableState(r.forget());
> +    mLoadResourceDispatched = true;
> +  }
> +}

Since these 5 lines are duplicated in BindToTree(), it's better to move them to a private helper method.

::: dom/html/HTMLTrackElement.cpp:299
(Diff revision 1)
> +    if (!mLoadResourceDispatched) {
> -    RefPtr<Runnable> r = NewRunnableMethod(this, &HTMLTrackElement::LoadResource);
> +      RefPtr<Runnable> r = NewRunnableMethod(this, &HTMLTrackElement::LoadResource);
> -    nsContentUtils::RunInStableState(r.forget());
> +      nsContentUtils::RunInStableState(r.forget());
> +      mLoadResourceDispatched = true;
> +    }
>    }

Call the same helper method here.
Attachment #8768674 - Flags: review?(giles) → review+
(Assignee)

Comment 6

2 years ago
https://reviewboard.mozilla.org/r/62794/#review59760

> Would it be safer to move video.play(); up to immediately after video.appendChild()? That makes sure play() is called even if the loadedmetadata listener isn't called. I don't remember if a race is possible in this case though.

I want to change the src after the first vtt src loaded, so the play() is better to be called after the check |trackElement.readyState <= 1|.
If we call play() before the first vtt loaded, the testcase might fail if ended event comes quickly and the second vtt src is not loaded.
(Assignee)

Comment 7

2 years ago
https://reviewboard.mozilla.org/r/62794/#review59760

> Can you remove this respin, or convert it to an an is() check?
> 
> This seems like it could loop forever if there's some playback problem, causing a timeout and slowing down the tests.
> 
> What is this protecting against? If 'ended' fires with the wrong ready state the video.textTracks checks below will probably still fail the tests, so we would still catch the problem.

If the ended event and the readystate are not match, I think the problem is very simple: parsing vtt file slower than the ended event.
(Assignee)

Comment 8

2 years ago
https://reviewboard.mozilla.org/r/62796/#review59774

> It seems odd to preserve the mode here. Is this a proxy for the HTML attribute? Or does the spec say it should be preserved if set?
> 
> A WebPlatformTest should probably check this.

Spec say nothing about changing src.
Since change the src will create a new TextTrack, I think it is better to not preserve the mode. And the I need to modify the testcase corresonding the change.
(Assignee)

Comment 9

2 years ago
Comment on attachment 8768673 [details]
Bug 1281418 - Add testcase for changing the src of TrackElement.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62794/diff/1-2/
(Assignee)

Comment 10

2 years ago
Comment on attachment 8768674 [details]
Bug 1281418 - Release and reload the vtt resource when the src attribute of TrackElement changed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62796/diff/1-2/
(Assignee)

Comment 11

2 years ago
https://reviewboard.mozilla.org/r/62796/#review62796

::: dom/html/HTMLTrackElement.cpp:194
(Diff revision 2)
> +  if (mTrack) {
> +    // Remove all the cues in MediaElement.
> +    mMediaParent->RemoveTextTrack(mTrack);
> +    // Recreate mTrack.
> +    CreateTextTrack();
> +  }

Failed case in cloneNode.html:

var elm = document.createElement('track');
elm.track.mode = 'showing';
elm.src = 'resources/track.vtt?pipe=trickle(d1)';
(Assignee)

Comment 12

2 years ago
Comment on attachment 8768674 [details]
Bug 1281418 - Release and reload the vtt resource when the src attribute of TrackElement changed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62796/diff/2-3/
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 13

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c319aec28a80
Add testcase for changing the src of TrackElement. r=rillian
https://hg.mozilla.org/integration/mozilla-inbound/rev/b37540cec0f4
Release and reload the vtt resource when the src attribute of TrackElement changed. r=rillian
Keywords: checkin-needed

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c319aec28a80
https://hg.mozilla.org/mozilla-central/rev/b37540cec0f4
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
(Assignee)

Updated

a year ago
Duplicate of this bug: 1149852

Updated

5 months ago
Depends on: 1429550
You need to log in before you can comment on or make changes to this bug.