Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2496173003:
Communicate audio state to renderer process on Android. (Closed)
DescriptionCommunicate audio state to renderer process on Android.
Use "have an active stream" signal on Android as a proxy for being
audible in AudioStreamMonitor when monitoring is not available.
BUG=642321
R=dalecurtis@chromium.org
CC=skyostil@chromium.org,alexclarke@chromium.org
Committed: https://crrev.com/ec87fd1fe94540572dc0c09c1f5313920debd371
Cr-Commit-Position: refs/heads/master@{#432950}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Second iteration #
Total comments: 8
Patch Set 3 : Addressed comments from dalecurtis@ and miu@ #Patch Set 4 : Make power_level_monitoring_available private #
Messages
Total messages: 39 (23 generated)
PTAL. This is the first draft of changes discussed in https://codereview.chromium.org/2383473002.
https://codereview.chromium.org/2496173003/diff/1/content/browser/media/audio... File content/browser/media/audio_stream_monitor.cc (left): https://codereview.chromium.org/2496173003/diff/1/content/browser/media/audio... content/browser/media/audio_stream_monitor.cc:57: if (!monitoring_available()) Can we change this to power_level_monitoring() or something else more clear? The comments in the header file also all need updates now. Likely monitoring_available() shouldn't be public anymore. https://codereview.chromium.org/2496173003/diff/1/content/browser/media/audio... File content/browser/media/audio_stream_monitor.cc (right): https://codereview.chromium.org/2496173003/diff/1/content/browser/media/audio... content/browser/media/audio_stream_monitor.cc:87: if (monitor) { Invert to if (!monitor) return to avoid extra nesting. https://codereview.chromium.org/2496173003/diff/1/content/browser/media/audio... content/browser/media/audio_stream_monitor.cc:89: // If audio level monitoring is not available, assume that an active Reflow comment. https://codereview.chromium.org/2496173003/diff/1/content/browser/media/audio... content/browser/media/audio_stream_monitor.cc:108: if (monitor) { Invert to if (!monitor) return to avoid extra nesting.
PTAL https://codereview.chromium.org/2496173003/diff/1/content/browser/media/audio... File content/browser/media/audio_stream_monitor.cc (left): https://codereview.chromium.org/2496173003/diff/1/content/browser/media/audio... content/browser/media/audio_stream_monitor.cc:57: if (!monitoring_available()) On 2016/11/14 18:51:07, DaleCurtis wrote: > Can we change this to power_level_monitoring() or something else more clear? The > comments in the header file also all need updates now. Likely > monitoring_available() shouldn't be public anymore. Done. There is some logic that uses this function to prevent sleep mode when audio is available. Sounds like legitimate usage to me. https://codereview.chromium.org/2496173003/diff/1/content/browser/media/audio... File content/browser/media/audio_stream_monitor.cc (right): https://codereview.chromium.org/2496173003/diff/1/content/browser/media/audio... content/browser/media/audio_stream_monitor.cc:87: if (monitor) { On 2016/11/14 18:51:07, DaleCurtis wrote: > Invert to if (!monitor) return to avoid extra nesting. Done. https://codereview.chromium.org/2496173003/diff/1/content/browser/media/audio... content/browser/media/audio_stream_monitor.cc:89: // If audio level monitoring is not available, assume that an active On 2016/11/14 18:51:07, DaleCurtis wrote: > Reflow comment. Done. https://codereview.chromium.org/2496173003/diff/1/content/browser/media/audio... content/browser/media/audio_stream_monitor.cc:108: if (monitor) { On 2016/11/14 18:51:07, DaleCurtis wrote: > Invert to if (!monitor) return to avoid extra nesting. Done.
The CQ bit was checked by altimin@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.
dalecurtis@chromium.org changed reviewers: + miu@chromium.org
+miu who additional commentary. https://codereview.chromium.org/2496173003/diff/20001/content/browser/media/a... File content/browser/media/audio_stream_monitor.cc (right): https://codereview.chromium.org/2496173003/diff/20001/content/browser/media/a... content/browser/media/audio_stream_monitor.cc:189: ++active_streams_; DCHECK_CURRENTLY_ON() ? https://codereview.chromium.org/2496173003/diff/20001/content/browser/media/m... File content/browser/media/media_web_contents_observer.cc (right): https://codereview.chromium.org/2496173003/diff/20001/content/browser/media/m... content/browser/media/media_web_contents_observer.cc:46: if (!AudioStreamMonitor::power_level_monitoring_available()) I think this can be removed now as well as some of the other pieces below since this code works on the same premise as what you've brought to the stream monitor.
I'm reluctant to l-g-t-m, because I don't understand why this approach was taken to begin with (i.e., why do we depend on the browser to tell the renderer whether audio is being output?). It seems like the renderer should already know that. Elaborated further (with requests to change naming to avoid confusion): https://codereview.chromium.org/2383473002/#msg83 https://codereview.chromium.org/2496173003/diff/20001/content/browser/media/a... File content/browser/media/audio_stream_monitor.h (right): https://codereview.chromium.org/2496173003/diff/20001/content/browser/media/a... content/browser/media/audio_stream_monitor.h:128: // is not available. This can go on the above line.
On 2016/11/15 at 22:06:04, miu wrote: > I'm reluctant to l-g-t-m, because I don't understand why this approach was taken to begin with (i.e., why do we depend on the browser to tell the renderer whether audio is being output?). It seems like the renderer should already know that. > > Elaborated further (with requests to change naming to avoid confusion): https://codereview.chromium.org/2383473002/#msg83 > > https://codereview.chromium.org/2496173003/diff/20001/content/browser/media/a... > File content/browser/media/audio_stream_monitor.h (right): > > https://codereview.chromium.org/2496173003/diff/20001/content/browser/media/a... > content/browser/media/audio_stream_monitor.h:128: // is not available. > This can go on the above line. We don't want silent audio affecting the scheduler in the general case; we allow so for Android because we don't have that information.
On 2016/11/15 22:21:47, DaleCurtis wrote: > We don't want silent audio affecting the scheduler in the general case; we allow > so for Android because we don't have that information. Okay. LGTM, then. Either in this CL or another follow-up, I'd appreciate it if comments from https://codereview.chromium.org/2383473002/#msg83 were addressed.
PTAL https://codereview.chromium.org/2496173003/diff/20001/content/browser/media/a... File content/browser/media/audio_stream_monitor.cc (right): https://codereview.chromium.org/2496173003/diff/20001/content/browser/media/a... content/browser/media/audio_stream_monitor.cc:189: ++active_streams_; On 2016/11/15 19:26:01, DaleCurtis wrote: > DCHECK_CURRENTLY_ON() ? Done. https://codereview.chromium.org/2496173003/diff/20001/content/browser/media/a... File content/browser/media/audio_stream_monitor.h (right): https://codereview.chromium.org/2496173003/diff/20001/content/browser/media/a... content/browser/media/audio_stream_monitor.h:128: // is not available. On 2016/11/15 22:06:04, miu wrote: > This can go on the above line. Done. https://codereview.chromium.org/2496173003/diff/20001/content/browser/media/m... File content/browser/media/media_web_contents_observer.cc (right): https://codereview.chromium.org/2496173003/diff/20001/content/browser/media/m... content/browser/media/media_web_contents_observer.cc:46: if (!AudioStreamMonitor::power_level_monitoring_available()) On 2016/11/15 19:26:01, DaleCurtis wrote: > I think this can be removed now as well as some of the other pieces below since > this code works on the same premise as what you've brought to the stream > monitor. I'm a little hesitant about it — it is really a good idea to prevent Android from entering power save mode when audio track is present? I'm afraid that we will use more power and I'd like an explicit confirmation from someone who knows what's happening here.
altimin@chromium.org changed reviewers: + clamy@chromium.org
+clamy@ for WebContents-related changes (trivial, comments and unittests only).
https://codereview.chromium.org/2496173003/diff/20001/content/browser/media/m... File content/browser/media/media_web_contents_observer.cc (right): https://codereview.chromium.org/2496173003/diff/20001/content/browser/media/m... content/browser/media/media_web_contents_observer.cc:46: if (!AudioStreamMonitor::power_level_monitoring_available()) On 2016/11/16 at 12:52:46, altimin wrote: > On 2016/11/15 19:26:01, DaleCurtis wrote: > > I think this can be removed now as well as some of the other pieces below since > > this code works on the same premise as what you've brought to the stream > > monitor. > > I'm a little hesitant about it — it is really a good idea to prevent Android from entering power save mode when audio track is present? I'm afraid that we will use more power and I'd like an explicit confirmation from someone who knows what's happening here. Sorry if I was unclear, there should be no behavioral change. If there is, something is unexpected. The existing power save blocker should only be configured when an audio stream is playing. Both implementations should now be doing the same thing IIRC. Also I wrote all this, so I'm probably that someone :)
Thanks! Lgtm.
The CQ bit was checked by altimin@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...
PTAL https://codereview.chromium.org/2496173003/diff/20001/content/browser/media/m... File content/browser/media/media_web_contents_observer.cc (right): https://codereview.chromium.org/2496173003/diff/20001/content/browser/media/m... content/browser/media/media_web_contents_observer.cc:46: if (!AudioStreamMonitor::power_level_monitoring_available()) On 2016/11/16 22:16:51, DaleCurtis wrote: > On 2016/11/16 at 12:52:46, altimin wrote: > > On 2016/11/15 19:26:01, DaleCurtis wrote: > > > I think this can be removed now as well as some of the other pieces below > since > > > this code works on the same premise as what you've brought to the stream > > > monitor. > > > > I'm a little hesitant about it — it is really a good idea to prevent Android > from entering power save mode when audio track is present? I'm afraid that we > will use more power and I'd like an explicit confirmation from someone who knows > what's happening here. > > Sorry if I was unclear, there should be no behavioral change. If there is, > something is unexpected. The existing power save blocker should only be > configured when an audio stream is playing. Both implementations should now be > doing the same thing IIRC. Also I wrote all this, so I'm probably that someone > :) Oh, I finally figured it out (previously I thought that power blocker was enabled only on desktop). Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by altimin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org, clamy@chromium.org Link to the patchset: https://codereview.chromium.org/2496173003/#ps60001 (title: "Make power_level_monitoring_available private")
The CQ bit was unchecked by altimin@chromium.org
The CQ bit was checked by altimin@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_asan_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 altimin@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...
lgtm, thanks for the cleanup! We might be able to remove some of the logic in AudioRendererHost now too.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by altimin@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.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Communicate audio state to renderer process on Android. Use "have an active stream" signal on Android as a proxy for being audible in AudioStreamMonitor when monitoring is not available. BUG=642321 R=dalecurtis@chromium.org CC=skyostil@chromium.org,alexclarke@chromium.org ========== to ========== Communicate audio state to renderer process on Android. Use "have an active stream" signal on Android as a proxy for being audible in AudioStreamMonitor when monitoring is not available. BUG=642321 R=dalecurtis@chromium.org CC=skyostil@chromium.org,alexclarke@chromium.org Committed: https://crrev.com/ec87fd1fe94540572dc0c09c1f5313920debd371 Cr-Commit-Position: refs/heads/master@{#432950} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/ec87fd1fe94540572dc0c09c1f5313920debd371 Cr-Commit-Position: refs/heads/master@{#432950} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
