Numerous fixes to audio/video buffered resource loading.

Numerous fixes to audio/video buffered resource loading.

This patch fixes a few related issues:
  1) Default loading strategy is now threshold-then-defer
  2) Specify more reasonable default bitrate/playback rate values
  3) Use a minimum buffer window size to prevent underflows on low bitrate content
  4) Remember bitrate/playback rate values between resource loaders

The default loading strategy of read-then-defer had a negative impact on initial latency as we were constantly deferring/undeferring the connection during the time when we need data the fastest.

While this change does result in loading a pinch more data for preload=metadata scenarios, it vastly improves the common preload=auto scenario.

BUG=99775
TEST=test_shell_tests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=105121

[acolwell GONE FROM CHROMIUM, 2011-10-11 16:18:01]

Looking pretty good. Where are the new tests that repro the bugs you were
seeing?

http://codereview.chromium.org/8224028/diff/1/webkit/glue/media/buffered_reso...
File webkit/glue/media/buffered_resource_loader.cc (left):

http://codereview.chromium.org/8224028/diff/1/webkit/glue/media/buffered_reso...
webkit/glue/media/buffered_resource_loader.cc:553: if (playback_rate_ == 0.0)
Sanity check. This was removed because of the new playback_rate clamping logic
in ComputeTargetBufferWindow()? Do we really want to adjust the buffer size on
pause?

http://codereview.chromium.org/8224028/diff/1/webkit/glue/media/buffered_reso...
File webkit/glue/media/buffered_resource_loader.cc (right):

http://codereview.chromium.org/8224028/diff/1/webkit/glue/media/buffered_reso...
webkit/glue/media/buffered_resource_loader.cc:74: playback_rate =
std::max(playback_rate, 1.0f);
Why are we mapping 0 & all negative playback_rates to 1 here? If so the comment
should be updated to reflect why.

If playback rate is 0 do we really want to adjust to 1x size? It seems like we'd
want to keep the buffer the same size since it is more likely that we'll unpause
at the same rate before the pause.

http://codereview.chromium.org/8224028/diff/1/webkit/glue/media/buffered_reso...
File webkit/glue/media/buffered_resource_loader_unittest.cc (right):

