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

content/browser/renderer_host/media/audio_renderer_host.cc

Index: content/browser/renderer_host/media/audio_renderer_host.cc
diff --git a/content/browser/renderer_host/media/audio_renderer_host.cc b/content/browser/renderer_host/media/audio_renderer_host.cc
index ae580222c519763e30ebe1715654343a94c4bea8..fab9b712584b780890390ce55a773923143d8a1d 100644
--- a/content/browser/renderer_host/media/audio_renderer_host.cc
+++ b/content/browser/renderer_host/media/audio_renderer_host.cc
@@ -6,6 +6,9 @@
 
 #include <stdint.h>
 #include <utility>
+#if defined(OS_WIN)
+#include <windows.h>
+#endif
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
@@ -17,6 +20,7 @@
 #include "content/browser/bad_message.h"
 #include "content/browser/browser_main_loop.h"
 #include "content/browser/child_process_security_policy_impl.h"
+#include "content/browser/media/audio_output_impl.h"
 #include "content/browser/media/audio_stream_monitor.h"
 #include "content/browser/media/capture/audio_mirroring_manager.h"
 #include "content/browser/media/media_internals.h"
@@ -36,6 +40,8 @@
 #include "media/audio/audio_streams_tracker.h"
 #include "media/base/audio_bus.h"
 #include "media/base/limits.h"
+#include "mojo/edk/embedder/embedder.h"
+#include "mojo/public/cpp/system/handle.h"
 
 using media::AudioBus;
 using media::AudioManager;
@@ -61,6 +67,28 @@ GURL ConvertToGURL(const url::Origin& origin) {
   return origin.unique() ? GURL() : GURL(origin.Serialize());
 }
 
