Tab Audio Mirroring: WebContentsAudioInputStream is a new implementation which represents the lifet…

content/browser/renderer_host/media/web_contents_audio_input_stream.cc

+// Copyright (c) 2013 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/web_contents_audio_input_stream.h"
+
+#include <string>
+
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/logging.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/message_loop_proxy.h"
+#include "content/browser/browser_main_loop.h"
+#include "content/browser/renderer_host/media/audio_mirroring_manager.h"
+#include "content/browser/renderer_host/media/web_contents_capture_util.h"
+#include "content/public/browser/browser_thread.h"
+#include "media/audio/virtual_audio_input_stream.h"
+#include "media/audio/virtual_audio_output_stream.h"
+
+namespace content {
+
+class WebContentsAudioInputStream::Impl
+    : public base::RefCountedThreadSafe<WebContentsAudioInputStream::Impl>,
+      public AudioMirroringManager::MirroringDestination {
+ public:
+  // Takes ownership of |mixer_stream|.  The rest outlive this instance.
+  Impl(int render_process_id, int render_view_id,
+       base::MessageLoopProxy* message_loop,
+       AudioMirroringManager* mirroring_manager,
+       const scoped_refptr<WebContentsTracker>& tracker,
+       media::VirtualAudioInputStream* mixer_stream);
+
+  // Open underlying VirtualAudioInputStream and start tracker.
+  bool Open();
+
+  // Start the underlying VirtualAudioInputStream and instruct
+  // AudioMirroringManager to begin a mirroring session.
+  void Start(AudioInputCallback* callback);
+
+  // Stop the underlying VirtualAudioInputStream and instruct
+  // AudioMirroringManager to shutdown a mirroring session.
+  void Stop();
+
+  // Close the underlying VirtualAudioInputStream and stop the tracker.
+  void Close();
+
+  // Accessor to underlying VirtualAudioInputStream.
+  media::VirtualAudioInputStream* mixer_stream() const {
+    return mixer_stream_.get();
+  }
+
+ private:
+  friend class RefCountedThreadSafe<Impl>;
+
+  enum State {
+    kConstructed,

[tommi (sloooow) - chröme, 2013/01/14 14:06:48]

CONSTRUCTED, OPENED, etc.

[miu, 2013/01/14 21:53:03]

Done.  Yeah, I keep forgetting the new style on these.

+    kOpened,
+    kMirroring,
+    kClosed
+  };
+
+  virtual ~Impl();
+
+  // Returns true if the mirroring target has been permanently lost.
+  bool IsTargetLost() const;
+
+  // Notifies the consumer callback that the stream is now dead.
+  void ReportError();
+
+  // Start/Stop mirroring by posting a call to AudioMirroringManager on the IO
+  // BrowserThread.
+  void StartMirroring();
+  void StopMirroring();
+
+  // AudioMirroringManager::MirroringDestination implementation
+  virtual media::AudioOutputStream* AddInput(
+      const media::AudioParameters& params) OVERRIDE;
+
+  // Callback which is run when |stream| is closed.  Deletes |stream|.
+  void ReleaseInput(media::VirtualAudioOutputStream* stream);
+
+  // Called by WebContentsTracker when the target of the audio mirroring has
+  // changed.
+  void OnTargetChanged(int render_process_id, int render_view_id);
+
+  // Injected dependencies.
+  base::MessageLoopProxy* const message_loop_;
+  AudioMirroringManager* const mirroring_manager_;
+  const scoped_refptr<WebContentsTracker> tracker_;
+  // The AudioInputStream implementation that handles the audio conversion and
+  // mixing details.
+  const scoped_ptr<media::VirtualAudioInputStream> mixer_stream_;
+
+  State state_;
+
+  // Current audio mirroring target.
+  int target_render_process_id_;
+  int target_render_view_id_;
+
+  // Current callback used to consume the resulting mixed audio data.
+  AudioInputCallback* callback_;
+
+  DISALLOW_COPY_AND_ASSIGN(Impl);
+};
+
+WebContentsAudioInputStream::Impl::Impl(
+    int render_process_id, int render_view_id,
+    base::MessageLoopProxy* message_loop,
+    AudioMirroringManager* mirroring_manager,
+    const scoped_refptr<WebContentsTracker>& tracker,
+    media::VirtualAudioInputStream* mixer_stream)
+    : message_loop_(message_loop), mirroring_manager_(mirroring_manager),
+      tracker_(tracker), mixer_stream_(mixer_stream), state_(kConstructed),
+      target_render_process_id_(render_process_id),
+      target_render_view_id_(render_view_id),
+      callback_(NULL) {
+  DCHECK(message_loop_);
+  DCHECK(mirroring_manager_);
+  DCHECK(tracker_);
+  DCHECK(mixer_stream_.get());
+}
+
+WebContentsAudioInputStream::Impl::~Impl() {
+  DCHECK(state_ == kConstructed || state_ == kClosed);
+}
+
+bool WebContentsAudioInputStream::Impl::Open() {
+  DCHECK(message_loop_->BelongsToCurrentThread());
+
+  if (state_ != kConstructed)

[tommi (sloooow) - chröme, 2013/01/14 14:06:48]

should we, in order to keep the implementation simple and the contract strict,
replace these "calling this method at an incorrect point in time won't work"
checks with DCHECKs instead of returning false here and assume that the caller
will handle the error?

I don't have a clear enough picture in my head about how this will all hang
together, but if it is a programmer error to call Open() when the state is not
kConstructed, then I would prefer to handle that with a DCHECK() or even a
CHECK() instead of an if()+return which can go unnoticed.

If however if it is a legitimate part of the contract that Open can be called at
any time and the caller should know what to do if it fails in cases like this
(i.e. handle mixer_stream_->Open() failures in the same way as state_ !=
kConstructed), then this is fine.  It just feels like the first check (state_ !=
kConstructed) would be the result of the programmer having done something wrong
and the second more likely to be related to some runtime error.

[miu, 2013/01/14 21:53:03]

I totally agree with you on principle.  I'm mainly worried about out-of-ordering
issues across threads (IO vs audio thread), and across render-->browser process.
 I took a look at the linux, mac, and win AudioInputStream implementations, and
all do similar "if state == X" guards in their Open, Start, Stop, and Close
methods.  So, there might be something that requires the guards be in-place for
stability?

For safety, I'm going to keep the if-guards in Start, Stop, and Close; but will
put a DCHECK in Open() and at the end of Close().  It's pretty clear that
Open()/Close() must only be called once, and must be called before/after any of
the others.

BTW--Another good reason to keep the if-guards: On the output side of things,
I've noticed AudioOutputController actually calls stream_->Stop() multiple
times.  I have a change I'm working on to fix this, and in the process of doing
so, I've already unveiled a bug in PPAPI!  So, it seems the code base isn't in
the pristine state we need it to be in to replace with DCHECKs everywhere.  ;-)

[tommi (sloooow) - chröme, 2013/01/15 10:58:42]

Great. Thanks for adding the checks and thanks for digging into the call
sequences.  Now that you mention it I think I've seen that multiple Stop() thing
too.

+    return false;
+
+  if (!mixer_stream_->Open())
+    return false;
+
+  state_ = kOpened;
+
+  tracker_->Start(
+      target_render_process_id_, target_render_view_id_,
+      base::Bind(&Impl::OnTargetChanged, this));
+
+  return true;
+}
+
+void WebContentsAudioInputStream::Impl::Start(AudioInputCallback* callback) {
+  DCHECK(message_loop_->BelongsToCurrentThread());
+  DCHECK(callback);
+
+  if (state_ != kOpened)

[tommi (sloooow) - chröme, 2013/01/14 14:06:48]

same question here

+    return;
+
+  if (IsTargetLost()) {
+    ReportError();
+    return;
+  }
+
+  state_ = kMirroring;
+
+  callback_ = callback;
+  mixer_stream_->Start(callback);
+
+  StartMirroring();
+}
+
+void WebContentsAudioInputStream::Impl::Stop() {
+  DCHECK(message_loop_->BelongsToCurrentThread());
+
+  if (state_ != kMirroring)
+    return;
+
+  state_ = kOpened;
+
+  mixer_stream_->Stop();
+  callback_ = NULL;
+
+  if (!IsTargetLost())
+    StopMirroring();
+}
+
+void WebContentsAudioInputStream::Impl::Close() {
+  DCHECK(message_loop_->BelongsToCurrentThread());
+
+  Stop();
+
+  if (state_ == kOpened) {
+    state_ = kConstructed;
+    tracker_->Stop();
+    mixer_stream_->Close();
+  }
+
+  if (state_ == kConstructed)
+    state_ = kClosed;
+}
+
+bool WebContentsAudioInputStream::Impl::IsTargetLost() const {
+  DCHECK(message_loop_->BelongsToCurrentThread());
+
+  return target_render_process_id_ <= 0 || target_render_view_id_ <= 0;
+}
+
+void WebContentsAudioInputStream::Impl::ReportError() {
+  DCHECK(message_loop_->BelongsToCurrentThread());
+
+  // TODO(miu): Need clean-up of AudioInputCallback interface in a future
+  // change, since its only implementation ignores the first argument entirely
+  // and the values for the second argument are undefined.
+  callback_->OnError(NULL, 0);
+}
+
+void WebContentsAudioInputStream::Impl::StartMirroring() {
+  DCHECK(message_loop_->BelongsToCurrentThread());
+
+  BrowserThread::PostTask(
+      BrowserThread::IO,
+      FROM_HERE,
+      base::Bind(&AudioMirroringManager::StartMirroring,
+                 base::Unretained(mirroring_manager_),
+                 target_render_process_id_, target_render_view_id_,
+                 make_scoped_refptr(this)));
+}
+
+void WebContentsAudioInputStream::Impl::StopMirroring() {
+  DCHECK(message_loop_->BelongsToCurrentThread());
+
+  BrowserThread::PostTask(
+      BrowserThread::IO,
+      FROM_HERE,
+      base::Bind(&AudioMirroringManager::StopMirroring,
+                 base::Unretained(mirroring_manager_),
+                 target_render_process_id_, target_render_view_id_,
+                 make_scoped_refptr(this)));
+}
+
+media::AudioOutputStream* WebContentsAudioInputStream::Impl::AddInput(
+    const media::AudioParameters& params) {
+  media::VirtualAudioOutputStream* const stream =
+      new media::VirtualAudioOutputStream(
+          params, message_loop_, mixer_stream_.get());
+  // Note: The closure created here holds a reference to "this," which will
+  // guarantee the VirtualAudioInputStream (mixer_stream_) outlives the
+  // VirtualAudioOutputStream.
+  stream->RunOnceClosed(base::Bind(&Impl::ReleaseInput, this, stream));
+
+  return stream;
+}
+
+void WebContentsAudioInputStream::Impl::ReleaseInput(
+    media::VirtualAudioOutputStream* stream) {
+  delete stream;
+}
+
+void WebContentsAudioInputStream::Impl::OnTargetChanged(int render_process_id,
+                                                        int render_view_id) {
+  DCHECK(message_loop_->BelongsToCurrentThread());
+
+  if (target_render_process_id_ == render_process_id &&
+      target_render_view_id_ == render_view_id)
+    return;  // No change.

[tommi (sloooow) - chröme, 2013/01/14 14:06:48]

{}

[miu, 2013/01/14 21:53:03]

I thought we don't put braces around one-line then-clauses?

[tommi (sloooow) - chröme, 2013/01/15 10:58:42]

Yeah, so this one I've learned through reviews and on the chromium-dev list (see
e.g. somewhat recent thread about lint).  The thing is that a one-line if()
means 1 line for the condition and 1 line for the body.  I.e. 2 lines total.  As
soon as either the body or condition needs to be wrapped, the convention is to
use {}.  I believe this is an old google style convention.

