Switch stream creation and closing in Chrome audio rendering from IPC to Mojo

content/browser/media/audio_output_impl.cc

Index: content/browser/media/audio_output_impl.cc
diff --git a/content/browser/media/audio_output_impl.cc b/content/browser/media/audio_output_impl.cc
new file mode 100644
index 0000000000000000000000000000000000000000..d2b8623940bcecb48ae12d038c37a46fc98c5455
--- /dev/null
+++ b/content/browser/media/audio_output_impl.cc
@@ -0,0 +1,187 @@
+// 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.
+
+#if defined(OS_WIN)
+#include <windows.h>
+#endif
+
+#include "base/memory/shared_memory.h"
+#include "content/browser/media/audio_output_impl.h"
+#include "content/browser/media/audio_output_stream_impl.h"
+#include "content/public/browser/render_frame_host.h"
+#include "mojo/edk/embedder/embedder.h"
+#include "mojo/public/cpp/system/handle.h"
+#include "mojo/public/cpp/system/platform_handle.h"
+
+namespace content {
+
+namespace {
+base::SyncSocket::TransitDescriptor DuplicateSocket(

[nasko, 2016/05/25 20:50:41]

Empty line between namespace and the code.

[rchtara, 2016/05/27 15:24:38]

Done.

+    base::SyncSocket::TransitDescriptor socket_descriptor) {
+  base::SyncSocket::TransitDescriptor socket_descriptor_dup;
+
+#if defined(OS_WIN)
+  socket_descriptor_dup = 0;
+  if (!::DuplicateHandle(GetCurrentProcess(),  // hSourceProcessHandle
+                         socket_descriptor,
+                         GetCurrentProcess(),  // hTargetProcessHandle
+                         &socket_descriptor_dup,
+                         0,      // dwDesiredAccess ignored due to SAME_ACCESS
+                         FALSE,  // !bInheritHandle
+                         DUPLICATE_SAME_ACCESS)) {
+    LOG(ERROR) << "Unable to duplicate socket handle.";
+  }
+
+#else
+  socket_descriptor_dup.fd = dup(socket_descriptor.fd);
+#endif
+  return socket_descriptor_dup;
+}
+}

[nasko, 2016/05/25 20:50:41]

Empty line between the two } and a // namespace ending comment.

[rchtara, 2016/05/27 15:24:38]

Done.

+
+AudioOutputImpl::AudioOutputImpl(RenderProcessHost* process,
+                                 int render_frame_id,
+                                 media::mojom::AudioOutputRequest request)
+    : process_(process), render_frame_id_(render_frame_id), binding_(this) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  binding_.Bind(std::move(request));
+  binding_.set_connection_error_handler(base::Bind(
+      &AudioOutputImpl::Disconnect, process->GetID(), render_frame_id));
+}
+
+AudioOutputImpl::~AudioOutputImpl() {
+  Disconnect(render_frame_id_, render_frame_id_);
+  for (auto it = create_stream_callbacks_.begin();
+       it != create_stream_callbacks_.end(); ++it)

[nasko, 2016/05/25 20:50:42]

Multiline loops need to have {}.

[rchtara, 2016/05/27 15:24:38]

Done.

+    it->second.reset();
+  create_stream_callbacks_.clear();
+}
+
+// static
+void AudioOutputImpl::CreateService(RenderProcessHost* process,

[nasko, 2016/05/25 20:50:41]

The comment in the header was saying this object is created and lives on the IO
thread and no mention about UI thread. However, this method expects to be called
on the UI thread. Please clarify this in the header file.

[rchtara, 2016/05/27 15:24:38]

Done.

+                                    int render_frame_id,
+                                    media::mojom::AudioOutputRequest request) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      base::Bind(&AudioOutputImpl::CreateServiceOnIOThread, process,
+                 render_frame_id, base::Passed(&request)));
+}
+
+// static
+void AudioOutputImpl::CreateServiceOnIOThread(
+    RenderProcessHost* process,
+    int render_frame_id,
+    media::mojom::AudioOutputRequest request) {
+  if (request.is_pending()) {

[nasko, 2016/05/25 20:50:41]

Is it safe to call this on other threads if the request isn't pending?

[rchtara, 2016/05/27 15:24:38]

Done.

+    DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  }
+
+  new AudioOutputImpl(process, render_frame_id, std::move(request));
+}
+
+void AudioOutputImpl::CreateStream(int stream_id,
+                                   const media::AudioParameters& params,
+                                   const CreateStreamCallback& callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  create_stream_callbacks_.insert(std::make_pair(stream_id, callback));
+  process_->audio_renderer_host()->CreateStream(stream_id, render_frame_id_,
+                                                params, this);
+}
+
+void AudioOutputImpl::CreateStreamFactory(
+    int stream_id,
+    base::SharedMemory* shared_memory,
+    base::SyncSocket::TransitDescriptor socket_descriptor) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  auto callback = create_stream_callbacks_.find(stream_id);
+  if (callback == create_stream_callbacks_.end()) {

[nasko, 2016/05/25 20:50:41]

Single line if statements don't need {}.

[rchtara, 2016/05/27 15:24:38]

Done.

+    return;
+  }
+
+  media::mojom::AudioOutputStreamPtr stream_ptr =
+      media::mojom::AudioOutputStreamPtr();
+  std::unique_ptr<AudioOutputStreamImpl> stream(new AudioOutputStreamImpl(
+      stream_id, render_frame_id_, this, mojo::GetProxy(&stream_ptr)));
+
+  DCHECK(stream_impls_.insert(std::make_pair(stream_id, std::move(stream)))

[nasko, 2016/05/25 20:50:41]

This statement will never execute in release build. You need to move the call
outside of the DCHECK, capture the return value, then DCHECK the return value.

[rchtara, 2016/05/27 15:24:38]

Done.

+             .second);
+
+  base::SharedMemoryHandle shared_memory_handle =
+      base::SharedMemory::DuplicateHandle(shared_memory->handle());
+  mojo::ScopedSharedBufferHandle shared_buffer_handle =
+      mojo::WrapSharedMemoryHandle(shared_memory_handle,

[nasko, 2016/05/25 20:50:41]

Would WrapSharedMemoryHandle work successfully if the DuplicateHandle call
failed? There is no check for error condition.

[rchtara, 2016/05/27 15:24:38]

Done.

+                                   shared_memory->requested_size(), false);
+
+  // The socket handle is going to be closed when |mojo_application_host_| is
+  // reset. The renderer is going to need to close the socket handle

