Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2471903002:
Add MediaServiceThrottler (Closed)
DescriptionAdd MediaServiceThrottler
WMPA used the MediaThrottler to display a notification bar to the user
if an unusual number of media server crashes were detected. This was
done in order to prevent a webpage from loading the same url over and
over in order to crash the Media Server.
This change introduces the MediaServerCrashListener which reuses the
same technique that the MediaThrottler used to monitor for Media Server
crashes.
It also introduces the MediaServiceThrottler which uses the
MediaServerCrashListener, and exponentially delays the creation of new
clients (MediaPlayerBridges for now) based on the number of Media
Server crashes.
The MediaServiceThrottler resets its internal clock state after a
minute of not having schedule new client creations, and reset its
crash counter after a minute of not having detected any crashes. It
also support "burst scheduling", allowing for a fixed number of clients
to be scheduled immediately within some sliding window, in order to
minimize impact from multiple requests coming in from new page loads.
BUG=636615
TEST=Added UTs
Committed: https://crrev.com/f524383bbd03acfba4ffe18b7e77a66c46228dcd
Cr-Commit-Position: refs/heads/master@{#432712}
Patch Set 1 #Patch Set 2 : Added comments and UTs #Patch Set 3 : Integrate throttler into MediaPlayerRender #
Total comments: 55
Patch Set 4 : Release resources after 1 min. +2 UTS. #
Total comments: 43
Patch Set 5 : Addressing comments #Patch Set 6 : Release watchdog MediaPlayer on crash #Patch Set 7 : Limit crashes from watchdog creation failures #
Total comments: 8
Patch Set 8 : Adressed comments #Messages
Total messages: 41 (19 generated)
Description was changed from ========== [Rough draft] Add MediaServiceThrottler // TODO(tguilbert): Description BUG= ========== to ========== Add MediaServiceThrottler WMPA used the MediaThrottler to display a notification bar to the user if an unusual number of media server crashes were detected. This was done in order to prevent a webpage from loading the same url over and over in order to crash the Media Server. This change introduces the MediaServerCrashListener which reuses the same technique that the MediaThrottler used to monitor for Media Server crashes. It also introduces the MediaServiceThrottler which uses the MediaServerCrashListener, and exponentially delays the creation of new clients (MediaPlayerBridges for now) based on the number of Media Server crashes. The MediaServiceThrottler resets its internal clock state after a minute of not having schedule new client creations, and reset its crash counter after a minute of not having detected any crashes. It also support "burst scheduling", allowing for a fixed number of clients to be scheduled immediately within some sliding window, in order to minimize impact from multiple requests coming in from new page loads. BUG=636615 TEST=Added UTs ==========
The CQ bit was checked by tguilbert@chromium.org to run a CQ dry run
tguilbert@chromium.org changed reviewers: + dalecurtis@google.com, sandersd@chromium.org
PTAL! Thank you :)
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/11/04 22:07:38, tguilbert wrote: > PTAL! Thank you :) (I will be adding the integration into MediaPlayerRenderer to this review shortly).
dalecurtis@chromium.org changed reviewers: + dalecurtis@chromium.org - dalecurtis@google.com
The old throttler shut down when there were no media player instances outstanding. Do we want do something similar here? I wonder if there might be power concerns.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/11/04 22:15:56, DaleCurtis wrote: > The old throttler shut down when there were no media player instances > outstanding. Do we want do something similar here? I wonder if there might be > power concerns. I was trying to simplify the way we reason about this code. I am not sure what the power implications are (if there are any).
https://codereview.chromium.org/2471903002/diff/40001/content/browser/media/a... File content/browser/media/android/media_player_renderer.cc (right): https://codereview.chromium.org/2471903002/diff/40001/content/browser/media/a... content/browser/media/android/media_player_renderer.cc:47: BrowserThread::PostDelayedTask( Worth skipping post if it's schedule == 0? https://codereview.chromium.org/2471903002/diff/40001/media/base/android/java... File media/base/android/java/src/org/chromium/media/MediaServerCrashListener.java (right): https://codereview.chromium.org/2471903002/diff/40001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaServerCrashListener.java:19: @JNINamespace("media") Need any @MainDex tagging? https://codereview.chromium.org/2471903002/diff/40001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaServerCrashListener.java:21: private static final String TAG = "cr_MediaCrashListen"; Listener? https://codereview.chromium.org/2471903002/diff/40001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaServerCrashListener.java:43: if (mPlayer != null) return true; Does if (mPlayer) work? https://codereview.chromium.org/2471903002/diff/40001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaServerCrashListener.java:47: } catch (IllegalStateException e) { (IllegalStateException | RuntimeException e) https://codereview.chromium.org/2471903002/diff/40001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaServerCrashListener.java:53: if (mPlayer == null) { If (!mPlayer) or flip so if (mPlayer) ? Also don't use else {} with blocks including return. https://codereview.chromium.org/2471903002/diff/40001/media/base/android/medi... File media/base/android/media_server_crash_listener.cc (right): https://codereview.chromium.org/2471903002/diff/40001/media/base/android/medi... media/base/android/media_server_crash_listener.cc:16: return base::Singleton< Typically we use LazyInstance instead of a base::Singleton. I think it's probably worth shutting this down when there haven't been any active listeners for > x seconds. https://codereview.chromium.org/2471903002/diff/40001/media/base/android/medi... media/base/android/media_server_crash_listener.cc:30: scoped_refptr<base::SingleThreadTaskRunner> callback_thread) { Unless you're using std::move() this should probably still be const&. I forget the latest rules though. https://codereview.chromium.org/2471903002/diff/40001/media/base/android/medi... File media/base/android/media_service_throttler.cc (right): https://codereview.chromium.org/2471903002/diff/40001/media/base/android/medi... media/base/android/media_service_throttler.cc:16: const base::TimeDelta kTimeUntilCrashReset = base::TimeDelta::FromMinutes(1); constexpr on all base::TimeDelta? Otherwise I think all of these will add static-initializers. https://codereview.chromium.org/2471903002/diff/40001/media/base/android/medi... media/base/android/media_service_throttler.cc:72: return base::Singleton< Hmm, why do both classes need a singleton? Seems one of them should be able to own the other? Also, LazyInstance. If possible it'd be great if LazyInstance is avoidable and instead the lifetime is bound to some existing object. https://codereview.chromium.org/2471903002/diff/40001/media/base/android/medi... media/base/android/media_service_throttler.cc:107: base::TimeDelta MediaServiceThrottler::ScheduleClientCreation() { I'd rename this to GetCreationDelay() or something similar. You're not really scheduling anything. https://codereview.chromium.org/2471903002/diff/40001/media/base/android/medi... media/base/android/media_service_throttler.cc:116: if (now > next_schedulable_slot_ || {} for multi-line if. https://codereview.chromium.org/2471903002/diff/40001/media/base/android/medi... media/base/android/media_service_throttler.cc:144: uint number_crashes = std::floor(current_crashes_); No uint. Specify an actual type. https://codereview.chromium.org/2471903002/diff/40001/media/base/android/medi... media/base/android/media_service_throttler.cc:145: DCHECK(number_crashes >= 0); DCHECK_GE https://codereview.chromium.org/2471903002/diff/40001/media/base/android/medi... media/base/android/media_service_throttler.cc:149: number_crashes = std::min(number_crashes, (uint)10); No c style cast. static_cast<> if you need. This should be fine as 10u -- might want to extract that to a constant. https://codereview.chromium.org/2471903002/diff/40001/media/base/android/medi... media/base/android/media_service_throttler.cc:168: current_crashes_ = 0.0; Just 0 is fine. https://codereview.chromium.org/2471903002/diff/40001/media/base/android/medi... media/base/android/media_service_throttler.cc:179: void MediaServiceThrottler::EnsureCrashListenerStarted() { Seems is_media_server_crash_listener_running_ is kind of unnecessary, just trigger a StartListening() or similar call during ScheduleClientCreation() (and queue a shutdown task probably). https://codereview.chromium.org/2471903002/diff/40001/media/base/android/medi... File media/base/android/media_service_throttler.h (right): https://codereview.chromium.org/2471903002/diff/40001/media/base/android/medi... media/base/android/media_service_throttler.h:18: class MediaServiceThrottler { MEDIA_EXPORT?
https://codereview.chromium.org/2471903002/diff/40001/content/browser/media/a... File content/browser/media/android/media_player_renderer.cc (right): https://codereview.chromium.org/2471903002/diff/40001/content/browser/media/a... content/browser/media/android/media_player_renderer.cc:54: void MediaPlayerRenderer::InitializeInternal( Rename to make the purpose more clear. Load() may be reasonable in this case? https://codereview.chromium.org/2471903002/diff/40001/content/browser/media/a... content/browser/media/android/media_player_renderer.cc:61: DLOG(ERROR) << "DemuxerStreamProvider is not of Type URL"; Should this stay in Initialize()? https://codereview.chromium.org/2471903002/diff/40001/media/base/android/medi... File media/base/android/media_service_throttler.cc (right): https://codereview.chromium.org/2471903002/diff/40001/media/base/android/medi... media/base/android/media_service_throttler.cc:16: const base::TimeDelta kTimeUntilCrashReset = base::TimeDelta::FromMinutes(1); These should probably all be in an anonymous namespace. https://codereview.chromium.org/2471903002/diff/40001/media/base/android/medi... media/base/android/media_service_throttler.cc:24: const uint kCrashDecayRateInMs = 60000; This is a period (as opposed to a rate). https://codereview.chromium.org/2471903002/diff/40001/media/base/android/medi... media/base/android/media_service_throttler.cc:29: // crashes still result in this delay being added once. An alternative would be to use |n ? 2^(n-1) : 0|. It matches intuition a little better, but whether that's overall simpler is for you to decide. https://codereview.chromium.org/2471903002/diff/40001/media/base/android/medi... media/base/android/media_service_throttler.cc:37: base::TimeDelta::FromMilliseconds(80); Also a period ("delay" and other synonyms are fine too). https://codereview.chromium.org/2471903002/diff/40001/media/base/android/medi... File media/base/android/media_service_throttler.h (right): https://codereview.chromium.org/2471903002/diff/40001/media/base/android/medi... media/base/android/media_service_throttler.h:24: virtual ~MediaServiceThrottler(); Can the destructor be made protected? If you need it just for unit tests then a subclass that redefines it as public would work for that. https://codereview.chromium.org/2471903002/diff/40001/media/base/android/medi... media/base/android/media_service_throttler.h:27: base::TimeDelta ScheduleClientCreation(); This is misleading because the scheduling is only partial (a time is assigned but nothing is actually configured to run at that time). I can't think of a more clear name or a better factoring of the concerns, so probably just a more detailed comment would be good. https://codereview.chromium.org/2471903002/diff/40001/media/base/android/medi... media/base/android/media_service_throttler.h:30: void OnMediaServerCrash(); Should be protected or private. https://codereview.chromium.org/2471903002/diff/40001/media/base/android/medi... media/base/android/media_service_throttler.h:66: base::TimeTicks last_update_time_; Nit: add 'crash' to variable name for clarity.
Addressed comments and added 2 UTs. PTAL, thank you! https://codereview.chromium.org/2471903002/diff/40001/content/browser/media/a... File content/browser/media/android/media_player_renderer.cc (right): https://codereview.chromium.org/2471903002/diff/40001/content/browser/media/a... content/browser/media/android/media_player_renderer.cc:47: BrowserThread::PostDelayedTask( On 2016/11/08 22:35:40, DaleCurtis wrote: > Worth skipping post if it's schedule == 0? Done. https://codereview.chromium.org/2471903002/diff/40001/content/browser/media/a... content/browser/media/android/media_player_renderer.cc:54: void MediaPlayerRenderer::InitializeInternal( On 2016/11/08 22:53:44, sandersd wrote: > Rename to make the purpose more clear. Load() may be reasonable in this case? I went with an (overly?) explicit name... LMK if you think there is a better way to say this. https://codereview.chromium.org/2471903002/diff/40001/content/browser/media/a... content/browser/media/android/media_player_renderer.cc:61: DLOG(ERROR) << "DemuxerStreamProvider is not of Type URL"; On 2016/11/08 22:53:44, sandersd wrote: > Should this stay in Initialize()? Done. https://codereview.chromium.org/2471903002/diff/40001/media/base/android/java... File media/base/android/java/src/org/chromium/media/MediaServerCrashListener.java (right): https://codereview.chromium.org/2471903002/diff/40001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaServerCrashListener.java:19: @JNINamespace("media") On 2016/11/08 22:35:41, DaleCurtis wrote: > Need any @MainDex tagging? Probably, since it might be used in the GPU process. Added. https://codereview.chromium.org/2471903002/diff/40001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaServerCrashListener.java:21: private static final String TAG = "cr_MediaCrashListen"; On 2016/11/08 22:35:40, DaleCurtis wrote: > Listener? Oops, fixed. Had to drop the '_' to have it fit in the 20 char limit. Does this violate any style guides? https://codereview.chromium.org/2471903002/diff/40001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaServerCrashListener.java:43: if (mPlayer != null) return true; On 2016/11/08 22:35:40, DaleCurtis wrote: > Does if (mPlayer) work? No : error message is "MediaPlayer cannot be converted to boolean". https://codereview.chromium.org/2471903002/diff/40001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaServerCrashListener.java:47: } catch (IllegalStateException e) { On 2016/11/08 22:35:40, DaleCurtis wrote: > (IllegalStateException | RuntimeException e) I get this error message: error: Alternatives in a multi-catch statement cannot be related by subclassing We would loose the contents of the exception, because e would only be of type according to http://stackoverflow.com/a/19470374 https://codereview.chromium.org/2471903002/diff/40001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaServerCrashListener.java:53: if (mPlayer == null) { On 2016/11/08 22:35:40, DaleCurtis wrote: > If (!mPlayer) or flip so if (mPlayer) ? Also don't use else {} with blocks > including return. Done. https://codereview.chromium.org/2471903002/diff/40001/media/base/android/medi... File media/base/android/media_server_crash_listener.cc (right): https://codereview.chromium.org/2471903002/diff/40001/media/base/android/medi... media/base/android/media_server_crash_listener.cc:16: return base::Singleton< On 2016/11/08 22:35:41, DaleCurtis wrote: > Typically we use LazyInstance instead of a base::Singleton. I think it's > probably worth shutting this down when there haven't been any active listeners > for > x seconds. Changed to have the lifetime managed by the MediaServiceThrottler. https://codereview.chromium.org/2471903002/diff/40001/media/base/android/medi... media/base/android/media_server_crash_listener.cc:30: scoped_refptr<base::SingleThreadTaskRunner> callback_thread) { On 2016/11/08 22:35:41, DaleCurtis wrote: > Unless you're using std::move() this should probably still be const&. I forget > the latest rules though. By value or std::move() is the latest. https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... https://codereview.chromium.org/2471903002/diff/40001/media/base/android/medi... File media/base/android/media_service_throttler.cc (right): https://codereview.chromium.org/2471903002/diff/40001/media/base/android/medi... media/base/android/media_service_throttler.cc:16: const base::TimeDelta kTimeUntilCrashReset = base::TimeDelta::FromMinutes(1); On 2016/11/08 22:53:44, sandersd wrote: > These should probably all be in an anonymous namespace. Done. https://codereview.chromium.org/2471903002/diff/40001/media/base/android/medi... media/base/android/media_service_throttler.cc:24: const uint kCrashDecayRateInMs = 60000; On 2016/11/08 22:53:44, sandersd wrote: > This is a period (as opposed to a rate). Done. https://codereview.chromium.org/2471903002/diff/40001/media/base/android/medi... media/base/android/media_service_throttler.cc:29: // crashes still result in this delay being added once. On 2016/11/08 22:53:44, sandersd wrote: > An alternative would be to use |n ? 2^(n-1) : 0|. It matches intuition a little > better, but whether that's overall simpler is for you to decide. That was how I had originally written it, but I found the code to look somehat cleaner this way (even though I agree that there is a tradeoff). I might revisit this decision before the end of the review. https://codereview.chromium.org/2471903002/diff/40001/media/base/android/medi... media/base/android/media_service_throttler.cc:37: base::TimeDelta::FromMilliseconds(80); On 2016/11/08 22:53:44, sandersd wrote: > Also a period ("delay" and other synonyms are fine too). Done. https://codereview.chromium.org/2471903002/diff/40001/media/base/android/medi... media/base/android/media_service_throttler.cc:72: return base::Singleton< On 2016/11/08 22:35:41, DaleCurtis wrote: > Hmm, why do both classes need a singleton? Seems one of them should be able to > own the other? Also, LazyInstance. If possible it'd be great if LazyInstance is > avoidable and instead the lifetime is bound to some existing object. Updated. The MediaServiceThrottler is now a lazy instance. The MediaServiceCrashListener is now managed by the MediaServiceThrottler. https://codereview.chromium.org/2471903002/diff/40001/media/base/android/medi... media/base/android/media_service_throttler.cc:107: base::TimeDelta MediaServiceThrottler::ScheduleClientCreation() { On 2016/11/08 22:35:41, DaleCurtis wrote: > I'd rename this to GetCreationDelay() or something similar. You're not really > scheduling anything. Done. https://codereview.chromium.org/2471903002/diff/40001/media/base/android/medi... media/base/android/media_service_throttler.cc:116: if (now > next_schedulable_slot_ || On 2016/11/08 22:35:41, DaleCurtis wrote: > {} for multi-line if. Done. https://codereview.chromium.org/2471903002/diff/40001/media/base/android/medi... media/base/android/media_service_throttler.cc:144: uint number_crashes = std::floor(current_crashes_); On 2016/11/08 22:35:41, DaleCurtis wrote: > No uint. Specify an actual type. Done. https://codereview.chromium.org/2471903002/diff/40001/media/base/android/medi... media/base/android/media_service_throttler.cc:145: DCHECK(number_crashes >= 0); On 2016/11/08 22:35:41, DaleCurtis wrote: > DCHECK_GE Done. https://codereview.chromium.org/2471903002/diff/40001/media/base/android/medi... media/base/android/media_service_throttler.cc:149: number_crashes = std::min(number_crashes, (uint)10); On 2016/11/08 22:35:41, DaleCurtis wrote: > No c style cast. static_cast<> if you need. This should be fine as 10u -- might > want to extract that to a constant. Done. https://codereview.chromium.org/2471903002/diff/40001/media/base/android/medi... media/base/android/media_service_throttler.cc:168: current_crashes_ = 0.0; On 2016/11/08 22:35:41, DaleCurtis wrote: > Just 0 is fine. Done. https://codereview.chromium.org/2471903002/diff/40001/media/base/android/medi... media/base/android/media_service_throttler.cc:179: void MediaServiceThrottler::EnsureCrashListenerStarted() { On 2016/11/08 22:35:41, DaleCurtis wrote: > Seems is_media_server_crash_listener_running_ is kind of unnecessary, just > trigger a StartListening() or similar call during ScheduleClientCreation() (and > queue a shutdown task probably). Updated. https://codereview.chromium.org/2471903002/diff/40001/media/base/android/medi... File media/base/android/media_service_throttler.h (right): https://codereview.chromium.org/2471903002/diff/40001/media/base/android/medi... media/base/android/media_service_throttler.h:18: class MediaServiceThrottler { On 2016/11/08 22:35:41, DaleCurtis wrote: > MEDIA_EXPORT? Done. https://codereview.chromium.org/2471903002/diff/40001/media/base/android/medi... media/base/android/media_service_throttler.h:24: virtual ~MediaServiceThrottler(); On 2016/11/08 22:53:44, sandersd wrote: > Can the destructor be made protected? If you need it just for unit tests then a > subclass that redefines it as public would work for that. Done. https://codereview.chromium.org/2471903002/diff/40001/media/base/android/medi... media/base/android/media_service_throttler.h:27: base::TimeDelta ScheduleClientCreation(); On 2016/11/08 22:53:44, sandersd wrote: > This is misleading because the scheduling is only partial (a time is assigned > but nothing is actually configured to run at that time). I can't think of a more > clear name or a better factoring of the concerns, so probably just a more > detailed comment would be good. Updated. https://codereview.chromium.org/2471903002/diff/40001/media/base/android/medi... media/base/android/media_service_throttler.h:30: void OnMediaServerCrash(); On 2016/11/08 22:53:44, sandersd wrote: > Should be protected or private. Done. https://codereview.chromium.org/2471903002/diff/40001/media/base/android/medi... media/base/android/media_service_throttler.h:66: base::TimeTicks last_update_time_; On 2016/11/08 22:53:44, sandersd wrote: > Nit: add 'crash' to variable name for clarity. Done. https://codereview.chromium.org/2471903002/diff/60001/media/base/android/medi... File media/base/android/media_service_throttler.h (right): https://codereview.chromium.org/2471903002/diff/60001/media/base/android/medi... media/base/android/media_service_throttler.h:45: friend class MediaServiceThrottlerTest; Is adding tests as a friend class frowned upon? sandersd@, you mentioned having a subclass redefining certain things as public. Is this the preferred way to go?
https://codereview.chromium.org/2471903002/diff/60001/media/base/android/java... File media/base/android/java/src/org/chromium/media/MediaServerCrashListener.java (right): https://codereview.chromium.org/2471903002/diff/60001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaServerCrashListener.java:58: Log.e(TAG, "Exception happens while creating the watch dog player.", e); Nits: remove 'happens', 'watch dog' -> 'watchdog'. https://codereview.chromium.org/2471903002/diff/60001/media/base/android/medi... File media/base/android/media_service_throttler.cc (right): https://codereview.chromium.org/2471903002/diff/60001/media/base/android/medi... media/base/android/media_service_throttler.cc:13: namespace { Nit: The anonymous namespace is inside the named namespace in the majority of Chromium code. https://codereview.chromium.org/2471903002/diff/60001/media/base/android/medi... File media/base/android/media_service_throttler.h (right): https://codereview.chromium.org/2471903002/diff/60001/media/base/android/medi... media/base/android/media_service_throttler.h:45: friend class MediaServiceThrottlerTest; On 2016/11/11 03:50:29, tguilbert wrote: > Is adding tests as a friend class frowned upon? sandersd@, you mentioned having > a subclass redefining certain things as public. Is this the preferred way to go? Friend classes are fine for testing, and common in Chromium. Note though that the friendship is with the base test class, not the test cases, so you have to forward everything through your own methods anyway. In many such cases I find that subclassing is cleaner, and is more likely to eliminate ForTesting() methods (which are an anti-pattern, even if they are popular in Chromium).
https://codereview.chromium.org/2471903002/diff/60001/content/browser/media/a... File content/browser/media/android/media_player_renderer.cc (right): https://codereview.chromium.org/2471903002/diff/60001/content/browser/media/a... content/browser/media/android/media_player_renderer.cc:53: pending_init_cb_ = init_cb; Bind it into CreateMediaPlayer instead? https://codereview.chromium.org/2471903002/diff/60001/content/browser/media/a... File content/browser/media/android/media_player_renderer.h (right): https://codereview.chromium.org/2471903002/diff/60001/content/browser/media/a... content/browser/media/android/media_player_renderer.h:99: void CreateUnderlyingMediaPlayer(const media::MediaUrlParams& params); Just CreateMediaPlayer? https://codereview.chromium.org/2471903002/diff/60001/media/base/android/java... File media/base/android/java/src/org/chromium/media/MediaServerCrashListener.java (right): https://codereview.chromium.org/2471903002/diff/60001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaServerCrashListener.java:27: // Application context. Not useful https://codereview.chromium.org/2471903002/diff/60001/media/base/android/medi... File media/base/android/media_server_crash_listener.cc (right): https://codereview.chromium.org/2471903002/diff/60001/media/base/android/medi... media/base/android/media_server_crash_listener.cc:52: if (on_server_crash_cb_) Seems this should clear is_listening_? https://codereview.chromium.org/2471903002/diff/60001/media/base/android/medi... File media/base/android/media_server_crash_listener.h (right): https://codereview.chromium.org/2471903002/diff/60001/media/base/android/medi... media/base/android/media_server_crash_listener.h:26: MediaServerCrashListener( Add some method comments to each of these. https://codereview.chromium.org/2471903002/diff/60001/media/base/android/medi... media/base/android/media_server_crash_listener.h:29: virtual ~MediaServerCrashListener(); Remove virtual? https://codereview.chromium.org/2471903002/diff/60001/media/base/android/medi... File media/base/android/media_service_throttler.cc (right): https://codereview.chromium.org/2471903002/diff/60001/media/base/android/medi... media/base/android/media_service_throttler.cc:207: base::TimeDelta::FromMinutes(1)); This should probably be more clearly related to your decay constants above. https://codereview.chromium.org/2471903002/diff/60001/media/base/android/medi... File media/base/android/media_service_throttler.h (right): https://codereview.chromium.org/2471903002/diff/60001/media/base/android/medi... media/base/android/media_service_throttler.h:32: void SetTickClockForTesting(base::TickClock* clock); Usually I'd put these as a single block with a comment "// Test only methods." to delineate the API a bit more clearly. https://codereview.chromium.org/2471903002/diff/60001/media/base/android/medi... media/base/android/media_service_throttler.h:69: // Effective number of media server crashes. Explain why it's a double.
watk@chromium.org changed reviewers: + watk@chromium.org
Just drive by nits https://codereview.chromium.org/2471903002/diff/60001/media/base/android/java... File media/base/android/java/src/org/chromium/media/MediaServerCrashListener.java (right): https://codereview.chromium.org/2471903002/diff/60001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaServerCrashListener.java:17: * Class for listening to Android MediaServer Crashes to throttle media decoding s/Crashes/crashes https://codereview.chromium.org/2471903002/diff/60001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaServerCrashListener.java:25: // Watch dog player. Used to listen to all media server crashes. watchdog https://codereview.chromium.org/2471903002/diff/60001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaServerCrashListener.java:59: } catch (RuntimeException e) { You can catch both exceptions in one block with | https://codereview.chromium.org/2471903002/diff/60001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaServerCrashListener.java:68: Log.e(TAG, "Unable to create watch dog player, treat it as server crash."); watchdog https://codereview.chromium.org/2471903002/diff/60001/media/base/android/medi... File media/base/android/media_server_crash_listener.cc (right): https://codereview.chromium.org/2471903002/diff/60001/media/base/android/medi... media/base/android/media_server_crash_listener.cc:23: callback_task_runner_(callback_thread) { You should move this into the member variable to avoid bumping the refcount unnecessarily. https://codereview.chromium.org/2471903002/diff/60001/media/base/android/medi... File media/base/android/media_server_crash_listener.h (right): https://codereview.chromium.org/2471903002/diff/60001/media/base/android/medi... media/base/android/media_server_crash_listener.h:17: // Uses a watchdog Android MediaPlayer to listen for media server crashes, and nit: MediaServer https://codereview.chromium.org/2471903002/diff/60001/media/base/android/medi... media/base/android/media_server_crash_listener.h:28: scoped_refptr<base::SingleThreadTaskRunner> callback_thread); nit: it's a task runner not a thread. https://codereview.chromium.org/2471903002/diff/60001/media/base/android/medi... media/base/android/media_server_crash_listener.h:44: // Thread on which the |on_server_crash_cb_| will be posted. s/thread on/task runner to/ https://codereview.chromium.org/2471903002/diff/60001/media/base/android/medi... File media/base/android/media_service_throttler.cc (right): https://codereview.chromium.org/2471903002/diff/60001/media/base/android/medi... media/base/android/media_service_throttler.cc:151: uint32_t number_crashes = std::floor(current_crashes_); number_crashes sounds weird but num_crashes doesn't. Weird. https://codereview.chromium.org/2471903002/diff/60001/media/base/android/medi... media/base/android/media_service_throttler.cc:173: if (time_since_last_crash > kTimeUntilCrashReset) { Sorry if this is a dumb question, but is this special case necessary? Seems handled by the else.
PTAL! Addressed comments. Also, sorry to change this up this late in a review, but I had to change some post-crash logic: The comments for MEDIA_ERROR_SERVER_DIED call for releasing the MediaPlayer. MediaThrottler does not explicitly do so, but it seems like the MediaThrottler starts/stops frequently enough for this to not be a problem. We keep |crash_listener_| alive for 60s at a time, so explicitly releasing/recreating the MediaPlayer seems important. As a follow up to releasing the watchdog, I also added code to limit the amount of times a failed watchdog creation is reported as a crash to once per 5s: Comments in the MediaThrottler mention that it takes ~5s for the media server to restart. Without this limit, the MediaServer could crash once, and we release the watchdog. In the 5s necessary for the MediaServer to restart, 10 MediaPlayerRenderer creations could each trigger calls to startListening(), which could all fail, and report the failure to create a watchdog as 10 crashes (thus triggering an extreme amount of throttling, from one initial crash). LMK if you think this is uncessary, or if there is a cleaner way to do this. Thanks! Thomas https://codereview.chromium.org/2471903002/diff/60001/content/browser/media/a... File content/browser/media/android/media_player_renderer.cc (right): https://codereview.chromium.org/2471903002/diff/60001/content/browser/media/a... content/browser/media/android/media_player_renderer.cc:53: pending_init_cb_ = init_cb; On 2016/11/11 23:32:15, DaleCurtis wrote: > Bind it into CreateMediaPlayer instead? Done. https://codereview.chromium.org/2471903002/diff/60001/content/browser/media/a... File content/browser/media/android/media_player_renderer.h (right): https://codereview.chromium.org/2471903002/diff/60001/content/browser/media/a... content/browser/media/android/media_player_renderer.h:99: void CreateUnderlyingMediaPlayer(const media::MediaUrlParams& params); On 2016/11/11 23:32:15, DaleCurtis wrote: > Just CreateMediaPlayer? Done. https://codereview.chromium.org/2471903002/diff/60001/media/base/android/java... File media/base/android/java/src/org/chromium/media/MediaServerCrashListener.java (right): https://codereview.chromium.org/2471903002/diff/60001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaServerCrashListener.java:17: * Class for listening to Android MediaServer Crashes to throttle media decoding On 2016/11/12 04:22:16, watk wrote: > s/Crashes/crashes Done. https://codereview.chromium.org/2471903002/diff/60001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaServerCrashListener.java:25: // Watch dog player. Used to listen to all media server crashes. On 2016/11/12 04:22:16, watk wrote: > watchdog Done. https://codereview.chromium.org/2471903002/diff/60001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaServerCrashListener.java:27: // Application context. On 2016/11/11 23:32:15, DaleCurtis wrote: > Not useful Done. https://codereview.chromium.org/2471903002/diff/60001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaServerCrashListener.java:58: Log.e(TAG, "Exception happens while creating the watch dog player.", e); On 2016/11/11 18:55:14, sandersd wrote: > Nits: remove 'happens', 'watch dog' -> 'watchdog'. Done. https://codereview.chromium.org/2471903002/diff/60001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaServerCrashListener.java:59: } catch (RuntimeException e) { On 2016/11/12 04:22:16, watk wrote: > You can catch both exceptions in one block with | Dale had the same comment last iteration. Copy pasting my response: I get this error message: error: Alternatives in a multi-catch statement cannot be related by subclassing We would loose the contents of the exception, because e would only be of type throwable according to http://stackoverflow.com/a/19470374 https://codereview.chromium.org/2471903002/diff/60001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaServerCrashListener.java:68: Log.e(TAG, "Unable to create watch dog player, treat it as server crash."); On 2016/11/12 04:22:16, watk wrote: > watchdog Done. https://codereview.chromium.org/2471903002/diff/60001/media/base/android/medi... File media/base/android/media_server_crash_listener.cc (right): https://codereview.chromium.org/2471903002/diff/60001/media/base/android/medi... media/base/android/media_server_crash_listener.cc:52: if (on_server_crash_cb_) On 2016/11/11 23:32:15, DaleCurtis wrote: > Seems this should clear is_listening_? This was an artifact of the previous iteration. Updated. (As to why this is not is_listening_: the method will never be called unless we are listening) https://codereview.chromium.org/2471903002/diff/60001/media/base/android/medi... File media/base/android/media_server_crash_listener.h (right): https://codereview.chromium.org/2471903002/diff/60001/media/base/android/medi... media/base/android/media_server_crash_listener.h:17: // Uses a watchdog Android MediaPlayer to listen for media server crashes, and On 2016/11/12 04:22:16, watk wrote: > nit: MediaServer Done. https://codereview.chromium.org/2471903002/diff/60001/media/base/android/medi... media/base/android/media_server_crash_listener.h:26: MediaServerCrashListener( On 2016/11/11 23:32:16, DaleCurtis wrote: > Add some method comments to each of these. Done. https://codereview.chromium.org/2471903002/diff/60001/media/base/android/medi... media/base/android/media_server_crash_listener.h:28: scoped_refptr<base::SingleThreadTaskRunner> callback_thread); On 2016/11/12 04:22:16, watk wrote: > nit: it's a task runner not a thread. Done. https://codereview.chromium.org/2471903002/diff/60001/media/base/android/medi... media/base/android/media_server_crash_listener.h:29: virtual ~MediaServerCrashListener(); On 2016/11/11 23:32:15, DaleCurtis wrote: > Remove virtual? Done. https://codereview.chromium.org/2471903002/diff/60001/media/base/android/medi... media/base/android/media_server_crash_listener.h:44: // Thread on which the |on_server_crash_cb_| will be posted. On 2016/11/12 04:22:16, watk wrote: > s/thread on/task runner to/ Done. https://codereview.chromium.org/2471903002/diff/60001/media/base/android/medi... File media/base/android/media_service_throttler.cc (right): https://codereview.chromium.org/2471903002/diff/60001/media/base/android/medi... media/base/android/media_service_throttler.cc:13: namespace { On 2016/11/11 18:55:14, sandersd wrote: > Nit: The anonymous namespace is inside the named namespace in the majority of > Chromium code. Done. https://codereview.chromium.org/2471903002/diff/60001/media/base/android/medi... media/base/android/media_service_throttler.cc:151: uint32_t number_crashes = std::floor(current_crashes_); On 2016/11/12 04:22:16, watk wrote: > number_crashes sounds weird but num_crashes doesn't. Weird. Done. https://codereview.chromium.org/2471903002/diff/60001/media/base/android/medi... media/base/android/media_service_throttler.cc:173: if (time_since_last_crash > kTimeUntilCrashReset) { On 2016/11/12 04:22:16, watk wrote: > Sorry if this is a dumb question, but is this special case necessary? Seems > handled by the else. Not a dumb question :) We decay at a rate of 1/min, but crashes can occur faster than 1/min. If we are crashing 3 times per min, |current_crashes| will go up at the effective rate of 2/min, and we will never enter the if. The 'if' portion is to completely reset the count if we did not crash for a minute. Otherwise, we might end up with a burst of crashes, bringing |current_crashes_| to 60, and it would take an hour to stop throttling. https://codereview.chromium.org/2471903002/diff/60001/media/base/android/medi... media/base/android/media_service_throttler.cc:207: base::TimeDelta::FromMinutes(1)); On 2016/11/11 23:32:16, DaleCurtis wrote: > This should probably be more clearly related to your decay constants above. Added a new constant and comments. https://codereview.chromium.org/2471903002/diff/60001/media/base/android/medi... File media/base/android/media_service_throttler.h (right): https://codereview.chromium.org/2471903002/diff/60001/media/base/android/medi... media/base/android/media_service_throttler.h:32: void SetTickClockForTesting(base::TickClock* clock); On 2016/11/11 23:32:16, DaleCurtis wrote: > Usually I'd put these as a single block with a comment "// Test only methods." > to delineate the API a bit more clearly. Done. https://codereview.chromium.org/2471903002/diff/60001/media/base/android/medi... media/base/android/media_service_throttler.h:69: // Effective number of media server crashes. On 2016/11/11 23:32:16, DaleCurtis wrote: > Explain why it's a double. Done.
lgtm https://codereview.chromium.org/2471903002/diff/120001/media/base/android/jav... File media/base/android/java/src/org/chromium/media/MediaServerCrashListener.java (right): https://codereview.chromium.org/2471903002/diff/120001/media/base/android/jav... media/base/android/java/src/org/chromium/media/MediaServerCrashListener.java:23: private static final String TAG = "crMediaCrashListener"; Don't know about the prefix, might be special in regards to some logging tools. https://codereview.chromium.org/2471903002/diff/120001/media/base/android/jav... media/base/android/java/src/org/chromium/media/MediaServerCrashListener.java:30: // Protecting the creation/release of the watchdog player. As discussed offline, lets not call releaseWatchdog() from JNI, do it instead from C++ to avoid all this locking. https://codereview.chromium.org/2471903002/diff/120001/media/base/android/med... File media/base/android/media_service_throttler.cc (right): https://codereview.chromium.org/2471903002/diff/120001/media/base/android/med... media/base/android/media_service_throttler.cc:157: uint32_t num_crashes = std::floor(current_crashes_); Just casting to uint32_t should be sufficient. https://codereview.chromium.org/2471903002/diff/120001/media/base/android/med... File media/base/android/media_service_throttler.h (right): https://codereview.chromium.org/2471903002/diff/120001/media/base/android/med... media/base/android/media_service_throttler.h:22: class MEDIA_EXPORT MediaServiceThrottler { Class docs?
The CQ bit was checked by tguilbert@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL :) https://codereview.chromium.org/2471903002/diff/120001/media/base/android/jav... File media/base/android/java/src/org/chromium/media/MediaServerCrashListener.java (right): https://codereview.chromium.org/2471903002/diff/120001/media/base/android/jav... media/base/android/java/src/org/chromium/media/MediaServerCrashListener.java:23: private static final String TAG = "crMediaCrashListener"; On 2016/11/15 00:49:32, DaleCurtis wrote: > Don't know about the prefix, might be special in regards to some logging tools. Acknowledged. https://codereview.chromium.org/2471903002/diff/120001/media/base/android/jav... media/base/android/java/src/org/chromium/media/MediaServerCrashListener.java:30: // Protecting the creation/release of the watchdog player. On 2016/11/15 00:49:32, DaleCurtis wrote: > As discussed offline, lets not call releaseWatchdog() from JNI, do it instead > from C++ to avoid all this locking. Done. https://codereview.chromium.org/2471903002/diff/120001/media/base/android/med... File media/base/android/media_service_throttler.cc (right): https://codereview.chromium.org/2471903002/diff/120001/media/base/android/med... media/base/android/media_service_throttler.cc:157: uint32_t num_crashes = std::floor(current_crashes_); On 2016/11/15 00:49:32, DaleCurtis wrote: > Just casting to uint32_t should be sufficient. Done. https://codereview.chromium.org/2471903002/diff/120001/media/base/android/med... File media/base/android/media_service_throttler.h (right): https://codereview.chromium.org/2471903002/diff/120001/media/base/android/med... media/base/android/media_service_throttler.h:22: class MEDIA_EXPORT MediaServiceThrottler { On 2016/11/15 00:49:32, DaleCurtis wrote: > Class docs? Done.
Still lgtm
lgtm
The CQ bit was checked by tguilbert@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by tguilbert@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by tguilbert@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add MediaServiceThrottler WMPA used the MediaThrottler to display a notification bar to the user if an unusual number of media server crashes were detected. This was done in order to prevent a webpage from loading the same url over and over in order to crash the Media Server. This change introduces the MediaServerCrashListener which reuses the same technique that the MediaThrottler used to monitor for Media Server crashes. It also introduces the MediaServiceThrottler which uses the MediaServerCrashListener, and exponentially delays the creation of new clients (MediaPlayerBridges for now) based on the number of Media Server crashes. The MediaServiceThrottler resets its internal clock state after a minute of not having schedule new client creations, and reset its crash counter after a minute of not having detected any crashes. It also support "burst scheduling", allowing for a fixed number of clients to be scheduled immediately within some sliding window, in order to minimize impact from multiple requests coming in from new page loads. BUG=636615 TEST=Added UTs ========== to ========== Add MediaServiceThrottler WMPA used the MediaThrottler to display a notification bar to the user if an unusual number of media server crashes were detected. This was done in order to prevent a webpage from loading the same url over and over in order to crash the Media Server. This change introduces the MediaServerCrashListener which reuses the same technique that the MediaThrottler used to monitor for Media Server crashes. It also introduces the MediaServiceThrottler which uses the MediaServerCrashListener, and exponentially delays the creation of new clients (MediaPlayerBridges for now) based on the number of Media Server crashes. The MediaServiceThrottler resets its internal clock state after a minute of not having schedule new client creations, and reset its crash counter after a minute of not having detected any crashes. It also support "burst scheduling", allowing for a fixed number of clients to be scheduled immediately within some sliding window, in order to minimize impact from multiple requests coming in from new page loads. BUG=636615 TEST=Added UTs ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Add MediaServiceThrottler WMPA used the MediaThrottler to display a notification bar to the user if an unusual number of media server crashes were detected. This was done in order to prevent a webpage from loading the same url over and over in order to crash the Media Server. This change introduces the MediaServerCrashListener which reuses the same technique that the MediaThrottler used to monitor for Media Server crashes. It also introduces the MediaServiceThrottler which uses the MediaServerCrashListener, and exponentially delays the creation of new clients (MediaPlayerBridges for now) based on the number of Media Server crashes. The MediaServiceThrottler resets its internal clock state after a minute of not having schedule new client creations, and reset its crash counter after a minute of not having detected any crashes. It also support "burst scheduling", allowing for a fixed number of clients to be scheduled immediately within some sliding window, in order to minimize impact from multiple requests coming in from new page loads. BUG=636615 TEST=Added UTs ========== to ========== Add MediaServiceThrottler WMPA used the MediaThrottler to display a notification bar to the user if an unusual number of media server crashes were detected. This was done in order to prevent a webpage from loading the same url over and over in order to crash the Media Server. This change introduces the MediaServerCrashListener which reuses the same technique that the MediaThrottler used to monitor for Media Server crashes. It also introduces the MediaServiceThrottler which uses the MediaServerCrashListener, and exponentially delays the creation of new clients (MediaPlayerBridges for now) based on the number of Media Server crashes. The MediaServiceThrottler resets its internal clock state after a minute of not having schedule new client creations, and reset its crash counter after a minute of not having detected any crashes. It also support "burst scheduling", allowing for a fixed number of clients to be scheduled immediately within some sliding window, in order to minimize impact from multiple requests coming in from new page loads. BUG=636615 TEST=Added UTs Committed: https://crrev.com/f524383bbd03acfba4ffe18b7e77a66c46228dcd Cr-Commit-Position: refs/heads/master@{#432712} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/f524383bbd03acfba4ffe18b7e77a66c46228dcd Cr-Commit-Position: refs/heads/master@{#432712} |
