MediaStream audio object graph untangling and clean-ups.

content/renderer/media/webrtc/media_stream_remote_audio_track.cc

 // Copyright 2015 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/renderer/media/webrtc/media_stream_remote_audio_track.h"
 
 #include <stddef.h>
 
 #include <list>
 
@@ skipping 102 matching lines @@
 };
 
 MediaStreamRemoteAudioTrack::MediaStreamRemoteAudioTrack(
     const blink::WebMediaStreamSource& source, bool enabled)
     : MediaStreamAudioTrack(false), source_(source), enabled_(enabled) {
   DCHECK(source.extraData());  // Make sure the source has a native source.
 }
 
 MediaStreamRemoteAudioTrack::~MediaStreamRemoteAudioTrack() {
   DCHECK(main_render_thread_checker_.CalledOnValidThread());
-  source()->RemoveAll(this);
+  // Ensure the track is stopped.
+  Stop();
 }
 
 void MediaStreamRemoteAudioTrack::SetEnabled(bool enabled) {
   DCHECK(main_render_thread_checker_.CalledOnValidThread());
 
   // This affects the shared state of the source for whether or not it's a part
   // of the mixed audio that's rendered for remote tracks from WebRTC.
   // All tracks from the same source will share this state and thus can step
   // on each other's toes.
   // This is also why we can't check the |enabled_| state for equality with
   // |enabled| before setting the mixing enabled state. |enabled_| and the
   // shared state might not be the same.
   source()->SetEnabledForMixing(enabled);
 
   enabled_ = enabled;
   source()->SetSinksEnabled(this, enabled);
 }
 
 void MediaStreamRemoteAudioTrack::Stop() {
   DCHECK(main_render_thread_checker_.CalledOnValidThread());
+
+  MediaStreamAudioTrack::WillStopTrack();

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

What if some observers rely on the fact that the callbacks are run before/after
the track has actually stopped (next lines)? Since observers subscribe in the
parent class, we probably should explicitly say there what is the expected
sequence of Stop()/WillStopTrack() actions (stop first, notify second, or the
opposite) and make the child classes to adhere to it, so that observers know
what to expect regardless of implementation the parent class is instantiated
with. What do you think?

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

Good point, and I made the behavior more precisely defined in the latest patch
set.  No more observers: The point is for the audio flow to be immediately
halted, and then let the subclasses perform any additional steps as needed.

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

I like it!

+
+  source()->RemoveAll(this);
+
   // Stop means that a track should be stopped permanently. But
   // since there is no proper way of doing that on a remote track, we can
   // at least disable the track. Blink will not call down to the content layer
   // after a track has been stopped.
   SetEnabled(false);
 }
 
 void MediaStreamRemoteAudioTrack::AddSink(MediaStreamAudioSink* sink) {
   DCHECK(main_render_thread_checker_.CalledOnValidThread());
   return source()->AddSink(sink, this, enabled_);
@@ skipping 68 matching lines @@
   if (sink_)
     sink_->RemoveAll(track);
 }
 
 webrtc::AudioTrackInterface* MediaStreamRemoteAudioSource::GetAudioAdapter() {
   DCHECK(thread_checker_.CalledOnValidThread());
   return track_.get();
 }
 
 }  // namespace content