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

content/renderer/media/media_stream_audio_source.h

Index: content/renderer/media/media_stream_audio_source.h
diff --git a/content/renderer/media/media_stream_audio_source.h b/content/renderer/media/media_stream_audio_source.h
index 6d7da5ccf003b6b1c54accf24d9c2d857f8a32de..1e0f210bd2e21777ed8abbb8464f95f619721e49 100644
--- a/content/renderer/media/media_stream_audio_source.h
+++ b/content/renderer/media/media_stream_audio_source.h
@@ -5,94 +5,154 @@
 #ifndef CONTENT_RENDERER_MEDIA_MEDIA_STREAM_AUDIO_SOURCE_H_
 #define CONTENT_RENDERER_MEDIA_MEDIA_STREAM_AUDIO_SOURCE_H_
 
+#include <string>
+#include <vector>
+
 #include "base/compiler_specific.h"
 #include "base/macros.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
+#include "base/synchronization/lock.h"
+#include "base/threading/thread_checker.h"
 #include "content/common/content_export.h"
 #include "content/renderer/media/media_stream_source.h"
-#include "content/renderer/media/webaudio_capturer_source.h"
-#include "content/renderer/media/webrtc/peer_connection_dependency_factory.h"
-#include "content/renderer/media/webrtc_audio_capturer.h"
-#include "third_party/webrtc/api/mediastreaminterface.h"
+#include "media/audio/audio_parameters.h"
+#include "third_party/WebKit/public/platform/WebMediaStreamSource.h"
+#include "third_party/WebKit/public/platform/WebMediaStreamTrack.h"
 
 namespace content {
 
 class MediaStreamAudioTrack;
 
-// TODO(miu): In a soon-upcoming set of refactoring changes, this class will
-// become a base class for managing tracks (part of what WebRtcAudioCapturer
-// does today).  Then, the rest of WebRtcAudioCapturer will be rolled into a
-// subclass.   http://crbug.com/577874
+// Represents a source of audio, and manages the delivery of audio data between
+// the source implementation and one or more MediaStreamAudioTracks.  This is
+// a base class providing all the necessary functionality to connect tracks and
+// have audio data delivered to them.  Subclasses provide the actual audio
+// source implementation (e.g., media::AudioCapturerSource), and should
+// implement the DoStopSource() and EnsureSourceIsStarted() methods, and call
+// DeliverDataToTracks().
+//
+// This base class can be instantiated, to be used as a place-holder or a "null"
+// source of audio.  This can be useful for unit testing, wherever a mock is
+// needed, and/or calls to DeliverDataToTracks() must be made at very specific
+// times.
+//
+// An instance of this class is owned by blink::WebMediaStreamSource.
+//
+// Usage example:
+//
+//   class MyAudioSource : public MediaStreamSource { ... };
+//
+//   blink::WebMediaStreamSource blink_source = ...;
+//   blink::WebMediaStreamTrack blink_track = ...;
+//   blink_source.setExtraData(new MyAudioSource());  // Takes ownership.
+//   if (MediaStreamAudioSource::From(blink_source)
+//           ->ConnectToTrack(blink_track)) {
+//     LOG(INFO) << "Success!";
+//   } else {
+//     LOG(ERROR) << "Failed!";
+//   }
 class CONTENT_EXPORT MediaStreamAudioSource
     : NON_EXPORTED_BASE(public MediaStreamSource) {
  public:
-  MediaStreamAudioSource(int render_frame_id,
-                         const StreamDeviceInfo& device_info,
-                         const SourceStoppedCallback& stop_callback,
-                         PeerConnectionDependencyFactory* factory);
-  MediaStreamAudioSource();
+  explicit MediaStreamAudioSource(bool is_local_source);
   ~MediaStreamAudioSource() override;
 
   // Returns the MediaStreamAudioSource instance owned by the given blink
   // |source| or null.
-  static MediaStreamAudioSource* From(const blink::WebMediaStreamSource& track);
-
-  void AddTrack(const blink::WebMediaStreamTrack& track,
-                const blink::WebMediaConstraints& constraints,
-                const ConstraintsCallback& callback);
+  static MediaStreamAudioSource* From(
+      const blink::WebMediaStreamSource& source);
 
+  // Provides a weak reference to this MediaStreamAudioSource.  The weak
+  // pointer may only be dereferenced on the main thread.
   base::WeakPtr<MediaStreamAudioSource> GetWeakPtr() {
     return weak_factory_.GetWeakPtr();
   }
 
-  // Removes |track| from the list of instances that get a copy of the source
-  // audio data.
-  void StopAudioDeliveryTo(MediaStreamAudioTrack* track);
+  // Returns true if the source of audio is local to the application (e.g.,
+  // microphone input or loopback audio capture) as opposed to audio being
+  // streamed-in from outside the application.
+  bool is_local_source() const { return is_local_source_; }
 
-  WebRtcAudioCapturer* audio_capturer() const { return audio_capturer_.get(); }
+  // Connects this source to the given |track|, creating the appropriate
+  // implementation of the content::MediaStreamAudioTrack interface, which
+  // becomes associated with and owned by |track|.
+  //
+  // Returns true if the source was successfully started and the
+  // MediaStreamAudioTrack assigned to |track.extraData()|.
+  bool ConnectToTrack(const blink::WebMediaStreamTrack& track);
 
-  void SetAudioCapturer(scoped_ptr<WebRtcAudioCapturer> capturer) {
-    DCHECK(!audio_capturer_.get());
-    audio_capturer_ = std::move(capturer);
-  }
+  // Returns the current format of the audio passing through this source to the
+  // sinks.  This can return invalid parameters if the source has not yet been
+  // started.  This method is thread-safe.
+  media::AudioParameters GetAudioParameters() const;
 
-  webrtc::AudioSourceInterface* local_audio_source() {
-    return local_audio_source_.get();
-  }
+  // Returns a unique class identifier.  Some subclasses override and use this
+  // method to provide safe down-casting to their type.
+  virtual void* GetClassIdentifier() const;
 
-  void SetLocalAudioSource(scoped_refptr<webrtc::AudioSourceInterface> source) {
-    local_audio_source_ = std::move(source);
-  }
+ protected:

[o1ka, 2016/03/30 15:00:57]

Readability side note: We've got here overrides, virtuals to override and
helpers to be called. It would be nice to sort out the list accordingly. Even
better if we can distinguish them by names... And probably even better if we do
not have to deal with such a variety :)

[miu, 2016/03/31 04:57:59]

I've tried, but I simply don't understand any part of your comment here.  Each
method is clearly documented in terms of what the subclass should do with it. 
What needs to be sorted differently?  What do you mean by "variety" and why is
it hurting readability?  Perhaps you could provide a code sample to help
explain?

Maybe now that I've untangled the StopSource()/DoStopSource()/is_stopped_
hairball, everything looks good to you now?

[o1ka, 2016/03/31 16:35:35]

Sorry, I meant it would be nice to have the protected methods ordered here, like
CreateMediaStreamAudioTrack and EnsureSourceStarted together, etc.

Now it's just perfect.

+  // Returns a new MediaStreamAudioTrack.  |id| is the blink track's ID in
+  // UTF-8.  Subclasses may override this to provide an extended implementation.
+  virtual scoped_ptr<MediaStreamAudioTrack> CreateMediaStreamAudioTrack(
+      const std::string& id);
 
-  WebAudioCapturerSource* webaudio_capturer() const {
-    return webaudio_capturer_.get();
-  }
+  // Default "no-op" MediaStreamAudioSource implementation that just sets
+  // |is_stopped_| to true.  Subclasses should override this method.
+  void DoStopSource() override;
 
-  void SetWebAudioCapturer(scoped_ptr<WebAudioCapturerSource> capturer) {
-    DCHECK(!webaudio_capturer_.get());
-    webaudio_capturer_ = std::move(capturer);
-  }
+  // Returns true if the source has already been started and has not yet been
+  // stopped.  Otherwise, attempts to start the source and returns true if
+  // successful.  Subclasses should override this method.
+  virtual bool EnsureSourceIsStarted();
 
- protected:
-  void DoStopSource() override;
+  // Called by subclasses to update the format of the audio passing through this
+  // source to the sinks.  This may be called at any time, before or after
+  // tracks have been connected; but must be called at least once before
+  // DeliverDataToTracks().  This method is thread-safe.
+  void SetFormat(const media::AudioParameters& params);
+
+  // Called by subclasses to deliver audio data to the currently-connected
+  // tracks.  This method is thread-safe.
+  void DeliverDataToTracks(const media::AudioBus& audio_bus,
+                           base::TimeTicks reference_time);
+
+  // Set to true by DoStopSource() once the source has been permanently
+  // stopped and no further calls to DeliverDataToTracks() will be made.
+  // Subclasses that override DoStopSource() must set this.
+  bool is_stopped_;

[o1ka, 2016/03/30 15:00:57]

I think this needs to be private. The reason: now it's accessed by both
MadiaStreamAudioSource and its children, which makes it very difficult to ensure
that the access is thread-safe.
(It's actually required by the style guide as well
https://google.github.io/styleguide/cppguide.html#Access_Control)

[miu, 2016/03/31 04:57:59]

Done.

+
+  // In debug builds, check that all methods that could cause object graph
+  // or data flow changes are being called on the main thread.
+  base::ThreadChecker thread_checker_;

[o1ka, 2016/03/30 15:00:57]

I think each class in the hierarchy should have its own thread checker: thread
checking is a predicate which should remain the same no matter who inherited the
class => it should no be altered by children => should not be accessed by them.
And it's against the style guide anyways to have it protected.

[miu, 2016/03/31 04:57:59]

Done.

 
  private:
-  const int render_frame_id_;
-  PeerConnectionDependencyFactory* const factory_;
+  // Instantiates the appropriate MediaStreamAudioTrack implementation and sets
+  // it as |track|'s extraData.  This is called by ConnectToTrack() after all
+  // preconditions have been met: 1) A prior call to EnsureSourceIsStarted()
+  // returned true; 2) |track| does not already hold a MediaStreamAudioTrack
+  // instance.
+  void ConnectStartedSourceToTrack(const blink::WebMediaStreamTrack& track);
 
-  // MediaStreamAudioSource is the owner of either a WebRtcAudioCapturer or a
-  // WebAudioCapturerSource.
-  //
-  // TODO(miu): In a series of soon-upcoming changes, WebRtcAudioCapturer and
-  // WebAudioCapturerSource will become subclasses of MediaStreamAudioSource
-  // instead.
-  scoped_ptr<WebRtcAudioCapturer> audio_capturer_;
-  scoped_ptr<WebAudioCapturerSource> webaudio_capturer_;
-
-  // This member holds an instance of webrtc::LocalAudioSource. This is used
-  // as a container for audio options.
-  scoped_refptr<webrtc::AudioSourceInterface> local_audio_source_;
+  // Removes |track| from the list of instances that get a copy of the source
+  // audio data.  The "stop callback" that was provided to the track calls
+  // this.
+  void StopAudioDeliveryTo(MediaStreamAudioTrack* track);
+
+  // True if the source of audio is a local device.  False if the source is
+  // remote (e.g., streamed-in from a server).
+  const bool is_local_source_;
+
+  // Protects concurrent access to |params_| and |tracks_|.
+  mutable base::Lock lock_;
+
+  // Specifies the current format of the audio passing through the pipeline.
+  media::AudioParameters params_;
+
+  // List of currently-connected MediaStreamAudioTracks.  While
+  // MediaStreamAudioSource creates these instances, blink::WebMediaStreamTrack
+  // instances own the objects.
+  std::vector<MediaStreamAudioTrack*> tracks_;
 
   // Provides weak pointers so that MediaStreamAudioTracks won't call
   // StopAudioDeliveryTo() if this instance dies first.