Factor out AudioOutputDelegate from AudioRendererHost.

content/browser/renderer_host/media/audio_output_delegate.cc

Index: content/browser/renderer_host/media/audio_output_delegate.cc
diff --git a/content/browser/renderer_host/media/audio_output_delegate.cc b/content/browser/renderer_host/media/audio_output_delegate.cc
new file mode 100644
index 0000000000000000000000000000000000000000..da498cb90bdf2d0e5113dad7555d21c59463bb71
--- /dev/null
+++ b/content/browser/renderer_host/media/audio_output_delegate.cc
@@ -0,0 +1,175 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "content/browser/renderer_host/media/audio_output_delegate.h"
+
+#include <utility>
+
+#include "base/bind.h"
+#include "content/browser/media/audio_stream_monitor.h"
+#include "content/browser/media/media_internals.h"
+#include "content/public/browser/browser_thread.h"
+
+namespace content {
+
+AudioOutputDelegate::AudioOutputDelegate(
+    EventHandler* handler,
+    std::unique_ptr<media::AudioLog> audio_log,
+    AudioMirroringManager* mirroring_manager,
+    MediaObserver* media_observer,
+    int stream_id,
+    int render_frame_id,
+    int render_process_id,
+    media::AudioParameters params,
+    const std::string& output_device_id)
+    : handler_(handler),
+      audio_log_(std::move(audio_log)),
+      mirroring_manager_(mirroring_manager),
+      reader_(AudioSyncReader::Create(params)),
+      controller_(
+          media::AudioOutputController::Create(media::AudioManager::Get(),

[o1ka, 2016/10/27 09:52:38]

We need to figure out and state here why it's guaranteed that
AudioOutputDelegate/AudioController  do not outlive AudioManager.

[Max Morin, 2016/10/27 15:04:38]

I'll just pass the AudioManager* in to the constructor.

+                                               this,
+                                               params,
+                                               output_device_id,
+                                               reader_.get())),
+      stream_id_(stream_id),
+      render_frame_id_(render_frame_id),
+      render_process_id_(render_process_id),
+      weak_factory_(this) {
+  DCHECK(controller_);
+  if (media_observer)
+    media_observer->OnCreatingAudioStream(render_process_id_, render_frame_id_);

[o1ka, 2016/10/27 09:52:38]

We can do it in Create() now.

[Max Morin, 2016/10/28 14:55:47]

Why is that preferable?

+  if (mirroring_manager_)
+    mirroring_manager_->AddDiverter(render_process_id_, render_frame_id_,
+                                    controller_.get());
+  audio_log_->OnCreated(stream_id, params, output_device_id);
+  MediaInternals::GetInstance()->SetWebContentsTitleForAudioLogEntry(

[o1ka, 2016/10/27 12:59:52]

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....

[Max Morin, 2016/10/28 14:55:47]

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.

+      stream_id_, render_process_id_, render_frame_id_, audio_log_.get());
+}
+
+AudioOutputDelegate::~AudioOutputDelegate() {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  AudioStreamMonitor::StopMonitoringStream(render_process_id_, render_frame_id_,
+                                           stream_id_);
+
+  mirroring_manager_->RemoveDiverter(controller_.get());
+  DCHECK(!playing_);

[o1ka, 2016/10/27 09:52:38]

nit: move DCHECK to the top? (It's a prerequisite)

[Max Morin, 2016/10/27 15:04:38]

Done.

+}
+
+void AudioOutputDelegate::Deleter::operator()(AudioOutputDelegate* delegate) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  delegate->UpdatePlayingStateForHandler(false);

[o1ka, 2016/10/28 09:13:22]

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).

[Max Morin, 2016/10/28 14:55:47]

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.

[o1ka, 2016/11/03 13:39:11]

Have you done that?

+  delegate->handler_ = nullptr;
+  delegate->audio_log_->OnClosed(delegate->stream_id_);
+  // Keep delegate alive until controller is closed:

[o1ka, 2016/10/28 09:13:22]

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 ;) )

[Max Morin, 2016/10/28 14:55:47]

Done.

+  delegate->controller_->Close(
+      base::Bind([](AudioOutputDelegate*) {}, base::Owned(delegate)));

[o1ka, 2016/10/27 09:52:38]

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.

[o1ka, 2016/10/27 12:59:52]

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.

[Max Morin, 2016/10/27 15:04:38]

Done.

+}
+// static
+AudioOutputDelegate::UniquePtr AudioOutputDelegate::Create(
+    EventHandler* handler,
+    std::unique_ptr<media::AudioLog> audio_log,
+    AudioMirroringManager* mirroring_manager,
+    MediaObserver* media_observer,
+    int stream_id,
+    int render_frame_id,
+    int render_process_id,
+    media::AudioParameters params,
+    const std::string& output_device_id) {
+  return UniquePtr(new AudioOutputDelegate(
+      handler, std::move(audio_log), mirroring_manager, media_observer,
+      stream_id, render_frame_id, render_process_id, params, output_device_id));
+}
+
+void AudioOutputDelegate::OnPlayStream() {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  controller_->Play();
+  audio_log_->OnStarted(stream_id_);
+}
+
+void AudioOutputDelegate::OnPauseStream() {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  controller_->Pause();
+  audio_log_->OnStopped(stream_id_);
+}
+
+void AudioOutputDelegate::OnSetVolume(double volume) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  controller_->SetVolume(volume);
+  audio_log_->OnSetVolume(stream_id_, volume);
+}
+
+void AudioOutputDelegate::OnControllerCreated() {
+  if (!BrowserThread::CurrentlyOn(BrowserThread::IO)) {
+    BrowserThread::PostTask(
+        BrowserThread::IO, FROM_HERE,
+        base::Bind(&AudioOutputDelegate::OnControllerCreated,
+                   weak_factory_.GetWeakPtr()));
+    return;
+  }
+  if (!handler_)
+    return;
+
+  handler_->OnStreamCreated(stream_id_, reader_->shared_memory(),
+                            reader_->foreign_socket());
+}
+
+void AudioOutputDelegate::OnControllerPlaying() {
+  if (!BrowserThread::CurrentlyOn(BrowserThread::IO)) {
+    BrowserThread::PostTask(
+        BrowserThread::IO, FROM_HERE,
+        base::Bind(&AudioOutputDelegate::OnControllerPlaying,
+                   weak_factory_.GetWeakPtr()));
+    return;
+  }
+  if (!handler_)
+    return;
+
+  UpdatePlayingStateForHandler(true);
+
+  AudioStreamMonitor::StartMonitoringStream(
+      render_process_id_, render_frame_id_, stream_id_,
+      base::Bind(&media::AudioOutputController::ReadCurrentPowerAndClip,
+                 controller_));
+}
+
+void AudioOutputDelegate::OnControllerPaused() {
+  if (!BrowserThread::CurrentlyOn(BrowserThread::IO)) {
+    BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
+                            base::Bind(&AudioOutputDelegate::OnControllerPaused,
+                                       weak_factory_.GetWeakPtr()));
+    return;
+  }
+  if (!handler_)
+    return;
+
+  UpdatePlayingStateForHandler(false);
+
+  AudioStreamMonitor::StopMonitoringStream(render_process_id_, render_frame_id_,

[o1ka, 2016/10/27 09:52:38]

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.

[Max Morin, 2016/10/27 15:04:38]

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.

+                                           stream_id_);
+}
+
+void AudioOutputDelegate::OnControllerError() {
+  if (!BrowserThread::CurrentlyOn(BrowserThread::IO)) {
+    BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
+                            base::Bind(&AudioOutputDelegate::OnControllerError,
+                                       weak_factory_.GetWeakPtr()));
+    return;
+  }
+  if (!handler_)
+    return;
+
+  audio_log_->OnError(stream_id_);

[o1ka, 2016/10/27 09:52:38]

Do we probably want to log an error even if |handler_| is reset?

[Max Morin, 2016/10/27 15:04:38]

Ok.

+  handler_->OnStreamError(stream_id_);
+}
+
+void AudioOutputDelegate::UpdatePlayingStateForHandler(bool playing) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  if (playing != playing_) {
+    playing_ = playing;

[o1ka, 2016/10/27 09:52:38]

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.

[Max Morin, 2016/10/27 15:04:38]

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.

[o1ka, 2016/10/28 09:13:22]

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.

[Max Morin, 2016/10/28 14:55:47]

Done.

+    handler_->OnStreamStateChanged(playing_);
+  }
+}
+
+}  // namespace content