Factor out AudioOutputDelegate from AudioRendererHost.

content/browser/renderer_host/media/audio_output_delegate.cc

+// 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,
+    media::AudioManager* audio_manager,
+    std::unique_ptr<media::AudioLog> audio_log,
+    int stream_id,
+    int render_frame_id,
+    int render_process_id,
+    const media::AudioParameters& params,
+    const std::string& output_device_id)
+    : handler_(handler),
+      audio_log_(std::move(audio_log)),
+      reader_(AudioSyncReader::Create(params)),
+      controller_(media::AudioOutputController::Create(audio_manager,

[DaleCurtis, 2016/11/29 20:31:22]

This should probably be constructed last since it's handing |this| off to a
thread could theoretically issue callbacks before construction finishes.

+                                                       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(handler_);
+  DCHECK(audio_manager);
+  DCHECK(audio_log_);
+  DCHECK(controller_);
+  audio_log_->OnCreated(stream_id, params, output_device_id);
+}
+
+AudioOutputDelegate::~AudioOutputDelegate() {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  DCHECK(!playing_);
+  DCHECK(!handler_);
+}
+
+void AudioOutputDelegate::Deleter::operator()(AudioOutputDelegate* delegate) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  delegate->UpdatePlayingState(false);
+  delegate->handler_ = nullptr;
+  delegate->audio_log_->OnClosed(delegate->stream_id_);
+

[DaleCurtis, 2016/11/29 20:31:22]

Early return if !mirroring_manager (don't forget to delete)? I forget if that
happens only in tests.

[miu, 2016/11/29 21:36:27]

Should only be. IIRC, the only reason the mirroring manager is injected via the
ctor is so tests can provide a mock.

[Max Morin, 2016/12/01 16:08:37]

We must wait until the controller is closed before deleting the delegate
anyways, so there is no point.

+  scoped_refptr<media::AudioOutputController> controller =

[DaleCurtis, 2016/11/29 20:31:22]

.swap?

+      delegate->controller_;
+
+  // |controller| will call the closure (on IO thread) when it's done closing,
+  // and it is only after that call that we can delete |delegate|. By giving the
+  // closure ownership of |delegate|, we keep delegate alive until |controller|
+  // is closed.
+  //
+  // The mirroring manager is a singleton, so Unretained is fine.
+  delegate->controller_->Close(base::Bind(
+      [](AudioOutputDelegate*, AudioMirroringManager* mirroring_manager,
+         scoped_refptr<media::AudioOutputController> controller) {
+        // De-register the controller from the AudioMirroringManager now that
+        // the controller has closed the AudioOutputStream and shut itself down.
+        // This ensures that calling RemoveDiverter() here won't trigger the
+        // controller to re-start the default AudioOutputStream and cause a
+        // brief audio blip to come out the user's speakers.
+        // http://crbug.com/474432
+        //
+        // It's fine if the task is canceled during shutdown, since the
+        // mirroring manager doesn't require that all diverters are
+        // removed.
+        if (mirroring_manager)
+          mirroring_manager->RemoveDiverter(controller.get());
+      },
+      base::Owned(delegate), base::Unretained(mirroring_manager_), controller));

[DaleCurtis, 2016/11/29 20:31:23]

std::move(controller)?

[Max Morin, 2016/12/01 16:08:37]

I simplified this code, PTAL.

+}
+
+// static
+AudioOutputDelegate::UniquePtr AudioOutputDelegate::Create(
+    EventHandler* handler,
+    media::AudioManager* audio_manager,
+    std::unique_ptr<media::AudioLog> audio_log,
+    AudioMirroringManager* mirroring_manager,
+    MediaObserver* media_observer,
+    int stream_id,
+    int render_frame_id,
+    int render_process_id,
+    const media::AudioParameters& params,
+    const std::string& output_device_id) {
+  if (media_observer)
+    media_observer->OnCreatingAudioStream(render_process_id, render_frame_id);
+  UniquePtr delegate = UniquePtr(
+      new AudioOutputDelegate(handler, audio_manager, std::move(audio_log),
+                              stream_id, render_frame_id, render_process_id,
+                              params, output_device_id),
+      Deleter(mirroring_manager));
+  if (mirroring_manager)
+    mirroring_manager->AddDiverter(render_process_id, render_frame_id,
+                                   delegate->controller_.get());
+  return delegate;
+}
+
+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);
+  DCHECK(volume >= 0 && volume <= 1);

[DaleCurtis, 2016/11/29 20:31:23]

Separate into DCHECK_GE() and DCHECK_LE() for easier debugging.

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

[DaleCurtis, 2016/11/29 20:31:22]

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.

+    return;
+  }
+  if (!handler_)
+    return;
+
+  handler_->OnStreamCreated(stream_id_, reader_->shared_memory(),
+                            reader_->foreign_socket());
+}
+
+void AudioOutputDelegate::OnControllerPlaying() {
+  if (!BrowserThread::CurrentlyOn(BrowserThread::IO)) {

[DaleCurtis, 2016/11/29 20:31:23]

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;

[miu, 2016/11/29 21:36:27]

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.

[Max Morin, 2016/12/01 16:08:37]

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.

+    BrowserThread::PostTask(
+        BrowserThread::IO, FROM_HERE,
+        base::Bind(&AudioOutputDelegate::OnControllerPlaying,
+                   weak_factory_.GetWeakPtr()));

[miu, 2016/11/29 21:36:27]

GetWeakPtr() is not thread-safe in this situation since GetWeakPtr() could be
called while the "owning thread" is invalidating the weak pointers.

+    return;
+  }
+  if (!handler_)
+    return;
+
+  UpdatePlayingState(true);
+}
+
+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;
+
+  UpdatePlayingState(false);
+}
+
+void AudioOutputDelegate::OnControllerError() {
+  if (!BrowserThread::CurrentlyOn(BrowserThread::IO)) {
+    BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
+                            base::Bind(&AudioOutputDelegate::OnControllerError,
+                                       weak_factory_.GetWeakPtr()));
+    return;
+  }
+  audio_log_->OnError(stream_id_);

[DaleCurtis, 2016/11/29 20:31:23]

Why the exception for handler_ here?

[DaleCurtis, 2016/11/29 20:31:23]

Why the exception for handler_ here?

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

[DaleCurtis, 2016/11/29 20:31:22]

If playing == playing_ return.

+    playing_ = playing;
+    handler_->OnStreamStateChanged(playing_);
+    if (playing) {
+      // Note that this takes a reference to |controller_|, and
+      // (Start|Stop)MonitoringStream calls are async, so we don't have a
+      // guarantee for when the controller is destroyed.
+      AudioStreamMonitor::StartMonitoringStream(
+          render_process_id_, render_frame_id_, stream_id_,
+          base::Bind(&media::AudioOutputController::ReadCurrentPowerAndClip,
+                     controller_));
+    } else {
+      AudioStreamMonitor::StopMonitoringStream(render_process_id_,
+                                               render_frame_id_, stream_id_);
+    }
+  }
+}
+
+}  // namespace content