Add an initial revision of an audio server.

services/media/audio/audio_output.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 "base/bind.h"
+#include "base/location.h"
+#include "base/logging.h"
+
+#include "services/media/audio/audio_output.h"
+#include "services/media/audio/audio_output_manager.h"
+#include "services/media/audio/audio_track_to_output_link.h"
+
+namespace mojo {
+namespace media {
+namespace audio {
+
+// Function used in a base::Closure to defer the final part of a shutdown task
+// to the main event loop.
+static void FinishShutdownSelf(AudioOutputManager* manager,
+                               AudioOutputWeakPtr weak_output) {
+  auto output  = weak_output.lock();
+  if (output) {
+    manager->ShutdownOutput(output);
+  }
+}
+
+AudioOutput::AudioOutput(AudioOutputManager* manager)
+  : manager_(manager) {
+  DCHECK(manager_);
+}
+
+AudioOutput::~AudioOutput() {
+  DCHECK(!task_runner_ && shutting_down_);
+}
+
+MediaResult AudioOutput::AddTrackLink(AudioTrackToOutputLinkPtr link) {
+  MediaResult res = InitializeLink(link);
+
+  if (res == MediaResult::OK) {
+    base::AutoLock lock(processing_lock_);
+
+    // Assert that we are the output in this link.
+    DCHECK(this == link->GetOutput().get());

[jeffbrown, 2015/11/04 23:43:32]

Not sure I understand why this has to happen inside the lock given that the
InitializeLink method manipulates the link outside of the lock.

Would it make sense for the initialization to happen on the worker thread too? 
We could say that everything to do with processing and link management is
effectively asynchronous.  That would make it easier to understand what state
gets manipulated where.  (Does processing touch the link?)

[johngro, 2015/11/06 02:20:24]

Link initialization happens outside of the scope of the processing lock in the
context of the message loop thread.  For a base AudioOutput, this is a no-op. 
For derived classes, it is an opportunity to perform tasks involving
initialization of link specific state (such as selecting mixer kernels and
initialization of filter state) without holding up the output's mixing tasks in
any way.

As described in earlier feedback, the processing lock serves two purposes.  In
this context, it serving to protect the links_ collection which must be held
static during a processing callback.

As mentioned earlier, the scope of this protection could be further limited, but
would require the introduction of a separate lock which would need to be
acquired and released in the more limited scope by the derived class.

Re: Does processing touch the link?
Yes, but only after the link has been added to the set of active links.  This is
why initialization and teardown happen outside of the scope of the processing
lock, and only when we can prove that the processing scope can not be holding
any references to the link object itself.

+
+    if (shutting_down_) {
+      return MediaResult::SHUTTING_DOWN;
+    }
+
+    auto insert_result = links_.emplace(link);
+    DCHECK(insert_result.second);
+  } else {
+    // TODO(johngro): Output didn't like this track for some reason...  Should
+    // probably log something about this.
+  }
+
+  return res;
+}
+
+MediaResult AudioOutput::RemoveTrackLink(
+    const AudioTrackToOutputLinkPtr& link) {
+  base::AutoLock lock(processing_lock_);
+
+  if (shutting_down_) {
+    return MediaResult::SHUTTING_DOWN;
+  }
+
+  auto iter = links_.find(link);
+  if (iter == links_.end()) {
+    return MediaResult::NOT_FOUND;
+  }
+
+  links_.erase(iter);
+  return MediaResult::OK;
+}
+
+MediaResult AudioOutput::Init() {
+  return MediaResult::OK;
+}
+
+void AudioOutput::Cleanup() {
+}
+
+MediaResult AudioOutput::InitializeLink(const AudioTrackToOutputLinkPtr& link) {
+  DCHECK(link);
+  return MediaResult::OK;
+}
+
+void AudioOutput::ScheduleCallback(LocalTime when) {
+  base::AutoLock lock(shutdown_lock_);
+
+  // If we are in the process of shutting down, then we are no longer permitted
+  // to schedule callbacks.

[jeffbrown, 2015/11/04 23:43:32]

There's a race condition here given that shutdown may be initiated both by the
processing function and by the output manager.  When the output manager
initiates it, it's possible the processing function is still running and may try
to schedule another callback.

This is one reason why it might be a good idea for shutdown to be more directed.

[johngro, 2015/11/06 02:20:24]

No, I do not think that there is.  I took care to ensure that there was not. 
Please refer to my extensive response in the header file feed back to see why
there is no race here.

If you disagree with my analysis (I cannot bring myself to refer to it as a
"proof") given there, please provide me the hypothetical sequence of actions
which would lead to the thread-safety violation.

+  if (shutting_down_) {
+    DCHECK(!task_runner_);
+    return;
+  }
+  DCHECK(task_runner_);
+
+  // TODO(johngro):  Someday, if there is ever a way to schedule delayed tasks
+  // with absolute time, or with resolution better than microseconds, do so.
+  // Until then figure out the relative time for scheduling the task and do so.
+  LocalTime now = LocalClock::now();
+  base::TimeDelta sched_time = (now > when)
+    ? base::TimeDelta::FromMicroseconds(0)
+    : base::TimeDelta::FromMicroseconds(
+        local_time::to_usec<int64_t>(when - now));
+
+  task_runner_->PostNonNestableDelayedTask(
+      FROM_HERE,
+      base::Bind(&ProcessThunk, weak_self_),
+      sched_time);
+}
+
+void AudioOutput::ShutdownSelf() {
+  // If we are not already in the process of shutting down, send a message to
+  // the main message loop telling it to complete the shutdown process.
+  if (!BeginShutdown()) {
+    DCHECK(manager_);
+    manager_->ScheduleMessageLoopTask(
+        FROM_HERE,
+        base::Bind(&FinishShutdownSelf, manager_, weak_self_));
+  }
+}
+
+void AudioOutput::ProcessThunk(AudioOutputWeakPtr weak_output) {
+  // If we are still around by the time this callback fires, enter the procesing
+  // lock and dispatch to our derived class's implementation.
+  auto output  = weak_output.lock();
+  if (output) {
+    base::AutoLock lock(output->processing_lock_);
+    output->Process();
+  }
+}
+
+MediaResult AudioOutput::Init(
+    const AudioOutputPtr& self,
+    scoped_refptr<base::SequencedTaskRunner> task_runner) {
+  DCHECK(this == self.get());
+  DCHECK(task_runner);
+
+  // If our derived class failed to initialize, don't bother to hold onto the
+  // state we will need drive our callback engine.  Begin the process of
+  // shutting ourselves down, the output manager will eventually finish the job
+  // for us.
+  MediaResult res = Init();
+  if (res != MediaResult::OK) {
+    ShutdownSelf();
+    return res;
+  }
+
+  // Stash our callback state and schedule an immediate callback to get things
+  // running.
+  task_runner_ = task_runner;
+  weak_self_   = self;
+  task_runner_->PostNonNestableTask(FROM_HERE,
+                                    base::Bind(&ProcessThunk, weak_self_));
+
+  return MediaResult::OK;
+}
+
+bool AudioOutput::BeginShutdown() {
+  // Start the process of shutting down if we have not already.  This method may
+  // be called from either a processing context, or from the audio output
+  // manager.  After it finishes, any pending processing callbacks will have
+  // been nerfed, although there may still be callbacks in flight.
+  base::AutoLock lock(shutdown_lock_);
+
+  if (shutting_down_) { return true; }
+
+  shutting_down_ = true;
+  task_runner_   = nullptr;
+
+  return false;
+}
+
+void AudioOutput::Shutdown() {
+  if (shut_down_) { return; }
+
+  // TODO(johngro): Assert that we are running on the audio server's main
+  // message loop thread.
+
+  // Make sure no new callbacks can be generated, and that pending callbacks
+  // have been nerfed.
+  BeginShutdown();
+
+  // Synchronize with any callbacks in flight.  By acquiring and releasing the
+  // processing lock, we are guaranteed that we have no callbacks which are in
+  // the middle of processing, and that any pending callbacks will be nerfed.
+  // It is safe to destroy this audio output at any point in time after this.
+  processing_lock_.Acquire();
+  processing_lock_.Release();

[jeffbrown, 2015/11/04 23:43:32]

Obviously this blocks the main thread but I have to wonder whether that's
strictly necessary.

If the processing state is fully decoupled from the output object itself then
there might not be any harm in letting it run to completion and kill itself
asynchronously.  We would only need to update the bookkeeping here.

This assumes there isn't any other shared state to worry about (and it might be
better if there weren't).

[johngro, 2015/11/06 02:20:24]

I believe that I have already address this in responses given previously. 
Processing state cannot be completely decoupled from the output object as it is
a function of the track/output relationship.  Please review my earlier analysis.
 If you still disagree, lets get together and work this out face to face; for
complex interactions like this, I feel that coming to a mutual understanding is
more efficiently done in the context of an interactive dialog instead of via
email/code-review with its high latency.

+
+  // Unlink ourselves from all of our tracks.  Then go ahead and clear the track
+  // set.
+  for (const auto& link : links_) {
+    DCHECK(link);
+    AudioTrackImplPtr track = link->GetTrack();
+    if (track) {
+      track->RemoveOutput(link);
+    }
+  }
+  links_.clear();
+
+  // Give our derived class a chance to clean up its resources.
+  Cleanup();
+
+  // We are now completely shut down.  The only reason we have this flag is to
+  // make sure that Shutdown is idempotent.
+  shut_down_ = true;
+}
+
+}  // namespace audio
+}  // namespace media
+}  // namespace mojo
+