Bug 102561 – HTMLMediaElement::configureTextTracks should configure all text tracks

• Component: Media
• Status: RESOLVED FIXED

[Eric Carlson]

The spec's "automatic text track selection" algorithm [1] select a track from "... a list consisting of the text tracks in the media element's list of text tracks ...", but HTMLMediaElement::configureTextTracks currently only considers <track> elements. It should consider all track types.

[1] http://dev.w3.org/html5/spec/media-elements.html#perform-automatic-text-track-selection

[Eric Carlson]

Created attachment 174777 [details]
Proposed patch

[WebKit Review Bot]

Attachment 174777 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/medi..." exit_code: 1
Source/WebCore/html/HTMLMediaElement.cpp:2944:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[kov's GTK+ EWS bot]

Comment on attachment 174777 [details]
Proposed patch

Attachment 174777 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/14858637

[Silvia Pfeiffer]

The layout test result has this line:
"ReferenceError: Can't find variable: showing"

I think something has gone wrong there and there's a bug in the layout test.

[Eric Carlson]

Created attachment 174787 [details]
Updated patch

[Eric Carlson]

(In reply to comment #4)
> The layout test result has this line:
> "ReferenceError: Can't find variable: showing"
> 
> I think something has gone wrong there and there's a bug in the layout test.

Oops I used the wrong version of the patch, thanks!

Attached new patch with correct test and style nits.

[Silvia Pfeiffer]

FWIW: looks good to me

Just wondering: would it be at all possible to add a media resource that has an in-band track and that we can base a layout test for TextTrack on?

[Eric Carlson]

(In reply to comment #7)
> FWIW: looks good to me
> 
> Just wondering: would it be at all possible to add a media resource that has an in-band track and that we can base a layout test for TextTrack on?

We already have LayoutTests/media/content/counting-captioned.mov, which has in-band CEA 608 captions, but I think only Apple's media ending supports it currently.

[Eric Carlson]

(In reply to comment #8)
> 
> We already have LayoutTests/media/content/counting-captioned.mov, which has in-band CEA 608 captions, but I think only Apple's media ending supports it currently.

And WebKit doesn't yet have code to integrate the in-band tracks supported by a platform media engine into the out-of-band and dynamically created tracks, so the caption track in this file doesn't show up as a TextTrack, but that is my next task.

[Philippe Normand]

Comment on attachment 174787 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174787&action=review

Patch looks good, bonus points for the smart pointers. I just have two small questions. Not setting r+ yet because of the test result. See below.

> Source/WebCore/html/HTMLMediaElement.cpp:2877
> +    RefPtr<TextTrack> trackEnable = 0;

I'm not a native English speaker so feel free to ignore this comment :) Would trackToEnable be a better name? I was just a bit confused by this new trackEnable name.

> LayoutTests/media/track/track-mode-not-changed-by-new-track-expected.txt:53
> +EXPECTED (track3.mode == 'hidden'), OBSERVED 'showing' FAIL

Is it normal to have that expectation here?

[Martin Robinson]

Comment on attachment 174787 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174787&action=review

> Source/WebCore/html/HTMLMediaElement.cpp:2879
> +    RefPtr<TextTrack> defaultTrack = 0;
> +    RefPtr<TextTrack> fallbackTrack = 0;

Just a drive by suggestion after this landed in my bugzilla mail, but I think that the default constructor for RefPtr is "ALWAYS_INLINE RefPtr() : m_ptr(0) { }", so you can probably omit "= 0" from all these declarations, as it's implied.

[Philippe Normand]

(In reply to comment #9)
> (In reply to comment #8)
> > 
> > We already have LayoutTests/media/content/counting-captioned.mov, which has in-band CEA 608 captions, but I think only Apple's media ending supports it currently.
> 
> And WebKit doesn't yet have code to integrate the in-band tracks supported by a platform media engine into the out-of-band and dynamically created tracks, so the caption track in this file doesn't show up as a TextTrack, but that is my next task.

From the GStreamer backend POV it would be nice as well to advertize the text tracks exposed by the GStreamer playbin element to the media element. From playbin we can get the number of text tracks but I'm afraid we can only get the data from the currently selected track. Anyway, something to investigate further at some point, I didn't mean to hijack this bug :)

[Silvia Pfeiffer]

(In reply to comment #8)
> (In reply to comment #7)
>>
> > Just wondering: would it be at all possible to add a media resource that has an in-band track and that we can base a layout test for TextTrack on?
> 
> We already have LayoutTests/media/content/counting-captioned.mov, which has in-band CEA 608 captions, but I think only Apple's media ending supports it currently.

Fair enough. I'll create myself a bug to add a WebM file with in-band WebVTT and have that work the same way.

[Eric Carlson]

Created attachment 175044 [details]
Updated patch

[Eric Carlson]

(In reply to comment #10)
> (From update of attachment 174787 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174787&action=review
> 
> Patch looks good, bonus points for the smart pointers. I just have two small questions. Not setting r+ yet because of the test result. See below.
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:2877
> > +    RefPtr<TextTrack> trackEnable = 0;
> 
> I'm not a native English speaker so feel free to ignore this comment :) Would trackToEnable be a better name? I was just a bit confused by this new trackEnable name.
> 
  Great suggestion, changed.

> > LayoutTests/media/track/track-mode-not-changed-by-new-track-expected.txt:53
> > +EXPECTED (track3.mode == 'hidden'), OBSERVED 'showing' FAIL
> 
> Is it normal to have that expectation here?
>
  No, the test was incorrect. Fixed.


(In reply to comment #11)
> (From update of attachment 174787 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174787&action=review
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:2879
> > +    RefPtr<TextTrack> defaultTrack = 0;
> > +    RefPtr<TextTrack> fallbackTrack = 0;
> 
> Just a drive by suggestion after this landed in my bugzilla mail, but I think that the default constructor for RefPtr is "ALWAYS_INLINE RefPtr() : m_ptr(0) { }", so you can probably omit "= 0" from all these declarations, as it's implied.

Thanks, fixed.

[Eric Carlson]

(In reply to comment #12)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > 
> > > We already have LayoutTests/media/content/counting-captioned.mov, which has in-band CEA 608 captions, but I think only Apple's media ending supports it currently.
> > 
> > And WebKit doesn't yet have code to integrate the in-band tracks supported by a platform media engine into the out-of-band and dynamically created tracks, so the caption track in this file doesn't show up as a TextTrack, but that is my next task.
> 
> From the GStreamer backend POV it would be nice as well to advertize the text tracks exposed by the GStreamer playbin element to the media element. From playbin we can get the number of text tracks but I'm afraid we can only get the data from the currently selected track. Anyway, something to investigate further at some point, I didn't mean to hijack this bug :)

My next couple of patches will make it possible for someone to hook this up.

[Philippe Normand]

Comment on attachment 175044 [details]
Updated patch

Ok!

[WebKit Review Bot]

Comment on attachment 175044 [details]
Updated patch

Clearing flags on attachment: 175044

Committed r135202: <http://trac.webkit.org/changeset/135202>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.