Closed Bug 1281418 Opened 8 years ago Closed 8 years ago

[webvtt] Implement HTMLTrackElement src attribute.

Categories

(Core :: Audio/Video: Playback, defect, P3)

Product:
Core
Component:
Audio/Video: Playback
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla50
Tracking Flags:
Tracking Status
firefox50 --- fixed

People

(Reporter: bechen, Assigned: bechen)

References

(Depends on 1 open bug, Blocks 3 open bugs)

Details

(Keywords: dev-doc-complete)

Attachments

(2 files)

Bug 1281418 - Add testcase for changing the src of TrackElement.
58 bytes, text/x-review-board-request
rillian
: review+
Details
Bug 1281418 - Release and reload the vtt resource when the src attribute of TrackElement changed.
58 bytes, text/x-review-board-request
rillian
: review+
Details 
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...
Mass change P2 -> P3
Priority: P2 → P3
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+
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.
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.
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.
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/
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/
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)';
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/
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/c319aec28a80
https://hg.mozilla.org/mozilla-central/rev/b37540cec0f4
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Blocks: 1523224
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: