Throttle media decoding after excessive Android media server crashes

Throttle media decoding after excessive Android media server crashes

This change adds a watch dog player to listen to android MediaServer
crashes.
When the number of crashes reaches a certain threshold,
all media decoding using Android MediaPlayer/MediaCodec will be throttled.
This will throttle both media and webaudio implementation.

BUG=532745
Committed: https://crrev.com/b2341d25796fe6f94fb9a2d00104f2f88ca67a7c
Cr-Commit-Position: refs/heads/master@{#352740}

[qinmin, 2015-09-28 20:32:37]

Patchset #1 (id:1) has been deleted

[qinmin, 2015-09-28 20:38:54]

qinmin@chromium.org changed reviewers:
+ jochen@chromium.org, jschuh@chromium.org, rtoy@chromium.org,
tedchoc@chromium.org, timav@chromium.org

[qinmin, 2015-09-28 20:38:54]

PTAL

tedchoc@ for java/ and infobar related changes
jochen@ for content/ and  component/ OWNER
timav@ for media_codec_player
rtoy@ for webaudio related changes

[Raymond Toy, 2015-09-28 20:44:48]

lgtm for media/base/android/*

[qinmin, 2015-09-28 20:45:42]

Reviewers, please prioritize this code review as this bug has quite big impact
on our users.

[Raymond Toy, 2015-09-28 20:47:18]

On 2015/09/28 at 20:44:48, Raymond Toy wrote:
> lgtm for media/base/android/*

Oops. media/base/android/webaudio*

[DaleCurtis, 2015-09-28 21:13:23]

Why do you need to keep a watchdog player alive? Why not just have all
registered players notify some observer when they die with the specific
media_codec_died error message and then throttle startup?

Keeping a single player alive all the time seems unnecessary overhead.

[qinmin, 2015-09-28 21:20:57]

On 2015/09/28 21:13:23, DaleCurtis wrote:
> Why do you need to keep a watchdog player alive? Why not just have all
> registered players notify some observer when they die with the specific
> media_codec_died error message and then throttle startup?
> 
> Keeping a single player alive all the time seems unnecessary overhead.

Media codec crash is different from media server crash, and there is no way of
telling that a MediaCodec has crashed the media server on Kitkat/JB builds.
And for MediaMetadataExtractor, there is no way of knowing whether it will crash
the media server, unless we create a MediaPlayer and listen to MediaServer.

The overhead should be pretty small since the player only lives while there is
another player requesting resources. And considering the impact of this issue,
the little cost introduced here is justified i think.

[DaleCurtis, 2015-09-28 21:26:34]

I see, thanks for the explanation! Have you quantified just how much overhead
this actually adds in terms of memory / codec resources?

[qinmin, 2015-09-28 21:46:44]

On 2015/09/28 21:26:34, DaleCurtis wrote:
> I see, thanks for the explanation! Have you quantified just how much overhead
> this actually adds in terms of memory / codec resources?

adb dumpsys meminfo show less than 10k (sometimes it doesn't even change) memory
size change when creating the watch dog player and showing up the infobar.
And since this CL uses software wav decoder on an empty wav file, there is no
impact on decoder resources.

[Tima Vaisburd, 2015-09-28 22:09:12]

lgtm for media_codec_player with comments

https://codereview.chromium.org/1372203002/diff/20001/content/browser/media/a...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/1372203002/diff/20001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.cc:545:
MediaPlayerAndroid* player = GetPlayer(player_id);
Do you need to get the |player| here and at the beginning of StartInternal()
again?

https://codereview.chromium.org/1372203002/diff/20001/content/browser/media/a...
File content/browser/media/android/media_throttler.h (right):

https://codereview.chromium.org/1372203002/diff/20001/content/browser/media/a...
content/browser/media/android/media_throttler.h:27: bool RequestToDecodeData();
nit: the naming was a little bit confusing for me. I believe the method asks for
an instance of a decoder here. The MediaSource/Codec players use "decode data"
regarding one frame only. In other places you speak about "resource". Maybe call
it RequestDecoderResources or RequestDecodingSession?

https://codereview.chromium.org/1372203002/diff/20001/media/base/android/medi...
File media/base/android/media_codec_player.cc (right):

https://codereview.chromium.org/1372203002/diff/20001/media/base/android/medi...
media/base/android/media_codec_player.cc:301:
on_player_released_cb_.Run(player_id());
Did you consider posting this callback to UI thread? Is it important to run this
cb before the body of the Release()?

[Ted C, 2015-09-28 22:42:30]

Seems like the infobar code is missing?

https://codereview.chromium.org/1372203002/diff/20001/content/browser/media/a...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/1372203002/diff/20001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.cc:156: ||
!RequestDecoderResources(media_player_params.player_id, true)) {
why is this temporary?  maybe my question from the other file for further
clarification would help with this.

https://codereview.chromium.org/1372203002/diff/20001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.cc:675: // request
temporary permissions.
should there be an assert about that?

https://codereview.chromium.org/1372203002/diff/20001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.cc:723: return;
is there any cleanup to be done here?

https://codereview.chromium.org/1372203002/diff/20001/content/browser/media/a...
File content/browser/media/android/browser_media_player_manager.h (right):

https://codereview.chromium.org/1372203002/diff/20001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.h:166: // permitted,
or false otherwise. The manager object maintain an array
maintain[s]

https://codereview.chromium.org/1372203002/diff/20001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.h:167: // of active
MediaPlayerAndroid objects and release the inactive resources
release[s]

https://codereview.chromium.org/1372203002/diff/20001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.h:169: virtual bool
RequestDecoderResources(int player_id, bool temporary);
Can you describe what temporary means and why you should use it?

https://codereview.chromium.org/1372203002/diff/20001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.h:172: // to release
itself.
What is the difference between this and ReleasePlayer below?  Must this always
be called first?

They are very similarly named, so I'm thinking some further explanation would be
nice.  (for example, this sounds like it should be called from within
ReleasePlayer below for consumption by subclasses, but I don't know if that is
the actually true).

https://codereview.chromium.org/1372203002/diff/20001/content/browser/media/a...
File content/browser/media/android/media_throttler.cc (right):

https://codereview.chromium.org/1372203002/diff/20001/content/browser/media/a...
content/browser/media/android/media_throttler.cc:11: // static
I would add a blank line above this.

https://codereview.chromium.org/1372203002/diff/20001/content/browser/media/a...
content/browser/media/android/media_throttler.cc:21:
MediaThrottler::MediaThrottler() {
this should go at the bottom to match the header ordering.

https://codereview.chromium.org/1372203002/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/MediaResourceGetter.java
(right):

https://codereview.chromium.org/1372203002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/MediaResourceGetter.java:138:
if (true) return EMPTY_METADATA;
debug code left in?

https://codereview.chromium.org/1372203002/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/MediaThrottler.java
(right):

https://codereview.chromium.org/1372203002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/MediaThrottler.java:24:
private static final String TAG = "cr.MediaThrottler";
there was an update on chromium dev:
[chromium-dev] [Android] Changes to the log tag format

but it should now be cr_ instead of cr.

https://codereview.chromium.org/1372203002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/MediaThrottler.java:40:
private int mServerCrashCount = 0;
0 is the default, no need to specify it.

https://codereview.chromium.org/1372203002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/MediaThrottler.java:64:
public static MediaThrottler create(Context context) {
are these only called by native?  If so, they can all be private.

https://codereview.chromium.org/1372203002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/MediaThrottler.java:73:
/*
add another * to make it javadoc instead of a multiline comment (applies
elsewhere in this file)

https://codereview.chromium.org/1372203002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/MediaThrottler.java:84:
&& mServerCrashCount >= 4) {
make 4 a constant as well for the maximum allowed

https://codereview.chromium.org/1372203002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/MediaThrottler.java:130:
}.run();
this doesn't actually post it to a different thread or anything, so there is no
benefit for it to be inside a runnable at all.  Should just have the
synchronized block directly inside the function

https://codereview.chromium.org/1372203002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/MediaThrottler.java:172:
mLastCrashTime = -1;
UNKNOWN_LAST_SERVER_CRASH_TIME instead of -1?

https://codereview.chromium.org/1372203002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/MediaThrottler.java:180:
new Runnable() {
same comment as above

https://codereview.chromium.org/1372203002/diff/20001/content/public/browser/...
File content/public/browser/web_contents_delegate.h (right):

https://codereview.chromium.org/1372203002/diff/20001/content/public/browser/...
content/public/browser/web_contents_delegate.h:41: class MediaThrottler;
why is this needed?

https://codereview.chromium.org/1372203002/diff/20001/media/base/android/medi...
File media/base/android/media_codec_player.cc (right):

https://codereview.chromium.org/1372203002/diff/20001/media/base/android/medi...
media/base/android/media_codec_player.cc:301:
on_player_released_cb_.Run(player_id());
On 2015/09/28 22:09:12, Tima Vaisburd wrote:
> Did you consider posting this callback to UI thread? Is it important to run
this
> cb before the body of the Release()?

And should we be doing something in the else?

[Tima Vaisburd, 2015-09-28 23:04:47]

https://codereview.chromium.org/1372203002/diff/20001/media/base/android/medi...
File media/base/android/media_codec_player.cc (right):

https://codereview.chromium.org/1372203002/diff/20001/media/base/android/medi...
media/base/android/media_codec_player.cc:301:
on_player_released_cb_.Run(player_id());
On 2015/09/28 22:42:30, Ted C wrote:
> On 2015/09/28 22:09:12, Tima Vaisburd wrote:
> > Did you consider posting this callback to UI thread? Is it important to run
> this
> > cb before the body of the Release()?
> 
> And should we be doing something in the else?

In the 'else' we continue :-)

Each method with RUN_ON_MEDIA_THREAD is called twice. First on UI thread, on
which the macro re-posts the very same method onto the Media thread, and then on
the Media thread.

In this patch we report that the player is released on the first run, before the
resources are actually released (it will happen on the Media thread later).

[jochen (gone - plz use gerrit), 2015-09-29 07:14:29]

https://codereview.chromium.org/1372203002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_message_filter.cc (right):

https://codereview.chromium.org/1372203002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_message_filter.cc:512: if
(close(pcm_output.fd)) {
why not just post it to the blocking worker pool?

[qinmin, 2015-09-29 23:16:23]

added the missing media_throttle_infobar_delegate.h(cc) file

https://codereview.chromium.org/1372203002/diff/20001/content/browser/media/a...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/1372203002/diff/20001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.cc:156: ||
!RequestDecoderResources(media_player_params.player_id, true)) {
On 2015/09/28 22:42:29, Ted C wrote:
> why is this temporary?  maybe my question from the other file for further
> clarification would help with this.

This is because calling Initialize() will decode a small amount of data abd
should be considered a temporary thing.
added comments here

https://codereview.chromium.org/1372203002/diff/20001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.cc:545:
MediaPlayerAndroid* player = GetPlayer(player_id);
On 2015/09/28 22:09:12, Tima Vaisburd wrote:
> Do you need to get the |player| here and at the beginning of StartInternal()
> again?

Yes, both places.
Here we will show the infobar, but if the player is already removed, there is no
point to do that.
And StartInternal will recheck the |player| in case it is destroyed while
showing the infobar.

https://codereview.chromium.org/1372203002/diff/20001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.cc:675: // request
temporary permissions.
On 2015/09/28 22:42:29, Ted C wrote:
> should there be an assert about that?

Done.

https://codereview.chromium.org/1372203002/diff/20001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.cc:723: return;
On 2015/09/28 22:42:29, Ted C wrote:
> is there any cleanup to be done here?

not needed, the player has to acquire permission first before they can start
decoding. So if |granted| is false, they still haven't started decoding yet, and
there is nothing to release.

https://codereview.chromium.org/1372203002/diff/20001/content/browser/media/a...
File content/browser/media/android/browser_media_player_manager.h (right):

https://codereview.chromium.org/1372203002/diff/20001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.h:166: // permitted,
or false otherwise. The manager object maintain an array
On 2015/09/28 22:42:29, Ted C wrote:
> maintain[s]

Done.

https://codereview.chromium.org/1372203002/diff/20001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.h:167: // of active
MediaPlayerAndroid objects and release the inactive resources
On 2015/09/28 22:42:29, Ted C wrote:
> release[s]

Done.

https://codereview.chromium.org/1372203002/diff/20001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.h:169: virtual bool
RequestDecoderResources(int player_id, bool temporary);
On 2015/09/28 22:42:29, Ted C wrote:
> Can you describe what temporary means and why you should use it?

Done.

https://codereview.chromium.org/1372203002/diff/20001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.h:172: // to release
itself.
On 2015/09/28 22:42:29, Ted C wrote:
> What is the difference between this and ReleasePlayer below?  Must this always
> be called first?
> 
> They are very similarly named, so I'm thinking some further explanation would
be
> nice.  (for example, this sounds like it should be called from within
> ReleasePlayer below for consumption by subclasses, but I don't know if that is
> the actually true).

Renamed OnPlayerReleased to OnDecoderResourcesReleased, and added more comments
here.

https://codereview.chromium.org/1372203002/diff/20001/content/browser/media/a...
File content/browser/media/android/media_throttler.cc (right):

https://codereview.chromium.org/1372203002/diff/20001/content/browser/media/a...
content/browser/media/android/media_throttler.cc:11: // static
On 2015/09/28 22:42:29, Ted C wrote:
> I would add a blank line above this.

Done.

https://codereview.chromium.org/1372203002/diff/20001/content/browser/media/a...
content/browser/media/android/media_throttler.cc:21:
MediaThrottler::MediaThrottler() {
On 2015/09/28 22:42:29, Ted C wrote:
> this should go at the bottom to match the header ordering.

Done.

https://codereview.chromium.org/1372203002/diff/20001/content/browser/media/a...
File content/browser/media/android/media_throttler.h (right):

https://codereview.chromium.org/1372203002/diff/20001/content/browser/media/a...
content/browser/media/android/media_throttler.h:27: bool RequestToDecodeData();
On 2015/09/28 22:09:12, Tima Vaisburd wrote:
> nit: the naming was a little bit confusing for me. I believe the method asks
for
> an instance of a decoder here. The MediaSource/Codec players use "decode data"
> regarding one frame only. In other places you speak about "resource". Maybe
call
> it RequestDecoderResources or RequestDecodingSession?

Done.

https://codereview.chromium.org/1372203002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_message_filter.cc (right):

https://codereview.chromium.org/1372203002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_message_filter.cc:512: if
(close(pcm_output.fd)) {
On 2015/09/29 07:14:29, jochen wrote:
> why not just post it to the blocking worker pool?

Done.

https://codereview.chromium.org/1372203002/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/MediaResourceGetter.java
(right):

https://codereview.chromium.org/1372203002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/MediaResourceGetter.java:138:
if (true) return EMPTY_METADATA;
On 2015/09/28 22:42:29, Ted C wrote:
> debug code left in?

I thought I removed it, Done removing this.

https://codereview.chromium.org/1372203002/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/MediaThrottler.java
(right):

https://codereview.chromium.org/1372203002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/MediaThrottler.java:24:
private static final String TAG = "cr.MediaThrottler";
On 2015/09/28 22:42:30, Ted C wrote:
> there was an update on chromium dev:
> [chromium-dev] [Android] Changes to the log tag format
> 
> but it should now be cr_ instead of cr.

Done.

https://codereview.chromium.org/1372203002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/MediaThrottler.java:40:
private int mServerCrashCount = 0;
On 2015/09/28 22:42:30, Ted C wrote:
> 0 is the default, no need to specify it.

Done.

https://codereview.chromium.org/1372203002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/MediaThrottler.java:64:
public static MediaThrottler create(Context context) {
On 2015/09/28 22:42:30, Ted C wrote:
> are these only called by native?  If so, they can all be private.

Done.

https://codereview.chromium.org/1372203002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/MediaThrottler.java:73:
/*
On 2015/09/28 22:42:30, Ted C wrote:
> add another * to make it javadoc instead of a multiline comment (applies
> elsewhere in this file)

Done.

https://codereview.chromium.org/1372203002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/MediaThrottler.java:84:
&& mServerCrashCount >= 4) {
On 2015/09/28 22:42:30, Ted C wrote:
> make 4 a constant as well for the maximum allowed

Done.

https://codereview.chromium.org/1372203002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/MediaThrottler.java:130:
}.run();
On 2015/09/28 22:42:30, Ted C wrote:
> this doesn't actually post it to a different thread or anything, so there is
no
> benefit for it to be inside a runnable at all.  Should just have the
> synchronized block directly inside the function

oops, forget to post the runnable. Making it an AsyncTask instead, so that we
don't need a dedicated thread for this.

https://codereview.chromium.org/1372203002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/MediaThrottler.java:172:
mLastCrashTime = -1;
On 2015/09/28 22:42:30, Ted C wrote:
> UNKNOWN_LAST_SERVER_CRASH_TIME instead of -1?

Done.

https://codereview.chromium.org/1372203002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/MediaThrottler.java:180:
new Runnable() {
On 2015/09/28 22:42:30, Ted C wrote:
> same comment as above

use an async task to do the release() call in background.

https://codereview.chromium.org/1372203002/diff/20001/content/public/browser/...
File content/public/browser/web_contents_delegate.h (right):

https://codereview.chromium.org/1372203002/diff/20001/content/public/browser/...
content/public/browser/web_contents_delegate.h:41: class MediaThrottler;
On 2015/09/28 22:42:30, Ted C wrote:
> why is this needed?

removed

https://codereview.chromium.org/1372203002/diff/20001/media/base/android/medi...
File media/base/android/media_codec_player.cc (right):

https://codereview.chromium.org/1372203002/diff/20001/media/base/android/medi...
media/base/android/media_codec_player.cc:301:
on_player_released_cb_.Run(player_id());
On 2015/09/28 22:09:12, Tima Vaisburd wrote:
> Did you consider posting this callback to UI thread? Is it important to run
this
> cb before the body of the Release()?

The problem to post the task is that the MediaPlayerManager can go away when the
task is posted on the UI thread. As a result, the callback will not be executed
and the singleton MediaThrottler will not know that the player has been
released.

An alternative is to pass MediaThrottler::OnDecodeRequestFinished() to
MediaPlayerAndroid. However, in that case, the MediaPlayerManager won't know
that the decoder resources are released.

Added a todo there.

[jochen (gone - plz use gerrit), 2015-09-30 09:43:28]

components and content (except for stuff inside /android/ or /media/) lgtm with
comments

https://codereview.chromium.org/1372203002/diff/40001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1372203002/diff/40001/chrome/app/generated_re...
chrome/app/generated_resources.grd:13326: +          Android media server has
crashed several times. Do you still want to play this video?
i assume you got some UI person sign off on this text?

https://codereview.chromium.org/1372203002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_message_filter.cc (right):

https://codereview.chromium.org/1372203002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_message_filter.cc:91: << ": " <<
strerror(errno);
i bet the ": " on the previous line

https://codereview.chromium.org/1372203002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_message_filter.cc:523: LOG(ERROR) <<
"Cannot decode audio data due to throttling";
VLOG(1) maybe (here and above)?

[ctserng, 2015-09-30 17:41:10]

tserng@amazon.com changed reviewers:
+ tserng@amazon.com

[ctserng, 2015-09-30 17:41:11]

https://codereview.chromium.org/1372203002/diff/40001/content/browser/media/a...
File content/browser/media/android/browser_media_player_manager.h (right):

https://codereview.chromium.org/1372203002/diff/40001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.h:172: virtual bool
RequestDecoderResources(int player_id, bool temporary);
Can we make RequestDecoderResources protected so that it'll be possible to
implement a derived class that uses the same base resource manager.

https://codereview.chromium.org/1372203002/diff/40001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.h:178: virtual void
OnDecorderResourcesReleased(int player_id);
/s/Decorder/Decoder

Also can we make OnDecoderResourcesReleased() protected so that it'll be
possible to implement a derived class that uses the same base resource manager.

[qinmin, 2015-09-30 19:27:41]

qinmin@chromium.org changed reviewers:
+ cpu@chromium.org

[qinmin, 2015-09-30 19:27:42]

https://codereview.chromium.org/1372203002/diff/40001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1372203002/diff/40001/chrome/app/generated_re...
chrome/app/generated_resources.grd:13326: +          Android media server has
crashed several times. Do you still want to play this video?
On 2015/09/30 09:43:28, jochen wrote:
> i assume you got some UI person sign off on this text?

In the OWNERs file, the review rule is to TBR an owner if the CL is from a
google employee, added cpu@ to the reviewer in case he want to take a look.

https://codereview.chromium.org/1372203002/diff/40001/content/browser/media/a...
File content/browser/media/android/browser_media_player_manager.h (right):

https://codereview.chromium.org/1372203002/diff/40001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.h:172: virtual bool
RequestDecoderResources(int player_id, bool temporary);
On 2015/09/30 17:41:11, ctserng wrote:
> Can we make RequestDecoderResources protected so that it'll be possible to
> implement a derived class that uses the same base resource manager.

Done.

https://codereview.chromium.org/1372203002/diff/40001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.h:178: virtual void
OnDecorderResourcesReleased(int player_id);
On 2015/09/30 17:41:10, ctserng wrote:
> /s/Decorder/Decoder
> 
> Also can we make OnDecoderResourcesReleased() protected so that it'll be
> possible to implement a derived class that uses the same base resource
manager.

Done.

https://codereview.chromium.org/1372203002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_message_filter.cc (right):

https://codereview.chromium.org/1372203002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_message_filter.cc:91: << ": " <<
strerror(errno);
On 2015/09/30 09:43:28, jochen wrote:
> i bet the ": " on the previous line

Done.

https://codereview.chromium.org/1372203002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_message_filter.cc:523: LOG(ERROR) <<
"Cannot decode audio data due to throttling";
On 2015/09/30 09:43:28, jochen wrote:
> VLOG(1) maybe (here and above)?

Done.

[cpu_(ooo_6.6-7.5), 2015-09-30 22:04:57]

can you place a screenshot into the bug? I can find a UX person to look at that.

[qinmin, 2015-09-30 22:33:22]

On 2015/09/30 22:04:57, cpu wrote:
> can you place a screenshot into the bug? I can find a UX person to look at
that.

I have attached a screenshot into the crbug

[Raymond Toy, 2015-09-30 22:37:54]

https://chromiumcodereview.appspot.com/1372203002/diff/60001/content/public/a...
File
content/public/android/java/src/org/chromium/content/browser/MediaThrottler.java
(right):

https://chromiumcodereview.appspot.com/1372203002/diff/60001/content/public/a...
content/public/android/java/src/org/chromium/content/browser/MediaThrottler.java:49:
// Intervals between media server crashes that are considered normal.
Why are crashes considered "normal"?  And why 60 sec?  How did you come with
this number?  An explanation of how you got the magic 60 sec would be nice.

https://chromiumcodereview.appspot.com/1372203002/diff/60001/content/public/a...
content/public/android/java/src/org/chromium/content/browser/MediaThrottler.java:58:
private static final int SERVER_CRASH_COUNT_THRESHOLD_FOR_THROTTLING = 4;
An explanation on how you decided 4 was enough would be very helpful.

[qinmin, 2015-09-30 22:56:04]

https://chromiumcodereview.appspot.com/1372203002/diff/60001/content/public/a...
File
content/public/android/java/src/org/chromium/content/browser/MediaThrottler.java
(right):

https://chromiumcodereview.appspot.com/1372203002/diff/60001/content/public/a...
content/public/android/java/src/org/chromium/content/browser/MediaThrottler.java:49:
// Intervals between media server crashes that are considered normal.
On 2015/09/30 22:37:54, Raymond Toy wrote:
> Why are crashes considered "normal"?  And why 60 sec?  How did you come with
> this number?  An explanation of how you got the magic 60 sec would be nice.

This value is based on the discussion with the security team. We don't have
anything substantial to support the 60 seconds interval. However, since
restarting the media server takes about 5 seconds, so anything larger than 5
seconds should be reasonable. We can add some UMAs to study the interval between
2 decoder requests, but that can come in a later patch.

https://chromiumcodereview.appspot.com/1372203002/diff/60001/content/public/a...
content/public/android/java/src/org/chromium/content/browser/MediaThrottler.java:58:
private static final int SERVER_CRASH_COUNT_THRESHOLD_FOR_THROTTLING = 4;
On 2015/09/30 22:37:54, Raymond Toy wrote:
> An explanation on how you decided 4 was enough would be very helpful.

same as above. This value is just introduced to kick off the throttling, there
is nothing to back whether 4 is the optimal number. However, for ASLR attacks,
normally it will involve many more than 4 crashes.

[Ted C, 2015-10-01 02:19:57]

https://codereview.chromium.org/1372203002/diff/60001/chrome/browser/android/...
File chrome/browser/android/media/media_throttle_infobar_delegate.cc (right):

https://codereview.chromium.org/1372203002/diff/60001/chrome/browser/android/...
chrome/browser/android/media/media_throttle_infobar_delegate.cc:29:
old_infobar->delegate()->AsMediaThrottleInfoBarDelegate();
It's unfortunate to have to add AsMediaThrottleInfobarDelegate() to the base
delegate class just for this replacement case.

I wonder if this case would be better suited as a userdata on webcontents itself
that would just keep a ref to the infobar.  (added in the constructor, removed
in the destructor for this class).  With that said, I think this is a relatively
normal pattern, so I guess it's fine for this.

https://codereview.chromium.org/1372203002/diff/60001/chrome/browser/android/...
chrome/browser/android/media/media_throttle_infobar_delegate.cc:72: void
MediaThrottleInfoBarDelegate::InfoBarDismissed() {
Is this called if you close the tab?  Just wondering if we should run the
callback in the destructor as well to cover that case.

callback_helpers.h ResetAndReturn is nice if you want to run the callbacks and
be able to check whether they it is null in the destructor before running it
there.

https://codereview.chromium.org/1372203002/diff/60001/content/browser/media/a...
File content/browser/media/android/browser_media_player_manager.h (right):

https://codereview.chromium.org/1372203002/diff/60001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.h:154: // permitted,
or false otherwise. The manager object maintains an array
"an array" seems a bit too specific.  maybe a list or mapping to avoid the usage
of a actual datatype

https://codereview.chromium.org/1372203002/diff/60001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/MediaThrottler.java
(right):

https://codereview.chromium.org/1372203002/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/MediaThrottler.java:130:
mHandler.removeCallbacks(mDelayedReleaseRunnable);
What happens if ReleaseWatchDogTask had already been executed on a background
thread by mDelayedReleaseRunnable?  Is this supposed to prevent that code from
happening?

If you want to prevent that, you'd need to call
http://developer.android.com/reference/android/os/AsyncTask.html#cancel(boolean)
on the async task itself (and check isCancelled() before and after the lock)

https://codereview.chromium.org/1372203002/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/MediaThrottler.java:134:
new StartWatchDogTask().execute();
Why post the async task?  As opposed to the runnable, it will be done in the
background.  Why can't you just run it?  Do you want it to be the end of the UI
message queue?

https://codereview.chromium.org/1372203002/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/MediaThrottler.java:178:
long currentTime = SystemClock.elapsedRealtime();
do you need to lock here?  I guess you are already locked in both cases, so it
would be just defensive programming to have it.

Maybe we could add an assert for
http://developer.android.com/reference/java/lang/Thread.html#holdsLock(java.l...

https://codereview.chromium.org/1372203002/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/MediaThrottler.java:195:
mServerCrashCount = 0;
should you cancel mDelayedReleaseRunnable?

[qinmin, 2015-10-01 18:14:02]

https://codereview.chromium.org/1372203002/diff/60001/content/browser/media/a...
File content/browser/media/android/browser_media_player_manager.h (right):

https://codereview.chromium.org/1372203002/diff/60001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.h:154: // permitted,
or false otherwise. The manager object maintains an array
On 2015/10/01 02:19:57, Ted C wrote:
> "an array" seems a bit too specific.  maybe a list or mapping to avoid the
usage
> of a actual datatype

Done.

https://codereview.chromium.org/1372203002/diff/60001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/MediaThrottler.java
(right):

https://codereview.chromium.org/1372203002/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/MediaThrottler.java:130:
mHandler.removeCallbacks(mDelayedReleaseRunnable);
If ReleaseWatchDogTask had already been executed, that should be ok.
There are 3 scenarios on the ordering:
1. ReleaseWatchDogTask is posted to the background thread, but not yet executed.
  In this case, when ReleaseWatchDogTask runs, it will find the requestCount ==
1,and aborting early.
2. ReleaseWatchDogTask has already been executed on the backgroundd thread.
  In this case, the watch dog player is already released, and this function will
just recreate it here.
3. ReleaseWatchDogTask is still not yet executed.
  In this case, the task will be cancelled here, so the watch dog player will
not be released.

On 2015/10/01 02:19:57, Ted C wrote:
> What happens if ReleaseWatchDogTask had already been executed on a background
> thread by mDelayedReleaseRunnable?  Is this supposed to prevent that code from
> happening?
> 
> If you want to prevent that, you'd need to call
>
http://developer.android.com/reference/android/os/AsyncTask.html#cancel(boolean)
> on the async task itself (and check isCancelled() before and after the lock)

https://codereview.chromium.org/1372203002/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/MediaThrottler.java:134:
new StartWatchDogTask().execute();
On 2015/10/01 02:19:57, Ted C wrote:
> Why post the async task?  As opposed to the runnable, it will be done in the
> background.  Why can't you just run it?  Do you want it to be the end of the
UI
> message queue?
> 

Creating the watch dog player will need to initialize the mediaplayer, that
blocks the UI thread for a while. So moving it to an async task frees the UI
thread from such temporary hangs. Alternatively, we can create a dedicated loop
thread for this. But since most of the time the looper thread will be idle, so
having it doesn't justify the cost.

https://codereview.chromium.org/1372203002/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/MediaThrottler.java:178:
long currentTime = SystemClock.elapsedRealtime();
On 2015/10/01 02:19:57, Ted C wrote:
> do you need to lock here?  I guess you are already locked in both cases, so it
> would be just defensive programming to have it.
> 
> Maybe we could add an assert for
>
http://developer.android.com/reference/java/lang/Thread.html#holdsLock(java.l...

Done.

https://codereview.chromium.org/1372203002/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/MediaThrottler.java:195:
mServerCrashCount = 0;
On 2015/10/01 02:19:57, Ted C wrote:
> should you cancel mDelayedReleaseRunnable?

No need. If mDelayedReleaseRunnable is already scheduled or posted on the
background thread while this is called, we should let it run so that the watch
dog player will get released.

[Raymond Toy, 2015-10-01 21:58:21]

https://codereview.chromium.org/1372203002/diff/60001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/MediaThrottler.java
(right):

https://codereview.chromium.org/1372203002/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/MediaThrottler.java:49:
// Intervals between media server crashes that are considered normal.
On 2015/09/30 at 22:56:04, qinmin wrote:
> On 2015/09/30 22:37:54, Raymond Toy wrote:
> > Why are crashes considered "normal"?  And why 60 sec?  How did you come with
> > this number?  An explanation of how you got the magic 60 sec would be nice.
> 
> This value is based on the discussion with the security team. We don't have
anything substantial to support the 60 seconds interval. However, since
restarting the media server takes about 5 seconds, so anything larger than 5
seconds should be reasonable. We can add some UMAs to study the interval between
2 decoder requests, but that can come in a later patch.

I think just saying anything larger than 5 sec or so is a reasonable value.  But
I'm not going to require you to say anything. I just hate random constants that
don't give a hint on how they were chosen.

https://codereview.chromium.org/1372203002/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/MediaThrottler.java:58:
private static final int SERVER_CRASH_COUNT_THRESHOLD_FOR_THROTTLING = 4;
On 2015/09/30 at 22:56:04, qinmin wrote:
> On 2015/09/30 22:37:54, Raymond Toy wrote:
> > An explanation on how you decided 4 was enough would be very helpful.
> 
> same as above. This value is just introduced to kick off the throttling, there
is nothing to back whether 4 is the optimal number. However, for ASLR attacks,
normally it will involve many more than 4 crashes.

As above, just a short comment saying what you just said here would satisfy me.

But I'm not blocking the CL on this.

[qinmin, 2015-10-01 22:53:34]

https://chromiumcodereview.appspot.com/1372203002/diff/60001/content/public/a...
File
content/public/android/java/src/org/chromium/content/browser/MediaThrottler.java
(right):

https://chromiumcodereview.appspot.com/1372203002/diff/60001/content/public/a...
content/public/android/java/src/org/chromium/content/browser/MediaThrottler.java:49:
// Intervals between media server crashes that are considered normal.
On 2015/10/01 21:58:20, Raymond Toy wrote:
> On 2015/09/30 at 22:56:04, qinmin wrote:
> > On 2015/09/30 22:37:54, Raymond Toy wrote:
> > > Why are crashes considered "normal"?  And why 60 sec?  How did you come
with
> > > this number?  An explanation of how you got the magic 60 sec would be
nice.
> > 
> > This value is based on the discussion with the security team. We don't have
> anything substantial to support the 60 seconds interval. However, since
> restarting the media server takes about 5 seconds, so anything larger than 5
> seconds should be reasonable. We can add some UMAs to study the interval
between
> 2 decoder requests, but that can come in a later patch.
> 
> I think just saying anything larger than 5 sec or so is a reasonable value. 
But
> I'm not going to require you to say anything. I just hate random constants
that
> don't give a hint on how they were chosen.

Done.

https://chromiumcodereview.appspot.com/1372203002/diff/60001/content/public/a...
content/public/android/java/src/org/chromium/content/browser/MediaThrottler.java:58:
private static final int SERVER_CRASH_COUNT_THRESHOLD_FOR_THROTTLING = 4;
On 2015/10/01 21:58:20, Raymond Toy wrote:
> On 2015/09/30 at 22:56:04, qinmin wrote:
> > On 2015/09/30 22:37:54, Raymond Toy wrote:
> > > An explanation on how you decided 4 was enough would be very helpful.
> > 
> > same as above. This value is just introduced to kick off the throttling,
there
> is nothing to back whether 4 is the optimal number. However, for ASLR attacks,
> normally it will involve many more than 4 crashes.
> 
> As above, just a short comment saying what you just said here would satisfy
me.
> 
> But I'm not blocking the CL on this.

Done.

[Ted C, 2015-10-02 17:23:38]

https://codereview.chromium.org/1372203002/diff/60001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/MediaThrottler.java
(right):

https://codereview.chromium.org/1372203002/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/MediaThrottler.java:134:
new StartWatchDogTask().execute();
On 2015/10/01 18:14:02, qinmin wrote:
> On 2015/10/01 02:19:57, Ted C wrote:
> > Why post the async task?  As opposed to the runnable, it will be done in the
> > background.  Why can't you just run it?  Do you want it to be the end of the
> UI
> > message queue?
> > 
> 
> Creating the watch dog player will need to initialize the mediaplayer, that
> blocks the UI thread for a while. So moving it to an async task frees the UI
> thread from such temporary hangs. Alternatively, we can create a dedicated
loop
> thread for this. But since most of the time the looper thread will be idle, so
> having it doesn't justify the cost.

Not blocking the UI thread is good, so an AsyncTask makes sense.  But still
doesn't explain why you need to post the task.  new
ReleaseWatchDogTask().execute();
will have it run in a background thread already.

So posting of the runnable just puts it to the back of the UI message queue,
and I'm not sure why that is needed.

[qinmin, 2015-10-02 17:31:55]

https://codereview.chromium.org/1372203002/diff/60001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/MediaThrottler.java
(right):

https://codereview.chromium.org/1372203002/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/MediaThrottler.java:134:
new StartWatchDogTask().execute();
On 2015/10/02 17:23:38, Ted C wrote:
> On 2015/10/01 18:14:02, qinmin wrote:
> > On 2015/10/01 02:19:57, Ted C wrote:
> > > Why post the async task?  As opposed to the runnable, it will be done in
the
> > > background.  Why can't you just run it?  Do you want it to be the end of
the
> > UI
> > > message queue?
> > > 
> > 
> > Creating the watch dog player will need to initialize the mediaplayer, that
> > blocks the UI thread for a while. So moving it to an async task frees the UI
> > thread from such temporary hangs. Alternatively, we can create a dedicated
> loop
> > thread for this. But since most of the time the looper thread will be idle,
so
> > having it doesn't justify the cost.
> 
> Not blocking the UI thread is good, so an AsyncTask makes sense.  But still
> doesn't explain why you need to post the task.  new
> ReleaseWatchDogTask().execute();
> will have it run in a background thread already.
> 
> So posting of the runnable just puts it to the back of the UI message queue,
> and I'm not sure why that is needed.

the issue is that async task has to be executed on the UI thread, and
requestDecoderResources can be called on any thread.

[Ted C, 2015-10-02 18:34:20]

lgtm

https://codereview.chromium.org/1372203002/diff/60001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/MediaThrottler.java
(right):

https://codereview.chromium.org/1372203002/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/MediaThrottler.java:134:
new StartWatchDogTask().execute();
On 2015/10/02 17:31:55, qinmin wrote:
> On 2015/10/02 17:23:38, Ted C wrote:
> > On 2015/10/01 18:14:02, qinmin wrote:
> > > On 2015/10/01 02:19:57, Ted C wrote:
> > > > Why post the async task?  As opposed to the runnable, it will be done in
> the
> > > > background.  Why can't you just run it?  Do you want it to be the end of
> the
> > > UI
> > > > message queue?
> > > > 
> > > 
> > > Creating the watch dog player will need to initialize the mediaplayer,
that
> > > blocks the UI thread for a while. So moving it to an async task frees the
UI
> > > thread from such temporary hangs. Alternatively, we can create a dedicated
> > loop
> > > thread for this. But since most of the time the looper thread will be
idle,
> so
> > > having it doesn't justify the cost.
> > 
> > Not blocking the UI thread is good, so an AsyncTask makes sense.  But still
> > doesn't explain why you need to post the task.  new
> > ReleaseWatchDogTask().execute();
> > will have it run in a background thread already.
> > 
> > So posting of the runnable just puts it to the back of the UI message queue,
> > and I'm not sure why that is needed.
> 
> the issue is that async task has to be executed on the UI thread, and
> requestDecoderResources can be called on any thread.

Ah ha!  Gotcha!

[cpu_(ooo_6.6-7.5), 2015-10-02 19:24:26]

there does not seem to be blissful agreement on the bug?

[qinmin, 2015-10-02 21:07:21]

On 2015/10/02 19:24:26, cpu wrote:
> there does not seem to be blissful agreement on the bug?

The infobar approach is a reasonable solution, otherwise user won't know why
media playback fails. 
If we really want to disable playback silently, that can be a separate CL later.

[qinmin, 2015-10-02 23:11:32]

On 2015/10/02 21:07:21, qinmin wrote:
> On 2015/10/02 19:24:26, cpu wrote:
> > there does not seem to be blissful agreement on the bug?
> 
> The infobar approach is a reasonable solution, otherwise user won't know why
> media playback fails. 
> If we really want to disable playback silently, that can be a separate CL
later.

chatted with felt@, and she is on board with the infobar approach. Adding @rolfe
on the crbug for UI review,

[qinmin, 2015-10-06 21:45:26]

@cpu, PTAL again. I have modified the infobar message and button labels
according to suggestions from the UI review.

[cpu_(ooo_6.6-7.5), 2015-10-06 21:48:29]

Still I don't see visible consensus on the bug. Afaik the bug is in the same
place as 5 days ago.

[jschuh, 2015-10-06 21:51:48]

On 2015/10/06 21:48:29, cpu wrote:
> Still I don't see visible consensus on the bug. Afaik the bug is in the same
> place as 5 days ago.

There were a few threads. It's either consensus or concession, but we've reached
agreement.

[qinmin, 2015-10-06 22:00:58]

On 2015/10/06 21:51:48, jschuh (very slow) wrote:
> On 2015/10/06 21:48:29, cpu wrote:
> > Still I don't see visible consensus on the bug. Afaik the bug is in the same
> > place as 5 days ago.
> 
> There were a few threads. It's either consensus or concession, but we've
reached
> agreement.

@cpu, please see the ui review thread here:
https://groups.google.com/a/google.com/forum/#!topic/chrome-ui-review/SLZnYEd...

[cpu_(ooo_6.6-7.5), 2015-10-06 22:27:23]

lgtm

Note the petition for UMA data. One the change lands wait a week and post UMA
results back in thread. Maybe two weeks.

[qinmin, 2015-10-06 22:37:22]

The CQ bit was checked by qinmin@chromium.org

[qinmin, 2015-10-06 22:37:22]

The patchset sent to the CQ was uploaded after l-g-t-m from timav@chromium.org,
rtoy@chromium.org, jochen@chromium.org, tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/1372203002/#ps130001
(title: "new infobar text per UI review")

[qinmin, 2015-10-06 22:37:28]

The CQ bit was unchecked by qinmin@chromium.org

[qinmin, 2015-10-06 22:37:33]

The CQ bit was checked by qinmin@chromium.org

[commit-bot: I haz the power, 2015-10-06 22:38:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1372203002/130001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1372203002/130001

[commit-bot: I haz the power, 2015-10-07 01:42:00]

Committed patchset #7 (id:130001)

[commit-bot: I haz the power, 2015-10-07 01:43:02]

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