fully cache small audio/video

fully cache small audio/video

Also, make fully cached audio/videos re-usable even if the server doesn't
support ranges.

This only fixes BUG 716478, for videos smaller than 25 Mb, and only if
we decide to read the whole video during preloading.

Next step: Make fully cached videos seekable.

BUG=716748
Review-Url: https://codereview.chromium.org/2910553002
Cr-Commit-Position: refs/heads/master@{#475122}
Committed: https://chromium.googlesource.com/chromium/src/+/1d6585623812b57dca641bb5ecb0fff82f9c9da4

[hubbe, 2017-05-25 22:41:20]

The CQ bit was checked by hubbe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-25 22:41:59]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hubbe, 2017-05-25 22:42:06]

hubbe@chromium.org changed reviewers:
+ dalecurtis@google.com

[commit-bot: I haz the power, 2017-05-25 23:11:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-25 23:11:26]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[DaleCurtis, 2017-05-25 23:15:26]

dalecurtis@chromium.org changed reviewers:
+ dalecurtis@chromium.org
- dalecurtis@google.com

[DaleCurtis, 2017-05-25 23:15:27]

I don't think we want to fully cache all 25mb videos; that seems like a lot of
data wasted. To add this we'd want to have data suggesting that the majority of
25mb videos are actually played after buffering starts.

For 200 type sources, I think they are rare enough that we could do this. Per
Media.IsStreaming, 2% of videos are 200 type sources.

Regardless this needs a unittest or two to verify the new functionality.

[hubbe, 2017-05-25 23:33:44]

On 2017/05/25 23:15:27, DaleCurtis wrote:
> I don't think we want to fully cache all 25mb videos; that seems like a lot of
> data wasted. To add this we'd want to have data suggesting that the majority
of
> 25mb videos are actually played after buffering starts.
> 
> For 200 type sources, I think they are rare enough that we could do this. Per
> Media.IsStreaming, 2% of videos are 200 type sources.
> 
> Regardless this needs a unittest or two to verify the new functionality.

Note that this doesn't change how much we preload, it only changes what we keep.
I'll look into the failures and add some tests.

[DaleCurtis, 2017-05-25 23:48:35]

Hmm, we're already caching 25mb even for preload=metadata cases?

[hubbe, 2017-05-26 00:22:27]

The CQ bit was checked by hubbe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-26 00:23:25]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hubbe, 2017-05-26 00:31:34]

On 2017/05/25 23:48:35, DaleCurtis wrote:
> Hmm, we're already caching 25mb even for preload=metadata cases?

Uhm, cache != preload...

We will preload only as much as we need to display the first frame.
We'll keep up to 25Mb in the cache before we start throwing stuff away.
preload needs to be less than cache size, but other than that they are
pretty much unrelated.

Or did I misunderstand the question?

[hubbe, 2017-05-26 00:32:07]

On 2017/05/26 00:31:34, hubbe wrote:
> On 2017/05/25 23:48:35, DaleCurtis wrote:
> > Hmm, we're already caching 25mb even for preload=metadata cases?
> 
> Uhm, cache != preload...
> 
> We will preload only as much as we need to display the first frame.
> We'll keep up to 25Mb in the cache before we start throwing stuff away.
> preload needs to be less than cache size, but other than that they are
> pretty much unrelated.
> 
> Or did I misunderstand the question?

Also, I updated and added tests, PTAL.

[commit-bot: I haz the power, 2017-05-26 00:51:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-26 00:51:30]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[DaleCurtis, 2017-05-26 00:58:30]

Okay, so this does not change the amount that we're speculatively loading after
first frame decode completes? This only keeps more around if we happen to have
read the whole file previously? I.e. a playback completed.

[hubbe, 2017-05-26 07:30:12]

On 2017/05/26 00:58:30, DaleCurtis wrote:
> Okay, so this does not change the amount that we're speculatively loading
after
> first frame decode completes? This only keeps more around if we happen to have
> read the whole file previously? I.e. a playback completed.

Exactly right. It changes two things:

1) Files that are already completely in memory, or are 25Mb or less, are locked
in memory for as long as the player is active.
2) Fully cached files can be cached, shared and re-used even if the server
doesn't support ranges

(2) won't work without (1), because if some part gets thrown out, we can't
really recover. (Without re-reading the file from the beginning.)

[DaleCurtis, 2017-05-26 16:43:58]

Thanks for the explanation. lgtm % nits and fixing bots
(Http_CheckLoadingTransition looks broken)

https://codereview.chromium.org/2910553002/diff/20001/media/blink/multibuffer...
File media/blink/multibuffer_data_source.cc (right):

https://codereview.chromium.org/2910553002/diff/20001/media/blink/multibuffer...
media/blink/multibuffer_data_source.cc:619: pin_forward = url_data_->length() *
2;
Why x2?

https://codereview.chromium.org/2910553002/diff/20001/media/blink/url_index.cc
File media/blink/url_index.cc (right):