[miu, 2013/01/16 03:22:18]

Done.  Okay, I understand it now.  I really wish the style decision had been to
always use braces.  :-/

+
+  DVLOG(1) << "Target RenderView has changed from "
+           << target_render_process_id_ << ':' << target_render_view_id_
+           << " to " << render_process_id << ':' << render_view_id;
+
+  if (state_ == kMirroring)
+    StopMirroring();
+
+  target_render_process_id_ = render_process_id;
+  target_render_view_id_ = render_view_id;
+
+  if (state_ == kMirroring) {
+    if (IsTargetLost()) {
+      ReportError();
+      Stop();
+    } else {
+      StartMirroring();
+    }
+  }
+}
+
+//static

[tommi (sloooow) - chröme, 2013/01/14 14:06:48]

// static

[miu, 2013/01/14 21:53:03]

Done.

+WebContentsAudioInputStream* WebContentsAudioInputStream::Create(
+    const std::string& device_id,
+    const media::AudioParameters& params,
+    base::MessageLoopProxy* message_loop) {
+  int render_process_id;
+  int render_view_id;
+  if (!WebContentsCaptureUtil::ExtractTabCaptureTarget(
+          device_id, &render_process_id, &render_view_id)) {
+    return NULL;
+  } else {

[tommi (sloooow) - chröme, 2013/01/14 14:06:48]

no need for else?

[miu, 2013/01/14 21:53:03]

Done.

+    return new WebContentsAudioInputStream(
+        render_process_id, render_view_id, message_loop,
+        BrowserMainLoop::GetAudioMirroringManager(),
+        new WebContentsTracker(),
+        new media::VirtualAudioInputStream(params, message_loop));
+  }
+}
+
+WebContentsAudioInputStream::WebContentsAudioInputStream(
+    int render_process_id, int render_view_id,
+    base::MessageLoopProxy* message_loop,
+    AudioMirroringManager* mirroring_manager,
+    const scoped_refptr<WebContentsTracker>& tracker,
+    media::VirtualAudioInputStream* mixer_stream)
+    : impl_(new Impl(render_process_id, render_view_id, message_loop,
+                     mirroring_manager, tracker, mixer_stream)) {}
+
+WebContentsAudioInputStream::~WebContentsAudioInputStream() {}
+
+bool WebContentsAudioInputStream::Open() {
+  return impl_->Open();
+}
+
+void WebContentsAudioInputStream::Start(AudioInputCallback* callback) {
+  impl_->Start(callback);
+}
+
+void WebContentsAudioInputStream::Stop() {
+  impl_->Stop();
+}
+
+void WebContentsAudioInputStream::Close() {
+  impl_->Close();
+  delete this;
+}
+
+double WebContentsAudioInputStream::GetMaxVolume() {
+  return impl_->mixer_stream()->GetMaxVolume();
+}
+
+void WebContentsAudioInputStream::SetVolume(double volume) {
+  impl_->mixer_stream()->SetVolume(volume);
+}
+
+double WebContentsAudioInputStream::GetVolume() {
+  return impl_->mixer_stream()->GetVolume();
+}
+
+void WebContentsAudioInputStream::SetAutomaticGainControl(bool enabled) {
+  impl_->mixer_stream()->SetAutomaticGainControl(enabled);
+}
+
+bool WebContentsAudioInputStream::GetAutomaticGainControl() {
+  return impl_->mixer_stream()->GetAutomaticGainControl();
+}
+
+}  // namespace content