Bug 106943 - Cues not rendered when they should be
Summary: Cues not rendered when they should be
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Victor Carbune
URL:
Keywords:
Depends on:
Blocks: 43668
  Show dependency treegraph
 
Reported: 2013-01-15 13:51 PST by Victor Carbune
Modified: 2013-01-21 11:53 PST (History)
7 users (show)

See Also:


Attachments
Simple fix and test case (7.57 KB, patch)
2013-01-15 16:56 PST, Victor Carbune
no flags Details | Formatted Diff | Diff
Updated test result (8.46 KB, patch)
2013-01-16 01:29 PST, Victor Carbune
no flags Details | Formatted Diff | Diff
Patch for landing (8.45 KB, patch)
2013-01-16 14:55 PST, Victor Carbune
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Victor Carbune 2013-01-15 13:51:25 PST
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.
Comment 1 Victor Carbune 2013-01-15 16:56:06 PST
Created attachment 182875 [details]
Simple fix and test case
Comment 2 Silvia Pfeiffer 2013-01-15 17:13:40 PST
FWIW: LGTM.
Comment 3 WebKit Review Bot 2013-01-15 17:38:00 PST
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
Comment 4 Victor Carbune 2013-01-16 01:29:20 PST
Created attachment 182934 [details]
Updated test result

The video-controls-captions test still works as expected, it just outputs a different error
Comment 5 Eric Carlson 2013-01-16 09:55:36 PST
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().
Comment 6 Victor Carbune 2013-01-16 14:55:38 PST
Created attachment 183038 [details]
Patch for landing
Comment 7 WebKit Review Bot 2013-01-16 15:59:15 PST
Comment on attachment 183038 [details]
Patch for landing

Clearing flags on attachment: 183038

Committed r139932: <http://trac.webkit.org/changeset/139932>
Comment 8 WebKit Review Bot 2013-01-16 15:59:19 PST
All reviewed patches have been landed.  Closing bug.
Comment 9 Victor Carbune 2013-01-16 16:02:21 PST
(In reply to comment #5)
Fixed the comments in the landing patch. Thanks for your fast review, Eric!
Comment 10 Benjamin Poulain 2013-01-16 18:23:46 PST
media/video-transformed.html started crashing on Mac shortly after this landed.
Any idea if that is related?
Comment 11 Eric Carlson 2013-01-16 18:43:19 PST
(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.
Comment 12 Eric Carlson 2013-01-21 10:37:32 PST
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.
Comment 13 Victor Carbune 2013-01-21 11:53:53 PST
(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