Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10537121:
Adds AudioDevice factory for all audio clients in Chrome (Closed)
|
Created:
8 years, 6 months ago by henrika (OOO until Aug 14) Modified:
8 years, 5 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, feature-media-reviews_chromium.org, jochen+watch-content_chromium.org, darin-cc_chromium.org, tommi (sloooow) - chröme Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionThis CL adds a new factory method called AudioDeviceFactory. It allows the user to create a media::AudioRenderSink implementation (AudioDevice) using a factory.
We can now mock (or try alternative implementations of) AudioDevice for all clients at one centralized place.
BUG=none
TEST=content_unittests, misc. WebRTC demos, WebAudio demos and <audio> tag demos.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=144888
Patch Set 1 #Patch Set 2 : Adds support for unit tests #Patch Set 3 : Renaming #Patch Set 4 : EOL conversions #Patch Set 5 : nits #Patch Set 6 : Improved comments and rebased #Patch Set 7 : Fixed build errors on Linux and Mac #Patch Set 8 : Moved factory to content/renderer/media #
Total comments: 4
Patch Set 9 : Changes based on review by jochen@ #Patch Set 10 : Removed DVLOGs #
Total comments: 8
Patch Set 11 : Removed usage of template to simplify #Patch Set 12 : Refactored based on input from all reviewers #Patch Set 13 : Minor cleanup #
Total comments: 10
Patch Set 14 : Changes based on review by Chris #
Total comments: 17
Patch Set 15 : Moved AudioDeviceFactory to content namespace #
Total comments: 18
Patch Set 16 : Changes based on review by John #Patch Set 17 : Minor fix before landing #Patch Set 18 : rebased #Messages
Total messages: 65 (0 generated)
Hi guys, I would appreciate your feedback on this one. The goal is to make it easier to test our stuff and at the same time make components more independent.
One more comment. I also removed: AudioDevice(const media::AudioParameters& params, RenderCallback* callback) om this CL, since it felt like a very odd contract to be able to create two different AudioDevices; one which had to be initialized and one that did not have to be initialized. In existing version, one must always call: ctor + Init and the exact time when Init is called depends on when the required parameters exists.
http://codereview.chromium.org/10537121/diff/8005/content/renderer/renderer_w... File content/renderer/renderer_webkitplatformsupport_impl.cc (right): http://codereview.chromium.org/10537121/diff/8005/content/renderer/renderer_w... content/renderer/renderer_webkitplatformsupport_impl.cc:646: return NULL; nit. indent http://codereview.chromium.org/10537121/diff/8005/content/renderer/renderer_w... content/renderer/renderer_webkitplatformsupport_impl.cc:647: RenderViewImpl* render_view = nit. less indent
http://codereview.chromium.org/10537121/diff/8005/content/renderer/renderer_w... File content/renderer/renderer_webkitplatformsupport_impl.cc (right): http://codereview.chromium.org/10537121/diff/8005/content/renderer/renderer_w... content/renderer/renderer_webkitplatformsupport_impl.cc:646: return NULL; On 2012/06/14 13:32:35, jochen wrote: > nit. indent Done. http://codereview.chromium.org/10537121/diff/8005/content/renderer/renderer_w... content/renderer/renderer_webkitplatformsupport_impl.cc:647: RenderViewImpl* render_view = On 2012/06/14 13:32:35, jochen wrote: > nit. less indent Done.
Antoine, seems like I need you as well on the few parts in content/renderer/ (can only find two owners in content/renderer). Care to take a look?
http://codereview.chromium.org/10537121/diff/4011/content/renderer/renderer_w... File content/renderer/renderer_webkitplatformsupport_impl.cc (right): http://codereview.chromium.org/10537121/diff/4011/content/renderer/renderer_w... content/renderer/renderer_webkitplatformsupport_impl.cc:644: WebFrame* web_frame = WebFrame::frameForCurrentContext(); This sounds like this method needs to be a member of WebViewClient (or WebFrameClient, as appropriate), and implemented by RenderViewImpl.
Thanks for your comments Antoine. http://codereview.chromium.org/10537121/diff/4011/content/renderer/renderer_w... File content/renderer/renderer_webkitplatformsupport_impl.cc (right): http://codereview.chromium.org/10537121/diff/4011/content/renderer/renderer_w... content/renderer/renderer_webkitplatformsupport_impl.cc:644: WebFrame* web_frame = WebFrame::frameForCurrentContext(); I can try to refactor if you think it is required. I have used the same approach as already exists in other methods here.
http://codereview.chromium.org/10537121/diff/4011/content/renderer/renderer_w... File content/renderer/renderer_webkitplatformsupport_impl.cc (right): http://codereview.chromium.org/10537121/diff/4011/content/renderer/renderer_w... content/renderer/renderer_webkitplatformsupport_impl.cc:644: WebFrame* web_frame = WebFrame::frameForCurrentContext(); On 2012/06/14 17:18:29, henrika wrote: > I can try to refactor if you think it is required. I have used the same approach > as already exists in other methods here. TBH I don't think the other uses in this file are valid either. WebFrame::frameForCurrentContext finds the "current" frame based on the JavaScript context that's highest on the stack. It's a very twisted way of getting to the RenderView. Plus there's many cases where it's not the frame you want (e.g. on frame scripts another frame from the same domain), and will lead to problems later if the RenderView that originally called this goes away (but the WebAudioDevice is attached to another one).
Hi Henrik, leaking internal implementation details of AudioDevice (the AudioMessageFilter) in the public API is a bit worrisome. I'm also concerned about the added complexity of the overall code change compared with any benefits we might get. We're at a critical point where we're trying to cleanup and *simplify* the audio back-end to make it easier to understand. This seems like it's making it unnecessarily complex. http://codereview.chromium.org/10537121/diff/4011/content/renderer/media/audi... File content/renderer/media/audio_device.cc (right): http://codereview.chromium.org/10537121/diff/4011/content/renderer/media/audi... content/renderer/media/audio_device.cc:42: AudioDevice::AudioDevice(AudioMessageFilter* filter) This is leaking internal implementation details for AudioDevice into the public interface which is not a good idea. The whole purpose of the AudioDevice interface is to shield the client from any particular low-level details such as this. With this change, any client of AudioDevice must now get entangled into AudioMessageFilters. http://codereview.chromium.org/10537121/diff/4011/content/renderer/media/audi... File content/renderer/media/audio_device.h (right): http://codereview.chromium.org/10537121/diff/4011/content/renderer/media/audi... content/renderer/media/audio_device.h:93: explicit AudioDevice(AudioMessageFilter* filter); This is leaking internal implementation details for AudioDevice into the public interface which is not a good idea. The whole purpose of the AudioDevice interface is to shield the client from any particular low-level details such as this. With this change, any client of AudioDevice must now get entangled into AudioMessageFilters. http://codereview.chromium.org/10537121/diff/4011/content/renderer/media/rend... File content/renderer/media/render_audiosourceprovider.h (right): http://codereview.chromium.org/10537121/diff/4011/content/renderer/media/rend... content/renderer/media/render_audiosourceprovider.h:43: AudioDeviceFactoryInterface* audio_device_factory); This seems unnecessarily complex to require a factory http://codereview.chromium.org/10537121/diff/4011/content/renderer/media/rend... File content/renderer/media/renderer_webaudiodevice_impl.h (right): http://codereview.chromium.org/10537121/diff/4011/content/renderer/media/rend... content/renderer/media/renderer_webaudiodevice_impl.h:25: RendererWebAudioDeviceImpl(AudioDeviceFactoryInterface* audio_device_factory, This seems unnecessarily complex to require a factory
Chris, sorry but I am not sure if I understand your arguments here. What I do is to follow a very common design patter in Chrome where objects are injected into the constructor. This decoupling makes it much easier to test and to try alternative design approaches. In this particular case, we can remove the constraint that all clients must be created on the main render thread (a restriction which has caused lots of issues for the WebRTC client). How can this be negative? If we want to try alternative AudioDevice implementations, we can modify the code at one place (where the factory is created) and the change will affect all clients.
On 2012/06/14 18:59:25, henrika wrote: > Chris, > > sorry but I am not sure if I understand your arguments here. What I do is to > follow a very common design patter in Chrome where objects are injected into the > constructor. This decoupling makes it much easier to test and to try alternative > design approaches. I'm very supportive of adding tests for this, but it's not clear that this large of a change would be necessary in order to add a unit test, for example. In this particular case, we can remove the constraint that > all clients must be created on the main render thread (a restriction which has > caused lots of issues for the WebRTC client). I'm *very* much in favor of improving AudioDevice so that it can be constructed on any thread. In fact, we will be needing this for the renderer-side <audio> mixing that Dale is working on. But surely there are simpler approaches that can be cleanly implemented which don't require something as complex as this CL, notably introducing more complexity into classes such as RenderAudioSourceProvider by now having them take factories in their constructors. This part really seem sub-optimal. How can this be negative? If we > want to try alternative AudioDevice implementations, we can modify the code at > one place (where the factory is created) and the change will affect all clients. I understand that you're doing some experiments with alternative implementations, but I'm very doubtful that we will need one, especially once we've made a series of simplifications and cleanup changes that are in the works. You can still experiment with alternative implementations locally on your own machine, but I don't think the chromium code-base should take the burden of this extra complexity for the benefit of these side experiments.
Is it correct that you like the result of this CL *very* much but you find it too complex? If so, care to provide and comments on how we can accomplis the same result in a less complex way? What I do here is to add one factory method. The rest of the changes adds more or less nothing to the code base in total.
On 2012/06/14 20:12:18, henrika wrote: > Is it correct that you like the result of this CL *very* much but you find it > too complex? If so, care to provide and comments on how we can accomplis the > same result in a less complex way? What I do here is to add one factory method. > The rest of the changes adds more or less nothing to the code base in total. The intentions are spot on (i.e., helping pave a path towards testing, permitting AudioDevice creation on different threads) but factories do introduce complexity by hiding which objects are created. For example code that used to say "new AudioDevice()" now require an injected factory + a call to Create(). Assuming in the end we've settled on one solution and we're done experimenting, will we end up removing the factories? Is it not possible to achieve some of these other goals w/o introducing factories?
I was looking at the main-thread issue with AudioDevice and think it could be fixed with something as simple as adding a static singleton getter in AudioMessageFilter. Ideally, I think we should fix this issue internally inside AudioDevice. On 2012/06/15 01:12:54, scherkus wrote: > On 2012/06/14 20:12:18, henrika wrote: > > Is it correct that you like the result of this CL *very* much but you find it > > too complex? If so, care to provide and comments on how we can accomplis the > > same result in a less complex way? What I do here is to add one factory > method. > > The rest of the changes adds more or less nothing to the code base in total. > > The intentions are spot on (i.e., helping pave a path towards testing, > permitting AudioDevice creation on different threads) but factories do introduce > complexity by hiding which objects are created. For example code that used to > say "new AudioDevice()" now require an injected factory + a call to Create(). > > Assuming in the end we've settled on one solution and we're done experimenting, > will we end up removing the factories? > > Is it not possible to achieve some of these other goals w/o introducing > factories?
I thought that the Singleton design pattern should be avoided in Chrome, or perhaps you did not mean to include base/memory/singleton.h? IMHO, injecting the filter through the ctor solves the problem in a clean way and at the same time simplifies testing.
On Fri, Jun 15, 2012 at 3:12 AM, <scherkus@chromium.org> wrote: > On 2012/06/14 20:12:18, henrika wrote: > >> Is it correct that you like the result of this CL *very* much but you >> find it >> too complex? If so, care to provide and comments on how we can accomplis >> the >> same result in a less complex way? What I do here is to add one factory >> > method. > >> The rest of the changes adds more or less nothing to the code base in >> total. >> > > The intentions are spot on (i.e., helping pave a path towards testing, > permitting AudioDevice creation on different threads) but factories do > introduce > complexity by hiding which objects are created. For example code that used > to > say "new AudioDevice()" now require an injected factory + a call to > Create(). > Given the approach I've taken with the filter, it felt like a natural approach to embed it into a factory. That way I hide that part from the user and at the same time solve the thread problem. I would not go for a factory if the construction was trivial. > Assuming in the end we've settled on one solution and we're done > experimenting, > will we end up removing the factories? > My main goal with this CL is not to allow experimenting; it is only a nice bonus effect. Even with a perfect master plan for how to solve all existing problems in the current path; would it not be beneficial with the possibility to compare different alternatives in the same code base? Let's say that you have three similar versions and want to compare them without rebuilding for each version. I am trying to solve a real problem and ensure that the AD can be created on any thread and at the same time simplify testing possibilities. If there are other ways to solve the same problems, now or in he future, without a factory then lets nuke it at that time. > > Is it not possible to achieve some of these other goals w/o introducing > factories? > I'm sure there is. Chris seems to have some ideas in this area but I would need more details from him before I can answer. This CL was the best proposal I could come up with. > http://codereview.chromium.**org/10537121/<http://codereview.chromium.org/105... >
> > I understand that you're doing some experiments with alternative > implementations, but I'm very doubtful that we will need one, especially > once > we've made a series of simplifications and cleanup changes that are in the > works. You can still experiment with alternative implementations locally > on > your own machine, but I don't think the chromium code-base should take the > burden of this extra complexity for the benefit of these side experiments. > You make it sound as if I am working against you instead of trying to improve things here. As I've mentioned earlier, this is one CL which I felt was suitable to break out from the experiments I've done lately and that would be beneficial today. The goal is to actively try to solve problems that we see and that we all discuss on a daily basis. Regarding "cleanup changes that are in the works", I have not seen any design doc, new issue reports or e-mail with details about ongoing work in this area which makes it a bit difficult to take into consideration when designing solutions. I do know what you want to accomplish though. I'm sorry to hear that these attempts are seen as a "burden on the chromium code base". If you don't want to approve this CL as is then fine; but I would have preferred some guidance on how to continue instead.
On 2012/06/15 13:45:35, henrika wrote: > I thought that the Singleton design pattern should be avoided in Chrome, or > perhaps you did not mean to include base/memory/singleton.h? I really just meant adding a simple getter accessor which should be a very simple change to the AudioMessageFilter API that AudioDevice can use internally. > > IMHO, injecting the filter through the ctor solves the problem in a clean way > and at the same time simplifies testing.
On 2012/06/15 14:43:37, henrika wrote: > > > > I understand that you're doing some experiments with alternative > > implementations, but I'm very doubtful that we will need one, especially > > once > > we've made a series of simplifications and cleanup changes that are in the > > works. You can still experiment with alternative implementations locally > > on > > your own machine, but I don't think the chromium code-base should take the > > burden of this extra complexity for the benefit of these side experiments. > > > > You make it sound as if I am working against you instead of trying to > improve things here. Henrik, I apologize if my comment came across as something personal against you. That certainly wasn't my intention. And, of course, I know you're trying your best to make improvements. But as a reviewer my job is to weigh the pros and cons of a particular CL. In this case I was just trying to point out that, in my opinion, there's non-trivial complexity added by this CL which has unclear benefit. Discussions about these kinds of trade-offs happen all the time in code reviews, so please don't take it personally. As I've mentioned earlier, this is one CL which I felt > was suitable to break out from the experiments I've done lately and that > would be beneficial today. The goal is to actively try to solve problems > that we see and that we all discuss on a daily basis. Regarding "cleanup > changes that are in the works", I have not seen any design doc, new issue > reports or e-mail with details about ongoing work in this area which makes > it a bit difficult to take into consideration when designing solutions. I did share the renderer-side mixer design doc with you, and have discussed many of the details of the ideas for cleanup with you in personal emails. I've discussed the ideas in even more detail with Andrew, and hope to include a list of the issues more formally in a design document. I > do know what you want to accomplish though. > > I'm sorry to hear that these attempts are seen as a "burden on the chromium > code base". If you don't want to approve this CL as is then fine; but I > would have preferred some guidance on how to continue instead. Sorry, this code-review thread for this particular CL is an imperfect forum for discussing the nuance of the overall audio architecture. I was just trying to concentrate on the details for this CL. I hope to setup a time to be able to talk with you more directly so we can have a good conversation about the overall architecture.
What about making the message filter an implementation detail of one of the factories? * Don't change the AudioRendererSink interface. * Make two separate factory implementations. * The AudioDevice factory takes care of injecting the filter into the AudioDevice instance inside it's Create() method as is done in the current template implementation except the constructor won't be virtual. * Other factories have different implementations for Create (i.e. don't use the template). On Fri, Jun 15, 2012 at 7:42 PM, <crogers@google.com> wrote: > On 2012/06/15 14:43:37, henrika wrote: > >> > >> > I understand that you're doing some experiments with alternative >> > implementations, but I'm very doubtful that we will need one, especially >> > once >> > we've made a series of simplifications and cleanup changes that are in >> the >> > works. You can still experiment with alternative implementations >> locally >> > on >> > your own machine, but I don't think the chromium code-base should take >> the >> > burden of this extra complexity for the benefit of these side >> experiments. >> > >> > > You make it sound as if I am working against you instead of trying to >> improve things here. >> > > Henrik, I apologize if my comment came across as something personal > against you. > That certainly wasn't my intention. And, of course, I know you're trying > your > best to make improvements. But as a reviewer my job is to weigh the pros > and > cons of a particular CL. In this case I was just trying to point out > that, in > my opinion, there's non-trivial complexity added by this CL which has > unclear > benefit. Discussions about these kinds of trade-offs happen all the time > in code > reviews, so please don't take it personally. > > > As I've mentioned earlier, this is one CL which I felt > >> was suitable to break out from the experiments I've done lately and that >> would be beneficial today. The goal is to actively try to solve problems >> that we see and that we all discuss on a daily basis. Regarding "cleanup >> changes that are in the works", I have not seen any design doc, new issue >> reports or e-mail with details about ongoing work in this area which makes >> it a bit difficult to take into consideration when designing solutions. >> > > I did share the renderer-side mixer design doc with you, and have > discussed many > of the details of the ideas for cleanup with you in personal emails. I've > discussed the ideas in even more detail with Andrew, and hope to include a > list > of the issues more formally in a design document. > > > I > >> do know what you want to accomplish though. >> > > I'm sorry to hear that these attempts are seen as a "burden on the >> chromium >> code base". If you don't want to approve this CL as is then fine; but I >> would have preferred some guidance on how to continue instead. >> > > Sorry, this code-review thread for this particular CL is an imperfect > forum for > discussing the nuance of the overall audio architecture. I was just > trying to > concentrate on the details for this CL. I hope to setup a time to be able > to > talk with you more directly so we can have a good conversation about the > overall > architecture. > > > > http://codereview.chromium.**org/10537121/<http://codereview.chromium.org/105... >
I just looked at what I believe to be the contention point and I think there's just a little misunderstanding. See below. http://codereview.chromium.org/10537121/diff/4011/content/renderer/media/audi... File content/renderer/media/audio_device.h (right): http://codereview.chromium.org/10537121/diff/4011/content/renderer/media/audi... content/renderer/media/audio_device.h:93: explicit AudioDevice(AudioMessageFilter* filter); On 2012/06/14 17:41:44, Chris Rogers wrote: > This is leaking internal implementation details for AudioDevice into the public > interface which is not a good idea. The whole purpose of the AudioDevice > interface is to shield the client from any particular low-level details such as > this. With this change, any client of AudioDevice must now get entangled into > AudioMessageFilters. AudioDevice is the implementation, media::AudioRendererSink is the interface (which btw should have been named media::AudioRendererSinkInterface in accordance with the style guide). Unless this constructor is a part of the interface, then there's nothing being leaked.
Chris, regarding singletons in Chrome, the following comment is added to singleton.h: // Instead of adding another singleton into the mix, try to identify either: // a) An existing singleton that can manage your object's lifetime // b) *Locations where you can deterministically create the object and pass* *// into other objects* This CL uses (b) above and the same is done for many other filters and members on the main render thread. I am not inventing anything new here. Also, as Tommi has commented, adding the filter to the ctor of AudioDevice is not "leaky" since it is the media::AudioRendererSink which is the interface here. You say that: *"I was looking at the main-thread issue with AudioDevice and think it could be fixed with something as simple as adding a static singleton getter in AudioMessageFilter. Ideally, I think we should fix this issue internally inside AudioDevice.* but I still don't understand how: (1) that would be done in practice, and (2) if it is possible, why it should be better to hide the filter details in the AudioDevice. It will make it much more difficult to add unit tests for the AudioDevice. If you feel that this is *the* way to go then please add some more details on how to implement it because it beats me how it should be done. I have now simplified the CL according to input from Tommi and the factory no longer uses a template. I have only created one factory (for AudioDevice) and it need the message filter as input.
On 2012/06/16 09:09:06, tommi wrote: > I just looked at what I believe to be the contention point and I think there's > just a little misunderstanding. See below. > > http://codereview.chromium.org/10537121/diff/4011/content/renderer/media/audi... > File content/renderer/media/audio_device.h (right): > > http://codereview.chromium.org/10537121/diff/4011/content/renderer/media/audi... > content/renderer/media/audio_device.h:93: explicit > AudioDevice(AudioMessageFilter* filter); > On 2012/06/14 17:41:44, Chris Rogers wrote: > > This is leaking internal implementation details for AudioDevice into the > public > > interface which is not a good idea. The whole purpose of the AudioDevice > > interface is to shield the client from any particular low-level details such > as > > this. With this change, any client of AudioDevice must now get entangled into > > AudioMessageFilters. > > AudioDevice is the implementation, media::AudioRendererSink is the interface > (which btw should have been named media::AudioRendererSinkInterface in > accordance with the style guide). Unless this constructor is a part of the > interface, then there's nothing being leaked. I'm not quite sure I agree with this. After all AudioDevice is a concrete implementation which has implementation details, which ideally will be hidden in a good design. In this case, AudioMessageFilter should not be exposed publicly in any way.
On 2012/06/18 09:15:31, henrika wrote:
> Chris,
>
> regarding singletons in Chrome, the following comment is added to
> singleton.h:
>
> // Instead of adding another singleton into the mix, try to identify either:
> // a) An existing singleton that can manage your object's lifetime
> // b) *Locations where you can deterministically create the object and
> pass*
> *// into other objects*
>
> This CL uses (b) above and the same is done for many other filters and
> members on the main render thread. I am not inventing anything new here.
> Also, as Tommi has commented, adding the filter to the ctor of AudioDevice
> is not "leaky" since it is the media::AudioRendererSink which is the
> interface here.
>
> You say that:
>
> *"I was looking at the main-thread issue with AudioDevice and think it
> could be fixed with something as simple as adding a static singleton getter
> in AudioMessageFilter. Ideally, I think we should fix this issue internally
> inside AudioDevice.*
>
> but I still don't understand how: (1) that would be done in practice, and
> (2) if it is possible, why it should be better to hide the filter details
> in the AudioDevice. It will make it much more difficult to add unit tests
> for the AudioDevice. If you feel that this is *the* way to go then please
> add some more details on how to implement it because it beats me how it
> should be done.
My recommendation is to maintain a pointer to the AudioMessageFilter as a static
member variable of AudioMessageFilter which is initialized when the constructor
is called. Then add a getter, something like:
static AudioMessageFilter* AudioMessageFilter::current() { return s_filter; }
Then have AudioDevice call this method. This seems quite a lot simpler to me.
>
> I have now simplified the CL according to input from Tommi and the factory
> no longer uses a template. I have only created one factory (for
> AudioDevice) and it need the message filter as input.
> My recommendation is to maintain a pointer to the AudioMessageFilter as a
static
> member variable of AudioMessageFilter which is initialized when the
constructor
> is called. Then add a getter, something like:
>
> static AudioMessageFilter* AudioMessageFilter::current() { return s_filter; }
>
> Then have AudioDevice call this method. This seems quite a lot simpler to me.
>
If we can avoid it, let's not add more global variables to the code. We've been
working on getting rid of designs like that all over Chrome, so let's try to
avoid it.
On 2012/06/18 17:32:40, Chris Rogers wrote: > On 2012/06/16 09:09:06, tommi wrote: > > I just looked at what I believe to be the contention point and I think there's > > just a little misunderstanding. See below. > > > > > http://codereview.chromium.org/10537121/diff/4011/content/renderer/media/audi... > > File content/renderer/media/audio_device.h (right): > > > > > http://codereview.chromium.org/10537121/diff/4011/content/renderer/media/audi... > > content/renderer/media/audio_device.h:93: explicit > > AudioDevice(AudioMessageFilter* filter); > > On 2012/06/14 17:41:44, Chris Rogers wrote: > > > This is leaking internal implementation details for AudioDevice into the > > public > > > interface which is not a good idea. The whole purpose of the AudioDevice > > > interface is to shield the client from any particular low-level details such > > as > > > this. With this change, any client of AudioDevice must now get entangled > into > > > AudioMessageFilters. > > > > AudioDevice is the implementation, media::AudioRendererSink is the interface > > (which btw should have been named media::AudioRendererSinkInterface in > > accordance with the style guide). Unless this constructor is a part of the > > interface, then there's nothing being leaked. > > I'm not quite sure I agree with this. After all AudioDevice is a concrete > implementation which has implementation details, which ideally will be hidden in > a good design. In this case, AudioMessageFilter should not be exposed publicly > in any way. Well, it's not really a question of agreeing or disagreeing. That is how it is. Direct AudioDevice pointers should not be held by anyone and AudioDevice's entire implementation should be considered private. We should clean up places that currently hold a pointer to AudioDevice and change those pointers to be pointers to the interface and not the much wider AudioDevice class which already inherits from AudioMessageFilter::Delegate, thus already leaking the notion of an AudioMessageFilter, SyncSocket, SharedMemory etc (all implementation details) via its public interface.
> Well, it's not really a question of agreeing or disagreeing. That is how > it is. > Direct AudioDevice pointers should not be held by anyone and AudioDevice's > entire implementation should be considered private. We should clean up > places > that currently hold a pointer to AudioDevice and change those pointers to > be > pointers to the interface and not the much wider AudioDevice class which > already > inherits from AudioMessageFilter::Delegate, thus already leaking the > notion of > an AudioMessageFilter, SyncSocket, SharedMemory etc (all implementation > details) > via its public interface. > Thanks for your comments Tommi. Just to clarify, the described cleanup is exactly what I do in this CL. See renderer_webaudiodevice_impl<http://codereview.chromium.org/10537121/diff/20001/content/renderer/media/renderer_webaudiodevice_impl.h> as one example.
On 2012/06/18 21:12:18, tommi wrote: > Well, it's not really a question of agreeing or disagreeing. That is how it is. > Direct AudioDevice pointers should not be held by anyone and AudioDevice's > entire implementation should be considered private. We should clean up places > that currently hold a pointer to AudioDevice and change those pointers to be > pointers to the interface But we use direct pointers to objects and *not* indirect pointers to interfaces/factories in the vast majority of cases when dealing with objects in Chrome, WebKit, or just about any large-scale C++ project I know about or have worked on. > and not the much wider AudioDevice class which already > inherits from AudioMessageFilter::Delegate, thus already leaking the notion of > an AudioMessageFilter, SyncSocket, SharedMemory etc (all implementation details) > via its public interface. But, we can fix that easily by changing audio_device.h to use "private" inheritance for AudioMessageFilter::Delegate(). I'm certainly in support of that cleanup and it's so easy to do, and is a tiny change.
On 2012/06/18 21:00:40, tommi wrote:
> > My recommendation is to maintain a pointer to the AudioMessageFilter as a
> static
> > member variable of AudioMessageFilter which is initialized when the
> constructor
> > is called. Then add a getter, something like:
> >
> > static AudioMessageFilter* AudioMessageFilter::current() { return s_filter;
}
> >
> > Then have AudioDevice call this method. This seems quite a lot simpler to
me.
> >
>
> If we can avoid it, let's not add more global variables to the code. We've
been
> working on getting rid of designs like that all over Chrome, so let's try to
> avoid it.
I think it's a good and simple design. John Abd-El-Malek and I just talked
about this and he thought that adding AudioMessageFilter::current() was a
perfectly reasonable solution. Using the technique I recommend is a *very*
small code change as compared with the current CL, and has the additional
benefit of not exposing dependencies and requiring several other files to keep
track of factories.
Chris (and Tommi who is on vacation...), thanks for all your input on this CL. Chris, I am not sure where to go from here since, even if your proposal will fly, it does in fact "cut off" some of the benefits with the initial CL. Then I talk about the more flexible testing possibilities and all the implementation-hiding details that Tommi lists. In addition, we will add more global variables to the code base. I would prefer the existing design instead. Andrew: care to fire off some or your comments here as well? If we can't agree, I guess John will use his new title and tell me what to do next ;-) If it sounds like a good idea, I could upload an alternative patch (based on Chris proposal) to make it easier to compare.
On Tue, Jun 19, 2012 at 12:43 AM, <crogers@google.com> wrote: > On 2012/06/18 21:12:18, tommi wrote: > >> Well, it's not really a question of agreeing or disagreeing. That is how >> it >> > is. > >> Direct AudioDevice pointers should not be held by anyone and >> AudioDevice's >> entire implementation should be considered private. We should clean up >> places >> that currently hold a pointer to AudioDevice and change those pointers to >> be >> pointers to the interface >> > > But we use direct pointers to objects and *not* indirect pointers to > interfaces/factories in the vast majority of cases when dealing with > objects in > Chrome, WebKit, or just about any large-scale C++ project I know about or > have > worked on. I'm not sure what you're saying here, but interfaces are used all over the place in Chrome and WebKit. They are a very important part of making things easily testable and necessary for supporting things like mocks. That's how the WebKit glue works among other things and it's used very heavily in Chrome. > and not the much wider AudioDevice class which already >> inherits from AudioMessageFilter::Delegate, thus already leaking the >> notion of >> an AudioMessageFilter, SyncSocket, SharedMemory etc (all implementation >> > details) > >> via its public interface. >> > > But, we can fix that easily by changing audio_device.h to use "private" > inheritance for AudioMessageFilter::Delegate()**. I'm certainly in > support of > that cleanup and it's so easy to do, and is a tiny change. > > The point is not to "fix" AudioDevice. AudioDevice is an implementation of an interface. Being able to swap out AudioDevice for an alternate implementation or mock is what this CL is about. > > http://codereview.chromium.**org/10537121/<http://codereview.chromium.org/105... >
[+john] On Tue, Jun 19, 2012 at 12:51 AM, <crogers@google.com> wrote: > On 2012/06/18 21:00:40, tommi wrote: > >> > My recommendation is to maintain a pointer to the AudioMessageFilter as >> a >> static >> > member variable of AudioMessageFilter which is initialized when the >> constructor >> > is called. Then add a getter, something like: >> > >> > static AudioMessageFilter* AudioMessageFilter::current() { return >> s_filter; >> > } > >> > >> > Then have AudioDevice call this method. This seems quite a lot simpler >> to >> > me. > >> > >> > > If we can avoid it, let's not add more global variables to the code. >> We've >> > been > >> working on getting rid of designs like that all over Chrome, so let's try >> to >> avoid it. >> > > I think it's a good and simple design. John Abd-El-Malek and I just talked > about this and he thought that adding AudioMessageFilter::current() was a > perfectly reasonable solution. Using the technique I recommend is a *very* > small code change as compared with the current CL, and has the additional > benefit of not exposing dependencies and requiring several other files to > keep > track of factories. > > Singletons are simple, no question about it. But to every complex question... The benefit you mention, is not a benefit at all since the dependencies you're talking about are currently in AudioDevice, they are there with your suggestion and they are also there in this CL. There's no dependency change. The difference is that one approach gets its pointer to the message filter via the constructor, the other via a global variable. I think that it's easy to write tests where the message filter is injected via the constructor, but a global variable that can't be set to a custom instance such as a mock, is more troublesome. What's the plan there? > http://codereview.chromium.**org/10537121/<http://codereview.chromium.org/105... >
On 2012/06/19 14:27:55, tommi wrote: > Singletons are simple, no question about it. But to every complex > question... > > The benefit you mention, is not a benefit at all since the dependencies > you're > talking about are currently in AudioDevice, they are there with your > suggestion > and they are also there in this CL. There's no dependency change. That is quite simply not true. If you examine the CL, there are now several files/classes touched which now require knowing about and passing factories which did not used to be required to know anything about factories (adding dependencies and clunkiness). In the case of the renderer-side mixing (work which has not yet landed), the place where the actual AudioDevice is created is several layers deep (objects creating objects which create objects), so we would have to add plumbing to pass the factory through all of these layers. This is so unnecessary. I'm not disputing that factories can sometimes be useful and appropriate design patterns, but they're not the solution to every problem. In this case, I think the fact that we have to touch so many classes (and in quite a cumbersome way with the renderer-side mixing code), adding plumbing for factories, etc. shows the inelegance of this particular solution compared with a simple and direct approach. > > The difference is that one approach gets its pointer to the message filter > via the > constructor, the other via a global variable. > > I think that it's easy to write tests where the message filter is injected > via the > constructor, but a global variable that can't be set to a custom instance > such > as a mock, is more troublesome. What's the plan there? I think this is a straw-man argument. We don't need to inject an AudioMessageFilter into AudioDevice in order to write a unit-test for it. As evidence, I point to the old version of AudioRendererImpl, before: http://codereview.chromium.org/8477037/ In this old version of AudioRendererImpl, it created an AudioMessageFilter internally, and we were able to write a decent unit test. > > > > > http://codereview.chromium.**org/10537121/%3Chttp://codereview.chromium.org/1...> > >
Hi Henrik, I've uploaded an example of how we could address the threading issues with AudioDevice: https://chromiumcodereview.appspot.com/10580033
On Wed, Jun 20, 2012 at 3:42 AM, <crogers@google.com> wrote: > Hi Henrik, I've uploaded an example of how we could address the threading > issues > with AudioDevice: > https://chromiumcodereview.**appspot.com/10580033<https://chromiumcodereview.... Added comments. > > > http://codereview.chromium.**org/10537121/<http://codereview.chromium.org/105... >
On Tue, Jun 19, 2012 at 9:56 PM, <crogers@google.com> wrote: > On 2012/06/19 14:27:55, tommi wrote: > >> Singletons are simple, no question about it. But to every complex >> question... >> > > The benefit you mention, is not a benefit at all since the dependencies >> you're >> talking about are currently in AudioDevice, they are there with your >> suggestion >> and they are also there in this CL. There's no dependency change. >> > > That is quite simply not true. If you examine the CL, there are now > several > files/classes touched which now require knowing about and passing factories > which did not used to be required to know anything about factories (adding > dependencies and clunkiness). In the case of the renderer-side mixing > (work > which has not yet landed), the place where the actual AudioDevice is > created is > several layers deep (objects creating objects which create objects), so we > would > have to add plumbing to pass the factory through all of these layers. > This is > so unnecessary. > The point with the factory is to be able to provide alternate implementations of media::AudioRendererSink. To do this, code that currently holds a raw pointer to the AudioDevice implementation need to be fixed to hold a pointer to the interface instead (which is what they need). This is one change that touches a few files. I think there's some confusion around the purpose of the factory. The factory is there so that there isn't a irect dependency on AudioDevice, which is a specific implementation of the interface. Using the global pointer to the audio message filter doesn't provide that functionality and in fact has nothing to do with the purpose of the factory. > I'm not disputing that factories can sometimes be useful and appropriate > design > patterns, but they're not the solution to every problem. In this case, I > think > the fact that we have to touch so many classes (and in quite a cumbersome > way > with the renderer-side mixing code), adding plumbing for factories, etc. > shows > the inelegance of this particular solution compared with a simple and > direct > approach. What would you recommend be done instead? Inelegant and/or cumbersome, this is what is done all over Chrome. > The difference is that one approach gets its pointer to the message filter >> via the >> constructor, the other via a global variable. >> > > I think that it's easy to write tests where the message filter is injected >> via the >> constructor, but a global variable that can't be set to a custom instance >> such >> as a mock, is more troublesome. What's the plan there? >> > > I think this is a straw-man argument. We don't need to inject an > AudioMessageFilter into AudioDevice in order to write a unit-test for it. > As > evidence, I point to the old version of AudioRendererImpl, before: > http://codereview.chromium.**org/8477037/<http://codereview.chromium.org/8477... > In this old version of AudioRendererImpl, it created an AudioMessageFilter > internally, and we were able to write a decent unit test. > This is not a straw-man argument, this is an argument for being able to use more than one implementation of an interface plain and simple. I took a quick look at your submitted changelists in order to understand your preference for writing tests, but I did not find a single test in all your submitted changelists to chrome. So, I'm assuming that you haven't written one for chrome at least but I certainly recommend that you start! :) I have though and I know the codebase well. From experience I can tell you that using injection is an easier way to writing tests than dealing with global variables to fixed implementations. It can be done without it, but it's a lot of work and makes for flaky tests. In fact the example you give shows how much work it is to simulate the entire global state of the renderer in order to test a single class. Furthermore, as more tests touch the same global state, the doors to flakiness open. As you know, being the original author, AudioDevice does not have any dedicated tests and I find it likely that its dependency on global state such as the audio message filter and the IO thread had something to do with that.
(late to the party, just saw this thread. for future reference, I'm CCd on a lot of changes because of watchlists so I don't really look at my CCd reviews, if you want me to look at a change, add me as a reviewer and I can be removed after commenting.) I've tried to read this whole thread, here are my thoughts, hopefully I won't make this thread longer :) If there will be unit tests written that create different implementations of media::AudioRendererSink, then it seems reasonable that some sort of factory for that interface would be needed. Small side note that I would prefer if instead of an interface, a callback is used (no need to create interfaces for just one function). One way that other chrome code avoids passing the factory all over the place is to have a default implementation if a factory isn't set. RenderViewHostFactory::Create is a good example. Unit tests can call RenderViewHostFactory::RegisterFactory to set a factory. If a factory isn't set, the default implementation is used. In this case, it would be creating an AudioDevice. If that's done, then AudioMessageFilter can be exposed with a static getter as Chris suggests. Even if a static getter is created and someone wants to write a unittest with the AudioDevice object, they can create an AudioMessageFilter object before hand which would set the global. What do you guys think of this compromise? It allows creation of other AudioRendererSink implementations like Henrik needs, while not making all the code know about factories like Chris is trying to avoid.
Hi John, thanks for taking the time to read through the whole thread. I think something like what you're saying sounds reasonable. Following up on your idea, what I would suggest is a default AudioDevice constructor which takes no arguments (not AudioMessageFilter) and internally uses the AudioMessageFilter static getter. Then there would be an alternate constructor (possibly private) which can take an AudioMessageFilter as an argument for use by any unit tests needing to control this. That way, ordinary clients of AudioDevice don't need to be exposed to the implementation details, but unit tests can have more flexibility.
John, thanks for suggesting an alternative solution. It sounds like a great idea but I would need to see the actual implementation before I can say for sure. Chris, to save us one day, would you mind patching your alternative CL according to the proposal from John? Given that you are both in MTV, it might be easier to sort out the last details if needed. If you don't have time, I can give it a try tomorrow. On Wed, Jun 20, 2012 at 6:07 PM, <jam@chromium.org> wrote: > (late to the party, just saw this thread. for future reference, I'm CCd on > a lot > of changes because of watchlists so I don't really look at my CCd reviews, > if > you want me to look at a change, add me as a reviewer and I can be removed > after > commenting.) > > I've tried to read this whole thread, here are my thoughts, hopefully I > won't > make this thread longer :) > > If there will be unit tests written that create different implementations > of > media::AudioRendererSink, then it seems reasonable that some sort of > factory for > that interface would be needed. Small side note that I would prefer if > instead > of an interface, a callback is used (no need to create interfaces for just > one > function). > > One way that other chrome code avoids passing the factory all over the > place is > to have a default implementation if a factory isn't set. > RenderViewHostFactory::Create is a good example. Unit tests can call > RenderViewHostFactory::**RegisterFactory to set a factory. If a factory > isn't set, > the default implementation is used. In this case, it would be creating an > AudioDevice. If that's done, then AudioMessageFilter can be exposed with a > static getter as Chris suggests. Even if a static getter is created and > someone > wants to write a unittest with the AudioDevice object, they can create an > AudioMessageFilter object before hand which would set the global. > > What do you guys think of this compromise? It allows creation of other > AudioRendererSink implementations like Henrik needs, while not making all > the > code know about factories like Chris is trying to avoid. > > http://codereview.chromium.**org/10537121/<http://codereview.chromium.org/105... >
Thanks John for chiming in and I think that's a great idea. On Wed, Jun 20, 2012 at 7:29 PM, <crogers@google.com> wrote: > Hi John, thanks for taking the time to read through the whole thread. I > think > something like what you're saying sounds reasonable. Following up on your > idea, > what I would suggest is a default AudioDevice constructor which takes no > arguments (not AudioMessageFilter) and internally uses the > AudioMessageFilter > static getter. Then there would be an alternate constructor (possibly > private) > which can take an AudioMessageFilter as an argument for use by any unit > tests > needing to control this. That way, ordinary clients of AudioDevice don't > need > to be exposed to the implementation details, but unit tests can have more > flexibility. > > * AudioDevice per se has no clients. * Only the callback will be aware of the constructors of the implementation (AudioDevice or other implementations). * The media::AudioRendererSink interface has clients and no dependency on the audio message filter. Thus no one will be exposed to implementation details and we can all be happy :) > http://codereview.chromium.**org/10537121/<http://codereview.chromium.org/105... >
Hey Henrik - here's an example that uses a callback (see ReadCompleteCallback): https://chromiumcodereview.appspot.com/10540047/ On Wed, Jun 20, 2012 at 7:30 PM, Henrik Andreasson <henrika@chromium.org>wrote: > John, > > thanks for suggesting an alternative solution. It sounds like a great idea > but I would need to see the actual implementation before I can say for > sure. > > Chris, to save us one day, would you mind patching your alternative CL > according to the proposal from John? Given that you are both in MTV, it > might be easier to sort out the last details if needed. If you don't have > time, I can give it a try tomorrow. > > > On Wed, Jun 20, 2012 at 6:07 PM, <jam@chromium.org> wrote: > >> (late to the party, just saw this thread. for future reference, I'm CCd >> on a lot >> of changes because of watchlists so I don't really look at my CCd >> reviews, if >> you want me to look at a change, add me as a reviewer and I can be >> removed after >> commenting.) >> >> I've tried to read this whole thread, here are my thoughts, hopefully I >> won't >> make this thread longer :) >> >> If there will be unit tests written that create different implementations >> of >> media::AudioRendererSink, then it seems reasonable that some sort of >> factory for >> that interface would be needed. Small side note that I would prefer if >> instead >> of an interface, a callback is used (no need to create interfaces for >> just one >> function). >> >> One way that other chrome code avoids passing the factory all over the >> place is >> to have a default implementation if a factory isn't set. >> RenderViewHostFactory::Create is a good example. Unit tests can call >> RenderViewHostFactory::**RegisterFactory to set a factory. If a factory >> isn't set, >> the default implementation is used. In this case, it would be creating an >> AudioDevice. If that's done, then AudioMessageFilter can be exposed with a >> static getter as Chris suggests. Even if a static getter is created and >> someone >> wants to write a unittest with the AudioDevice object, they can create an >> AudioMessageFilter object before hand which would set the global. >> >> What do you guys think of this compromise? It allows creation of other >> AudioRendererSink implementations like Henrik needs, while not making all >> the >> code know about factories like Chris is trying to avoid. >> >> http://codereview.chromium.**org/10537121/<http://codereview.chromium.org/105... >> > >
I will try to wrap something up that makes everyone happy. Stay tuned.
Apologies for mysteriously disappearing from this thread! I see this as another case of "making things easier to test and swap impls (i.e., via injection)" being at odds with "complicating production code for the sake of tests and swapping impls" -- so what to do!? Plumbing factories / objects through multiple layers of abstraction is not fun to maintain. For example I've authored and maintained code in Chromium where I thought plumbing MessageLoop pointers into every object was A Good Thing Because It Allowed For Tests With Injected Message Loops(tm) but using MessageLoop::current() in the ctor (or in an Initialize() method) accomplishes the same thing without the clutter. What jam@ proposes helps solve the multiple-impls-of-AudioRendererSink case. What crogers@ proposes helps the injecting-AudioMessageFilters-into-tests case (especially if the static is cleared in the dtor!). I think this works -- what do people think?
Hi all, I have now modified this CL and tried to take all input into account to make everyone happy. I've also tried to make it as simple and clean as possible. To be honest, I am not sure if I still cover the same ground as in the original CL due to all the discussions. John, you say "If a factory isn't set,the default implementation is used. In this case, it would be creating an AudioDevice. If that's done, then AudioMessageFilter can be exposed with a static getter as Chris suggests. Even if a static getter is created and someone wants to write a unittest with the AudioDevice object, they can create an AudioMessageFilter object before hand which would set the global." Can you please elaborate on that comment since I am not sure what is the best way to combine Chris static getter with the new static Create() method in AudioDeviceFactory. The provided patch allows us to mock out and AudioDevice by calling AudioDeviceFactory::RegisterFactory() but there is no clean way of using the default AudioDevice but inject a mocked AudioMessageFilter. Or am I missing something here? John: I was not sure on what you mean by using a callback instead of an interface and backed down on that part as well. Andrew and Chris: any input that will render "the perfect harmonized solution" would be appreciated. Hope we are closer to an agreement in this version.
Hi Henrik, this looks great - much simpler! http://codereview.chromium.org/10537121/diff/40001/content/renderer/media/aud... File content/renderer/media/audio_device.cc (right): http://codereview.chromium.org/10537121/diff/40001/content/renderer/media/aud... content/renderer/media/audio_device.cc:49: DVLOG(1) << "AudioDevice::AudioDevice()"; I'd remove the DVLOG http://codereview.chromium.org/10537121/diff/40001/content/renderer/media/aud... content/renderer/media/audio_device.cc:63: DVLOG(1) << "AudioDevice::AudioDevice(filter)"; I'd remove the DVLOG http://codereview.chromium.org/10537121/diff/40001/content/renderer/media/aud... content/renderer/media/audio_device.cc:79: DVLOG(1) << "AudioDevice::~AudioDevice()"; I'd remove the DVLOG http://codereview.chromium.org/10537121/diff/40001/content/renderer/media/aud... File content/renderer/media/audio_device.h (right): http://codereview.chromium.org/10537121/diff/40001/content/renderer/media/aud... content/renderer/media/audio_device.h:94: explicit AudioDevice(AudioMessageFilter* filter); I wouldn't mind making this constructor (taking the filter) private, then making the test be a friend. http://codereview.chromium.org/10537121/diff/40001/content/renderer/renderer_... File content/renderer/renderer_webkitplatformsupport_impl.cc (right): http://codereview.chromium.org/10537121/diff/40001/content/renderer/renderer_... content/renderer/renderer_webkitplatformsupport_impl.cc:8: #include <vector> Not sure if this or the other changes in this file are still needed?
On 2012/06/25 15:56:01, henrika wrote: > Hi all, > > I have now modified this CL and tried to take all input into account to make > everyone happy. I've also tried to make it as simple and clean as possible. > > To be honest, I am not sure if I still cover the same ground as in the original > CL due to all the discussions. > > John, you say "If a factory isn't set,the default implementation is used. You did that in this change :) (I had meant not having to create a default factory) > In > this case, it would be creating an AudioDevice. If that's done, then > AudioMessageFilter can be exposed with a > static getter as Chris suggests. Even if a static getter is created and someone > wants to write a unittest with the AudioDevice object, they can create an > AudioMessageFilter object before hand which would set the global." > > > Can you please elaborate on that comment since I am not sure what is the best > way to combine Chris static getter with the new static Create() method in > AudioDeviceFactory. > > The provided patch allows us to mock out and AudioDevice by calling > AudioDeviceFactory::RegisterFactory() but there is no clean way of using the > default AudioDevice but inject a mocked AudioMessageFilter. Or am I missing > something here? > > John: I was not sure on what you mean by using a callback instead of an > interface and backed down on that part as well. I meant to use base::Callback since it's just one function. > > Andrew and Chris: any input that will render "the perfect harmonized solution" > would be appreciated. Hope we are closer to an agreement in this version.
Moved tommi to CC list since he is on vacation. jam: Need your OK for minor parts since you are owner in content. Chris: Simplified even further. We can add more parts suitable for testing when tests are added instead. http://codereview.chromium.org/10537121/diff/40001/content/renderer/media/aud... File content/renderer/media/audio_device.cc (right): http://codereview.chromium.org/10537121/diff/40001/content/renderer/media/aud... content/renderer/media/audio_device.cc:49: DVLOG(1) << "AudioDevice::AudioDevice()"; On 2012/06/25 20:05:17, Chris Rogers wrote: > I'd remove the DVLOG Done. http://codereview.chromium.org/10537121/diff/40001/content/renderer/media/aud... content/renderer/media/audio_device.cc:63: DVLOG(1) << "AudioDevice::AudioDevice(filter)"; On 2012/06/25 20:05:17, Chris Rogers wrote: > I'd remove the DVLOG Done. http://codereview.chromium.org/10537121/diff/40001/content/renderer/media/aud... content/renderer/media/audio_device.cc:79: DVLOG(1) << "AudioDevice::~AudioDevice()"; On 2012/06/25 20:05:17, Chris Rogers wrote: > I'd remove the DVLOG Done. http://codereview.chromium.org/10537121/diff/40001/content/renderer/media/aud... File content/renderer/media/audio_device.h (right): http://codereview.chromium.org/10537121/diff/40001/content/renderer/media/aud... content/renderer/media/audio_device.h:94: explicit AudioDevice(AudioMessageFilter* filter); Good comment. Given that I have not added a unit test in this CL, we might as well remove this method for now and add it when a unit test is written instead. http://codereview.chromium.org/10537121/diff/40001/content/renderer/renderer_... File content/renderer/renderer_webkitplatformsupport_impl.cc (right): http://codereview.chromium.org/10537121/diff/40001/content/renderer/renderer_... content/renderer/renderer_webkitplatformsupport_impl.cc:8: #include <vector> I removed this file from the CL.
http://codereview.chromium.org/10537121/diff/47001/content/renderer/media/aud... File content/renderer/media/audio_message_filter.cc (right): http://codereview.chromium.org/10537121/diff/47001/content/renderer/media/aud... content/renderer/media/audio_message_filter.cc:25: // DCHECK(RenderThreadImpl::current()) << Will remove this part to simplify unit testing.
Thanks Henrik - LGTM
http://codereview.chromium.org/10537121/diff/47001/content/renderer/media/aud... File content/renderer/media/audio_device.h (right): http://codereview.chromium.org/10537121/diff/47001/content/renderer/media/aud... content/renderer/media/audio_device.h:79: class AudioMessageFilter; It seems like you can probably just revert all the changes in this file since you've now removed the AudioMessageFilter constructor.
http://codereview.chromium.org/10537121/diff/47001/content/renderer/media/aud... File content/renderer/media/audio_device.h (right): http://codereview.chromium.org/10537121/diff/47001/content/renderer/media/aud... content/renderer/media/audio_device.h:79: class AudioMessageFilter; To me the existing change still makes sense. It feels odd to have a contract where Initialize() can be optional. Agree?
Sorry, mis-read that part. Yes, I agree we should remove the old constructor I was wondering if we have to add the forward declaration for: class AudioMessageFilter; And also, the comment change "constructor" -> "constructors" now seems not to match On 2012/06/26 20:28:51, henrika wrote: > http://codereview.chromium.org/10537121/diff/47001/content/renderer/media/aud... > File content/renderer/media/audio_device.h (right): > > http://codereview.chromium.org/10537121/diff/47001/content/renderer/media/aud... > content/renderer/media/audio_device.h:79: class AudioMessageFilter; > To me the existing change still makes sense. It feels odd to have a contract > where Initialize() can be optional. Agree?
very nice! http://codereview.chromium.org/10537121/diff/47001/content/renderer/media/aud... File content/renderer/media/audio_device.cc (right): http://codereview.chromium.org/10537121/diff/47001/content/renderer/media/aud... content/renderer/media/audio_device.cc:14: #include "content/renderer/media/audio_message_filter.h" nit: we include this in the .h http://codereview.chromium.org/10537121/diff/47001/content/renderer/media/aud... File content/renderer/media/audio_device.h (right): http://codereview.chromium.org/10537121/diff/47001/content/renderer/media/aud... content/renderer/media/audio_device.h:79: class AudioMessageFilter; On 2012/06/26 20:28:51, henrika wrote: > To me the existing change still makes sense. It feels odd to have a contract > where Initialize() can be optional. Agree? If no one is using the 2-arg AD() ctor I don't see why we should keep it in -- I agree w/ henrika@ that optional Initialize() smells a bit funny http://codereview.chromium.org/10537121/diff/47001/content/renderer/media/aud... content/renderer/media/audio_device.h:79: class AudioMessageFilter; nit: this fwd decl can be removed as we #include a_m_f.h http://codereview.chromium.org/10537121/diff/47001/content/renderer/media/aud... content/renderer/media/audio_device.h:92: // Minimal constructors where Initialize() must be called later. If we're going w/ the single ctor then I'd rephrase this to say "Creates an uninitialized AudioDevice. Clients must call Initialize() before using." or something like that http://codereview.chromium.org/10537121/diff/47001/content/renderer/media/aud... File content/renderer/media/audio_device_factory.h (right): http://codereview.chromium.org/10537121/diff/47001/content/renderer/media/aud... content/renderer/media/audio_device_factory.h:15: new content code (and existing code if you're touching it!) should go in the content namespace check out https://groups.google.com/a/chromium.org/forum/?fromgroups#!topic/chromium-de... http://codereview.chromium.org/10537121/diff/47001/content/renderer/media/web... File content/renderer/media/webrtc_audio_device_impl.cc (right): http://codereview.chromium.org/10537121/diff/47001/content/renderer/media/web... content/renderer/media/webrtc_audio_device_impl.cc:144: DVLOG(1) << "WebRtcAudioDeviceImpl::WebRtcAudioDeviceImpl()"; can you fix the indentation here? this code should be 2-space indent
Moved factory class to content as Andrew suggested. Resulted in some minor changes. http://codereview.chromium.org/10537121/diff/47001/content/renderer/media/aud... File content/renderer/media/audio_device.cc (right): http://codereview.chromium.org/10537121/diff/47001/content/renderer/media/aud... content/renderer/media/audio_device.cc:14: #include "content/renderer/media/audio_message_filter.h" On 2012/06/26 23:34:30, scherkus wrote: > nit: we include this in the .h Done. http://codereview.chromium.org/10537121/diff/47001/content/renderer/media/aud... File content/renderer/media/audio_device.h (right): http://codereview.chromium.org/10537121/diff/47001/content/renderer/media/aud... content/renderer/media/audio_device.h:79: class AudioMessageFilter; On 2012/06/26 19:05:15, Chris Rogers wrote: > It seems like you can probably just revert all the changes in this file since > you've now removed the AudioMessageFilter constructor. Done. http://codereview.chromium.org/10537121/diff/47001/content/renderer/media/aud... content/renderer/media/audio_device.h:79: class AudioMessageFilter; On 2012/06/26 23:34:30, scherkus wrote: > On 2012/06/26 20:28:51, henrika wrote: > > To me the existing change still makes sense. It feels odd to have a contract > > where Initialize() can be optional. Agree? > > If no one is using the 2-arg AD() ctor I don't see why we should keep it in -- I > agree w/ henrika@ that optional Initialize() smells a bit funny Done. http://codereview.chromium.org/10537121/diff/47001/content/renderer/media/aud... content/renderer/media/audio_device.h:79: class AudioMessageFilter; On 2012/06/26 23:34:30, scherkus wrote: > nit: this fwd decl can be removed as we #include a_m_f.h Done. http://codereview.chromium.org/10537121/diff/47001/content/renderer/media/aud... content/renderer/media/audio_device.h:92: // Minimal constructors where Initialize() must be called later. Thanks. We can add a 2nd ctor at a later stage instead. http://codereview.chromium.org/10537121/diff/47001/content/renderer/media/aud... File content/renderer/media/audio_device_factory.h (right): http://codereview.chromium.org/10537121/diff/47001/content/renderer/media/aud... content/renderer/media/audio_device_factory.h:15: Thanks. Will fix. http://codereview.chromium.org/10537121/diff/47001/content/renderer/media/aud... File content/renderer/media/audio_message_filter.cc (right): http://codereview.chromium.org/10537121/diff/47001/content/renderer/media/aud... content/renderer/media/audio_message_filter.cc:25: // DCHECK(RenderThreadImpl::current()) << On 2012/06/26 15:55:48, henrika wrote: > Will remove this part to simplify unit testing. Done. http://codereview.chromium.org/10537121/diff/47001/content/renderer/media/web... File content/renderer/media/webrtc_audio_device_impl.cc (right): http://codereview.chromium.org/10537121/diff/47001/content/renderer/media/web... content/renderer/media/webrtc_audio_device_impl.cc:144: DVLOG(1) << "WebRtcAudioDeviceImpl::WebRtcAudioDeviceImpl()"; On 2012/06/26 23:34:30, scherkus wrote: > can you fix the indentation here? this code should be 2-space indent Done.
http://codereview.chromium.org/10537121/diff/56001/content/renderer/media/aud... File content/renderer/media/audio_device_factory.h (right): http://codereview.chromium.org/10537121/diff/56001/content/renderer/media/aud... content/renderer/media/audio_device_factory.h:22: class AudioDeviceFactory { nit: it's a bit odd that this factory creates the interface but is named after the implementation. It seems it should be AudioRendererSinkFactory and be in audio_renderer_sink_factory.h. http://codereview.chromium.org/10537121/diff/56001/content/renderer/media/aud... content/renderer/media/audio_device_factory.h:30: CONTENT_EXPORT static bool has_factory() { this isn't used, so take it out http://codereview.chromium.org/10537121/diff/56001/content/renderer/media/aud... content/renderer/media/audio_device_factory.h:50: CONTENT_EXPORT static void UnregisterFactory(); nit: you can get rid of the Register and Unregister functions, and instead have this automatically be done in the constructor and destructor. http://codereview.chromium.org/10537121/diff/56001/content/renderer/media/aud... content/renderer/media/audio_device_factory.h:55: CONTENT_EXPORT static AudioDeviceFactory* factory_; don't need CONTENT_EXPORT here http://codereview.chromium.org/10537121/diff/56001/content/renderer/media/aud... File content/renderer/media/audio_message_filter.h (right): http://codereview.chromium.org/10537121/diff/56001/content/renderer/media/aud... content/renderer/media/audio_message_filter.h:43: static AudioMessageFilter* current(); per style guide, since the implementation is in the cc file this should be CamelCase, i.e. Get(). The current() naming for RenderThread where I assume you got this is incorrect (it was a holdover from when it was inlined) and just needs to update all the code. http://codereview.chromium.org/10537121/diff/56001/content/renderer/media/aud... content/renderer/media/audio_message_filter.h:82: // Should only be accessed on the main renderer thread. nit: i thought the whole point of exposing the getter above is that it'll called on multiple threads?
On 2012/06/27 15:56:28, John Abd-El-Malek wrote: > http://codereview.chromium.org/10537121/diff/56001/content/renderer/media/aud... > File content/renderer/media/audio_device_factory.h (right): > > http://codereview.chromium.org/10537121/diff/56001/content/renderer/media/aud... > content/renderer/media/audio_device_factory.h:22: class AudioDeviceFactory { > nit: it's a bit odd that this factory creates the interface but is named after > the implementation. It seems it should be AudioRendererSinkFactory and be in > audio_renderer_sink_factory.h. I think that Henrik wants a factory only for things which act specifically like AudioDevices. AudioRendererSink is a *very* general interface and there are several other concreate classes implementing this interface (like AudioRendererImpl, RenderAudioSourceProvider, and the new audio-mixing work introduces even more), so I'm pretty sure we can't have such a general factory. We *could* introduce an intermediate interface called AudioDeviceInterface which is an AudioRendererSink, I suppose, but not sure if it's worth it, so I think that's why Henrik used AudioDevice directly.
John: Replied but did not make the changes. Will fix tomorrow. Please check my comments just in case. Thanks. http://codereview.chromium.org/10537121/diff/56001/content/renderer/media/aud... File content/renderer/media/audio_device_factory.h (right): http://codereview.chromium.org/10537121/diff/56001/content/renderer/media/aud... content/renderer/media/audio_device_factory.h:22: class AudioDeviceFactory { It's a very good comment. I will improve it. Thanks. http://codereview.chromium.org/10537121/diff/56001/content/renderer/media/aud... content/renderer/media/audio_device_factory.h:30: CONTENT_EXPORT static bool has_factory() { Will do. http://codereview.chromium.org/10537121/diff/56001/content/renderer/media/aud... content/renderer/media/audio_device_factory.h:50: CONTENT_EXPORT static void UnregisterFactory(); That's a smart observation. Note, also applies to RenderViewHostFactory. http://codereview.chromium.org/10537121/diff/56001/content/renderer/media/aud... content/renderer/media/audio_device_factory.h:55: CONTENT_EXPORT static AudioDeviceFactory* factory_; Will do. http://codereview.chromium.org/10537121/diff/56001/content/renderer/media/aud... File content/renderer/media/audio_message_filter.h (right): http://codereview.chromium.org/10537121/diff/56001/content/renderer/media/aud... content/renderer/media/audio_message_filter.h:43: static AudioMessageFilter* current(); Got it. Not GetCurrent() then? http://codereview.chromium.org/10537121/diff/56001/content/renderer/media/aud... content/renderer/media/audio_message_filter.h:82: // Should only be accessed on the main renderer thread. I do need a vacation. Thanks.
http://codereview.chromium.org/10537121/diff/56001/content/renderer/media/aud... File content/renderer/media/audio_message_filter.h (right): http://codereview.chromium.org/10537121/diff/56001/content/renderer/media/aud... content/renderer/media/audio_message_filter.h:43: static AudioMessageFilter* current(); On 2012/06/27 17:54:47, henrika wrote: > Got it. Not GetCurrent() then? current() was named like because initially there was a thought that there would be different RenderThread objects in the same renderer process, so current() would give the current one for the given thread. but that never panned out. Get() is more consistent with getters
On 2012/06/27 17:46:17, Chris Rogers wrote: > On 2012/06/27 15:56:28, John Abd-El-Malek wrote: > > > http://codereview.chromium.org/10537121/diff/56001/content/renderer/media/aud... > > File content/renderer/media/audio_device_factory.h (right): > > > > > http://codereview.chromium.org/10537121/diff/56001/content/renderer/media/aud... > > content/renderer/media/audio_device_factory.h:22: class AudioDeviceFactory { > > nit: it's a bit odd that this factory creates the interface but is named after > > the implementation. It seems it should be AudioRendererSinkFactory and be in > > audio_renderer_sink_factory.h. > > I think that Henrik wants a factory only for things which act specifically like > AudioDevices. AudioRendererSink is a *very* general interface and there are > several other concreate classes implementing this interface (like > AudioRendererImpl, RenderAudioSourceProvider, and the new audio-mixing work > introduces even more), so I'm pretty sure we can't have such a general factory. > > We *could* introduce an intermediate interface called AudioDeviceInterface which > is an AudioRendererSink, I suppose, but not sure if it's worth it, so I think > that's why Henrik used AudioDevice directly. I think adding another interface, just to wrap AudioRendererSink but adds nothing to it, will make reading the code harder. I don't quite understand by your explanation, i.e. what does it mean to act specifically like AudioDevice if this will be used for testing? I assume the testing class will be quite different.
On 2012/06/27 18:06:18, John Abd-El-Malek wrote: > On 2012/06/27 17:46:17, Chris Rogers wrote: > > On 2012/06/27 15:56:28, John Abd-El-Malek wrote: > > > > > > http://codereview.chromium.org/10537121/diff/56001/content/renderer/media/aud... > > > File content/renderer/media/audio_device_factory.h (right): > > > > > > > > > http://codereview.chromium.org/10537121/diff/56001/content/renderer/media/aud... > > > content/renderer/media/audio_device_factory.h:22: class AudioDeviceFactory { > > > nit: it's a bit odd that this factory creates the interface but is named > after > > > the implementation. It seems it should be AudioRendererSinkFactory and be in > > > audio_renderer_sink_factory.h. > > > > I think that Henrik wants a factory only for things which act specifically > like > > AudioDevices. AudioRendererSink is a *very* general interface and there are > > several other concreate classes implementing this interface (like > > AudioRendererImpl, RenderAudioSourceProvider, and the new audio-mixing work > > introduces even more), so I'm pretty sure we can't have such a general > factory. > > > > We *could* introduce an intermediate interface called AudioDeviceInterface > which > > is an AudioRendererSink, I suppose, but not sure if it's worth it, so I think > > that's why Henrik used AudioDevice directly. > > I think adding another interface, just to wrap AudioRendererSink but adds > nothing to it, will make reading the code harder. I agree. > > I don't quite understand by your explanation, i.e. what does it mean to act > specifically like AudioDevice if this will be used for testing? I assume the > testing class will be quite different. I was just hoping the name of the factory and create method would have the word AudioDevice in it, because this factory (in its default/normal implementation) has nothing to do with the other types of AudioRendererSinks, and is specifically designed to return something that talks with the audio hardware. I think that AudioRendererSinkFactory is too general of a name.
Uploaded new patch taking all feedback from John into account. Still have not changed the name of the factory class. http://codereview.chromium.org/10537121/diff/56001/content/renderer/media/aud... File content/renderer/media/audio_device_factory.h (right): http://codereview.chromium.org/10537121/diff/56001/content/renderer/media/aud... content/renderer/media/audio_device_factory.h:30: CONTENT_EXPORT static bool has_factory() { On 2012/06/27 17:54:47, henrika wrote: > Will do. Done. http://codereview.chromium.org/10537121/diff/56001/content/renderer/media/aud... content/renderer/media/audio_device_factory.h:50: CONTENT_EXPORT static void UnregisterFactory(); On 2012/06/27 17:54:47, henrika wrote: > That's a smart observation. Note, also applies to RenderViewHostFactory. Done. http://codereview.chromium.org/10537121/diff/56001/content/renderer/media/aud... content/renderer/media/audio_device_factory.h:55: CONTENT_EXPORT static AudioDeviceFactory* factory_; On 2012/06/27 17:54:47, henrika wrote: > Will do. Done. http://codereview.chromium.org/10537121/diff/56001/content/renderer/media/aud... File content/renderer/media/audio_message_filter.h (right): http://codereview.chromium.org/10537121/diff/56001/content/renderer/media/aud... content/renderer/media/audio_message_filter.h:43: static AudioMessageFilter* current(); On 2012/06/27 18:03:58, John Abd-El-Malek wrote: > On 2012/06/27 17:54:47, henrika wrote: > > Got it. Not GetCurrent() then? > > current() was named like because initially there was a thought that there would > be different RenderThread objects in the same renderer process, so current() > would give the current one for the given thread. but that never panned out. > > Get() is more consistent with getters Done. http://codereview.chromium.org/10537121/diff/56001/content/renderer/media/aud... content/renderer/media/audio_message_filter.h:82: // Should only be accessed on the main renderer thread. On 2012/06/27 15:56:29, John Abd-El-Malek wrote: > nit: i thought the whole point of exposing the getter above is that it'll called > on multiple threads? Done.
lgtm because I don't want to hold up this patch longer. > > I was just hoping the name of the factory and create method would have the word > AudioDevice in it, because > this factory (in its default/normal implementation) has nothing to do with the > other types of AudioRendererSinks, and is specifically designed to return > something that talks with the audio hardware. I think that > AudioRendererSinkFactory is too general of a name. I'm still not following :) If it's returning a generic type, why shouldn't it have a generic name? It's unintuitive that a factory for an interface is named after one implementation. If it only returns AudioDevices, then there would be no need for a factory. I would prefer that this is named after the object that it returns, but not strongly enough to hold off this patch.
Chris, if you are fine with the proposal from John, I can make that last change tomorrow and commit. Andrew: I guess I need your OK as well. Not sure about the exact "rules" here.
Henrik: I'm happy with the patch (except for changing the factory name) John: we can talk offline and I can hopefully try to better explain what I mean exactly On 2012/06/28 15:07:12, henrika wrote: > Chris, if you are fine with the proposal from John, I can make that last change > tomorrow and commit. > > Andrew: I guess I need your OK as well. Not sure about the exact "rules" here.
LGTM % any last minute naming changes you want to do :) |
