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..a00070700be099f99ddf4ab3061158ada32b1303
--- /dev/null
+++ b/content/browser/media/audio_output_impl.cc
@@ -0,0 +1,90 @@
+// 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 "content/browser/media/audio_output_stream_impl.h"
+
+namespace content {
+
+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));
+  LOG(ERROR) << "AudioOutputImpl";

[tommi (sloooow) - chröme, 2016/05/20 13:26:12]

?

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

Done.

+  factory_ = base::Bind(AudioOutputStreamFactory::Factory);

[Henrik Grunell, 2016/05/20 13:35:39]

I suppose |factory_| is needed for being modified in tests. Instead, pass the
factory in the constructor.

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

it's removed: not needed for the new tests

+}
+
+AudioOutputImpl::~AudioOutputImpl() {
+  LOG(ERROR) << "~AudioOutputImpl";

[tommi (sloooow) - chröme, 2016/05/20 13:26:12]

please remove all of these

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

Done.

+}
+
+// static
+void AudioOutputImpl::CreateService(
+    RenderProcessHost* process,
+    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()) {
+    DCHECK_CURRENTLY_ON(BrowserThread::IO);

[tommi (sloooow) - chröme, 2016/05/20 13:26:12]

why would we only be on the io thread if the request is pending? (seems like
there might be a bug here)

[Henrik Grunell, 2016/05/20 13:35:39]

Why can it be called on another thread if not pending? Should be just the
DCHECK?

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

Done.

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

Done.

+  }
+
+  auto service = new AudioOutputImpl(process, render_frame_id,
+                                     std::move(request));
+
+  process->audio_renderer_host()->set_audio_output_impl(render_frame_id, service);

[tommi (sloooow) - chröme, 2016/05/20 13:26:12]

please run git cl format

[Henrik Grunell, 2016/05/20 13:35:39]

This is what we talked about offline, right? It won't work since this mojo
service when existing per frame will be created before the AudioRendererHost.

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

yes. exactly 

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

Done.

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

Done.

+}
+
+void AudioOutputImpl::CreateStream(int stream_id,
+                                   const media::AudioParameters& params,
+                                   const CreateStreamCallback& callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  media::mojom::AudioOutputStreamPtr stream;
+  process_->audio_renderer_host()->CreateStream(stream_id, render_frame_id_,
+      params, callback);
+}
+
+media::mojom::AudioOutputStreamPtr AudioOutputImpl::StreamFactory(

[Henrik Grunell, 2016/05/20 13:35:39]

It seems to be unnecessarily complicated to call the ARH above, which then calls
back here to create the stream. Is it possible to move all the code here to ARH?
Or perhaps create the AOSI in CreateStream() and pass it to ARH directly?

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

Yes, but dale wants to removes code from the ARH. And doing so will make things
even more complicated 

+    int stream_id,
+    int render_frame_id,
+    AudioRendererHost* audio_renderer_host) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  media::mojom::AudioOutputStreamPtr stream =
+      media::mojom::AudioOutputStreamPtr();
+  std::unique_ptr<AudioOutputStreamImpl> stream_ptr(
+      factory_.Run(mojo::GetProxy(&stream), stream_id, render_frame_id,
+                   audio_renderer_host));
+
+  auto ret =
+      stream_impls_.insert(std::make_pair(stream_id, std::move(stream_ptr)));
+  DCHECK(ret.second);

[tommi (sloooow) - chröme, 2016/05/20 13:26:12]

do you get a build error for official builds?  (ret becomes an unused variable)

+  return stream;
+}
+
+bool AudioOutputImpl::RemoveStream(int stream_id) {
+  LOG(ERROR) << "rm stream_id" << stream_id;
+  if (stream_impls_.erase(stream_id))
+    return true;
+  else
+    return false;

[tommi (sloooow) - chröme, 2016/05/20 13:26:12]

please just do:

return stream_impls_.erase(stream_id) != 0;

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

Done.

+}
+
+}  // namespace content