Closed Bug 1018933 Opened 10 years ago Closed 10 years ago

[webvtt] Subtitles not displayed in web-platform-tests WebVTT reftests

Categories

(Core :: Audio/Video, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: jgraham, Assigned: reyre)

References

()

Details

Attachments

(2 files, 3 obsolete files)

On Tue, May 27, 2014 at 8:18 PM, Richard Eyre <rick.eyre@hotmail.com> wrote:
> I'm thinking that the 'honor user preferences for automatic track selection'
> [1] algorithm is running after the script on the test page and resetting the
> mode value to 'disabled' since there is no default attribute on the track. A
> more fail safe way to do this would be to set a default attribute on the
> track and the browser will enable the track itself. Otherwise, we need to
> listen to 'loadedmetadata' or some other video event before setting the mode
> through script.

On Tue, May 27, 2014 at 2:36 PM, Philip Jägenstedt <philipj@opera.com>wrote:

> The 'honor user preferences for automatic track selection' algorithm
> can only enable tracks, it doesn't disable them. Also, "If any of the
> text tracks in candidates have a text track mode set to showing, abort
> these steps."

It seems like it would be possible to change all the tests here to use a different mechanism for enabling the subtitles, but there is also a bug that needs to be fixed.
Assignee: nobody → rick.eyre
I remember you showed me a site where you could run these tests, James. Could you point me towards them again?
http://w3c-test.org/webvtt/rendering/cues-with-video/processing-model/

Or, if you want to check them out and run locally, see the instructions at [1] (it's intended to be relatively straightforward).

[1] https://github.com/w3c/web-platform-tests
Great, thanks James! I should be able to look into this this week.
The issue was that 'GetTrack()' was being called before we 'BindToTree' was being called.
This caused an issue where two TextTracks would be created. The first one when 'GetTrack'
was called from script, and the second when 'BindToTree' triggered the creation of a new
Track. The reftests weren't working because two TextTracks were being added and the TextTrack
the it was trying to manipulate for the tests was the incorrect one.
Attachment #8447691 - Flags: review?(giles)
Attached patch Part 2: Add regression test (obsolete) — Splinter Review
Attachment #8447692 - Flags: review?(giles)
Comment on attachment 8447692 [details] [diff] [review]
Part 2: Add regression test

Review of attachment 8447692 [details] [diff] [review]:
-----------------------------------------------------------------

Looks correct to me, but I can't get the test to fail with current nightly. Can we make the regression more robust?
Attachment #8447692 - Flags: review?(giles) → review+
Comment on attachment 8447691 [details] [diff] [review]
Part 1: HTMLTrackElement should not create another TextTrack if one already exists

Review of attachment 8447691 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the comment nit addressed.

::: content/html/content/src/HTMLTrackElement.cpp
@@ +229,5 @@
>    }
>  
> +  // We may already have a TextTrack at this point if GetTrack() has already
> +  // been called. This happens, for instance, if script tries to get the
> +  // TextTrack before it's HTMLTrackElement has been bound to the DOM tree.

s/it's HTMLTrackElement/its mTrackElement/

Or at least fix the 'its'.
Attachment #8447691 - Flags: review?(giles) → review+
Attached patch Part 2: Add regression test (obsolete) — Splinter Review
Attachment #8447692 - Attachment is obsolete: true
Attachment #8449926 - Flags: review?(giles)
This should be more robust. Can you confirm Ralph?
Attachment #8449926 - Attachment is obsolete: true
Attachment #8449926 - Flags: review?(giles)
Attachment #8449927 - Flags: review?(giles)
Fixed up the code comment nits. Carrying forward r=rillian
Attachment #8447691 - Attachment is obsolete: true
Comment on attachment 8449927 [details] [diff] [review]
Part 2: Add regression test

Review of attachment 8449927 [details] [diff] [review]:
-----------------------------------------------------------------

This one fails for me without the fix. Thanks for finding a better test.
Attachment #8449927 - Flags: review?(giles) → review+
Thanks for review Ralph! Thanks for the try push as well, I pushed it last night though and that one looks green: https://tbpl.mozilla.org/?tree=Try&rev=68a888e6d704
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/49c338731007
https://hg.mozilla.org/mozilla-central/rev/6460b4e4452b
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: