Add a mojo implementation of AudioOutputIPC.

content/renderer/media/mojo_audio_output_ipc.cc

Index: content/renderer/media/mojo_audio_output_ipc.cc
diff --git a/content/renderer/media/mojo_audio_output_ipc.cc b/content/renderer/media/mojo_audio_output_ipc.cc
new file mode 100644
index 0000000000000000000000000000000000000000..b4c1770ac11b454926b3b83eec08aa291296b148
--- /dev/null
+++ b/content/renderer/media/mojo_audio_output_ipc.cc
@@ -0,0 +1,227 @@
+// Copyright 2017 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/mojo_audio_output_ipc.h"
+
+#include <utility>
+
+#include "media/audio/audio_device_description.h"
+#include "mojo/public/cpp/system/platform_handle.h"
+
+namespace content {
+
+namespace {
+
+void TrivialAuthorizedCallback(media::OutputDeviceStatus,
+                               const media::AudioParameters&,
+                               const std::string&) {}
+
+}  // namespace
+
+// This class wraps a callback. If this class is destroyed without the callback
+// being called, it will call it with an error. This is needed since the
+// AudioOutputDevice could otherwise wait forever for device parameters in the
+// case of a connection error.

[o1ka, 2017/05/11 10:58:41]

Nice!

+class AuthorizationCallbackWrapper {
+ public:
+  using CallbackType = base::OnceCallback<void(media::OutputDeviceStatus,
+                                               const media::AudioParameters&,
+                                               const std::string&)>;
+
+  static CallbackType Wrap(CallbackType callback) {
+    return base::BindOnce(
+        AuthorizationCallbackWrapper::Call,
+        base::Passed(AuthorizationCallbackWrapper(std::move(callback))));
+  }
+
+  AuthorizationCallbackWrapper(AuthorizationCallbackWrapper&& other)
+      : callback_(std::move(other.callback_)) {
+    // It's not explicitly stated that moving from a OnceCallback resets the

[Ken Rockot(use gerrit already), 2017/05/08 16:04:24]

Seems like a documentation bug then. Moving a OnceCallback definitely resets
source callback. It's equivalent to moving the internal BindStateBase, which is
itself a RefCountedThreadSafe. Moving a RefCountedThreadSafe necessarily resets
the source object.

[Max Morin, 2017/05/11 15:31:17]

Leaving this after discussion on cxx.

+    // moved-from callback, so reset it here.
+    other.callback_ = CallbackType();
+  }
+
+  AuthorizationCallbackWrapper& operator=(
+      AuthorizationCallbackWrapper&& other) = delete;
+
+  ~AuthorizationCallbackWrapper() {
+    if (callback_) {
+      std::move(callback_).Run(
+          media::OutputDeviceStatus::OUTPUT_DEVICE_STATUS_ERROR_INTERNAL,
+          media::AudioParameters::UnavailableDeviceParams(), std::string());
+    }
+  }
+
+ private:
+  explicit AuthorizationCallbackWrapper(CallbackType callback)
+      : callback_(std::move(callback)) {}
+
+  static void Call(AuthorizationCallbackWrapper callback_wrapper,
+                   media::OutputDeviceStatus status,
+                   const media::AudioParameters& params,
+                   const std::string& device_id) {
+    if (callback_wrapper.callback_) {
+      std::move(callback_wrapper.callback_).Run(status, params, device_id);
+      // Make sure we don't call again in destructor:
+      callback_wrapper.callback_ = CallbackType();

[Ken Rockot(use gerrit already), 2017/05/08 16:04:24]

Also unnecessary.

[Max Morin, 2017/05/11 15:31:17]

Same.

+    }
+  }
+
+  CallbackType callback_;
+
+  DISALLOW_COPY_AND_ASSIGN(AuthorizationCallbackWrapper);
+};
+
+MojoAudioOutputIPC::MojoAudioOutputIPC(FactoryAccessor factory_accessor)
+    : factory_accessor_(std::move(factory_accessor)), weak_factory_(this) {
+  thread_checker_.DetachFromThread();
+}
+
+MojoAudioOutputIPC::~MojoAudioOutputIPC() {
+  // No thread check.
+  DCHECK(!AuthorizationRequested());
+  DCHECK(!StreamCreationRequested());
+  // Destructing |weak_factory_| on any thread is safe since it's not used after
+  // the final call to CloseStream, where its pointers are invalidated.

[o1ka, 2017/05/11 10:58:40]

What guarantees that CloseStream() has been called?

[Max Morin, 2017/05/11 15:31:18]

This is part of the AudioOutputIPC contract. It's not actually written in the
AudioOutputIPC definition, so I updated that.

+}
+
+media::mojom::AudioOutputStreamProviderRequest
+MojoAudioOutputIPC::MakeProviderRequest(
+    media::AudioOutputIPCDelegate* delegate) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(!AuthorizationRequested());
+  media::mojom::AudioOutputStreamProviderRequest request =
+      mojo::MakeRequest(&stream_provider_);
+
+  // Unretained is safe because |delegate| owns |this|.

[o1ka, 2017/05/11 10:58:41]

See the comment about storing a pointer to |delegate|.

[Max Morin, 2017/05/11 15:31:18]

Not sure what it has to do with this code?

+  stream_provider_.set_connection_error_handler(base::Bind(
+      &media::AudioOutputIPCDelegate::OnError, base::Unretained(delegate)));
+  return request;
+}
+
+void MojoAudioOutputIPC::RequestDeviceAuthorization(
+    media::AudioOutputIPCDelegate* delegate,
+    int session_id,
+    const std::string& device_id,
+    const url::Origin& security_origin) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(delegate);
+  DCHECK(!AuthorizationRequested());
+  DCHECK(!StreamCreationRequested());
+  auto* factory = factory_accessor_.Run();
+  if (!factory) {
+    LOG(ERROR) << "MojoAudioOutputIPC failed to acquire factory";
+    delegate->OnIPCClosed();  // deletes |this|.
+    return;
+  }
+
+  // We wrap the callback here so that we are sure to always get the
+  // authorization reply, even if the connection is closed.

