Bug 106943 – Cues not rendered when they should be

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Victor Carbune]

Description: http://code.google.com/p/chromium/issues/detail?id=155099

The issue within the code is that updateActiveTextTrackCues is called when
one track has mode = "showing" and the other one has mode = "hidden".

This means that cues from both tracks get added in the cue tree, but
only the ones from the first track get rendered. 

When the second track gets the mode set to "showing", its cues were
already added in the cue tree and marked as active, and therefore
activeSetChanged will be false and not rendering will occur.

[Victor Carbune]

Created attachment 182875 [details]
Simple fix and test case

[Silvia Pfeiffer]

FWIW: LGTM.

[WebKit Review Bot]

Comment on attachment 182875 [details]
Simple fix and test case

Attachment 182875 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15904203

New failing tests:
media/video-controls-captions.html

[Victor Carbune]

Created attachment 182934 [details]
Updated test result

The video-controls-captions test still works as expected, it just outputs a different error

[Eric Carlson]

Comment on attachment 182934 [details]
Updated test result

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

I really like the new test - it is concise, thorough, and easy to understand. Good fix, thanks!

> LayoutTests/media/track/track-cue-rendering-mode-changed.html:12
> +        var testTrackEnglish, testTrackArabic;

Nit: each variable should be declared independently.

> LayoutTests/media/track/track-cue-rendering-mode-changed.html:15
> +        function addTracks() {

Nit: a function's opening brace should be on a new line.

> LayoutTests/media/track/track-cue-rendering-mode-changed.html:16
> +            findMediaElement();

Nit: this should not be necessary, it was call in loaded().

[Victor Carbune]

Created attachment 183038 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 183038 [details]
Patch for landing

Clearing flags on attachment: 183038

Committed r139932: <http://trac.webkit.org/changeset/139932>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Victor Carbune]

(In reply to comment #5)
Fixed the comments in the landing patch. Thanks for your fast review, Eric!

[Benjamin Poulain]

media/video-transformed.html started crashing on Mac shortly after this landed.
Any idea if that is related?

[Eric Carlson]

(In reply to comment #10)
> media/video-transformed.html started crashing on Mac shortly after this landed.
> Any idea if that is related?

From the backtrace, eg. http://build.webkit.org/results/Apple%20MountainLion%20Release%20WK2%20(Tests)/r139933%20(4912)/media/video-transformed-crash-log.txt, I don't think so.

[Eric Carlson]

Comment on attachment 183038 [details]
Patch for landing

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

> Source/WebCore/html/HTMLMediaElement.cpp:1175
> +    if (!activeSetChanged) {
> +        // Even though the active set has not changed, it is possible that the
> +        // the mode of a track has changed from 'hidden' to 'showing' and the
> +        // cues have not yet been rendered.
> +        if (hasMediaControls())
> +            mediaControls()->updateTextTrackDisplay();
> +

This sometimes causes an assert in MediaControlTextTrackContainerElement::updateDisplay after a track is removed, because m_currentlyActiveCues has not been updated.

[Victor Carbune]

(In reply to comment #12)
> (From update of attachment 183038 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=183038&action=review
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:1175
> > +    if (!activeSetChanged) {
> > +        // Even though the active set has not changed, it is possible that the
> > +        // the mode of a track has changed from 'hidden' to 'showing' and the
> > +        // cues have not yet been rendered.
> > +        if (hasMediaControls())
> > +            mediaControls()->updateTextTrackDisplay();
> > +
> 
> This sometimes causes an assert in MediaControlTextTrackContainerElement::updateDisplay after a track is removed, because m_currentlyActiveCues has not been updated.

Indeed; since it's a new issue, filed a bug to track it:
https://bugs.webkit.org/show_bug.cgi?id=107465