Bug 135332 - [Mac, iOS] Paint-on closed captions get out-of-order in Safari
Summary: [Mac, iOS] Paint-on closed captions get out-of-order in Safari
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac All
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on:
Blocks: 135680
  Show dependency treegraph
 
Reported: 2014-07-27 20:21 PDT by Brent Fulgham
Modified: 2014-08-06 17:47 PDT (History)
11 users (show)

See Also:


Attachments
Patch (9.33 KB, patch)
2014-07-27 20:56 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (9.24 KB, patch)
2014-07-27 22:11 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Re-Upload to allow EWS runs (8.30 KB, patch)
2014-07-28 09:04 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (Remarking as r+) (8.88 KB, patch)
2014-07-28 14:00 PDT, Brent Fulgham
bfulgham: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2014-07-27 20:21:07 PDT
If you attempt to watch media that contains roll-up style 608 closed captions, they will show up out-of-order and be very difficult to follow.

Example:
1. Navigate to the KTVU live stream at <http://bcoveliveios-i.akamaihd.net/hls/live/207145/374678397001/KTVU_NEWSCAST/index.m3u8>
2. Click the Subtitles button in the playback controls and choose Unknown CC from the pop-up menu

Close captions appear, but the text wraps in a weird way and often two captions are present at the same time, displayed out of order.
Comment 1 Brent Fulgham 2014-07-27 20:21:22 PDT
<rdar://problem/15317278>
Comment 2 Brent Fulgham 2014-07-27 20:27:03 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.
Comment 3 Brent Fulgham 2014-07-27 20:56:54 PDT
Created attachment 235588 [details]
Patch
Comment 4 Brent Fulgham 2014-07-27 22:11:03 PDT
Created attachment 235590 [details]
Patch
Comment 5 Brent Fulgham 2014-07-27 22:13:26 PDT
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.
Comment 6 Brent Fulgham 2014-07-28 09:04:45 PDT
Created attachment 235596 [details]
Re-Upload to allow EWS runs
Comment 7 Brent Fulgham 2014-07-28 09:06:34 PDT
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 8 Jer Noble 2014-07-28 13:21:41 PDT
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+.
Comment 9 Brent Fulgham 2014-07-28 14:00:32 PDT
Created attachment 235614 [details]
Patch (Remarking as r+)
Comment 10 Brent Fulgham 2014-07-28 14:01:07 PDT
(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 11 Brent Fulgham 2014-07-28 14:58:03 PDT
Comment on attachment 235614 [details]
Patch (Remarking as r+)

Jer approved this patch in person.
Comment 12 Brent Fulgham 2014-07-28 14:59:52 PDT
Committed in r171700. <http://trac.webkit.org/changeset/171700>