MediaStream audio object graph untangling and clean-ups.

content/renderer/media/webrtc/webrtc_local_audio_track_adapter.cc

 // 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.
 
 #include "content/renderer/media/webrtc/webrtc_local_audio_track_adapter.h"
 
 #include "base/location.h"
 #include "base/logging.h"
 #include "content/renderer/media/media_stream_audio_processor.h"
 #include "content/renderer/media/webrtc/peer_connection_dependency_factory.h"
 #include "content/renderer/media/webrtc/webrtc_audio_sink_adapter.h"
 #include "content/renderer/media/webrtc_local_audio_track.h"
 #include "content/renderer/render_thread_impl.h"
 #include "third_party/webrtc/api/mediastreaminterface.h"
 
 namespace content {
 
 static const char kAudioTrackKind[] = "audio";
 
 scoped_refptr<WebRtcLocalAudioTrackAdapter>
 WebRtcLocalAudioTrackAdapter::Create(
     const std::string& label,
     webrtc::AudioSourceInterface* track_source) {
   // TODO(tommi): Change this so that the signaling thread is one of the
   // parameters to this method.
-  scoped_refptr<base::SingleThreadTaskRunner> signaling_thread;
+  scoped_refptr<base::SingleThreadTaskRunner> signaling_task_runner;
   RenderThreadImpl* current = RenderThreadImpl::current();
   if (current) {
     PeerConnectionDependencyFactory* pc_factory =
         current->GetPeerConnectionDependencyFactory();
-    signaling_thread = pc_factory->GetWebRtcSignalingThread();
+    signaling_task_runner = pc_factory->GetWebRtcSignalingThread();
   }
 
-  LOG_IF(ERROR, !signaling_thread.get()) << "No signaling thread!";
+  LOG_IF(ERROR, !signaling_task_runner.get()) << "No signaling thread!";
 
   rtc::RefCountedObject<WebRtcLocalAudioTrackAdapter>* adapter =
       new rtc::RefCountedObject<WebRtcLocalAudioTrackAdapter>(
-          label, track_source, signaling_thread);
+          label, track_source, signaling_task_runner);
   return adapter;
 }
 
 WebRtcLocalAudioTrackAdapter::WebRtcLocalAudioTrackAdapter(
     const std::string& label,
     webrtc::AudioSourceInterface* track_source,
-    const scoped_refptr<base::SingleThreadTaskRunner>& signaling_thread)
+    const scoped_refptr<base::SingleThreadTaskRunner>& signaling_task_runner)
     : webrtc::MediaStreamTrack<webrtc::AudioTrackInterface>(label),
       owner_(NULL),
       track_source_(track_source),
-      signaling_thread_(signaling_thread),
-      signal_level_(0) {
-  signaling_thread_checker_.DetachFromThread();
-  capture_thread_.DetachFromThread();
-}
+      signaling_task_runner_(signaling_task_runner) {}

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

How thread safety of the class is guaranteed if signaling_task_runner is NULL,
and what is the use case for that?

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

Many of the unit tests run single-threaded.  See lines 27-34 above.

tommi's TODO (line 24) would be the way to resolve this problem.  As much as I'd
like to fix everything, this CL is already too big.  ;)

For now, I've added a stricter check in Create() and also log a warning when we
think we're running in a single-threaded unit test.

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

