content/browser/media/audio_output_impl.h - Issue 1896883002: Mojo interfaces needed for switching audio rendering stream creation and closing from IPC to Mojo

Side by Side Diff: content/browser/media/audio_output_impl.h

Issue 1896883002: Mojo interfaces needed for switching audio rendering stream creation and closing from IPC to Mojo (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master

Patch Set: All grunell comments resolved Created 4 years, 8 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View unified diff | Download patch

OLD	NEW
(Empty)
	1 // Copyright 2016 The Chromium Authors. All rights reserved.

	2 // Use of this source code is governed by a BSD-style license that can be

	3 // found in the LICENSE file.

	4

	5 #ifndef CONTENT_BROWSER_MEDIA_AUDIO_OUTPUT_IMPL_H_

	6 #define CONTENT_BROWSER_MEDIA_AUDIO_OUTPUT_IMPL_H_

	7

	8 #include "base/callback.h"

	9 #include "base/compiler_specific.h"

	10 #include "base/memory/ref_counted.h"

	11 #include "content/browser/renderer_host/media/audio_renderer_host.h"

	12 #include "content/common/content_export.h"

	13 #include "media/mojo/interfaces/audio_output.mojom.h"

	14 #include "mojo/public/cpp/bindings/interface_request.h"

	15 #include "mojo/public/cpp/bindings/strong_binding.h"

	16

	17 namespace base {

	18 class FilePath;

	19 }

	20

	21 namespace content {

	22

	23 class CONTENT_EXPORT AudioOutputStreamImpl : public mojom::AudioOutputStream {

	24 // All the instance of this class are going to be owned by AudioOutputImpl.
	Henrik Grunell 2016/04/22 08:23:03 Put comment before the "class" line. Put comment before the "class" line. Henrik Grunell 2016/04/22 08:23:04 Add a brief general description of the class. Then Add a brief general description of the class. Then you can just have "Owned by AudioOutputImpl." after that. rchtara 2016/04/29 12:54:44 Done. Show quoted text On 2016/04/22 08:23:03, Henrik Grunell wrote: > Put comment before the "class" line. Done. rchtara 2016/04/29 12:54:46 Done. Show quoted text On 2016/04/22 08:23:03, Henrik Grunell wrote: > Put comment before the "class" line. Done.
	25 public:

	26 explicit AudioOutputStreamImpl(
	xhwang 2016/04/26 22:54:42 no explicit no explicit rchtara 2016/04/29 12:54:45 Done. Show quoted text On 2016/04/26 22:54:42, xhwang wrote: > no explicit Done.
	27 mojo::InterfaceRequest<mojom::AudioOutputStream> request,
	tommi (sloooow) - chröme 2016/04/26 15:29:53 Niklas - can you recommend someone to remove this Niklas - can you recommend someone to remove this CL from the mojo perspective?
	28 int stream_id,

	29 AudioRendererHost* audio_renderer_host);
	tommi (sloooow) - chröme 2016/04/26 15:29:53 document ownership/lifetime of raw pointers document ownership/lifetime of raw pointers rchtara 2016/04/29 12:54:45 I have done that while describing \|audio_renderer_ Show quoted text On 2016/04/26 15:29:53, tommi-chrömium wrote: > document ownership/lifetime of raw pointers I have done that while describing \|audio_renderer_host_\|
	30

	31 // AudioOutputStream interface implementation.

	32 void Close() override;
	tommi (sloooow) - chröme 2016/04/26 15:29:53 make this private? (assuming callers of this metho make this private? (assuming callers of this method will have an interface pointer and not a pointer to this implementation. rchtara 2016/04/29 12:54:45 Done. Show quoted text On 2016/04/26 15:29:53, tommi-chrömium wrote: > make this private? (assuming callers of this method will have an interface > pointer and not a pointer to this implementation. Done.
	33

	34 ~AudioOutputStreamImpl() override;
	tommi (sloooow) - chröme 2016/04/26 15:29:52 dtor follows ctor(s) in class layout dtor follows ctor(s) in class layout rchtara 2016/04/29 12:54:45 Done. Show quoted text On 2016/04/26 15:29:52, tommi-chrömium wrote: > dtor follows ctor(s) in class layout Done.
	35

	36 private:

	37 mojo::StrongBinding<mojom::AudioOutputStream> binding_;

	38 int stream_id_;

	39 // \|audio_renderer_host_\| is scoped_refptr in AudioOutputImpl. And because all
	Henrik Grunell 2016/04/22 08:23:03 You could put it as: "AudioRendererHost is ref cou You could put it as: "AudioRendererHost is ref counted and AudioOutputImpl holds a reference to it." rchtara 2016/04/29 12:54:45 Done. Show quoted text On 2016/04/22 08:23:03, Henrik Grunell wrote: > You could put it as: > "AudioRendererHost is ref counted and AudioOutputImpl holds a reference to it." Done.
	40 // AudioOutputStreamImpl instances are going to be owned by AudioOutputImpl
	Henrik Grunell 2016/04/22 08:23:03 "going to be" means that it will happen in the fut "going to be" means that it will happen in the future. Maybe something like "AudioOutputStreamImpl is owned by AudioOutputImpl". rchtara 2016/04/29 12:54:46 Done. Show quoted text On 2016/04/22 08:23:03, Henrik Grunell wrote: > "going to be" means that it will happen in the future. > > Maybe something like "AudioOutputStreamImpl is owned by AudioOutputImpl". Done.
	41 // too, it's safe to access \|audio_renderer_host\| from AudioOutputStreamImpl.

	42 // The only issue that could happen is during AudioOutputStreamImpl

	43 // destructor. \|audio_renderer_host\| cannot access from there.

	44 AudioRendererHost* audio_renderer_host_;

	45 };

	46

	47 // This class must always be accessed from the creation thread.

	48 class CONTENT_EXPORT AudioOutputImpl

	49 : NON_EXPORTED_BASE(public mojom::AudioOutput) {

	50 public:

	51 AudioOutputImpl(mojom::AudioOutputRequest request);
	xhwang 2016/04/26 22:54:42 explicit explicit rchtara 2016/04/29 12:54:45 Done. Show quoted text On 2016/04/26 22:54:42, xhwang wrote: > explicit Done.
	52

	53 ~AudioOutputImpl() override;

	54

	55 // Create a Mojo service.

	56 static void CreateService(

	57 scoped_refptr<AudioRendererHost> audio_renderer_host,

	58 mojom::AudioOutputRequest request);
	tommi (sloooow) - chröme 2016/04/26 15:29:52 passed by value on purpose? passed by value on purpose? rchtara 2016/04/29 12:54:45 yes it's a mojo::InterfaceRequest<mojom::AudioOutp Show quoted text On 2016/04/26 15:29:52, tommi-chrömium wrote: > passed by value on purpose? yes it's a mojo::InterfaceRequest<mojom::AudioOutput> which needs to be handle as if it's a smart pointer. If I start using references, the compiler will give me a lot of error.
	59

	60 // Initialize Mojo on the BrowserThread::IO. Mojo is going to be bound to
	Henrik Grunell 2016/04/22 08:23:03 Does it create or only initialize? Both the commen Does it create or only initialize? Both the comment and function name should reflect what it does. "Mojo" -> "Mojo service" "BrowserThread::IO" -> "IO thread" "Mojo is going to be bound" -> "The service is bound" Rephrase/clarify "future Mojo calls". (What is a "mojo call"?) rchtara 2016/04/29 12:54:45 Done. Show quoted text On 2016/04/22 08:23:03, Henrik Grunell wrote: > Does it create or only initialize? Both the comment and function name should > reflect what it does. > > "Mojo" -> "Mojo service" > > "BrowserThread::IO" -> "IO thread" > > "Mojo is going to be bound" -> "The service is bound" > > Rephrase/clarify "future Mojo calls". (What is a "mojo call"?) Done.
	61 // BrowserThread::IO so future Mojo calls are going to be run on

	62 // that thread.
	xhwang 2016/04/26 22:54:41 Could you please explain more why this service has Could you please explain more why this service has to run on the IO thread? Typically IO thread is very performance sensitive and we want to move stuff off the IO thread. rchtara 2016/04/29 12:54:45 Done. Show quoted text On 2016/04/26 22:54:41, xhwang wrote: > Could you please explain more why this service has to run on the IO thread? > Typically IO thread is very performance sensitive and we want to move stuff off > the IO thread. Done.
	63 static void CreateServiceOnIOThread(

	64 scoped_refptr<AudioRendererHost> audio_renderer_host,

	65 mojo::InterfaceRequest<mojom::AudioOutput> request);
	xhwang 2016/04/26 22:54:42 You can use mojom::AudioOutputRequest now I believ You can use mojom::AudioOutputRequest now I believe. rchtara 2016/04/29 12:54:45 Done. Show quoted text On 2016/04/26 22:54:42, xhwang wrote: > You can use mojom::AudioOutputRequest now I believe. Done.
	66

	67 // Called by AudioRendererHost::DoCompleteCreation to create the streams.

	68 mojom::AudioOutputStreamPtr* StreamFactory(

	69 int stream_id,

	70 AudioRendererHost* audio_renderer_host);

	71

	72 private:

	73
	tommi (sloooow) - chröme 2016/04/26 15:29:52 remove this empty line remove this empty line rchtara 2016/04/29 12:54:45 Done. Show quoted text On 2016/04/26 15:29:52, tommi-chrömium wrote: > remove this empty line Done.
	74 // AudioOutput interface implementation.

	75 void CreateStream(int stream_id,

	76 int render_frame_id,

	77 media::interfaces::AudioOutputStreamParametersPtr params,
	tommi (sloooow) - chröme 2016/04/26 15:29:52 intentionally by value? intentionally by value? rchtara 2016/04/29 12:54:45 AudioOutputStreamParametersPtr is just a smart poi Show quoted text On 2016/04/26 15:29:52, tommi-chrömium wrote: > intentionally by value? AudioOutputStreamParametersPtr is just a smart pointer: InterfacePtr https://code.google.com/p/chromium/codesearch#chromium/src/mojo/public/cpp/bi... This method is going to be called by the mojo client and this is how it should be called.
	78 const CreateStreamCallback& callback) override;

	79

	80 // Mojo connection error callback.

	81 void OnDisconnect(AudioOutputImpl* impl);

	82

	83 scoped_refptr<AudioRendererHost> audio_renderer_host_;

	84 mojo::StrongBinding<AudioOutput> binding_;

	85 std::map<int, std::unique_ptr<AudioOutputStreamImpl>> stream_impls_;
	tommi (sloooow) - chröme 2016/04/26 15:29:53 since there's no lock in this class, can we use a since there's no lock in this class, can we use a thread checker to ensure that all method calls execute on expected threads? rchtara 2016/04/29 12:54:45 Done. Show quoted text On 2016/04/26 15:29:53, tommi-chrömium wrote: > since there's no lock in this class, can we use a thread checker to ensure that > all method calls execute on expected threads? Done.
	86 DISALLOW_COPY_AND_ASSIGN(AudioOutputImpl);

	87 };

	88

	89 } // namespace content

	90

	91 #endif // CONTENT_BROWSER_MEDIA_AUDIO_OUTPUT_IMPL_H_

OLD	NEW

« content/browser/BUILD.gn ('K') | « content/browser/BUILD.gn ('k') | content/browser/media/audio_output_impl.cc » ('j') | content/browser/media/audio_output_impl.cc » ('J')