Summary: | [Mac, iOS] Paint-on closed captions get out-of-order in Safari | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brent Fulgham <bfulgham> | ||||||||||
Component: | Media | Assignee: | Brent Fulgham <bfulgham> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | bfulgham, calvaris, commit-queue, eric.carlson, esprehn+autocc, glenn, gyuyoung.kim, jer.noble, philipj, sergio, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Mac | ||||||||||||
OS: | All | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 135680 | ||||||||||||
Attachments: |
|
Description
Brent Fulgham
2014-07-27 20:21:07 PDT
This bug came about partially because of my changes this year to reduce flickering (by keeping the CSS boxes around longer when the caption is only changing its end time, rather than tearing down and rebuilding caption boxes several times a second), and partially due to the tension between the WebVTT "boxes in boxes" layout algorithm (which tends to want to push new captions UP in the hierarchy), while roll-up captions want new content to be at the bottom of the caption box. I had to make two changes: 1. When the number of active captions on the screen grows, we want to throw away the existing CSS boxes and build new ones, so that the newest caption can be placed at the bottom of the caption block, which the older captions are 'pushed up' to the top. 2. The ordering of captions needed to be adjusted so that the order of active captions places the newer captions at handled first during layout. Previously, older captions would be drawn to screen first, and newer captions would bubble up towards the top. This change shouldn't affect most captions, since we typically use pop-on captions (which don't suffer from any of these problems). More complicated caption layout is generally accompanied with positioning information, which will override the simple ordering logic. Created attachment 235588 [details]
Patch
Created attachment 235590 [details]
Patch
Comment on attachment 235590 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=235590&action=review > Source/WebCore/html/shadow/MediaControlElements.cpp:1312 > + removeChildren(); It's possible we might only want this behavior on Mac/iOS, but I imagine other platforms will have a similar problem with roll-up captions. > Source/WebCore/html/track/TextTrackCue.cpp:203 > + return startTime() > other->startTime(); Alternatively, this special Generic behavior could be in TextTrackGenericCue, instead of calling this base class method. > Source/WebCore/platform/graphics/avfoundation/InbandTextTrackPrivateAVF.cpp:80 > +extern "C" { This section is some kind of line-ending foo. I made no changes to these structs. Created attachment 235596 [details]
Re-Upload to allow EWS runs
Comment on attachment 235596 [details] Re-Upload to allow EWS runs View in context: https://bugs.webkit.org/attachment.cgi?id=235596&action=review > Source/WebCore/html/track/TextTrackCue.cpp:203 > + return startTime() > other->startTime(); I'm not sure if this should just be in TextTrackCueGeneric. If it is, we have to bypass calling into this base class entirely, because the base class will return early with 'true' for roll-up cases that we need to treat as 'false'. Comment on attachment 235596 [details] Re-Upload to allow EWS runs View in context: https://bugs.webkit.org/attachment.cgi?id=235596&action=review >> Source/WebCore/html/track/TextTrackCue.cpp:203 >> + return startTime() > other->startTime(); > > I'm not sure if this should just be in TextTrackCueGeneric. If it is, we have to bypass calling into this base class entirely, because the base class will return early with 'true' for roll-up cases that we need to treat as 'false'. Yes, I think this would be best put into the Generic class. Other than that, r+. Created attachment 235614 [details]
Patch (Remarking as r+)
(In reply to comment #8) > (From update of attachment 235596 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=235596&action=review > > >> Source/WebCore/html/track/TextTrackCue.cpp:203 > >> + return startTime() > other->startTime(); > > > > I'm not sure if this should just be in TextTrackCueGeneric. If it is, we have to bypass calling into this base class entirely, because the base class will return early with 'true' for roll-up cases that we need to treat as 'false'. > > Yes, I think this would be best put into the Generic class. Other than that, r+. Done. I'm uploading a revised patch to get EWS coverage. Comment on attachment 235614 [details]
Patch (Remarking as r+)
Jer approved this patch in person.
Committed in r171700. <http://trac.webkit.org/changeset/171700> |