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

content/renderer/media/media_stream_audio_source.h

 // Copyright 2014 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.
 
 #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);
+  static MediaStreamAudioSource* From(
+      const blink::WebMediaStreamSource& source);
 
-  void AddTrack(const blink::WebMediaStreamTrack& track,
-                const blink::WebMediaConstraints& constraints,
-                const ConstraintsCallback& callback);
-
+  // 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();
   }
 
+  // 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_; }
+
+  // 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);
+
+  // 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;
+
+  // Returns a unique class identifier.  Some subclasses override and use this
+  // method to provide safe down-casting to their type.
+  virtual void* GetClassIdentifier() const;
+
+ 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);
+
+  // Default "no-op" MediaStreamAudioSource implementation that just sets
+  // |is_stopped_| to true.  Subclasses should override this method.
+  void DoStopSource() override;
+
+  // 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();
+
+  // 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:
+  // 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);
+
   // Removes |track| from the list of instances that get a copy of the source
-  // audio data.
+  // audio data.  The "stop callback" that was provided to the track calls
+  // this.
   void StopAudioDeliveryTo(MediaStreamAudioTrack* track);
 
-  WebRtcAudioCapturer* audio_capturer() const { return audio_capturer_.get(); }
+  // 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_;
 
-  void SetAudioCapturer(scoped_ptr<WebRtcAudioCapturer> capturer) {
-    DCHECK(!audio_capturer_.get());
-    audio_capturer_ = std::move(capturer);
-  }
+  // Protects concurrent access to |params_| and |tracks_|.
+  mutable base::Lock lock_;
 
-  webrtc::AudioSourceInterface* local_audio_source() {
-    return local_audio_source_.get();
-  }
+  // Specifies the current format of the audio passing through the pipeline.
+  media::AudioParameters params_;
 
-  void SetLocalAudioSource(scoped_refptr<webrtc::AudioSourceInterface> source) {
-    local_audio_source_ = std::move(source);
-  }
-
-  WebAudioCapturerSource* webaudio_capturer() const {
-    return webaudio_capturer_.get();
-  }
-
-  void SetWebAudioCapturer(scoped_ptr<WebAudioCapturerSource> capturer) {
-    DCHECK(!webaudio_capturer_.get());
-    webaudio_capturer_ = std::move(capturer);
-  }
-
- protected:
-  void DoStopSource() override;
-
- private:
-  const int render_frame_id_;
-  PeerConnectionDependencyFactory* const factory_;
-
-  // 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_;
+  // 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.
   base::WeakPtrFactory<MediaStreamAudioSource> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(MediaStreamAudioSource);
 };
 
 }  // namespace content
 
 #endif  // CONTENT_RENDERER_MEDIA_MEDIA_STREAM_AUDIO_SOURCE_H_