Summary: | Cues not rendered when they should be | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Victor Carbune <vcarbune> | ||||||||
Component: | Media | Assignee: | 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
Victor Carbune
2013-01-15 13:51:25 PST
Created attachment 182875 [details]
Simple fix and test case
FWIW: LGTM. 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 Created attachment 182934 [details]
Updated test result
The video-controls-captions test still works as expected, it just outputs a different error
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(). Created attachment 183038 [details]
Patch for landing
Comment on attachment 183038 [details] Patch for landing Clearing flags on attachment: 183038 Committed r139932: <http://trac.webkit.org/changeset/139932> All reviewed patches have been landed. Closing bug. (In reply to comment #5) Fixed the comments in the landing patch. Thanks for your fast review, Eric! media/video-transformed.html started crashing on Mac shortly after this landed. Any idea if that is related? (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 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. (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 |