MediaStream audio object graph untangling and clean-ups.

content/renderer/media/media_stream_audio_track.cc

Index: content/renderer/media/media_stream_audio_track.cc
diff --git a/content/renderer/media/media_stream_audio_track.cc b/content/renderer/media/media_stream_audio_track.cc
index 278ab05bb107369cdd23614565470e40846a3518..587b68214331954d652deecebfc7bc3eaf3dc5e8 100644
--- a/content/renderer/media/media_stream_audio_track.cc
+++ b/content/renderer/media/media_stream_audio_track.cc
@@ -12,13 +12,19 @@ namespace content {
 
 MediaStreamAudioTrack::MediaStreamAudioTrack(bool is_local_track)
     : MediaStreamTrack(is_local_track) {
+  DVLOG(1) << "MediaStreamAudioTrack::MediaStreamAudioTrack(is a "
+           << (is_local_track ? "local" : "remote") << " track)";
 }
 
 MediaStreamAudioTrack::~MediaStreamAudioTrack() {
+  DCHECK(main_render_thread_checker_.CalledOnValidThread());
+  DVLOG(1) << "MediaStreamAudioTrack::~MediaStreamAudioTrack()";
+  Stop();

[o1ka, 2016/02/29 14:28:04]

If you add a method of a child class as a stop observer, wouldn't you call that
method on an already-deleted child object in Stop() here?

For example, these lines from WebRtcLocalAudioTrack bother me:
  MediaStreamAudioTrack::AddStopObserver(base::Bind(
       &WebRtcLocalAudioTrack::RemoveAllSinks, base::Unretained(this)));

[miu, 2016/03/01 09:43:54]

Yes, you're right.  I had mitigated that by having the child class' dtor call
Stop().

However, I've looked at this again, and I see now that I over-used this
"pattern."  It should not be used for calling back into the MSAudioTrack
subclasses themselves.  So, I put back the Stop() method in the subclasses and
removed the base::Closures that would have them to refer to themselves.

The stop observers are meant for other classes (the capturers) to be notified
that the track is stopping.  Therefore, now I just require MSAudioTrack
subclasses to implement Stop(), and they are required to call the
WillStopTrack() method along with their other "stop" tasks.  SGTY?

[o1ka, 2016/03/01 14:18:58]

Though I had the same idea initially, the more I look at it, the more I feel
uncomfortable bout the fact that it's up to child classes to call
WillStopTrack() whenever they think reasonable (see my comment in
MediaStreamRemoteAudioTrack). It means the stop observers can't be sure when to
expect those calls. So, I would probably do an opposite thing: introduce a
protected OnStop() for children to implement, leave Stop() to be final in
MediaStreamTrack and make it call OnStop() after(before?) notifying stop
observers (but without calling Stop() in ~MediaStreamTrack()). And if you also
state that child classes must call Stop() in their destructors, it means clients
do not need to care of mandatory call to Stop(). What do you think?

[miu, 2016/03/02 01:12:50]

Done.  I also re-evaluated why I needed to add multiple stop observers.  After
looking over all the code (including future changes), it turns out I really
don't need multiple stop observers.  So, there was more room for simplifying
things.

What I really needed was a way for a track to be able to halt the audio flow
from the capturer when its Stop() method is called.  So, I've explicitly added a
Start() method which takes a single callback.  The callback's sole purpose is to
halt the audio flow when the track requires it.

[o1ka, 2016/03/02 16:31:13]

Looks good!

+  DCHECK(stop_callbacks_.empty());
 }
 
 // static
-MediaStreamAudioTrack* MediaStreamAudioTrack::GetTrack(
+MediaStreamAudioTrack* MediaStreamAudioTrack::From(
     const blink::WebMediaStreamTrack& track) {
   if (track.isNull() ||
       track.source().type() != blink::WebMediaStreamSource::TypeAudio) {
@@ -27,6 +33,23 @@ MediaStreamAudioTrack* MediaStreamAudioTrack::GetTrack(
   return static_cast<MediaStreamAudioTrack*>(track.extraData());
 }
 
+void MediaStreamAudioTrack::AddStopObserver(
+    const base::Closure& stop_callback) {
+  DCHECK(main_render_thread_checker_.CalledOnValidThread());
+  DCHECK(!stop_callback.is_null());
+  DVLOG(1) << "MediaStreamAudioTrack::AddStopObserver()";
+  stop_callbacks_.push_back(stop_callback);
+}
+
+void MediaStreamAudioTrack::Stop() {
+  DCHECK(main_render_thread_checker_.CalledOnValidThread());
+  DVLOG(1) << "MediaStreamAudioTrack::Stop()";
+  std::vector<base::Closure> callbacks_to_run;
+  callbacks_to_run.swap(stop_callbacks_);
+  for (const base::Closure& callback : callbacks_to_run)
+    callback.Run();
+}
+
 webrtc::AudioTrackInterface* MediaStreamAudioTrack::GetAudioAdapter() {
   NOTREACHED();
   return nullptr;