Bug 85925 – [chromium] Default media controls should render only the currentTime-containing buffered range

• Component: Media
• Status: RESOLVED FIXED

[Ami Fischman]

<audio> and <video> elements have a buffered() accessor that is meant to return time ranges corresponding to buffered bytes.
Today chromium always claims to have buffered [0,t) where t corresponds to the time of the latest buffered byte, even when a seek might have skipped over the majority of the bytes.

My current plan of attack is:
1) Change RenderMediaControlsChromium.cpp:paintMediaSlider() to preserve the current user-visible controls painting in the presence of multiple buffered regions.
2) Fix chromium to provide the correct TimeRanges for buffered() (this is http://crbug.com/103513).  This will let us tell JS the truth.
3) Prototype RenderMediaControlsChromium changes to paint disjoint buffered ranges in the slider
...and then collect feedback on whether this is last step is a good idea or not.  Either land this to have default controls UI tell the truth, or else revert and continue with the lie (or present an alternative UI).

[Ami Fischman]

Items 1 & 2 are done.
Conversations around UX have converged to feeling showing more than one buffered range in the default controls would be too "busy" visually, and that instead only the buffered range containing currentTime should be rendered.  I'll upload a patch for that.

[Ami Fischman]

Created attachment 144088 [details]
Patch

[Eric Carlson]

Comment on attachment 144088 [details]
Patch

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

Overall this looks fine, but I am extremely hesitant to r+ it without a test, or a compelling explanation of why one isn't necessary.

> Source/WebCore/ChangeLog:8
> +        No new tests - the buffering window is significantly bigger than any of our test data files.

I would *really* like to see a test for this. Can you try using a long audio-only file (long for our tests, maybe 60 seconds) with http/tests/media/video-throttled-load.cgi?

> Source/WebCore/rendering/RenderMediaControlsChromium.cpp:147
> +    if (isnan(duration) || isinf(duration) || isnan(currentTime))
> +        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 (isnan(start) || isnan(end) || start > currentTime || end < currentTime)
> +            continue;
> +        float startFraction = start / duration;
> +        float endFraction = end / duration;

What if duration is 0?

> Source/WebCore/rendering/RenderMediaControlsChromium.cpp:153
> +        // Don't bother drawing an empty area.
> +        if (!bufferedRect.isEmpty()) {

You might change this to an early return.

[Ami Fischman]

Created attachment 144094 [details]
Patch

[Ami Fischman]

Comment on attachment 144088 [details]
Patch

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

>> Source/WebCore/ChangeLog:8
>> +        No new tests - the buffering window is significantly bigger than any of our test data files.
> 
> I would *really* like to see a test for this. Can you try using a long audio-only file (long for our tests, maybe 60 seconds) with http/tests/media/video-throttled-load.cgi?

The window is a max of time & space, and the space is 20MB.
I think the only way to actually test this is with a CGI that *generates* data (so the total bytes can exceed the buffering window) but also knows how to generate a container that promises a long duration.  Maybe once bug 84365 lands we can use that.

>> Source/WebCore/rendering/RenderMediaControlsChromium.cpp:147
>> +        float endFraction = end / duration;
> 
> What if duration is 0?

Done (at l.138).

>> Source/WebCore/rendering/RenderMediaControlsChromium.cpp:153
>> +        if (!bufferedRect.isEmpty()) {
> 
> You might change this to an early return.

Done.

[Ami Fischman]

Created attachment 144129 [details]
Patch

[Ami Fischman]

(In reply to comment #5)
> (From update of attachment 144088 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144088&action=review
> 
> >> Source/WebCore/ChangeLog:8
> >> +        No new tests - the buffering window is significantly bigger than any of our test data files.
> > 
> > I would *really* like to see a test for this. Can you try using a long audio-only file (long for our tests, maybe 60 seconds) with http/tests/media/video-throttled-load.cgi?
> 
> The window is a max of time & space, and the space is 20MB.

I was forgetting about the effect of throttling.  
Added layouttest for this now.

[Eric Carlson]

Comment on attachment 144129 [details]
Patch

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

Nicely done!

> Source/WebCore/ChangeLog:7
> +        [chromium] Default media controls should render only the currentTime-containing buffered range
> +        https://bugs.webkit.org/show_bug.cgi?id=85925
> +
> +        Reviewed by NOBODY (OOPS!).
> +

Don't forget to mention your spiffy new test!

[Ami Fischman]

Committed r118577: <http://trac.webkit.org/changeset/118577>

[Chris Dumez]

The new test is failing on both GTK and EFL ports (Bug 87575).

[Ami Fischman]

(In reply to comment #10)
> The new test is failing on both GTK and EFL ports (Bug 87575).

Ok, will follow up there.