Reduce number of active codecs on low end devices.

Reduce number of active codecs on low end devices.

This fixes OOM crashes in the mediaserver process on older devices
by suspending players more frequently and preventing unused players
and SurfaceTextures from ever being created. Both fixes are required
to enable playback on JellyBean devices; independently neither is
sufficient for playback to succeed.

- On low end devices, suspends all idle players immediately upon
  playback of another player.
- On low end devices, if a codec already exists, codec creation is
  deferred until the first Decode() call. Resolves issues with sites
  like vimeo.com that are appending initialization segments to MSE
  but never any further data.
- Once more than 2 players exist on a low end device, every extra
  player will cause immediate idle player collection. Normal devices
  will now do this after 8 players.

A followup CL will set preload=none for all JellyBean devices.

BUG=612909
TEST=manual, new tests.

Committed: https://crrev.com/d0ab2edb1cd3b57fbc48b3b39492a8d23221e648
Cr-Commit-Position: refs/heads/master@{#419375}

[DaleCurtis, 2016-09-12 23:50:08]

dalecurtis@chromium.org changed reviewers:
+ liberato@chromium.org, sandersd@chromium.org

[DaleCurtis, 2016-09-12 23:50:09]

Still needs test delegate test updates, but is otherwise ready for review.

[liberato (no reviews please), 2016-09-13 15:34:28]

nifty.

https://codereview.chromium.org/2333983002/diff/1/content/renderer/media/rend...
File content/renderer/media/renderer_webmediaplayer_delegate.cc (right):

https://codereview.chromium.org/2333983002/diff/1/content/renderer/media/rend...
content/renderer/media/renderer_webmediaplayer_delegate.cc:202:
CleanupIdleDelegates(idle_timeout_ / 10);
10?

https://codereview.chromium.org/2333983002/diff/1/content/renderer/media/rend...
content/renderer/media/renderer_webmediaplayer_delegate.cc:219: base::TimeDelta
idle_timeout) {
|idle_timeout| and |idle_timeout_| are a little confusing.  perhaps just
|timeout| here?

https://codereview.chromium.org/2333983002/diff/1/media/gpu/android_video_dec...
File media/gpu/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/2333983002/diff/1/media/gpu/android_video_dec...
media/gpu/android_video_decode_accelerator.cc:516:
deferred_initialization_pending_ = false;
can you restructure this a bit so that |deferred_initialization_pending_|
doesn't get set-unless-we-reset-it-later?  something like setting
|defer_surface_creation_| and |deferred_init...| earlier, and just
if(defer_surface_creation_) { Notify...;return true; } here.

otherwise, |deferred_...| is set incorrectly for a while, in some sense.

https://codereview.chromium.org/2333983002/diff/1/media/gpu/android_video_dec...
media/gpu/android_video_decode_accelerator.cc:543:
DCHECK(deferred_initialization_pending_);
maybe DCHECK !defer_surface_creation_, mostly as the other half of the check in
::Decode.

https://codereview.chromium.org/2333983002/diff/1/media/gpu/android_video_dec...
media/gpu/android_video_decode_accelerator.cc:946: if
(!make_context_current_cb_.Run() ||
hrm, i just noticed that the OnSurfaceAvailable =>
InitializePictureBufferManager path doesn't make the context current.

Since ::Initialize only makes the context current for InitializePBM anyway,
would you mind moving both |make_contenxt...| calls to somewhere near the top of
InitializePBM?  then it can take care of the context itself.

https://codereview.chromium.org/2333983002/diff/1/media/gpu/android_video_dec...
File media/gpu/android_video_decode_accelerator.h (right):

https://codereview.chromium.org/2333983002/diff/1/media/gpu/android_video_dec...
media/gpu/android_video_decode_accelerator.h:146: bool
InitializePictureBufferManager(bool force_async_init);
i think that it's almost the same if one has InitializePictureBufferManager
check (and clear) |defer_surface_creation_|, rather than take an argument.

[sandersd (OOO until July 31), 2016-09-13 23:02:42]

Not much to add to Frank's comments.

https://codereview.chromium.org/2333983002/diff/1/content/renderer/media/rend...
File content/renderer/media/renderer_webmediaplayer_delegate.cc (right):

https://codereview.chromium.org/2333983002/diff/1/content/renderer/media/rend...
content/renderer/media/renderer_webmediaplayer_delegate.cc:28: bool
IsLowEndDevice() {
Please comment that this check is specifically to include Jelly Bean. (Possibly
worth throwing in that JB MediaCodec is notoriously buggy.)

[DaleCurtis, 2016-09-15 23:03:55]

PTAL, thanks!

https://codereview.chromium.org/2333983002/diff/1/content/renderer/media/rend...
File content/renderer/media/renderer_webmediaplayer_delegate.cc (right):

https://codereview.chromium.org/2333983002/diff/1/content/renderer/media/rend...
content/renderer/media/renderer_webmediaplayer_delegate.cc:28: bool
IsLowEndDevice() {
On 2016/09/13 at 23:02:42, sandersd wrote:
> Please comment that this check is specifically to include Jelly Bean.
(Possibly worth throwing in that JB MediaCodec is notoriously buggy.)

Done.

https://codereview.chromium.org/2333983002/diff/1/content/renderer/media/rend...
content/renderer/media/renderer_webmediaplayer_delegate.cc:202:
CleanupIdleDelegates(idle_timeout_ / 10);
On 2016/09/13 at 15:34:28, liberato wrote:
> 10?

Just wanted something over 0, but now I don't see the point, so no additional
timeouts added.

https://codereview.chromium.org/2333983002/diff/1/content/renderer/media/rend...
content/renderer/media/renderer_webmediaplayer_delegate.cc:219: base::TimeDelta
idle_timeout) {
On 2016/09/13 at 15:34:28, liberato wrote:
> |idle_timeout| and |idle_timeout_| are a little confusing.  perhaps just
|timeout| here?

Done.

https://codereview.chromium.org/2333983002/diff/1/media/gpu/android_video_dec...
File media/gpu/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/2333983002/diff/1/media/gpu/android_video_dec...
media/gpu/android_video_decode_accelerator.cc:516:
deferred_initialization_pending_ = false;
On 2016/09/13 at 15:34:28, liberato wrote:
> can you restructure this a bit so that |deferred_initialization_pending_|
doesn't get set-unless-we-reset-it-later?  something like setting
|defer_surface_creation_| and |deferred_init...| earlier, and just
if(defer_surface_creation_) { Notify...;return true; } here.
> 
> otherwise, |deferred_...| is set incorrectly for a while, in some sense.

Done.

https://codereview.chromium.org/2333983002/diff/1/media/gpu/android_video_dec...
media/gpu/android_video_decode_accelerator.cc:543:
DCHECK(deferred_initialization_pending_);
On 2016/09/13 at 15:34:28, liberato wrote:
> maybe DCHECK !defer_surface_creation_, mostly as the other half of the check
in ::Decode.

Done.

https://codereview.chromium.org/2333983002/diff/1/media/gpu/android_video_dec...
media/gpu/android_video_decode_accelerator.cc:946: if
(!make_context_current_cb_.Run() ||
On 2016/09/13 at 15:34:28, liberato wrote:
> hrm, i just noticed that the OnSurfaceAvailable =>
InitializePictureBufferManager path doesn't make the context current.
> 
> Since ::Initialize only makes the context current for InitializePBM anyway,
would you mind moving both |make_contenxt...| calls to somewhere near the top of
InitializePBM?  then it can take care of the context itself.

Done.

https://codereview.chromium.org/2333983002/diff/1/media/gpu/android_video_dec...
File media/gpu/android_video_decode_accelerator.h (right):

https://codereview.chromium.org/2333983002/diff/1/media/gpu/android_video_dec...
media/gpu/android_video_decode_accelerator.h:146: bool
InitializePictureBufferManager(bool force_async_init);
On 2016/09/13 at 15:34:28, liberato wrote:
> i think that it's almost the same if one has InitializePictureBufferManager
check (and clear) |defer_surface_creation_|, rather than take an argument.

Good point. Done.

[liberato (no reviews please), 2016-09-16 06:06:35]

On 2016/09/15 23:03:55, DaleCurtis wrote:
> PTAL, thanks!
> 
>
https://codereview.chromium.org/2333983002/diff/1/content/renderer/media/rend...
> File content/renderer/media/renderer_webmediaplayer_delegate.cc (right):
> 
>
https://codereview.chromium.org/2333983002/diff/1/content/renderer/media/rend...
> content/renderer/media/renderer_webmediaplayer_delegate.cc:28: bool
> IsLowEndDevice() {
> On 2016/09/13 at 23:02:42, sandersd wrote:
> > Please comment that this check is specifically to include Jelly Bean.
> (Possibly worth throwing in that JB MediaCodec is notoriously buggy.)
> 
> Done.
> 
>
https://codereview.chromium.org/2333983002/diff/1/content/renderer/media/rend...
> content/renderer/media/renderer_webmediaplayer_delegate.cc:202:
> CleanupIdleDelegates(idle_timeout_ / 10);
> On 2016/09/13 at 15:34:28, liberato wrote:
> > 10?
> 
> Just wanted something over 0, but now I don't see the point, so no additional
> timeouts added.
> 
>
https://codereview.chromium.org/2333983002/diff/1/content/renderer/media/rend...
> content/renderer/media/renderer_webmediaplayer_delegate.cc:219:
base::TimeDelta
> idle_timeout) {
> On 2016/09/13 at 15:34:28, liberato wrote:
> > |idle_timeout| and |idle_timeout_| are a little confusing.  perhaps just
> |timeout| here?
> 
> Done.
> 
>
https://codereview.chromium.org/2333983002/diff/1/media/gpu/android_video_dec...
> File media/gpu/android_video_decode_accelerator.cc (right):
> 
>
https://codereview.chromium.org/2333983002/diff/1/media/gpu/android_video_dec...
> media/gpu/android_video_decode_accelerator.cc:516:
> deferred_initialization_pending_ = false;
> On 2016/09/13 at 15:34:28, liberato wrote:
> > can you restructure this a bit so that |deferred_initialization_pending_|
> doesn't get set-unless-we-reset-it-later?  something like setting
> |defer_surface_creation_| and |deferred_init...| earlier, and just
> if(defer_surface_creation_) { Notify...;return true; } here.
> > 
> > otherwise, |deferred_...| is set incorrectly for a while, in some sense.
> 
> Done.
> 
>
https://codereview.chromium.org/2333983002/diff/1/media/gpu/android_video_dec...
> media/gpu/android_video_decode_accelerator.cc:543:
> DCHECK(deferred_initialization_pending_);
> On 2016/09/13 at 15:34:28, liberato wrote:
> > maybe DCHECK !defer_surface_creation_, mostly as the other half of the check
> in ::Decode.
> 
> Done.
> 
>
https://codereview.chromium.org/2333983002/diff/1/media/gpu/android_video_dec...
> media/gpu/android_video_decode_accelerator.cc:946: if
> (!make_context_current_cb_.Run() ||
> On 2016/09/13 at 15:34:28, liberato wrote:
> > hrm, i just noticed that the OnSurfaceAvailable =>
> InitializePictureBufferManager path doesn't make the context current.
> > 
> > Since ::Initialize only makes the context current for InitializePBM anyway,
> would you mind moving both |make_contenxt...| calls to somewhere near the top
of
> InitializePBM?  then it can take care of the context itself.
> 
> Done.
> 
>
https://codereview.chromium.org/2333983002/diff/1/media/gpu/android_video_dec...
> File media/gpu/android_video_decode_accelerator.h (right):
> 
>
https://codereview.chromium.org/2333983002/diff/1/media/gpu/android_video_dec...
> media/gpu/android_video_decode_accelerator.h:146: bool
> InitializePictureBufferManager(bool force_async_init);
> On 2016/09/13 at 15:34:28, liberato wrote:
> > i think that it's almost the same if one has InitializePictureBufferManager
> check (and clear) |defer_surface_creation_|, rather than take an argument.
> 
> Good point. Done.

lgtm.

[sandersd (OOO until July 31), 2016-09-16 18:09:50]

lgtm % nit.

https://codereview.chromium.org/2333983002/diff/20001/content/renderer/media/...
File content/renderer/media/renderer_webmediaplayer_delegate.cc (right):

https://codereview.chromium.org/2333983002/diff/20001/content/renderer/media/...
content/renderer/media/renderer_webmediaplayer_delegate.cc:203: if
(idle_delegate_map_.size() > (is_low_end_device_ ? 2 : 8))
Nit: please explain how these constants were chosen.

[DaleCurtis, 2016-09-16 21:59:16]

The CQ bit was checked by dalecurtis@chromium.org

[DaleCurtis, 2016-09-16 21:59:16]

https://codereview.chromium.org/2333983002/diff/20001/content/renderer/media/...
File content/renderer/media/renderer_webmediaplayer_delegate.cc (right):

https://codereview.chromium.org/2333983002/diff/20001/content/renderer/media/...
content/renderer/media/renderer_webmediaplayer_delegate.cc:203: if
(idle_delegate_map_.size() > (is_low_end_device_ ? 2 : 8))
On 2016/09/16 at 18:09:50, sandersd wrote:
> Nit: please explain how these constants were chosen.

Done.

[DaleCurtis, 2016-09-16 21:59:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
liberato@chromium.org, sandersd@chromium.org
Link to the patchset: https://codereview.chromium.org/2333983002/#ps40001
(title: "Add comment.")

[commit-bot: I haz the power, 2016-09-16 22:00:01]

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, 2016-09-16 23:19:38]

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

[commit-bot: I haz the power, 2016-09-16 23:19:40]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[DaleCurtis, 2016-09-16 23:34:56]

The CQ bit was checked by dalecurtis@chromium.org

[DaleCurtis, 2016-09-16 23:34:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
liberato@chromium.org, sandersd@chromium.org
Link to the patchset: https://codereview.chromium.org/2333983002/#ps60001
(title: "Fix windows.")

[commit-bot: I haz the power, 2016-09-16 23:35:36]

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, 2016-09-17 01:34:06]

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

[commit-bot: I haz the power, 2016-09-17 01:34:07]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[DaleCurtis, 2016-09-17 05:31:46]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2016-09-17 05:31:59]

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, 2016-09-17 19:39:29]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-17 19:43:04]

Description was changed from

==========
Reduce number of active codecs on low end devices.

This fixes OOM crashes in the mediaserver process on older devices
by suspending players more frequently and preventing unused players
and SurfaceTextures from ever being created. Both fixes are required
to enable playback on JellyBean devices; independently neither is
sufficient for playback to succeed.

- On low end devices, suspends all idle players immediately upon
  playback of another player.
- On low end devices, if a codec already exists, codec creation is
  deferred until the first Decode() call. Resolves issues with sites
  like vimeo.com that are appending initialization segments to MSE
  but never any further data.
- Once more than 2 players exist on a low end device, every extra
  player will cause immediate idle player collection. Normal devices
  will now do this after 8 players.

A followup CL will set preload=none for all JellyBean devices.

BUG=612909
TEST=manual, new tests.
==========

to

==========
Reduce number of active codecs on low end devices.

This fixes OOM crashes in the mediaserver process on older devices
by suspending players more frequently and preventing unused players
and SurfaceTextures from ever being created. Both fixes are required
to enable playback on JellyBean devices; independently neither is
sufficient for playback to succeed.

- On low end devices, suspends all idle players immediately upon
  playback of another player.
- On low end devices, if a codec already exists, codec creation is
  deferred until the first Decode() call. Resolves issues with sites
  like vimeo.com that are appending initialization segments to MSE
  but never any further data.
- Once more than 2 players exist on a low end device, every extra
  player will cause immediate idle player collection. Normal devices
  will now do this after 8 players.

A followup CL will set preload=none for all JellyBean devices.

BUG=612909
TEST=manual, new tests.

Committed: https://crrev.com/d0ab2edb1cd3b57fbc48b3b39492a8d23221e648
Cr-Commit-Position: refs/heads/master@{#419375}
==========

[commit-bot: I haz the power, 2016-09-17 19:43:06]

Patchset 4 (id:??) landed as
https://crrev.com/d0ab2edb1cd3b57fbc48b3b39492a8d23221e648
Cr-Commit-Position: refs/heads/master@{#419375}