Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2948613002:
[AudioStreamMonitor] Adds API to collect frame-level audibility. (Closed)
|
Created:
3 years, 6 months ago by lpy Modified:
3 years, 5 months ago CC:
chrisha, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, miu+watch_chromium.org, nasko+codewatch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[AudioStreamMonitor] Adds API to collect frame-level audibility.
This patch creates API to collect frame-level audibility in order to do
frame-level throttling and suspension, only sends audio changed state to
RenderFrameHost when we don't have power level monitoring or the audible state
of a stream changes, and defers to RenderFrameHost to send signals to
RenderProcessHost about audio stream added/removed.
BUG=731270
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2948613002
Cr-Commit-Position: refs/heads/master@{#483075}
Committed: https://chromium.googlesource.com/chromium/src/+/5e9ff6090994e9e6b3e63e40436654eb4e968271
Patch Set 1 #
Total comments: 8
Patch Set 2 : Addressed comments, fixed tests #
Total comments: 22
Patch Set 3 : Addressed comments #Patch Set 4 : Correct comments in code #
Total comments: 6
Patch Set 5 : Removed public API, fixed tests #
Total comments: 21
Patch Set 6 : Addressed comments #
Total comments: 6
Patch Set 7 : Addressed comments and rebased #
Total comments: 14
Patch Set 8 : Addressed comments and rebased #Patch Set 9 : Fix windows build #
Messages
Total messages: 75 (55 generated)
Description was changed from ========== [AudioStreamMonitor] Adds API to collect frame-level audibility. This patch creates API to collect frame-level audibility in order to do frame-level throttling and suspension, only sends audio changed signals to RenderProcessHost when we don't have power level monitoring or the audible state of a stream changes. BUG=731270 ========== to ========== [AudioStreamMonitor] Adds API to collect frame-level audibility. This patch creates API to collect frame-level audibility in order to do frame-level throttling and suspension, only sends audio changed signals to RenderProcessHost when we don't have power level monitoring or the audible state of a stream changes. BUG=731270 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by lpy@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...
lpy@chromium.org changed reviewers: + dalecurtis@chromium.org, miu@chromium.org, nasko@chromium.org
dalecurtis@, miu@, ptal. nasko@, ptal as content owner. chrisha@, fyi
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2948613002/diff/1/content/browser/media/audio... File content/browser/media/audio_stream_monitor.cc (right): https://codereview.chromium.org/2948613002/diff/1/content/browser/media/audio... content/browser/media/audio_stream_monitor.cc:149: // We should send audio-added signal to RenderProcessHost when we don't have style nit: Try not to use "we" in comments. :) https://codereview.chromium.org/2948613002/diff/1/content/browser/media/audio... content/browser/media/audio_stream_monitor.cc:153: RenderProcessHost::FromID(render_process_id)->OnAudioStreamAdded(); Always null-check the result of RPH::FromID() (here and elsewhere), since you never know when a process will go away. Think of the |render_frame_id| as a weak pointer. I usually write code like this: if (auto* host = RenderProcesHost::FromId(...)) host->OnAudioStreamAdded(); https://codereview.chromium.org/2948613002/diff/1/content/browser/media/audio... content/browser/media/audio_stream_monitor.cc:169: // We should send audio-removed signal to RenderProcessHost when we don't have style nit: ditto (no "we") https://codereview.chromium.org/2948613002/diff/1/content/browser/media/audio... content/browser/media/audio_stream_monitor.cc:182: std::unordered_map<RenderFrameHost*, bool> is_frame_audible; It feels expensive to build-up a data structure on each Poll() call (several per second). I'm trying to think of whether there might be lighter-weight approaches to accomplish our goals here. Can you point us to a design doc? We should consider how the various audio playback cases will determine when/how each render frame is throttled. For example, if we want a "throttle-down after N seconds of silence" heuristic, then we may only need to record the |last_blurt_time_| per render frame.
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Thanks for the feedback, I updated the patch. https://codereview.chromium.org/2948613002/diff/1/content/browser/media/audio... File content/browser/media/audio_stream_monitor.cc (right): https://codereview.chromium.org/2948613002/diff/1/content/browser/media/audio... content/browser/media/audio_stream_monitor.cc:149: // We should send audio-added signal to RenderProcessHost when we don't have On 2017/06/19 20:44:01, miu wrote: > style nit: Try not to use "we" in comments. :) Done. https://codereview.chromium.org/2948613002/diff/1/content/browser/media/audio... content/browser/media/audio_stream_monitor.cc:153: RenderProcessHost::FromID(render_process_id)->OnAudioStreamAdded(); On 2017/06/19 20:44:01, miu wrote: > Always null-check the result of RPH::FromID() (here and elsewhere), since you > never know when a process will go away. Think of the |render_frame_id| as a weak > pointer. I usually write code like this: > > if (auto* host = RenderProcesHost::FromId(...)) > host->OnAudioStreamAdded(); Done. https://codereview.chromium.org/2948613002/diff/1/content/browser/media/audio... content/browser/media/audio_stream_monitor.cc:169: // We should send audio-removed signal to RenderProcessHost when we don't have On 2017/06/19 20:44:01, miu wrote: > style nit: ditto (no "we") Done. https://codereview.chromium.org/2948613002/diff/1/content/browser/media/audio... content/browser/media/audio_stream_monitor.cc:182: std::unordered_map<RenderFrameHost*, bool> is_frame_audible; On 2017/06/19 20:44:01, miu wrote: > It feels expensive to build-up a data structure on each Poll() call (several per > second). I'm trying to think of whether there might be lighter-weight approaches > to accomplish our goals here. > > Can you point us to a design doc? We should consider how the various audio > playback cases will determine when/how each render frame is throttled. For > example, if we want a "throttle-down after N seconds of silence" heuristic, then > we may only need to record the |last_blurt_time_| per render frame. https://docs.google.com/document/d/15wWebVCLU3UM-OAQBVi1mOX0ZPzYupPN9tw4wy7lb... As an example, a metric we want to collect, is the duration from a tab becomes backgrounded to a frame(say root frame) in it becomes audible, we consider a frame becomes audible if it's not audible in the last 1 minute. I think that's a good suggestion. What about sending audio-added/removed signal to RenderProcessHost? It's really tricky in RenderProcessGone(), because we don't want to call RenderProcessHost::OnAudioStreamRemoved() everytime, we only want to call it when RenderFrameHost is audible, and since the |poll_callbacks_| can have multiple entries for one render frame host, we also want to avoid calling multiple times on RenderProcessHost, we want to call just one time for a render frame host. Do I understand correctly?
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2948613002/diff/20001/content/browser/media/a... File content/browser/media/audio_stream_monitor.cc (right): https://codereview.chromium.org/2948613002/diff/20001/content/browser/media/a... content/browser/media/audio_stream_monitor.cc:179: if (it == poll_callbacks_.end()) { No need for {} on single line conditional. https://codereview.chromium.org/2948613002/diff/20001/content/browser/media/a... content/browser/media/audio_stream_monitor.cc:201: std::unordered_map<RenderFrameHost*, bool> is_frame_audible; I don't think there should be very many of these, ~100 at worst, so a base::flat_map should be sufficient. You should initialize the size to poll_callbacks_.size() via flat_map.reserve(). Also the name should be something audible_frame_map https://codereview.chromium.org/2948613002/diff/20001/content/browser/media/a... content/browser/media/audio_stream_monitor.cc:221: if (!render_frame_host) I think this should be impossible here, we would have gotten a going away call. https://codereview.chromium.org/2948613002/diff/20001/content/browser/media/a... content/browser/media/audio_stream_monitor.cc:226: is_frame_audible[render_frame_host] |= is_stream_audible; I think you just need this line. Default value of false will be inserted, so no reason to do a find. Also doing find then [] is two lookups. https://codereview.chromium.org/2948613002/diff/20001/content/browser/media/a... content/browser/media/audio_stream_monitor.cc:230: // Update RenderFrameHost and RenderProcessHost audible state only when state Do we want to be responsible for both RenderFrameHost and RenderProcessHost updates? Or should we have RenderFrameHosts update their RenderProcessHost? https://codereview.chromium.org/2948613002/diff/20001/content/browser/media/a... content/browser/media/audio_stream_monitor.cc:232: for (auto it = is_frame_audible.begin(); it != is_frame_audible.end(); ++it) { for (auto& kv : ...) { } https://codereview.chromium.org/2948613002/diff/20001/content/browser/media/a... content/browser/media/audio_stream_monitor.cc:238: if (!render_process_host) Again shouldn't be possible at this point.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
chrisha@chromium.org changed reviewers: + chrisha@chromium.org
(drive by nit) https://codereview.chromium.org/2948613002/diff/20001/content/browser/media/a... File content/browser/media/audio_stream_monitor.h (right): https://codereview.chromium.org/2948613002/diff/20001/content/browser/media/a... content/browser/media/audio_stream_monitor.h:153: using StreamID = std::tuple<int, int, int>; IMO it would be useful to document what each entry in the tuple represents in the comment: (process_id, frame_id, stream_id).
https://codereview.chromium.org/2948613002/diff/20001/content/browser/media/a... File content/browser/media/audio_stream_monitor.cc (right): https://codereview.chromium.org/2948613002/diff/20001/content/browser/media/a... content/browser/media/audio_stream_monitor.cc:80: RenderProcessHost::FromID(render_process_id)) innermost if statement also requires {}, since it is multiline. https://codereview.chromium.org/2948613002/diff/20001/content/browser/media/a... content/browser/media/audio_stream_monitor.cc:165: RenderProcessHost::FromID(render_process_id)) Same here about needing {} on multiline if statements. https://codereview.chromium.org/2948613002/diff/20001/content/public/browser/... File content/public/browser/render_frame_host.h (right): https://codereview.chromium.org/2948613002/diff/20001/content/public/browser/... content/public/browser/render_frame_host.h:245: virtual void OnAudioStateChanged(bool audible) = 0; These APIs are not used outside of content/, so they should not be added to the public RenderFrameHost interface, but kept internally only on RenderFrameHostImpl.
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Thanks for the feedback, ptal https://codereview.chromium.org/2948613002/diff/20001/content/browser/media/a... File content/browser/media/audio_stream_monitor.cc (right): https://codereview.chromium.org/2948613002/diff/20001/content/browser/media/a... content/browser/media/audio_stream_monitor.cc:80: RenderProcessHost::FromID(render_process_id)) On 2017/06/20 16:17:45, nasko (slow) wrote: > innermost if statement also requires {}, since it is multiline. Done. https://codereview.chromium.org/2948613002/diff/20001/content/browser/media/a... content/browser/media/audio_stream_monitor.cc:165: RenderProcessHost::FromID(render_process_id)) On 2017/06/20 16:17:45, nasko (slow) wrote: > Same here about needing {} on multiline if statements. Done. https://codereview.chromium.org/2948613002/diff/20001/content/browser/media/a... content/browser/media/audio_stream_monitor.cc:179: if (it == poll_callbacks_.end()) { On 2017/06/20 00:32:53, DaleCurtis wrote: > No need for {} on single line conditional. Done. https://codereview.chromium.org/2948613002/diff/20001/content/browser/media/a... content/browser/media/audio_stream_monitor.cc:201: std::unordered_map<RenderFrameHost*, bool> is_frame_audible; On 2017/06/20 00:32:53, DaleCurtis wrote: > I don't think there should be very many of these, ~100 at worst, so a > base::flat_map should be sufficient. You should initialize the size to > poll_callbacks_.size() via flat_map.reserve(). Also the name should be something > audible_frame_map Done. https://codereview.chromium.org/2948613002/diff/20001/content/browser/media/a... content/browser/media/audio_stream_monitor.cc:221: if (!render_frame_host) On 2017/06/20 00:32:53, DaleCurtis wrote: > I think this should be impossible here, we would have gotten a going away call. Done. https://codereview.chromium.org/2948613002/diff/20001/content/browser/media/a... content/browser/media/audio_stream_monitor.cc:226: is_frame_audible[render_frame_host] |= is_stream_audible; On 2017/06/20 00:32:53, DaleCurtis wrote: > I think you just need this line. Default value of false will be inserted, so no > reason to do a find. Also doing find then [] is two lookups. Done. https://codereview.chromium.org/2948613002/diff/20001/content/browser/media/a... content/browser/media/audio_stream_monitor.cc:230: // Update RenderFrameHost and RenderProcessHost audible state only when state On 2017/06/20 00:32:53, DaleCurtis wrote: > Do we want to be responsible for both RenderFrameHost and RenderProcessHost > updates? Or should we have RenderFrameHosts update their RenderProcessHost? Done. I defer to RenderFrameHosts to update their RenderProcessHost. https://codereview.chromium.org/2948613002/diff/20001/content/browser/media/a... content/browser/media/audio_stream_monitor.cc:232: for (auto it = is_frame_audible.begin(); it != is_frame_audible.end(); ++it) { On 2017/06/20 00:32:53, DaleCurtis wrote: > for (auto& kv : ...) { } Done. https://codereview.chromium.org/2948613002/diff/20001/content/browser/media/a... content/browser/media/audio_stream_monitor.cc:238: if (!render_process_host) On 2017/06/20 00:32:53, DaleCurtis wrote: > Again shouldn't be possible at this point. Done. https://codereview.chromium.org/2948613002/diff/20001/content/browser/media/a... File content/browser/media/audio_stream_monitor.h (right): https://codereview.chromium.org/2948613002/diff/20001/content/browser/media/a... content/browser/media/audio_stream_monitor.h:153: using StreamID = std::tuple<int, int, int>; On 2017/06/20 14:30:35, chrisha wrote: > IMO it would be useful to document what each entry in the tuple represents in > the comment: (process_id, frame_id, stream_id). Done. https://codereview.chromium.org/2948613002/diff/20001/content/public/browser/... File content/public/browser/render_frame_host.h (right): https://codereview.chromium.org/2948613002/diff/20001/content/public/browser/... content/public/browser/render_frame_host.h:245: virtual void OnAudioStateChanged(bool audible) = 0; On 2017/06/20 16:17:45, nasko (slow) wrote: > These APIs are not used outside of content/, so they should not be added to the > public RenderFrameHost interface, but kept internally only on > RenderFrameHostImpl. Done.
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 checked by lpy@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...
Description was changed from ========== [AudioStreamMonitor] Adds API to collect frame-level audibility. This patch creates API to collect frame-level audibility in order to do frame-level throttling and suspension, only sends audio changed signals to RenderProcessHost when we don't have power level monitoring or the audible state of a stream changes. BUG=731270 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== [AudioStreamMonitor] Adds API to collect frame-level audibility. This patch creates API to collect frame-level audibility in order to do frame-level throttling and suspension, only sends audio changed state to RenderFrameHost when we don't have power level monitoring or the audible state of a stream changes, and defers to RenderFrameHost to send signals to RenderProcessHost about audio stream added/removed. BUG=731270 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lpy@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: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2948613002/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2948613002/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:1000: if (IsAudible()) Why not use is_audible_? It is used directly in other new code below, so it is best to be consistent. https://codereview.chromium.org/2948613002/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/2948613002/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:186: void OnAudioStateChanged(bool audible); Move this method out of the section of RenderFrameHost overrides, likely further down in the file. Then the .cc file will have to keep the same ordering. https://codereview.chromium.org/2948613002/diff/60001/content/public/browser/... File content/public/browser/render_frame_host.h (right): https://codereview.chromium.org/2948613002/diff/60001/content/public/browser/... content/public/browser/render_frame_host.h:244: virtual bool IsAudible() = 0; This is still a public method that isn't used outside of content/. Please do not add methods here unless other code uses it.
The CQ bit was checked by lpy@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...
Thanks, I uploaded another patch, ptal. https://codereview.chromium.org/2948613002/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2948613002/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:1000: if (IsAudible()) On 2017/06/21 21:04:07, nasko (slow) wrote: > Why not use is_audible_? It is used directly in other new code below, so it is > best to be consistent. Done. https://codereview.chromium.org/2948613002/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/2948613002/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:186: void OnAudioStateChanged(bool audible); On 2017/06/21 21:04:07, nasko (slow) wrote: > Move this method out of the section of RenderFrameHost overrides, likely further > down in the file. Then the .cc file will have to keep the same ordering. Done. https://codereview.chromium.org/2948613002/diff/60001/content/public/browser/... File content/public/browser/render_frame_host.h (right): https://codereview.chromium.org/2948613002/diff/60001/content/public/browser/... content/public/browser/render_frame_host.h:244: virtual bool IsAudible() = 0; On 2017/06/21 21:04:07, nasko (slow) wrote: > This is still a public method that isn't used outside of content/. Please do not > add methods here unless other code uses it. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Just a bunch of nits, but overall lg2m modulo the question about the nullptr in tests. https://codereview.chromium.org/2948613002/diff/80001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2948613002/diff/80001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:1194: if (is_audible != is_audible_) { early return is preferred. https://codereview.chromium.org/2948613002/diff/80001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/2948613002/diff/80001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:251: bool IsAudible(); const? Prefer bool is_audible() const { return is_audible_; } for consistency with the other hacker_style() accessors here. https://codereview.chromium.org/2948613002/diff/80001/content/browser/media/a... File content/browser/media/audio_stream_monitor.cc (right): https://codereview.chromium.org/2948613002/diff/80001/content/browser/media/a... content/browser/media/audio_stream_monitor.cc:147: // Sends audible signal to RenderFrameHost when there is no power level Line above. https://codereview.chromium.org/2948613002/diff/80001/content/browser/media/a... content/browser/media/audio_stream_monitor.cc:154: } Line below. https://codereview.chromium.org/2948613002/diff/80001/content/browser/media/a... content/browser/media/audio_stream_monitor.cc:170: // Sends non-audible signal to RenderFrameHost when there is no power level Line above. https://codereview.chromium.org/2948613002/diff/80001/content/browser/media/a... content/browser/media/audio_stream_monitor.cc:189: for (StreamPollCallbackMap::const_iterator it = poll_callbacks_.begin(); We don't seem to be actually using the iterator, so how about for (auto& kv : poll_callbacks_) ? https://codereview.chromium.org/2948613002/diff/80001/content/browser/media/a... content/browser/media/audio_stream_monitor.cc:197: bool is_stream_audible = power_dbfs >= kSilenceThresholdDBFS; const for consistency. https://codereview.chromium.org/2948613002/diff/80001/content/browser/media/a... content/browser/media/audio_stream_monitor.cc:209: // This may be nullptr in tests. Which tests? It'd be better to avoid this conditional and make it a DCHECK() if the tests are doing things we don't expect in production. https://codereview.chromium.org/2948613002/diff/80001/content/browser/media/a... content/browser/media/audio_stream_monitor.cc:219: if (is_frame_audible != render_frame_host_impl->IsAudible()) { No {} for single line conditionals. https://codereview.chromium.org/2948613002/diff/80001/content/browser/media/a... File content/browser/media/audio_stream_monitor.h (right): https://codereview.chromium.org/2948613002/diff/80001/content/browser/media/a... content/browser/media/audio_stream_monitor.h:154: using StreamID = std::tuple<int, int, int>; This would be clearer as: struct StreamID { int render_process_id; int render_frame_id; int stream_id; }; You'll still be able to initialize it the same way (as POD type you can use = {1,2,3}), but now we have named fields for legibility.
The CQ bit was checked by lpy@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...
Thanks, I updated the patch https://codereview.chromium.org/2948613002/diff/80001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2948613002/diff/80001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:1194: if (is_audible != is_audible_) { On 2017/06/22 00:35:54, DaleCurtis wrote: > early return is preferred. Done. https://codereview.chromium.org/2948613002/diff/80001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/2948613002/diff/80001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:251: bool IsAudible(); On 2017/06/22 00:35:54, DaleCurtis wrote: > const? Prefer bool is_audible() const { return is_audible_; } for consistency > with the other hacker_style() accessors here. Done. https://codereview.chromium.org/2948613002/diff/80001/content/browser/media/a... File content/browser/media/audio_stream_monitor.cc (right): https://codereview.chromium.org/2948613002/diff/80001/content/browser/media/a... content/browser/media/audio_stream_monitor.cc:147: // Sends audible signal to RenderFrameHost when there is no power level On 2017/06/22 00:35:55, DaleCurtis wrote: > Line above. Done. https://codereview.chromium.org/2948613002/diff/80001/content/browser/media/a... content/browser/media/audio_stream_monitor.cc:154: } On 2017/06/22 00:35:55, DaleCurtis wrote: > Line below. Done. https://codereview.chromium.org/2948613002/diff/80001/content/browser/media/a... content/browser/media/audio_stream_monitor.cc:170: // Sends non-audible signal to RenderFrameHost when there is no power level On 2017/06/22 00:35:55, DaleCurtis wrote: > Line above. Done. https://codereview.chromium.org/2948613002/diff/80001/content/browser/media/a... content/browser/media/audio_stream_monitor.cc:189: for (StreamPollCallbackMap::const_iterator it = poll_callbacks_.begin(); On 2017/06/22 00:35:55, DaleCurtis wrote: > We don't seem to be actually using the iterator, so how about for (auto& kv : > poll_callbacks_) ? Done. https://codereview.chromium.org/2948613002/diff/80001/content/browser/media/a... content/browser/media/audio_stream_monitor.cc:197: bool is_stream_audible = power_dbfs >= kSilenceThresholdDBFS; On 2017/06/22 00:35:54, DaleCurtis wrote: > const for consistency. Done. https://codereview.chromium.org/2948613002/diff/80001/content/browser/media/a... content/browser/media/audio_stream_monitor.cc:209: // This may be nullptr in tests. On 2017/06/22 00:35:55, DaleCurtis wrote: > Which tests? It'd be better to avoid this conditional and make it a DCHECK() if > the tests are doing things we don't expect in production. AudioStreamMonitorTest.HandlesMultipleStreamsBlurting and AudioStreamMonitorTest.ImpulsesKeepIndicatorOnUntilHoldingPeriodHasPassed, ids in FromID are not real ids. https://codereview.chromium.org/2948613002/diff/80001/content/browser/media/a... content/browser/media/audio_stream_monitor.cc:219: if (is_frame_audible != render_frame_host_impl->IsAudible()) { On 2017/06/22 00:35:55, DaleCurtis wrote: > No {} for single line conditionals. Done. https://codereview.chromium.org/2948613002/diff/80001/content/browser/media/a... File content/browser/media/audio_stream_monitor.h (right): https://codereview.chromium.org/2948613002/diff/80001/content/browser/media/a... content/browser/media/audio_stream_monitor.h:154: using StreamID = std::tuple<int, int, int>; On 2017/06/22 00:35:55, DaleCurtis wrote: > This would be clearer as: > > struct StreamID { > int render_process_id; > int render_frame_id; > int stream_id; > }; > > You'll still be able to initialize it the same way (as POD type you can use = > {1,2,3}), but now we have named fields for legibility. Done.
media/ lgtm % nits. Thanks for your patience! https://codereview.chromium.org/2948613002/diff/80001/content/browser/media/a... File content/browser/media/audio_stream_monitor.cc (right): https://codereview.chromium.org/2948613002/diff/80001/content/browser/media/a... content/browser/media/audio_stream_monitor.cc:209: // This may be nullptr in tests. On 2017/06/22 at 02:28:52, lpy wrote: > On 2017/06/22 00:35:55, DaleCurtis wrote: > > Which tests? It'd be better to avoid this conditional and make it a DCHECK() if > > the tests are doing things we don't expect in production. > > AudioStreamMonitorTest.HandlesMultipleStreamsBlurting and AudioStreamMonitorTest.ImpulsesKeepIndicatorOnUntilHoldingPeriodHasPassed, ids in FromID are not real ids. Ah, probably they shouldn't do that, or should set it up such that this doesn't resolve to nullptr, but that's not necessary for you to fix. https://codereview.chromium.org/2948613002/diff/100001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/2948613002/diff/100001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:251: bool is_audible() const; Method should be inlined when using hacker_style(), just like on l.254 https://codereview.chromium.org/2948613002/diff/100001/content/browser/media/... File content/browser/media/audio_stream_monitor.h (right): https://codereview.chromium.org/2948613002/diff/100001/content/browser/media/... content/browser/media/audio_stream_monitor.h:158: using StreamPollCallbackMap = std::map<StreamID, ReadPowerAndClipCallback>; Can probably use flat_map here too.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Sorry for the delay. I will be out Thu. Will take another look first thing Friday.
LGTM with an optional nit, assuming all other reviewer nits are also addressed. https://codereview.chromium.org/2948613002/diff/100001/content/browser/media/... File content/browser/media/audio_stream_monitor.cc (right): https://codereview.chromium.org/2948613002/diff/100001/content/browser/media/... content/browser/media/audio_stream_monitor.cc:46: return stream_id < other.stream_id; nit: Using std::tie makes code a bit more readable. An example: https://cs.chromium.org/chromium/src/url/scheme_host_port.cc?l=189
The CQ bit was checked by lpy@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...
Thanks for the comments, I updated the patch. https://codereview.chromium.org/2948613002/diff/100001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/2948613002/diff/100001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:251: bool is_audible() const; On 2017/06/22 03:15:40, DaleCurtis wrote: > Method should be inlined when using hacker_style(), just like on l.254 Done. https://codereview.chromium.org/2948613002/diff/100001/content/browser/media/... File content/browser/media/audio_stream_monitor.cc (right): https://codereview.chromium.org/2948613002/diff/100001/content/browser/media/... content/browser/media/audio_stream_monitor.cc:46: return stream_id < other.stream_id; On 2017/06/22 15:38:06, nasko (slow) wrote: > nit: Using std::tie makes code a bit more readable. An example: > https://cs.chromium.org/chromium/src/url/scheme_host_port.cc?l=189 Done. https://codereview.chromium.org/2948613002/diff/100001/content/browser/media/... File content/browser/media/audio_stream_monitor.h (right): https://codereview.chromium.org/2948613002/diff/100001/content/browser/media/... content/browser/media/audio_stream_monitor.h:158: using StreamPollCallbackMap = std::map<StreamID, ReadPowerAndClipCallback>; On 2017/06/22 03:15:40, DaleCurtis wrote: > Can probably use flat_map here too. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
miu@, gentle ping for review.
The CQ bit was checked by lpy@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: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
Comments on PS7: https://codereview.chromium.org/2948613002/diff/120001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2948613002/diff/120001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:1193: GetProcess()->OnAudioStreamAdded(); The OnAudioStreamAdded/Removed() methods need to be renamed. I'm not sure what their original purpose was: Do they have any other callers besides this code? Since they are now being called to report changes in audibility, and not whether audio streams have actually been created or destroyed, I'd suggest something like: OnRenderFrameBecameAudible() OnRenderFrameBecameSilent() https://codereview.chromium.org/2948613002/diff/120001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/2948613002/diff/120001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:253: void OnAudioStateChanged(bool is_audible); naming nit: OnAudibleStateChanged https://codereview.chromium.org/2948613002/diff/120001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:1195: // If true, then the RenderFrame is playing audio. nit: Let's be a little more specific here, to help future developers that might not be intimately familiar with this mechanism. It's important to distinguish the difference between the existence of audio streams versus whether there are audio streams present that are not silent. Suggestion: // If true, then this RenderFrame has one or more audio streams with audible // signal. If false, all audio streams are currently silent (or there are no // audio streams). https://codereview.chromium.org/2948613002/diff/120001/content/browser/media/... File content/browser/media/audio_stream_monitor.cc (right): https://codereview.chromium.org/2948613002/diff/120001/content/browser/media/... content/browser/media/audio_stream_monitor.cc:205: if (!is_audible_ && is_stream_audible) { It doesn't seem like the conditional here should be changed this way: The purpose of this code block is to update the timer that will later cause the UI indicator to turn on or off (via a notification sent from MaybeToggle()). FWICT, this code block needs to run once (and only once) for each call to Poll() while any stream is audible. Side note: Can we get rid of |last_blurt_time_| since nothing needs it? MaybeToggle() could just call clock_->NowTicks() itself. https://codereview.chromium.org/2948613002/diff/120001/content/browser/media/... content/browser/media/audio_stream_monitor.cc:211: // Record whether or not the RenderFrame is audible, a RenderFrame is There are separate sentences here: "audible, a RenderFrame" should be "audible. A RenderFrame" https://codereview.chromium.org/2948613002/diff/120001/content/browser/media/... content/browser/media/audio_stream_monitor.cc:212: // audible when there is a audio stream in it that is audible. nit: s/when there is a audio stream in it/it has an audio stream/ https://codereview.chromium.org/2948613002/diff/120001/content/browser/media/... content/browser/media/audio_stream_monitor.cc:234: void AudioStreamMonitor::MaybeToggle() { Consider renaming this method. Suggestion: MaybeToggleIndicator().
The CQ bit was checked by lpy@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: 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 lpy@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...
Patchset #8 (id:140001) has been deleted
Thanks, I updated the patch. https://codereview.chromium.org/2948613002/diff/120001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2948613002/diff/120001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:1193: GetProcess()->OnAudioStreamAdded(); On 2017/06/26 22:57:59, miu wrote: > The OnAudioStreamAdded/Removed() methods need to be renamed. I'm not sure what > their original purpose was: Do they have any other callers besides this code? > Since they are now being called to report changes in audibility, and not whether > audio streams have actually been created or destroyed, I'd suggest something > like: > > OnRenderFrameBecameAudible() > OnRenderFrameBecameSilent() > I will leave it to separate patches. https://codereview.chromium.org/2948613002/diff/120001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/2948613002/diff/120001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:253: void OnAudioStateChanged(bool is_audible); On 2017/06/26 22:57:59, miu wrote: > naming nit: OnAudibleStateChanged Done. https://codereview.chromium.org/2948613002/diff/120001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:1195: // If true, then the RenderFrame is playing audio. On 2017/06/26 22:57:59, miu wrote: > nit: Let's be a little more specific here, to help future developers that might > not be intimately familiar with this mechanism. It's important to distinguish > the difference between the existence of audio streams versus whether there are > audio streams present that are not silent. Suggestion: > > // If true, then this RenderFrame has one or more audio streams with audible > // signal. If false, all audio streams are currently silent (or there are no > // audio streams). Done. https://codereview.chromium.org/2948613002/diff/120001/content/browser/media/... File content/browser/media/audio_stream_monitor.cc (right): https://codereview.chromium.org/2948613002/diff/120001/content/browser/media/... content/browser/media/audio_stream_monitor.cc:205: if (!is_audible_ && is_stream_audible) { On 2017/06/26 22:57:59, miu wrote: > It doesn't seem like the conditional here should be changed this way: The > purpose of this code block is to update the timer that will later cause the UI > indicator to turn on or off (via a notification sent from MaybeToggle()). FWICT, > this code block needs to run once (and only once) for each call to Poll() while > any stream is audible. I don't think it changes the logic here. It still only calls MaybeToggle() once if there's an audio stream that is audible. > Side note: Can we get rid of |last_blurt_time_| since nothing needs it? > MaybeToggle() could just call clock_->NowTicks() itself. I will leave this to separate patch to clean it up. https://codereview.chromium.org/2948613002/diff/120001/content/browser/media/... content/browser/media/audio_stream_monitor.cc:211: // Record whether or not the RenderFrame is audible, a RenderFrame is On 2017/06/26 22:57:59, miu wrote: > There are separate sentences here: "audible, a RenderFrame" should be "audible. > A RenderFrame" Done. https://codereview.chromium.org/2948613002/diff/120001/content/browser/media/... content/browser/media/audio_stream_monitor.cc:212: // audible when there is a audio stream in it that is audible. On 2017/06/26 22:57:59, miu wrote: > nit: s/when there is a audio stream in it/it has an audio stream/ Done. https://codereview.chromium.org/2948613002/diff/120001/content/browser/media/... content/browser/media/audio_stream_monitor.cc:234: void AudioStreamMonitor::MaybeToggle() { On 2017/06/26 22:57:59, miu wrote: > Consider renaming this method. Suggestion: MaybeToggleIndicator(). I will leave it to separate cleanup patches.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by lpy@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: 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 lpy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org, nasko@chromium.org Link to the patchset: https://codereview.chromium.org/2948613002/#ps180001 (title: "Fix windows build")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1498672909570810,
"parent_rev": "6a96b1c50789ce0f63946137b7c8f1a223699add", "commit_rev":
"5e9ff6090994e9e6b3e63e40436654eb4e968271"}
Message was sent while issue was closed.
Description was changed from ========== [AudioStreamMonitor] Adds API to collect frame-level audibility. This patch creates API to collect frame-level audibility in order to do frame-level throttling and suspension, only sends audio changed state to RenderFrameHost when we don't have power level monitoring or the audible state of a stream changes, and defers to RenderFrameHost to send signals to RenderProcessHost about audio stream added/removed. BUG=731270 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== [AudioStreamMonitor] Adds API to collect frame-level audibility. This patch creates API to collect frame-level audibility in order to do frame-level throttling and suspension, only sends audio changed state to RenderFrameHost when we don't have power level monitoring or the audible state of a stream changes, and defers to RenderFrameHost to send signals to RenderProcessHost about audio stream added/removed. BUG=731270 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2948613002 Cr-Commit-Position: refs/heads/master@{#483075} Committed: https://chromium.googlesource.com/chromium/src/+/5e9ff6090994e9e6b3e63e404366... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:180001) as https://chromium.googlesource.com/chromium/src/+/5e9ff6090994e9e6b3e63e404366... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