[o1ka, 2017/05/11 10:58:41]

Looking forward to unit tests :)

[Max Morin, 2017/05/11 15:31:18]

:)

+  factory->RequestDeviceAuthorization(
+      MakeProviderRequest(delegate), session_id, device_id,
+      AuthorizationCallbackWrapper::Wrap(
+          base::Bind(&MojoAudioOutputIPC::RecievedDeviceAuthorization,
+                     weak_factory_.GetWeakPtr(), delegate)));
+}
+
+void MojoAudioOutputIPC::RecievedDeviceAuthorization(
+    media::AudioOutputIPCDelegate* delegate,

[o1ka, 2017/05/11 10:58:41]

What is the point of passing |delegate| around? Why not to have it as a member
which is initialized either on RequestDeviceAuthorization() or on
CreateStream()?

[Max Morin, 2017/05/11 15:31:17]

The AudioOutputIPC interface is written to get |delegate| whenever it might need
to call back, so it feels more natural to not store the delegate.

[o1ka, 2017/05/15 13:27:08]

Not sure why it is more natural. MojoAudioOutputIPC is bound to a |delegate|
passed to it in the first call the client makes. Storing a pointer to it is a
common indicator that |delegate| is expected to outlive the object.

"whenever" - There are only 2 cases whe it gets |delegate|: 
RequestDeviceAuthorization() and CreateStream().
In this implementation RequestDeviceAuthorization() calls
set_connection_error_handler(), passing |delegate| to it.
How the code verifies that subsequent call to Create() is made with the same
|delegate|?

[Max Morin, 2017/05/16 15:51:35]

I changed the class to store |delegate| so we can move on :).

+    media::OutputDeviceStatus status,
+    const media::AudioParameters& params,
+    const std::string& device_id) const {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  delegate->OnDeviceAuthorized(status, params, device_id);
+}
+
+void MojoAudioOutputIPC::CreateStream(media::AudioOutputIPCDelegate* delegate,

[o1ka, 2017/05/11 10:58:41]

Could you arrange implementations in the declaration order?

[Max Morin, 2017/05/11 15:31:18]

Done.

+                                      const media::AudioParameters& params) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(delegate);
+  DCHECK(!StreamCreationRequested());
+  if (!AuthorizationRequested()) {
+    // No authorization requested yet. Request one for the default device.
+    // This also means we shouldn't callback to the delegate for the reply.
+    auto* factory = factory_accessor_.Run();
+    if (!factory) {
+      LOG(ERROR) << "MojoAudioOutputIPC failed to acquire factory";
+      delegate->OnIPCClosed();  // deletes |this|.
+      return;
+    }
+
+    factory->RequestDeviceAuthorization(

[o1ka, 2017/05/11 10:58:40]

Combine duplicated code here and in RequestDeviceAuthorization()?

[Max Morin, 2017/05/11 15:31:17]

If I introduced a helper function, it would have to take lots of arguements, and
I'd still need a conditional for returning early, so I don't think it actually
saves complexity.

[o1ka, 2017/05/15 13:27:08]

Not sure what do you mean.
bool DoRequestDeviceAuthorization(delegate, session id, device id, callback)?

[Max Morin, 2017/05/16 15:51:35]

Done.

+        MakeProviderRequest(delegate), 0,
+        media::AudioDeviceDescription::kDefaultDeviceId,
+        base::Bind(&TrivialAuthorizedCallback));

[o1ka, 2017/05/11 10:58:40]

Comment why you don't process authorization response?

[Max Morin, 2017/05/11 15:31:17]

Done.

+  }
+
+  // Since the creation callback won't fire if the provider binding is gone,
+  // unretained is safe.

