Suppress pause and buffer for local resources

Suppress pause and buffer for local resources

For local resources we do not need to buffer while the player is paused. "Pause and Buffer" amounts to unnecessary transmission between browser and renderer.

BufferedDataSource already knows whether the resource is local or not. This patch changes the defer strategy in BufferedResourceLoader for local resources.

BUG=129120
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=275840

[amogh.bihani, 2014-05-27 12:04:38]

PTAL.

[AmoghBihani, 2014-05-27 14:17:30]

https://codereview.chromium.org/302553006/diff/1/content/renderer/media/buffe...
File content/renderer/media/buffered_data_source.cc (right):

https://codereview.chromium.org/302553006/diff/1/content/renderer/media/buffe...
content/renderer/media/buffered_data_source.cc:531: // in buffer is not
necessary while the player is paused.
Oh I am sorry, I will change this statement to "Loading data for local sources
is not necessary while player is paused." in the next patch set

[Ami GONE FROM CHROMIUM, 2014-05-27 15:28:37]

I no longer have enough state on the resource loading codepaths to provide a
useful review.  Redirecting to scherkus@.

The CL description could use more motivation about the expected impact.  What
does "buffering" a local resource mean?  What does not buffering such a resource
imply?

[amogh.bihani, 2014-05-28 13:29:14]

Scherkus@
PTAL.

[scherkus (not reviewing), 2014-05-29 03:06:13]

hrmm... it almost seems better if BufferedDataSource could decide this
information itself seeing as it passes in a GURL via Initialize()

for example, BufferedDataSource does an equivalent check inside Initialize() but
stores the information as "assume_fully_buffered_" :)

can we use that instead?


(Ideally, we'd make the BufferedDataSource constructor accept a GURL and
CORSMode and make Initialize() only take a InitializeCB ... that should probably
be done as a separate CL either before this or afterwards)

[amogh.bihani, 2014-05-29 09:38:57]

Thanks for the review. :)
I have made the changes and added an unit test.
PTAL.

https://codereview.chromium.org/302553006/diff/80001/content/renderer/media/b...
File content/renderer/media/buffered_data_source.cc (right):

https://codereview.chromium.org/302553006/diff/80001/content/renderer/media/b...
content/renderer/media/buffered_data_source.cc:520: if
(!loader_->range_supported() && !assume_fully_buffered_) {
in buffered_resource_loader 'range_supported_' is set to false in constructor.
This value changes only in this[1] if block. For local resources this variable
never changes as the code will not go in that if block.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...

https://codereview.chromium.org/302553006/diff/80001/content/renderer/media/b...
File content/renderer/media/buffered_data_source_unittest.cc (right):

https://codereview.chromium.org/302553006/diff/80001/content/renderer/media/b...
content/renderer/media/buffered_data_source_unittest.cc:698:
set_might_be_reused_from_cache_in_future(true);
Need to do this as buffered_resource_loader set it false changes the defer
strategy[1] for the test URL.
---------
[1]
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...

[scherkus (not reviewing), 2014-05-30 20:37:38]

https://codereview.chromium.org/302553006/diff/80001/content/renderer/media/b...
File content/renderer/media/buffered_data_source.cc (right):

https://codereview.chromium.org/302553006/diff/80001/content/renderer/media/b...
content/renderer/media/buffered_data_source.cc:532:
loader_->UpdateDeferStrategy(BufferedResourceLoader::kReadThenDefer);
I feel like kCapacityDefer is right one to use for this case, in which case we
can simplify this code by just having a block that checks for this + move the
check higher in this function

// No need to aggressive buffer when we are assuming the resource
// is fully buffered.
if (assume_fully_buffered_) {
  loader_->UpdateDeferStrategy(BufferedResourceLoader::kCapacityDefer);
  return;
}

https://codereview.chromium.org/302553006/diff/80001/content/renderer/media/b...
File content/renderer/media/buffered_data_source_unittest.cc (right):

https://codereview.chromium.org/302553006/diff/80001/content/renderer/media/b...
content/renderer/media/buffered_data_source_unittest.cc:682: // Test for
pause-and-buffer
nit: comments should end with periods

you should also expand this to cover why we get this result

[amogh.bihani, 2014-06-02 05:01:28]

Thanks for the review. :)
I have made the changes, PTAL.

