Mojo interfaces needed for switching audio rendering stream creation and closing 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..694562ec0f1551fbdb80bbb58ccccfddb6a16773
--- /dev/null
+++ b/content/browser/media/audio_output_impl.cc
@@ -0,0 +1,94 @@
+// 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/browser/media/audio_output_impl.h"
+#include "media/mojo/common/media_type_converters.h"
+
+namespace content {
+
+AudioOutputStreamImpl::AudioOutputStreamImpl(
+    mojo::InterfaceRequest<AudioOutputStream> request,
+    int stream_id,
+    AudioRendererHost* audio_renderer_host)
+    : binding_(this, std::move(request)),
+      stream_id_(stream_id),
+      audio_renderer_host_(audio_renderer_host) {
+}
+
+AudioOutputStreamImpl::~AudioOutputStreamImpl() {
+}
+
+void AudioOutputStreamImpl::Close() {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  audio_renderer_host_->OnCloseStream(stream_id_);
+}
+
+AudioOutputImpl::AudioOutputImpl(mojom::AudioOutputRequest request)
+    : binding_(this) {
+  binding_.Bind(std::move(request));
+
+  binding_.set_connection_error_handler(base::Bind(
+      base::Bind(&AudioOutputImpl::OnDisconnect, base::Unretained(this)),
+      base::Unretained(this)));

[tommi (sloooow) - chröme, 2016/04/26 15:29:52]

using Unretained() always raises an alarm. Is it guaranteed that this pointer
will never be dereferenced after the object has been deleted?  How is that done?
(can you add a comment for why this is safe?)

[rchtara, 2016/04/29 12:54:44]

I will not use AudioOutputImpl::OnDisconnect. So I will remove it.

+}
+
+AudioOutputImpl::~AudioOutputImpl() {}
+
+// static
+void AudioOutputImpl::CreateService(
+      scoped_refptr<AudioRendererHost> audio_renderer_host,
+      mojom::AudioOutputRequest request) {
+

[tommi (sloooow) - chröme, 2016/04/26 15:29:52]

remove whitespace

[rchtara, 2016/04/29 12:54:44]

Done.

+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      base::Bind(&AudioOutputImpl::CreateServiceOnIOThread, audio_renderer_host,
+                 base::Passed(&request)));
+}
+
+void AudioOutputImpl::CreateServiceOnIOThread(
+      scoped_refptr<AudioRendererHost> audio_renderer_host,
+      mojo::InterfaceRequest<AudioOutput> request) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  auto service = new AudioOutputImpl(std::move(request));
+  service->audio_renderer_host_ = audio_renderer_host;
+  audio_renderer_host->audio_output_impl_ = service;
+}
+
+// static
+void AudioOutputImpl::CreateStream(
+    int stream_id,
+    int render_frame_id,
+    media::interfaces::AudioOutputStreamParametersPtr params,
+    const CreateStreamCallback& callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  mojom::AudioOutputStreamPtr stream;
+  media::AudioParameters audio_params =
+      mojo::ConvertTo<media::AudioParameters>(params);
+
+  audio_renderer_host_->CreateStream(stream_id, render_frame_id, audio_params,
+                                       callback);
+}
+
+mojom::AudioOutputStreamPtr* AudioOutputImpl::StreamFactory(
+    int stream_id,
+    AudioRendererHost* audio_renderer_host) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  mojom::AudioOutputStreamPtr* stream = new mojom::AudioOutputStreamPtr();
+  std::unique_ptr<AudioOutputStreamImpl> stream_ptr(new AudioOutputStreamImpl(
+      mojo::GetProxy(stream), stream_id, audio_renderer_host));
+
+  stream_impls_[stream_id] = std::move(stream_ptr);

[tommi (sloooow) - chröme, 2016/04/26 15:29:52]

use insert() instead?  This will do a lookup+insert.  If the stream_id is not
expected to exist in the map, you can DCHECK the return value from insert() for
correctness.

[rchtara, 2016/04/29 12:54:44]

Done.

+  return stream;

[tommi (sloooow) - chröme, 2016/04/26 15:29:52]

returning a pointer to an object that's owned by the map, could be risky.  How
is it safe to return this pointer and be sure that the object is not going to be
deleted while in use?

[tommi (sloooow) - chröme, 2016/04/26 15:29:52]

this returns a "Ptr*".  is that a **?  Maybe the "Ptr" part is a bit misleading.

[rchtara, 2016/04/29 12:54:44]

Because the map is changed in this function only.
The map is going to be deleted only when the AudioOutputImpl is destructed
(which could happens when we have a connection error or when the current message
loop is destructed)
The mojom::AudioOutputStreamPtr is going to be only iuused to call the callback
and if in this 
Later in hte rendrer when the AudioOutputStreamPtr is used to perform operations
on AudioOutputStream, 

[rchtara, 2016/04/29 12:54:44]

Done.

+}
+
+void AudioOutputImpl::OnDisconnect(AudioOutputImpl* impl) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  DLOG(ERROR) << "Mojo client connection error";

[tommi (sloooow) - chröme, 2016/04/26 15:29:52]

LOG?  even fatal or dfatal?

[rchtara, 2016/04/29 12:54:44]

This could be caused by an error in mojo connection. But most  of the time it's
just going to be called when the current message loop is destructed.
But as we don't need it (I used just for debugging), I will remove it.

+}
+
+}  // namespace content