[o1ka, 2017/05/11 10:58:41]

Add a comment why we can't use Weak here?

[Max Morin, 2017/05/11 15:31:17]

We can, but we don't need to.

+  stream_provider_->Acquire(mojo::MakeRequest(&stream_), params,
+                            base::Bind(&MojoAudioOutputIPC::StreamCreated,
+                                       base::Unretained(this), delegate));
+
+  // Unretained is safe because |delegate| owns |this|.

[o1ka, 2017/05/11 10:58:40]

I'd prefer the term "outlives". The code changes quickly.
Also, if we store a pointer to |delegate| in the member(see the comment above),
it's more explicit.

[Max Morin, 2017/05/11 15:31:18]

Comment is bad, I changed it. See above for storing a pointer to |delegate| in a
member.

+  stream_.set_connection_error_handler(base::Bind(
+      &media::AudioOutputIPCDelegate::OnError, base::Unretained(delegate)));

[o1ka, 2017/05/11 10:58:40]

This is already done in MakeProviderRequest(), isn't it?

[Max Morin, 2017/05/11 15:31:18]

For the provider. This is for the stream.

[o1ka, 2017/05/15 13:27:08]

Acknowledged.

+}
+
+void MojoAudioOutputIPC::StreamCreated(
+    media::AudioOutputIPCDelegate* delegate,
+    mojo::ScopedSharedBufferHandle shared_memory,
+    mojo::ScopedHandle socket) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(socket.is_valid());
+  DCHECK(shared_memory.is_valid());

[o1ka, 2017/05/11 10:58:40]

CHECK those two?

[Max Morin, 2017/05/11 15:31:18]

There are no security implications of this code, since it's receiving IPC from a
more privileged process.

+
+  base::PlatformFile socket_handle;
+  mojo::UnwrapPlatformFile(std::move(socket), &socket_handle);

[o1ka, 2017/05/11 10:58:41]

CHECK the result?

[Max Morin, 2017/05/11 15:31:18]

same

[o1ka, 2017/05/15 13:27:08]

Why?

+
+  base::SharedMemoryHandle memory_handle;
+  bool read_only = false;
+  size_t memory_length = 0;
+  auto result = mojo::UnwrapSharedMemoryHandle(
+      std::move(shared_memory), &memory_handle, &memory_length, &read_only);
+  DCHECK_EQ(result, MOJO_RESULT_OK);

[o1ka, 2017/05/11 10:58:41]

CHECK?
Is it really safe to continue?

[Max Morin, 2017/05/11 15:31:18]

same

[o1ka, 2017/05/15 13:27:08]

Why? How about out of memory scenario?

+  DCHECK(!read_only);

[o1ka, 2017/05/11 10:58:41]

CHECK as well?

[Max Morin, 2017/05/11 15:31:18]

same

+
+  delegate->OnStreamCreated(memory_handle, socket_handle, memory_length);

[o1ka, 2017/05/11 10:58:40]

CHECK(memory_length) as well?

(make sure to have a security review for this code)

[Max Morin, 2017/05/11 15:31:18]

same

+}
+
+void MojoAudioOutputIPC::PlayStream() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  if (stream_.is_bound())
+    stream_->Play();
+}
+
+void MojoAudioOutputIPC::PauseStream() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  if (stream_.is_bound())
+    stream_->Pause();
+}
+
+void MojoAudioOutputIPC::CloseStream() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  stream_provider_.reset();
+  stream_.reset();
+
+  // Make sure we don't send an authorization callback for this stream to the
+  // delegate.
+  weak_factory_.InvalidateWeakPtrs();
+}
+
+void MojoAudioOutputIPC::SetVolume(double volume) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  if (stream_.is_bound())
+    stream_->SetVolume(volume);
+}
+
+bool MojoAudioOutputIPC::AuthorizationRequested() {
+  return stream_provider_.is_bound();
+}
+
+bool MojoAudioOutputIPC::StreamCreationRequested() {
+  return stream_.is_bound();
+}
+
+}  // namespace content