https://codereview.chromium.org/302553006/diff/80001/content/renderer/media/b...
File content/renderer/media/buffered_data_source.cc (right):

https://codereview.chromium.org/302553006/diff/80001/content/renderer/media/b...
content/renderer/media/buffered_data_source.cc:532:
loader_->UpdateDeferStrategy(BufferedResourceLoader::kReadThenDefer);
On 2014/05/30 20:37:39, scherkus wrote:
> I feel like kCapacityDefer is right one to use for this case, in which case we
> can simplify this code by just having a block that checks for this + move the
> check higher in this function
> 
> // No need to aggressive buffer when we are assuming the resource
> // is fully buffered.
> if (assume_fully_buffered_) {
>   loader_->UpdateDeferStrategy(BufferedResourceLoader::kCapacityDefer);
>   return;
> }

Done.

https://codereview.chromium.org/302553006/diff/80001/content/renderer/media/b...
File content/renderer/media/buffered_data_source_unittest.cc (right):

https://codereview.chromium.org/302553006/diff/80001/content/renderer/media/b...
content/renderer/media/buffered_data_source_unittest.cc:682: // Test for
pause-and-buffer
On 2014/05/30 20:37:39, scherkus wrote:
> nit: comments should end with periods
> 
> you should also expand this to cover why we get this result

Done.

[scherkus (not reviewing), 2014-06-02 20:02:33]

https://codereview.chromium.org/302553006/diff/90001/content/renderer/media/b...
File content/renderer/media/buffered_data_source_unittest.cc (right):

https://codereview.chromium.org/302553006/diff/90001/content/renderer/media/b...
content/renderer/media/buffered_data_source_unittest.cc:223: void
set_might_be_reused_from_cache_in_future(bool value) {
instead of setting this private variable it'd be preferable to create the proper
conditions to get it set in the first place by using response_generator_ to
respond with claiming range isn't supported

I believe Generate200() will do the right thing for you

https://codereview.chromium.org/302553006/diff/90001/content/renderer/media/b...
content/renderer/media/buffered_data_source_unittest.cc:680:
EXPECT_EQ(BufferedResourceLoader::kCapacityDefer, defer_strategy());
I believe this only tests the initial value and not the conditions under which
it could get changed:
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...

we should basically have a few tests:
1) Initialize with file response, test initial value, test after playing, test
after pausing
2) Initialize with 200 response, test initial value, test after playing, test
after pausing
3) Initialize with 206 response, test initial value, test after playing, test
after pausing
4) Initialize with preload == metadata, test initial value == kReadThenDefer

[amogh.bihani, 2014-06-03 10:15:42]

Thanks!
Comments are in PS6 and PS7

https://codereview.chromium.org/302553006/diff/90001/content/renderer/media/b...
File content/renderer/media/buffered_data_source_unittest.cc (right):

https://codereview.chromium.org/302553006/diff/90001/content/renderer/media/b...
content/renderer/media/buffered_data_source_unittest.cc:223: void
set_might_be_reused_from_cache_in_future(bool value) {
On 2014/06/02 20:02:33, scherkus wrote:
> instead of setting this private variable it'd be preferable to create the
proper
> conditions to get it set in the first place by using response_generator_ to
> respond with claiming range isn't supported
> 
> I believe Generate200() will do the right thing for you

Ummm... We want to have 'range_supported_' to be true and
'might_be_used_in_the_future_' also to be true. The latter is set false by
GetReasonsForUncashability[1].
We can set this to true by creating a new response like Generate206WithValidator
to dodge the if statement[2] and get reason == 0.
Otherwise we won't be able to test for kNeverDefer as BufferedResourceLoader
would convert it to kCapacityDefer[3].

So should we create a new repsonse - Generate206WithValidator()?
-----
[1]
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...

[2]
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...

[3]
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...

https://codereview.chromium.org/302553006/diff/90001/content/renderer/media/b...
content/renderer/media/buffered_data_source_unittest.cc:680:
EXPECT_EQ(BufferedResourceLoader::kCapacityDefer, defer_strategy());
On 2014/06/02 20:02:33, scherkus wrote:
> I believe this only tests the initial value and not the conditions under which
> it could get changed:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...
> 
> we should basically have a few tests:
> 1) Initialize with file response, test initial value, test after playing, test
> after pausing
> 2) Initialize with 200 response, test initial value, test after playing, test
> after pausing
> 3) Initialize with 206 response, test initial value, test after playing, test
> after pausing
> 4) Initialize with preload == metadata, test initial value == kReadThenDefer

