Break out WebAudio suspension code into new class. Add tests.

media/base/silent_sink_suspender.cc

+// Copyright 2016 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 "media/base/silent_sink_suspender.h"
+
+#include "base/single_thread_task_runner.h"
+#include "base/threading/thread_task_runner_handle.h"
+
+namespace media {
+
+SilentSinkSuspender::SilentSinkSuspender(
+    AudioRendererSink::RenderCallback* callback,
+    base::TimeDelta silence_timeout,
+    const AudioParameters& params,
+    const scoped_refptr<AudioRendererSink>& sink,
+    const scoped_refptr<base::SingleThreadTaskRunner>& worker)
+    : callback_(callback),
+      params_(params),
+      sink_(sink),
+      task_runner_(base::ThreadTaskRunnerHandle::Get()),
+      silence_timeout_(silence_timeout),
+      fake_sink_(worker, params_),
+      sink_transition_callback_(
+          base::Bind(&SilentSinkSuspender::TransitionSinks,
+                     base::Unretained(this))) {
+  DCHECK(params_.IsValid());
+  DCHECK(sink_);
+  DCHECK(callback_);
+  DCHECK(task_runner_->BelongsToCurrentThread());
+}
+
+SilentSinkSuspender::~SilentSinkSuspender() {
+  DCHECK(task_runner_->BelongsToCurrentThread());
+  fake_sink_.Stop();
+}
+
+int SilentSinkSuspender::Render(AudioBus* dest,
+                                uint32_t frames_delayed,
+                                uint32_t frames_skipped) {
+  // Lock required since AudioRendererSink::Pause() is not synchronous, we need
+  // to discard these calls during the transition to the fake sink.
+  base::AutoLock al(transition_lock_);

[o1ka, 2016/09/26 12:21:55]

It would be good to mention in the header that transition to a fake
sink/backward is not entirely smooth and Render() calls may come in uneven
intervals during the switch.

[DaleCurtis, 2016/09/26 19:45:58]

Done.

+  if (is_using_fake_sink_ && dest) {
+    // Audio should be silent at this point, if not, it will be handled once the
+    // transition to the fake sink is complete.
+    dest->Zero();
+    return dest->frames();
+  }
+
+  // When we're using the |fake_sink_| a null destination will be sent; we store
+  // the audio data for a future transition out of silence.
+  if (!dest) {
+    DCHECK(is_using_fake_sink_);
+    DCHECK_EQ(frames_delayed, 0u);
+    DCHECK_EQ(frames_skipped, 0u);
+
+    // If we have no buffers or a transition is pending, one or more extra
+    // Render() calls have occurred in before TransitionSinks() can run, so we
+    // store this data for the eventual transition.
+    if (buffers_after_silence_.empty() || is_transition_pending_)
+      buffers_after_silence_.push_back(AudioBus::Create(params_));
+    dest = buffers_after_silence_.back().get();
+  } else if (!buffers_after_silence_.empty()) {
+    // Drain any non-silent transitional buffers before queuing more audio data.

[o1ka, 2016/09/26 12:21:55]

Maybe log here?

[DaleCurtis, 2016/09/26 19:45:58]

Don't want any logs on the render thread since they can be spammy.

+    // Note: These do not skew clocks derived from frame count since we don't
+    // issue Render() to the client when returning these buffers.
+    DCHECK(!is_using_fake_sink_);

[o1ka, 2016/09/26 12:21:55]

We may want to DCHECK it even if buffers_after_silence_.empty().
Probably just a general DCHECK_EQ(is_using_fake_sink, !dest) around l.50?
And make l.53 "if (is_using_fake_sink_)"?

[DaleCurtis, 2016/09/26 19:45:59]

I think the existing pair of DCHECKs covers the appropriate cases and reads
nicer to me.

+    buffers_after_silence_.front()->CopyTo(dest);
+    buffers_after_silence_.pop_front();
+    return dest->frames();
+  }
+
+  // Pass-through to client and request rendering.

[o1ka, 2016/09/26 12:21:55]

Could you add "In case of a fake sink we render to a last element of
|buffers_after_silence_|"? Though it follows from the code above, it would
simplify reading a bit.

[DaleCurtis, 2016/09/26 19:45:59]

I think that's covered by the comment in l.65 pretty well.

+  callback_->Render(dest, frames_delayed, frames_skipped);
+
+  // Check for silence or real audio data and transition if necessary.
+  if (!dest->AreFramesZero()) {
+    first_silence_time_ = base::TimeTicks();
+    if (is_using_fake_sink_) {
+      is_transition_pending_ = true;
+      task_runner_->PostTask(
+          FROM_HERE, base::Bind(sink_transition_callback_.callback(), false));
+      return dest->frames();
+    }
+  } else if (!is_using_fake_sink_) {
+    const base::TimeTicks now = base::TimeTicks::Now();
+    if (first_silence_time_.is_null())
+      first_silence_time_ = now;
+    if (now - first_silence_time_ > silence_timeout_) {
+      is_transition_pending_ = true;
+      task_runner_->PostTask(
+          FROM_HERE, base::Bind(sink_transition_callback_.callback(), true));
+    }
+  }
+
+  return dest->frames();
+}
+
+void SilentSinkSuspender::OnRenderError() {
+  callback_->OnRenderError();
+}
+
+void SilentSinkSuspender::TransitionSinks(bool use_fake_sink) {

[o1ka, 2016/09/26 12:21:55]

Maybe some logging? It could help to rule-out this code as a cause in "no audio"
cases.

[DaleCurtis, 2016/09/26 19:45:59]

Was going to add some LOG(INFO) calls, but it's actually blocked by the
presubmit and typically frowned upon. So didn't add these.

+  DCHECK(task_runner_->BelongsToCurrentThread());
+
+  // Ignore duplicate requests which can occur if the transition takes too long
+  // and multiple Render() events occur.
+  if (use_fake_sink && is_using_fake_sink_)
+    return;
+  if (!use_fake_sink && !is_using_fake_sink_)
+    return;

[o1ka, 2016/09/26 12:21:55]

the two can be (use_fake_sink == is_using_fake_sink_)
(looks more verbal to me)

[DaleCurtis, 2016/09/26 19:45:59]

Done.

+
+  if (use_fake_sink) {
+    sink_->Pause();
+
+    // |sink_| may still be executing Render() at this point or even sometime
+    // after this point, so we must acquire the lock to make sure we don't have
+    // concurrent Render() execution. Once |is_using_fake_sink_| is set to true,
+    // calls from |sink_| will be dropped.
+    {
+      base::AutoLock al(transition_lock_);
+      is_transition_pending_ = false;
+      is_using_fake_sink_ = true;
+    }
+    fake_sink_.Start(
+        base::Bind(base::IgnoreResult(&SilentSinkSuspender::Render),
+                   base::Unretained(this), nullptr, 0, 0));
+  } else {
+    fake_sink_.Stop();
+    // Stop() is synchronous for the FakeWorker, so no need to hold a lock here.

[o1ka, 2016/09/26 12:21:55]

When using a short timeout (for testing, for example, or if  SilentSinkSuspender
is used somewhere else), it may be that the real sink has not paused yet and its
Render() call is in progress (same may happen if there is some issue causing
sink pausing to be delayed). I think it's safer to hold a lock here, and it's
more future-prove as well. (And we gain almost nothing by not holding it)

[DaleCurtis, 2016/09/26 16:54:27]

What do you mean? FakeWorker::Stop() will try to grab a lock which prevents
concurrent execution and clear the internal callback so that no future call can
happen. I think the code is clearer without it. Otherwise I need to write a
comment like "hold the lock, just in case." :)

[DaleCurtis, 2016/09/26 19:45:58]

Done.

+    is_transition_pending_ = false;
+    is_using_fake_sink_ = false;
+    sink_->Play();
+  }
+}
+
+}  // namespace content