+base::SyncSocket::TransitDescriptor DuplicateSocket(
+    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

[Henrik Grunell, 2016/05/02 12:12:23]

Two spaces after "0," instead of aligning with the below comment.

[rchtara, 2016/05/23 16:38:17]

Done.

+                         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;
+}
+
 bool IsValidDeviceId(const std::string& device_id) {
   static const std::string::size_type kValidLength = 64;
 
@@ -120,13 +148,15 @@ void MaybeFixAudioParameters(media::AudioParameters* params) {
 class AudioRendererHost::AudioEntry
     : public media::AudioOutputController::EventHandler {
  public:
-  AudioEntry(AudioRendererHost* host,
-             int stream_id,
-             int render_frame_id,
-             const media::AudioParameters& params,
-             const std::string& output_device_id,
-             std::unique_ptr<base::SharedMemory> shared_memory,
-             std::unique_ptr<media::AudioOutputController::SyncReader> reader);
+  AudioEntry(
+      AudioRendererHost* host,
+      int stream_id,
+      int render_frame_id,
+      const media::AudioParameters& params,
+      const std::string& output_device_id,
+      std::unique_ptr<base::SharedMemory> shared_memory,
+      std::unique_ptr<media::AudioOutputController::SyncReader> reader,
+      const media::interfaces::AudioOutput::CreateStreamCallback& callback);
   ~AudioEntry() override;
 
   int stream_id() const {
@@ -171,6 +201,9 @@ class AudioRendererHost::AudioEntry
   const scoped_refptr<media::AudioOutputController> controller_;
 
   bool playing_;
+
+  // Callback for media::interfaces::AudioOutput::CreateStream.
+  media::interfaces::AudioOutput::CreateStreamCallback create_stream_callback_;
 };
 
 AudioRendererHost::AudioEntry::AudioEntry(
@@ -180,7 +213,8 @@ AudioRendererHost::AudioEntry::AudioEntry(
     const media::AudioParameters& params,
     const std::string& output_device_id,
     std::unique_ptr<base::SharedMemory> shared_memory,
-    std::unique_ptr<media::AudioOutputController::SyncReader> reader)
+    std::unique_ptr<media::AudioOutputController::SyncReader> reader,
+    const media::interfaces::AudioOutput::CreateStreamCallback& callback)
     : host_(host),
       stream_id_(stream_id),
       render_frame_id_(render_frame_id),
@@ -192,6 +226,7 @@ AudioRendererHost::AudioEntry::AudioEntry(
                                                        output_device_id,
                                                        reader_.get())),
       playing_(false) {
+  create_stream_callback_ = callback;

[Henrik Grunell, 2016/05/02 12:12:23]

Put it in the initialization list?

[rchtara, 2016/05/23 16:38:17]

Done.

   DCHECK(controller_.get());
 }
 
@@ -264,9 +299,9 @@ void AudioRendererHost::OnDestruct() const {
 
 void AudioRendererHost::AudioEntry::OnCreated() {
   BrowserThread::PostTask(
-      BrowserThread::IO,
-      FROM_HERE,
-      base::Bind(&AudioRendererHost::DoCompleteCreation, host_, stream_id_));
+      BrowserThread::IO, FROM_HERE,
+      base::Bind(&AudioRendererHost::DoCompleteCreation, host_, stream_id_,

[Henrik Grunell, 2016/05/02 12:12:23]

Could |callback_| be used again? If not, what about clearing it after the
PostTask and a DCHECK before to make sure it's valid?

[rchtara, 2016/05/23 16:38:17]

Done.

+                 create_stream_callback_));
 }
 
 void AudioRendererHost::AudioEntry::OnPlaying() {
@@ -296,29 +331,39 @@ void AudioRendererHost::AudioEntry::OnError() {
       base::Bind(&AudioRendererHost::ReportErrorAndClose, host_, stream_id_));
 }
 
-void AudioRendererHost::DoCompleteCreation(int stream_id) {
+void AudioRendererHost::DoCompleteCreation(
+    int stream_id,
+    const media::interfaces::AudioOutput::CreateStreamCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   if (!PeerHandle()) {
     DLOG(WARNING) << "Renderer process handle is invalid.";
-    ReportErrorAndClose(stream_id);
+    ReportErrorAndCloseStream(stream_id, callback);
     return;
   }
 
   AudioEntry* const entry = LookupById(stream_id);
   if (!entry) {
-    ReportErrorAndClose(stream_id);
+    ReportErrorAndCloseStream(stream_id, callback);
     return;
   }
 
-  // Once the audio stream is created then complete the creation process by
-  // mapping shared memory and sharing with the renderer process.
-  base::SharedMemoryHandle foreign_memory_handle;
-  if (!entry->shared_memory()->ShareToProcess(PeerHandle(),
-                                              &foreign_memory_handle)) {
-    // If we failed to map and share the shared memory then close the audio
-    // stream and send an error message.
-    ReportErrorAndClose(entry->stream_id());
+  media::interfaces::AudioOutputStreamPtr stream_ptr =
+      (audio_output_impl_->StreamFactory(stream_id, this));
+
+  base::SharedMemoryHandle shared_memory_handle =
+      base::SharedMemory::DuplicateHandle(entry->shared_memory()->handle());
+
+  MojoHandle mojo_foreign_memory_handle;
+
+  MojoResult shared_buffer_result = mojo::edk::CreateSharedBufferWrapper(
+      shared_memory_handle, entry->shared_memory()->requested_size(), false,
+      &mojo_foreign_memory_handle);
+
+  if (shared_buffer_result != MOJO_RESULT_OK) {
+    DLOG(WARNING) << "Failed to wrap transit descriptor. Closing: "
+                  << shared_buffer_result;
+    ReportErrorAndCloseStream(entry->stream_id(), callback);
     return;
   }
 
@@ -329,13 +374,49 @@ void AudioRendererHost::DoCompleteCreation(int stream_id) {
   // If we failed to prepare the sync socket for the renderer then we fail
   // the construction of audio stream.
   if (!reader->PrepareForeignSocket(PeerHandle(), &socket_descriptor)) {
-    ReportErrorAndClose(entry->stream_id());
+    ReportErrorAndCloseStream(entry->stream_id(), callback);
+    return;
+  }
+  mojo::ScopedSharedBufferHandle shared_buffer_handle =
+      mojo::ScopedSharedBufferHandle(
+          mojo::SharedBufferHandle(mojo_foreign_memory_handle));
+
+  MojoHandle socket_descriptor_handle;
+  MojoResult platform_handle_result;
+
+  // The socket handle is going to be closed when |mojo_application_host_| is
+  // reset. AudioOutputIPCDelegate is going to need to close the socket handle

[Henrik Grunell, 2016/05/02 12:12:23]

AudioOutputIPCDelegate is an implementation detail on the client side, so don't
mention it here.

It sounds like the socket handle ownership should be transferred to the client.
Why is it closed in |mojo_application_host_| and can we avoid that?

[rchtara, 2016/05/23 16:38:17]

Mojo close it automatically, but not sure if it's possible to transfer the
ownership from Mojo to the renderer

+  // too when the steam it controls 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)
+  platform_handle_result = mojo::edk::CreatePlatformHandleWrapper(
+      mojo::edk::ScopedPlatformHandle(
+          mojo::edk::PlatformHandle(socket_descriptor_dup),
+      &socket_descriptor_handle);
+#else
+  platform_handle_result = mojo::edk::CreatePlatformHandleWrapper(
+      mojo::edk::ScopedPlatformHandle(
+          mojo::edk::PlatformHandle(socket_descriptor_dup.fd)),
+      &socket_descriptor_handle);
+#endif
+
+  if (platform_handle_result != MOJO_RESULT_OK) {
+    DLOG(WARNING) << "Failed to wrap platform handle. Closing: "
+                  << platform_handle_result;
+    ReportErrorAndCloseStream(stream_id, callback);
     return;
   }
 
-  Send(new AudioMsg_NotifyStreamCreated(
-      entry->stream_id(), foreign_memory_handle, socket_descriptor,
-      entry->shared_memory()->requested_size()));
+  mojo::ScopedHandle socket_handle =
+      mojo::ScopedHandle(mojo::Handle(socket_descriptor_handle));
+  callback.Run(stream_id, std::move(stream_ptr),
+               std::move(shared_buffer_handle), std::move(socket_handle));
 }
 
 void AudioRendererHost::DoNotifyStreamStateChanged(int stream_id,
@@ -386,7 +467,6 @@ bool AudioRendererHost::OnMessageReceived(const IPC::Message& message) {
   IPC_BEGIN_MESSAGE_MAP(AudioRendererHost, message)
     IPC_MESSAGE_HANDLER(AudioHostMsg_RequestDeviceAuthorization,
                         OnRequestDeviceAuthorization)
-    IPC_MESSAGE_HANDLER(AudioHostMsg_CreateStream, OnCreateStream)
     IPC_MESSAGE_HANDLER(AudioHostMsg_PlayStream, OnPlayStream)
     IPC_MESSAGE_HANDLER(AudioHostMsg_PauseStream, OnPauseStream)
     IPC_MESSAGE_HANDLER(AudioHostMsg_CloseStream, OnCloseStream)
@@ -525,30 +605,35 @@ void AudioRendererHost::OnDeviceIDTranslated(
       stream_id, media::OUTPUT_DEVICE_STATUS_OK, output_params, std::string()));
 }
 
-void AudioRendererHost::OnCreateStream(int stream_id,
-                                       int render_frame_id,
-                                       const media::AudioParameters& params) {
+void AudioRendererHost::CreateStream(
+    int stream_id,
+    int render_frame_id,
+    const media::AudioParameters& params,
+    const media::interfaces::AudioOutput::CreateStreamCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  DVLOG(1) << "AudioRendererHost@" << this << "::OnCreateStream"
+  DVLOG(1) << "AudioRendererHost@" << this << "::CreateStream"
            << "(stream_id=" << stream_id << ")";
 
   const auto& auth_data = authorizations_.find(stream_id);
 
   // If no previous authorization requested, assume default device
   if (auth_data == authorizations_.end()) {
-    DoCreateStream(stream_id, render_frame_id, params, std::string());
+    DoCreateStream(stream_id, render_frame_id, params, std::string(), callback);
     return;
   }
 
   CHECK(auth_data->second.first);
-  DoCreateStream(stream_id, render_frame_id, params, auth_data->second.second);
+  DoCreateStream(stream_id, render_frame_id, params, auth_data->second.second,
+                 callback);
   authorizations_.erase(auth_data);
 }
 
-void AudioRendererHost::DoCreateStream(int stream_id,
-                                       int render_frame_id,
-                                       const media::AudioParameters& params,
-                                       const std::string& device_unique_id) {
+void AudioRendererHost::DoCreateStream(
+    int stream_id,
+    int render_frame_id,
+    const media::AudioParameters& params,
+    const std::string& device_unique_id,
+    const media::interfaces::AudioOutput::CreateStreamCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   // media::AudioParameters is validated in the deserializer.
@@ -580,7 +665,7 @@ void AudioRendererHost::DoCreateStream(int stream_id,
 
   std::unique_ptr<AudioEntry> entry(
       new AudioEntry(this, stream_id, render_frame_id, params, device_unique_id,
-                     std::move(shared_memory), std::move(reader)));
+                     std::move(shared_memory), std::move(reader), callback));
   if (mirroring_manager_) {
     mirroring_manager_->AddDiverter(
         render_process_id_, entry->render_frame_id(), entry->controller());
@@ -693,6 +778,28 @@ void AudioRendererHost::ReportErrorAndClose(int stream_id) {
   OnCloseStream(stream_id);
 }
 
+void AudioRendererHost::ReportErrorAndCloseStream(
+    int stream_id,
+    const media::interfaces::AudioOutput::CreateStreamCallback& callback) {
+  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.
+  if (!LookupById(stream_id))
+    return;
+  mojo::ScopedSharedBufferHandle shared_buffer_handle =

[Henrik Grunell, 2016/05/02 12:12:22]

Nit: add empty line before this.

[rchtara, 2016/05/23 16:38:17]

Done.

+      mojo::ScopedSharedBufferHandle(mojo::SharedBufferHandle());
+
+  mojo::ScopedHandle socket_handle = mojo::ScopedHandle(mojo::Handle());
+
+  callback.Run(stream_id, media::interfaces::AudioOutputStreamPtr(),
+               std::move(shared_buffer_handle), std::move(socket_handle));
+
+  audio_log_->OnError(stream_id);
+  OnCloseStream(stream_id);
+}
+
 AudioRendererHost::AudioEntry* AudioRendererHost::LookupById(int stream_id) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);