Acknowledged.

 
 WebRtcLocalAudioTrackAdapter::~WebRtcLocalAudioTrackAdapter() {
 }
 
 void WebRtcLocalAudioTrackAdapter::Initialize(WebRtcLocalAudioTrack* owner) {
   DCHECK(!owner_);
   DCHECK(owner);
   owner_ = owner;
 }
 
 void WebRtcLocalAudioTrackAdapter::SetAudioProcessor(
     const scoped_refptr<MediaStreamAudioProcessor>& processor) {
-  // SetAudioProcessor will be called when a new capture thread has been
-  // initialized, so we need to detach from any current capture thread we're
-  // checking and attach to the current one.
-  capture_thread_.DetachFromThread();
-  DCHECK(capture_thread_.CalledOnValidThread());
-  base::AutoLock auto_lock(lock_);
+  // |audio_processor_| must only be set once.  This eliminates the need to
+  // protect |audio_processor_| with a mutex.

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

Is there any guarantee that there won't be a race between two
SetAudioProcessor() calls?
And how calling it only once guarantees that it does not race with
GetAudioProcessor()?

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

No, but the header comments and the DCHECK on lines 64-65 are meant to ensure
the method is only called once.  So, if SetAudioProcessor() is called a second
time, that's a bug since it's explicitly documented that should not be done.
Two ways: 1) This method is called during initialization, before the signaling
thread from WebRTC would start calling GetAudioProcessor().  So, there is no
possibility for concurrent access.  2) Even if there were concurrent access,
assigning to a scoped_refptr<> only once guarantees the signaling thread will
only ever see either nullptr or a non-null pointer that never changes.  Either
way, the signaling thread will do the right thing.

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

Agree that calling SetAudioProcessor() second time is a bug. There is still a
tiny chance that DCHECK won't catch it because of a race. Also, the code may
evolve in a way that this race can become less obvious and lead to
harder-to-catch bugs.
I think (1) worth documenting in the header. As far as I understand,
scope_refptr operations are not atomic, so (2) is not guaranteed without (1). If
I'm not missing something and this is the case, the comments should be like
"|audio_processor_| must only be set once and _before_ any call to
GetAudioProcessor(). (And now the question is how to ensure the correct usage.
My paranoia tells me to introduce an atomic "initialized" flag. You never know
how the code itself and its usage are going to evolve. Probably it's an overkill
though.)

Also it would be good to have a header comment like "The variables below are not
protected by locks because... Take this into account when messing up with this
code" :).

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

Where is the race condition?  I don't see what else should be done here.  If
someone in the future decides to change the code in a way that violates the
contract documented both in the header file and here, they had better make the
appropriate changes necessary to ensure thread safety.  I can't anticipate every
possible change that *might* be made.  ;-)
Done.  Good idea.
I probably should never have de-railed the discussion with point #2.  ;-) 
Basically, it's irrelevant here because we don't need to worry about concurrent
reads and mutations.

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

Acknowledged.

+  DCHECK(processor.get());
+  DCHECK(!audio_processor_);
   audio_processor_ = processor;
 }
 