[nasko, 2016/05/25 20:50:41]

nit: s/renderer/renderer process/

[rchtara, 2016/05/27 15:24:38]

Done.

+  // too when the output steam is closed. This is going to cause an issue
+  // because both of them are going to close the same socket handle.
+  // This is why a duplicate of the socket handler is going to be created,
+  // stored in |socket_descriptor_dup| and sent through Mojo to the renderer so
+  // it can be used by AudioOutputIPCDelegate.
+
+  base::SyncSocket::TransitDescriptor socket_descriptor_dup =
+      DuplicateSocket(socket_descriptor);
+#if defined(OS_WIN)
+  mojo::ScopedHandle socket_handle =
+      mojo::WrapPlatformFil(socket_descriptor_dup);
+#else
+  mojo::ScopedHandle socket_handle =
+      mojo::WrapPlatformFile(socket_descriptor_dup.fd);
+#endif
+  callback->second.Run(stream_id, std::move(stream_ptr),

[nasko, 2016/05/25 20:50:41]

If there is always a pointer to the stream, why is the mojom file annotated as
optional? Same for the other two params.

[rchtara, 2016/05/27 15:24:38]

Done.

[nasko, 2016/05/27 22:52:00]

What does "Done" mean here? I don't see a change to the mojom file, nor to this
code. How was the initial question addressed as done?

+                       std::move(shared_buffer_handle),
+                       std::move(socket_handle));
+
+  callback->second.reset();
+  create_stream_callbacks_.erase(stream_id);
+}
+
+bool AudioOutputImpl::CloseStream(int stream_id) {
+  process_->audio_renderer_host()->CloseStream(stream_id);
+
+  auto stream = stream_impls_.find(stream_id);
+  if (stream == stream_impls_.end()) {
+    return false;
+  }
+  stream_impls_.erase(stream_id);
+
+  auto callback = create_stream_callbacks_.find(stream_id);
+
+  if (callback != create_stream_callbacks_.end()) {
+    callback->second.reset();
+    create_stream_callbacks_.erase(stream_id);
+  }
+  return true;
+}
+
+// static
+void AudioOutputImpl::Disconnect(int process_id, int render_frame_id) {}
+
+void AudioOutputImpl::ReportErrorAndCloseStream(int stream_id) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  // Make sure this isn't a stray callback executing after the stream has been
+  // closed, so error notifications aren't sent after clients believe the stream
+  // is closed.
+  auto callback = create_stream_callbacks_.find(stream_id);
+
+  mojo::ScopedSharedBufferHandle shared_buffer_handle =
+      mojo::ScopedSharedBufferHandle(mojo::SharedBufferHandle());
+
+  mojo::ScopedHandle socket_handle = mojo::ScopedHandle(mojo::Handle());
+
+  callback->second.Run(stream_id, media::mojom::AudioOutputStreamPtr(),
+                       std::move(shared_buffer_handle),
+                       std::move(socket_handle));
+
+  process_->audio_renderer_host()->get_audio_log()->OnError(stream_id);
+  CloseStream(stream_id);
+}
+
+}  // namespace content