Bug 135332 – [Mac, iOS] Paint-on closed captions get out-of-order in Safari

• Component: Media
• Status: RESOLVED FIXED

[Brent Fulgham]

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.

[Brent Fulgham]

<rdar://problem/15317278>

[Brent Fulgham]

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.

[Brent Fulgham]

Created attachment 235588 [details]
Patch

[Brent Fulgham]

Created attachment 235590 [details]
Patch

[Brent Fulgham]

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.

[Brent Fulgham]

Created attachment 235596 [details]
Re-Upload to allow EWS runs

[Brent Fulgham]

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'.

[Jer Noble]

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+.

[Brent Fulgham]

Created attachment 235614 [details]
Patch (Remarking as r+)

[Brent Fulgham]

(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.

[Brent Fulgham]

Comment on attachment 235614 [details]
Patch (Remarking as r+)

Jer approved this patch in person.

[Brent Fulgham]

Committed in r171700. <http://trac.webkit.org/changeset/171700>