+void WebRtcLocalAudioTrackAdapter::SetLevel(
+    scoped_refptr<MediaStreamAudioLevelCalculator::Level> level) {
+  // |level_| must only be set once.  This eliminates the need to protect
+  // |level_| with a mutex.

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

Same questions as for SetAudioProcessor()

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

Good questions.  Answered in other comment.

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

Same as above: looks like just "setting it once" is not enough to guarantee
thread safety.

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

I removed these comments since I think they were making it sound like
|audio_processor_| and |level_| were being accessed concurrently.  Part of
that's my fault, since I was in the frame-of-mind of having just removed the
unnecessary Lock around them.

IMO, since the header file comments explain when the setters and getters must be
called, that should be enough.

+  DCHECK(level.get());
+  DCHECK(!level_);
+  level_ = level;
+}
+
 std::string WebRtcLocalAudioTrackAdapter::kind() const {
   return kAudioTrackKind;
 }
 
 bool WebRtcLocalAudioTrackAdapter::set_enabled(bool enable) {
   // If we're not called on the signaling thread, we need to post a task to
   // change the state on the correct thread.
-  if (signaling_thread_.get() && !signaling_thread_->BelongsToCurrentThread()) {
-    signaling_thread_->PostTask(FROM_HERE,
+  if (signaling_task_runner_.get() &&

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

nit: if (signaling_task_runner_ && ...)

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

Done.

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

Acknowledged.

+      !signaling_task_runner_->BelongsToCurrentThread()) {
+    signaling_task_runner_->PostTask(FROM_HERE,
         base::Bind(
             base::IgnoreResult(&WebRtcLocalAudioTrackAdapter::set_enabled),
             this, enable));
     return true;
   }
 
   return webrtc::MediaStreamTrack<webrtc::AudioTrackInterface>::

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

will be called from a random thread  if signaling_task_runner_ is NULL (see my
question in the constructor)

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

Explained above.

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

Acknowledged.

       set_enabled(enable);
 }
 
 void WebRtcLocalAudioTrackAdapter::AddSink(
     webrtc::AudioTrackSinkInterface* sink) {
-  DCHECK(signaling_thread_checker_.CalledOnValidThread());
+  DCHECK(!signaling_task_runner_ ||
+         signaling_task_runner_->RunsTasksOnCurrentThread());
   DCHECK(sink);
 #ifndef NDEBUG
   // Verify that |sink| has not been added.
   for (ScopedVector<WebRtcAudioSinkAdapter>::const_iterator it =
            sink_adapters_.begin();
        it != sink_adapters_.end(); ++it) {
     DCHECK(!(*it)->IsEqual(sink));
   }
 #endif
 
   scoped_ptr<WebRtcAudioSinkAdapter> adapter(
       new WebRtcAudioSinkAdapter(sink));
   owner_->AddSink(adapter.get());
   sink_adapters_.push_back(adapter.release());
 }
 
 void WebRtcLocalAudioTrackAdapter::RemoveSink(
     webrtc::AudioTrackSinkInterface* sink) {
-  DCHECK(signaling_thread_checker_.CalledOnValidThread());
+  DCHECK(!signaling_task_runner_ ||
+         signaling_task_runner_->RunsTasksOnCurrentThread());
   DCHECK(sink);
   for (ScopedVector<WebRtcAudioSinkAdapter>::iterator it =
            sink_adapters_.begin();
        it != sink_adapters_.end(); ++it) {
     if ((*it)->IsEqual(sink)) {
       owner_->RemoveSink(*it);
       sink_adapters_.erase(it);
       return;
     }
   }
 }
 
 bool WebRtcLocalAudioTrackAdapter::GetSignalLevel(int* level) {
-  DCHECK(signaling_thread_checker_.CalledOnValidThread());
+  DCHECK(!signaling_task_runner_ ||
+         signaling_task_runner_->RunsTasksOnCurrentThread());
 
-  base::AutoLock auto_lock(lock_);
-  *level = signal_level_;
+  // |level_| is only set once, so it's safe to read without first acquiring a
+  // mutex.
+  if (!level_)

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

How does it guarantee that there is no race?

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

Answered in comment above.

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

Same as above.

+    return false;
+  const float signal_level = level_->GetCurrent();
+  DCHECK_GE(signal_level, 0.0f);
+  DCHECK_LE(signal_level, 1.0f);
+  // Convert from float in range [0.0,1.0] to an int in range [0,32767].
+  *level = static_cast<int>(signal_level * std::numeric_limits<int16_t>::max() +
+                            0.5f /* rounding to nearest int */);
   return true;
 }
 
 rtc::scoped_refptr<webrtc::AudioProcessorInterface>
 WebRtcLocalAudioTrackAdapter::GetAudioProcessor() {
-  DCHECK(signaling_thread_checker_.CalledOnValidThread());
-  base::AutoLock auto_lock(lock_);
+  DCHECK(!signaling_task_runner_ ||
+         signaling_task_runner_->RunsTasksOnCurrentThread());
   return audio_processor_.get();
 }
 
-void WebRtcLocalAudioTrackAdapter::SetSignalLevel(int signal_level) {
-  DCHECK(capture_thread_.CalledOnValidThread());
-  base::AutoLock auto_lock(lock_);
-  signal_level_ = signal_level;
-}
-
 webrtc::AudioSourceInterface* WebRtcLocalAudioTrackAdapter::GetSource() const {
-  DCHECK(signaling_thread_checker_.CalledOnValidThread());
+  DCHECK(!signaling_task_runner_ ||
+         signaling_task_runner_->RunsTasksOnCurrentThread());
   return track_source_;
 }
 
 }  // namespace content