MediaStream audio: Refactor 3 separate "glue" implementations into one.

content/renderer/media/media_stream_audio_source.cc

Index: content/renderer/media/media_stream_audio_source.cc
diff --git a/content/renderer/media/media_stream_audio_source.cc b/content/renderer/media/media_stream_audio_source.cc
index d2027fd939e797db1171625fcf469f8466c2fd36..b4b559f4c8aaf6f8d6dbb5982a3b1904313e8f5e 100644
--- a/content/renderer/media/media_stream_audio_source.cc
+++ b/content/renderer/media/media_stream_audio_source.cc
@@ -4,29 +4,28 @@
 
 #include "content/renderer/media/media_stream_audio_source.h"
 
-#include "content/renderer/media/webrtc_local_audio_track.h"
-#include "content/renderer/render_frame_impl.h"
+#include <algorithm>
+
+#include "base/bind.h"
+#include "content/renderer/media/media_stream_audio_track.h"
 #include "third_party/WebKit/public/platform/WebMediaStreamSource.h"
+#include "third_party/WebKit/public/platform/WebString.h"
 
 namespace content {
 
-MediaStreamAudioSource::MediaStreamAudioSource(
-    int render_frame_id,
-    const StreamDeviceInfo& device_info,
-    const SourceStoppedCallback& stop_callback,
-    PeerConnectionDependencyFactory* factory)
-    : render_frame_id_(render_frame_id), factory_(factory),
+MediaStreamAudioSource::MediaStreamAudioSource(bool is_local_source)
+    : is_local_source_(is_local_source),

[perkj_chrome, 2016/04/08 14:05:41]

remove?

[miu, 2016/04/19 00:40:22]

Looked into this.  To make a long-story short, this is plumbed through to the
track and the MediaStreamAudioRenderer's.  However, it's not used for anything
anywhere except the UMA stats in TrackAudioRenderer:
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...

That said, this is plumbed-in across almost a dozen files.  It would make this
huge CL even larger.  So, let's fix this in a follow-up CL.  Sound good?

[perkj_chrome, 2016/04/20 13:34:53]

sure

+      is_stopped_(false),
       weak_factory_(this) {
-  SetDeviceInfo(device_info);
-  SetStopCallback(stop_callback);
+  DVLOG(1) << "MediaStreamAudioSource::MediaStreamAudioSource(is a "
+           << (is_local_source_ ? "local" : "remote") << " source)";
 }
 
-MediaStreamAudioSource::MediaStreamAudioSource()
-    : render_frame_id_(-1), factory_(NULL), weak_factory_(this) {
+MediaStreamAudioSource::~MediaStreamAudioSource() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DVLOG(1) << "MediaStreamAudioSource::~MediaStreamAudioSource()";
 }
 
-MediaStreamAudioSource::~MediaStreamAudioSource() {}
-
 // static
 MediaStreamAudioSource* MediaStreamAudioSource::From(
     const blink::WebMediaStreamSource& source) {
@@ -37,50 +36,125 @@ MediaStreamAudioSource* MediaStreamAudioSource::From(
   return static_cast<MediaStreamAudioSource*>(source.getExtraData());
 }
 
+bool MediaStreamAudioSource::ConnectToTrack(
+    const blink::WebMediaStreamTrack& track) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(!track.isNull());
+
+  // Sanity-check that there is not already a MediaStreamAudioTrack instance
+  // associated with |track|.
+  if (MediaStreamAudioTrack::From(track)) {
+    LOG(DFATAL)
+        << "Attempting to connect another source to a WebMediaStreamTrack.";
+    return false;
+  }
+
+  if (is_stopped_)

[perkj_chrome, 2016/04/08 14:05:41]

Is it necessary to check for if the source is stopped?
I think that a track that is added a to a stopped source should produce a track
where the state is ended. 
That can happen if you clone a track that has already been stopped in JS.

[miu, 2016/04/19 00:40:22]

Good point.  Done.  I also just moved the code from
ConnectStartedSourceToTrack() into this method since nothing else calls it.

+    return false;
+  if (!EnsureSourceIsStarted())
+    return false;
+  ConnectStartedSourceToTrack(track);
+  return true;
+}
+
+media::AudioParameters MediaStreamAudioSource::GetAudioParameters() const {
+  base::AutoLock auto_lock(lock_);
+  return params_;
+}
+
+void* MediaStreamAudioSource::GetClassIdentifier() const {
+  return nullptr;
+}
+
+scoped_ptr<MediaStreamAudioTrack>
+MediaStreamAudioSource::CreateMediaStreamAudioTrack(const std::string& id) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  return make_scoped_ptr(new MediaStreamAudioTrack(is_local_source()));
+}
+
+bool MediaStreamAudioSource::EnsureSourceIsStarted() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DVLOG(1) << "MediaStreamAudioSource::EnsureSourceIsStarted()";
+  return true;
+}
+
+void MediaStreamAudioSource::EnsureSourceIsStopped() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DVLOG(1) << "MediaStreamAudioSource::EnsureSourceIsStopped()";
+}
+
+void MediaStreamAudioSource::SetFormat(const media::AudioParameters& params) {
+  // Note: May be called on any thread.
+  DCHECK(params.IsValid());
+  base::AutoLock auto_lock(lock_);
+  DVLOG(1) << "MediaStreamAudioSource::SetFormat("
+           << params.AsHumanReadableString() << "), was previously set to {"
+           << params_.AsHumanReadableString() << "}.";
+  if (params_.Equals(params))
+    return;
+  params_ = params;
+  for (MediaStreamAudioTrack* track : tracks_)
+    track->SetFormat(params);
+}
+
+void MediaStreamAudioSource::DeliverDataToTracks(
+    const media::AudioBus& audio_bus,
+    base::TimeTicks reference_time) {
+  // Note: May be called on any thread.
+  base::AutoLock auto_lock(lock_);
+  DCHECK(params_.IsValid());
+  for (MediaStreamAudioTrack* track : tracks_)
+    track->DeliverDataToSinks(audio_bus, reference_time);
+}
+
 void MediaStreamAudioSource::DoStopSource() {

[perkj_chrome, 2016/04/08 14:05:41]

why this extra indirection? Just for naming? 

[miu, 2016/04/19 00:40:22]

Are you asking why I have this call EnsureSourceIsStopped(), instead of just
requiring subclasses override DoStopSource() directly?  The main reason is that
there is some state changed in MSAudioSource (the is_stopped_ bool is set to
true), and some separate unknown state managed by subclasses.  Olga and I talked
about this in prior comments, and decided we didn't want to place requirements
on the code in subclasses to have to be sure to also change state in their
parent class.  This keeps the functionality specific to each class in the
inheritance hierarchy better encapsulated.

[perkj_chrome, 2016/04/20 13:34:53]

Acknowledged.

-  if (audio_capturer_)
-    audio_capturer_->Stop();
-  if (webaudio_capturer_)
-    webaudio_capturer_->Stop();
+  DCHECK(thread_checker_.CalledOnValidThread());
+  EnsureSourceIsStopped();
+  is_stopped_ = true;
 }
 
