Changeset 254767 in webkit

Timestamp:

Jan 17, 2020 2:31:58 PM (4 years ago)

Author:

eric.carlson@apple.com

Message:

REGRESSION (r254483): media/track/track-cues-sorted-before-dispatch.html became very flaky
​https://bugs.webkit.org/show_bug.cgi?id=206225
<rdar://problem/58634315>

Reviewed by Jer Noble.

The list of text track cues that are to fire events are sorted before events are
fired. Cue were being sorted by track, then by start time, and then by end time.
This meant that the sort order of two cues in the same track with identical start
and end times was not stable, causing this test to be flaky. The spec says to sort
by a cue's position in the track cue list when start and end times are identical,
so do that.

No new tests, this fixes a flaky test.

• html/track/TextTrackCue.cpp:

(WebCore::TextTrackCue::cueIndex const):
(WebCore::TextTrackCue::isOrderedBefore const):

• html/track/TextTrackCue.h:
• html/track/TextTrackCueList.cpp:

(WebCore::cueSortsBefore):
(WebCore::TextTrackCueList::cueIndex const):
(WebCore::TextTrackCueList::add):
(WebCore::TextTrackCueList::updateCueIndex):
(WebCore::compareCues): Deleted.

• html/track/TextTrackCueList.h:

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• html/track/TextTrackCue.cpp (modified) (3 diffs)
• html/track/TextTrackCue.h (modified) (2 diffs)
• html/track/TextTrackCueList.cpp (modified) (6 diffs)
• html/track/TextTrackCueList.h (modified) (2 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2020-01-17  Eric Carlson  <eric.carlson@apple.com>
+
+        REGRESSION (r254483): media/track/track-cues-sorted-before-dispatch.html became very flaky
+        https://bugs.webkit.org/show_bug.cgi?id=206225
+        <rdar://problem/58634315>
+
+        Reviewed by Jer Noble.
+
+        The list of text track cues that are to fire events are sorted before events are
+        fired. Cue were being sorted by track, then by start time, and then by end time. 
+        This meant that the sort order of two cues in the same track with identical start 
+        and end times was not stable, causing this test to be flaky. The spec says to sort
+        by a cue's position in the track cue list when start and end times are identical,
+        so do that.
+
+        No new tests, this fixes a flaky test.
+
+        * html/track/TextTrackCue.cpp:
+        (WebCore::TextTrackCue::cueIndex const):
+        (WebCore::TextTrackCue::isOrderedBefore const):
+        * html/track/TextTrackCue.h:
+        * html/track/TextTrackCueList.cpp:
+        (WebCore::cueSortsBefore):
+        (WebCore::TextTrackCueList::cueIndex const):
+        (WebCore::TextTrackCueList::add):
+        (WebCore::TextTrackCueList::updateCueIndex):
+        (WebCore::compareCues): Deleted.
+        * html/track/TextTrackCueList.h:
+
 2020-01-17  Andres Gonzalez  <andresg_22@apple.com>
 

trunk/Source/WebCore/html/track/TextTrackCue.cpp

@@ -1 +1 @@
 /*
  * Copyright (C) 2011, 2013 Google Inc.  All rights reserved.
- * Copyright (C) 2011-2017 Apple Inc. All rights reserved.
+ * Copyright (C) 2011-2020 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -51 +51 @@
 #include "VTTCue.h"
 #include "VTTRegionList.h"
+#include <limits.h>
 #include <wtf/HexNumber.h>
 #include <wtf/IsoMallocInlines.h>
@@ -339 +340 @@
 }
 
+unsigned TextTrackCue::cueIndex() const
+{
+    ASSERT(m_track && m_track->cues());
+    if (!m_track || !m_track->cues())
+        return std::numeric_limits<unsigned>::max();
+
+    return m_track->cues()->cueIndex(*this);
+}
+
 bool TextTrackCue::isOrderedBefore(const TextTrackCue* other) const
 {
-    return startMediaTime() < other->startMediaTime() || (startMediaTime() == other->startMediaTime() && endMediaTime() > other->endMediaTime());
+    // ... cues must be sorted by their start time, earliest first;
+    if (startMediaTime() != other->startMediaTime())
+        return startMediaTime() < other->startMediaTime();
+
+    // then, any cues with the same start time must be sorted by their end time, latest first;
+    if (endMediaTime() != other->endMediaTime())
+        return endMediaTime() > other->endMediaTime();
+
+    // and finally, any cues with identical end times must be sorted in the order they were last added to
+    // their respective text track list of cues, oldest first (so e.g. for cues from a WebVTT file, that
+    // would initially be the order in which the cues were listed in the file)
+    return cueIndex() < other->cueIndex();
 }
 

trunk/Source/WebCore/html/track/TextTrackCue.h

@@ -1 +1 @@
 /*
  * Copyright (C) 2011 Google Inc. All rights reserved.
- * Copyright (C) 2012-2017 Apple Inc. All rights reserved.
+ * Copyright (C) 2012-2020 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -128 +128 @@
     virtual void updateDisplayTree(const MediaTime&) { };
 
+    unsigned cueIndex() const;
+
 protected:
     TextTrackCue(ScriptExecutionContext&, const MediaTime& start, const MediaTime& end, DocumentFragment&&);

trunk/Source/WebCore/html/track/TextTrackCueList.cpp

@@ -1 +1 @@
 /*
  * Copyright (C) 2011 Google Inc. All rights reserved.
- * Copyright (C) 2017 Apple Inc. All rights reserved.
+ * Copyright (C) 2017-2020 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -35 +35 @@
 
 #ifdef CHECK_SORTING
-#define ASSERT_SORTED(begin, end) ASSERT(std::is_sorted(begin, end, compareCues))
+#define ASSERT_SORTED(begin, end) ASSERT(std::is_sorted(begin, end, cueSortsBefore))
 #else
 #define ASSERT_SORTED(begin, end) ((void)0)
@@ -42 +42 @@
 namespace WebCore {
 
-static inline bool compareCues(const RefPtr<TextTrackCue>& a, const RefPtr<TextTrackCue>& b)
+static inline bool cueSortsBefore(const RefPtr<TextTrackCue>& a, const RefPtr<TextTrackCue>& b)
 {
-    return a->isOrderedBefore(b.get());
+    if (a->startMediaTime() < b->startMediaTime())
+        return true;
+
+    return a->startMediaTime() == b->startMediaTime() && a->endMediaTime() > b->endMediaTime();
 }
 
-unsigned TextTrackCueList::cueIndex(TextTrackCue& cue) const
+unsigned TextTrackCueList::cueIndex(const TextTrackCue& cue) const
 {
     ASSERT(m_vector.contains(&cue));
@@ -94 +97 @@
 
     RefPtr<TextTrackCue> cueRefPtr { WTFMove(cue) };
-    unsigned insertionPosition = std::upper_bound(m_vector.begin(), m_vector.end(), cueRefPtr, compareCues) - m_vector.begin();
+    unsigned insertionPosition = std::upper_bound(m_vector.begin(), m_vector.end(), cueRefPtr, cueSortsBefore) - m_vector.begin();
     ASSERT_SORTED(m_vector.begin(), m_vector.end());
     m_vector.insert(insertionPosition, WTFMove(cueRefPtr));
@@ -114 +117 @@
 }
 
-void TextTrackCueList::updateCueIndex(TextTrackCue& cue)
+void TextTrackCueList::updateCueIndex(const TextTrackCue& cue)
 {
     auto cuePosition = m_vector.begin() + cueIndex(cue);
@@ -122 +125 @@
     ASSERT_SORTED(afterCuePosition, m_vector.end());
 
-    auto reinsertionPosition = std::upper_bound(m_vector.begin(), cuePosition, *cuePosition, compareCues);
+    auto reinsertionPosition = std::upper_bound(m_vector.begin(), cuePosition, *cuePosition, cueSortsBefore);
     if (reinsertionPosition != cuePosition)
         std::rotate(reinsertionPosition, cuePosition, afterCuePosition);
     else {
-        reinsertionPosition = std::upper_bound(afterCuePosition, m_vector.end(), *cuePosition, compareCues);
+        reinsertionPosition = std::upper_bound(afterCuePosition, m_vector.end(), *cuePosition, cueSortsBefore);
         if (reinsertionPosition != afterCuePosition)
             std::rotate(cuePosition, afterCuePosition, reinsertionPosition);

trunk/Source/WebCore/html/track/TextTrackCueList.h

@@ -1 +1 @@
 /*
  * Copyright (C) 2011 Google Inc. All rights reserved.
- * Copyright (C) 2017 Apple Inc. All rights reserved.
+ * Copyright (C) 2017-2020 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -41 +41 @@
     TextTrackCue* getCueById(const String&) const;
 
-    unsigned cueIndex(TextTrackCue&) const;
+    unsigned cueIndex(const TextTrackCue&) const;
 
     void add(Ref<TextTrackCue>&&);
     void remove(TextTrackCue&);
-    void updateCueIndex(TextTrackCue&);
+    void updateCueIndex(const TextTrackCue&);
 
     void clear();