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

content/browser/media/audio_output_impl.h

+// 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.
+
+#ifndef CONTENT_BROWSER_MEDIA_AUDIO_OUTPUT_IMPL_H_
+#define CONTENT_BROWSER_MEDIA_AUDIO_OUTPUT_IMPL_H_
+
+#include "base/callback.h"
+#include "content/browser/renderer_host/media/audio_renderer_host.h"
+#include "media/mojo/interfaces/audio_output.mojom.h"
+#include "mojo/public/cpp/bindings/interface_request.h"
+#include "mojo/public/cpp/bindings/strong_binding.h"
+
+namespace content {
+class AudioOutputStreamImpl;
+
+// This class implements AudioOutput Mojo interface. It will be used
+// by the AudioOutputClient to perform operations on audio output streams.

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

This isn't really correct. AudioOutputClient doesn't "use" this class as in
calls functions on it. You could instead write that it's connected to the
AudioOutputClient.

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

Done.

+// AudioOutputClient is going to perform operations remotely and Mojo will
+// transfer them to AudioOutputImpl.
+// This class must always be accessed from the creation thread.
+class CONTENT_EXPORT AudioOutputImpl
+    : NON_EXPORTED_BASE(public media::mojom::AudioOutput) {
+ public:
+  explicit AudioOutputImpl(RenderProcessHost* process_,

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

No trailing _

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

Done.

+                           int render_frame_id,
+                           media::mojom::AudioOutputRequest request);
+
+  ~AudioOutputImpl() override;
+
+  // Creates a Mojo service.
+  static void CreateService(
+      RenderProcessHost* process_,
+      int render_frame_id,
+      media::mojom::AudioOutputRequest request);
+  // Initialized Mojo service on the IO thread. The service is bound to the

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

add an empty line above this one.

Should the first sentence be "Returns an initialized ..."?

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

Empty line before comment.

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

Done.

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

It doesn't return anything. it just initialize service 

+  // IO thread so future Mojo calls performed by the AudioOutputClient to the
+  // AudioOutput interface will be run on that thread.
+  // There are a lot of dependencies used in audio output that require to be
+  // called on the IO thread, this is why we initialize Mojo service on this
+  // thread. There is a plan to enable these dependencies to run on another

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

I don't think there is a plan for doing that here on the browser side. On the
render side it does. So, you should remove that part of the comment.

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

Done.

+  // thread and move the Mojo service to this thread as IO thread is very
+  // performance sensitive and we want to move stuff off it.
+  static void CreateServiceOnIOThread(
+      RenderProcessHost* process_,

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

process (no underscore for params)

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

Done.

+      int render_frame_id,
+      media::mojom::AudioOutputRequest request);

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

passed by value on purpose?

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

yes, it's going to be initialized in RenderFrameHostImpl::RegisterMojoServices()
like this:
 GetServiceRegistry()->AddService(
       base::Bind(&AudioOutputImpl::CreateService,
                  process_, routing_id_));
So we have to give it request by value otherwise it doesn't work

+
+  // Called by AudioRendererHost::DoCompleteCreation to create the streams.
+  virtual media::mojom::AudioOutputStreamPtr StreamFactory(
+      int stream_id,
+      int render_frame_id,
+      AudioRendererHost* audio_renderer_host);
+
+  virtual bool RemoveStream(int stream_id);
+
+ private:
+  friend class AudioOutputImplTest;
+  friend class MockAudioOutputImpl;
+  FRIEND_TEST_ALL_PREFIXES(AudioOutputImplTest, CreateStream);
+  FRIEND_TEST_ALL_PREFIXES(AudioOutputImplTest, StreamFactory);
+  FRIEND_TEST_ALL_PREFIXES(AudioOutputImplTest, RemoveStream);
+  // AudioOutput interface implementation.

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

Empty line before comment.

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

Done.

+  void CreateStream(int stream_id,
+                    const media::AudioParameters& params,
+                    const CreateStreamCallback& callback) override;
+
+  RenderProcessHost* process_;
+  int render_frame_id_;
+  mojo::Binding<AudioOutput> binding_;
+  std::map<int, std::unique_ptr<AudioOutputStreamImpl>> stream_impls_;

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

can you add information about this map? (maps from ? to AudioOutputStreamImpl)

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

Comment on why this is needed. It's just for ownership and deleting afaict.

Would it make sense to pass the ownership to the AudioRendererHost?

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

Done.

+  base::Callback<AudioOutputStreamImpl*(
+      media::mojom::AudioOutputStreamRequest request,
+      int stream_id,
+      int renderer_frame_id,
+      AudioRendererHost* audio_renderer_host)>

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

can you create a typedef for this?

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

Done.

[Henrik Grunell, 2016/05/30 12:44:56]

Or preferable "using".

+      factory_;

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

Indentation seems odd. Run git cl format.

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

Done.

+  DISALLOW_COPY_AND_ASSIGN(AudioOutputImpl);

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

Empty line before.

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

Done.

+};
+
+}  // namespace content
+
+#endif  // CONTENT_BROWSER_MEDIA_AUDIO_OUTPUT_IMPL_H_