http://codereview.chromium.org/8224028/diff/1/webkit/glue/media/buffered_reso...
webkit/glue/media/buffered_resource_loader_unittest.cc:729:
TEST_F(BufferedResourceLoaderTest, BufferWindow_PlaybackRate_TooLow) {
Not a fan of the _TooLow, _JustRight, and _TooHigh suffixes. They make me think
"Compared to what?" These tests are to verify clamping behavior so perhaps
something like _BelowLowerBound, _WithinBounds, _AboveUpperBound would be
better.

[vrk (LEFT CHROMIUM), 2011-10-11 18:23:20]

Looks good; mostly just questions! Also echoing what acolwell said about tests -
can you add a test to detect the improper behavior?

Thanks for fixing!!

http://codereview.chromium.org/8224028/diff/1/webkit/glue/media/buffered_data...
File webkit/glue/media/buffered_data_source.cc (right):

http://codereview.chromium.org/8224028/diff/1/webkit/glue/media/buffered_data...
webkit/glue/media/buffered_data_source.cc:361: // If playback hasn't started or
the page indicated preload=auto,
"If playback hasn't started" should be "if video is in the middle of playback"

http://codereview.chromium.org/8224028/diff/1/webkit/glue/media/buffered_data...
webkit/glue/media/buffered_data_source.cc:362: // buffer enable/disable
deferring when the buffer is full/depleted.
If playback hasn't started *and* the page indicated preload=auto, the logic has
changed from (before) aggressively download as much of the video as possible to
(in this CL) download only up to a threshold.

Why the change? The spec is shady in the whole preload area but it gives us
permission to load the entire resource
(http://www.w3.org/TR/html5/video.html#attr-media-preload)

Not necessarily opposed, but curious!

http://codereview.chromium.org/8224028/diff/1/webkit/glue/media/buffered_reso...
File webkit/glue/media/buffered_resource_loader.cc (left):

http://codereview.chromium.org/8224028/diff/1/webkit/glue/media/buffered_reso...
webkit/glue/media/buffered_resource_loader.cc:553: if (playback_rate_ == 0.0)
+1 what acolwell said; I don't think it makes sense to adjust buffer window on
pause.

http://codereview.chromium.org/8224028/diff/1/webkit/glue/media/buffered_reso...
File webkit/glue/media/buffered_resource_loader_unittest.cc (left):

http://codereview.chromium.org/8224028/diff/1/webkit/glue/media/buffered_reso...
webkit/glue/media/buffered_resource_loader_unittest.cc:808:
TEST_F(BufferedResourceLoaderTest, BufferWindow_PlaybackRate_Decrease) {
I like the clean-up!

One comment is that the unit tests now only test the clamping behavior of the
buffer window when adjusting the playback rate/bitrate. Is that the only thing
we care to test?

(For example, the old tests included sanity check on increasing/decreasing
window sizes and going from play to pause. Is the ugliness/usefulness trade-off
not worth it, do you think?)

[scherkus (not reviewing), 2011-10-11 20:13:18]

added unit tests and fixed comments

http://codereview.chromium.org/8224028/diff/1/webkit/glue/media/buffered_data...
File webkit/glue/media/buffered_data_source.cc (right):

http://codereview.chromium.org/8224028/diff/1/webkit/glue/media/buffered_data...
webkit/glue/media/buffered_data_source.cc:361: // If playback hasn't started or
the page indicated preload=auto,
On 2011/10/11 18:23:21, Victoria Kirst wrote:
> "If playback hasn't started" should be "if video is in the middle of playback"

Done.

http://codereview.chromium.org/8224028/diff/1/webkit/glue/media/buffered_data...
webkit/glue/media/buffered_data_source.cc:362: // buffer enable/disable
deferring when the buffer is full/depleted.
On 2011/10/11 18:23:21, Victoria Kirst wrote:
> If playback hasn't started *and* the page indicated preload=auto, the logic
has
> changed from (before) aggressively download as much of the video as possible
to
> (in this CL) download only up to a threshold.
> 
> Why the change? The spec is shady in the whole preload area but it gives us
> permission to load the entire resource
> (http://www.w3.org/TR/html5/video.html#attr-media-preload)
> 
> Not necessarily opposed, but curious!

kNeverDefer isn't preferable in all cases.

On a video larger than the media cache (i.e., ~50MB), or for a page with many
files, using kNeverDefer will "download" the content hoping that it'll get
cached.

When we actually go to play the content there's an increased risk we'll have an
immediate cache miss and have to re-fetch the media again negating any benefits
from using kNeverDefer.

http://codereview.chromium.org/8224028/diff/1/webkit/glue/media/buffered_reso...
File webkit/glue/media/buffered_resource_loader.cc (left):

http://codereview.chromium.org/8224028/diff/1/webkit/glue/media/buffered_reso...
webkit/glue/media/buffered_resource_loader.cc:553: if (playback_rate_ == 0.0)
On 2011/10/11 16:18:01, acolwell wrote:
> Sanity check. This was removed because of the new playback_rate clamping logic
> in ComputeTargetBufferWindow()? Do we really want to adjust the buffer size on
> pause?

Moved to SetPlaybackRate()

http://codereview.chromium.org/8224028/diff/1/webkit/glue/media/buffered_reso...
webkit/glue/media/buffered_resource_loader.cc:553: if (playback_rate_ == 0.0)
On 2011/10/11 18:23:21, Victoria Kirst wrote:
> +1 what acolwell said; I don't think it makes sense to adjust buffer window on
> pause.

Done.

http://codereview.chromium.org/8224028/diff/1/webkit/glue/media/buffered_reso...
File webkit/glue/media/buffered_resource_loader.cc (right):

http://codereview.chromium.org/8224028/diff/1/webkit/glue/media/buffered_reso...
webkit/glue/media/buffered_resource_loader.cc:74: playback_rate =
std::max(playback_rate, 1.0f);
On 2011/10/11 16:18:01, acolwell wrote:
> Why are we mapping 0 & all negative playback_rates to 1 here? If so the
comment
> should be updated to reflect why.
> 
> If playback rate is 0 do we really want to adjust to 1x size? It seems like
we'd
> want to keep the buffer the same size since it is more likely that we'll
unpause
> at the same rate before the pause.

whoops missed absolute value of playback_rate!

it's the values from [-1..1] that we shouldn't bother scaling

I removed the check for playback_rate == 0 because I view this function as
something that should always give us the most sensible buffer size, even if we
don't have the best information on hand (i.e., during construction when
bitrate==0, playback_rate==0)

Initializing buffer_ in the constructor with canned values as opposed to using
this function could lead to an immediate buffer resize as soon as
bitrate/playback rate is set, which is why we now always use this function when
calculating sizes for buffer_

http://codereview.chromium.org/8224028/diff/1/webkit/glue/media/buffered_reso...
File webkit/glue/media/buffered_resource_loader_unittest.cc (right):

http://codereview.chromium.org/8224028/diff/1/webkit/glue/media/buffered_reso...
webkit/glue/media/buffered_resource_loader_unittest.cc:729:
TEST_F(BufferedResourceLoaderTest, BufferWindow_PlaybackRate_TooLow) {
On 2011/10/11 16:18:01, acolwell wrote:
> Not a fan of the _TooLow, _JustRight, and _TooHigh suffixes. They make me
think
> "Compared to what?" These tests are to verify clamping behavior so perhaps
> something like _BelowLowerBound, _WithinBounds, _AboveUpperBound would be
> better.

Done.

[acolwell GONE FROM CHROMIUM, 2011-10-11 20:27:09]

LGTM

[vrk (LEFT CHROMIUM), 2011-10-11 20:44:40]

Small nits but LGTM

http://codereview.chromium.org/8224028/diff/4005/webkit/glue/media/buffered_d...
File webkit/glue/media/buffered_data_source_unittest.cc (right):

http://codereview.chromium.org/8224028/diff/4005/webkit/glue/media/buffered_d...
webkit/glue/media/buffered_data_source_unittest.cc:598: 
nit: remove line

http://codereview.chromium.org/8224028/diff/4005/webkit/glue/media/buffered_d...
webkit/glue/media/buffered_data_source_unittest.cc:692: // Accessors for private
variables.
nit: "for |data_source_|'s private variables"

http://codereview.chromium.org/8224028/diff/4005/webkit/glue/media/buffered_d...
webkit/glue/media/buffered_data_source_unittest.cc:709: MessageLoop*
message_loop_;
nit: add private:
DISALLOW_COPY_AND_ASSIGN(BufferedDataSourceTest2);

[scherkus (not reviewing), 2011-10-11 22:28:57]

thanks for the review!!

http://codereview.chromium.org/8224028/diff/4005/webkit/glue/media/buffered_d...
File webkit/glue/media/buffered_data_source_unittest.cc (right):

http://codereview.chromium.org/8224028/diff/4005/webkit/glue/media/buffered_d...
webkit/glue/media/buffered_data_source_unittest.cc:598: 
On 2011/10/11 20:44:40, Victoria Kirst wrote:
> nit: remove line

Done.

http://codereview.chromium.org/8224028/diff/4005/webkit/glue/media/buffered_d...
webkit/glue/media/buffered_data_source_unittest.cc:692: // Accessors for private
variables.
On 2011/10/11 20:44:40, Victoria Kirst wrote:
> nit: "for |data_source_|'s private variables"

Done.

http://codereview.chromium.org/8224028/diff/4005/webkit/glue/media/buffered_d...
webkit/glue/media/buffered_data_source_unittest.cc:709: MessageLoop*
message_loop_;
On 2011/10/11 20:44:40, Victoria Kirst wrote:
> nit: add private:
> DISALLOW_COPY_AND_ASSIGN(BufferedDataSourceTest2);

Done.