Done.

https://codereview.chromium.org/302553006/diff/110001/content/renderer/media/...
File content/renderer/media/buffered_data_source_unittest.cc (right):

https://codereview.chromium.org/302553006/diff/110001/content/renderer/media/...
content/renderer/media/buffered_data_source_unittest.cc:231: void
set_might_be_reused_from_cache_in_future(bool value) {
I have explained why we need it here in comments in previous patchset.

https://codereview.chromium.org/302553006/diff/110001/content/renderer/media/...
File content/renderer/media/buffered_resource_loader.cc (right):

https://codereview.chromium.org/302553006/diff/110001/content/renderer/media/...
content/renderer/media/buffered_resource_loader.cc:392: if
(url_.SchemeIsHTTPOrHTTPS()) {
This is OOC. I thought I should not create a new patch just for this.
I hope it is ok.

[scherkus (not reviewing), 2014-06-03 18:08:51]

lgtm

[amogh.bihani, 2014-06-04 02:54:10]

The CQ bit was checked by amogh.bihani@samsung.com

[commit-bot: I haz the power, 2014-06-04 02:55:28]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/amogh.bihani@samsung.com/302553006/110001

[amogh.bihani, 2014-06-04 04:18:17]

The CQ bit was unchecked by amogh.bihani@samsung.com

[amogh.bihani, 2014-06-04 09:41:53]

The CQ bit was checked by amogh.bihani@samsung.com

[commit-bot: I haz the power, 2014-06-04 09:44:10]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/amogh.bihani@samsung.com/302553006/120001

[AmoghBihani, 2014-06-04 15:30:46]

Sorry, I need you to again have a look.
Unittests had to be changed after the patch which modified the
BufferedDataSource constructor landed.

Now the scoped_ptr is reset in Initialize() and hence we cannot set preload
before calling Initialize(), otherwise the constructor will set preload_ to
AUTO.
Also preload value should be set before data_source_->Initialize() is called,
for the test scenario we want to test, hence I had to do these changes.

PTAL.

[amogh.bihani, 2014-06-06 13:44:42]

On 2014/06/04 15:30:46, AmoghBihani wrote:
> Sorry, I need you to again have a look.
> Unittests had to be changed after the patch which modified the
> BufferedDataSource constructor landed.
> 
> Now the scoped_ptr is reset in Initialize() and hence we cannot set preload
> before calling Initialize(), otherwise the constructor will set preload_ to
> AUTO.
> Also preload value should be set before data_source_->Initialize() is called,
> for the test scenario we want to test, hence I had to do these changes.
> 
> PTAL.

Ping!

[scherkus (not reviewing), 2014-06-06 16:39:20]

https://codereview.chromium.org/302553006/diff/180001/content/renderer/media/...
File content/renderer/media/buffered_data_source_unittest.cc (right):

https://codereview.chromium.org/302553006/diff/180001/content/renderer/media/...
content/renderer/media/buffered_data_source_unittest.cc:124: void
Initialize(const char* url, bool expected, Preload preload = AUTO) {
instead of default arguments (which the style guide recommends against) use a
member variable and a setter on BufferedDataSourceTest

e.g., 

void set_preload(Preload preload) { preload_ = preload; }
Preload preload_;  // ctor sets it to AUTO

void Initialize(...) {
  data_source_->SetPreload(preload_);
}

that way only the tests that need to have it set to METADATA can call
set_preload(METADATA) prior to calling InitializeXXX()

[amogh.bihani, 2014-06-09 03:41:30]

Thanks!
I have made the changes.

[amogh.bihani, 2014-06-09 08:36:23]

The CQ bit was checked by amogh.bihani@samsung.com

[commit-bot: I haz the power, 2014-06-09 08:38:09]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/amogh.bihani@samsung.com/302553006/220001

[commit-bot: I haz the power, 2014-06-09 12:56:11]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-06-09 19:58:50]

Change committed as 275840