Multiple (stacked) cues when shuttling through video while playing closed captions

• Component: Media
• Status: RESOLVED FIXED

[Brent Fulgham]

A bug in the cue comparison logic caused us to display the same cue multiple times. This had two causes:

1. Recent WebVTT work introduced a bug where any non-VTT cue was considered to be unequal, causing many in-band tracks to fail this test.
2. Small variations in cue start time (esp. on streaming video) would cause the track comparison logic to fail.

Either of these problems would cause an additional cue to be displayed each time a section of video was played.

[Brent Fulgham]

<rdar://problem/16795782>

[Brent Fulgham]

*** Bug 123498 has been marked as a duplicate of this bug. ***

[Brent Fulgham]

Created attachment 231110 [details]
Patch

[Eric Carlson]

Comment on attachment 231110 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=231110&action=review

> Source/WebCore/html/track/VTTCue.cpp:1071
>      if (cue.cueType() != WebVTT)
> -        return false;
> +        return true;

I don't understand this. How can a a non-WebVTT cue be equal?

[Brent Fulgham]

Comment on attachment 231110 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=231110&action=review

>> Source/WebCore/html/track/VTTCue.cpp:1071
>> +        return true;
> 
> I don't understand this. How can a a non-WebVTT cue be equal?

This code is called at the end of TextTrackCueGeneric::isEqual. We are not confirming equality of cueTypes (that happens in the base TextTrack::isEqual call). This is just a short circuit so that we don't run through the VTT-specific tests below.

Alternative fixes would be:
1. Instead of short-circuiting, just use this conditional to decide whether to cast to VTTCue and do specific tests in that stanza.
2. Modify TextTrackCueGeneric::isEqual so that id bypasses VTTCue entirely.

I don't like #1 because we tend to prefer short-circuit in our style.
I am not crazy about #2 because we do call VTTCue parent class methods in other TextTrackCueGeneric calls. Someone might notice the discrepancy and try to "fix" it at some point in the future, breaking things again.

[Brent Fulgham]

(In reply to comment #5)
> (From update of attachment 231110 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=231110&action=review
> 
> >> Source/WebCore/html/track/VTTCue.cpp:1071
> >> +        return true;
> > 
> > I don't understand this. How can a a non-WebVTT cue be equal?
> 
> This code is called at the end of TextTrackCueGeneric::isEqual. We are not confirming equality of cueTypes (that happens in the base TextTrack::isEqual call). This is just a short circuit so that we don't run through the VTT-specific tests below.

This whole bug makes me wonder if TextTrackCueGeneric really should be a subclass of VTTCue. Maybe TTCGeneric and VTTCue should be siblings of some common ancestor.

At any rate, any of the three suggestions will work and I'm happy to proceed with whichever you think is the most clear.

Maybe I should bypass the VTTCue::isEqual in TextTrackCueGeneric::isEqual and have a comment there explaining why we don't use the parent isEqual implementation.

[Brent Fulgham]

(In reply to comment #5)
> (From update of attachment 231110 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=231110&action=review
> 
> >> Source/WebCore/html/track/VTTCue.cpp:1071
> >> +        return true;
> > 
> > I don't understand this. How can a a non-WebVTT cue be equal?
> 
> This code is called at the end of TextTrackCueGeneric::isEqual. We are not confirming equality of cueTypes (that happens in the base TextTrack::isEqual call). This is just a short circuit so that we don't run through the VTT-specific tests below.

This whole bug makes me wonder if TextTrackCueGeneric really should be a subclass of VTTCue. Maybe TTCGeneric and VTTCue should be siblings of some common ancestor.

At any rate, any of the three suggestions will work and I'm happy to proceed with whichever you think is the most clear.

Maybe I should bypass the VTTCue::isEqual in TextTrackCueGeneric::isEqual and have a comment there explaining why we don't use the parent isEqual implementation.

[Brent Fulgham]

Created attachment 231126 [details]
Patch

[Eric Carlson]

Comment on attachment 231126 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=231126&action=review

> Source/WebCore/html/track/TextTrackCueGeneric.cpp:159
> +    // Do not call the parent class isEqual here, because we are not cueType() == VTTCue,
> +    // and will fail that equality test.
>      if (!TextTrackCue::isEqual(cue, match))
>          return false;

Thanks for the explanation. I think this is the best solution for now.

[WebKit Commit Bot]

Comment on attachment 231126 [details]
Patch

Clearing flags on attachment: 231126

Committed r168519: <http://trac.webkit.org/changeset/168519>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.