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

content/renderer/media/audio_output_client.cc

Index: content/renderer/media/audio_output_client.cc
diff --git a/content/renderer/media/audio_output_client.cc b/content/renderer/media/audio_output_client.cc
new file mode 100644
index 0000000000000000000000000000000000000000..da00ae36e5983acddcd63c4679b84424e3daf9d1
--- /dev/null
+++ b/content/renderer/media/audio_output_client.cc
@@ -0,0 +1,176 @@
+// 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 "content/renderer/media/audio_output_client.h"
+
+#include <utility>
+
+#include "base/files/file.h"
+#include "base/message_loop/message_loop.h"
+#include "base/single_thread_task_runner.h"
+#include "base/time/time.h"
+#include "content/common/media/audio_messages.h"
+#include "content/public/common/service_registry.h"
+#include "content/renderer/media/audio_message_filter.h"
+#include "content/renderer/media/webrtc_logging.h"
+#include "media/audio/audio_parameters.h"
+#include "media/mojo/common/media_type_converters.h"
+#include "mojo/edk/embedder/embedder.h"
+#include "mojo/public/c/system/buffer.h"
+#include "mojo/public/cpp/bindings/binding.h"
+#include "mojo/public/cpp/system/handle.h"
+
+namespace content {
+
+namespace {
+
+int FindAudioOutputStreamPtrID(
+    const AudioOutputClient::ScopedAudioOutputStreamPtrMap& stream_ids,
+    media::interfaces::AudioOutputStreamPtr* const key) {
+  for (auto& it : stream_ids) {
+    if (it.second.get() == key) {

[mcasas, 2016/04/29 18:29:01]

No {} for one-line bodies

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

Done.

+      return it.first;
+    }
+  }

[mcasas, 2016/04/29 18:29:00]

Nit: Consider std::find_if()

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

Tried to do that:

bool f(const std::pair<int,
                   std::unique_ptr<media::mojom::AudioOutputStreamPtr>> x) {
  return true;
}
auto it = std::find_if(stream_ids.begin(), stream_ids.end(), f);
But failed as I was calling a delete constructor of
'std::unique_ptr<mojo::InterfacePtr<media::mojom::AudioOutputStream>,
std::default_delete<mojo::InterfacePtr<media::mojom::AudioOutputStream> > >'
Tried to do it with lambda, failed too. 

+  return -1;

[mcasas, 2016/04/29 18:29:00]

Consider making |kStreamIDNotSet| not 
public (and/or rename to |kStreamIDNotValid|)
and using it here.

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

Done.

+}
+
+}  // namespace
+
+AudioOutputClient::AudioOutputClient(
+    ServiceRegistry* service_registry,
+    scoped_refptr<AudioMessageFilter> audio_message_filter)
+    : audio_message_filter_(audio_message_filter),
+      main_thread_task_runner_(base::MessageLoop::current()->task_runner()) {
+  if (service_registry) {
+    service_registry->ConnectToRemoteService(mojo::GetProxy(&service_));
+    service_.set_connection_error_handler(base::Bind(
+        &AudioOutputClient::OnConnectionError, base::Unretained(this)));

[mcasas, 2016/04/29 18:29:01]

base::Unretained should be used sparingly.
Since here you just want to get a DLOG(ERROR),
move OnConnectionError() to the anonymous namespace 
in l.26, and make it a file-static function.

+  } else {
+    service_ = 0;

[mcasas, 2016/04/29 18:29:01]

No need to do this (and is semantically wrong),
instead check for service_.is_bound() in l.82.

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

Done.

+  }
+}
+
+AudioOutputClient::~AudioOutputClient() {}
+
+void AudioOutputClient::OnConnectionError() {
+  LOG(ERROR) << "Mojo client connection error";

[mcasas, 2016/04/29 18:29:01]

s/LOG/DLOG/
(I think there's a presubmit warning for this).

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

Done.

+}
+
+void AudioOutputClient::OnStreamError(int stream_id) {
+  audio_message_filter_->io_task_runner()->PostTask(
+      FROM_HERE, base::Bind(&AudioOutputClient::ReportErrorOnIOThread,
+                            base::Unretained(this), stream_id));

[mcasas, 2016/04/29 18:29:01]

base::Unretained() Should not be used. If this
class is genuinely multi-threaded, consider 
making it ref-counted, but more often than not,
a WeakPtrFactory is a better solution, although
it poses restrictions on where the destructor is
run.

http://www.chromium.org/developers/coding-style/important-abstractions-and-da...

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

I failed to use WeakPtrFactory. So I will made this refcouned

+}
+
+void AudioOutputClient::CreateStream(int stream_id,
+                                     int render_frame_id,
+                                     const media::AudioParameters& params) {
+  main_thread_task_runner_->PostTask(

[mcasas, 2016/04/29 18:29:01]

Consider merging with CreateStreamOnMainThread():

if (!main_thread_task_runner_->BelongsToCurrentThread()) {
  // PostTask...
  return;
}
DCHECK(main_thread_task_runner_->BelongsToCurrentThread());
if (!service)  // Should be if (!service.is_bound())
  return;
service->CreateStream(...)

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

Done.

+      FROM_HERE,
+      base::Bind(&AudioOutputClient::CreateStreamOnMainThread,
+                 base::Unretained(this), stream_id, render_frame_id, params));
+}
+
+void AudioOutputClient::CreateStreamOnMainThread(
+    int stream_id,
+    int render_frame_id,
+    const media::AudioParameters& params) {
+  media::AudioParameters param;
+  DCHECK(main_thread_task_runner_->BelongsToCurrentThread());
+  if (service_) {
+    service_->CreateStream(
+        stream_id, render_frame_id,
+        mojo::ConvertTo<media::interfaces::AudioOutputStreamParametersPtr>(
+            params),
+        base::Bind(&AudioOutputClient::CreateStreamCallback,
+                   base::Unretained(this)));
+  }
+}
+
+void AudioOutputClient::CreateStreamCallback(
+    int stream_id,
+    media::interfaces::AudioOutputStreamPtr stream,
+    mojo::ScopedSharedBufferHandle shared_buffer,
+    mojo::ScopedHandle socket_descriptor) {
+  DCHECK(main_thread_task_runner_->BelongsToCurrentThread());
+
+  if (!stream.is_bound()) {
+    audio_message_filter_->io_task_runner()->PostTask(
+        FROM_HERE, base::Bind(&AudioOutputClient::ReportErrorOnIOThread,
+                              base::Unretained(this), stream_id));
+    return;
+  }
+
+  stream.set_connection_error_handler(base::Bind(
+      &AudioOutputClient::OnStreamError, base::Unretained(this), stream_id));
+
+  base::SharedMemoryHandle shared_memory_handle;
+  size_t length;
+
+  MojoResult pass_shared_memory_result = mojo::edk::PassSharedMemoryHandle(
+      shared_buffer.release().value(), &shared_memory_handle, &length, nullptr);
+
+  if (pass_shared_memory_result != MOJO_RESULT_OK) {
+    LOG(ERROR) << "Failed to pass shared memory. Closing: "

[mcasas, 2016/04/29 18:29:00]

s/LOG/DLOG/

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

Done.

+               << pass_shared_memory_result;
+    return;
+  }
+
+  mojo::edk::ScopedPlatformHandle platform_handle;
+
+  MojoResult pass_platform_handle_result = mojo::edk::PassWrappedPlatformHandle(
+      socket_descriptor.release().value(), &platform_handle);
+
+  if (pass_platform_handle_result != MOJO_RESULT_OK) {
+    LOG(ERROR) << "Failed to pass transit descriptor. Closing: "
+               << pass_platform_handle_result;
+    return;
+  }
+
+  base::SyncSocket::TransitDescriptor descriptor;
+
+#if defined(OS_WIN)
+  descriptor = platform_handle.release().handle;
+#else
+  descriptor.fd = platform_handle.release().handle;
+#endif
+  streams_[stream_id] = make_scoped_ptr(
+      new media::interfaces::AudioOutputStreamPtr(std::move(stream)));
+
+  audio_message_filter_->io_task_runner()->PostTask(
+      FROM_HERE, base::Bind(&AudioOutputClient::CreateStreamOnIOThread,
+                            base::Unretained(this), streams_[stream_id].get(),
+                            shared_memory_handle, descriptor, length));
+}
+
+void AudioOutputClient::CreateStreamOnIOThread(
+    media::interfaces::AudioOutputStreamPtr* const stream,
+    base::SharedMemoryHandle handle,
+    base::SyncSocket::TransitDescriptor socket_descriptor,
+    uint32_t length) {
+  DCHECK(audio_message_filter_->io_task_runner()->BelongsToCurrentThread());
+  audio_message_filter_->OnStreamCreated(
+      FindAudioOutputStreamPtrID(streams_, stream), handle, socket_descriptor,
+      length);
+}
+
+void AudioOutputClient::CloseStream(int stream_id) {
+  main_thread_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&AudioOutputClient::CloseStreamOnMainThread,
+                            base::Unretained(this), stream_id));
+}
+
+void AudioOutputClient::CloseStreamOnMainThread(int stream_id) {
+  DCHECK(main_thread_task_runner_->BelongsToCurrentThread());
+  streams_[stream_id]->get()->Close();

[mcasas, 2016/04/29 18:29:01]

Remove get()->, a unique_ptr behaves like a
pointer.

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

Done.

+}
+
+void AudioOutputClient::ReportErrorOnIOThread(int stream_id) {
+  DCHECK(audio_message_filter_->io_task_runner()->BelongsToCurrentThread());
+  audio_message_filter_->OnStreamStateChanged(
+      stream_id, media::AUDIO_OUTPUT_IPC_DELEGATE_STATE_ERROR);
+}
+
+}  // namespace content