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.
<rdar://problem/16795782>
*** Bug 123498 has been marked as a duplicate of this bug. ***
Created attachment 231110 [details] Patch
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?
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.
(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.
Created attachment 231126 [details] Patch
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.
Comment on attachment 231126 [details] Patch Clearing flags on attachment: 231126 Committed r168519: <http://trac.webkit.org/changeset/168519>
All reviewed patches have been landed. Closing bug.