-void MediaStreamAudioSource::AddTrack(
-    const blink::WebMediaStreamTrack& track,
-    const blink::WebMediaConstraints& constraints,
-    const ConstraintsCallback& callback) {
-  // TODO(xians): Properly implement for audio sources.
-  if (!local_audio_source_.get()) {
-    if (!factory_->InitializeMediaStreamAudioSource(render_frame_id_,
-                                                    constraints, this)) {
-      // The source failed to start.
-      // UserMediaClientImpl rely on the |stop_callback| to be triggered when
-      // the last track is removed from the source. But in this case, the
-      // source is is not even started. So we need to fail both adding the
-      // track and trigger |stop_callback|.
-      callback.Run(this, MEDIA_DEVICE_TRACK_START_FAILURE, "");
-      StopSource();
-      return;
-    }
-  }
+void MediaStreamAudioSource::ConnectStartedSourceToTrack(
+    const blink::WebMediaStreamTrack& blink_track) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(!is_stopped_);
 
-  factory_->CreateLocalAudioTrack(track);
-  callback.Run(this, MEDIA_DEVICE_OK, "");
+  // Create a MediaStreamAudioTrack to deliver audio data directly from the
+  // calls to Capture() to all its managed sinks.  Pass ownership of it to the
+  // WebMediaStreamTrack.
+  scoped_ptr<MediaStreamAudioTrack> track =
+      CreateMediaStreamAudioTrack(blink_track.id().utf8());
+  track->Start(base::Bind(&MediaStreamAudioSource::StopAudioDeliveryTo,
+                          weak_factory_.GetWeakPtr(), track.get()));
+  track->SetEnabled(blink_track.isEnabled());
+  {
+    base::AutoLock auto_lock(lock_);
+    if (params_.IsValid())
+      track->SetFormat(params_);

[o1ka, 2016/04/01 15:11:41]

I would probably use a separate lock for |params_|.
And then I would use getter to access them here, there seem to be no need to
call SetFormat() under the lock.

[miu, 2016/04/19 00:40:22]

Done.  BTW--I realized MediaStreamAudioSource and MediaStreamAudioTrack both do
the same thing w.r.t. Add/Remove and audio format/deliver; so I factored those
things out into a separate MediaStreamAudioDeliverer "utility" class.

[o1ka, 2016/04/21 18:51:22]

Really like it!

+    tracks_.push_back(track.get());
+  }
+  blink::WebMediaStreamTrack mutable_blink_track = blink_track;
+  mutable_blink_track.setExtraData(track.release());
 }
 
 void MediaStreamAudioSource::StopAudioDeliveryTo(MediaStreamAudioTrack* track) {
-  DCHECK(track);
-  if (audio_capturer_) {
-    // The cast here is safe because only WebRtcLocalAudioTracks are ever used
-    // with WebRtcAudioCapturer sources.
-    //
-    // TODO(miu): That said, this ugly cast won't be necessary after my
-    // soon-upcoming refactoring change.
-    audio_capturer_->RemoveTrack(static_cast<WebRtcLocalAudioTrack*>(track));
-  }
-  if (webaudio_capturer_) {
-    // A separate source is created for each track, so just stop the source.
-    webaudio_capturer_->Stop();
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  // Remove |track| from the list of tracks, which will immediatly halt all
+  // further audio data delivery.
+  bool did_remove_last_track = false;
+  {
+    base::AutoLock auto_lock(lock_);
+    const bool had_tracks = !tracks_.empty();
+    const auto it = std::find(tracks_.begin(), tracks_.end(), track);
+    if (it != tracks_.end())
+      tracks_.erase(it);
+    did_remove_last_track = had_tracks && tracks_.empty();
   }
+  // TODO(miu): Is this the behavior we want?  Perhaps sources should be
+  // explicitly closed, rather than auto-stop on the removal of the last track?

[perkj_chrome, 2016/04/08 14:05:41]

This is according to spec. 

If you have two tracks for the same source- stopping the first track only stops
that track. Stopping the second track actually stops the source.

[miu, 2016/04/19 00:40:22]

Thanks for the clarification.  I've replaced this TODO comment with a comment
that states the spec requires this.

+  // As of this writing, UserMediaClientImpl depends on this.
+  if (!is_stopped_ && did_remove_last_track)
+    MediaStreamSource::StopSource();
 }
 
 }  // namespace content