Paint buffered range closest to the current play position

Source/core/layout/LayoutMediaControls.cpp

Index: Source/core/layout/LayoutMediaControls.cpp
diff --git a/Source/core/layout/LayoutMediaControls.cpp b/Source/core/layout/LayoutMediaControls.cpp
index 4c1a2c4faa321772a3456c4801fb9f5e4ec918e5..f3c51a6c9502044bc9060ba05048fd4618bae1ce 100644
--- a/Source/core/layout/LayoutMediaControls.cpp
+++ b/Source/core/layout/LayoutMediaControls.cpp
@@ -207,24 +207,31 @@ static bool paintMediaSlider(LayoutObject* object, const PaintInfo& paintInfo, c
     // Draw the buffered range. Since the element may have multiple buffered ranges and it'd be
     // distracting/'busy' to show all of them, show only the buffered range containing the current play head.
     RefPtrWillBeRawPtr<TimeRanges> bufferedTimeRanges = mediaElement->buffered();
-    float duration = mediaElement->duration();
-    float currentTime = mediaElement->currentTime();
-    if (std::isnan(duration) || std::isinf(duration) || !duration || std::isnan(currentTime))
+    double duration = mediaElement->duration();
+    double currentTime = mediaElement->currentTime();
+    if (std::isnan(duration) || std::isinf(duration) || !duration || std::isnan(currentTime)
+        || bufferedTimeRanges->length() == 0)
         return true;
 
-    for (unsigned i = 0; i < bufferedTimeRanges->length(); ++i) {
-        float start = bufferedTimeRanges->start(i, ASSERT_NO_EXCEPTION);
-        float end = bufferedTimeRanges->end(i, ASSERT_NO_EXCEPTION);
-        if (std::isnan(start) || std::isnan(end) || start > currentTime || end < currentTime)

[philipj_slow, 2015/03/06 03:16:17]

Could we fix the whole problem by tweaking this to use "start > currentTime +
some delta" instead? The delta could be a new
HTMLMediaElement::cachedTimeDelta() implemented as m_cachedTime -
webMediaPlayer()->currentTime(), which could find use in
HTMLMediaElement::currentTime() as well.

[landell, 2015/03/06 13:49:06]

I can look at such a solution. Don't know if the special handling for seeking
and default playback position might get in the way though.

-            continue;
-        int startPosition = int(start * rect.width() / duration);
-        int currentPosition = int(currentTime * rect.width() / duration);
-        int endPosition = int(end * rect.width() / duration);
+    unsigned nearestIdx = bufferedTimeRanges->nearestRange(currentTime);

[philipj_slow, 2015/03/06 03:16:17]

So I was thinking that we could have a getter for the nearest range, but the
ranges themselves aren't objects at all. This API ends up a bit cumbersome I
think.

[landell, 2015/03/06 13:49:06]

I agree. To me the problem is that the TimeRanges interface is kind of weird and
this was an attempt to play nicely with it. :)
I could change it to return a TimeRanges::Range instead if you think it is
better.

+    double start = bufferedTimeRanges->start(nearestIdx, ASSERT_NO_EXCEPTION);
+    double end = bufferedTimeRanges->end(nearestIdx, ASSERT_NO_EXCEPTION);
+    if (std::isnan(start) || std::isnan(end)) {
+        return true;
+    }
+
+    int startPosition = int(start * rect.width() / duration);
+    int currentPosition = int(currentTime * rect.width() / duration);
+    int endPosition = int(end * rect.width() / duration);
 
-        // Add half the thumb width proportionally adjusted to the current painting position.
-        int thumbCenter = mediaSliderThumbWidth / 2;
-        int addWidth = thumbCenter * (1.0 - 2.0 * currentPosition / rect.width());
-        currentPosition += addWidth;
+    // Add half the thumb width proportionally adjusted to the current painting position.
+    int thumbCenter = mediaSliderThumbWidth / 2;
+    int addWidth = thumbCenter * (1.0 - 2.0 * currentPosition / rect.width());
+    currentPosition += addWidth;
+
+    // Only paint ranges that will intersect with the slider thumb

[philipj_slow, 2015/03/06 03:16:17]

I see no explicit delta, so I guess the resolution of the slider is what serves
that role? Couldn't also break in edge cases just like today, where the
alignment of values is just unfortunate?

[landell, 2015/03/06 13:49:06]

My thinking was that it made sense to make the delta condition based on
visibility, i.e. we avoid painting ranges that would introduce gaps between the
thumb and range. It's kind of artificial though, since painting will use the
currentPosition at one end anyway. As you say, there are some corner cases here
but I only think it would be significant for short streams, assuming the diff is
small.

I could add some timeline based delta here or maybe switch to it entirely. This
solution will always have corner cases though but a delta based on timestamps is
probably a bit more robust.

+    if (currentPosition <= endPosition + thumbCenter
+        || currentPosition >= startPosition - thumbCenter) {
 
         // Draw white-ish highlight before current time.
         Color startColor = Color(195, 195, 195);
@@ -238,8 +245,6 @@ static bool paintMediaSlider(LayoutObject* object, const PaintInfo& paintInfo, c
 
         if (endPosition > currentPosition)
             paintSliderRangeHighlight(rect, style, context, currentPosition, endPosition, startColor, endColor);
-
-        return true;
     }
 
     return true;