Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2443573003:
Factor out AudioOutputDelegate from AudioRendererHost. (Closed)
|
Created:
4 years, 2 months ago by Max Morin Modified:
4 years ago CC:
audio-mojo-cl_google.com, chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, miu+watch_chromium.org, posciak+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFactor out AudioOutputDelegate from AudioRendererHost.
This change moves logic related to IPC requests regarding a single stream into its own class. This prepares for adding a mojo alternative to ARH. The mojo implementation will have a separate interface for an audio stream, which makes it very reasonable to have a class responsible for handling requests to a stream. This change also slims down ARH, so that it's easier to replace.
Reference for mojofication work: https://docs.google.com/document/d/1PjV_EutyJANmsEjhH9mTNZWSslTPvAit7Cy2RO3cbmk/edit, in particular the diagram https://docs.google.com/drawings/d/1xg6z2FwLdTe5eG-Nvek9q-IfWzMI01XxlbVH3y0mzAE/edit.
BUG=656923
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel,linux_chromium_msan_rel_ng,linux_chromium_chromeos_asan_rel_ng,linux_chromium_chromeos_msan_rel_ng,linux_chromium_tsan_rel_ng,linux_chromium_ubsan_rel_ng;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel,mac_chromium_asan_rel_ng;master.tryserver.chromium.win:win_optional_gpu_tests_rel,win_chromium_syzyasan_rel
Committed: https://crrev.com/65f77fb7054d31082b55e7385617c819f31902ec
Cr-Commit-Position: refs/heads/master@{#436278}
Patch Set 1 #Patch Set 2 : Small fixes #Patch Set 3 : Simplify. #
Total comments: 10
Patch Set 4 : Fix comments. #Patch Set 5 : Improve AudioOutputController comment. #
Total comments: 9
Patch Set 6 : More comments. #
Total comments: 2
Patch Set 7 : Change iterator. #
Total comments: 8
Patch Set 8 : Add AudioOutputDelegate::Deleter. #
Total comments: 34
Patch Set 9 : Small fixes. #Patch Set 10 : . #Patch Set 11 : Remove the loop when shutting down ARH. Unit tests. #Patch Set 12 : Remove [&] from lambda. #
Total comments: 36
Patch Set 13 : Rebase. Comments. #
Total comments: 40
Patch Set 14 : . #
Total comments: 10
Patch Set 15 : . #
Total comments: 63
Patch Set 16 : . #Patch Set 17 : . #
Total comments: 9
Patch Set 18 : Add AudioDeviceThread. Other fixes. #Patch Set 19 : . #
Total comments: 3
Patch Set 20 : . #
Total comments: 1
Patch Set 21 : \n #Patch Set 22 : Mac compile. #
Total comments: 10
Patch Set 23 : Final comments addressed. #Messages
Total messages: 72 (22 generated)
Description was changed from ========== Factor out AudioOutputDelegate from AudioRendererHost. BUG=656923 ========== to ========== Factor out AudioOutputDelegate from AudioRendererHost. BUG=656923 ==========
maxmorin@chromium.org changed reviewers: + olka@chromium.org
Olga: Could you take a look at this before I fix all the tests?
On 2016/10/25 08:21:40, Max Morin Chromium wrote: > Olga: Could you take a look at this before I fix all the tests? PTAL
https://codereview.chromium.org/2443573003/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/audio_output_delegate.cc (right): https://codereview.chromium.org/2443573003/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_output_delegate.cc:38: audio_log_(audio_log), It's probably fine to have an individual audio_log per AudioOutputDelegate, because it's just an accessor to media log, and this way you don't need to care about audio_log lifetime. https://codereview.chromium.org/2443573003/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_output_delegate.cc:61: AudioOutputDelegate::~AudioOutputDelegate() {} AudioOutputController is refcounted, so it may now be pointing to invalid memory. Let's implement the fix we discussed offline. https://codereview.chromium.org/2443573003/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_output_delegate.cc:91: &RemoveDiverter, base::Unretained(mirroring_manager_), controller_)); Unretained is dangerous: no obvious guarantee that mirror_manager still exists when RemoveDiverter() is executed. Safer to make RemoveDiverter() to be a method of AudioOutputDelegate andpass delegate as weak_ptr - this way you have one place less to worry about mirroring_manager lifetime. https://codereview.chromium.org/2443573003/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_output_delegate.cc:93: controller_->Close(base::Bind([]() {})); base::Bind(&DoNothing) is a common way https://cs.chromium.org/chromium/src/base/bind_helpers.h?dr=C&sq=package:chro... https://codereview.chromium.org/2443573003/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/2443573003/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:490: i != delegates_.end() && (*i)->stream_id() != stream_id; ++i) { (alternatively you can use std::find_if)
https://codereview.chromium.org/2443573003/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/audio_output_delegate.cc (right): https://codereview.chromium.org/2443573003/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_output_delegate.cc:74: weak_factory_.InvalidateWeakPtrs(); Why do you want to do it at this very moment? https://codereview.chromium.org/2443573003/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/2443573003/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:175: OnCloseStream(delegates_[0]->stream_id()); it would probably be more a bit more efficient to work on delegates_->back() https://codereview.chromium.org/2443573003/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:472: for (i = delegates_.begin(); (can be std::find_if as well) https://codereview.chromium.org/2443573003/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:478: std::unique_ptr<AudioOutputDelegate> delegate = std::move(*i); Probably something like this to avoid an extra move? And you want to swap only if it's not the last element. if (i != delegates_.back()) std::swap(i, delegates_.back()); delegates_.back()->OnCloseStream(std::move(delegates_.back())); delegates_.pop_back();
PTAL https://codereview.chromium.org/2443573003/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/audio_output_delegate.cc (right): https://codereview.chromium.org/2443573003/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_output_delegate.cc:38: audio_log_(audio_log), On 2016/10/25 12:32:34, o1ka wrote: > It's probably fine to have an individual audio_log per AudioOutputDelegate, > because it's just an accessor to media log, and this way you don't need to care > about audio_log lifetime. Done. https://codereview.chromium.org/2443573003/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_output_delegate.cc:61: AudioOutputDelegate::~AudioOutputDelegate() {} On 2016/10/25 12:32:34, o1ka wrote: > AudioOutputController is refcounted, so it may now be pointing to invalid > memory. Let's implement the fix we discussed offline. Done. https://codereview.chromium.org/2443573003/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_output_delegate.cc:91: &RemoveDiverter, base::Unretained(mirroring_manager_), controller_)); On 2016/10/25 12:32:34, o1ka wrote: > Unretained is dangerous: no obvious guarantee that mirror_manager still exists > when RemoveDiverter() is executed. > Safer to make RemoveDiverter() to be a method of AudioOutputDelegate andpass > delegate as weak_ptr - this way you have one place less to worry about > mirroring_manager lifetime. Done. https://codereview.chromium.org/2443573003/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_output_delegate.cc:93: controller_->Close(base::Bind([]() {})); On 2016/10/25 12:32:34, o1ka wrote: > base::Bind(&DoNothing) is a common way > https://cs.chromium.org/chromium/src/base/bind_helpers.h?dr=C&sq=package:chro... Acknowledged. https://codereview.chromium.org/2443573003/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/2443573003/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:490: i != delegates_.end() && (*i)->stream_id() != stream_id; ++i) { On 2016/10/25 12:32:34, o1ka wrote: > (alternatively you can use std::find_if) Done. https://codereview.chromium.org/2443573003/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/audio_output_delegate.cc (right): https://codereview.chromium.org/2443573003/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_output_delegate.cc:74: weak_factory_.InvalidateWeakPtrs(); On 2016/10/25 15:59:41, o1ka wrote: > Why do you want to do it at this very moment? I'll just add a flag instead. https://codereview.chromium.org/2443573003/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/2443573003/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:175: OnCloseStream(delegates_[0]->stream_id()); On 2016/10/25 15:59:41, o1ka wrote: > it would probably be more a bit more efficient to work on delegates_->back() Only if we also search backwards. I updated the code. https://codereview.chromium.org/2443573003/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:472: for (i = delegates_.begin(); On 2016/10/25 15:59:41, o1ka wrote: > (can be std::find_if as well) Done. https://codereview.chromium.org/2443573003/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:478: std::unique_ptr<AudioOutputDelegate> delegate = std::move(*i); On 2016/10/25 15:59:41, o1ka wrote: > Probably something like this to avoid an extra move? And you want to swap only > if it's not the last element. > > if (i != delegates_.back()) > std::swap(i, delegates_.back()); > delegates_.back()->OnCloseStream(std::move(delegates_.back())); > delegates_.pop_back(); Are you sure branching is faster than just doing the swap? The pointers should be in L1 cache (since we just searched the vector), so the swap should be cheaper than a branch mispredict. But more importantly, the code is simpler without the branch :)
https://codereview.chromium.org/2443573003/diff/100001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/2443573003/diff/100001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:476: std::unique_ptr<AudioOutputDelegate> delegate = std::move(*i); *i should be delegates_.back(), of course.
On 2016/10/26 09:50:45, Max Morin Chromium wrote: > https://codereview.chromium.org/2443573003/diff/100001/content/browser/render... > File content/browser/renderer_host/media/audio_renderer_host.cc (right): > > https://codereview.chromium.org/2443573003/diff/100001/content/browser/render... > content/browser/renderer_host/media/audio_renderer_host.cc:476: > std::unique_ptr<AudioOutputDelegate> delegate = std::move(*i); > *i should be delegates_.back(), of course. PTAL
https://codereview.chromium.org/2443573003/diff/100001/content/browser/render... File content/browser/renderer_host/media/audio_output_delegate.h (right): https://codereview.chromium.org/2443573003/diff/100001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.h:51: const scoped_refptr<media::AudioOutputController>& controller() const { add a comment that this method is to be removed upon WebrtcAudioPrivate(Set/Get)ActiveSinkFunction removal (https://crbug/647185) https://codereview.chromium.org/2443573003/diff/120001/content/browser/render... File content/browser/renderer_host/media/audio_output_delegate.cc (right): https://codereview.chromium.org/2443573003/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:52: DCHECK(closed_); thread check? https://codereview.chromium.org/2443573003/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:79: DCHECK_CURRENTLY_ON(BrowserThread::IO); DCHEK(this == owned_this.get())? https://codereview.chromium.org/2443573003/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:121: UpdatePlayingStateForHandler(false); true? https://codereview.chromium.org/2443573003/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:159: void AudioOutputDelegate::UpdatePlayingStateForHandler(bool playing) { thread DCHECK? https://codereview.chromium.org/2443573003/diff/120001/content/browser/render... File content/browser/renderer_host/media/audio_output_delegate.h (right): https://codereview.chromium.org/2443573003/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.h:61: // It will delete itself when ready. After this has been called, I would remove "when ready" sentence: it's a bit confusing. https://codereview.chromium.org/2443573003/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.h:64: void OnCloseStream(std::unique_ptr<AudioOutputDelegate> owned_this); Probably implement this as a deleter? It's deleter semantic anyways. Then we can "hide" the destructor from the users, and thus will guarantee that FinishClosing() logic is executed before (or rather during) destruction. https://codereview.chromium.org/2443573003/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.h:74: void FinishClosing(); I do not quite like this name, but I'm bad in naming :( Probably just "Close"? https://codereview.chromium.org/2443573003/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.h:89: bool closed_ = false; Looks like there is no need for the flag: we can just reset handler_ to nullptr - it's always accessed on the same thread.
PTAL
https://codereview.chromium.org/2443573003/diff/140001/content/browser/render... File content/browser/renderer_host/media/audio_output_delegate.cc (right): https://codereview.chromium.org/2443573003/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:31: media::AudioOutputController::Create(media::AudioManager::Get(), We need to figure out and state here why it's guaranteed that AudioOutputDelegate/AudioController do not outlive AudioManager. https://codereview.chromium.org/2443573003/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:42: media_observer->OnCreatingAudioStream(render_process_id_, render_frame_id_); We can do it in Create() now. https://codereview.chromium.org/2443573003/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:57: DCHECK(!playing_); nit: move DCHECK to the top? (It's a prerequisite) https://codereview.chromium.org/2443573003/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:67: base::Bind([](AudioOutputDelegate*) {}, base::Owned(delegate))); Looks like there is no guarantee that the destructor will be called on IO thread. Also, we do not probably want delegate to be destroyed before controller is destroyed (controller points to it as an event handler). Let's discuss offline. https://codereview.chromium.org/2443573003/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:149: AudioStreamMonitor::StopMonitoringStream(render_process_id_, render_frame_id_, We may never stop monitoring if |handler_| was reset between start and stop. I would move if (!handler_) into UpdatePlayingStateForHandler(), and start/stop monitoring regardless. https://codereview.chromium.org/2443573003/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:163: audio_log_->OnError(stream_id_); Do we probably want to log an error even if |handler_| is reset? https://codereview.chromium.org/2443573003/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:170: playing_ = playing; Note that we want to update |playing_| even if |handler_| is nullpltr. otherwise you may just hit DCHECK(!playing_) in the destructor. Overall, looks like all the code needs to be modified like that: if (handler_) handler_->On... Otherwise resetting |handler_| changes the behavior in many places. https://codereview.chromium.org/2443573003/diff/140001/content/browser/render... File content/browser/renderer_host/media/audio_output_delegate.h (right): https://codereview.chromium.org/2443573003/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.h:40: class Deleter { nit: can be a struct https://codereview.chromium.org/2443573003/diff/140001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/2443573003/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:125: media::AudioLogFactory* log_factory, This is a nice cleanup. https://codereview.chromium.org/2443573003/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:468: // AudioOutputDelegate twice. Is this comment still relevant? https://codereview.chromium.org/2443573003/diff/140001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host_unittest.cc (right): https://codereview.chromium.org/2443573003/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host_unittest.cc:105: MOCK_METHOD1(WasNotifiedOfStreamCreation, void(int stream_id)); This pair of names looks strange: what's the difference? https://codereview.chromium.org/2443573003/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host_unittest.cc:169: WasNotifiedOfStreamCreation(stream_id); Do not quite get it: why notified of stream creation on error?
https://codereview.chromium.org/2443573003/diff/140001/content/browser/render... File content/browser/renderer_host/media/audio_output_delegate.cc (right): https://codereview.chromium.org/2443573003/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:47: MediaInternals::GetInstance()->SetWebContentsTitleForAudioLogEntry( Would be nice to avoid dependency on MediaInternals here. Can we set the title before passing the log in constructor? Also, this method can't be called on FakeAudioLogImpl, see https://cs.chromium.org/chromium/src/content/browser/media/media_internals.cc.... https://codereview.chromium.org/2443573003/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:67: base::Bind([](AudioOutputDelegate*) {}, base::Owned(delegate))); On 2016/10/27 09:52:38, o1ka wrote: > Looks like there is no guarantee that the destructor will be called on IO > thread. Also, we do not probably want delegate to be destroyed before controller > is destroyed (controller points to it as an event handler). > Let's discuss offline. Ok, looks like it's fine, we just need to add a comment for AudioOutputController::Close that callback is guaranteed to be called on the same thread.
Still thinking about the dependency on MediaInternals. https://codereview.chromium.org/2443573003/diff/140001/content/browser/render... File content/browser/renderer_host/media/audio_output_delegate.cc (right): https://codereview.chromium.org/2443573003/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:31: media::AudioOutputController::Create(media::AudioManager::Get(), On 2016/10/27 09:52:38, o1ka wrote: > We need to figure out and state here why it's guaranteed that > AudioOutputDelegate/AudioController do not outlive AudioManager. I'll just pass the AudioManager* in to the constructor. https://codereview.chromium.org/2443573003/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:57: DCHECK(!playing_); On 2016/10/27 09:52:38, o1ka wrote: > nit: move DCHECK to the top? (It's a prerequisite) Done. https://codereview.chromium.org/2443573003/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:67: base::Bind([](AudioOutputDelegate*) {}, base::Owned(delegate))); On 2016/10/27 12:59:52, o1ka wrote: > On 2016/10/27 09:52:38, o1ka wrote: > > Looks like there is no guarantee that the destructor will be called on IO > > thread. Also, we do not probably want delegate to be destroyed before > controller > > is destroyed (controller points to it as an event handler). > > Let's discuss offline. > > Ok, looks like it's fine, we just need to add a comment for > AudioOutputController::Close that callback is guaranteed to be called on the > same thread. Done. https://codereview.chromium.org/2443573003/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:149: AudioStreamMonitor::StopMonitoringStream(render_process_id_, render_frame_id_, On 2016/10/27 09:52:38, o1ka wrote: > We may never stop monitoring if |handler_| was reset between start and stop. > I would move if (!handler_) into UpdatePlayingStateForHandler(), and start/stop > monitoring regardless. We stop monitoring in the destructor. The idea with null-checking the handler everywhere is to just ignore all calls we get while in the process of being deleted. In effect, I want this: We start closing (call to Deleter::operator()) AudioOutputController closes We finish closing (by destructor) The null checks are to ensure there aren't other confusing stuff happening in between, decreasing headache. https://codereview.chromium.org/2443573003/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:163: audio_log_->OnError(stream_id_); On 2016/10/27 09:52:38, o1ka wrote: > Do we probably want to log an error even if |handler_| is reset? Ok. https://codereview.chromium.org/2443573003/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:170: playing_ = playing; On 2016/10/27 09:52:38, o1ka wrote: > Note that we want to update |playing_| even if |handler_| is nullpltr. otherwise > you may just hit DCHECK(!playing_) in the destructor. > Overall, looks like all the code needs to be modified like that: > if (handler_) > handler_->On... > > Otherwise resetting |handler_| changes the behavior in many places. If we reset the handler while |playing_| is true, it's an error (the stream will never be removed from the count that the handler keeps). The only place where the handler is reset is in Deleter::operator(), and it calls UpdatePlayingStateForHandler(false) before, so |playing_| will be stuck at false until the destructor is ran. https://codereview.chromium.org/2443573003/diff/140001/content/browser/render... File content/browser/renderer_host/media/audio_output_delegate.h (right): https://codereview.chromium.org/2443573003/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.h:40: class Deleter { On 2016/10/27 09:52:38, o1ka wrote: > nit: can be a struct Done. https://codereview.chromium.org/2443573003/diff/140001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/2443573003/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:468: // AudioOutputDelegate twice. On 2016/10/27 09:52:39, o1ka wrote: > Is this comment still relevant? I think so. OnCloseStream can be called from all over the place, so it's hard to be sure it will only be scheduled once. https://codereview.chromium.org/2443573003/diff/140001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host_unittest.cc (right): https://codereview.chromium.org/2443573003/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host_unittest.cc:105: MOCK_METHOD1(WasNotifiedOfStreamCreation, void(int stream_id)); On 2016/10/27 09:52:39, o1ka wrote: > This pair of names looks strange: what's the difference? I messed them up, the second one is supposed to be error. The reason for the renaming is that OnStreamCreated/OnStreamError are real functions in ARH now. https://codereview.chromium.org/2443573003/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host_unittest.cc:169: WasNotifiedOfStreamCreation(stream_id); On 2016/10/27 09:52:39, o1ka wrote: > Do not quite get it: why notified of stream creation on error? Typo :)
https://codereview.chromium.org/2443573003/diff/140001/content/browser/render... File content/browser/renderer_host/media/audio_output_delegate.cc (right): https://codereview.chromium.org/2443573003/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:62: delegate->UpdatePlayingStateForHandler(false); It will call handler_->OnStreamStateChange, and this is not allowed. setting |handler_| to nullptr should be the first thing we do in this method (and add a comment that this way we are indicated the start of shutdown). https://codereview.chromium.org/2443573003/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:65: // Keep delegate alive until controller is closed: Could you extend the comment to explain that controller will reply with the passed callback on this same thread after it closes the stream, and at that point we can safely delete both delegate and controller (which will be done when callback is destroyed). This may save a couple of hours of staring into the code for somebody (and probably for you in a year ;) ) https://codereview.chromium.org/2443573003/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:170: playing_ = playing; On 2016/10/27 15:04:38, Max Morin Chromium wrote: > On 2016/10/27 09:52:38, o1ka wrote: > > Note that we want to update |playing_| even if |handler_| is nullpltr. > otherwise > > you may just hit DCHECK(!playing_) in the destructor. > > Overall, looks like all the code needs to be modified like that: > > if (handler_) > > handler_->On... > > > > Otherwise resetting |handler_| changes the behavior in many places. > > If we reset the handler while |playing_| is true, it's an error (the stream will > never be removed from the count that the handler keeps). The only place where > the handler is reset is in Deleter::operator(), and it calls > UpdatePlayingStateForHandler(false) before, so |playing_| will be stuck at false > until the destructor is ran. Ok, sounds good. Than let's (1) DCHECK |handler_| here; (2) comment on |handler_| in the header that if it's nullptr it means that we are in the process of deletion and scheduled operations won't be processed. https://codereview.chromium.org/2443573003/diff/140001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/2443573003/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:468: // AudioOutputDelegate twice. On 2016/10/27 15:04:38, Max Morin Chromium wrote: > On 2016/10/27 09:52:39, o1ka wrote: > > Is this comment still relevant? > I think so. OnCloseStream can be called from all over the place, so it's hard to > be sure it will only be scheduled once. Acknowledged.
https://codereview.chromium.org/2443573003/diff/140001/content/browser/render... File content/browser/renderer_host/media/audio_output_delegate.cc (right): https://codereview.chromium.org/2443573003/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:42: media_observer->OnCreatingAudioStream(render_process_id_, render_frame_id_); On 2016/10/27 09:52:38, o1ka wrote: > We can do it in Create() now. Why is that preferable? https://codereview.chromium.org/2443573003/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:47: MediaInternals::GetInstance()->SetWebContentsTitleForAudioLogEntry( On 2016/10/27 12:59:52, o1ka wrote: > Would be nice to avoid dependency on MediaInternals here. Can we set the title > before passing the log in constructor? Also, this method can't be called on > FakeAudioLogImpl, see > https://cs.chromium.org/chromium/src/content/browser/media/media_internals.cc.... There is probably a reason OnCreated is called before SetWebCont..., so I want to leave them in that order. I could move the logging call out as well, but I think it's neater to have all the logging in this class. https://codereview.chromium.org/2443573003/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:62: delegate->UpdatePlayingStateForHandler(false); On 2016/10/28 09:13:22, o1ka wrote: > It will call handler_->OnStreamStateChange, and this is not allowed. > setting |handler_| to nullptr should be the first thing we do in this method > (and add a comment that this way we are indicated the start of shutdown). I see the point that the handler could otherwise be partially-destructed. However, we need to set playing state to false before we are destructed, or the stream count in ARH will be incorrect. Maybe we could just add a comment to Create stating that |handler| must be valid until the destructor of the returned UniquePtr is ran? In ARH, |delegates_| is emptied in OnChannelClosing anyways, and the ARH destructor DCHECKs that |delegates_| is empty anyways. https://codereview.chromium.org/2443573003/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:65: // Keep delegate alive until controller is closed: On 2016/10/28 09:13:22, o1ka wrote: > Could you extend the comment to explain that controller will reply with the > passed callback on this same thread after it closes the stream, and at that > point we can safely delete both delegate and controller (which will be done when > callback is destroyed). > This may save a couple of hours of staring into the code for somebody (and > probably for you in a year ;) ) Done. https://codereview.chromium.org/2443573003/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:170: playing_ = playing; On 2016/10/28 09:13:22, o1ka wrote: > On 2016/10/27 15:04:38, Max Morin Chromium wrote: > > On 2016/10/27 09:52:38, o1ka wrote: > > > Note that we want to update |playing_| even if |handler_| is nullpltr. > > otherwise > > > you may just hit DCHECK(!playing_) in the destructor. > > > Overall, looks like all the code needs to be modified like that: > > > if (handler_) > > > handler_->On... > > > > > > Otherwise resetting |handler_| changes the behavior in many places. > > > > If we reset the handler while |playing_| is true, it's an error (the stream > will > > never be removed from the count that the handler keeps). The only place where > > the handler is reset is in Deleter::operator(), and it calls > > UpdatePlayingStateForHandler(false) before, so |playing_| will be stuck at > false > > until the destructor is ran. > > Ok, sounds good. Than let's (1) DCHECK |handler_| here; (2) comment on > |handler_| in the header that if it's nullptr it means that we are in the > process of deletion and scheduled operations won't be processed. Done.
Now with tests. PTAL. https://codereview.chromium.org/2443573003/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/2443573003/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:175: OnCloseStream(delegates_[0]->stream_id()); On 2016/10/26 09:20:18, Max Morin Chromium wrote: > On 2016/10/25 15:59:41, o1ka wrote: > > it would probably be more a bit more efficient to work on delegates_->back() > > Only if we also search backwards. I updated the code. Actually this loop is ugly and confusing and not needed anymore, so I removed it.
Description was changed from ========== Factor out AudioOutputDelegate from AudioRendererHost. BUG=656923 ========== to ========== Factor out AudioOutputDelegate from AudioRendererHost. This change moves logic related to IPC requests regarding a single stream into its own class. This prepares for adding a mojo alternative to ARH in two ways. The mojo implementation will have a separate interface for an audio stream, which makes it very reasonable to have a class responsible for handling requests to a stream. This change also slims down ARH, so that it's easier to replace. BUG=656923 ==========
https://codereview.chromium.org/2443573003/diff/140001/content/browser/render... File content/browser/renderer_host/media/audio_output_delegate.cc (right): https://codereview.chromium.org/2443573003/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:62: delegate->UpdatePlayingStateForHandler(false); On 2016/10/28 14:55:47, Max Morin Chromium wrote: > On 2016/10/28 09:13:22, o1ka wrote: > > It will call handler_->OnStreamStateChange, and this is not allowed. > > setting |handler_| to nullptr should be the first thing we do in this method > > (and add a comment that this way we are indicated the start of shutdown). > > I see the point that the handler could otherwise be partially-destructed. > However, we need to set playing state to false before we are destructed, or the > stream count in ARH will be incorrect. Maybe we could just add a comment to > Create stating that |handler| must be valid until the destructor of the returned > UniquePtr is ran? In ARH, |delegates_| is emptied in OnChannelClosing anyways, > and the ARH destructor DCHECKs that |delegates_| is empty anyways. Have you done that? https://codereview.chromium.org/2443573003/diff/220001/content/browser/BUILD.gn File content/browser/BUILD.gn (right): https://codereview.chromium.org/2443573003/diff/220001/content/browser/BUILD.... content/browser/BUILD.gn:1: # Copyright 2014 The Chromium Authors. All rights reserved. (It would be convenient to point to the design doc or diagrams in the review) https://codereview.chromium.org/2443573003/diff/220001/content/browser/render... File content/browser/renderer_host/media/audio_output_delegate.cc (right): https://codereview.chromium.org/2443573003/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:27: : handler_(handler), Either we need to DCHECK(handler_) or we need to have "if" before l.62 in the deleter. https://codereview.chromium.org/2443573003/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:53: DCHECK(!playing_); DCHECK(!handler_) probably? https://codereview.chromium.org/2443573003/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:54: AudioStreamMonitor::StopMonitoringStream(render_process_id_, render_frame_id_, I suspect we do not monitor at this point? Is it fine to stop twice? https://codereview.chromium.org/2443573003/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:57: mirroring_manager_->RemoveDiverter(controller_.get()); Please restore the comment on why diverter is removed here. https://codereview.chromium.org/2443573003/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:72: // static nit: add a line before https://codereview.chromium.org/2443573003/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:136: AudioStreamMonitor::StartMonitoringStream( We want to do it only if we switched from pause to play, right? https://codereview.chromium.org/2443573003/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:139: controller_)); I would add an explicit comment that AudioStreamMonitor will hold a ref ptr to the controller (So, basically, we never know when the controller is deleted: note that "StopMonitoring..." is async - which probably also makes sense to mention in the destructor). https://codereview.chromium.org/2443573003/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:154: AudioStreamMonitor::StopMonitoringStream(render_process_id_, render_frame_id_, same here: if we do not monitor it, do we want to stop? https://codereview.chromium.org/2443573003/diff/220001/content/browser/render... File content/browser/renderer_host/media/audio_output_delegate.h (right): https://codereview.chromium.org/2443573003/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.h:31: // most have been one more "playing" call than "not playing" call, and in A little hard to parse sentence. Can we simplify it? https://codereview.chromium.org/2443573003/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.h:32: // total (when destroyed) there will have been an equal number of "playing" can we avoid future perfect? :) we have enough of tricky c++ here to have tricky English on top ;) https://codereview.chromium.org/2443573003/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.h:50: static UniquePtr Create(EventHandler* handler, Add a description of what the guarantee is for calling |handler|, and specifically state that Deleter must not be called on |handler| destruction. https://codereview.chromium.org/2443573003/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.h:63: const scoped_refptr<media::AudioOutputController>& controller() const { Please add a comment on who uses it and what happens when Deleter is executed when somebody holds a reference to the controller. https://codereview.chromium.org/2443573003/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.h:85: // AudioOutputController::EventHandler implementation. nit: empty line before https://codereview.chromium.org/2443573003/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.h:93: // handler_ will be null we are in the process of destruction. In this case, nits: |handler_|; "if we are" https://codereview.chromium.org/2443573003/diff/220001/content/browser/render... File content/browser/renderer_host/media/audio_output_delegate_unittest.cc (right): https://codereview.chromium.org/2443573003/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate_unittest.cc:28: namespace content { nit: empty line after each namespace { https://codereview.chromium.org/2443573003/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate_unittest.cc:103: TestBrowserThreadBundle thread_bundle_; Do we want to have some real threads? (I think we do, because of our complex deletion chain) https://codereview.chromium.org/2443573003/diff/220001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/2443573003/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:407: std::unique_ptr<media::AudioLog> audio_log = log_factory_->CreateAudioLog( This |log_factory_| is MediaInternals, right (it cannot be anything else, otherwise "SetWebContents..." will do an invalid cast)? Do we really need to pass it then, if we use it explicitly on the next line?
Description was changed from ========== Factor out AudioOutputDelegate from AudioRendererHost. This change moves logic related to IPC requests regarding a single stream into its own class. This prepares for adding a mojo alternative to ARH in two ways. The mojo implementation will have a separate interface for an audio stream, which makes it very reasonable to have a class responsible for handling requests to a stream. This change also slims down ARH, so that it's easier to replace. BUG=656923 ========== to ========== Factor out AudioOutputDelegate from AudioRendererHost. This change moves logic related to IPC requests regarding a single stream into its own class. This prepares for adding a mojo alternative to ARH. The mojo implementation will have a separate interface for an audio stream, which makes it very reasonable to have a class responsible for handling requests to a stream. This change also slims down ARH, so that it's easier to replace. Reference for mojofication work: https://docs.google.com/document/d/1PjV_EutyJANmsEjhH9mTNZWSslTPvAit7Cy2RO3cb... BUG=656923 ==========
Description was changed from ========== Factor out AudioOutputDelegate from AudioRendererHost. This change moves logic related to IPC requests regarding a single stream into its own class. This prepares for adding a mojo alternative to ARH. The mojo implementation will have a separate interface for an audio stream, which makes it very reasonable to have a class responsible for handling requests to a stream. This change also slims down ARH, so that it's easier to replace. Reference for mojofication work: https://docs.google.com/document/d/1PjV_EutyJANmsEjhH9mTNZWSslTPvAit7Cy2RO3cb... BUG=656923 ========== to ========== Factor out AudioOutputDelegate from AudioRendererHost. This change moves logic related to IPC requests regarding a single stream into its own class. This prepares for adding a mojo alternative to ARH. The mojo implementation will have a separate interface for an audio stream, which makes it very reasonable to have a class responsible for handling requests to a stream. This change also slims down ARH, so that it's easier to replace. Reference for mojofication work: https://docs.google.com/document/d/1PjV_EutyJANmsEjhH9mTNZWSslTPvAit7Cy2RO3cb..., in particular the diagram https://docs.google.com/drawings/d/1xg6z2FwLdTe5eG-Nvek9q-IfWzMI01XxlbVH3y0mz.... BUG=656923 ==========
Description was changed from ========== Factor out AudioOutputDelegate from AudioRendererHost. This change moves logic related to IPC requests regarding a single stream into its own class. This prepares for adding a mojo alternative to ARH. The mojo implementation will have a separate interface for an audio stream, which makes it very reasonable to have a class responsible for handling requests to a stream. This change also slims down ARH, so that it's easier to replace. Reference for mojofication work: https://docs.google.com/document/d/1PjV_EutyJANmsEjhH9mTNZWSslTPvAit7Cy2RO3cb..., in particular the diagram https://docs.google.com/drawings/d/1xg6z2FwLdTe5eG-Nvek9q-IfWzMI01XxlbVH3y0mz.... BUG=656923 ========== to ========== Factor out AudioOutputDelegate from AudioRendererHost. This change moves logic related to IPC requests regarding a single stream into its own class. This prepares for adding a mojo alternative to ARH. The mojo implementation will have a separate interface for an audio stream, which makes it very reasonable to have a class responsible for handling requests to a stream. This change also slims down ARH, so that it's easier to replace. Reference for mojofication work: https://docs.google.com/document/d/1PjV_EutyJANmsEjhH9mTNZWSslTPvAit7Cy2RO3cb..., in particular the diagram https://docs.google.com/drawings/d/1xg6z2FwLdTe5eG-Nvek9q-IfWzMI01XxlbVH3y0mz.... BUG=656923 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
PTAL https://codereview.chromium.org/2443573003/diff/220001/content/browser/BUILD.gn File content/browser/BUILD.gn (right): https://codereview.chromium.org/2443573003/diff/220001/content/browser/BUILD.... content/browser/BUILD.gn:1: # Copyright 2014 The Chromium Authors. All rights reserved. On 2016/11/03 13:39:12, o1ka wrote: > (It would be convenient to point to the design doc or diagrams in the review) Done. https://codereview.chromium.org/2443573003/diff/220001/content/browser/render... File content/browser/renderer_host/media/audio_output_delegate.cc (right): https://codereview.chromium.org/2443573003/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:27: : handler_(handler), On 2016/11/03 13:39:12, o1ka wrote: > Either we need to DCHECK(handler_) or we need to have "if" before l.62 in the > deleter. Done. https://codereview.chromium.org/2443573003/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:53: DCHECK(!playing_); On 2016/11/03 13:39:12, o1ka wrote: > DCHECK(!handler_) probably? Done. https://codereview.chromium.org/2443573003/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:54: AudioStreamMonitor::StopMonitoringStream(render_process_id_, render_frame_id_, On 2016/11/03 13:39:12, o1ka wrote: > I suspect we do not monitor at this point? Is it fine to stop twice? Yes, it's fine to start monitoring while already monitoring (in this case, the old monitoring is overwritten with the new one) and it's also fine to stop a nonexistent monitoring (it's a thread hop followed by a no-op). It does not seem like controller pauses the stream while closing it, so if it's not paused before closing, it will have monitoring. To simplify it a bit, I moved the start/stop monitoring into the same function as the one notifying handler. This also means that monitoring is stopped when the deleter is called, so we do not need to stop monitoring here. https://codereview.chromium.org/2443573003/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:57: mirroring_manager_->RemoveDiverter(controller_.get()); On 2016/11/03 13:39:12, o1ka wrote: > Please restore the comment on why diverter is removed here. Done. https://codereview.chromium.org/2443573003/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:72: // static On 2016/11/03 13:39:12, o1ka wrote: > nit: add a line before Done. https://codereview.chromium.org/2443573003/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:136: AudioStreamMonitor::StartMonitoringStream( On 2016/11/03 13:39:12, o1ka wrote: > We want to do it only if we switched from pause to play, right? Done. https://codereview.chromium.org/2443573003/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:139: controller_)); On 2016/11/03 13:39:12, o1ka wrote: > I would add an explicit comment that AudioStreamMonitor will hold a ref ptr to > the controller (So, basically, we never know when the controller is deleted: > note that "StopMonitoring..." is async - which probably also makes sense to > mention in the destructor). Good point. We don't have to worry too much since the controller doesn't send notifications when closed, but it's good to keep its lifetime in mind. https://codereview.chromium.org/2443573003/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:154: AudioStreamMonitor::StopMonitoringStream(render_process_id_, render_frame_id_, On 2016/11/03 13:39:12, o1ka wrote: > same here: if we do not monitor it, do we want to stop? Done. https://codereview.chromium.org/2443573003/diff/220001/content/browser/render... File content/browser/renderer_host/media/audio_output_delegate.h (right): https://codereview.chromium.org/2443573003/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.h:31: // most have been one more "playing" call than "not playing" call, and in On 2016/11/03 13:39:12, o1ka wrote: > A little hard to parse sentence. Can we simplify it? Done. https://codereview.chromium.org/2443573003/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.h:32: // total (when destroyed) there will have been an equal number of "playing" On 2016/11/03 13:39:12, o1ka wrote: > can we avoid future perfect? :) we have enough of tricky c++ here to have tricky > English on top ;) Done. https://codereview.chromium.org/2443573003/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.h:50: static UniquePtr Create(EventHandler* handler, On 2016/11/03 13:39:12, o1ka wrote: > Add a description of what the guarantee is for calling |handler|, and > specifically state that Deleter must not be called on |handler| destruction. Done. https://codereview.chromium.org/2443573003/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.h:63: const scoped_refptr<media::AudioOutputController>& controller() const { On 2016/11/03 13:39:12, o1ka wrote: > Please add a comment on who uses it and what happens when Deleter is executed > when somebody holds a reference to the controller. Done. https://codereview.chromium.org/2443573003/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.h:85: // AudioOutputController::EventHandler implementation. On 2016/11/03 13:39:12, o1ka wrote: > nit: empty line before Done. https://codereview.chromium.org/2443573003/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.h:93: // handler_ will be null we are in the process of destruction. In this case, On 2016/11/03 13:39:12, o1ka wrote: > nits: |handler_|; "if we are" Done. https://codereview.chromium.org/2443573003/diff/220001/content/browser/render... File content/browser/renderer_host/media/audio_output_delegate_unittest.cc (right): https://codereview.chromium.org/2443573003/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate_unittest.cc:28: namespace content { On 2016/11/03 13:39:12, o1ka wrote: > nit: empty line after each namespace { Done. https://codereview.chromium.org/2443573003/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate_unittest.cc:103: TestBrowserThreadBundle thread_bundle_; On 2016/11/03 13:39:12, o1ka wrote: > Do we want to have some real threads? (I think we do, because of our complex > deletion chain) Done. I learned from the authorization CL that a separate UI thread is a bad idea (since it'll mess up global state for other tests), but I added IO and audio threads. https://codereview.chromium.org/2443573003/diff/220001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/2443573003/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:407: std::unique_ptr<media::AudioLog> audio_log = log_factory_->CreateAudioLog( On 2016/11/03 13:39:12, o1ka wrote: > This |log_factory_| is MediaInternals, right (it cannot be anything else, > otherwise "SetWebContents..." will do an invalid cast)? Do we really need to > pass it then, if we use it explicitly on the next line? Done.
Very nice! https://codereview.chromium.org/2443573003/diff/240001/content/browser/render... File content/browser/renderer_host/media/audio_output_delegate.cc (right): https://codereview.chromium.org/2443573003/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:43: if (media_observer) Do it in Create() instead? https://codereview.chromium.org/2443573003/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:45: if (mirroring_manager_) Instead of keeping pointer in AudioOutputDelegate, you can probably add diverter in Create() and remove it in Deleter? Looks like we keep the pointer in AOD to only remove diverter in the destructor. Then making Deleter to manage that seems to be a better separation of concerns. https://codereview.chromium.org/2443573003/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:110: controller_->SetVolume(volume); We may want to sanitize volume here (https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/audi...) to avoid doing it for IPC/Mojo separately. https://codereview.chromium.org/2443573003/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:178: // Note that this takes a reference to |controller_|, and (just in case you are fixing nits I pointed to: extra line before comment is not needed in such cases) https://codereview.chromium.org/2443573003/diff/240001/content/browser/render... File content/browser/renderer_host/media/audio_output_delegate.h (right): https://codereview.chromium.org/2443573003/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.h:30: // Suitable for tracking the number of active streams. This comment is not clear. Just explain when it is called (and on which thread). https://codereview.chromium.org/2443573003/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.h:32: // Called when construction is finished and the stream is ready for nit: everywhere: add a blank line between a method declaration and the next method comment https://codereview.chromium.org/2443573003/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.h:62: // This function is used to provide control of the audio stream to I'm not sure if this is true. As far as I remember, webrtc extensions use it to get some info only, and they do not control streams. https://codereview.chromium.org/2443573003/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.h:100: // case, we will not run any scheduled operations. Comment is not quite clear: scheduled where/for whom? https://codereview.chromium.org/2443573003/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.h:109: // This flag ensures that we only send OnStreamStateChanged notifications not: add an empty line before (and in other similar places) https://codereview.chromium.org/2443573003/diff/240001/content/browser/render... File content/browser/renderer_host/media/audio_output_delegate_unittest.cc (right): https://codereview.chromium.org/2443573003/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate_unittest.cc:30: // Not tested: add "yet", it will look cooler ;) https://codereview.chromium.org/2443573003/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate_unittest.cc:46: MockAudioMirroringManager() : AudioMirroringManager() {} just =default? https://codereview.chromium.org/2443573003/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate_unittest.cc:47: virtual ~MockAudioMirroringManager() {} override instead https://codereview.chromium.org/2443573003/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate_unittest.cc:155: NiceMock<MockAudioMirroringManager> mirroring_manager_; Question: why NiceMocks are needed here? https://codereview.chromium.org/2443573003/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate_unittest.cc:179: AudioOutputDelegate::UniquePtr delegate = CreateDelegate(); On which thread delegates are created in real life? I suspect it's on IO thread. https://codereview.chromium.org/2443573003/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate_unittest.cc:351: TEST_F(AudioOutputDelegateTest, Pause_DoesNotCallHandler) { Naming needs clarification https://codereview.chromium.org/2443573003/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate_unittest.cc:379: } // namespace content I'm not sure if various cases of destruction sequence are covered - and destruction is the most sensitive part of the code here. https://codereview.chromium.org/2443573003/diff/240001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/2443573003/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:141: g_audio_streams_tracker.Get().DecreaseStreamCount(delegates_.size()); Comment that OnCloseStream won't be called, and that's why stream count is decreased here.
https://codereview.chromium.org/2443573003/diff/240001/content/browser/render... File content/browser/renderer_host/media/audio_output_delegate.cc (right): https://codereview.chromium.org/2443573003/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:43: if (media_observer) On 2016/11/22 13:24:03, o1ka wrote: > Do it in Create() instead? Done. https://codereview.chromium.org/2443573003/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:45: if (mirroring_manager_) On 2016/11/22 13:24:03, o1ka wrote: > Instead of keeping pointer in AudioOutputDelegate, you can probably add diverter > in Create() and remove it in Deleter? Looks like we keep the pointer in AOD to > only remove diverter in the destructor. Then making Deleter to manage that seems > to be a better separation of concerns. Very well. Seems like it just makes the code more complicated though? https://codereview.chromium.org/2443573003/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:110: controller_->SetVolume(volume); On 2016/11/22 13:24:03, o1ka wrote: > We may want to sanitize volume here > (https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/audi...) > to avoid doing it for IPC/Mojo separately. Makes more sense to sanitize in the IPC layer. Ideally, this class doesn't know/care that it's near an IPC edge. I added a DCHECK. https://codereview.chromium.org/2443573003/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:178: // Note that this takes a reference to |controller_|, and On 2016/11/22 13:24:03, o1ka wrote: > (just in case you are fixing nits I pointed to: extra line before comment is not > needed in such cases) Acknowledged. https://codereview.chromium.org/2443573003/diff/240001/content/browser/render... File content/browser/renderer_host/media/audio_output_delegate.h (right): https://codereview.chromium.org/2443573003/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.h:30: // Suitable for tracking the number of active streams. On 2016/11/22 13:24:03, o1ka wrote: > This comment is not clear. Just explain when it is called (and on which thread). Done. https://codereview.chromium.org/2443573003/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.h:32: // Called when construction is finished and the stream is ready for On 2016/11/22 13:24:03, o1ka wrote: > nit: everywhere: add a blank line between a method declaration and the next > method comment Done. https://codereview.chromium.org/2443573003/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.h:62: // This function is used to provide control of the audio stream to On 2016/11/22 13:24:03, o1ka wrote: > I'm not sure if this is true. As far as I remember, webrtc extensions use it to > get some info only, and they do not control streams. See https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/webrtc_aud... https://codereview.chromium.org/2443573003/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.h:100: // case, we will not run any scheduled operations. On 2016/11/22 13:24:03, o1ka wrote: > Comment is not quite clear: scheduled where/for whom? Done. https://codereview.chromium.org/2443573003/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.h:109: // This flag ensures that we only send OnStreamStateChanged notifications On 2016/11/22 13:24:03, o1ka wrote: > not: add an empty line before (and in other similar places) Done. https://codereview.chromium.org/2443573003/diff/240001/content/browser/render... File content/browser/renderer_host/media/audio_output_delegate_unittest.cc (right): https://codereview.chromium.org/2443573003/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate_unittest.cc:30: // Not tested: On 2016/11/22 13:24:03, o1ka wrote: > add "yet", it will look cooler ;) Done. https://codereview.chromium.org/2443573003/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate_unittest.cc:46: MockAudioMirroringManager() : AudioMirroringManager() {} On 2016/11/22 13:24:03, o1ka wrote: > just =default? Done. https://codereview.chromium.org/2443573003/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate_unittest.cc:47: virtual ~MockAudioMirroringManager() {} On 2016/11/22 13:24:03, o1ka wrote: > override instead Done. https://codereview.chromium.org/2443573003/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate_unittest.cc:155: NiceMock<MockAudioMirroringManager> mirroring_manager_; On 2016/11/22 13:24:04, o1ka wrote: > Question: why NiceMocks are needed here? The test output is hard to read if there is a bunch of gmock spam. https://codereview.chromium.org/2443573003/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate_unittest.cc:179: AudioOutputDelegate::UniquePtr delegate = CreateDelegate(); On 2016/11/22 13:24:03, o1ka wrote: > On which thread delegates are created in real life? I suspect it's on IO thread. Yes. This method actually created them on the IO thread. I renamed the function. https://codereview.chromium.org/2443573003/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate_unittest.cc:351: TEST_F(AudioOutputDelegateTest, Pause_DoesNotCallHandler) { On 2016/11/22 13:24:03, o1ka wrote: > Naming needs clarification Done. https://codereview.chromium.org/2443573003/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate_unittest.cc:379: } // namespace content On 2016/11/22 13:24:03, o1ka wrote: > I'm not sure if various cases of destruction sequence are covered - and > destruction is the most sensitive part of the code here. There are cases for various events being in progress (play, pause, create, error) as well as checks for all the handlers during creation and destruction (I merged those tests into a test "CreateAndDelete"). There is only one case of the destruction sequence (disregarding null checks of the delegate and the mirroring manager, those will never be null anyways). https://codereview.chromium.org/2443573003/diff/240001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/2443573003/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:141: g_audio_streams_tracker.Get().DecreaseStreamCount(delegates_.size()); On 2016/11/22 13:24:04, o1ka wrote: > Comment that OnCloseStream won't be called, and that's why stream count is > decreased here. Done.
https://codereview.chromium.org/2443573003/diff/240001/content/browser/render... File content/browser/renderer_host/media/audio_output_delegate.cc (right): https://codereview.chromium.org/2443573003/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:45: if (mirroring_manager_) On 2016/11/22 16:04:29, Max Morin Chromium wrote: > On 2016/11/22 13:24:03, o1ka wrote: > > Instead of keeping pointer in AudioOutputDelegate, you can probably add > diverter > > in Create() and remove it in Deleter? Looks like we keep the pointer in AOD to > > only remove diverter in the destructor. Then making Deleter to manage that > seems > > to be a better separation of concerns. > > Very well. Seems like it just makes the code more complicated though? To me it looks fine. Maybe replacing Owned and lambda with Passed and a helper function which takes ownership of diverter will simplify reading a bit. Up to you. https://codereview.chromium.org/2443573003/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:110: controller_->SetVolume(volume); On 2016/11/22 16:04:29, Max Morin Chromium wrote: > On 2016/11/22 13:24:03, o1ka wrote: > > We may want to sanitize volume here > > > (https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/audi...) > > to avoid doing it for IPC/Mojo separately. > > Makes more sense to sanitize in the IPC layer. Ideally, this class doesn't > know/care that it's near an IPC edge. I added a DCHECK. Acknowledged. https://codereview.chromium.org/2443573003/diff/240001/content/browser/render... File content/browser/renderer_host/media/audio_output_delegate.h (right): https://codereview.chromium.org/2443573003/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.h:62: // This function is used to provide control of the audio stream to On 2016/11/22 16:04:29, Max Morin Chromium wrote: > On 2016/11/22 13:24:03, o1ka wrote: > > I'm not sure if this is true. As far as I remember, webrtc extensions use it > to > > get some info only, and they do not control streams. > > See > https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/webrtc_aud... Acknowledged. https://codereview.chromium.org/2443573003/diff/240001/content/browser/render... File content/browser/renderer_host/media/audio_output_delegate_unittest.cc (right): https://codereview.chromium.org/2443573003/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate_unittest.cc:155: NiceMock<MockAudioMirroringManager> mirroring_manager_; On 2016/11/22 16:04:29, Max Morin Chromium wrote: > On 2016/11/22 13:24:04, o1ka wrote: > > Question: why NiceMocks are needed here? > > The test output is hard to read if there is a bunch of gmock spam. But should not you set all the appropriate expectations instead? https://codereview.chromium.org/2443573003/diff/260001/content/browser/render... File content/browser/renderer_host/media/audio_output_delegate.cc (right): https://codereview.chromium.org/2443573003/diff/260001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:73: if (mirroring_manager) We need more comments :) It's ok that RemoveDiverter() won't be called if the callback is destroyed on message loop destruction - because |mirroring_manager| is leaky and does not care if there are unregistered diverters. https://codereview.chromium.org/2443573003/diff/260001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:118: DCHECK(volume >= 0 || volume <= 1); && ? https://codereview.chromium.org/2443573003/diff/260001/content/browser/render... File content/browser/renderer_host/media/audio_output_delegate_unittest.cc (right): https://codereview.chromium.org/2443573003/diff/260001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate_unittest.cc:93: TestBrowserThreadBundle::Options::REAL_IO_THREAD); Add a comment on why this threading setup is chosen for tests (IO/UI/Audio thread) https://codereview.chromium.org/2443573003/diff/260001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate_unittest.cc:208: BrowserThread::PostTask( I still think it does not make sense to post each step to IO thread separately and then wait for its completion, and it is not what happens in real life. https://codereview.chromium.org/2443573003/diff/260001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/2443573003/diff/260001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:142: // removal isn't done through OnCloseStream. OnCloseStream won't be called because ...; so decrementing stream tracker here.
https://codereview.chromium.org/2443573003/diff/240001/content/browser/render... File content/browser/renderer_host/media/audio_output_delegate.cc (right): https://codereview.chromium.org/2443573003/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:45: if (mirroring_manager_) On 2016/11/22 21:48:16, o1ka wrote: > On 2016/11/22 16:04:29, Max Morin Chromium wrote: > > On 2016/11/22 13:24:03, o1ka wrote: > > > Instead of keeping pointer in AudioOutputDelegate, you can probably add > > diverter > > > in Create() and remove it in Deleter? Looks like we keep the pointer in AOD > to > > > only remove diverter in the destructor. Then making Deleter to manage that > > seems > > > to be a better separation of concerns. > > > > Very well. Seems like it just makes the code more complicated though? > > To me it looks fine. Maybe replacing Owned and lambda with Passed and a helper > function which takes ownership of diverter will simplify reading a bit. Up to > you. This is fine. Another possibility: ScopedDiverterRegistration, which adds a diverter to the mirroring manager on construction and removes it on destruction. Sounds like over-engineering though :) https://codereview.chromium.org/2443573003/diff/240001/content/browser/render... File content/browser/renderer_host/media/audio_output_delegate_unittest.cc (right): https://codereview.chromium.org/2443573003/diff/240001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate_unittest.cc:155: NiceMock<MockAudioMirroringManager> mirroring_manager_; On 2016/11/22 21:48:16, o1ka wrote: > On 2016/11/22 16:04:29, Max Morin Chromium wrote: > > On 2016/11/22 13:24:04, o1ka wrote: > > > Question: why NiceMocks are needed here? > > > > The test output is hard to read if there is a bunch of gmock spam. > > But should not you set all the appropriate expectations instead? Ok then. In my opinion, this makes it less clear what each test case is really supposed to test. https://codereview.chromium.org/2443573003/diff/260001/content/browser/render... File content/browser/renderer_host/media/audio_output_delegate.cc (right): https://codereview.chromium.org/2443573003/diff/260001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:73: if (mirroring_manager) On 2016/11/22 21:48:16, o1ka wrote: > We need more comments :) > It's ok that RemoveDiverter() won't be called if the callback is destroyed on > message loop destruction > - because |mirroring_manager| is leaky and does not care if there are > unregistered diverters. Done. https://codereview.chromium.org/2443573003/diff/260001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:118: DCHECK(volume >= 0 || volume <= 1); On 2016/11/22 21:48:16, o1ka wrote: > && ? Yes https://codereview.chromium.org/2443573003/diff/260001/content/browser/render... File content/browser/renderer_host/media/audio_output_delegate_unittest.cc (right): https://codereview.chromium.org/2443573003/diff/260001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate_unittest.cc:93: TestBrowserThreadBundle::Options::REAL_IO_THREAD); On 2016/11/22 21:48:16, o1ka wrote: > Add a comment on why this threading setup is chosen for tests (IO/UI/Audio > thread) Done. https://codereview.chromium.org/2443573003/diff/260001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate_unittest.cc:208: BrowserThread::PostTask( On 2016/11/22 21:48:16, o1ka wrote: > I still think it does not make sense to post each step to IO thread separately > and then wait for its completion, and it is not what happens in real life. I fixed this now. https://codereview.chromium.org/2443573003/diff/260001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/2443573003/diff/260001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:142: // removal isn't done through OnCloseStream. On 2016/11/22 21:48:16, o1ka wrote: > OnCloseStream won't be called because ...; so decrementing stream tracker here. Not sure what you mean. OnCloseStream won't be called because we are not calling it :)
I also checked what it would take to test AudioStreamMonitor. Setting up fake webcontents et c. is possible (but adds a second of execution time for the test), but we must also feed nonzero audio data to the AudioStreamMonitor. This might be done with something like a TestAudioOutputControllerFactory (similar thing exists for input https://cs.chromium.org/chromium/src/media/audio/test_audio_input_controller_..., so we should probably do it symmetrically).
Ping!
Description was changed from ========== Factor out AudioOutputDelegate from AudioRendererHost. This change moves logic related to IPC requests regarding a single stream into its own class. This prepares for adding a mojo alternative to ARH. The mojo implementation will have a separate interface for an audio stream, which makes it very reasonable to have a class responsible for handling requests to a stream. This change also slims down ARH, so that it's easier to replace. Reference for mojofication work: https://docs.google.com/document/d/1PjV_EutyJANmsEjhH9mTNZWSslTPvAit7Cy2RO3cb..., in particular the diagram https://docs.google.com/drawings/d/1xg6z2FwLdTe5eG-Nvek9q-IfWzMI01XxlbVH3y0mz.... BUG=656923 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Factor out AudioOutputDelegate from AudioRendererHost. This change moves logic related to IPC requests regarding a single stream into its own class. This prepares for adding a mojo alternative to ARH. The mojo implementation will have a separate interface for an audio stream, which makes it very reasonable to have a class responsible for handling requests to a stream. This change also slims down ARH, so that it's easier to replace. Reference for mojofication work: https://docs.google.com/document/d/1PjV_EutyJANmsEjhH9mTNZWSslTPvAit7Cy2RO3cb..., in particular the diagram https://docs.google.com/drawings/d/1xg6z2FwLdTe5eG-Nvek9q-IfWzMI01XxlbVH3y0mz.... BUG=656923 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
maxmorin@chromium.org changed reviewers: + dalecurtis@chromium.org, miu@chromium.org
Dale, Miu: PTAL at this refactoring. Sorry for the large CL, at least most of it is tests :). A specific question: The old code had audio_log_->OnCreated(...); MediaInternals::GetInstance()->SetWebContentsTitleForAudioLogEntry(..., audio_log_.get()); and in this CL I first set the title of the audio log and then pass it into the AudioOutputDelegate which calls OnCreated, so that AudioOutputDelegate doesn't have a dependency on MediaInternals. Is there a specific reason for the old order of calls?
Overall looking great, thanks for the cleanup! https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... File content/browser/renderer_host/media/audio_output_delegate.cc (right): https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:28: controller_(media::AudioOutputController::Create(audio_manager, This should probably be constructed last since it's handing |this| off to a thread could theoretically issue callbacks before construction finishes. https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:55: Early return if !mirroring_manager (don't forget to delete)? I forget if that happens only in tests. https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:56: scoped_refptr<media::AudioOutputController> controller = .swap? https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:81: base::Owned(delegate), base::Unretained(mirroring_manager_), controller)); std::move(controller)? https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:123: DCHECK(volume >= 0 && volume <= 1); Separate into DCHECK_GE() and DCHECK_LE() for easier debugging. https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:133: weak_factory_.GetWeakPtr())); Precreate a weak_this_ during construction and use it instead. You can't reliably issue WeakPtrs from a different thread than the WeakPtr is intended for. https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:144: if (!BrowserThread::CurrentlyOn(BrowserThread::IO)) { This block (l.144-l.152) is repeated fairly regularly. Perhaps a helper function that lets you do something like this would be cleaner? if (ShouldAbortOrTrampolineToIO(&AudioOutputDelegate::OnControllerPlaying)) return; https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:177: audio_log_->OnError(stream_id_); Why the exception for handler_ here? https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:177: audio_log_->OnError(stream_id_); Why the exception for handler_ here? https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:188: if (playing != playing_) { If playing == playing_ return. https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... File content/browser/renderer_host/media/audio_output_delegate.h (right): https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.h:14: #include "base/sync_socket.h" Forward declare. This and almost all the others. https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.h:26: : private media::AudioOutputController::EventHandler { Google C++ only allows public inheritance: https://google.github.io/styleguide/cppguide.html#Inheritance https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.h:28: class EventHandler { This must have a virtual destructor: https://google.github.io/styleguide/cppguide.html#Interfaces https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.h:56: using UniquePtr = std::unique_ptr<AudioOutputDelegate, Deleter>; Needs a more "unique" name, though since this is only used in a handful of places, it seems fine to always specify it. https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.h:58: // |handler| must be valid until the moment that the UniquePtr destructor Needs more comments. https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.h:60: static UniquePtr Create(EventHandler* handler, It's sad there are so many parameters here. I don't have any obvious ideas for removal, but it's worth thinking about. https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.h:79: const scoped_refptr<media::AudioOutputController>& controller() const { Shouldn't this be returned by value? or a raw ptr if it's not expected to own. https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.h:125: } // namespace content Add line above. https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... File content/browser/renderer_host/media/audio_output_delegate_unittest.cc (right): https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate_unittest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. No (c), 2016? https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate_unittest.cc:34: // Not yet tested: TBD? https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate_unittest.cc:49: struct MockAudioMirroringManager : public AudioMirroringManager { Why use struct all over here? Seems a lie :) https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate_unittest.cc:110: // Audio manager creation stolen from content/browser/browser_main_loop.cc. Probably don't need all this. Just use one of the other fake threads in the bundle for the audio thread and worker thread. I don't think the test needs to worry about these details and they're likely to rot -- but A+ for effort :) https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:108: CHECK(delegates_.empty()); Probably can go back to a DCHECK() now, was for debugging IIRC. https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:161: SendErrorMessage(stream_id); Why a different error path than above? https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:169: // TODO(maxmorin): Figure out how/if this is safe, since shared_memory is also Can you elaborate? IIRC this completes before any usage on other threads. https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:187: base::AtomicRefCountInc(&num_playing_streams_); We might be able to drop all this code now. altimin@ added stream counting to the AudioStreamMonitor which should do everything this is doing. https://codereview.chromium.org/2496173003/ https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:378: g_audio_streams_tracker.Get().IncreaseStreamCount(); Seems like code that should be moved to AudioStreamMonitor now instead. https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:431: auto i = LookupIteratorById(stream_id); Could just use the delegates_.erase(std::remove_if()) idiom. https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host.h (right): https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.h:56: #include "base/memory/shared_memory.h" Forward declare (here and elsewhere). https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.h:86: private AudioOutputDelegate::EventHandler { No private inheritance.
My "addendum" to Dale's comments (on PS15): https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... File content/browser/renderer_host/media/audio_output_delegate.cc (right): https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:55: On 2016/11/29 20:31:22, DaleCurtis wrote: > Early return if !mirroring_manager (don't forget to delete)? I forget if that > happens only in tests. Should only be. IIRC, the only reason the mirroring manager is injected via the ctor is so tests can provide a mock. https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:144: if (!BrowserThread::CurrentlyOn(BrowserThread::IO)) { On 2016/11/29 20:31:23, DaleCurtis wrote: > This block (l.144-l.152) is repeated fairly regularly. Perhaps a helper function > that lets you do something like this would be cleaner? > > if (ShouldAbortOrTrampolineToIO(&AudioOutputDelegate::OnControllerPlaying)) > return; Or...Just unconditionally post the task, regardless of what thread you're on. Code like this says "if I'm on the IO thread, I have to call UpdatePlayingState() before returning; but if I'm not, it's okay to call it later." So, if there's no requirement that UpdatePlayingState() must be called before this method returns, just post a task. A question: The class comments say "this is operated on the IO thread." So, you could burden the caller with posting a task to the IO thread, instead of making this class self-manage. https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:148: weak_factory_.GetWeakPtr())); GetWeakPtr() is not thread-safe in this situation since GetWeakPtr() could be called while the "owning thread" is invalidating the weak pointers. https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... File content/browser/renderer_host/media/audio_output_delegate.h (right): https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.h:79: const scoped_refptr<media::AudioOutputController>& controller() const { On 2016/11/29 20:31:23, DaleCurtis wrote: > Shouldn't this be returned by value? or a raw ptr if it's not expected to own. Hmm...I don't like the shared-ownership model here, but it seems we'll be correcting that soon. Based on style guidelines, #2 applies: 1. Raw pointer if caller is not expected to take ownership. 2. scoped_refptr<...> (no const, not by ref) if caller takes ownership. https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... File content/browser/renderer_host/media/audio_output_delegate_unittest.cc (right): https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate_unittest.cc:49: struct MockAudioMirroringManager : public AudioMirroringManager { On 2016/11/29 20:31:23, DaleCurtis wrote: > Why use struct all over here? Seems a lie :) This is probably my fault (circa 2012). https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:46: if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) { ditto here: Burden the caller, not the callee with this problem. https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:138: delegates_.clear(); Get ready for some pain here. In the past, whenever I've changed the shutdown semantics of AudioRendererHost, it usually resulted in crash issues and/or several re-attempts to land in the tree. The complexity is in the shared ownership of AudioOutputController, and the timing sequence of tasks being posted across IO/UI/audio threads. So, I suggest you run browser_tests or layout tests on an ASAN bot as a sanity-check. https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:418: if (volume < 0 || volume > 1.0) Should this volume check be in the delegate instead? FWIW, there is valid semantic meaning for volume to be >1.0 (gain), and so maybe it should be up to the delegate (which would have more knowledge about what to do here). https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:438: std::swap(*i, delegates_.back()); I like this: Since ordering of this list doesn't matter, this is a much more efficient element erasure. :) https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host.h (right): https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.h:124: using AudioOutputDelegateList = std::vector<AudioOutputDelegate::UniquePtr>; If AudioOutputDelegate::UniquePtr is an alias for std::unique_ptr<AudioOutputDeletegate>, please don't alias it. Less hopping around makes code easier to read. :)
On 2016/11/29 13:53:37, Max Morin Chromium wrote: > A specific question: The old code had > ... > and in this CL I first set the title of the audio log and then pass it into the > AudioOutputDelegate which calls OnCreated, so that AudioOutputDelegate doesn't > have a dependency on MediaInternals. Is there a specific reason for the old > order of calls? I'm not sure. I'd suggest running `git blame` to see who wrote those LOC, and asking them.
Thanks for all the comments! PTAL. Dale: is there a reason for SetWebContentsTitleForAudioLogEntry to be called after AudioLog::OnCreated? https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... File content/browser/renderer_host/media/audio_output_delegate.cc (right): https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:55: On 2016/11/29 20:31:22, DaleCurtis wrote: > Early return if !mirroring_manager (don't forget to delete)? I forget if that > happens only in tests. We must wait until the controller is closed before deleting the delegate anyways, so there is no point. https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:81: base::Owned(delegate), base::Unretained(mirroring_manager_), controller)); On 2016/11/29 20:31:23, DaleCurtis wrote: > std::move(controller)? I simplified this code, PTAL. https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:144: if (!BrowserThread::CurrentlyOn(BrowserThread::IO)) { On 2016/11/29 21:36:27, miu wrote: > On 2016/11/29 20:31:23, DaleCurtis wrote: > > This block (l.144-l.152) is repeated fairly regularly. Perhaps a helper > function > > that lets you do something like this would be cleaner? > > > > if (ShouldAbortOrTrampolineToIO(&AudioOutputDelegate::OnControllerPlaying)) > > return; > > Or...Just unconditionally post the task, regardless of what thread you're on. > Code like this says "if I'm on the IO thread, I have to call > UpdatePlayingState() before returning; but if I'm not, it's okay to call it > later." So, if there's no requirement that UpdatePlayingState() must be called > before this method returns, just post a task. > > A question: The class comments say "this is operated on the IO thread." So, you > could burden the caller with posting a task to the IO thread, instead of making > this class self-manage. I was not aware of the "no private inheritance" rule. What i meant by the comment was that all the public methods are called on the IO thread. The methods implementing the AudioOutputController event handler will be called on the audio thread. The comment will be fixed. https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... File content/browser/renderer_host/media/audio_output_delegate.h (right): https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.h:56: using UniquePtr = std::unique_ptr<AudioOutputDelegate, Deleter>; On 2016/11/29 20:31:23, DaleCurtis wrote: > Needs a more "unique" name, though since this is only used in a handful of > places, it seems fine to always specify it. How about AudioOutputDelegate::ScopedPointer, similar to ScopedAudioManagerPointer? I think std::unique_ptr<AudioOutputDelegate, AudioOutputDelegate::Deleter> is too long. https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.h:79: const scoped_refptr<media::AudioOutputController>& controller() const { On 2016/11/29 21:36:27, miu wrote: > On 2016/11/29 20:31:23, DaleCurtis wrote: > > Shouldn't this be returned by value? or a raw ptr if it's not expected to own. > > Hmm...I don't like the shared-ownership model here, but it seems we'll be > correcting that soon. Based on style guidelines, #2 applies: > > 1. Raw pointer if caller is not expected to take ownership. > > 2. scoped_refptr<...> (no const, not by ref) if caller takes ownership. Right, no const and no & (not changing to raw pointer since the dependency needs scoped_refptr). Not having AudioOutputController as reference counted at all would be nice, but depends on the bug mentioned. Ownership is also shared with AudioStreamMonitor (bound in callback), but I think that can be fixed if we want to. https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... File content/browser/renderer_host/media/audio_output_delegate_unittest.cc (right): https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate_unittest.cc:34: // Not yet tested: On 2016/11/29 20:31:23, DaleCurtis wrote: > TBD? Setting up TestWebContents and all the stuff around it to test interactions with AudioStreamMonitor is possible, but we also need to send real audio data to the controller (or introduce a mock somewhere in this mess). It is still possible, but might not be worth the effort. I don't plan to test logging (unless you think it's important). Testing the returned socket/memory (at least somewhat) is done transitively in the AudioRendererHost tests. https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate_unittest.cc:49: struct MockAudioMirroringManager : public AudioMirroringManager { On 2016/11/29 21:36:27, miu wrote: > On 2016/11/29 20:31:23, DaleCurtis wrote: > > Why use struct all over here? Seems a lie :) > > This is probably my fault (circa 2012). Use of struct was just a lazy way to also say public :). I changed struct to class everywhere. https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate_unittest.cc:110: // Audio manager creation stolen from content/browser/browser_main_loop.cc. On 2016/11/29 20:31:23, DaleCurtis wrote: > Probably don't need all this. Just use one of the other fake threads in the > bundle for the audio thread and worker thread. I don't think the test needs to > worry about these details and they're likely to rot -- but A+ for effort :) :). I'd like to at least have tests when the audio thread is and isn't the UI thread, to cover the real conditions that the code runs in. How about an AudioThread class which has the construction code, which can be instantiated from both the BrowserMainLoop and tests? https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:138: delegates_.clear(); On 2016/11/29 21:36:27, miu wrote: > Get ready for some pain here. In the past, whenever I've changed the shutdown > semantics of AudioRendererHost, it usually resulted in crash issues and/or > several re-attempts to land in the tree. The complexity is in the shared > ownership of AudioOutputController, and the timing sequence of tasks being > posted across IO/UI/audio threads. So, I suggest you run browser_tests or layout > tests on an ASAN bot as a sanity-check. Yes, this is tricky (and I and Olga spent a lot of time discussing it). I think the new cleanup is nicer, so it will be worth it. https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:169: // TODO(maxmorin): Figure out how/if this is safe, since shared_memory is also On 2016/11/29 20:31:24, DaleCurtis wrote: > Can you elaborate? IIRC this completes before any usage on other threads. This function gets the shared memory from the audio thread, where the AudioSyncReader sets it up for reading. Looking at it, I suppose the AudioSyncReader finishes setting up before this function is called, so it's safe, but it would be nice if it was more obvious. :) After reading, I have convinced myself that this is actually safe, so I'll remove the todo. https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:187: base::AtomicRefCountInc(&num_playing_streams_); On 2016/11/29 20:31:24, DaleCurtis wrote: > We might be able to drop all this code now. altimin@ added stream counting to > the AudioStreamMonitor which should do everything this is doing. > https://codereview.chromium.org/2496173003/ Looks good, but it should probably be another CL? https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:378: g_audio_streams_tracker.Get().IncreaseStreamCount(); On 2016/11/29 20:31:24, DaleCurtis wrote: > Seems like code that should be moved to AudioStreamMonitor now instead. For another CL? https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:418: if (volume < 0 || volume > 1.0) On 2016/11/29 21:36:28, miu wrote: > Should this volume check be in the delegate instead? FWIW, there is valid > semantic meaning for volume to be >1.0 (gain), and so maybe it should be up to > the delegate (which would have more knowledge about what to do here). Right now I'm leaning towards "0 is minimum and 1 is maximum" and other values are invalid, meaning that they should be filtered out in the IPC layer. This value will always be passed on to AudioOutputStream which uses the interval [0, 1] for its volume (a bit inconsistent, since AudioInputStream has a GetMaxVolume, so on the input side this value is scaled). https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:431: auto i = LookupIteratorById(stream_id); On 2016/11/29 20:31:24, DaleCurtis wrote: > Could just use the delegates_.erase(std::remove_if()) idiom. This would require checking if the returned iterator is end before calling erase, since we need to know if we should DecreaseStreamCount. I think the current code is easy to understand (but it's hard to guess what remove_if does if you don't already know).
lgtm % some minor changes. https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... File content/browser/renderer_host/media/audio_output_delegate.h (right): https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.h:56: using UniquePtr = std::unique_ptr<AudioOutputDelegate, Deleter>; On 2016/12/01 at 16:08:37, Max Morin Chromium wrote: > On 2016/11/29 20:31:23, DaleCurtis wrote: > > Needs a more "unique" name, though since this is only used in a handful of > > places, it seems fine to always specify it. > > How about AudioOutputDelegate::ScopedPointer, similar to ScopedAudioManagerPointer? I think std::unique_ptr<AudioOutputDelegate, AudioOutputDelegate::Deleter> is too long. Hmm, Scoped shouldn't be used anymore for this. I was going to suggest AudioOutputDelegateUniquePtr, but I guess that's not any better than AudioOutputDelegate::UniquePtr except it's always qualified. Up to you. https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... File content/browser/renderer_host/media/audio_output_delegate_unittest.cc (right): https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate_unittest.cc:34: // Not yet tested: On 2016/12/01 at 16:08:37, Max Morin Chromium wrote: > On 2016/11/29 20:31:23, DaleCurtis wrote: > > TBD? > > Setting up TestWebContents and all the stuff around it to test interactions with AudioStreamMonitor is possible, but we also need to send real audio data to the controller (or introduce a mock somewhere in this mess). It is still possible, but might not be worth the effort. I don't plan to test logging (unless you think it's important). Testing the returned socket/memory (at least somewhat) is done transitively in the AudioRendererHost tests. I was just asking if you were planning to fix this or if it should be marked as TODO(). https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate_unittest.cc:110: // Audio manager creation stolen from content/browser/browser_main_loop.cc. On 2016/12/01 at 16:08:37, Max Morin Chromium wrote: > On 2016/11/29 20:31:23, DaleCurtis wrote: > > Probably don't need all this. Just use one of the other fake threads in the > > bundle for the audio thread and worker thread. I don't think the test needs to > > worry about these details and they're likely to rot -- but A+ for effort :) > > :). I'd like to at least have tests when the audio thread is and isn't the UI thread, to cover the real conditions that the code runs in. How about an AudioThread class which has the construction code, which can be instantiated from both the BrowserMainLoop and tests? That seems fine, but if such a class is created I think it should be the same class/function used by BrowserMainLoop to setup its threads. https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:187: base::AtomicRefCountInc(&num_playing_streams_); On 2016/12/01 at 16:08:37, Max Morin Chromium wrote: > On 2016/11/29 20:31:24, DaleCurtis wrote: > > We might be able to drop all this code now. altimin@ added stream counting to > > the AudioStreamMonitor which should do everything this is doing. > > https://codereview.chromium.org/2496173003/ > > Looks good, but it should probably be another CL? Sure, I was just pointing it out. Filed http://crbug.com/670383 https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:378: g_audio_streams_tracker.Get().IncreaseStreamCount(); On 2016/12/01 at 16:08:37, Max Morin Chromium wrote: > On 2016/11/29 20:31:24, DaleCurtis wrote: > > Seems like code that should be moved to AudioStreamMonitor now instead. > > For another CL? Covered by http://crbug.com/670383 https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:431: auto i = LookupIteratorById(stream_id); On 2016/12/01 at 16:08:37, Max Morin Chromium wrote: > On 2016/11/29 20:31:24, DaleCurtis wrote: > > Could just use the delegates_.erase(std::remove_if()) idiom. > > This would require checking if the returned iterator is end before calling erase, since we need to know if we should DecreaseStreamCount. I think the current code is easy to understand (but it's hard to guess what remove_if does if you don't already know). Not true, remove_if() returns a range, so if nothing is found the range is empty and erase() does nothing. https://codereview.chromium.org/2443573003/diff/320001/content/browser/render... File content/browser/renderer_host/media/audio_output_delegate.cc (right): https://codereview.chromium.org/2443573003/diff/320001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:39: controller_ = media::AudioOutputController::Create( Has to be very last call to avoid any risks. https://codereview.chromium.org/2443573003/diff/320001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:96: ScopedPtr delegate = ScopedPtr( Should be able to just do ScopedPtr delegate(new ...); https://codereview.chromium.org/2443573003/diff/320001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:153: if (handler_) Multiline if needs {} or just early return. https://codereview.chromium.org/2443573003/diff/320001/content/browser/render... File content/browser/renderer_host/media/audio_output_delegate.h (right): https://codereview.chromium.org/2443573003/diff/320001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.h:69: using ScopedPtr = std::unique_ptr<AudioOutputDelegate, Deleter>; Should be UniquePtr if you want to keep this style; ScopedPtr is dead :) https://codereview.chromium.org/2443573003/diff/320001/content/browser/render... File content/browser/renderer_host/media/audio_output_delegate_unittest.cc (right): https://codereview.chromium.org/2443573003/diff/320001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate_unittest.cc:35: // Not yet tested: Mark with TODO(maxmorin) if you're going to leave this comment. https://codereview.chromium.org/2443573003/diff/320001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/2443573003/diff/320001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:184: if (!is_valid) { Early return if you want. https://codereview.chromium.org/2443573003/diff/320001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:403: auto i = std::remove_if(delegates_.begin(), delegates_.end(), This should only be paired with erase(), otherwise just use find(). https://codereview.chromium.org/2443573003/diff/320001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:443: // thread that |num_plaing_streams| may be updated on. num_playing_streams_
https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... File content/browser/renderer_host/media/audio_output_delegate_unittest.cc (right): https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate_unittest.cc:110: // Audio manager creation stolen from content/browser/browser_main_loop.cc. On 2016/12/01 19:33:06, DaleCurtis wrote: > On 2016/12/01 at 16:08:37, Max Morin Chromium wrote: > > On 2016/11/29 20:31:23, DaleCurtis wrote: > > > Probably don't need all this. Just use one of the other fake threads in the > > > bundle for the audio thread and worker thread. I don't think the test needs > to > > > worry about these details and they're likely to rot -- but A+ for effort :) > > > > :). I'd like to at least have tests when the audio thread is and isn't the UI > thread, to cover the real conditions that the code runs in. How about an > AudioThread class which has the construction code, which can be instantiated > from both the BrowserMainLoop and tests? > > That seems fine, but if such a class is created I think it should be the same > class/function used by BrowserMainLoop to setup its threads. Yes, I absolutely agree! I created content/browser/audio_device_thread.{cc,h}. https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:431: auto i = LookupIteratorById(stream_id); On 2016/12/01 19:33:06, DaleCurtis wrote: > On 2016/12/01 at 16:08:37, Max Morin Chromium wrote: > > On 2016/11/29 20:31:24, DaleCurtis wrote: > > > Could just use the delegates_.erase(std::remove_if()) idiom. > > > > This would require checking if the returned iterator is end before calling > erase, since we need to know if we should DecreaseStreamCount. I think the > current code is easy to understand (but it's hard to guess what remove_if does > if you don't already know). > > Not true, remove_if() returns a range, so if nothing is found the range is empty > and erase() does nothing. Right, but the code must still check if it should do g_audio_streams_tracker.Get().DecreaseStreamCount(), which depends on if the range is empty or not. https://codereview.chromium.org/2443573003/diff/320001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/2443573003/diff/320001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:403: auto i = std::remove_if(delegates_.begin(), delegates_.end(), On 2016/12/01 19:33:07, DaleCurtis wrote: > This should only be paired with erase(), otherwise just use find(). I was experimenting with different ways to write this and forgot to rollback completely. :)
Great job! lgtm https://codereview.chromium.org/2443573003/diff/360001/content/browser/browse... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/2443573003/diff/360001/content/browser/browse... content/browser/browser_main_loop.cc:1614: audio_manager_ = media::AudioManager::Create( nice! https://codereview.chromium.org/2443573003/diff/360001/content/browser/render... File content/browser/renderer_host/media/audio_output_delegate.cc (right): https://codereview.chromium.org/2443573003/diff/360001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:62: // The mirroring manager is a singleton, so Unretained is fine. Comment on when it is deleted? (singleton by itself is not a guarantee)
maxmorin@chromium.org changed reviewers: + avi@chromium.org
Thanks! Avi: PTAL at contents/ except contents/browser/renderer_host/media/. https://codereview.chromium.org/2443573003/diff/360001/content/browser/render... File content/browser/renderer_host/media/audio_output_delegate.cc (right): https://codereview.chromium.org/2443573003/diff/360001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.cc:62: // The mirroring manager is a singleton, so Unretained is fine. On 2016/12/02 13:01:46, o1ka wrote: > Comment on when it is deleted? (singleton by itself is not a guarantee) Done.
Description was changed from ========== Factor out AudioOutputDelegate from AudioRendererHost. This change moves logic related to IPC requests regarding a single stream into its own class. This prepares for adding a mojo alternative to ARH. The mojo implementation will have a separate interface for an audio stream, which makes it very reasonable to have a class responsible for handling requests to a stream. This change also slims down ARH, so that it's easier to replace. Reference for mojofication work: https://docs.google.com/document/d/1PjV_EutyJANmsEjhH9mTNZWSslTPvAit7Cy2RO3cb..., in particular the diagram https://docs.google.com/drawings/d/1xg6z2FwLdTe5eG-Nvek9q-IfWzMI01XxlbVH3y0mz.... BUG=656923 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Factor out AudioOutputDelegate from AudioRendererHost. This change moves logic related to IPC requests regarding a single stream into its own class. This prepares for adding a mojo alternative to ARH. The mojo implementation will have a separate interface for an audio stream, which makes it very reasonable to have a class responsible for handling requests to a stream. This change also slims down ARH, so that it's easier to replace. Reference for mojofication work: https://docs.google.com/document/d/1PjV_EutyJANmsEjhH9mTNZWSslTPvAit7Cy2RO3cb..., in particular the diagram https://docs.google.com/drawings/d/1xg6z2FwLdTe5eG-Nvek9q-IfWzMI01XxlbVH3y0mz.... BUG=656923 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel,linux_chromium_asan_rel_ng,linux_chromium_msan_rel_ng,linux_chromium_chromeos_asan_rel_ng,linux_chromium_chromeos_msan_rel_ng,linux_chromium_tsan_rel_ng,linux_chromium_ubsan_rel_ng;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel,mac_chromium_asan_rel_ng;master.tryserver.chromium.win:win_optional_gpu_tests_rel,win_chromium_syzyasan_rel;master.tryserver.webrtc:linux_asan,linux_msan,linux_tsan2,linux_ubsan_vptr,mac_asan,win_asan ==========
The CQ bit was checked by maxmorin@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...
There were warnings when CQ was processing your CL: * CQ is not running the linux_chromium_asan_rel_ng trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_asan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/20085) linux_msan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_msan/builds/15578) linux_tsan2 on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/17648) linux_ubsan_vptr on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...)
LGTM https://codereview.chromium.org/2443573003/diff/380001/content/browser/audio_... File content/browser/audio_device_thread.h (right): https://codereview.chromium.org/2443573003/diff/380001/content/browser/audio_... content/browser/audio_device_thread.h:4: #ifndef CONTENT_BROWSER_AUDIO_DEVICE_THREAD_H_ blank line above
Description was changed from ========== Factor out AudioOutputDelegate from AudioRendererHost. This change moves logic related to IPC requests regarding a single stream into its own class. This prepares for adding a mojo alternative to ARH. The mojo implementation will have a separate interface for an audio stream, which makes it very reasonable to have a class responsible for handling requests to a stream. This change also slims down ARH, so that it's easier to replace. Reference for mojofication work: https://docs.google.com/document/d/1PjV_EutyJANmsEjhH9mTNZWSslTPvAit7Cy2RO3cb..., in particular the diagram https://docs.google.com/drawings/d/1xg6z2FwLdTe5eG-Nvek9q-IfWzMI01XxlbVH3y0mz.... BUG=656923 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel,linux_chromium_asan_rel_ng,linux_chromium_msan_rel_ng,linux_chromium_chromeos_asan_rel_ng,linux_chromium_chromeos_msan_rel_ng,linux_chromium_tsan_rel_ng,linux_chromium_ubsan_rel_ng;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel,mac_chromium_asan_rel_ng;master.tryserver.chromium.win:win_optional_gpu_tests_rel,win_chromium_syzyasan_rel;master.tryserver.webrtc:linux_asan,linux_msan,linux_tsan2,linux_ubsan_vptr,mac_asan,win_asan ========== to ========== Factor out AudioOutputDelegate from AudioRendererHost. This change moves logic related to IPC requests regarding a single stream into its own class. This prepares for adding a mojo alternative to ARH. The mojo implementation will have a separate interface for an audio stream, which makes it very reasonable to have a class responsible for handling requests to a stream. This change also slims down ARH, so that it's easier to replace. Reference for mojofication work: https://docs.google.com/document/d/1PjV_EutyJANmsEjhH9mTNZWSslTPvAit7Cy2RO3cb..., in particular the diagram https://docs.google.com/drawings/d/1xg6z2FwLdTe5eG-Nvek9q-IfWzMI01XxlbVH3y0mz.... BUG=656923 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel,linux_chromium_asan_rel_ng,linux_chromium_msan_rel_ng,linux_chromium_chromeos_asan_rel_ng,linux_chromium_chromeos_msan_rel_ng,linux_chromium_tsan_rel_ng,linux_chromium_ubsan_rel_ng;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel,mac_chromium_asan_rel_ng;master.tryserver.chromium.win:win_optional_gpu_tests_rel,win_chromium_syzyasan_rel ==========
Still lgtm, just some minor comments in addition to avi's. https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/2443573003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:431: auto i = LookupIteratorById(stream_id); On 2016/12/02 at 12:07:37, Max Morin Chromium wrote: > On 2016/12/01 19:33:06, DaleCurtis wrote: > > On 2016/12/01 at 16:08:37, Max Morin Chromium wrote: > > > On 2016/11/29 20:31:24, DaleCurtis wrote: > > > > Could just use the delegates_.erase(std::remove_if()) idiom. > > > > > > This would require checking if the returned iterator is end before calling > > erase, since we need to know if we should DecreaseStreamCount. I think the > > current code is easy to understand (but it's hard to guess what remove_if does > > if you don't already know). > > > > Not true, remove_if() returns a range, so if nothing is found the range is empty > > and erase() does nothing. > > Right, but the code must still check if it should do g_audio_streams_tracker.Get().DecreaseStreamCount(), which depends on if the range is empty or not. Whoops, sorry misread the question. https://codereview.chromium.org/2443573003/diff/420001/content/browser/audio_... File content/browser/audio_device_thread.h (right): https://codereview.chromium.org/2443573003/diff/420001/content/browser/audio_... content/browser/audio_device_thread.h:7: #include "base/threading/thread.h" #include build/build_config.h for OS_### https://codereview.chromium.org/2443573003/diff/420001/content/browser/audio_... content/browser/audio_device_thread.h:21: I thought complex classes were required to have out-of-line destructor, but if it passes CQ seems okay. https://codereview.chromium.org/2443573003/diff/420001/content/browser/audio_... content/browser/audio_device_thread.h:26: return base::ThreadTaskRunnerHandle::Get(); Probably should be stored instead if we're using hacker_style(). https://codereview.chromium.org/2443573003/diff/420001/content/browser/browse... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/2443573003/diff/420001/content/browser/browse... content/browser/browser_main_loop.cc:1615: audio_thread_->task_runner(), audio_thread_->worker_task_runner(), Do these need std::move() or does that happen automatically in this case? I'm still unused to this new pass-by-value world :) https://codereview.chromium.org/2443573003/diff/420001/content/browser/render... File content/browser/renderer_host/media/audio_output_authorization_handler_unittest.cc (right): https://codereview.chromium.org/2443573003/diff/420001/content/browser/render... content/browser/renderer_host/media/audio_output_authorization_handler_unittest.cc:76: audio_thread_->task_runner(), audio_thread_->worker_task_runner(), Ditto? https://codereview.chromium.org/2443573003/diff/420001/content/browser/render... File content/browser/renderer_host/media/audio_output_delegate.h (right): https://codereview.chromium.org/2443573003/diff/420001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.h:124: base::WeakPtr<AudioOutputDelegate> weak_this_; Should be down with the WeakPtrFactory. https://codereview.chromium.org/2443573003/diff/420001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host.h (right): https://codereview.chromium.org/2443573003/diff/420001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.h:117: using AudioOutputDelegateList = std::vector<AudioOutputDelegate::UniquePtr>; DelegateVector for clarity?
https://codereview.chromium.org/2443573003/diff/420001/content/browser/browse... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/2443573003/diff/420001/content/browser/browse... content/browser/browser_main_loop.cc:1615: audio_thread_->task_runner(), audio_thread_->worker_task_runner(), On 2016/12/02 18:20:13, DaleCurtis wrote: > Do these need std::move() or does that happen automatically in this case? I'm > still unused to this new pass-by-value world :) These are temporary variables, so they are already rvalues and don't need std::move. I'll fix your comments on monday.
PS22 lgtm, assuming other reviewer's nit and the following are addressed: https://codereview.chromium.org/2443573003/diff/420001/content/browser/render... File content/browser/renderer_host/media/audio_output_delegate.h (right): https://codereview.chromium.org/2443573003/diff/420001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.h:124: base::WeakPtr<AudioOutputDelegate> weak_this_; On 2016/12/02 18:20:13, DaleCurtis wrote: > Should be down with the WeakPtrFactory. Put it after the WeakPtrFactory, and then the ctor can initialize this in its initializer list.
The CQ bit was checked by maxmorin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from olka@chromium.org, avi@chromium.org, dalecurtis@chromium.org, miu@chromium.org Link to the patchset: https://codereview.chromium.org/2443573003/#ps440001 (title: "Final comments addressed.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
There were warnings when CQ was processing your CL: * CQ is not running the linux_chromium_asan_rel_ng trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the linux_chromium_asan_rel_ng trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the linux_chromium_asan_rel_ng trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the linux_chromium_asan_rel_ng trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the linux_chromium_asan_rel_ng trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the linux_chromium_asan_rel_ng trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the linux_chromium_asan_rel_ng trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the linux_chromium_asan_rel_ng trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the linux_chromium_asan_rel_ng trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the linux_chromium_asan_rel_ng trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
Description was changed from ========== Factor out AudioOutputDelegate from AudioRendererHost. This change moves logic related to IPC requests regarding a single stream into its own class. This prepares for adding a mojo alternative to ARH. The mojo implementation will have a separate interface for an audio stream, which makes it very reasonable to have a class responsible for handling requests to a stream. This change also slims down ARH, so that it's easier to replace. Reference for mojofication work: https://docs.google.com/document/d/1PjV_EutyJANmsEjhH9mTNZWSslTPvAit7Cy2RO3cb..., in particular the diagram https://docs.google.com/drawings/d/1xg6z2FwLdTe5eG-Nvek9q-IfWzMI01XxlbVH3y0mz.... BUG=656923 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel,linux_chromium_asan_rel_ng,linux_chromium_msan_rel_ng,linux_chromium_chromeos_asan_rel_ng,linux_chromium_chromeos_msan_rel_ng,linux_chromium_tsan_rel_ng,linux_chromium_ubsan_rel_ng;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel,mac_chromium_asan_rel_ng;master.tryserver.chromium.win:win_optional_gpu_tests_rel,win_chromium_syzyasan_rel ========== to ========== Factor out AudioOutputDelegate from AudioRendererHost. This change moves logic related to IPC requests regarding a single stream into its own class. This prepares for adding a mojo alternative to ARH. The mojo implementation will have a separate interface for an audio stream, which makes it very reasonable to have a class responsible for handling requests to a stream. This change also slims down ARH, so that it's easier to replace. Reference for mojofication work: https://docs.google.com/document/d/1PjV_EutyJANmsEjhH9mTNZWSslTPvAit7Cy2RO3cb..., in particular the diagram https://docs.google.com/drawings/d/1xg6z2FwLdTe5eG-Nvek9q-IfWzMI01XxlbVH3y0mz.... BUG=656923 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel,linux_chromium_msan_rel_ng,linux_chromium_chromeos_asan_rel_ng,linux_chromium_chromeos_msan_rel_ng,linux_chromium_tsan_rel_ng,linux_chromium_ubsan_rel_ng;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel,mac_chromium_asan_rel_ng;master.tryserver.chromium.win:win_optional_gpu_tests_rel,win_chromium_syzyasan_rel ==========
CQ is committing da patch.
Bot data: {"patchset_id": 440001, "attempt_start_ts": 1480927695222180,
"parent_rev": "7e678aa4d1984f32f94442472a4973ddc55291ec", "commit_rev":
"4b084d0822b7f4b98628c7bfe16728f33a1637da"}
Message was sent while issue was closed.
Description was changed from ========== Factor out AudioOutputDelegate from AudioRendererHost. This change moves logic related to IPC requests regarding a single stream into its own class. This prepares for adding a mojo alternative to ARH. The mojo implementation will have a separate interface for an audio stream, which makes it very reasonable to have a class responsible for handling requests to a stream. This change also slims down ARH, so that it's easier to replace. Reference for mojofication work: https://docs.google.com/document/d/1PjV_EutyJANmsEjhH9mTNZWSslTPvAit7Cy2RO3cb..., in particular the diagram https://docs.google.com/drawings/d/1xg6z2FwLdTe5eG-Nvek9q-IfWzMI01XxlbVH3y0mz.... BUG=656923 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel,linux_chromium_msan_rel_ng,linux_chromium_chromeos_asan_rel_ng,linux_chromium_chromeos_msan_rel_ng,linux_chromium_tsan_rel_ng,linux_chromium_ubsan_rel_ng;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel,mac_chromium_asan_rel_ng;master.tryserver.chromium.win:win_optional_gpu_tests_rel,win_chromium_syzyasan_rel ========== to ========== Factor out AudioOutputDelegate from AudioRendererHost. This change moves logic related to IPC requests regarding a single stream into its own class. This prepares for adding a mojo alternative to ARH. The mojo implementation will have a separate interface for an audio stream, which makes it very reasonable to have a class responsible for handling requests to a stream. This change also slims down ARH, so that it's easier to replace. Reference for mojofication work: https://docs.google.com/document/d/1PjV_EutyJANmsEjhH9mTNZWSslTPvAit7Cy2RO3cb..., in particular the diagram https://docs.google.com/drawings/d/1xg6z2FwLdTe5eG-Nvek9q-IfWzMI01XxlbVH3y0mz.... BUG=656923 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel,linux_chromium_msan_rel_ng,linux_chromium_chromeos_asan_rel_ng,linux_chromium_chromeos_msan_rel_ng,linux_chromium_tsan_rel_ng,linux_chromium_ubsan_rel_ng;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel,mac_chromium_asan_rel_ng;master.tryserver.chromium.win:win_optional_gpu_tests_rel,win_chromium_syzyasan_rel ==========
Message was sent while issue was closed.
Committed patchset #23 (id:440001)
Message was sent while issue was closed.
Description was changed from ========== Factor out AudioOutputDelegate from AudioRendererHost. This change moves logic related to IPC requests regarding a single stream into its own class. This prepares for adding a mojo alternative to ARH. The mojo implementation will have a separate interface for an audio stream, which makes it very reasonable to have a class responsible for handling requests to a stream. This change also slims down ARH, so that it's easier to replace. Reference for mojofication work: https://docs.google.com/document/d/1PjV_EutyJANmsEjhH9mTNZWSslTPvAit7Cy2RO3cb..., in particular the diagram https://docs.google.com/drawings/d/1xg6z2FwLdTe5eG-Nvek9q-IfWzMI01XxlbVH3y0mz.... BUG=656923 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel,linux_chromium_msan_rel_ng,linux_chromium_chromeos_asan_rel_ng,linux_chromium_chromeos_msan_rel_ng,linux_chromium_tsan_rel_ng,linux_chromium_ubsan_rel_ng;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel,mac_chromium_asan_rel_ng;master.tryserver.chromium.win:win_optional_gpu_tests_rel,win_chromium_syzyasan_rel ========== to ========== Factor out AudioOutputDelegate from AudioRendererHost. This change moves logic related to IPC requests regarding a single stream into its own class. This prepares for adding a mojo alternative to ARH. The mojo implementation will have a separate interface for an audio stream, which makes it very reasonable to have a class responsible for handling requests to a stream. This change also slims down ARH, so that it's easier to replace. Reference for mojofication work: https://docs.google.com/document/d/1PjV_EutyJANmsEjhH9mTNZWSslTPvAit7Cy2RO3cb..., in particular the diagram https://docs.google.com/drawings/d/1xg6z2FwLdTe5eG-Nvek9q-IfWzMI01XxlbVH3y0mz.... BUG=656923 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel,linux_chromium_msan_rel_ng,linux_chromium_chromeos_asan_rel_ng,linux_chromium_chromeos_msan_rel_ng,linux_chromium_tsan_rel_ng,linux_chromium_ubsan_rel_ng;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel,mac_chromium_asan_rel_ng;master.tryserver.chromium.win:win_optional_gpu_tests_rel,win_chromium_syzyasan_rel Committed: https://crrev.com/65f77fb7054d31082b55e7385617c819f31902ec Cr-Commit-Position: refs/heads/master@{#436278} ==========
Message was sent while issue was closed.
Patchset 23 (id:??) landed as https://crrev.com/65f77fb7054d31082b55e7385617c819f31902ec Cr-Commit-Position: refs/heads/master@{#436278}
Message was sent while issue was closed.
Thanks again everyone, let's hope there are no complications :) https://codereview.chromium.org/2443573003/diff/420001/content/browser/render... File content/browser/renderer_host/media/audio_output_delegate.h (right): https://codereview.chromium.org/2443573003/diff/420001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate.h:124: base::WeakPtr<AudioOutputDelegate> weak_this_; On 2016/12/02 22:57:58, miu wrote: > On 2016/12/02 18:20:13, DaleCurtis wrote: > > Should be down with the WeakPtrFactory. > > Put it after the WeakPtrFactory, and then the ctor can initialize this in its > initializer list. The WeakPtrFactory must be last (enforced by compiler), so I put the weak ptr just above it. All other comments have been addressed. |
