Bug 106943

Summary: Cues not rendered when they should be
Product: WebKit Reporter: Victor Carbune <vcarbune>
Component: MediaAssignee: Victor Carbune <vcarbune>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, dglazkov, eric.carlson, feature-media-reviews, ojan.autocc, silviapf, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 43668    
Attachments:
Description Flags
Simple fix and test case
none
Updated test result
none
Patch for landing none

Victor Carbune
Reported 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.
Attachments
Simple fix and test case (7.57 KB, patch)
2013-01-15 16:56 PST, Victor Carbune 		no flags
DetailsFormatted DiffDiff
Updated test result (8.46 KB, patch)
2013-01-16 01:29 PST, Victor Carbune 		no flags
DetailsFormatted DiffDiff
Patch for landing (8.45 KB, patch)
2013-01-16 14:55 PST, Victor Carbune 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Victor Carbune
Comment 1 2013-01-15 16:56:06 PST
Created attachment 182875 [details] Simple fix and test case
Silvia Pfeiffer
Comment 2 2013-01-15 17:13:40 PST
FWIW: LGTM.
WebKit Review Bot
Comment 3 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
Victor Carbune
Comment 4 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
Eric Carlson
Comment 5 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().
Victor Carbune
Comment 6 2013-01-16 14:55:38 PST
Created attachment 183038 [details] Patch for landing
WebKit Review Bot
Comment 7 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>
WebKit Review Bot
Comment 8 2013-01-16 15:59:19 PST
All reviewed patches have been landed. Closing bug.
Victor Carbune
Comment 9 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!
Benjamin Poulain
Comment 10 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?
Eric Carlson
Comment 11 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.
Eric Carlson
Comment 12 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.
Victor Carbune
Comment 13 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
Note You need to log in before you can comment on or make changes to this bug.