Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
562273008:
Add audio signal to the ResourceScheduler. (Closed)
|
Created:
6 years, 3 months ago by aiolos (Not reviewing) Modified:
6 years, 2 months ago CC:
chromium-reviews, posciak+watch_chromium.org, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, wjia+watch_chromium.org, miu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAdd audio signal to the ResourceScheduler.
BUG=128035
Committed: https://crrev.com/6e7ce491a74481f8e3d7293ebb0ae58d62564526
Cr-Commit-Position: refs/heads/master@{#299029}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Addressed comments. #
Total comments: 4
Patch Set 3 : Use render_view level HasActiveAudio instead of process level. #
Total comments: 8
Patch Set 4 : nits from Dale. #
Total comments: 9
Patch Set 5 : Nits from Matt #
Total comments: 13
Patch Set 6 : Fix trybot error. #
Total comments: 3
Patch Set 7 : Fixed error, added start state and updated unittests. #Patch Set 8 : Rebase #Patch Set 9 : Fix. #
Total comments: 2
Patch Set 10 : #Patch Set 11 : Dale comment. #
Total comments: 6
Patch Set 12 : Change for Testing #Patch Set 13 : Rebase #
Total comments: 3
Patch Set 14 : More Dale comments. #Patch Set 15 : Use bool instead of null pointer. #Patch Set 16 : Set playing to correct value. #Patch Set 17 : Must still check for NULL AudioRenderHost #
Total comments: 1
Patch Set 18 : Naming nit. #
Total comments: 1
Patch Set 19 : Rebase #Messages
Total messages: 58 (3 generated)
aiolos@chromium.org changed reviewers: + dalecurtis@chromium.org, mmenke@chromium.org
I considered putting the hook for this into the AudioStreamMonitor itself, but was wary of unforeseen issues caused by cutting off the resource loading of tabs with playing but non-audible audio streams. Feedback on this is welcome. https://codereview.chromium.org/562273008/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/562273008/diff/1/content/browser/renderer_hos... content/browser/renderer_host/media/audio_renderer_host.cc:283: // TODO(dalecurtis): See about using AudioStreamMonitor instead. Dale, I wasn't quite sure where this TODO should go. Should I move it?
https://codereview.chromium.org/562273008/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/562273008/diff/1/content/browser/renderer_hos... content/browser/renderer_host/media/audio_renderer_host.cc:282: if (entry->playing() != is_playing) { Suggest making a method: AudioRendererHost::UpdatePlayingStreams(entry, is_playing) or somesuch, and move this entire code block there (including the initial if statement). Then can use it in DeleteEntry, too. https://codereview.chromium.org/562273008/diff/1/content/browser/renderer_hos... content/browser/renderer_host/media/audio_renderer_host.cc:297: render_process_id_, entry->render_view_id(), is_playing)); Can't we have multiple playing streams per RenderView? https://codereview.chromium.org/562273008/diff/1/content/browser/renderer_hos... content/browser/renderer_host/media/audio_renderer_host.cc:476: render_process_id_, entry->render_view_id(), false)); Same here - if we have two playing entries for a single render view, this doesn't work.
cc: miu. The more I think about it, the less I think the AudioStreamMonitor is the right choice here. The play state will toggle on pretty immediately for any tab creating audio. However it may play silence for a little while before actual audio comes through; this is due to prestart done by AudioRendererMixer for HTML5 audio. We don't want periods of silent audio to trigger resource deprioritization for streaming music sites. Especially since that silence may be the end of a track. Ideally you might consider both signals. I.e. the HasActiveAudio() data point can be used to deprioritize within a few hundred milliseconds. While WasRecentlyAudible() would a longer tail, possibly in the 10s of seconds. Both signals would be good for immediate prioritization. https://codereview.chromium.org/562273008/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/562273008/diff/1/content/browser/renderer_hos... content/browser/renderer_host/media/audio_renderer_host.cc:283: // TODO(dalecurtis): See about using AudioStreamMonitor instead. On 2014/09/18 01:13:24, aiolos wrote: > Dale, I wasn't quite sure where this TODO should go. Should I move it? Hmm, I guess it's not actually all that clear. You can just remove it. https://codereview.chromium.org/562273008/diff/1/content/browser/renderer_hos... content/browser/renderer_host/media/audio_renderer_host.cc:291: BrowserThread::PostTask( Does this need to be a PostTask? You're already on the IO thread. https://codereview.chromium.org/562273008/diff/1/content/browser/renderer_hos... content/browser/renderer_host/media/audio_renderer_host.cc:297: render_process_id_, entry->render_view_id(), is_playing)); On 2014/09/18 14:12:42, mmenke wrote: > Can't we have multiple playing streams per RenderView? Yeah, this should probably just return HasActiveAudio() instead of is_playing. https://codereview.chromium.org/562273008/diff/1/content/browser/renderer_hos... content/browser/renderer_host/media/audio_renderer_host.cc:476: render_process_id_, entry->render_view_id(), false)); On 2014/09/18 14:12:42, mmenke wrote: > Same here - if we have two playing entries for a single render view, this > doesn't work. Ditto.
On 2014/09/18 17:51:24, DaleCurtis wrote: > cc: miu. > > The more I think about it, the less I think the AudioStreamMonitor is the right > choice here. > > The play state will toggle on pretty immediately for any tab creating audio. > However it may play silence for a little while before actual audio comes > through; this is due to prestart done by AudioRendererMixer for HTML5 audio. > > We don't want periods of silent audio to trigger resource deprioritization for > streaming music sites. Especially since that silence may be the end of a track. > > Ideally you might consider both signals. I.e. the HasActiveAudio() data point > can be used to deprioritize within a few hundred milliseconds. While > WasRecentlyAudible() would a longer tail, possibly in the 10s of seconds. Both > signals would be good for immediate prioritization. > I was under the impression that HasActiveAudio() referred to whether there were any audio streams was playing, while WasRecentlyAudible() referred to whether audio was actually heard recently on one of the streams. I assumed that the way the CL was currently written, it would be a much more conservative signal to use for deprioritization than WasRecentlyAudible(). Is there something I'm missing?
On 2014/09/18 20:30:59, aiolos wrote: > On 2014/09/18 17:51:24, DaleCurtis wrote: > > cc: miu. > > > > The more I think about it, the less I think the AudioStreamMonitor is the > right > > choice here. > > > > The play state will toggle on pretty immediately for any tab creating audio. > > However it may play silence for a little while before actual audio comes > > through; this is due to prestart done by AudioRendererMixer for HTML5 audio. > > > > We don't want periods of silent audio to trigger resource deprioritization for > > streaming music sites. Especially since that silence may be the end of a > track. > > > > Ideally you might consider both signals. I.e. the HasActiveAudio() data point > > can be used to deprioritize within a few hundred milliseconds. While > > WasRecentlyAudible() would a longer tail, possibly in the 10s of seconds. > Both > > signals would be good for immediate prioritization. > > > > I was under the impression that HasActiveAudio() referred to whether there were > any audio streams was playing, while WasRecentlyAudible() referred to whether > audio was actually heard recently on one of the streams. I assumed that the way > the CL was currently written, it would be a much more conservative signal to use > for deprioritization than WasRecentlyAudible(). > > Is there something I'm missing? No, I think you understand correctly. I was suggesting that the long term ideal probably isn't either one individually but a function of the two data points.
On 2014/09/18 20:34:11, DaleCurtis wrote: > On 2014/09/18 20:30:59, aiolos wrote: > > On 2014/09/18 17:51:24, DaleCurtis wrote: > > > cc: miu. > > > > > > The more I think about it, the less I think the AudioStreamMonitor is the > > right > > > choice here. > > > > > > The play state will toggle on pretty immediately for any tab creating audio. > > > However it may play silence for a little while before actual audio comes > > > through; this is due to prestart done by AudioRendererMixer for HTML5 audio. > > > > > > We don't want periods of silent audio to trigger resource deprioritization > for > > > streaming music sites. Especially since that silence may be the end of a > > track. > > > > > > Ideally you might consider both signals. I.e. the HasActiveAudio() data > point > > > can be used to deprioritize within a few hundred milliseconds. While > > > WasRecentlyAudible() would a longer tail, possibly in the 10s of seconds. > > Both > > > signals would be good for immediate prioritization. > > > > > > > I was under the impression that HasActiveAudio() referred to whether there > were > > any audio streams was playing, while WasRecentlyAudible() referred to whether > > audio was actually heard recently on one of the streams. I assumed that the > way > > the CL was currently written, it would be a much more conservative signal to > use > > for deprioritization than WasRecentlyAudible(). > > > > Is there something I'm missing? > > No, I think you understand correctly. I was suggesting that the long term ideal > probably isn't either one individually but a function of the two data points. I agree, but think it should probably go into a separate CL after more thought has gone into what the correct behavior should be for a tab with silent audio. I'm especially concerned about correctly determining after what period of time we should coalesce it's requests. And I don't think there are enough cases where it will hurt us to rush the decision without better data. (Although I'd love to hear some advice on how to gather useful data to determine that.)
https://codereview.chromium.org/562273008/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/562273008/diff/1/content/browser/renderer_hos... content/browser/renderer_host/media/audio_renderer_host.cc:283: // TODO(dalecurtis): See about using AudioStreamMonitor instead. On 2014/09/18 17:51:23, DaleCurtis wrote: > On 2014/09/18 01:13:24, aiolos wrote: > > Dale, I wasn't quite sure where this TODO should go. Should I move it? > > Hmm, I guess it's not actually all that clear. You can just remove it. Done. https://codereview.chromium.org/562273008/diff/1/content/browser/renderer_hos... content/browser/renderer_host/media/audio_renderer_host.cc:291: BrowserThread::PostTask( On 2014/09/18 17:51:23, DaleCurtis wrote: > Does this need to be a PostTask? You're already on the IO thread. Done. https://codereview.chromium.org/562273008/diff/1/content/browser/renderer_hos... content/browser/renderer_host/media/audio_renderer_host.cc:297: render_process_id_, entry->render_view_id(), is_playing)); On 2014/09/18 17:51:24, DaleCurtis wrote: > On 2014/09/18 14:12:42, mmenke wrote: > > Can't we have multiple playing streams per RenderView? > > Yeah, this should probably just return HasActiveAudio() instead of is_playing. Changed to check that we're actually changing the active audio state to avoid extraneous signals as well. https://codereview.chromium.org/562273008/diff/1/content/browser/renderer_hos... content/browser/renderer_host/media/audio_renderer_host.cc:476: render_process_id_, entry->render_view_id(), false)); On 2014/09/18 17:51:23, DaleCurtis wrote: > On 2014/09/18 14:12:42, mmenke wrote: > > Same here - if we have two playing entries for a single render view, this > > doesn't work. > > Ditto. Done.
https://codereview.chromium.org/562273008/diff/20001/content/browser/renderer... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/562273008/diff/20001/content/browser/renderer... content/browser/renderer_host/media/audio_renderer_host.cc:476: bool had_active_audio = HasActiveAudio(); This is for the entire renderer process, not a particular RenderView, no? That's why the AudioRendererHost has a render_process_id_, but not render view ID. So we actually need to go through all entries, and see if we have any playing with the given render_view_id(Don't technically have to do this if is_playing is true, since |entry| is such an Entry.
https://codereview.chromium.org/562273008/diff/20001/content/browser/renderer... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/562273008/diff/20001/content/browser/renderer... content/browser/renderer_host/media/audio_renderer_host.cc:476: bool had_active_audio = HasActiveAudio(); On 2014/09/18 21:55:51, mmenke wrote: > This is for the entire renderer process, not a particular RenderView, no? > That's why the AudioRendererHost has a render_process_id_, but not render view > ID. So we actually need to go through all entries, and see if we have any > playing with the given render_view_id(Don't technically have to do this if > is_playing is true, since |entry| is such an Entry. Ah, good catch. Dale, is there a better way to tell if a specific render view is playing audio than iterating through the entries or adding book keeping for it? I don't see one atm.
https://codereview.chromium.org/562273008/diff/20001/content/browser/renderer... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/562273008/diff/20001/content/browser/renderer... content/browser/renderer_host/media/audio_renderer_host.cc:476: bool had_active_audio = HasActiveAudio(); On 2014/09/18 22:23:11, aiolos wrote: > On 2014/09/18 21:55:51, mmenke wrote: > > This is for the entire renderer process, not a particular RenderView, no? > > That's why the AudioRendererHost has a render_process_id_, but not render view > > ID. So we actually need to go through all entries, and see if we have any > > playing with the given render_view_id(Don't technically have to do this if > > is_playing is true, since |entry| is such an Entry. > > Ah, good catch. Dale, is there a better way to tell if a specific render view is > playing audio than iterating through the entries or adding book keeping for it? > I don't see one atm. Ah, no there isn't. The list shouldn't be large and you can iterate over it on the IO thread, so it's not a huge deal.
https://codereview.chromium.org/562273008/diff/20001/content/browser/renderer... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/562273008/diff/20001/content/browser/renderer... content/browser/renderer_host/media/audio_renderer_host.cc:476: bool had_active_audio = HasActiveAudio(); On 2014/09/18 22:57:38, DaleCurtis wrote: > On 2014/09/18 22:23:11, aiolos wrote: > > On 2014/09/18 21:55:51, mmenke wrote: > > > This is for the entire renderer process, not a particular RenderView, no? > > > That's why the AudioRendererHost has a render_process_id_, but not render > view > > > ID. So we actually need to go through all entries, and see if we have any > > > playing with the given render_view_id(Don't technically have to do this if > > > is_playing is true, since |entry| is such an Entry. > > > > Ah, good catch. Dale, is there a better way to tell if a specific render view > is > > playing audio than iterating through the entries or adding book keeping for > it? > > I don't see one atm. > > Ah, no there isn't. The list shouldn't be large and you can iterate over it on > the IO thread, so it's not a huge deal. Done.
media/ lgtm % style nits. https://codereview.chromium.org/562273008/diff/40001/content/browser/renderer... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/562273008/diff/40001/content/browser/renderer... content/browser/renderer_host/media/audio_renderer_host.cc:478: if (is_playing) { no {} for single line statements. https://codereview.chromium.org/562273008/diff/40001/content/browser/renderer... content/browser/renderer_host/media/audio_renderer_host.cc:485: DCHECK_CURRENTLY_ON(BrowserThread::IO); Why here? Can this just go at the top like elsewhere? https://codereview.chromium.org/562273008/diff/40001/content/browser/renderer... content/browser/renderer_host/media/audio_renderer_host.cc:497: AudioEntryMap::const_iterator it = audio_entries_.begin(); Assign within for? https://codereview.chromium.org/562273008/diff/40001/content/browser/renderer... content/browser/renderer_host/media/audio_renderer_host.cc:500: if (entry->render_view_id() == render_view_id && entry->playing()) { No {} again.
https://codereview.chromium.org/562273008/diff/40001/content/browser/renderer... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/562273008/diff/40001/content/browser/renderer... content/browser/renderer_host/media/audio_renderer_host.cc:478: if (is_playing) { On 2014/09/19 00:42:39, DaleCurtis wrote: > no {} for single line statements. Done. https://codereview.chromium.org/562273008/diff/40001/content/browser/renderer... content/browser/renderer_host/media/audio_renderer_host.cc:485: DCHECK_CURRENTLY_ON(BrowserThread::IO); On 2014/09/19 00:42:38, DaleCurtis wrote: > Why here? Can this just go at the top like elsewhere? Done. https://codereview.chromium.org/562273008/diff/40001/content/browser/renderer... content/browser/renderer_host/media/audio_renderer_host.cc:497: AudioEntryMap::const_iterator it = audio_entries_.begin(); On 2014/09/19 00:42:39, DaleCurtis wrote: > Assign within for? Done. Also changed another iterator in the code that followed the same pattern. https://codereview.chromium.org/562273008/diff/40001/content/browser/renderer... content/browser/renderer_host/media/audio_renderer_host.cc:500: if (entry->render_view_id() == render_view_id && entry->playing()) { On 2014/09/19 00:42:39, DaleCurtis wrote: > No {} again. Done.
LGTM https://codereview.chromium.org/562273008/diff/60001/content/browser/renderer... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/562273008/diff/60001/content/browser/renderer... content/browser/renderer_host/media/audio_renderer_host.cc:8: #include "base/bind_helpers.h" While you're here, mind adding base/logging.h, for DCHECK? At times, I feel like I'm on a one-man-crusade to fix that missing include in all files I review. :) https://codereview.chromium.org/562273008/diff/60001/content/browser/renderer... content/browser/renderer_host/media/audio_renderer_host.cc:478: if (entry->playing() != is_playing) { nit: Early return is generally preferred https://codereview.chromium.org/562273008/diff/60001/content/browser/renderer... content/browser/renderer_host/media/audio_renderer_host.cc:483: base::AtomicRefCountDec(&num_playing_streams_); optional nit: Suggest use braces {} when using else if (It's a little more common than not doing so, I believe) https://codereview.chromium.org/562273008/diff/60001/content/browser/renderer... content/browser/renderer_host/media/audio_renderer_host.cc:485: if (is_playing || !RenderViewHasActiveAudio(entry->render_view_id())) { optional: Could add a DCHECK_EQ(RenderViewHasActiveAudio(entry->render_view_id(), is_playing);, not sure it's worth the effort, though. https://codereview.chromium.org/562273008/diff/60001/content/browser/renderer... File content/browser/renderer_host/media/audio_renderer_host.h (right): https://codereview.chromium.org/562273008/diff/60001/content/browser/renderer... content/browser/renderer_host/media/audio_renderer_host.h:166: bool RenderViewHasActiveAudio(int render_view_id); nit: bool RenderViewHasActiveAudio(int render_view_id) const;
Drive-by comment: Consider eliminating any new per-RenderView code and instead making it per-RenderFrame to help the Cross-Site Isolation project.
On 2014/09/19 02:21:24, miu wrote: > Drive-by comment: Consider eliminating any new per-RenderView code and instead > making it per-RenderFrame to help the Cross-Site Isolation project. This depends on the IDs used by ResourceDispatchHost, so that would presumably be a fairly involved project - not as involved as hooking it up to tab navigation, perhaps, but would involve mucking with how the ResourceLoader is hooked up in renderer land.
On 2014/09/19 02:30:08, mmenke wrote: > On 2014/09/19 02:21:24, miu wrote: > > Drive-by comment: Consider eliminating any new per-RenderView code and instead > > making it per-RenderFrame to help the Cross-Site Isolation project. > > This depends on the IDs used by ResourceDispatchHost, so that would presumably > be a fairly involved project - not as involved as hooking it up to tab > navigation, perhaps, but would involve mucking with how the ResourceLoader is > hooked up in renderer land. Yeah, that might be a good idea in the long run, but it's way more involved than this CL is scoped for. https://codereview.chromium.org/562273008/diff/60001/content/browser/renderer... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/562273008/diff/60001/content/browser/renderer... content/browser/renderer_host/media/audio_renderer_host.cc:8: #include "base/bind_helpers.h" On 2014/09/19 01:28:57, mmenke wrote: > While you're here, mind adding base/logging.h, for DCHECK? At times, I feel > like I'm on a one-man-crusade to fix that missing include in all files I review. > :) Done. I didn't even notice it wasn't already included. I'll be sure to look even when others are already using it. Thanks for catching that. https://codereview.chromium.org/562273008/diff/60001/content/browser/renderer... content/browser/renderer_host/media/audio_renderer_host.cc:478: if (entry->playing() != is_playing) { On 2014/09/19 01:28:57, mmenke wrote: > nit: Early return is generally preferred Done. https://codereview.chromium.org/562273008/diff/60001/content/browser/renderer... content/browser/renderer_host/media/audio_renderer_host.cc:483: base::AtomicRefCountDec(&num_playing_streams_); On 2014/09/19 01:28:57, mmenke wrote: > optional nit: Suggest use braces {} when using else if (It's a little more > common than not doing so, I believe) Done. https://codereview.chromium.org/562273008/diff/60001/content/browser/renderer... content/browser/renderer_host/media/audio_renderer_host.cc:485: if (is_playing || !RenderViewHasActiveAudio(entry->render_view_id())) { On 2014/09/19 01:28:57, mmenke wrote: > optional: Could add a > DCHECK_EQ(RenderViewHasActiveAudio(entry->render_view_id(), is_playing);, not > sure it's worth the effort, though. Probably not worth iterating through a second time unless you're worried about there being a race.
https://codereview.chromium.org/562273008/diff/80001/content/browser/renderer... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/562273008/diff/80001/content/browser/renderer... content/browser/renderer_host/media/audio_renderer_host.cc:451: UpdateNumPlayingStreams(entry.get(), false); It looks like entry.get() is sometimes failing here on SEGV_MAPERR. Is there a way to keep entry from going out of scope until after UpdateNumPlayingStreams?
https://codereview.chromium.org/562273008/diff/80001/content/browser/renderer... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/562273008/diff/80001/content/browser/renderer... content/browser/renderer_host/media/audio_renderer_host.cc:451: UpdateNumPlayingStreams(entry.get(), false); On 2014/09/19 17:33:56, aiolos wrote: > It looks like entry.get() is sometimes failing here on SEGV_MAPERR. Is there a > way to keep entry from going out of scope until after UpdateNumPlayingStreams? That doesn't make sense. It shouldn't go out of scope until this function returns. The trybot failures suggest that |client| is NULL: [26648:26664:0919/102833:171931532:FATAL:resource_scheduler.cc(844)] Check failed: client. https://codereview.chromium.org/562273008/diff/80001/content/browser/renderer... content/browser/renderer_host/media/audio_renderer_host.cc:482: if (is_playing) { Remove {} from single line if. https://codereview.chromium.org/562273008/diff/80001/content/browser/renderer... content/browser/renderer_host/media/audio_renderer_host.cc:487: if (is_playing || !RenderViewHasActiveAudio(entry->render_view_id())) Multiline if needs {} https://codereview.chromium.org/562273008/diff/80001/content/browser/renderer... content/browser/renderer_host/media/audio_renderer_host.cc:488: ResourceDispatcherHostImpl::Get()->OnAudioRenderHostStreamStateChanged( I take it your upstream code will just ignore repeated play events?
https://codereview.chromium.org/562273008/diff/80001/content/browser/renderer... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/562273008/diff/80001/content/browser/renderer... content/browser/renderer_host/media/audio_renderer_host.cc:451: UpdateNumPlayingStreams(entry.get(), false); On 2014/09/19 17:40:40, DaleCurtis wrote: > On 2014/09/19 17:33:56, aiolos wrote: > > It looks like entry.get() is sometimes failing here on SEGV_MAPERR. Is there a > > way to keep entry from going out of scope until after UpdateNumPlayingStreams? > > That doesn't make sense. It shouldn't go out of scope until this function > returns. The trybot failures suggest that |client| is NULL: > > [26648:26664:0919/102833:171931532:FATAL:resource_scheduler.cc(844)] Check > failed: client. It didn't make sense to me either, but this was the important part of the stacktrace I was looking at that led me to ask that question: Received signal 11 SEGV_MAPERR 000000000208 #0 0x7fc55babe8fe base::debug::StackTrace::StackTrace() #1 0x7fc55babe433 base::debug::(anonymous namespace)::StackDumpSignalHandler() #2 0x7fc54cad3cb0 <unknown> #3 0x7fc5510355bc base::internal::scoped_ptr_impl<>::get() #4 0x7fc551018da9 scoped_ptr<>::operator->() #5 0x7fc551010c09 content::ResourceDispatcherHostImpl::OnAudioRenderHostStreamStateChanged() #6 0x7fc55117e6bf content::AudioRendererHost::UpdateNumPlayingStreams() #7 0x7fc55117de5a content::AudioRendererHost::DoNotifyStreamStateChanged() https://codereview.chromium.org/562273008/diff/80001/content/browser/renderer... content/browser/renderer_host/media/audio_renderer_host.cc:482: if (is_playing) { On 2014/09/19 17:40:40, DaleCurtis wrote: > Remove {} from single line if. Even though it's part of the if/else? You and Matt seem to have differing thoughts on that case. https://codereview.chromium.org/562273008/diff/80001/content/browser/renderer... content/browser/renderer_host/media/audio_renderer_host.cc:487: if (is_playing || !RenderViewHasActiveAudio(entry->render_view_id())) On 2014/09/19 17:40:40, DaleCurtis wrote: > Multiline if needs {} Done. https://codereview.chromium.org/562273008/diff/80001/content/browser/renderer... content/browser/renderer_host/media/audio_renderer_host.cc:488: ResourceDispatcherHostImpl::Get()->OnAudioRenderHostStreamStateChanged( On 2014/09/19 17:40:40, DaleCurtis wrote: > I take it your upstream code will just ignore repeated play events? Close. It will quickly recompute whether the tab should be background or not, but won't change the behavior. All of the signals are designed to handle that upstream. I think I'll change this to avoid confusion since both you and Matt had a question about it though.
https://codereview.chromium.org/562273008/diff/80001/content/browser/loader/r... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/562273008/diff/80001/content/browser/loader/r... content/browser/loader/resource_dispatcher_host_impl.cc:1389: scheduler_->OnAudibilityChanged(child_id, route_id, is_playing); It's crashing here, not in AUdioRendererHost. https://codereview.chromium.org/562273008/diff/80001/content/browser/renderer... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/562273008/diff/80001/content/browser/renderer... content/browser/renderer_host/media/audio_renderer_host.cc:451: UpdateNumPlayingStreams(entry.get(), false); On 2014/09/19 18:59:18, aiolos wrote: > On 2014/09/19 17:40:40, DaleCurtis wrote: > > On 2014/09/19 17:33:56, aiolos wrote: > > > It looks like entry.get() is sometimes failing here on SEGV_MAPERR. Is there > a > > > way to keep entry from going out of scope until after > UpdateNumPlayingStreams? > > > > That doesn't make sense. It shouldn't go out of scope until this function > > returns. The trybot failures suggest that |client| is NULL: > > > > [26648:26664:0919/102833:171931532:FATAL:resource_scheduler.cc(844)] Check > > failed: client. > > It didn't make sense to me either, but this was the important part of the > stacktrace I was looking at that led me to ask that question: > > Received signal 11 SEGV_MAPERR 000000000208 > #0 0x7fc55babe8fe base::debug::StackTrace::StackTrace() > #1 0x7fc55babe433 base::debug::(anonymous namespace)::StackDumpSignalHandler() > #2 0x7fc54cad3cb0 <unknown> > #3 0x7fc5510355bc base::internal::scoped_ptr_impl<>::get() > #4 0x7fc551018da9 scoped_ptr<>::operator->() > #5 0x7fc551010c09 > content::ResourceDispatcherHostImpl::OnAudioRenderHostStreamStateChanged() > #6 0x7fc55117e6bf content::AudioRendererHost::UpdateNumPlayingStreams() > #7 0x7fc55117de5a content::AudioRendererHost::DoNotifyStreamStateChanged() Wait, this crash is in content::ResourceDispatcherHostImpl::OnAudioRenderHostStreamStateChanged() not AudioRendererHost.
https://codereview.chromium.org/562273008/diff/80001/content/browser/renderer... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/562273008/diff/80001/content/browser/renderer... content/browser/renderer_host/media/audio_renderer_host.cc:482: if (is_playing) { On 2014/09/19 18:59:18, aiolos wrote: > On 2014/09/19 17:40:40, DaleCurtis wrote: > > Remove {} from single line if. > > Even though it's part of the if/else? You and Matt seem to have differing > thoughts on that case. I'm certainly not going to fight over this, but I've seen comments myself over not using them, and it's my experience that using them seems to be the slightly preferred style. The Google style guide technically allows both styles.
https://codereview.chromium.org/562273008/diff/80001/content/browser/renderer... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/562273008/diff/80001/content/browser/renderer... content/browser/renderer_host/media/audio_renderer_host.cc:482: if (is_playing) { On 2014/09/19 20:32:40, mmenke wrote: > On 2014/09/19 18:59:18, aiolos wrote: > > On 2014/09/19 17:40:40, DaleCurtis wrote: > > > Remove {} from single line if. > > > > Even though it's part of the if/else? You and Matt seem to have differing > > thoughts on that case. > > I'm certainly not going to fight over this, but I've seen comments myself over > not using them, and it's my experience that using them seems to be the slightly > preferred style. > > The Google style guide technically allows both styles. I didn't see Matt's previous disagreement. I don't care either and there's no precedent elsewhere in this file for one style or the other, so choose whatever style you like. http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Conditionals
https://codereview.chromium.org/562273008/diff/100001/content/browser/rendere... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/562273008/diff/100001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:451: AudioEntry* const entry_ptr = entry.get(); Why de-inline this? https://codereview.chromium.org/562273008/diff/100001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:490: RenderViewHasActiveAudio(entry->render_view_id()); is_playing || https://codereview.chromium.org/562273008/diff/100001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:491: DCHECK_EQ(is_playing, render_view_is_playing); This isn't correct, is_playing could be false if there are two streams and one was just stopped but the other isn't.
aiolos@chromium.org changed reviewers: + jam@chromium.org
Sorry about that. Fixed the error (there was a race between when the render_view was updating it's audio and when the ResourceDispatcherHostImpl was initialized. Added jam as a reviewer.
https://codereview.chromium.org/562273008/diff/160001/content/browser/rendere... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/562273008/diff/160001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:485: should_alert_resource_scheduler = I don't think this does what you wanted, the AtomicRefCount changes don't affect the ActiveEntry. So should_alert will always be set to the same thing regardless of is_playing. You need to know the state before setting is_playing on the entry if you want to deduplicate notifications. I.e. something like: const bool was_audible = RenderViewHasActiveAudio(entry->render_view_id()); ... if ((!was_audible && is_playing) || (was_audible && !is_playing && !RenderViewHasActiveAudio(entry->render_view_id())) { // notify } There may be a cleaner way to express that, but it accomplishes what you're looking to do I think.
I've been unable to reproduce the error that the try bot are seeing locally since the last CL. Uploaded fix for Dale, and hoping that things work this time. https://codereview.chromium.org/562273008/diff/160001/content/browser/rendere... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/562273008/diff/160001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:485: should_alert_resource_scheduler = On 2014/09/22 18:45:41, DaleCurtis wrote: > I don't think this does what you wanted, the AtomicRefCount changes don't affect > the ActiveEntry. So should_alert will always be set to the same thing regardless > of is_playing. > > You need to know the state before setting is_playing on the entry if you want to > deduplicate notifications. I.e. something like: > > const bool was_audible = RenderViewHasActiveAudio(entry->render_view_id()); > > ... > > if ((!was_audible && is_playing) || > (was_audible && !is_playing && > !RenderViewHasActiveAudio(entry->render_view_id())) { > // notify > } > > There may be a cleaner way to express that, but it accomplishes what you're > looking to do I think. That's not actually quite right either. New Patch uploaded.
The trybot errors from PS#10 look like a race condition or failure to initialize the AudioRendererHost. You'll need to take a closer look at what might be going on. I'd guess you're not waiting for the proper startup before trying to access the AudioRendererHost. https://codereview.chromium.org/562273008/diff/200001/content/browser/rendere... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/562273008/diff/200001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:481: bool should_alert_resource_scheduler = false; pair with if {} block below and don't initialize to false since it should always be set after the block (this makes it obvious and will allow tooling to potentially catch issues if a refactoring breaks something). https://codereview.chromium.org/562273008/diff/200001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:486: entry->set_playing(is_playing); Change this to true and false below for clarity. https://codereview.chromium.org/562273008/diff/200001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:489: base::AtomicRefCountDec(&num_playing_streams_); Move to end of {} block for clarity.
PTAL. The issue was two fold: The MockRenderProcessHost subclasses RenderProcessHost, not RenderProcessHostImpl, and the TestRenderViewHost called the RenderViewHostImpl constructor which attempted to get the audio state from the RenderProcessHostImpl. The ResourceScheduler test that was failing didn't correctly use a RenderViewHostFactory like it should. I changed the constructor for RenderViewHostImpl. Should I have added a second one for testing (or done something else) instead?
jam@ is best suited to answer those questions. I suspect the mocks were intentionally not using the impls. Does this compile on all platforms?
On 2014/09/29 22:34:56, DaleCurtis wrote: > jam@ is best suited to answer those questions. I suspect the mocks were > intentionally not using the impls. Does this compile on all platforms? They were in fact intentionally not using them. (I already asked jam.) That's why I opted to change The RenderViewHostImpl constructor to just default to not audible if it was created on a testing path, instead of mucking with what impls were being used. It seemed simpler than attempting to get a RenderProcessHostImpl or AudioRenderHost for a bunch of tests that don't actually use the audio signal. I compiled it on linux and mac (although my trust in my linux box is rather questionable atm.) I figured the try bots will determine if it doesn't compile on something else.
Looks like the patch needs a rebase.
lgtm % addressing comments on ps#13 + 2 new ones. https://codereview.chromium.org/562273008/diff/240001/content/browser/rendere... File content/browser/renderer_host/media/audio_renderer_host.h (right): https://codereview.chromium.org/562273008/diff/240001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.h:94: bool RenderViewHasActiveAudio(int render_view_id); const https://codereview.chromium.org/562273008/diff/240001/content/browser/rendere... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/562273008/diff/240001/content/browser/rendere... content/browser/renderer_host/render_view_host_impl.cc:199: scoped_refptr<AudioRendererHost> audio_host = It's a bit convoluted to create an empty refptr simply to get the active audio state. Just track a bool: const AudioRendererHost* arh = *(static_cast<RenderProcessHostImpl*>( GetProcess())->audio_renderer_host()); const bool has_active_audio = setup_for_testing || !arh ? false : arh->RenderViewHasActiveAudio(GetRoutingID()); If |arh| is only NULL during testing (I'm not sure), then you can inline that into the second clause instead.
Err, comments on PS#11, though PS#13 too :)
On 2014/09/30 17:15:15, DaleCurtis wrote: > lgtm % addressing comments on ps#13 + 2 new ones. > > https://codereview.chromium.org/562273008/diff/240001/content/browser/rendere... > File content/browser/renderer_host/media/audio_renderer_host.h (right): > > https://codereview.chromium.org/562273008/diff/240001/content/browser/rendere... > content/browser/renderer_host/media/audio_renderer_host.h:94: bool > RenderViewHasActiveAudio(int render_view_id); > const > > https://codereview.chromium.org/562273008/diff/240001/content/browser/rendere... > File content/browser/renderer_host/render_view_host_impl.cc (right): > > https://codereview.chromium.org/562273008/diff/240001/content/browser/rendere... > content/browser/renderer_host/render_view_host_impl.cc:199: > scoped_refptr<AudioRendererHost> audio_host = > It's a bit convoluted to create an empty refptr simply to get the active audio > state. Just track a bool: > > const AudioRendererHost* arh = *(static_cast<RenderProcessHostImpl*>( > GetProcess())->audio_renderer_host()); > const bool has_active_audio = > setup_for_testing || !arh ? false > : arh->RenderViewHasActiveAudio(GetRoutingID()); > > If |arh| is only NULL during testing (I'm not sure), then you can inline that > into the second clause instead. You can't do that. GetProcess()->audio_renderer_host() will crash because the RenderProcessHost doesn't have that function.
OIC, is ARH only NULL when setup_for_testing is true?
On 2014/09/30 17:44:33, DaleCurtis wrote: > OIC, is ARH only NULL when setup_for_testing is true? I'm only setting it to NULL then. I haven't seen it being NULL otherwise, but could conceivably see there being a race where the ARH wasn't initialized correctly yet when the RVHI was created and active_renderer_host might return NULL.
ARH can never be NULL post RPHI::Init(). The SiteInstance::GetProcess() method
runs before the rest of RVHI construction so it can't be NULL unless
GetProcess() returns something that's not actually an RPHI. You should be able
to safely use:
const bool has_active_audio =
setup_for_testing ? false
: static_cast<RenderProcessHostImpl*>(GetProcess())
->audio_renderer_host()
->RenderViewHasActiveAudio(GetRoutingID());
To be more clear, there is no RenderProcessHostImpl when setup_for_testing is true, and that isn't something that many of the tests want to change. I opted to create a NULL pointer for the ARH in that case. I wanted to be able to gracefully handle any cases where the ARH would be NULL otherwise anyway, and it seemed clear to me that it should be considered NULL if there was no RPHI. If there is a better solution for this, I'm open to suggestions. https://codereview.chromium.org/562273008/diff/200001/content/browser/rendere... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/562273008/diff/200001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:481: bool should_alert_resource_scheduler = false; On 2014/09/22 21:28:25, DaleCurtis wrote: > pair with if {} block below and don't initialize to false since it should always > be set after the block (this makes it obvious and will allow tooling to > potentially catch issues if a refactoring breaks something). Done. https://codereview.chromium.org/562273008/diff/200001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:486: entry->set_playing(is_playing); On 2014/09/22 21:28:25, DaleCurtis wrote: > Change this to true and false below for clarity. Done. https://codereview.chromium.org/562273008/diff/200001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:489: base::AtomicRefCountDec(&num_playing_streams_); On 2014/09/22 21:28:25, DaleCurtis wrote: > Move to end of {} block for clarity. Done. https://codereview.chromium.org/562273008/diff/240001/content/browser/rendere... File content/browser/renderer_host/media/audio_renderer_host.h (right): https://codereview.chromium.org/562273008/diff/240001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.h:94: bool RenderViewHasActiveAudio(int render_view_id); On 2014/09/30 17:15:15, DaleCurtis wrote: > const Done.
On 2014/09/30 18:07:57, DaleCurtis wrote: > ARH can never be NULL post RPHI::Init(). The SiteInstance::GetProcess() method > runs before the rest of RVHI construction so it can't be NULL unless > GetProcess() returns something that's not actually an RPHI. You should be able > to safely use: > > const bool has_active_audio = > setup_for_testing ? false > : static_cast<RenderProcessHostImpl*>(GetProcess()) > ->audio_renderer_host() > ->RenderViewHasActiveAudio(GetRoutingID()); GetProcess() does in fact return something that isn't a RPHI when setup_for_testing is true. That's why all of the tests were crashing. It returns a MockRenderProcessHost, which subclasses RPH, not RPHI.
On 2014/09/30 18:17:37, aiolos wrote: > On 2014/09/30 18:07:57, DaleCurtis wrote: > > ARH can never be NULL post RPHI::Init(). The SiteInstance::GetProcess() > method > > runs before the rest of RVHI construction so it can't be NULL unless > > GetProcess() returns something that's not actually an RPHI. You should be able > > to safely use: > > > > const bool has_active_audio = > > setup_for_testing ? false > > : static_cast<RenderProcessHostImpl*>(GetProcess()) > > ->audio_renderer_host() > > ->RenderViewHasActiveAudio(GetRoutingID()); > > GetProcess() does in fact return something that isn't a RPHI when > setup_for_testing is true. That's why all of the tests were crashing. It returns > a MockRenderProcessHost, which subclasses RPH, not RPHI. Correct, hence the check on setup_for_testing in the recommended code.
On 2014/09/30 18:17:37, aiolos wrote: > On 2014/09/30 18:07:57, DaleCurtis wrote: > > ARH can never be NULL post RPHI::Init(). The SiteInstance::GetProcess() > method > > runs before the rest of RVHI construction so it can't be NULL unless > > GetProcess() returns something that's not actually an RPHI. You should be able > > to safely use: > > > > const bool has_active_audio = > > setup_for_testing ? false > > : static_cast<RenderProcessHostImpl*>(GetProcess()) > > ->audio_renderer_host() > > ->RenderViewHasActiveAudio(GetRoutingID()); > > GetProcess() does in fact return something that isn't a RPHI when > setup_for_testing is true. That's why all of the tests were crashing. It returns > a MockRenderProcessHost, which subclasses RPH, not RPHI. It also doesn't return a NULL ARH that I can check for. It straight up crashes because RPH doesn't have an audio_renderer_host().
lgtm
lgtm https://codereview.chromium.org/562273008/diff/320001/content/browser/rendere... File content/browser/renderer_host/render_view_host_impl.h (right): https://codereview.chromium.org/562273008/diff/320001/content/browser/rendere... content/browser/renderer_host/render_view_host_impl.h:112: bool setup_for_testing); nit: can we just call this create_audio_renderer_host and flip its meaning? setup_for_testing is vague
On 2014/10/01 00:12:36, jam wrote: > lgtm > > https://codereview.chromium.org/562273008/diff/320001/content/browser/rendere... > File content/browser/renderer_host/render_view_host_impl.h (right): > > https://codereview.chromium.org/562273008/diff/320001/content/browser/rendere... > content/browser/renderer_host/render_view_host_impl.h:112: bool > setup_for_testing); > nit: can we just call this create_audio_renderer_host and flip its meaning? > setup_for_testing is vague I can, although we aren't actually creating an audio_renderer_host. Would has_audio_render_host wok?
On 2014/10/01 00:23:46, aiolos wrote: > On 2014/10/01 00:12:36, jam wrote: > > lgtm > > > > > https://codereview.chromium.org/562273008/diff/320001/content/browser/rendere... > > File content/browser/renderer_host/render_view_host_impl.h (right): > > > > > https://codereview.chromium.org/562273008/diff/320001/content/browser/rendere... > > content/browser/renderer_host/render_view_host_impl.h:112: bool > > setup_for_testing); > > nit: can we just call this create_audio_renderer_host and flip its meaning? > > setup_for_testing is vague > > I can, although we aren't actually creating an audio_renderer_host. Would > has_audio_render_host wok? Or even better, has_render_process_impl, since that is actually what the check is for?
On 2014/10/01 00:23:46, aiolos wrote: > On 2014/10/01 00:12:36, jam wrote: > > lgtm > > > > > https://codereview.chromium.org/562273008/diff/320001/content/browser/rendere... > > File content/browser/renderer_host/render_view_host_impl.h (right): > > > > > https://codereview.chromium.org/562273008/diff/320001/content/browser/rendere... > > content/browser/renderer_host/render_view_host_impl.h:112: bool > > setup_for_testing); > > nit: can we just call this create_audio_renderer_host and flip its meaning? > > setup_for_testing is vague > > I can, although we aren't actually creating an audio_renderer_host. Would > has_audio_render_host wok? Or even better, has_render_process_impl, since that is actually what the check is for?
https://codereview.chromium.org/562273008/diff/340001/content/browser/rendere... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/562273008/diff/340001/content/browser/rendere... content/browser/renderer_host/render_view_host_impl.cc:216: } actually, now that I look at this some more. how about we move all this code in the constructor to the Init method? the coding style does say to avoid doing work in the constructor. then if the Init method isn't called by the unittest, we're good to go?
On 2014/10/01 20:31:38, jam wrote: > https://codereview.chromium.org/562273008/diff/340001/content/browser/rendere... > File content/browser/renderer_host/render_view_host_impl.cc (right): > > https://codereview.chromium.org/562273008/diff/340001/content/browser/rendere... > content/browser/renderer_host/render_view_host_impl.cc:216: } > actually, now that I look at this some more. how about we move all this code in > the constructor to the Init method? the coding style does say to avoid doing > work in the constructor. > > then if the Init method isn't called by the unittest, we're good to go? That would change the behavior of the ResourceScheduler since we've been getting the OnClientCreation signal from here since it started. And we need to let the ResourceScheduler know the starting audio state regardless of whether we're in a test or not so it would move the work around, but not reduce any of the requirements for this CL. I can run some tests to see if that would still work with the scheduling algorithm, but I'd strongly prefer to do that change in a separate CL if that's something that you would like to happen.
On 2014/10/01 20:40:19, aiolos wrote: > On 2014/10/01 20:31:38, jam wrote: > > > https://codereview.chromium.org/562273008/diff/340001/content/browser/rendere... > > File content/browser/renderer_host/render_view_host_impl.cc (right): > > > > > https://codereview.chromium.org/562273008/diff/340001/content/browser/rendere... > > content/browser/renderer_host/render_view_host_impl.cc:216: } > > actually, now that I look at this some more. how about we move all this code > in > > the constructor to the Init method? the coding style does say to avoid doing > > work in the constructor. > > > > then if the Init method isn't called by the unittest, we're good to go? > > That would change the behavior of the ResourceScheduler since we've been getting > the OnClientCreation signal from here since it started. And we need to let the > ResourceScheduler know the starting audio state regardless of whether we're in a > test or not so it would move the work around, but not reduce any of the > requirements for this CL. I can run some tests to see if that would still work > with the scheduling algorithm, but I'd strongly prefer to do that change in a > separate CL if that's something that you would like to happen. I see, ok nvm then. re 'has_render_process_host_impl', I don't like that name because we have avoided RVH knowing about the type of RenderProcessHost implementation that's in use. can we call it something related to audio initialization? i.e. initialize_audo_host?
On 2014/10/02 01:09:34, jam wrote: > On 2014/10/01 20:40:19, aiolos wrote: > > On 2014/10/01 20:31:38, jam wrote: > > > > > > https://codereview.chromium.org/562273008/diff/340001/content/browser/rendere... > > > File content/browser/renderer_host/render_view_host_impl.cc (right): > > > > > > > > > https://codereview.chromium.org/562273008/diff/340001/content/browser/rendere... > > > content/browser/renderer_host/render_view_host_impl.cc:216: } > > > actually, now that I look at this some more. how about we move all this code > > in > > > the constructor to the Init method? the coding style does say to avoid doing > > > work in the constructor. > > > > > > then if the Init method isn't called by the unittest, we're good to go? > > > > That would change the behavior of the ResourceScheduler since we've been > getting > > the OnClientCreation signal from here since it started. And we need to let the > > ResourceScheduler know the starting audio state regardless of whether we're in > a > > test or not so it would move the work around, but not reduce any of the > > requirements for this CL. I can run some tests to see if that would still work > > with the scheduling algorithm, but I'd strongly prefer to do that change in a > > separate CL if that's something that you would like to happen. > > I see, ok nvm then. > > re 'has_render_process_host_impl', I don't like that name because we have > avoided RVH knowing about the type of RenderProcessHost implementation that's in > use. can we call it something related to audio initialization? i.e. > initialize_audo_host? TBH, I'll call it whatever makes you happy, but we do nothing but query for the audio state in this function even if we have the RPHI.
On 2014/10/02 01:12:51, aiolos wrote: > On 2014/10/02 01:09:34, jam wrote: > > On 2014/10/01 20:40:19, aiolos wrote: > > > On 2014/10/01 20:31:38, jam wrote: > > > > > > > > > > https://codereview.chromium.org/562273008/diff/340001/content/browser/rendere... > > > > File content/browser/renderer_host/render_view_host_impl.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/562273008/diff/340001/content/browser/rendere... > > > > content/browser/renderer_host/render_view_host_impl.cc:216: } > > > > actually, now that I look at this some more. how about we move all this > code > > > in > > > > the constructor to the Init method? the coding style does say to avoid > doing > > > > work in the constructor. > > > > > > > > then if the Init method isn't called by the unittest, we're good to go? > > > > > > That would change the behavior of the ResourceScheduler since we've been > > getting > > > the OnClientCreation signal from here since it started. And we need to let > the > > > ResourceScheduler know the starting audio state regardless of whether we're > in > > a > > > test or not so it would move the work around, but not reduce any of the > > > requirements for this CL. I can run some tests to see if that would still > work > > > with the scheduling algorithm, but I'd strongly prefer to do that change in > a > > > separate CL if that's something that you would like to happen. > > > > I see, ok nvm then. > > > > re 'has_render_process_host_impl', I don't like that name because we have > > avoided RVH knowing about the type of RenderProcessHost implementation that's > in > > use. can we call it something related to audio initialization? i.e. > > initialize_audo_host? > > TBH, I'll call it whatever makes you happy, but we do nothing but query for the > audio state in this function even if we have the RPHI. (And we're technically initializing the audio state in the ResourceScheduler here via the ResourceDispatcherHostImpl in both cases. We just use the default of false if there is no RPHI, and thus no ARH to query.)
The CQ bit was checked by aiolos@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/562273008/360001
Message was sent while issue was closed.
Committed patchset #19 (id:360001)
Message was sent while issue was closed.
Patchset 19 (id:??) landed as https://crrev.com/6e7ce491a74481f8e3d7293ebb0ae58d62564526 Cr-Commit-Position: refs/heads/master@{#299029} |
