Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2821203005:
Add a mojo implementation of AudioOutputIPC. (Closed)
|
Created:
3 years, 8 months ago by Max Morin Modified:
3 years, 6 months ago CC:
Aaron Boodman, abarth-chromium, alokp+watch_chromium.org, audio-mojo-cl_google.com, chromium-reviews, creis+watch_chromium.org, darin (slow to review), darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, ossu-chromium, posciak+watch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a mojo implementation of AudioOutputIPC.
A couple of tricky points:
* The requirement to always call CloseStream is added to
media/audio/audio_output_ipc.h. This requirement isn't new, but it
wasn't documented before.
* If the RendererAudioOutputStreamFactory used by MojoAudioOutputIPC is
destroyed prior to receiving authorization (e.g. because of frame
destruction), we need to signal the AudioOutputDevice anyways to
avoid waiting forever. We pass a ScopedClosureRunner into the
authorization callback to make sure we get a callback even if the
factory is destroyed early.
* Code for intergrating this into the AudioDeviceFactory is in
https://codereview.chromium.org/2890753003
* Browser tests (using this and the dependent CL) in
https://codereview.chromium.org/2812883003/#ps380001.
BUG=425368
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Review-Url: https://codereview.chromium.org/2821203005
Cr-Commit-Position: refs/heads/master@{#475827}
Committed: https://chromium.googlesource.com/chromium/src/+/aa9020e577bcd1f459094168b00a07aaba063a9f
Patch Set 1 #
Total comments: 49
Patch Set 2 : Comments, fix issue with factory disconnected #Patch Set 3 : Fix test issues #
Total comments: 71
Patch Set 4 : Comments. SWITCH TO MACRO-BASED THREAD CHECKER. #
Total comments: 2
Patch Set 5 : Comments. Tests! #Patch Set 6 : Move factory to separate CL #Patch Set 7 : rebase #
Total comments: 22
Patch Set 8 : Death tests, Olga's comments #
Total comments: 19
Patch Set 9 : Use ScopedClosureRunner. #
Dependent Patchsets: Messages
Total messages: 84 (45 generated)
Description was changed from ========== Add mojo implementation of AudioOutputIPC. * Add a new feature UseMojoAudioOutputStreamFactory to control whether Mojo or Chrome IPC AudioOutputIPC implementations are created. * When processing an authorization request, it is not safe to use other threads than the IO thread, as they may be blocked waiting for the response. Thus, AudioIPCFactory is introduced to hold a mapping (frame id->remote factory) on the IO thread. This also means we have to create/destruct the RendererAudioOutputStreamFactoryPtr with the RenderFrameImpl instead of the preferrable way of doing it lazily. This can be revisited if we stop blocking while waiting for authorization (crbug.com/668275). * For running browser tests with this, the browser-side glue code is needed. See the dependent CL. I have verified that the browser tests pass. As the pepper code doesn't seem well covered by tests, I also verified that flash sound output works manually. BUG=425368 ========== to ========== Add a mojo implementation of AudioOutputIPC. * Add a new feature UseMojoAudioOutputStreamFactory to control whether Mojo or Chrome IPC AudioOutputIPC implementations are created. * When processing an authorization request, it is not safe to use other threads than the IO thread, as they may be blocked waiting for the response. Thus, AudioIPCFactory is introduced to hold a mapping (frame id->remote factory) on the IO thread. This also means we have to create/destruct the RendererAudioOutputStreamFactoryPtr with the RenderFrameImpl instead of the preferrable way of doing it lazily. This can be revisited if we stop blocking while waiting for authorization (crbug.com/668275). * For running browser tests with this, the browser-side glue code is needed. See the dependent CL. I have verified that the browser tests pass. As the pepper code doesn't seem well covered by tests, I also manually verified that flash sound output works. BUG=425368 ==========
maxmorin@chromium.org changed reviewers: + olka@chromium.org
Olga: This code isn't unit tested but is otherwise ready for review. Could you take a look?
Description was changed from
==========
Add a mojo implementation of AudioOutputIPC.
* Add a new feature UseMojoAudioOutputStreamFactory to control whether
Mojo or Chrome IPC AudioOutputIPC implementations are created.
* When processing an authorization request, it is not safe to use other
threads than the IO thread, as they may be blocked waiting for the
response. Thus, AudioIPCFactory is introduced to hold a mapping
(frame id->remote factory) on the IO thread. This also means we have
to create/destruct the RendererAudioOutputStreamFactoryPtr with the
RenderFrameImpl instead of the preferrable way of doing it lazily.
This can be revisited if we stop blocking while waiting for
authorization (crbug.com/668275).
* For running browser tests with this, the browser-side glue code is
needed. See the dependent CL. I have verified that the browser tests
pass. As the pepper code doesn't seem well covered by tests, I also
manually verified that flash sound output works.
BUG=425368
==========
to
==========
Add a mojo implementation of AudioOutputIPC.
* Add a new feature UseMojoAudioOutputStreamFactory to control whether
Mojo or Chrome IPC AudioOutputIPC implementations are created.
* When processing an authorization request, it is not safe to use other
threads than the IO thread, as they may be blocked waiting for the
response. Thus, AudioIPCFactory is introduced to hold a mapping
(frame id->remote factory) on the IO thread. This also means we have
to create/destruct the RendererAudioOutputStreamFactoryPtr with the
RenderFrameImpl instead of the preferrable way of doing it lazily.
This can be revisited if we stop blocking while waiting for
authorization (crbug.com/668275).
* For running browser tests with this, the browser-side glue code is
needed. See the dependent CL. I have verified that the browser tests
pass. As the pepper code doesn't seem well covered by tests, I also
manually verified that flash sound output works.
* Suggested reading order:
- content/renderer/media/mojo_audio_output_ipc.{h,cc}.
- content/renderer/media/*.
- whatever else that seems interesting.
BUG=425368
==========
Only structural comments so far, but I'm not sure I followed all the paths yet. https://codereview.chromium.org/2821203005/diff/1/content/public/common/conte... File content/public/common/content_features.cc (right): https://codereview.chromium.org/2821203005/diff/1/content/public/common/conte... content/public/common/content_features.cc:232: "UseMojoAudioOutputStreamFactory", base::FEATURE_DISABLED_BY_DEFAULT}; This is "enabling a feature behind a flag", so it needs to be mentioned in the CL description. I would do it in a separate CL though (see https://codereview.chromium.org/2108673004 as an example of switching to a feature later): it would be cheaper to revert. Do we want to have separate experiments for input and output? Since mirroring connects outputs with inputs they are not totally independent. We'll end up with 4 experimental subsets instead of 2. https://codereview.chromium.org/2821203005/diff/1/content/renderer/media/audi... File content/renderer/media/audio_ipc_factory.cc (right): https://codereview.chromium.org/2821203005/diff/1/content/renderer/media/audi... content/renderer/media/audio_ipc_factory.cc:25: DCHECK(instance_); DCHECK_EQ(instance_, this); https://codereview.chromium.org/2821203005/diff/1/content/renderer/media/audi... content/renderer/media/audio_ipc_factory.cc:42: FROM_HERE, base::Bind(&AudioIPCFactory::RegisterRemoteFactoryOnIOThread, Just post RegisterRemoteFactory() to IO thread if it's not there? https://codereview.chromium.org/2821203005/diff/1/content/renderer/media/audi... content/renderer/media/audio_ipc_factory.cc:51: FROM_HERE, base::Bind(&AudioIPCFactory::DeregisterRemoteFactoryOnIOThread, Same here https://codereview.chromium.org/2821203005/diff/1/content/renderer/media/audi... content/renderer/media/audio_ipc_factory.cc:57: auto it = factory_ptrs_.find(frame_id); Thread check? https://codereview.chromium.org/2821203005/diff/1/content/renderer/media/audi... File content/renderer/media/audio_ipc_factory.h (right): https://codereview.chromium.org/2821203005/diff/1/content/renderer/media/audi... content/renderer/media/audio_ipc_factory.h:24: class AudioIPCFactory { RendererAudioIPCFactory? https://codereview.chromium.org/2821203005/diff/1/content/renderer/media/audi... content/renderer/media/audio_ipc_factory.h:32: static AudioIPCFactory* get() { Get(), and move implementation to .cc? https://codereview.chromium.org/2821203005/diff/1/content/renderer/media/audi... content/renderer/media/audio_ipc_factory.h:41: // Enables |this| to create AudioOutputIPCs for the specified frame. It is Looking at CreateAudioOutputIPC() it's not clear how it enables it? https://codereview.chromium.org/2821203005/diff/1/content/renderer/media/audi... content/renderer/media/audio_ipc_factory.h:42: // automatically deregistered on frame destruction. What does "automatically" mean here? Doesn't frame call DeregisterRemoteFactory() on destruction? https://codereview.chromium.org/2821203005/diff/1/content/renderer/media/audi... content/renderer/media/audio_ipc_factory.h:43: void RegisterRemoteFactory( "RemoteFactory" sounds confusing, it's hard to associate it with any concept by this name. "FrameStreamFactory" maybe? Also, do you plan to overload it for input? https://codereview.chromium.org/2821203005/diff/1/content/renderer/media/audi... content/renderer/media/audio_ipc_factory.h:54: mojom::RendererAudioOutputStreamFactory* GetRemoteFactory(int frame_id); It's used by MojoAudioOutputIPC only, right? WDYT of passing it as a callback instead of exposing it as public? https://codereview.chromium.org/2821203005/diff/1/content/renderer/media/mojo... File content/renderer/media/mojo_audio_output_ipc.cc (right): https://codereview.chromium.org/2821203005/diff/1/content/renderer/media/mojo... content/renderer/media/mojo_audio_output_ipc.cc:31: // the final call to CloseStream. Could you please clarify the comment? https://codereview.chromium.org/2821203005/diff/1/content/renderer/media/mojo... content/renderer/media/mojo_audio_output_ipc.cc:31: // the final call to CloseStream. * if CloseStream() has been called. DCHECKs for that? https://codereview.chromium.org/2821203005/diff/1/content/renderer/media/mojo... content/renderer/media/mojo_audio_output_ipc.cc:85: if (!stream_provider_.is_bound()) { M.b. would be more readable to have AuthorizationRequested()/StreamCreationRequested()? https://codereview.chromium.org/2821203005/diff/1/content/renderer/media/mojo... content/renderer/media/mojo_audio_output_ipc.cc:95: factory->RequestDeviceAuthorization( WDYT about making a helper function and using it here and in RequestDeviceAuthorization() ? https://codereview.chromium.org/2821203005/diff/1/content/renderer/media/mojo... content/renderer/media/mojo_audio_output_ipc.cc:101: // Since the callback won't fire if the binding is gone, unretained is safe. Which callback and which binding? It's hard for an unprepared reader :) https://codereview.chromium.org/2821203005/diff/1/content/renderer/media/mojo... content/renderer/media/mojo_audio_output_ipc.cc:150: // Make sure we don't send an authorization callback for this stream to the We need a comment explaining why we are never stuck waiting for authorization. (Either authorization is signaled or OnIPCClosed() is called on every path?) https://codereview.chromium.org/2821203005/diff/1/content/renderer/media/mojo... File content/renderer/media/mojo_audio_output_ipc.h (right): https://codereview.chromium.org/2821203005/diff/1/content/renderer/media/mojo... content/renderer/media/mojo_audio_output_ipc.h:19: // initialization and control of an output stream. Could you add a comment on threading (it can run on IO thread only, the limitation comes from AudioIPCFactory::GetRemoteFactory() )? (and maybe in media::AudioOutputIPC as well) https://codereview.chromium.org/2821203005/diff/1/content/renderer/media/mojo... content/renderer/media/mojo_audio_output_ipc.h:51: base::ThreadChecker thread_checker_; it can run on AudioIPCFactory::io_task_runner only, right? https://codereview.chromium.org/2821203005/diff/1/content/renderer/pepper/pep... File content/renderer/pepper/pepper_platform_audio_output.cc (right): https://codereview.chromium.org/2821203005/diff/1/content/renderer/pepper/pep... content/renderer/pepper/pepper_platform_audio_output.cc:141: features::kUseMojoAudioOutputStreamFactory)) { Instead of doing it here and in AOD separately, can we hide it inside AudioIPCFactory? https://codereview.chromium.org/2821203005/diff/1/content/renderer/render_fra... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2821203005/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:1225: if (base::FeatureList::IsEnabled( Hide register/deregister logic in static methods of AudioIPCFactory? https://codereview.chromium.org/2821203005/diff/1/content/renderer/render_thr... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2821203005/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:679: audio_ipc_factory_.emplace(GetIOTaskRunner()); I'd prefer all the notion of kUseMojoAudioOutputStreamFactory to be hidden in IPC factory. audio_ipc_factory_ = AudioIPCFactory::Create(GetIOTaskRunner()); // no need for optional if (!audio_ipc_factory_) { // create & reg a filter } https://codereview.chromium.org/2821203005/diff/1/content/renderer/render_thr... File content/renderer/render_thread_impl.h (right): https://codereview.chromium.org/2821203005/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.h:644: base::Optional<AudioIPCFactory> audio_ipc_factory_; unique_ptr? Comment for it and |audio_message_filter_| than only one of those is non-null?
Patchset #2 (id:20001) has been deleted
New revision. There is now a mechanism for ensuring that the authorization callback is always called (CallbackWrapper in MojoAudioOutputIPC). AudioIPCFactory can now create either an old AudioOutputIPC or the new mojo impl. Currently, the old way is hard-coded. In addition to this, all the things you mentioned. :) Left to do: Nicer way to obtain the mojo factories. I'll see what I can do about that. PTAL. https://codereview.chromium.org/2821203005/diff/1/content/public/common/conte... File content/public/common/content_features.cc (right): https://codereview.chromium.org/2821203005/diff/1/content/public/common/conte... content/public/common/content_features.cc:232: "UseMojoAudioOutputStreamFactory", base::FEATURE_DISABLED_BY_DEFAULT}; On 2017/04/20 10:35:59, o1ka wrote: > This is "enabling a feature behind a flag", so it needs to be mentioned in the > CL description. > > I would do it in a separate CL though (see > https://codereview.chromium.org/2108673004 as an example of switching to a > feature later): it would be cheaper to revert. I removed the feature. > > Do we want to have separate experiments for input and output? Since mirroring > connects outputs with inputs they are not totally independent. We'll end up with > 4 experimental subsets instead of 2. The hope is that this will be done before input. :) https://codereview.chromium.org/2821203005/diff/1/content/renderer/media/audi... File content/renderer/media/audio_ipc_factory.cc (right): https://codereview.chromium.org/2821203005/diff/1/content/renderer/media/audi... content/renderer/media/audio_ipc_factory.cc:25: DCHECK(instance_); On 2017/04/20 10:35:59, o1ka wrote: > DCHECK_EQ(instance_, this); Done. https://codereview.chromium.org/2821203005/diff/1/content/renderer/media/audi... content/renderer/media/audio_ipc_factory.cc:42: FROM_HERE, base::Bind(&AudioIPCFactory::RegisterRemoteFactoryOnIOThread, On 2017/04/20 10:35:59, o1ka wrote: > Just post RegisterRemoteFactory() to IO thread if it's not there? The functions have different signatures :). https://codereview.chromium.org/2821203005/diff/1/content/renderer/media/audi... content/renderer/media/audio_ipc_factory.cc:51: FROM_HERE, base::Bind(&AudioIPCFactory::DeregisterRemoteFactoryOnIOThread, On 2017/04/20 10:35:59, o1ka wrote: > Same here Changed it here. https://codereview.chromium.org/2821203005/diff/1/content/renderer/media/audi... content/renderer/media/audio_ipc_factory.cc:57: auto it = factory_ptrs_.find(frame_id); On 2017/04/20 10:35:59, o1ka wrote: > Thread check? Done. https://codereview.chromium.org/2821203005/diff/1/content/renderer/media/audi... File content/renderer/media/audio_ipc_factory.h (right): https://codereview.chromium.org/2821203005/diff/1/content/renderer/media/audi... content/renderer/media/audio_ipc_factory.h:24: class AudioIPCFactory { On 2017/04/20 10:35:59, o1ka wrote: > RendererAudioIPCFactory? Most stuff in renderer/ is not prefixed renderer. It seems redundant to me. https://codereview.chromium.org/2821203005/diff/1/content/renderer/media/audi... content/renderer/media/audio_ipc_factory.h:32: static AudioIPCFactory* get() { On 2017/04/20 10:35:59, o1ka wrote: > Get(), and move implementation to .cc? I couldn't find the rule against this in the style guide, checking https://google.github.io/styleguide/cppguide.html#Inline_Functions and https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md.... https://www.chromium.org/developers/coding-style/cpp-dos-and-donts mentions simple accessors. This is as simple as a function could be, and also an accessor, so it should be fine. https://codereview.chromium.org/2821203005/diff/1/content/renderer/media/audi... content/renderer/media/audio_ipc_factory.h:41: // Enables |this| to create AudioOutputIPCs for the specified frame. It is On 2017/04/20 10:35:59, o1ka wrote: > Looking at CreateAudioOutputIPC() it's not clear how it enables it? Does it matter? https://codereview.chromium.org/2821203005/diff/1/content/renderer/media/audi... content/renderer/media/audio_ipc_factory.h:42: // automatically deregistered on frame destruction. On 2017/04/20 10:35:59, o1ka wrote: > What does "automatically" mean here? Doesn't frame call > DeregisterRemoteFactory() on destruction? Right, I had to change this and didn't update the comment :). https://codereview.chromium.org/2821203005/diff/1/content/renderer/media/audi... content/renderer/media/audio_ipc_factory.h:43: void RegisterRemoteFactory( On 2017/04/20 10:35:59, o1ka wrote: > "RemoteFactory" sounds confusing, it's hard to associate it with any concept by > this name. "FrameStreamFactory" maybe? A remote factory is a factory that is remote, in the sense of being in another process. "FrameStreamFactory" can be associated with frames and streams, but the most important thing about the factory is that it's the one in the browser process. "IPCStreamFactory" would make sense, but it's almost the same as the name for this class. WDYT of MojoStreamFactory? > Also, do you plan to overload it for input? I wouldn't overload it, I suppose it's open if input should go into this class or in a separate one. If it goes into this class, I will put "Input" or "Output" in all the names to clarify. https://codereview.chromium.org/2821203005/diff/1/content/renderer/media/audi... content/renderer/media/audio_ipc_factory.h:54: mojom::RendererAudioOutputStreamFactory* GetRemoteFactory(int frame_id); On 2017/04/20 10:35:59, o1ka wrote: > It's used by MojoAudioOutputIPC only, right? > WDYT of passing it as a callback instead of exposing it as public? Done. https://codereview.chromium.org/2821203005/diff/1/content/renderer/media/mojo... File content/renderer/media/mojo_audio_output_ipc.cc (right): https://codereview.chromium.org/2821203005/diff/1/content/renderer/media/mojo... content/renderer/media/mojo_audio_output_ipc.cc:31: // the final call to CloseStream. On 2017/04/20 10:36:00, o1ka wrote: > Could you please clarify the comment? Done. https://codereview.chromium.org/2821203005/diff/1/content/renderer/media/mojo... content/renderer/media/mojo_audio_output_ipc.cc:31: // the final call to CloseStream. On 2017/04/20 10:36:00, o1ka wrote: > * if CloseStream() has been called. DCHECKs for that? Done. https://codereview.chromium.org/2821203005/diff/1/content/renderer/media/mojo... content/renderer/media/mojo_audio_output_ipc.cc:85: if (!stream_provider_.is_bound()) { On 2017/04/20 10:35:59, o1ka wrote: > M.b. would be more readable to have > AuthorizationRequested()/StreamCreationRequested()? Done. https://codereview.chromium.org/2821203005/diff/1/content/renderer/media/mojo... content/renderer/media/mojo_audio_output_ipc.cc:95: factory->RequestDeviceAuthorization( On 2017/04/20 10:35:59, o1ka wrote: > WDYT about making a helper function and using it here and in > RequestDeviceAuthorization() ? Looks more complex (since it returns early if there is no factory, there would have to be some logic to take care of that). https://codereview.chromium.org/2821203005/diff/1/content/renderer/media/mojo... content/renderer/media/mojo_audio_output_ipc.cc:101: // Since the callback won't fire if the binding is gone, unretained is safe. On 2017/04/20 10:35:59, o1ka wrote: > Which callback and which binding? It's hard for an unprepared reader :) Done. https://codereview.chromium.org/2821203005/diff/1/content/renderer/media/mojo... content/renderer/media/mojo_audio_output_ipc.cc:150: // Make sure we don't send an authorization callback for this stream to the On 2017/04/20 10:36:00, o1ka wrote: > We need a comment explaining why we are never stuck waiting for authorization. > (Either authorization is signaled or OnIPCClosed() is called on every path?) In this case, the AudioOutputDevice must take care of it. In general, see comment in RequestDeviceAuthorization. https://codereview.chromium.org/2821203005/diff/1/content/renderer/media/mojo... File content/renderer/media/mojo_audio_output_ipc.h (right): https://codereview.chromium.org/2821203005/diff/1/content/renderer/media/mojo... content/renderer/media/mojo_audio_output_ipc.h:19: // initialization and control of an output stream. On 2017/04/20 10:36:00, o1ka wrote: > Could you add a comment on threading (it can run on IO thread only, the > limitation comes from AudioIPCFactory::GetRemoteFactory() )? > (and maybe in media::AudioOutputIPC as well) In the current version, it may only be used on a single thread. (In practice this is the io thread since the factory accessor may only be called from the io thread, but this not really a property of this class). https://codereview.chromium.org/2821203005/diff/1/content/renderer/media/mojo... content/renderer/media/mojo_audio_output_ipc.h:51: base::ThreadChecker thread_checker_; On 2017/04/20 10:36:00, o1ka wrote: > it can run on AudioIPCFactory::io_task_runner only, right? See above. https://codereview.chromium.org/2821203005/diff/1/content/renderer/pepper/pep... File content/renderer/pepper/pepper_platform_audio_output.cc (right): https://codereview.chromium.org/2821203005/diff/1/content/renderer/pepper/pep... content/renderer/pepper/pepper_platform_audio_output.cc:141: features::kUseMojoAudioOutputStreamFactory)) { On 2017/04/20 10:36:00, o1ka wrote: > Instead of doing it here and in AOD separately, can we hide it inside > AudioIPCFactory? I changed this, PTAL. https://codereview.chromium.org/2821203005/diff/1/content/renderer/render_fra... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2821203005/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:1225: if (base::FeatureList::IsEnabled( On 2017/04/20 10:36:00, o1ka wrote: > Hide register/deregister logic in static methods of AudioIPCFactory? Done. https://codereview.chromium.org/2821203005/diff/1/content/renderer/render_thr... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2821203005/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:679: audio_ipc_factory_.emplace(GetIOTaskRunner()); On 2017/04/20 10:36:00, o1ka wrote: > I'd prefer all the notion of kUseMojoAudioOutputStreamFactory to be hidden in > IPC factory. > audio_ipc_factory_ = AudioIPCFactory::Create(GetIOTaskRunner()); // no need for > optional > if (!audio_ipc_factory_) { > // create & reg a filter > } I didn't do exactly this, PTAL. https://codereview.chromium.org/2821203005/diff/1/content/renderer/render_thr... File content/renderer/render_thread_impl.h (right): https://codereview.chromium.org/2821203005/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.h:644: base::Optional<AudioIPCFactory> audio_ipc_factory_; On 2017/04/20 10:36:00, o1ka wrote: > unique_ptr? > > Comment for it and |audio_message_filter_| than only one of those is non-null? Changed to be a member.
https://codereview.chromium.org/2821203005/diff/1/content/renderer/render_thr... File content/renderer/render_thread_impl.h (right): https://codereview.chromium.org/2821203005/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.h:644: base::Optional<AudioIPCFactory> audio_ipc_factory_; On 2017/05/05 13:10:59, Max Morin wrote: > On 2017/04/20 10:36:00, o1ka wrote: > > unique_ptr? > > > > Comment for it and |audio_message_filter_| than only one of those is non-null? > > Changed to be a member. Oh, actually I left it as an optional since it's not initialized in the constructor. :) https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... File content/renderer/media/mojo_audio_output_ipc.h (right): https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/mojo_audio_output_ipc.h:26: // |factory_accessor| is required to provide a Ignore stale comment, will change it to a better one. :)
Description was changed from
==========
Add a mojo implementation of AudioOutputIPC.
* Add a new feature UseMojoAudioOutputStreamFactory to control whether
Mojo or Chrome IPC AudioOutputIPC implementations are created.
* When processing an authorization request, it is not safe to use other
threads than the IO thread, as they may be blocked waiting for the
response. Thus, AudioIPCFactory is introduced to hold a mapping
(frame id->remote factory) on the IO thread. This also means we have
to create/destruct the RendererAudioOutputStreamFactoryPtr with the
RenderFrameImpl instead of the preferrable way of doing it lazily.
This can be revisited if we stop blocking while waiting for
authorization (crbug.com/668275).
* For running browser tests with this, the browser-side glue code is
needed. See the dependent CL. I have verified that the browser tests
pass. As the pepper code doesn't seem well covered by tests, I also
manually verified that flash sound output works.
* Suggested reading order:
- content/renderer/media/mojo_audio_output_ipc.{h,cc}.
- content/renderer/media/*.
- whatever else that seems interesting.
BUG=425368
==========
to
==========
Add a mojo implementation of AudioOutputIPC.
* Add a new feature UseMojoAudioOutputStreamFactory to control whether
Mojo or Chrome IPC AudioOutputIPC implementations are created.
* When processing an authorization request, it is not safe to use other
threads than the IO thread, as they may be blocked waiting for the
response. Thus, AudioIPCFactory is introduced to hold a mapping
(frame id->remote factory) on the IO thread. This also means we have
to create/destruct the RendererAudioOutputStreamFactoryPtr with the
RenderFrameImpl instead of the preferrable way of doing it lazily.
This can be revisited if we stop blocking while waiting for
authorization (crbug.com/668275).
* For running browser tests with this, the browser-side glue code is
needed. See the dependent CL. I have verified that the browser tests
pass. As the pepper code doesn't seem well covered by tests, I also
manually verified that flash sound output works.
* Suggested reading order:
- content/renderer/media/mojo_audio_output_ipc.{h,cc}.
- content/renderer/media/*.
- whatever else that seems interesting.
There's also a diagram at
https://docs.google.com/drawings/d/1U1e-0gH3WVSV9Td9ZCEcPDBMC7WWWeh58sRYAdXbr...
BUG=425368
==========
I added a diagram to the CL description. I tried to find a good compromise between detail and readability.
maxmorin@chromium.org changed reviewers: + rockot@chromium.org
Ken: Could you take a look at this WIP CL? I have a couple of questions in particular. Do you think the AuthorizationCallbackWrapper in mojo_audio_output_stream.cc makes sense? In the current PS, the RenderFrameImpl creates the RendererAudioOutputStreamFactoryPtr in Initialize, even though it might not be needed, because the main thread can be blocked when we do need it. Do you have some suggestion for how to avoid that?
Description was changed from
==========
Add a mojo implementation of AudioOutputIPC.
* Add a new feature UseMojoAudioOutputStreamFactory to control whether
Mojo or Chrome IPC AudioOutputIPC implementations are created.
* When processing an authorization request, it is not safe to use other
threads than the IO thread, as they may be blocked waiting for the
response. Thus, AudioIPCFactory is introduced to hold a mapping
(frame id->remote factory) on the IO thread. This also means we have
to create/destruct the RendererAudioOutputStreamFactoryPtr with the
RenderFrameImpl instead of the preferrable way of doing it lazily.
This can be revisited if we stop blocking while waiting for
authorization (crbug.com/668275).
* For running browser tests with this, the browser-side glue code is
needed. See the dependent CL. I have verified that the browser tests
pass. As the pepper code doesn't seem well covered by tests, I also
manually verified that flash sound output works.
* Suggested reading order:
- content/renderer/media/mojo_audio_output_ipc.{h,cc}.
- content/renderer/media/*.
- whatever else that seems interesting.
There's also a diagram at
https://docs.google.com/drawings/d/1U1e-0gH3WVSV9Td9ZCEcPDBMC7WWWeh58sRYAdXbr...
BUG=425368
==========
to
==========
Add a mojo implementation of AudioOutputIPC.
* When processing an authorization request, it is not safe to use other
threads than the IO thread, as they may be blocked waiting for the
response. Thus, AudioIPCFactory is introduced to hold a mapping
(frame id->remote factory) on the IO thread. This also means we have
to create/destruct the RendererAudioOutputStreamFactoryPtr with the
RenderFrameImpl instead of the preferrable way of doing it lazily.
This can be revisited if we stop blocking while waiting for
authorization (crbug.com/668275).
* For running browser tests with this, the browser-side glue code is
needed. See the dependent CL. I have verified that the browser tests
pass. As the pepper code doesn't seem well covered by tests, I also
manually verified that flash sound output works.
* Suggested reading order:
- content/renderer/media/mojo_audio_output_ipc.{h,cc}.
- content/renderer/media/*.
- whatever else that seems interesting.
There's also a diagram at
https://docs.google.com/drawings/d/1U1e-0gH3WVSV9Td9ZCEcPDBMC7WWWeh58sRYAdXbr...
BUG=425368
==========
The callback thing seems reasonable. This pattern has come up in a few other places, so maybe we need a more generalized solution. Good for now though. Regarding the early RendererAudioStreamOutputFactoryPtr acquisition, I don't understand what problem this solves. InterfacePtr is not a thread-safe object, so if the main thread is blocked you still have no way of using the interface. https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... File content/renderer/media/mojo_audio_output_ipc.cc (right): https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/mojo_audio_output_ipc.cc:40: // It's not explicitly stated that moving from a OnceCallback resets the Seems like a documentation bug then. Moving a OnceCallback definitely resets source callback. It's equivalent to moving the internal BindStateBase, which is itself a RefCountedThreadSafe. Moving a RefCountedThreadSafe necessarily resets the source object. https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/mojo_audio_output_ipc.cc:67: callback_wrapper.callback_ = CallbackType(); Also unnecessary.
Thanks for the fast response! On 2017/05/08 16:04:24, Ken Rockot wrote: > The callback thing seems reasonable. This pattern has come up in a few other > places, so maybe we need a more generalized solution. Good for now though. > > Regarding the early RendererAudioStreamOutputFactoryPtr acquisition, I don't > understand what problem this solves. InterfacePtr is not a thread-safe object, > so if the main thread is blocked you still have no way of using the interface. I call PassInterface in AudioIPCFactory::RegisterRemoteFactory and bind it again on the IO thread. This will allow it to be used on the IO thread, right? > https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... > File content/renderer/media/mojo_audio_output_ipc.cc (right): > > https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... > content/renderer/media/mojo_audio_output_ipc.cc:40: // It's not explicitly > stated that moving from a OnceCallback resets the > Seems like a documentation bug then. Moving a OnceCallback definitely resets > source callback. It's equivalent to moving the internal BindStateBase, which is > itself a RefCountedThreadSafe. Moving a RefCountedThreadSafe necessarily resets > the source object. > > https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... > content/renderer/media/mojo_audio_output_ipc.cc:67: callback_wrapper.callback_ = > CallbackType(); > Also unnecessary. Hmm, I'm sure a reviewer on some other CL told me not to assume moved-from callback was reset (even though we all agree it's true when reading the implementation). Either way is fine by me.
On 2017/05/08 at 16:14:03, maxmorin wrote: > Thanks for the fast response! > > On 2017/05/08 16:04:24, Ken Rockot wrote: > > The callback thing seems reasonable. This pattern has come up in a few other > > places, so maybe we need a more generalized solution. Good for now though. > > > > Regarding the early RendererAudioStreamOutputFactoryPtr acquisition, I don't > > understand what problem this solves. InterfacePtr is not a thread-safe object, > > so if the main thread is blocked you still have no way of using the interface. > I call PassInterface in AudioIPCFactory::RegisterRemoteFactory and bind it again on the IO thread. This will allow it to be used on the IO thread, right? Ah, sorry. Indeed you do :) Looks good to me. > > > https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... > > File content/renderer/media/mojo_audio_output_ipc.cc (right): > > > > https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... > > content/renderer/media/mojo_audio_output_ipc.cc:40: // It's not explicitly > > stated that moving from a OnceCallback resets the > > Seems like a documentation bug then. Moving a OnceCallback definitely resets > > source callback. It's equivalent to moving the internal BindStateBase, which is > > itself a RefCountedThreadSafe. Moving a RefCountedThreadSafe necessarily resets > > the source object. > > > > https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... > > content/renderer/media/mojo_audio_output_ipc.cc:67: callback_wrapper.callback_ = > > CallbackType(); > > Also unnecessary. > > Hmm, I'm sure a reviewer on some other CL told me not to assume moved-from callback was reset (even though we all agree it's true when reading the implementation). Either way is fine by me. I would prefer we didn't have redundant code based on documentation bugs. If the person who advised this was a base owner or someone responsible for the Callback implementation, then I'd defer to them (albeit with some confusion.) If not, maybe it's worth bringing this up on chromium-dev for some clarity?
On 2017/05/08 at 16:17:45, Ken Rockot wrote: > On 2017/05/08 at 16:14:03, maxmorin wrote: > > Thanks for the fast response! > > > > On 2017/05/08 16:04:24, Ken Rockot wrote: > > > The callback thing seems reasonable. This pattern has come up in a few other > > > places, so maybe we need a more generalized solution. Good for now though. > > > > > > Regarding the early RendererAudioStreamOutputFactoryPtr acquisition, I don't > > > understand what problem this solves. InterfacePtr is not a thread-safe object, > > > so if the main thread is blocked you still have no way of using the interface. > > I call PassInterface in AudioIPCFactory::RegisterRemoteFactory and bind it again on the IO thread. This will allow it to be used on the IO thread, right? > > Ah, sorry. Indeed you do :) Looks good to me. Oh... I also seem to have lost sight of the original question. There are ways we can improve this, but it requires some reworking of how each frame's InterfaceProvider is set up. That code is in need of some refactoring anyway, but I wouldn't block your work on it. Note that creating and binding the InterfacePtr is cheap so I wouldn't worry about the cost there as long as the browser-side binding of the request doesn't have some large cost itself. We can do something better with a little extra work. We could make frame_host_interface_broker_ > > > > > > https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... > > > File content/renderer/media/mojo_audio_output_ipc.cc (right): > > > > > > https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... > > > content/renderer/media/mojo_audio_output_ipc.cc:40: // It's not explicitly > > > stated that moving from a OnceCallback resets the > > > Seems like a documentation bug then. Moving a OnceCallback definitely resets > > > source callback. It's equivalent to moving the internal BindStateBase, which is > > > itself a RefCountedThreadSafe. Moving a RefCountedThreadSafe necessarily resets > > > the source object. > > > > > > https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... > > > content/renderer/media/mojo_audio_output_ipc.cc:67: callback_wrapper.callback_ = > > > CallbackType(); > > > Also unnecessary. > > > > Hmm, I'm sure a reviewer on some other CL told me not to assume moved-from callback was reset (even though we all agree it's true when reading the implementation). Either way is fine by me. > > I would prefer we didn't have redundant code based on documentation bugs. If the person who advised this was a base owner or someone responsible for the Callback implementation, then I'd defer to them (albeit with some confusion.) If not, maybe it's worth bringing this up on chromium-dev for some clarity?
On 2017/05/08 at 16:24:56, Ken Rockot wrote: > On 2017/05/08 at 16:17:45, Ken Rockot wrote: > > On 2017/05/08 at 16:14:03, maxmorin wrote: > > > Thanks for the fast response! > > > > > > On 2017/05/08 16:04:24, Ken Rockot wrote: > > > > The callback thing seems reasonable. This pattern has come up in a few other > > > > places, so maybe we need a more generalized solution. Good for now though. > > > > > > > > Regarding the early RendererAudioStreamOutputFactoryPtr acquisition, I don't > > > > understand what problem this solves. InterfacePtr is not a thread-safe object, > > > > so if the main thread is blocked you still have no way of using the interface. > > > I call PassInterface in AudioIPCFactory::RegisterRemoteFactory and bind it again on the IO thread. This will allow it to be used on the IO thread, right? > > > > Ah, sorry. Indeed you do :) Looks good to me. > > Oh... I also seem to have lost sight of the original question. There are ways we can improve this, but it requires some reworking of how each frame's InterfaceProvider is set up. That code is in need of some refactoring anyway, but I wouldn't block your work on it. > > Note that creating and binding the InterfacePtr is cheap so I wouldn't worry about the cost there as long as the browser-side binding of the request doesn't have some large cost itself. > > We can do something better with a little extra work. We could make frame_host_interface_broker_ ^ Ignore that last line :) > > > > > > > > > > https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... > > > > File content/renderer/media/mojo_audio_output_ipc.cc (right): > > > > > > > > https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... > > > > content/renderer/media/mojo_audio_output_ipc.cc:40: // It's not explicitly > > > > stated that moving from a OnceCallback resets the > > > > Seems like a documentation bug then. Moving a OnceCallback definitely resets > > > > source callback. It's equivalent to moving the internal BindStateBase, which is > > > > itself a RefCountedThreadSafe. Moving a RefCountedThreadSafe necessarily resets > > > > the source object. > > > > > > > > https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... > > > > content/renderer/media/mojo_audio_output_ipc.cc:67: callback_wrapper.callback_ = > > > > CallbackType(); > > > > Also unnecessary. > > > > > > Hmm, I'm sure a reviewer on some other CL told me not to assume moved-from callback was reset (even though we all agree it's true when reading the implementation). Either way is fine by me. > > > > I would prefer we didn't have redundant code based on documentation bugs. If the person who advised this was a base owner or someone responsible for the Callback implementation, then I'd defer to them (albeit with some confusion.) If not, maybe it's worth bringing this up on chromium-dev for some clarity?
Awesome, thanks. I posted https://groups.google.com/a/chromium.org/forum/#!topic/cxx/5JWdjOKuqh8 about the state of moved-from objects.
On 2017/05/09 at 10:09:40, maxmorin wrote: > Awesome, thanks. I posted https://groups.google.com/a/chromium.org/forum/#!topic/cxx/5JWdjOKuqh8 about the state of moved-from objects. Thanks for posting. Sounds like there is actually strong rationale for explicitly resetting the callbacks even after calling Run()&&. Who am I to disagree? :)
Nice work! https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... File content/renderer/media/audio_device_factory.cc (right): https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/audio_device_factory.cc:15: #include "content/public/common/content_features.h" Not needed? https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/audio_device_factory.cc:55: std::move(ipc), io_task_runner, session_id, device_id, security_origin, std::move(io_task_runner)? https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... File content/renderer/media/audio_ipc_factory.cc (right): https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/audio_ipc_factory.cc:53: // Unretained is safe because |this| is owned by the RenderThreadImpl which is If "not being destroyed ever" is a requirement for the class to work correctly - add a comment into the header and DCHECK that destructor is never called? And rephrase these comments? https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... File content/renderer/media/audio_ipc_factory.h (right): https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/audio_ipc_factory.h:35: // The following functions may be called on any thread: All public functions? Then move it to the class-level comment? https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/audio_ipc_factory.h:39: scoped_refptr<base::SingleThreadTaskRunner> io_task_runner() const { Return a pointer? https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/audio_ipc_factory.h:75: AudioMessageFilter* audio_message_filter_; const? https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... File content/renderer/media/mojo_audio_output_ipc.cc (right): https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/mojo_audio_output_ipc.cc:25: // case of a connection error. Nice! https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/mojo_audio_output_ipc.cc:86: // the final call to CloseStream, where its pointers are invalidated. What guarantees that CloseStream() has been called? https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/mojo_audio_output_ipc.cc:97: // Unretained is safe because |delegate| owns |this|. See the comment about storing a pointer to |delegate|. https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/mojo_audio_output_ipc.cc:120: // authorization reply, even if the connection is closed. Looking forward to unit tests :) https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/mojo_audio_output_ipc.cc:129: media::AudioOutputIPCDelegate* delegate, What is the point of passing |delegate| around? Why not to have it as a member which is initialized either on RequestDeviceAuthorization() or on CreateStream()? https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/mojo_audio_output_ipc.cc:137: void MojoAudioOutputIPC::CreateStream(media::AudioOutputIPCDelegate* delegate, Could you arrange implementations in the declaration order? https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/mojo_audio_output_ipc.cc:152: factory->RequestDeviceAuthorization( Combine duplicated code here and in RequestDeviceAuthorization()? https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/mojo_audio_output_ipc.cc:155: base::Bind(&TrivialAuthorizedCallback)); Comment why you don't process authorization response? https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/mojo_audio_output_ipc.cc:159: // unretained is safe. Add a comment why we can't use Weak here? https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/mojo_audio_output_ipc.cc:164: // Unretained is safe because |delegate| owns |this|. I'd prefer the term "outlives". The code changes quickly. Also, if we store a pointer to |delegate| in the member(see the comment above), it's more explicit. https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/mojo_audio_output_ipc.cc:166: &media::AudioOutputIPCDelegate::OnError, base::Unretained(delegate))); This is already done in MakeProviderRequest(), isn't it? https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/mojo_audio_output_ipc.cc:175: DCHECK(shared_memory.is_valid()); CHECK those two? https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/mojo_audio_output_ipc.cc:178: mojo::UnwrapPlatformFile(std::move(socket), &socket_handle); CHECK the result? https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/mojo_audio_output_ipc.cc:185: DCHECK_EQ(result, MOJO_RESULT_OK); CHECK? Is it really safe to continue? https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/mojo_audio_output_ipc.cc:186: DCHECK(!read_only); CHECK as well? https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/mojo_audio_output_ipc.cc:188: delegate->OnStreamCreated(memory_handle, socket_handle, memory_length); CHECK(memory_length) as well? (make sure to have a security review for this code) https://codereview.chromium.org/2821203005/diff/60001/content/renderer/pepper... File content/renderer/pepper/pepper_platform_audio_output_dev.cc (right): https://codereview.chromium.org/2821203005/diff/60001/content/renderer/pepper... content/renderer/pepper/pepper_platform_audio_output_dev.cc:29: const int64_t kMaxAuthorizationTimeoutMs = 0; // No timeout. We may need a separate CL to remove this code duplication at all. https://codereview.chromium.org/2821203005/diff/60001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2821203005/diff/60001/content/renderer/render... content/renderer/render_frame_impl.cc:1233: AudioIPCFactory::get()->MaybeDeregisterRemoteFactory(GetRoutingID()); Wrap it into a static method? https://codereview.chromium.org/2821203005/diff/60001/content/renderer/render... content/renderer/render_frame_impl.cc:1312: AudioIPCFactory::get()->RegisterRemoteFactory(GetRoutingID(), It looks like a static MaybeRegisterRemoteFactories()? https://codereview.chromium.org/2821203005/diff/60001/content/renderer/render... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2821203005/diff/60001/content/renderer/render... content/renderer/render_thread_impl.cc:710: audio_ipc_factory_.emplace(GetIOTaskRunner(), audio_message_filter_.get()); Make it own |audio_message_filter_|? And remove audio_message_filter() getter from renderer thread? https://codereview.chromium.org/2821203005/diff/60001/content/renderer/render... File content/renderer/render_thread_impl.h (right): https://codereview.chromium.org/2821203005/diff/60001/content/renderer/render... content/renderer/render_thread_impl.h:640: base::Optional<AudioIPCFactory> audio_ipc_factory_; Would unique_ptr<> be enough for that? And under which conditions it's uninitialized?
Description was changed from
==========
Add a mojo implementation of AudioOutputIPC.
* When processing an authorization request, it is not safe to use other
threads than the IO thread, as they may be blocked waiting for the
response. Thus, AudioIPCFactory is introduced to hold a mapping
(frame id->remote factory) on the IO thread. This also means we have
to create/destruct the RendererAudioOutputStreamFactoryPtr with the
RenderFrameImpl instead of the preferrable way of doing it lazily.
This can be revisited if we stop blocking while waiting for
authorization (crbug.com/668275).
* For running browser tests with this, the browser-side glue code is
needed. See the dependent CL. I have verified that the browser tests
pass. As the pepper code doesn't seem well covered by tests, I also
manually verified that flash sound output works.
* Suggested reading order:
- content/renderer/media/mojo_audio_output_ipc.{h,cc}.
- content/renderer/media/*.
- whatever else that seems interesting.
There's also a diagram at
https://docs.google.com/drawings/d/1U1e-0gH3WVSV9Td9ZCEcPDBMC7WWWeh58sRYAdXbr...
BUG=425368
==========
to
==========
Add a mojo implementation of AudioOutputIPC.
* When processing an authorization request, it is not safe to use other
threads than the IO thread, as they may be blocked waiting for the
response. Thus, AudioIPCFactory is introduced to hold a mapping
(frame id->remote factory) on the IO thread. This also means we have
to create/destruct the RendererAudioOutputStreamFactoryPtr with the
RenderFrameImpl instead of the preferrable way of doing it lazily.
This can be revisited if we stop blocking while waiting for
authorization (crbug.com/668275).
* For running browser tests with this, the browser-side glue code is
needed. See the dependent CL. I have verified that the browser tests
pass. As the pepper code doesn't seem well covered by tests, I also
manually verified that flash sound output works.
* Suggested reading order:
- content/renderer/media/mojo_audio_output_ipc.{h,cc}.
- content/renderer/media/*.
- whatever else that seems interesting.
There's also a diagram at
https://docs.google.com/drawings/d/1U1e-0gH3WVSV9Td9ZCEcPDBMC7WWWeh58sRYAdXbr...
BUG=425368
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========
Will re-run browser tests tomorrow and start writing unit tests. https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... File content/renderer/media/audio_device_factory.cc (right): https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/audio_device_factory.cc:15: #include "content/public/common/content_features.h" On 2017/05/11 10:58:40, o1ka wrote: > Not needed? Done. https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/audio_device_factory.cc:55: std::move(ipc), io_task_runner, session_id, device_id, security_origin, On 2017/05/11 10:58:40, o1ka wrote: > std::move(io_task_runner)? Done. https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... File content/renderer/media/audio_ipc_factory.cc (right): https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/audio_ipc_factory.cc:53: // Unretained is safe because |this| is owned by the RenderThreadImpl which is On 2017/05/11 10:58:40, o1ka wrote: > If "not being destroyed ever" is a requirement for the class to work correctly - > add a comment into the header and DCHECK that destructor is never called? And > rephrase these comments? Added header comment and updated these comments. Destructing may still make sense in tests though. Added a comment for that. https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... File content/renderer/media/audio_ipc_factory.h (right): https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/audio_ipc_factory.h:35: // The following functions may be called on any thread: On 2017/05/11 10:58:40, o1ka wrote: > All public functions? Then move it to the class-level comment? Done. https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/audio_ipc_factory.h:39: scoped_refptr<base::SingleThreadTaskRunner> io_task_runner() const { On 2017/05/11 10:58:40, o1ka wrote: > Return a pointer? I prefer this, since it clearly show the ownership is being shared with the caller. https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/audio_ipc_factory.h:75: AudioMessageFilter* audio_message_filter_; On 2017/05/11 10:58:40, o1ka wrote: > const? Done. https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... File content/renderer/media/mojo_audio_output_ipc.cc (right): https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/mojo_audio_output_ipc.cc:40: // It's not explicitly stated that moving from a OnceCallback resets the On 2017/05/08 16:04:24, Ken Rockot wrote: > Seems like a documentation bug then. Moving a OnceCallback definitely resets > source callback. It's equivalent to moving the internal BindStateBase, which is > itself a RefCountedThreadSafe. Moving a RefCountedThreadSafe necessarily resets > the source object. Leaving this after discussion on cxx. https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/mojo_audio_output_ipc.cc:67: callback_wrapper.callback_ = CallbackType(); On 2017/05/08 16:04:24, Ken Rockot wrote: > Also unnecessary. Same. https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/mojo_audio_output_ipc.cc:86: // the final call to CloseStream, where its pointers are invalidated. On 2017/05/11 10:58:40, o1ka wrote: > What guarantees that CloseStream() has been called? This is part of the AudioOutputIPC contract. It's not actually written in the AudioOutputIPC definition, so I updated that. https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/mojo_audio_output_ipc.cc:97: // Unretained is safe because |delegate| owns |this|. On 2017/05/11 10:58:41, o1ka wrote: > See the comment about storing a pointer to |delegate|. Not sure what it has to do with this code? https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/mojo_audio_output_ipc.cc:120: // authorization reply, even if the connection is closed. On 2017/05/11 10:58:41, o1ka wrote: > Looking forward to unit tests :) :) https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/mojo_audio_output_ipc.cc:129: media::AudioOutputIPCDelegate* delegate, On 2017/05/11 10:58:41, o1ka wrote: > What is the point of passing |delegate| around? Why not to have it as a member > which is initialized either on RequestDeviceAuthorization() or on > CreateStream()? The AudioOutputIPC interface is written to get |delegate| whenever it might need to call back, so it feels more natural to not store the delegate. https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/mojo_audio_output_ipc.cc:137: void MojoAudioOutputIPC::CreateStream(media::AudioOutputIPCDelegate* delegate, On 2017/05/11 10:58:41, o1ka wrote: > Could you arrange implementations in the declaration order? Done. https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/mojo_audio_output_ipc.cc:152: factory->RequestDeviceAuthorization( On 2017/05/11 10:58:40, o1ka wrote: > Combine duplicated code here and in RequestDeviceAuthorization()? If I introduced a helper function, it would have to take lots of arguements, and I'd still need a conditional for returning early, so I don't think it actually saves complexity. https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/mojo_audio_output_ipc.cc:155: base::Bind(&TrivialAuthorizedCallback)); On 2017/05/11 10:58:40, o1ka wrote: > Comment why you don't process authorization response? Done. https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/mojo_audio_output_ipc.cc:159: // unretained is safe. On 2017/05/11 10:58:41, o1ka wrote: > Add a comment why we can't use Weak here? We can, but we don't need to. https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/mojo_audio_output_ipc.cc:164: // Unretained is safe because |delegate| owns |this|. On 2017/05/11 10:58:40, o1ka wrote: > I'd prefer the term "outlives". The code changes quickly. > Also, if we store a pointer to |delegate| in the member(see the comment above), > it's more explicit. Comment is bad, I changed it. See above for storing a pointer to |delegate| in a member. https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/mojo_audio_output_ipc.cc:166: &media::AudioOutputIPCDelegate::OnError, base::Unretained(delegate))); On 2017/05/11 10:58:40, o1ka wrote: > This is already done in MakeProviderRequest(), isn't it? For the provider. This is for the stream. https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/mojo_audio_output_ipc.cc:175: DCHECK(shared_memory.is_valid()); On 2017/05/11 10:58:40, o1ka wrote: > CHECK those two? There are no security implications of this code, since it's receiving IPC from a more privileged process. https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/mojo_audio_output_ipc.cc:178: mojo::UnwrapPlatformFile(std::move(socket), &socket_handle); On 2017/05/11 10:58:41, o1ka wrote: > CHECK the result? same https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/mojo_audio_output_ipc.cc:185: DCHECK_EQ(result, MOJO_RESULT_OK); On 2017/05/11 10:58:41, o1ka wrote: > CHECK? > Is it really safe to continue? same https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/mojo_audio_output_ipc.cc:186: DCHECK(!read_only); On 2017/05/11 10:58:41, o1ka wrote: > CHECK as well? same https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/mojo_audio_output_ipc.cc:188: delegate->OnStreamCreated(memory_handle, socket_handle, memory_length); On 2017/05/11 10:58:40, o1ka wrote: > CHECK(memory_length) as well? > > (make sure to have a security review for this code) same https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... File content/renderer/media/mojo_audio_output_ipc.h (right): https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/mojo_audio_output_ipc.h:26: // |factory_accessor| is required to provide a On 2017/05/05 14:06:14, Max Morin wrote: > Ignore stale comment, will change it to a better one. :) Done. https://codereview.chromium.org/2821203005/diff/60001/content/renderer/pepper... File content/renderer/pepper/pepper_platform_audio_output_dev.cc (right): https://codereview.chromium.org/2821203005/diff/60001/content/renderer/pepper... content/renderer/pepper/pepper_platform_audio_output_dev.cc:29: const int64_t kMaxAuthorizationTimeoutMs = 0; // No timeout. On 2017/05/11 10:58:41, o1ka wrote: > We may need a separate CL to remove this code duplication at all. Yes. https://codereview.chromium.org/2821203005/diff/60001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2821203005/diff/60001/content/renderer/render... content/renderer/render_frame_impl.cc:1233: AudioIPCFactory::get()->MaybeDeregisterRemoteFactory(GetRoutingID()); On 2017/05/11 10:58:41, o1ka wrote: > Wrap it into a static method? I feel like this code is consistent with the code above (for input_handler_manager), so it makes sense. https://codereview.chromium.org/2821203005/diff/60001/content/renderer/render... content/renderer/render_frame_impl.cc:1312: AudioIPCFactory::get()->RegisterRemoteFactory(GetRoutingID(), On 2017/05/11 10:58:41, o1ka wrote: > It looks like a static MaybeRegisterRemoteFactories()? I moved some logic into AudioIPCFactory, but still null check as I feel this is consistent with the treatment of input_handler_manager above. https://codereview.chromium.org/2821203005/diff/60001/content/renderer/render... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2821203005/diff/60001/content/renderer/render... content/renderer/render_thread_impl.cc:710: audio_ipc_factory_.emplace(GetIOTaskRunner(), audio_message_filter_.get()); On 2017/05/11 10:58:41, o1ka wrote: > Make it own |audio_message_filter_|? And remove audio_message_filter() getter > from renderer thread? Done. Now AudioMessageFilter is completely cannibalized :). https://codereview.chromium.org/2821203005/diff/60001/content/renderer/render... File content/renderer/render_thread_impl.h (right): https://codereview.chromium.org/2821203005/diff/60001/content/renderer/render... content/renderer/render_thread_impl.h:640: base::Optional<AudioIPCFactory> audio_ipc_factory_; On 2017/05/11 10:58:41, o1ka wrote: > Would unique_ptr<> be enough for that? > And under which conditions it's uninitialized? There's no need for unique_ptr, since Optional is enough :). unique_ptr is an extra allocation, so I default to using Optional, and use unique_ptr if needed. Added a comment about initialization.
https://codereview.chromium.org/2821203005/diff/1/content/renderer/media/audi... File content/renderer/media/audio_ipc_factory.h (right): https://codereview.chromium.org/2821203005/diff/1/content/renderer/media/audi... content/renderer/media/audio_ipc_factory.h:32: static AudioIPCFactory* get() { On 2017/05/05 13:10:58, Max Morin wrote: > On 2017/04/20 10:35:59, o1ka wrote: > > Get(), and move implementation to .cc? > > I couldn't find the rule against this in the style guide, checking > https://google.github.io/styleguide/cppguide.html#Inline_Functions and > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md.... > https://www.chromium.org/developers/coding-style/cpp-dos-and-donts mentions > simple accessors. This is as simple as a function could be, and also an > accessor, so it should be fine. it has DCHECK, so technically it's not a "simple accessor" => should be named Get() https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... File content/renderer/media/audio_ipc_factory.cc (right): https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/audio_ipc_factory.cc:53: // Unretained is safe because |this| is owned by the RenderThreadImpl which is On 2017/05/11 15:31:17, Max Morin wrote: > On 2017/05/11 10:58:40, o1ka wrote: > > If "not being destroyed ever" is a requirement for the class to work correctly > - > > add a comment into the header and DCHECK that destructor is never called? And > > rephrase these comments? > > Added header comment and updated these comments. Destructing may still make > sense in tests though. Added a comment for that. It still a bit confusing, since the comment says |this| is leaked, but it's not leaked in test according to the destructor comment. https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... File content/renderer/media/audio_ipc_factory.h (right): https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/audio_ipc_factory.h:39: scoped_refptr<base::SingleThreadTaskRunner> io_task_runner() const { On 2017/05/11 15:31:17, Max Morin wrote: > On 2017/05/11 10:58:40, o1ka wrote: > > Return a pointer? > > I prefer this, since it clearly show the ownership is being shared with the > caller. It's up to the caller do decide if there is a need to share ownership, right? Why to mess up with refs if the caller just wants to post a message? https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... File content/renderer/media/mojo_audio_output_ipc.cc (right): https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/mojo_audio_output_ipc.cc:129: media::AudioOutputIPCDelegate* delegate, On 2017/05/11 15:31:17, Max Morin wrote: > On 2017/05/11 10:58:41, o1ka wrote: > > What is the point of passing |delegate| around? Why not to have it as a member > > which is initialized either on RequestDeviceAuthorization() or on > > CreateStream()? > > The AudioOutputIPC interface is written to get |delegate| whenever it might need > to call back, so it feels more natural to not store the delegate. Not sure why it is more natural. MojoAudioOutputIPC is bound to a |delegate| passed to it in the first call the client makes. Storing a pointer to it is a common indicator that |delegate| is expected to outlive the object. "whenever" - There are only 2 cases whe it gets |delegate|: RequestDeviceAuthorization() and CreateStream(). In this implementation RequestDeviceAuthorization() calls set_connection_error_handler(), passing |delegate| to it. How the code verifies that subsequent call to Create() is made with the same |delegate|? https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/mojo_audio_output_ipc.cc:152: factory->RequestDeviceAuthorization( On 2017/05/11 15:31:17, Max Morin wrote: > On 2017/05/11 10:58:40, o1ka wrote: > > Combine duplicated code here and in RequestDeviceAuthorization()? > > If I introduced a helper function, it would have to take lots of arguements, and > I'd still need a conditional for returning early, so I don't think it actually > saves complexity. Not sure what do you mean. bool DoRequestDeviceAuthorization(delegate, session id, device id, callback)? https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/mojo_audio_output_ipc.cc:166: &media::AudioOutputIPCDelegate::OnError, base::Unretained(delegate))); On 2017/05/11 15:31:18, Max Morin wrote: > On 2017/05/11 10:58:40, o1ka wrote: > > This is already done in MakeProviderRequest(), isn't it? > > For the provider. This is for the stream. Acknowledged. https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/mojo_audio_output_ipc.cc:178: mojo::UnwrapPlatformFile(std::move(socket), &socket_handle); On 2017/05/11 15:31:18, Max Morin wrote: > On 2017/05/11 10:58:41, o1ka wrote: > > CHECK the result? > > same Why? https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/mojo_audio_output_ipc.cc:185: DCHECK_EQ(result, MOJO_RESULT_OK); On 2017/05/11 15:31:18, Max Morin wrote: > On 2017/05/11 10:58:41, o1ka wrote: > > CHECK? > > Is it really safe to continue? > > same Why? How about out of memory scenario? https://codereview.chromium.org/2821203005/diff/60001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2821203005/diff/60001/content/renderer/render... content/renderer/render_frame_impl.cc:1233: AudioIPCFactory::get()->MaybeDeregisterRemoteFactory(GetRoutingID()); On 2017/05/11 15:31:18, Max Morin wrote: > On 2017/05/11 10:58:41, o1ka wrote: > > Wrap it into a static method? > > I feel like this code is consistent with the code above (for > input_handler_manager), so it makes sense. Why would you need consistency here? This is "Maybe" method anyways. Why complicating client code? Up to you. https://codereview.chromium.org/2821203005/diff/80001/content/renderer/media/... File content/renderer/media/mojo_audio_output_ipc.cc (right): https://codereview.chromium.org/2821203005/diff/80001/content/renderer/media/... content/renderer/media/mojo_audio_output_ipc.cc:192: // Unretained is safe because |delegate| owns |this|. l. 142 says "Unretained is safe because |delegate| must be valid until CloseStream is called". Can we combine it all in a unified lifetime statement? We should not rely on the ownership unless it's a contract stated in the header file.
Could you explain like I'm five why CHECKs are needed?
To not debug werid out of memory crashes? On May 15, 2017 4:30 PM, <maxmorin@chromium.org> wrote: > Could you explain like I'm five why CHECKs are needed? > > https://codereview.chromium.org/2821203005/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
By https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md..., we should only CHECK when code has security implications. Having some sort of CHECK for any piece of code that's using memory one way or another would result in millions of CHECKs across the code base :). A stronger reason is needed to use CHECK. On 2017/05/15 14:34:39, chromium-reviews wrote: > To not debug werid out of memory crashes? > > On May 15, 2017 4:30 PM, <mailto:maxmorin@chromium.org> wrote: > > > Could you explain like I'm five why CHECKs are needed? > > > > https://codereview.chromium.org/2821203005/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
On 2017/05/15 14:34:39, chromium-reviews wrote: > To not debug werid out of memory crashes? > > On May 15, 2017 4:30 PM, <mailto:maxmorin@chromium.org> wrote: > > > Could you explain like I'm five why CHECKs are needed? > > > > https://codereview.chromium.org/2821203005/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. (It was me). Why would we want to continue if there is a problem? If shared memory is guaranteed to be always valid - Add a comment why
On 2017/05/15 14:40:12, Max Morin wrote: > By > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md..., > we should only CHECK when code has security implications. Having some sort of > CHECK for any piece of code that's using memory one way or another would result > in millions of CHECKs across the code base :). A stronger reason is needed to > use CHECK. > > On 2017/05/15 14:34:39, chromium-reviews wrote: > > To not debug werid out of memory crashes? > > > > On May 15, 2017 4:30 PM, <mailto:maxmorin@chromium.org> wrote: > > > > > Could you explain like I'm five why CHECKs are needed? > > > > > > https://codereview.chromium.org/2821203005/ > > > > > > > -- > > You received this message because you are subscribed to the Google Groups > > "Chromium-reviews" group. > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. If not CHECK then check the return val. Why it's safe to proceed without checking the result?
On 2017/05/15 14:44:15, o1ka wrote: > On 2017/05/15 14:40:12, Max Morin wrote: > > By > > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md..., > > we should only CHECK when code has security implications. Having some sort of > > CHECK for any piece of code that's using memory one way or another would > result > > in millions of CHECKs across the code base :). A stronger reason is needed to > > use CHECK. > > > > On 2017/05/15 14:34:39, chromium-reviews wrote: > > > To not debug werid out of memory crashes? > > > > > > On May 15, 2017 4:30 PM, <mailto:maxmorin@chromium.org> wrote: > > > > > > > Could you explain like I'm five why CHECKs are needed? > > > > > > > > https://codereview.chromium.org/2821203005/ > > > > > > > > > > -- > > > You received this message because you are subscribed to the Google Groups > > > "Chromium-reviews" group. > > > To unsubscribe from this group and stop receiving emails from it, send an > > email > > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > If not CHECK then check the return val. Why it's safe to proceed without > checking the result? I'll update the mojom to clarify that the handles must be valid.
Sgtm On May 15, 2017 4:46 PM, <maxmorin@chromium.org> wrote: > On 2017/05/15 14:44:15, o1ka wrote: > > On 2017/05/15 14:40:12, Max Morin wrote: > > > By > > > > > > https://chromium.googlesource.com/chromium/src/+/master/ > styleguide/c++/c++.md#CHECK_DCHECK_and-NOTREACHED, > > > we should only CHECK when code has security implications. Having some > sort > of > > > CHECK for any piece of code that's using memory one way or another > would > > result > > > in millions of CHECKs across the code base :). A stronger reason is > needed > to > > > use CHECK. > > > > > > On 2017/05/15 14:34:39, chromium-reviews wrote: > > > > To not debug werid out of memory crashes? > > > > > > > > On May 15, 2017 4:30 PM, <mailto:maxmorin@chromium.org> wrote: > > > > > > > > > Could you explain like I'm five why CHECKs are needed? > > > > > > > > > > https://codereview.chromium.org/2821203005/ > > > > > > > > > > > > > -- > > > > You received this message because you are subscribed to the Google > Groups > > > > "Chromium-reviews" group. > > > > To unsubscribe from this group and stop receiving emails from it, > send an > > > email > > > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > If not CHECK then check the return val. Why it's safe to proceed without > > checking the result? > I'll update the mojom to clarify that the handles must be valid. > > https://codereview.chromium.org/2821203005/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by maxmorin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I updated the comment in media/mojo/interfaces/audio_output_stream.mojom to address the validity of the passed handles. Added tests for MojoAudioOutputIPC. It's easy to split AudioIPCFactory into a separate CL (it still needs tests), so I'll do that tomorrow. https://codereview.chromium.org/2821203005/diff/1/content/renderer/media/audi... File content/renderer/media/audio_ipc_factory.h (right): https://codereview.chromium.org/2821203005/diff/1/content/renderer/media/audi... content/renderer/media/audio_ipc_factory.h:32: static AudioIPCFactory* get() { On 2017/05/15 13:27:08, o1ka wrote: > On 2017/05/05 13:10:58, Max Morin wrote: > > On 2017/04/20 10:35:59, o1ka wrote: > > > Get(), and move implementation to .cc? > > > > I couldn't find the rule against this in the style guide, checking > > https://google.github.io/styleguide/cppguide.html#Inline_Functions and > > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md.... > > https://www.chromium.org/developers/coding-style/cpp-dos-and-donts mentions > > simple accessors. This is as simple as a function could be, and also an > > accessor, so it should be fine. > > it has DCHECK, so technically it's not a "simple accessor" => should be named > Get() It doesn't have the DCHECK in later patch sets (since it's null in some tests involving render frames) :). https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... File content/renderer/media/audio_ipc_factory.cc (right): https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/audio_ipc_factory.cc:53: // Unretained is safe because |this| is owned by the RenderThreadImpl which is On 2017/05/15 13:27:08, o1ka wrote: > On 2017/05/11 15:31:17, Max Morin wrote: > > On 2017/05/11 10:58:40, o1ka wrote: > > > If "not being destroyed ever" is a requirement for the class to work > correctly > > - > > > add a comment into the header and DCHECK that destructor is never called? > And > > > rephrase these comments? > > > > Added header comment and updated these comments. Destructing may still make > > sense in tests though. Added a comment for that. > > It still a bit confusing, since the comment says > |this| is leaked, but it's not leaked in test according to the destructor > comment. Ok, I expanded the header comment to include the case of unit tests. https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... File content/renderer/media/audio_ipc_factory.h (right): https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/audio_ipc_factory.h:39: scoped_refptr<base::SingleThreadTaskRunner> io_task_runner() const { On 2017/05/15 13:27:08, o1ka wrote: > On 2017/05/11 15:31:17, Max Morin wrote: > > On 2017/05/11 10:58:40, o1ka wrote: > > > Return a pointer? > > > > I prefer this, since it clearly show the ownership is being shared with the > > caller. > > It's up to the caller do decide if there is a need to share ownership, right? > Why to mess up with refs if the caller just wants to post a message? In general, converting from * to an owning pointer is unexpected. If an API returns a raw pointer, it looks like it doesn't share ownership, so if someone takes ownership through that API anyways it could cause issues if the implementation expects to not share the object. In this case, performance gains are 0, since all callers of this method take ownership of the task runner. https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... File content/renderer/media/mojo_audio_output_ipc.cc (right): https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/mojo_audio_output_ipc.cc:129: media::AudioOutputIPCDelegate* delegate, On 2017/05/15 13:27:08, o1ka wrote: > On 2017/05/11 15:31:17, Max Morin wrote: > > On 2017/05/11 10:58:41, o1ka wrote: > > > What is the point of passing |delegate| around? Why not to have it as a > member > > > which is initialized either on RequestDeviceAuthorization() or on > > > CreateStream()? > > > > The AudioOutputIPC interface is written to get |delegate| whenever it might > need > > to call back, so it feels more natural to not store the delegate. > > Not sure why it is more natural. MojoAudioOutputIPC is bound to a |delegate| > passed to it in the first call the client makes. Storing a pointer to it is a > common indicator that |delegate| is expected to outlive the object. > > "whenever" - There are only 2 cases whe it gets |delegate|: > RequestDeviceAuthorization() and CreateStream(). > In this implementation RequestDeviceAuthorization() calls > set_connection_error_handler(), passing |delegate| to it. > How the code verifies that subsequent call to Create() is made with the same > |delegate|? I changed the class to store |delegate| so we can move on :). https://codereview.chromium.org/2821203005/diff/60001/content/renderer/media/... content/renderer/media/mojo_audio_output_ipc.cc:152: factory->RequestDeviceAuthorization( On 2017/05/15 13:27:08, o1ka wrote: > On 2017/05/11 15:31:17, Max Morin wrote: > > On 2017/05/11 10:58:40, o1ka wrote: > > > Combine duplicated code here and in RequestDeviceAuthorization()? > > > > If I introduced a helper function, it would have to take lots of arguements, > and > > I'd still need a conditional for returning early, so I don't think it actually > > saves complexity. > Not sure what do you mean. > bool DoRequestDeviceAuthorization(delegate, session id, device id, callback)? Done. https://codereview.chromium.org/2821203005/diff/80001/content/renderer/media/... File content/renderer/media/mojo_audio_output_ipc.cc (right): https://codereview.chromium.org/2821203005/diff/80001/content/renderer/media/... content/renderer/media/mojo_audio_output_ipc.cc:192: // Unretained is safe because |delegate| owns |this|. On 2017/05/15 13:27:08, o1ka wrote: > l. 142 says "Unretained is safe because |delegate| must be valid until > CloseStream is called". Can we combine it all in a unified lifetime statement? > We should not rely on the ownership unless it's a contract stated in the header > file. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by maxmorin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from
==========
Add a mojo implementation of AudioOutputIPC.
* When processing an authorization request, it is not safe to use other
threads than the IO thread, as they may be blocked waiting for the
response. Thus, AudioIPCFactory is introduced to hold a mapping
(frame id->remote factory) on the IO thread. This also means we have
to create/destruct the RendererAudioOutputStreamFactoryPtr with the
RenderFrameImpl instead of the preferrable way of doing it lazily.
This can be revisited if we stop blocking while waiting for
authorization (crbug.com/668275).
* For running browser tests with this, the browser-side glue code is
needed. See the dependent CL. I have verified that the browser tests
pass. As the pepper code doesn't seem well covered by tests, I also
manually verified that flash sound output works.
* Suggested reading order:
- content/renderer/media/mojo_audio_output_ipc.{h,cc}.
- content/renderer/media/*.
- whatever else that seems interesting.
There's also a diagram at
https://docs.google.com/drawings/d/1U1e-0gH3WVSV9Td9ZCEcPDBMC7WWWeh58sRYAdXbr...
BUG=425368
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========
to
==========
Add a mojo implementation of AudioOutputIPC.
A couple of tricky points:
* The requirement to always call CloseStream is added to
media/audio/audio_output_ipc.h. This requirement isn't new, but it
wasn't documented before.
* If the RendererAudioOutputStreamFactory used by MojoAudioOutputIPC is
destroyed prior to receiving authorization (e.g. because of frame
destruction), we need to signal the AudioOutputDevice anyways to
avoid waiting forever. The class AuthorizationCallbackWrapper is
used for this, and it is implemented and documented in
mojo_audio_output_ipc.cc.
BUG=425368
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========
Ok, this CL is now only for MojoAudioOutputIPC, with the factory being in https://codereview.chromium.org/2890753003#ps1
Patchset #6 (id:120001) has been deleted
The CQ bit was checked by maxmorin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_optional_gpu_tests_rel on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_optional_gpu_...)
The CQ bit was checked by maxmorin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
LGTM! (%nits) https://codereview.chromium.org/2821203005/diff/160001/content/renderer/media... File content/renderer/media/mojo_audio_output_ipc.cc (right): https://codereview.chromium.org/2821203005/diff/160001/content/renderer/media... content/renderer/media/mojo_audio_output_ipc.cc:43: AuthorizationCallbackWrapper&& other) = delete; It has a destructor, so move constructor is not generated, right? (and deleted methods usually go to private section) https://codereview.chromium.org/2821203005/diff/160001/content/renderer/media... content/renderer/media/mojo_audio_output_ipc.cc:194: auto* factory = factory_accessor_.Run(); DCHECK_CALLED_ON_VALID_THREAD? (just for easy reading) https://codereview.chromium.org/2821203005/diff/160001/content/renderer/media... content/renderer/media/mojo_audio_output_ipc.cc:229: mojo::UnwrapPlatformFile(std::move(socket), &socket_handle); Please DCHECK :) https://codereview.chromium.org/2821203005/diff/160001/content/renderer/media... File content/renderer/media/mojo_audio_output_ipc_unittest.cc (right): https://codereview.chromium.org/2821203005/diff/160001/content/renderer/media... content/renderer/media/mojo_audio_output_ipc_unittest.cc:85: DCHECK(!expect_request_); EXPECTation? https://codereview.chromium.org/2821203005/diff/160001/content/renderer/media... content/renderer/media/mojo_audio_output_ipc_unittest.cc:94: DCHECK(!expect_request_); same? https://codereview.chromium.org/2821203005/diff/160001/content/renderer/media... content/renderer/media/mojo_audio_output_ipc_unittest.cc:141: DCHECK(!binding_); EXPECT_TRUE? https://codereview.chromium.org/2821203005/diff/160001/content/renderer/media... content/renderer/media/mojo_audio_output_ipc_unittest.cc:142: EXPECT_TRUE(stream_); EXPECT_NE(nullptr, stream_) https://codereview.chromium.org/2821203005/diff/160001/content/renderer/media... content/renderer/media/mojo_audio_output_ipc_unittest.cc:268: // an constructing the TestStreamProvider with a |&stream| EXPECTs that the nit: and https://codereview.chromium.org/2821203005/diff/160001/content/renderer/media... content/renderer/media/mojo_audio_output_ipc_unittest.cc:384: FactoryDisconnectedBeforeAuthorizationReply_CallsAuthorizedAnyways) { Please add a comment explaining what is tested https://codereview.chromium.org/2821203005/diff/160001/content/renderer/media... content/renderer/media/mojo_audio_output_ipc_unittest.cc:408: FactoryDisconnectedAfterAuthorizationReply_CallsAuthorizedOnlyOnce) { ditto https://codereview.chromium.org/2821203005/diff/160001/content/renderer/media... content/renderer/media/mojo_audio_output_ipc_unittest.cc:510: WDYT of testing that we crash with DCHECK if CloseStream is not called?
Description was changed from ========== Add a mojo implementation of AudioOutputIPC. A couple of tricky points: * The requirement to always call CloseStream is added to media/audio/audio_output_ipc.h. This requirement isn't new, but it wasn't documented before. * If the RendererAudioOutputStreamFactory used by MojoAudioOutputIPC is destroyed prior to receiving authorization (e.g. because of frame destruction), we need to signal the AudioOutputDevice anyways to avoid waiting forever. The class AuthorizationCallbackWrapper is used for this, and it is implemented and documented in mojo_audio_output_ipc.cc. BUG=425368 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Add a mojo implementation of AudioOutputIPC. A couple of tricky points: * The requirement to always call CloseStream is added to media/audio/audio_output_ipc.h. This requirement isn't new, but it wasn't documented before. * If the RendererAudioOutputStreamFactory used by MojoAudioOutputIPC is destroyed prior to receiving authorization (e.g. because of frame destruction), we need to signal the AudioOutputDevice anyways to avoid waiting forever. The class AuthorizationCallbackWrapper is used for this, and it is implemented and documented in mojo_audio_output_ipc.cc. * Code for intergrating this into the AudioDeviceFactory is in https://codereview.chromium.org/2890753003 * Browser tests (using this and the dependent CL) is in https://codereview.chromium.org/2812883003 BUG=425368 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Thanks! https://codereview.chromium.org/2821203005/diff/160001/content/renderer/media... File content/renderer/media/mojo_audio_output_ipc.cc (right): https://codereview.chromium.org/2821203005/diff/160001/content/renderer/media... content/renderer/media/mojo_audio_output_ipc.cc:43: AuthorizationCallbackWrapper&& other) = delete; On 2017/05/18 09:16:50, o1ka wrote: > It has a destructor, so move constructor is not generated, right? > (and deleted methods usually go to private section) Right, I figured it might generate one since I defined the move assignment, but it won't. I removed this. https://codereview.chromium.org/2821203005/diff/160001/content/renderer/media... content/renderer/media/mojo_audio_output_ipc.cc:194: auto* factory = factory_accessor_.Run(); On 2017/05/18 09:16:50, o1ka wrote: > DCHECK_CALLED_ON_VALID_THREAD? (just for easy reading) Done. https://codereview.chromium.org/2821203005/diff/160001/content/renderer/media... content/renderer/media/mojo_audio_output_ipc.cc:229: mojo::UnwrapPlatformFile(std::move(socket), &socket_handle); On 2017/05/18 09:16:50, o1ka wrote: > Please DCHECK :) Done. https://codereview.chromium.org/2821203005/diff/160001/content/renderer/media... File content/renderer/media/mojo_audio_output_ipc_unittest.cc (right): https://codereview.chromium.org/2821203005/diff/160001/content/renderer/media... content/renderer/media/mojo_audio_output_ipc_unittest.cc:85: DCHECK(!expect_request_); On 2017/05/18 09:16:50, o1ka wrote: > EXPECTation? Done. https://codereview.chromium.org/2821203005/diff/160001/content/renderer/media... content/renderer/media/mojo_audio_output_ipc_unittest.cc:94: DCHECK(!expect_request_); On 2017/05/18 09:16:50, o1ka wrote: > same? Done. https://codereview.chromium.org/2821203005/diff/160001/content/renderer/media... content/renderer/media/mojo_audio_output_ipc_unittest.cc:141: DCHECK(!binding_); On 2017/05/18 09:16:50, o1ka wrote: > EXPECT_TRUE? Done. https://codereview.chromium.org/2821203005/diff/160001/content/renderer/media... content/renderer/media/mojo_audio_output_ipc_unittest.cc:142: EXPECT_TRUE(stream_); On 2017/05/18 09:16:50, o1ka wrote: > EXPECT_NE(nullptr, stream_) Done. https://codereview.chromium.org/2821203005/diff/160001/content/renderer/media... content/renderer/media/mojo_audio_output_ipc_unittest.cc:268: // an constructing the TestStreamProvider with a |&stream| EXPECTs that the On 2017/05/18 09:16:50, o1ka wrote: > nit: and Done. https://codereview.chromium.org/2821203005/diff/160001/content/renderer/media... content/renderer/media/mojo_audio_output_ipc_unittest.cc:384: FactoryDisconnectedBeforeAuthorizationReply_CallsAuthorizedAnyways) { On 2017/05/18 09:16:50, o1ka wrote: > Please add a comment explaining what is tested Done. https://codereview.chromium.org/2821203005/diff/160001/content/renderer/media... content/renderer/media/mojo_audio_output_ipc_unittest.cc:408: FactoryDisconnectedAfterAuthorizationReply_CallsAuthorizedOnlyOnce) { On 2017/05/18 09:16:50, o1ka wrote: > ditto Done. https://codereview.chromium.org/2821203005/diff/160001/content/renderer/media... content/renderer/media/mojo_audio_output_ipc_unittest.cc:510: On 2017/05/18 09:16:50, o1ka wrote: > WDYT of testing that we crash with DCHECK if CloseStream is not called? Added a couple of death tests.
maxmorin@chromium.org changed reviewers: + tommi@chromium.org
Tommi: PTAL at media/ and content/renderer/media/. Ken: Do you have anything to add about mojo_audio_output_ipc?
Nothing to add, LGTM
The CQ bit was checked by maxmorin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
maxmorin@chromium.org changed reviewers: + dalecurtis@chromium.org - tommi@chromium.org
Changing media reviewer as Tommi seems busy. Dale: PTAL.
Description was changed from ========== Add a mojo implementation of AudioOutputIPC. A couple of tricky points: * The requirement to always call CloseStream is added to media/audio/audio_output_ipc.h. This requirement isn't new, but it wasn't documented before. * If the RendererAudioOutputStreamFactory used by MojoAudioOutputIPC is destroyed prior to receiving authorization (e.g. because of frame destruction), we need to signal the AudioOutputDevice anyways to avoid waiting forever. The class AuthorizationCallbackWrapper is used for this, and it is implemented and documented in mojo_audio_output_ipc.cc. * Code for intergrating this into the AudioDeviceFactory is in https://codereview.chromium.org/2890753003 * Browser tests (using this and the dependent CL) is in https://codereview.chromium.org/2812883003 BUG=425368 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Add a mojo implementation of AudioOutputIPC. A couple of tricky points: * The requirement to always call CloseStream is added to media/audio/audio_output_ipc.h. This requirement isn't new, but it wasn't documented before. * If the RendererAudioOutputStreamFactory used by MojoAudioOutputIPC is destroyed prior to receiving authorization (e.g. because of frame destruction), we need to signal the AudioOutputDevice anyways to avoid waiting forever. The class AuthorizationCallbackWrapper is used for this, and it is implemented and documented in mojo_audio_output_ipc.cc. * Code for intergrating this into the AudioDeviceFactory is in https://codereview.chromium.org/2890753003 * Browser tests (using this and the dependent CL) is in https://codereview.chromium.org/2812883003/#ps300001. BUG=425368 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
https://codereview.chromium.org/2821203005/diff/180001/content/renderer/media... File content/renderer/media/mojo_audio_output_ipc.cc (right): https://codereview.chromium.org/2821203005/diff/180001/content/renderer/media... content/renderer/media/mojo_audio_output_ipc.cc:101: AuthorizationCallbackWrapper::Wrap( Hmm, why not just have a bool set here and then notify delegate_ in ~MojoAudioOutputIPC if it's still set? You're not actually using any of the parameters passed in, so I'm not sure why the wrap is needed. If I'm missing something here you could also achieve this by binding in a base::ScopedClosureRunner here that is released without running in ReceivedDeviceAuthorization. A la: base::BindOnce( &MojoAudioOutputIPC::ReceivedDeviceAuthorization, weak_factory_.GetWeakPtr(), base::MakeUnique<base::ScopedClosureRunner>(base::BindOnce( &MojoAudioOutputIPC::RecievedDeviceAuthorization, weak_factory_.GetWeakPtr(), media::OutputDeviceStatus::OUTPUT_DEVICE_STATUS_ERROR_INTERNAL, media::AudioParameters::UnavailableDeviceParams(), std::string()))); Then in ReceivedDeviceAuthorization() you'd do something like scoped_closure_runner->Release().Reset() so it's destroyed without running. https://codereview.chromium.org/2821203005/diff/180001/content/renderer/media... content/renderer/media/mojo_audio_output_ipc.cc:124: DCHECK(delegate_ == delegate); DCHECK_EQ https://codereview.chromium.org/2821203005/diff/180001/content/renderer/media... content/renderer/media/mojo_audio_output_ipc.cc:124: DCHECK(delegate_ == delegate); DCHECK_EQ https://codereview.chromium.org/2821203005/diff/180001/content/renderer/media... content/renderer/media/mojo_audio_output_ipc.cc:155: // Cancel any pending callbacks for this stream. Per above, if you use a bool you'd want to notify the delegate of uncalled callbacks. https://codereview.chromium.org/2821203005/diff/180001/content/renderer/media... content/renderer/media/mojo_audio_output_ipc.cc:202: // We wrap the callback here so that we are sure to always get the No wrapping done here? https://codereview.chromium.org/2821203005/diff/180001/content/renderer/media... content/renderer/media/mojo_audio_output_ipc.cc:209: void MojoAudioOutputIPC::RecievedDeviceAuthorization( Received https://codereview.chromium.org/2821203005/diff/180001/content/renderer/media... File content/renderer/media/mojo_audio_output_ipc.h (right): https://codereview.chromium.org/2821203005/diff/180001/content/renderer/media... content/renderer/media/mojo_audio_output_ipc.h:29: explicit MojoAudioOutputIPC(FactoryAccessor factory_accessor); Typically we end callbacks types with CB for clarity. https://codereview.chromium.org/2821203005/diff/180001/content/renderer/media... content/renderer/media/mojo_audio_output_ipc.h:34: void RequestDeviceAuthorization(media::AudioOutputIPCDelegate* delegate, Technically I/O parameters should be last. I realize this is an old API though so no need to change it here. Ditto on the method below. https://codereview.chromium.org/2821203005/diff/180001/content/renderer/media... content/renderer/media/mojo_audio_output_ipc.h:46: using AuthorizationCallback = mojom::RendererAudioOutputStreamFactory:: AuthorizationCB is sufficient if you want. https://codereview.chromium.org/2821203005/diff/180001/content/renderer/media... File content/renderer/media/mojo_audio_output_ipc_unittest.cc (right): https://codereview.chromium.org/2821203005/diff/180001/content/renderer/media... content/renderer/media/mojo_audio_output_ipc_unittest.cc:439: #if DCHECK_IS_ON() There's EXPECT_DCHECK_DEATH().
oh, lgtm otherwise defer to olka@ for final stamp if more changes are needed.
Description was changed from ========== Add a mojo implementation of AudioOutputIPC. A couple of tricky points: * The requirement to always call CloseStream is added to media/audio/audio_output_ipc.h. This requirement isn't new, but it wasn't documented before. * If the RendererAudioOutputStreamFactory used by MojoAudioOutputIPC is destroyed prior to receiving authorization (e.g. because of frame destruction), we need to signal the AudioOutputDevice anyways to avoid waiting forever. The class AuthorizationCallbackWrapper is used for this, and it is implemented and documented in mojo_audio_output_ipc.cc. * Code for intergrating this into the AudioDeviceFactory is in https://codereview.chromium.org/2890753003 * Browser tests (using this and the dependent CL) is in https://codereview.chromium.org/2812883003/#ps300001. BUG=425368 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Add a mojo implementation of AudioOutputIPC. A couple of tricky points: * The requirement to always call CloseStream is added to media/audio/audio_output_ipc.h. This requirement isn't new, but it wasn't documented before. * If the RendererAudioOutputStreamFactory used by MojoAudioOutputIPC is destroyed prior to receiving authorization (e.g. because of frame destruction), we need to signal the AudioOutputDevice anyways to avoid waiting forever. We pass a ScopedClosureRunner into the authorization callback to make sure we get a callback even if the factory is destroyed early. * Code for intergrating this into the AudioDeviceFactory is in https://codereview.chromium.org/2890753003 * Browser tests (using this and the dependent CL) in https://codereview.chromium.org/2812883003/#ps300001. BUG=425368 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Description was changed from ========== Add a mojo implementation of AudioOutputIPC. A couple of tricky points: * The requirement to always call CloseStream is added to media/audio/audio_output_ipc.h. This requirement isn't new, but it wasn't documented before. * If the RendererAudioOutputStreamFactory used by MojoAudioOutputIPC is destroyed prior to receiving authorization (e.g. because of frame destruction), we need to signal the AudioOutputDevice anyways to avoid waiting forever. We pass a ScopedClosureRunner into the authorization callback to make sure we get a callback even if the factory is destroyed early. * Code for intergrating this into the AudioDeviceFactory is in https://codereview.chromium.org/2890753003 * Browser tests (using this and the dependent CL) in https://codereview.chromium.org/2812883003/#ps300001. BUG=425368 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Add a mojo implementation of AudioOutputIPC. A couple of tricky points: * The requirement to always call CloseStream is added to media/audio/audio_output_ipc.h. This requirement isn't new, but it wasn't documented before. * If the RendererAudioOutputStreamFactory used by MojoAudioOutputIPC is destroyed prior to receiving authorization (e.g. because of frame destruction), we need to signal the AudioOutputDevice anyways to avoid waiting forever. We pass a ScopedClosureRunner into the authorization callback to make sure we get a callback even if the factory is destroyed early. * Code for intergrating this into the AudioDeviceFactory is in https://codereview.chromium.org/2890753003 * Browser tests (using this and the dependent CL) in https://codereview.chromium.org/2812883003/#ps380001. BUG=425368 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
maxmorin@chromium.org changed reviewers: + avi@chromium.org, tsepez@chromium.org
I reran the browser tests for PS9 to make sure they pass. Olga: I changed the code to use ScopedClosureRunner instead of AuthorizationCallbackWrapper. Do you have any comments on PS9? tsepez: Please RS the mojom (only an updated comment). Avi: Please RS BUILD files. https://codereview.chromium.org/2821203005/diff/180001/content/renderer/media... File content/renderer/media/mojo_audio_output_ipc.cc (right): https://codereview.chromium.org/2821203005/diff/180001/content/renderer/media... content/renderer/media/mojo_audio_output_ipc.cc:101: AuthorizationCallbackWrapper::Wrap( On 2017/05/24 01:36:54, DaleCurtis wrote: > Hmm, why not just have a bool set here and then notify delegate_ in > ~MojoAudioOutputIPC if it's still set? You're not actually using any of the > parameters passed in, so I'm not sure why the wrap is needed. > > If I'm missing something here you could also achieve this by binding in a > base::ScopedClosureRunner here that is released without running in > ReceivedDeviceAuthorization. A la: > > base::BindOnce( > &MojoAudioOutputIPC::ReceivedDeviceAuthorization, > weak_factory_.GetWeakPtr(), > base::MakeUnique<base::ScopedClosureRunner>(base::BindOnce( > &MojoAudioOutputIPC::RecievedDeviceAuthorization, > weak_factory_.GetWeakPtr(), > media::OutputDeviceStatus::OUTPUT_DEVICE_STATUS_ERROR_INTERNAL, > media::AudioParameters::UnavailableDeviceParams(), std::string()))); > > Then in ReceivedDeviceAuthorization() you'd do something like > scoped_closure_runner->Release().Reset() so it's destroyed without running. MojoAudioOutputIPC is owned by the AudioOutputDevice, so it's possible for it to stay around even if the corresponding RendererAudioOutputStreamFactoryPtr is disconnected in the case of the frame being closed (in which case we want to avoid waiting for the timeout). Nice tip about ScopedClosureRunner, I updated the code to use it. https://codereview.chromium.org/2821203005/diff/180001/content/renderer/media... content/renderer/media/mojo_audio_output_ipc.cc:124: DCHECK(delegate_ == delegate); On 2017/05/24 01:36:54, DaleCurtis wrote: > DCHECK_EQ Done. https://codereview.chromium.org/2821203005/diff/180001/content/renderer/media... content/renderer/media/mojo_audio_output_ipc.cc:155: // Cancel any pending callbacks for this stream. On 2017/05/24 01:36:54, DaleCurtis wrote: > Per above, if you use a bool you'd want to notify the delegate of uncalled > callbacks. There could be a sequence of events like "RequestDeviceAuth, CloseStream, RequestDeviceAuth" in which case we would have a hard time to keep track of if an authorization callback belongs to the first call or the second one using a bool. Using weak pointers fixes that. https://codereview.chromium.org/2821203005/diff/180001/content/renderer/media... content/renderer/media/mojo_audio_output_ipc.cc:202: // We wrap the callback here so that we are sure to always get the On 2017/05/24 01:36:54, DaleCurtis wrote: > No wrapping done here? Done. https://codereview.chromium.org/2821203005/diff/180001/content/renderer/media... content/renderer/media/mojo_audio_output_ipc.cc:209: void MojoAudioOutputIPC::RecievedDeviceAuthorization( On 2017/05/24 01:36:54, DaleCurtis wrote: > Received Done. https://codereview.chromium.org/2821203005/diff/180001/content/renderer/media... File content/renderer/media/mojo_audio_output_ipc.h (right): https://codereview.chromium.org/2821203005/diff/180001/content/renderer/media... content/renderer/media/mojo_audio_output_ipc.h:29: explicit MojoAudioOutputIPC(FactoryAccessor factory_accessor); On 2017/05/24 01:36:54, DaleCurtis wrote: > Typically we end callbacks types with CB for clarity. Done. https://codereview.chromium.org/2821203005/diff/180001/content/renderer/media... content/renderer/media/mojo_audio_output_ipc.h:34: void RequestDeviceAuthorization(media::AudioOutputIPCDelegate* delegate, On 2017/05/24 01:36:54, DaleCurtis wrote: > Technically I/O parameters should be last. I realize this is an old API though > so no need to change it here. Ditto on the method below. Yes, maybe we can revisit the AudioOutputIPC interface later, but doing it now would also require changing AudioMessageFilter so I'll leave this as it is. https://codereview.chromium.org/2821203005/diff/180001/content/renderer/media... content/renderer/media/mojo_audio_output_ipc.h:46: using AuthorizationCallback = mojom::RendererAudioOutputStreamFactory:: On 2017/05/24 01:36:55, DaleCurtis wrote: > AuthorizationCB is sufficient if you want. Done. https://codereview.chromium.org/2821203005/diff/180001/content/renderer/media... File content/renderer/media/mojo_audio_output_ipc_unittest.cc (right): https://codereview.chromium.org/2821203005/diff/180001/content/renderer/media... content/renderer/media/mojo_audio_output_ipc_unittest.cc:439: #if DCHECK_IS_ON() On 2017/05/24 01:36:55, DaleCurtis wrote: > There's EXPECT_DCHECK_DEATH(). Done.
The CQ bit was checked by maxmorin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
still lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM on adding comment.
Thanks everyone for the fast and helpful reviews!
The CQ bit was checked by maxmorin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org, dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/2821203005/#ps200001 (title: "Use ScopedClosureRunner.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1496211270458810,
"parent_rev": "21b9affc7f019d878b3118c2c0833822d78dfec4", "commit_rev":
"aa9020e577bcd1f459094168b00a07aaba063a9f"}
Message was sent while issue was closed.
Description was changed from ========== Add a mojo implementation of AudioOutputIPC. A couple of tricky points: * The requirement to always call CloseStream is added to media/audio/audio_output_ipc.h. This requirement isn't new, but it wasn't documented before. * If the RendererAudioOutputStreamFactory used by MojoAudioOutputIPC is destroyed prior to receiving authorization (e.g. because of frame destruction), we need to signal the AudioOutputDevice anyways to avoid waiting forever. We pass a ScopedClosureRunner into the authorization callback to make sure we get a callback even if the factory is destroyed early. * Code for intergrating this into the AudioDeviceFactory is in https://codereview.chromium.org/2890753003 * Browser tests (using this and the dependent CL) in https://codereview.chromium.org/2812883003/#ps380001. BUG=425368 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Add a mojo implementation of AudioOutputIPC. A couple of tricky points: * The requirement to always call CloseStream is added to media/audio/audio_output_ipc.h. This requirement isn't new, but it wasn't documented before. * If the RendererAudioOutputStreamFactory used by MojoAudioOutputIPC is destroyed prior to receiving authorization (e.g. because of frame destruction), we need to signal the AudioOutputDevice anyways to avoid waiting forever. We pass a ScopedClosureRunner into the authorization callback to make sure we get a callback even if the factory is destroyed early. * Code for intergrating this into the AudioDeviceFactory is in https://codereview.chromium.org/2890753003 * Browser tests (using this and the dependent CL) in https://codereview.chromium.org/2812883003/#ps380001. BUG=425368 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2821203005 Cr-Commit-Position: refs/heads/master@{#475827} Committed: https://chromium.googlesource.com/chromium/src/+/aa9020e577bcd1f459094168b00a... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:200001) as https://chromium.googlesource.com/chromium/src/+/aa9020e577bcd1f459094168b00a... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
