Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2383473002:
[scheduler] Teach scheduler about audio state (Closed)
|
Created:
4 years, 2 months ago by altimin Modified:
4 years, 1 month ago CC:
DaleCurtis, blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, dglazkov+blink, feature-media-reviews_chromium.org, jam, miu+watch_chromium.org, miu, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, posciak+watch_chromium.org, scheduler-bugs_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[scheduler] Teach scheduler about audio state
This patch wires information about audio state to renderer scheduler
and stops all throttling (background and offscreen) while
audio is playing and for a short period after audio is stopped.
BUG=642321, 616519, 656019
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
Committed: https://crrev.com/a7a651d546b76499821b4ba47b8f017d4b8becac
Committed: https://crrev.com/d8bd26c6124f76be5a0c7cfacc39c3996dd29adf
Cr-Original-Commit-Position: refs/heads/master@{#423405}
Cr-Commit-Position: refs/heads/master@{#429861}
Patch Set 1 #
Total comments: 36
Patch Set 2 : Addressed comments from skyostil@ and alexclarke@ #Patch Set 3 : Reverted DEPS #
Total comments: 13
Patch Set 4 : Switched approach to use WebContentsImpl::WasRecentlyAudible #
Total comments: 12
Patch Set 5 : Address comments from skyostil@ #Patch Set 6 : Support out-of-process frames #
Total comments: 2
Patch Set 7 : WebFrames! #
Total comments: 1
Patch Set 8 : Fixed nit for clamy@ #Patch Set 9 : Switched to page messages #
Total comments: 1
Patch Set 10 : constexpr! #Patch Set 11 : Rebased #
Total comments: 6
Messages
Total messages: 84 (47 generated)
Description was changed from ========== [scheduler] Educate scheduler about renderer state This patch wires information about audio state to renderer scheduler and stops all throttling (including background and offscreen) while audio is playing and for a short period after audio stopped. BUG=642321,616519 ========== to ========== [scheduler] Educate scheduler about audio state This patch wires information about audio state to renderer scheduler and stops all throttling (including background and offscreen) while audio is playing and for a short period after audio stopped. BUG=642321,616519 ==========
altimin@chromium.org changed reviewers: + alexclarke@chromium.org, skyostil@chromium.org
Description was changed from ========== [scheduler] Educate scheduler about audio state This patch wires information about audio state to renderer scheduler and stops all throttling (including background and offscreen) while audio is playing and for a short period after audio stopped. BUG=642321,616519 ========== to ========== [scheduler] Educate scheduler about audio state This patch wires information about audio state to renderer scheduler and stops all throttling (background and offscreen) while audio is playing and for a short period after audio is stopped. BUG=642321,616519 ==========
PTAL
https://codereview.chromium.org/2383473002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/base/delayed_switch.cc (right): https://codereview.chromium.org/2383473002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/base/delayed_switch.cc:32: delayed_disable_task_.Cancel(); DCHECK(task_runner_->RunsTasksOnCurrentThread()); https://codereview.chromium.org/2383473002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/base/delayed_switch.cc:44: if (state_ == State::DISABLED) DCHECK(task_runner_->RunsTasksOnCurrentThread()); https://codereview.chromium.org/2383473002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/base/delayed_switch.cc:56: return state_ == State::ENABLED; DCHECK(task_runner_->RunsTasksOnCurrentThread()); https://codereview.chromium.org/2383473002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/base/delayed_switch.h (right): https://codereview.chromium.org/2383473002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/base/delayed_switch.h:29: base::TaskRunner* task_runner, Any particular reason for using base::TaskRunner instead of a TaskQueue? https://codereview.chromium.org/2383473002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/base/delayed_switch.h:35: void Enable(); In some ways I like Enable/Disable but the the content/ layer has a bool as does the TaskQueueThrottler. Feels a little odd. https://codereview.chromium.org/2383473002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/base/delayed_switch.h:51: base::TaskRunner* task_runner_; // NOT OWNED This should be a scoped_refptr<> https://codereview.chromium.org/2383473002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/base/delayed_switch.h:62: }; DISALLOW_COPY_AND_ASSIGN(DelayedSwitch); https://codereview.chromium.org/2383473002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2383473002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1220: void RendererSchedulerImpl::OnAudioStartedImpl() { I don't think we need these two functions, why not bind task_queue_throttler()->SetIsAudioActive directly? https://codereview.chromium.org/2383473002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h (right): https://codereview.chromium.org/2383473002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:240: void UpdateGlobalThrottlingSetting(); Sami how do you feel about this vs UpdatePolicy? Maybe it's OK for this to be separate but usually we'd have triggered a policy update to deal with state changes.
https://codereview.chromium.org/2383473002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/2383473002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/audio_renderer_host.cc:79: bool is_audio_active) { nit: also call this is_playing? (elsewhere too) https://codereview.chromium.org/2383473002/diff/1/content/common/child_proces... File content/common/child_process_messages.h (right): https://codereview.chromium.org/2383473002/diff/1/content/common/child_proces... content/common/child_process_messages.h:126: IPC_MESSAGE_CONTROL1(ChildProcessMsg_SetAudioPlaying, bikeshed: SetProcessAudible would make it a little more obvious that it's *this* process that is playing audio. I think we use "audible" elsewhere for this purpose too. https://codereview.chromium.org/2383473002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/base/delayed_switch.cc (right): https://codereview.chromium.org/2383473002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/base/delayed_switch.cc:62: DCHECK_EQ(State::ENABLED, state_); Does this work? If so, please mark https://bugs.chromium.org/p/chromium/issues/detail?id=463869 as fixed :) https://codereview.chromium.org/2383473002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/base/delayed_switch.h (right): https://codereview.chromium.org/2383473002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/base/delayed_switch.h:26: class BLINK_PLATFORM_EXPORT DelayedSwitch { bikeshed: DebouncedSwitch might be a more accurate name for this because the enable isn't delayed. https://codereview.chromium.org/2383473002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/base/delayed_switch.h:51: base::TaskRunner* task_runner_; // NOT OWNED On 2016/09/29 11:16:53, alex clarke wrote: > This should be a scoped_refptr<> +1, also pass it in as one in the constructor to make it clear ownership is taken. https://codereview.chromium.org/2383473002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2383473002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:171: base::Unretained(renderer_scheduler_impl)), Could we instead change things so that we can get a weak pointer here? We could construct the factory earlier for instance. https://codereview.chromium.org/2383473002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:494: MainThreadOnly().audio_playback_switch.Enable(); These could use trace events I think. https://codereview.chromium.org/2383473002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h (right): https://codereview.chromium.org/2383473002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:164: void SetIsAudioActive(bool is_audio_active); I don't think TaskQueueThrottler should know about audio. This feels more like a RendererScheduler type of thing. https://codereview.chromium.org/2383473002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:240: void UpdateGlobalThrottlingSetting(); On 2016/09/29 11:16:53, alex clarke wrote: > Sami how do you feel about this vs UpdatePolicy? Maybe it's OK for this to be > separate but usually we'd have triggered a policy update to deal with state > changes. Yeah I think we want to cause a policy update instead. I could see us needing the audio state for setting task queue priorities etc. in the future. https://codereview.chromium.org/2383473002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:252: bool is_throttling_disabled_; Negative flags hurt my head -- how about allow_throttling_?
https://codereview.chromium.org/2383473002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h (right): https://codereview.chromium.org/2383473002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:240: void UpdateGlobalThrottlingSetting(); On 2016/09/29 13:36:07, Sami wrote: > On 2016/09/29 11:16:53, alex clarke wrote: > > Sami how do you feel about this vs UpdatePolicy? Maybe it's OK for this to be > > separate but usually we'd have triggered a policy update to deal with state > > changes. > > Yeah I think we want to cause a policy update instead. I could see us needing > the audio state for setting task queue priorities etc. in the future. Thinking about this more I'd suggest instead of adding DelayedSwitch we track the last time audio was played in RendererSchedulerImpl::MainThreadOnly Change |new_policy_duration| to be the min of last audio play time + kThrottlingDelayAfterAudioIsPlayedInSeconds and |expected_use_case_duration|. SetIsAudioActive should update the last audio play time and call MaybeUpdatePolicy.
Description was changed from ========== [scheduler] Educate scheduler about audio state This patch wires information about audio state to renderer scheduler and stops all throttling (background and offscreen) while audio is playing and for a short period after audio is stopped. BUG=642321,616519 ========== to ========== [scheduler] Teach scheduler about audio state This patch wires information about audio state to renderer scheduler and stops all throttling (background and offscreen) while audio is playing and for a short period after audio is stopped. BUG=642321,616519 ==========
PTAL https://codereview.chromium.org/2383473002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/2383473002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/audio_renderer_host.cc:79: bool is_audio_active) { On 2016/09/29 13:36:07, Sami wrote: > nit: also call this is_playing? (elsewhere too) Done. https://codereview.chromium.org/2383473002/diff/1/content/common/child_proces... File content/common/child_process_messages.h (right): https://codereview.chromium.org/2383473002/diff/1/content/common/child_proces... content/common/child_process_messages.h:126: IPC_MESSAGE_CONTROL1(ChildProcessMsg_SetAudioPlaying, On 2016/09/29 13:36:07, Sami wrote: > bikeshed: SetProcessAudible would make it a little more obvious that it's *this* > process that is playing audio. I think we use "audible" elsewhere for this > purpose too. Done. https://codereview.chromium.org/2383473002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/base/delayed_switch.cc (right): https://codereview.chromium.org/2383473002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/base/delayed_switch.cc:32: delayed_disable_task_.Cancel(); On 2016/09/29 11:16:53, alex clarke wrote: > DCHECK(task_runner_->RunsTasksOnCurrentThread()); Acknowledged. https://codereview.chromium.org/2383473002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/base/delayed_switch.cc:44: if (state_ == State::DISABLED) On 2016/09/29 11:16:53, alex clarke wrote: > DCHECK(task_runner_->RunsTasksOnCurrentThread()); Acknowledged. https://codereview.chromium.org/2383473002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/base/delayed_switch.cc:56: return state_ == State::ENABLED; On 2016/09/29 11:16:53, alex clarke wrote: > DCHECK(task_runner_->RunsTasksOnCurrentThread()); Acknowledged. https://codereview.chromium.org/2383473002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/base/delayed_switch.cc:62: DCHECK_EQ(State::ENABLED, state_); On 2016/09/29 13:36:07, Sami wrote: > Does this work? If so, please mark > https://bugs.chromium.org/p/chromium/issues/detail?id=463869 as fixed :) Done. https://codereview.chromium.org/2383473002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/base/delayed_switch.h (right): https://codereview.chromium.org/2383473002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/base/delayed_switch.h:26: class BLINK_PLATFORM_EXPORT DelayedSwitch { On 2016/09/29 13:36:07, Sami wrote: > bikeshed: DebouncedSwitch might be a more accurate name for this because the > enable isn't delayed. Acknowledged. https://codereview.chromium.org/2383473002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/base/delayed_switch.h:29: base::TaskRunner* task_runner, On 2016/09/29 11:16:53, alex clarke wrote: > Any particular reason for using base::TaskRunner instead of a TaskQueue? Acknowledged. https://codereview.chromium.org/2383473002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/base/delayed_switch.h:35: void Enable(); On 2016/09/29 11:16:53, alex clarke wrote: > In some ways I like Enable/Disable but the the content/ layer has a bool as does > the TaskQueueThrottler. Feels a little odd. Done. https://codereview.chromium.org/2383473002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/base/delayed_switch.h:51: base::TaskRunner* task_runner_; // NOT OWNED On 2016/09/29 13:36:07, Sami wrote: > On 2016/09/29 11:16:53, alex clarke wrote: > > This should be a scoped_refptr<> > > +1, also pass it in as one in the constructor to make it clear ownership is > taken. Acknowledged. https://codereview.chromium.org/2383473002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/base/delayed_switch.h:62: }; On 2016/09/29 11:16:53, alex clarke wrote: > DISALLOW_COPY_AND_ASSIGN(DelayedSwitch); Acknowledged. https://codereview.chromium.org/2383473002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2383473002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:171: base::Unretained(renderer_scheduler_impl)), On 2016/09/29 13:36:07, Sami wrote: > Could we instead change things so that we can get a weak pointer here? We could > construct the factory earlier for instance. Note: per Chromium style guide weak pointer factory should be last member of the class and you will get a lot of complains from compiler. https://codereview.chromium.org/2383473002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1220: void RendererSchedulerImpl::OnAudioStartedImpl() { On 2016/09/29 11:16:53, alex clarke wrote: > I don't think we need these two functions, why not bind > task_queue_throttler()->SetIsAudioActive directly? Done. https://codereview.chromium.org/2383473002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h (right): https://codereview.chromium.org/2383473002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:164: void SetIsAudioActive(bool is_audio_active); On 2016/09/29 13:36:07, Sami wrote: > I don't think TaskQueueThrottler should know about audio. This feels more like a > RendererScheduler type of thing. Done. https://codereview.chromium.org/2383473002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:240: void UpdateGlobalThrottlingSetting(); On 2016/09/29 13:46:18, alex clarke wrote: > On 2016/09/29 13:36:07, Sami wrote: > > On 2016/09/29 11:16:53, alex clarke wrote: > > > Sami how do you feel about this vs UpdatePolicy? Maybe it's OK for this to > be > > > separate but usually we'd have triggered a policy update to deal with state > > > changes. > > > > Yeah I think we want to cause a policy update instead. I could see us needing > > the audio state for setting task queue priorities etc. in the future. > > Thinking about this more I'd suggest instead of adding DelayedSwitch we track > the last time audio was played in RendererSchedulerImpl::MainThreadOnly > > Change |new_policy_duration| to be the min of last audio play time + > kThrottlingDelayAfterAudioIsPlayedInSeconds and |expected_use_case_duration|. > > SetIsAudioActive should update the last audio play time and call > MaybeUpdatePolicy. > Done. https://codereview.chromium.org/2383473002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:252: bool is_throttling_disabled_; On 2016/09/29 13:36:07, Sami wrote: > Negative flags hurt my head -- how about allow_throttling_? Done.
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...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
Keep in mind this will prevent throttling even for silent audio streams. To avoid this you'll need to plumb an extra bit from WebContentsImpl::WasRecentlyAudible().
That's much simpler now, thanks. Scheduler LGTM https://codereview.chromium.org/2383473002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2383473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:822: if (MainThreadOnly().last_audio_state_change && Please add a brief comment outlining why we do this. https://codereview.chromium.org/2383473002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h (right): https://codereview.chromium.org/2383473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:163: // Tells the TaskQueueThrottler we're using virtual time, which disables all Remove out of date comment.
On 2016/09/29 17:26:35, DaleCurtis_OOO_Until_Oct_18 wrote: > Keep in mind this will prevent throttling even for silent audio streams. To > avoid this you'll need to plumb an extra bit from > WebContentsImpl::WasRecentlyAudible(). +1 please do this.
Could we also do WebContentsImpl::WasRecentlyAudible() as part of this patch? https://codereview.chromium.org/2383473002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2383473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:996: ShouldDisableThrottlingBecauseOfAudio(now) || By the way, this means that we might get more scroll jank on pages that play audio, but those are most likely pretty rare. https://codereview.chromium.org/2383473002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h (right): https://codereview.chromium.org/2383473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:168: void DisableThrottling(); These two need a unit test or a couple I think. https://codereview.chromium.org/2383473002/diff/40001/third_party/WebKit/publ... File third_party/WebKit/public/platform/scheduler/renderer/renderer_scheduler.h (right): https://codereview.chromium.org/2383473002/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/platform/scheduler/renderer/renderer_scheduler.h:132: // Must be called on main thread. nit: move to previous line. https://codereview.chromium.org/2383473002/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/platform/scheduler/renderer/renderer_scheduler.h:133: virtual void OnAudioStateChanged(bool is_audio_active) = 0; nit: is_playing for consistency with the impls (or the other way around).
dalecurtis@chromium.org changed reviewers: + dalecurtis@chromium.org
cc:miu since I'll be OOO in case any review questions come up about WasRecentlyAudible.
dalecurtis@chromium.org changed reviewers: - dalecurtis@chromium.org
Description was changed from ========== [scheduler] Teach scheduler about audio state This patch wires information about audio state to renderer scheduler and stops all throttling (background and offscreen) while audio is playing and for a short period after audio is stopped. BUG=642321,616519 ========== to ========== [scheduler] Teach scheduler about audio state This patch wires information about audio state to renderer scheduler and stops all throttling (background and offscreen) while audio is playing and for a short period after audio is stopped. BUG=642321,616519 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
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/2383473002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2383473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:822: if (MainThreadOnly().last_audio_state_change && On 2016/09/30 10:33:46, alex clarke wrote: > Please add a brief comment outlining why we do this. Done. https://codereview.chromium.org/2383473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:996: ShouldDisableThrottlingBecauseOfAudio(now) || On 2016/09/30 11:52:09, Sami wrote: > By the way, this means that we might get more scroll jank on pages that play > audio, but those are most likely pretty rare. How are throttling and scroll jank related? Are you talking about offscreen iframes throttling? https://codereview.chromium.org/2383473002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h (right): https://codereview.chromium.org/2383473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:163: // Tells the TaskQueueThrottler we're using virtual time, which disables all On 2016/09/30 10:33:46, alex clarke wrote: > Remove out of date comment. Done. https://codereview.chromium.org/2383473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:168: void DisableThrottling(); On 2016/09/30 11:52:09, Sami wrote: > These two need a unit test or a couple I think. Done. https://codereview.chromium.org/2383473002/diff/40001/third_party/WebKit/publ... File third_party/WebKit/public/platform/scheduler/renderer/renderer_scheduler.h (right): https://codereview.chromium.org/2383473002/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/platform/scheduler/renderer/renderer_scheduler.h:132: // Must be called on main thread. On 2016/09/30 11:52:09, Sami wrote: > nit: move to previous line. Acknowledged. https://codereview.chromium.org/2383473002/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/platform/scheduler/renderer/renderer_scheduler.h:133: virtual void OnAudioStateChanged(bool is_audio_active) = 0; On 2016/09/30 11:52:09, Sami wrote: > nit: is_playing for consistency with the impls (or the other way around). Acknowledged.
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_...)
altimin@chromium.org changed reviewers: + clamy@chromium.org, dcheng@chromium.org
+clamy@ for content/ changes +dcheng@ for IPC and non-scheduler blink changes PTAL
lgtm with a couple of nits. https://codereview.chromium.org/2383473002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2383473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:996: ShouldDisableThrottlingBecauseOfAudio(now) || On 2016/10/03 11:15:25, altimin wrote: > On 2016/09/30 11:52:09, Sami wrote: > > By the way, this means that we might get more scroll jank on pages that play > > audio, but those are most likely pretty rare. > > How are throttling and scroll jank related? Are you talking about offscreen > iframes throttling? Not just visibility-based throttling. Previously we would always throttle, e.g., long timer tasks when the user was trying to scroll. With this change we no longer do that on pages that play audio. Things like games might be impacted, but then again we already try to be very careful about throttling anything when there is complicated input handling going on. https://codereview.chromium.org/2383473002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h (right): https://codereview.chromium.org/2383473002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h:400: base::TimeDelta throttling_delay_after_audio_is_played; const? https://codereview.chromium.org/2383473002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h:420: bool is_audio_playing; Is this being initialized? Also, consider adding this to the trace state dump too. https://codereview.chromium.org/2383473002/diff/60001/third_party/WebKit/publ... File third_party/WebKit/public/web/WebView.h (right): https://codereview.chromium.org/2383473002/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/web/WebView.h:360: // Notifies WebView about audio is started or stopped. s/about/when/? https://codereview.chromium.org/2383473002/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/web/WebView.h:361: virtual void audioStateChanged(bool isAudioPlaying) = 0; nit: Could we be consistent about isAudioPlaying/isPlayingAudio?-)
https://codereview.chromium.org/2383473002/diff/60001/cc/test/ordered_simple_... File cc/test/ordered_simple_task_runner.cc (right): https://codereview.chromium.org/2383473002/diff/60001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.cc:252: if (advance_now_ && now_src_->NowTicks() < time) { Not sure why this change is needed?
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/2383473002/diff/60001/cc/test/ordered_simple_... File cc/test/ordered_simple_task_runner.cc (right): https://codereview.chromium.org/2383473002/diff/60001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.cc:252: if (advance_now_ && now_src_->NowTicks() < time) { On 2016/10/03 14:37:30, Sami wrote: > Not sure why this change is needed? tl;dr: Without this change new test does not work (RunUntilTime does not advance time). The problem is that currently time advance is stopped when we have any pending task. Guessing from tests, the initial idea was not to advance time when task limit was reached, fixed to do so. https://codereview.chromium.org/2383473002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h (right): https://codereview.chromium.org/2383473002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h:400: base::TimeDelta throttling_delay_after_audio_is_played; On 2016/10/03 14:36:32, Sami wrote: > const? Done. https://codereview.chromium.org/2383473002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h:420: bool is_audio_playing; On 2016/10/03 14:36:32, Sami wrote: > Is this being initialized? > > Also, consider adding this to the trace state dump too. Done. https://codereview.chromium.org/2383473002/diff/60001/third_party/WebKit/publ... File third_party/WebKit/public/web/WebView.h (right): https://codereview.chromium.org/2383473002/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/web/WebView.h:360: // Notifies WebView about audio is started or stopped. On 2016/10/03 14:36:32, Sami wrote: > s/about/when/? Done. https://codereview.chromium.org/2383473002/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/web/WebView.h:361: virtual void audioStateChanged(bool isAudioPlaying) = 0; On 2016/10/03 14:36:32, Sami wrote: > nit: Could we be consistent about isAudioPlaying/isPlayingAudio?-) Sorry, it was the last typo in WebSchedulerImpl.
lgtm. https://codereview.chromium.org/2383473002/diff/60001/cc/test/ordered_simple_... File cc/test/ordered_simple_task_runner.cc (right): https://codereview.chromium.org/2383473002/diff/60001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.cc:252: if (advance_now_ && now_src_->NowTicks() < time) { On 2016/10/03 15:53:38, altimin wrote: > On 2016/10/03 14:37:30, Sami wrote: > > Not sure why this change is needed? > > tl;dr: Without this change new test does not work (RunUntilTime does not advance > time). > > The problem is that currently time advance is stopped when we have any pending > task. > Guessing from tests, the initial idea was not to advance time when task limit > was reached, fixed to do so. Yeah okay, I think it seems a little weird to not advance time if the task limit is reached.
https://codereview.chromium.org/2383473002/diff/60001/cc/test/ordered_simple_... File cc/test/ordered_simple_task_runner.cc (right): https://codereview.chromium.org/2383473002/diff/60001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.cc:252: if (advance_now_ && now_src_->NowTicks() < time) { On 2016/10/03 16:25:29, Sami wrote: > On 2016/10/03 15:53:38, altimin wrote: > > On 2016/10/03 14:37:30, Sami wrote: > > > Not sure why this change is needed? > > > > tl;dr: Without this change new test does not work (RunUntilTime does not > advance > > time). > > > > The problem is that currently time advance is stopped when we have any pending > > task. > > Guessing from tests, the initial idea was not to advance time when task limit > > was reached, fixed to do so. > > Yeah okay, I think it seems a little weird to not advance time if the task limit > is reached. It kinda makes sense: please take a look at RunUntilTimeout test in [1]. [1] https://cs.chromium.org/chromium/src/cc/test/ordered_simple_task_runner_unitt...
How should this work with OOPIF? Right now, this only unthrottles one process (the one the main frame's WebView happens to live in)
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 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
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks! As explained in the comment, I think the render_view_messages may not be the most appropriate ones to support OOPIF. Appart from that, the code looks mostly good. https://codereview.chromium.org/2383473002/diff/100001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2383473002/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:885: int WebContentsImpl::SendToAllViews(IPC::Message* message) { I think it would make a lot more sense to have the ViewMsg_AudioStateChanged become a FrameMsg_AudioStateChanged rather than add this function. On top of that it is a bit misleading since it may potentially send the same IPC multiple times to the same view (if several frames share the same view).
The CQ bit was checked by altimin@chromium.org to run a CQ dry run
PTAL https://codereview.chromium.org/2383473002/diff/100001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2383473002/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:885: int WebContentsImpl::SendToAllViews(IPC::Message* message) { On 2016/10/05 12:11:35, clamy wrote: > I think it would make a lot more sense to have the ViewMsg_AudioStateChanged > become a FrameMsg_AudioStateChanged rather than add this function. On top of > that it is a bit misleading since it may potentially send the same IPC multiple > times to the same view (if several frames share the same view). Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! content/ lgtm with a nit. https://codereview.chromium.org/2383473002/diff/120001/content/public/browser... File content/public/browser/web_contents.h (right): https://codereview.chromium.org/2383473002/diff/120001/content/public/browser... content/public/browser/web_contents.h:410: // Method for notifying WebContents that audio started or stopped being nit:s/Method for notifying WebContents/Notifies the WebContent
Started to use PageMsg as discussed with dcheng@. PTAL.
lgtm https://codereview.chromium.org/2383473002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h (right): https://codereview.chromium.org/2383473002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h:400: const base::TimeDelta throttling_delay_after_audio_is_played; Nit: this can be a constexpr in the .cc (also naming with kConstantStyle naming)
The CQ bit was checked by altimin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alexclarke@chromium.org, skyostil@chromium.org, clamy@chromium.org Link to the patchset: https://codereview.chromium.org/2383473002/#ps160001 (title: "Switched to page messages")
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 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 altimin@chromium.org
The CQ bit was checked by altimin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from clamy@chromium.org, skyostil@chromium.org, dcheng@chromium.org, alexclarke@chromium.org Link to the patchset: https://codereview.chromium.org/2383473002/#ps180001 (title: "constexpr!")
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 #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== [scheduler] Teach scheduler about audio state This patch wires information about audio state to renderer scheduler and stops all throttling (background and offscreen) while audio is playing and for a short period after audio is stopped. BUG=642321,616519 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== [scheduler] Teach scheduler about audio state This patch wires information about audio state to renderer scheduler and stops all throttling (background and offscreen) while audio is playing and for a short period after audio is stopped. BUG=642321,616519 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel Committed: https://crrev.com/a7a651d546b76499821b4ba47b8f017d4b8becac Cr-Commit-Position: refs/heads/master@{#423405} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/a7a651d546b76499821b4ba47b8f017d4b8becac Cr-Commit-Position: refs/heads/master@{#423405}
Message was sent while issue was closed.
dalecurtis@chromium.org changed reviewers: + dalecurtis@chromium.org
Message was sent while issue was closed.
AudioStreamMonitor is not enabled on Android, so I think this is not working as intended on Android. We need to either enable stream monitoring (may have power concerns) or rely on the AudioRendererHost::HasActiveAudio() signal on that platform.
Message was sent while issue was closed.
Description was changed from ========== [scheduler] Teach scheduler about audio state This patch wires information about audio state to renderer scheduler and stops all throttling (background and offscreen) while audio is playing and for a short period after audio is stopped. BUG=642321,616519 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel Committed: https://crrev.com/a7a651d546b76499821b4ba47b8f017d4b8becac Cr-Commit-Position: refs/heads/master@{#423405} ========== to ========== [scheduler] Teach scheduler about audio state This patch wires information about audio state to renderer scheduler and stops all throttling (background and offscreen) while audio is playing and for a short period after audio is stopped. BUG=642321,616519 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel Committed: https://crrev.com/a7a651d546b76499821b4ba47b8f017d4b8becac Cr-Commit-Position: refs/heads/master@{#423405} ==========
On 2016/10/18 22:15:32, DaleCurtis wrote: > AudioStreamMonitor is not enabled on Android, so I think this is not working as > intended on Android. We need to either enable stream monitoring (may have power > concerns) or rely on the AudioRendererHost::HasActiveAudio() signal on that > platform. Thank you for pointing this out! But given that this bug is more important on desktops, I will try to fix crashed and land this patch first and address your comment in the next patch.
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...
Description was changed from ========== [scheduler] Teach scheduler about audio state This patch wires information about audio state to renderer scheduler and stops all throttling (background and offscreen) while audio is playing and for a short period after audio is stopped. BUG=642321,616519 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel Committed: https://crrev.com/a7a651d546b76499821b4ba47b8f017d4b8becac Cr-Commit-Position: refs/heads/master@{#423405} ========== to ========== [scheduler] Teach scheduler about audio state This patch wires information about audio state to renderer scheduler and stops all throttling (background and offscreen) while audio is playing and for a short period after audio is stopped. BUG=642321,616519, 656019 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel Committed: https://crrev.com/a7a651d546b76499821b4ba47b8f017d4b8becac Cr-Commit-Position: refs/heads/master@{#423405} ==========
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
The patchset sent to the CQ was uploaded after l-g-t-m from clamy@chromium.org, alexclarke@chromium.org, dcheng@chromium.org, skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/2383473002/#ps200001 (title: "Rebased")
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 ========== [scheduler] Teach scheduler about audio state This patch wires information about audio state to renderer scheduler and stops all throttling (background and offscreen) while audio is playing and for a short period after audio is stopped. BUG=642321,616519, 656019 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel Committed: https://crrev.com/a7a651d546b76499821b4ba47b8f017d4b8becac Cr-Commit-Position: refs/heads/master@{#423405} ========== to ========== [scheduler] Teach scheduler about audio state This patch wires information about audio state to renderer scheduler and stops all throttling (background and offscreen) while audio is playing and for a short period after audio is stopped. BUG=642321,616519, 656019 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel Committed: https://crrev.com/a7a651d546b76499821b4ba47b8f017d4b8becac Cr-Commit-Position: refs/heads/master@{#423405} ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== [scheduler] Teach scheduler about audio state This patch wires information about audio state to renderer scheduler and stops all throttling (background and offscreen) while audio is playing and for a short period after audio is stopped. BUG=642321,616519, 656019 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel Committed: https://crrev.com/a7a651d546b76499821b4ba47b8f017d4b8becac Cr-Commit-Position: refs/heads/master@{#423405} ========== to ========== [scheduler] Teach scheduler about audio state This patch wires information about audio state to renderer scheduler and stops all throttling (background and offscreen) while audio is playing and for a short period after audio is stopped. BUG=642321,616519, 656019 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel Committed: https://crrev.com/a7a651d546b76499821b4ba47b8f017d4b8becac Committed: https://crrev.com/d8bd26c6124f76be5a0c7cfacc39c3996dd29adf Cr-Original-Commit-Position: refs/heads/master@{#423405} Cr-Commit-Position: refs/heads/master@{#429861} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/d8bd26c6124f76be5a0c7cfacc39c3996dd29adf Cr-Commit-Position: refs/heads/master@{#429861}
Message was sent while issue was closed.
A revert of this CL (patchset #11 id:200001) has been created in https://codereview.chromium.org/2483843002/ by magjed@chromium.org. The reason for reverting is: This CL breaks chromium.webrtc browser_tests on all platforms with the following failure: 5 tests timed out: WebRtcPerfBrowserTest.MANUAL_RunsAudioVideoCall60SecsAndLogsInternalMetricsH264 (../../chrome/browser/media/webrtc/webrtc_perf_browsertest.cc:242) WebRtcPerfBrowserTest.MANUAL_RunsAudioVideoCall60SecsAndLogsInternalMetricsVp8 (../../chrome/browser/media/webrtc/webrtc_perf_browsertest.cc:228) WebRtcPerfBrowserTest.MANUAL_RunsAudioVideoCall60SecsAndLogsInternalMetricsVp9 (../../chrome/browser/media/webrtc/webrtc_perf_browsertest.cc:234) WebRtcPerfBrowserTest.MANUAL_RunsOneWayCall60SecsAndLogsInternalMetricsDefault (../../chrome/browser/media/webrtc/webrtc_perf_browsertest.cc:257) WebRtcPerfBrowserTest.MANUAL_RunsOneWayCall60SecsAndLogsInternalMetricsWithOpusDtx (../../chrome/browser/media/webrtc/webrtc_perf_browsertest.cc:263) See for example https://build.chromium.org/p/chromium.webrtc/builders/Linux%20Tester/builds/2....
Message was sent while issue was closed.
miu@chromium.org changed reviewers: + miu@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2383473002/diff/200001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2383473002/diff/200001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:1322: // Notification for UI updates in response to the changed muting state. nit: The wording here could be confusing. There are two concepts in the UI: 1) Whether an audio stream is audible (i.e., not silent); and 2) Whether an audio stream has been muted (i.e., user or extension API controls were used to mute audio stream). Can you fix this? https://codereview.chromium.org/2383473002/diff/200001/content/common/page_me... File content/common/page_messages.h (right): https://codereview.chromium.org/2383473002/diff/200001/content/common/page_me... content/common/page_messages.h:46: IPC_MESSAGE_ROUTED1(PageMsg_AudioStateChanged, bool /* is_audio_playing */) OOC, why are we doing this whole "round about" approach: send sound to the browser process, then have it do audible detection (takes time), and finally send the renderer back a message to determine whether it should disable throttling? Instead, it seems like anything that causes audio to be generated should directly call DisableThrottling() (e.g., from HTMLMediaElement.cpp, the WebAudio modules, etc.). Then, correct behavior would not be dependent on the browser process. https://codereview.chromium.org/2383473002/diff/200001/content/public/browser... File content/public/browser/web_contents.h (right): https://codereview.chromium.org/2383473002/diff/200001/content/public/browser... content/public/browser/web_contents.h:401: virtual void OnAudioStateChanged(bool is_audio_playing) = 0; Throughout this change "playing" is inaccurate. "playing" means samples are being delivered to the OS sound system. The audio samples may represent silent or audible sound. So, can you please fix the naming to "audible" everywhere?
Message was sent while issue was closed.
https://codereview.chromium.org/2383473002/diff/200001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2383473002/diff/200001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:1322: // Notification for UI updates in response to the changed muting state. On 2016/11/15 22:04:04, miu wrote: > nit: The wording here could be confusing. There are two concepts in the UI: 1) > Whether an audio stream is audible (i.e., not silent); and 2) Whether an audio > stream has been muted (i.e., user or extension API controls were used to mute > audio stream). Can you fix this? Addressed in crrev.com/2496173003 https://codereview.chromium.org/2383473002/diff/200001/content/common/page_me... File content/common/page_messages.h (right): https://codereview.chromium.org/2383473002/diff/200001/content/common/page_me... content/common/page_messages.h:46: IPC_MESSAGE_ROUTED1(PageMsg_AudioStateChanged, bool /* is_audio_playing */) On 2016/11/15 22:04:04, miu wrote: > OOC, why are we doing this whole "round about" approach: send sound to the > browser process, then have it do audible detection (takes time), and finally > send the renderer back a message to determine whether it should disable > throttling? > > Instead, it seems like anything that causes audio to be generated should > directly call DisableThrottling() (e.g., from HTMLMediaElement.cpp, the WebAudio > modules, etc.). Then, correct behavior would not be dependent on the browser > process. Because we do not want to disable throttling when page is not audible. Also this approach makes easy to explain when we avoid throttling -- the rules are the same as for displaying sound notification in browser UI. (Also controlling scheduler settings is not uncommon - for example, foreground/background page stage is forwarded to the scheduler via browser process). If you have further questions, let's continue discussion in crrev.com/2496173003. https://codereview.chromium.org/2383473002/diff/200001/content/public/browser... File content/public/browser/web_contents.h (right): https://codereview.chromium.org/2383473002/diff/200001/content/public/browser... content/public/browser/web_contents.h:401: virtual void OnAudioStateChanged(bool is_audio_playing) = 0; On 2016/11/15 22:04:04, miu wrote: > Throughout this change "playing" is inaccurate. "playing" means samples are > being delivered to the OS sound system. The audio samples may represent silent > or audible sound. So, can you please fix the naming to "audible" everywhere? Addressed in crrev.com/2496173003 |