https://codereview.chromium.org/2910553002/diff/20001/media/blink/url_index.c...
media/blink/url_index.cc:156: return (multibuffer()->FindNextUnavailable(0) <<
kBlockSizeShift) >= length_;
Needs an explanation.

[hubbe, 2017-05-26 17:39:17]

The CQ bit was checked by hubbe@chromium.org to run a CQ dry run

[hubbe, 2017-05-26 17:39:45]

https://codereview.chromium.org/2910553002/diff/20001/media/blink/multibuffer...
File media/blink/multibuffer_data_source.cc (right):

https://codereview.chromium.org/2910553002/diff/20001/media/blink/multibuffer...
media/blink/multibuffer_data_source.cc:619: pin_forward = url_data_->length() *
2;
On 2017/05/26 16:43:57, DaleCurtis wrote:
> Why x2?

I just wanted to avoid any problems at the edges.
Stuff like: Oh, I managed to seek slightly beyond the end of the file, so now
it's ok to throw away the first block...

https://codereview.chromium.org/2910553002/diff/20001/media/blink/url_index.cc
File media/blink/url_index.cc (right):

https://codereview.chromium.org/2910553002/diff/20001/media/blink/url_index.c...
media/blink/url_index.cc:156: return (multibuffer()->FindNextUnavailable(0) <<
kBlockSizeShift) >= length_;
On 2017/05/26 16:43:57, DaleCurtis wrote:
> Needs an explanation.

Done.

[commit-bot: I haz the power, 2017-05-26 17:39:57]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[DaleCurtis, 2017-05-26 18:19:33]

https://codereview.chromium.org/2910553002/diff/20001/media/blink/multibuffer...
File media/blink/multibuffer_data_source.cc (right):

https://codereview.chromium.org/2910553002/diff/20001/media/blink/multibuffer...
media/blink/multibuffer_data_source.cc:619: pin_forward = url_data_->length() *
2;
On 2017/05/26 at 17:39:45, hubbe wrote:
> On 2017/05/26 16:43:57, DaleCurtis wrote:
> > Why x2?
> 
> I just wanted to avoid any problems at the edges.
> Stuff like: Oh, I managed to seek slightly beyond the end of the file, so now
it's ok to throw away the first block...

Okay, please add a comment to that effect since it's not obvious why you'd
choose these values.

[hubbe, 2017-05-26 18:45:12]

https://codereview.chromium.org/2910553002/diff/20001/media/blink/multibuffer...
File media/blink/multibuffer_data_source.cc (right):

https://codereview.chromium.org/2910553002/diff/20001/media/blink/multibuffer...
media/blink/multibuffer_data_source.cc:619: pin_forward = url_data_->length() *
2;
On 2017/05/26 18:19:33, DaleCurtis wrote:
> On 2017/05/26 at 17:39:45, hubbe wrote:
> > On 2017/05/26 16:43:57, DaleCurtis wrote:
> > > Why x2?
> > 
> > I just wanted to avoid any problems at the edges.
> > Stuff like: Oh, I managed to seek slightly beyond the end of the file, so
now
> it's ok to throw away the first block...
> 
> Okay, please add a comment to that effect since it's not obvious why you'd
> choose these values.

Done.

[hubbe, 2017-05-26 18:45:19]

The CQ bit was checked by hubbe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-26 18:46:11]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-26 20:27:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-26 20:27:02]

Dry run: This issue passed the CQ dry run.

[hubbe, 2017-05-26 20:53:11]

The CQ bit was checked by hubbe@chromium.org

[hubbe, 2017-05-26 20:53:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/2910553002/#ps60001
(title: "comment added")

[commit-bot: I haz the power, 2017-05-26 20:54:03]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-26 20:58:46]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1495831991744250,
"parent_rev": "d43f552484b80d116b5e07960d2565e3399d67c0", "commit_rev":
"1d6585623812b57dca641bb5ecb0fff82f9c9da4"}

[commit-bot: I haz the power, 2017-05-26 20:59:03]

Description was changed from

==========
fully cache small audio/video

Also, make fully cached audio/videos re-usable even if the server doesn't
support ranges.

This only fixes BUG 716478, for videos smaller than 25 Mb, and only if
we decide to read the whole video during preloading.

Next step: Make fully cached videos seekable.

BUG=716748
==========

to

==========
fully cache small audio/video

Also, make fully cached audio/videos re-usable even if the server doesn't
support ranges.

This only fixes BUG 716478, for videos smaller than 25 Mb, and only if
we decide to read the whole video during preloading.

Next step: Make fully cached videos seekable.

BUG=716748

Review-Url: https://codereview.chromium.org/2910553002
Cr-Commit-Position: refs/heads/master@{#475122}
Committed:
https://chromium.googlesource.com/chromium/src/+/1d6585623812b57dca641bb5ecb0...
==========

[commit-bot: I haz the power, 2017-05-26 20:59:04]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/1d6585623812b57dca641bb5ecb0...