Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2697663003:
Add mojo interface+impl creation of audio streams. (Closed)
|
Created:
3 years, 10 months ago by Max Morin Modified:
3 years, 8 months ago Reviewers:
Ken Rockot(use gerrit already), Avi (use Gerrit), Alexei Svitkine (slow), DaleCurtis, o1ka, Mike West CC:
Aaron Boodman, abarth-chromium, alokp+watch_chromium.org, asvitkine+watch_chromium.org, audio-mojo-cl_google.com, chromium-reviews, darin (slow to review), darin-cc_chromium.org, feature-media-reviews_chromium.org, Guido Urdaneta, jam, miu+watch_chromium.org, ossu-chromium, posciak+watch_chromium.org, qsr+mojo_chromium.org, Solis, viettrungluu+watch_chromium.org, xjz+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd mojo interface+impl creation of audio streams.
Summary:
* RendererAudioOutputStreamFactory is an interface for creating
AudioOutputProviders.
* Since a lot of data is common for all factories of a single renderer
process, it is held in a context object.
(AudioOutputStreamFactoryContext is per-process and
RendererAudioOutputStreamFactory is per-frame).
* Replaces AudioRendererHost (eventually, after field trials).
* Follow-up on https://codereview.chromium.org/2697793002/.
* Design doc at https://docs.google.com/document/d/1awQoajq_DLmz2AIU9iweC0zEYlVuHCvEIRQepeYyxh8/edit
* audio_output_stream_factory_context_impl_unittest.cc is actually an
integration test, but it seems common to suffix test files with
"_unittest" anyways, so I did that.
BUG=425368
Review-Url: https://codereview.chromium.org/2697663003
Cr-Commit-Position: refs/heads/master@{#461103}
Committed: https://chromium.googlesource.com/chromium/src/+/8757b429c30f14d20697f7c6fe4dfdd510cf0b7f
Patch Set 1 #
Total comments: 22
Patch Set 2 #
Total comments: 34
Patch Set 3 #
Total comments: 3
Patch Set 4 #
Total comments: 30
Patch Set 5 : ++tests, other comments #
Total comments: 24
Patch Set 6 : Dale's comments #
Total comments: 6
Patch Set 7 : Get origin from frame host. #Patch Set 8 : Take AM pointer in RAOSFCI constructor. #
Total comments: 2
Patch Set 9 : Use mojo::ReportBadMessage and add a thread check. #Messages
Total messages: 92 (58 generated)
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...
Description was changed from ========== Add mojo interface+impl for audio stream creation. Summary: * RendererAudioOutputService is an interface for Creating AudioOutputs. * Replaces AudioRendererHost (eventually, after field trials). * content/common/media/audio_output.mojom defines the IPC interface. * Depends on https://codereview.chromium.org/2697793002/. * Split off from https://codereview.chromium.org/2319493002/. * Design doc at https://docs.google.com/document/d/1awQoajq_DLmz2AIU9iweC0zEYlVuHCvEIRQepeYyx... BUG=425368 Add mojo interface+impl for audio stream control. ========== to ========== Add mojo interface+impl for audio stream creation. Summary: * RendererAudioOutputService is an interface for Creating AudioOutputs. * Replaces AudioRendererHost (eventually, after field trials). * content/common/media/audio_output.mojom defines the IPC interface. * Depends on https://codereview.chromium.org/2697793002/. * Split off from https://codereview.chromium.org/2319493002/. * Design doc at https://docs.google.com/document/d/1awQoajq_DLmz2AIU9iweC0zEYlVuHCvEIRQepeYyx... BUG=425368 Add mojo interface+impl for audio stream control. ==========
maxmorin@chromium.org changed reviewers: + olka@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: 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...
Description was changed from ========== Add mojo interface+impl for audio stream creation. Summary: * RendererAudioOutputService is an interface for Creating AudioOutputs. * Replaces AudioRendererHost (eventually, after field trials). * content/common/media/audio_output.mojom defines the IPC interface. * Depends on https://codereview.chromium.org/2697793002/. * Split off from https://codereview.chromium.org/2319493002/. * Design doc at https://docs.google.com/document/d/1awQoajq_DLmz2AIU9iweC0zEYlVuHCvEIRQepeYyx... BUG=425368 Add mojo interface+impl for audio stream control. ========== to ========== Add mojo interface+impl for audio stream creation. Summary: * RendererAudioOutputService is an interface for Creating AudioOutputs. * Replaces AudioRendererHost (eventually, after field trials). * content/common/media/audio_output.mojom defines the IPC interface. * Depends on https://codereview.chromium.org/2697793002/. * Split off from https://codereview.chromium.org/2319493002/. * Design doc at https://docs.google.com/document/d/1awQoajq_DLmz2AIU9iweC0zEYlVuHCvEIRQepeYyx... AudioOutputs and RendererAudioServices cannot be held in StrongBindingSets, since they may encounter errors other than binding errors, in which case they must be able to signal that they should be destroyed. BUG=425368 Add mojo interface+impl for audio stream control. ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) 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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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.
Description was changed from ========== Add mojo interface+impl for audio stream creation. Summary: * RendererAudioOutputService is an interface for Creating AudioOutputs. * Replaces AudioRendererHost (eventually, after field trials). * content/common/media/audio_output.mojom defines the IPC interface. * Depends on https://codereview.chromium.org/2697793002/. * Split off from https://codereview.chromium.org/2319493002/. * Design doc at https://docs.google.com/document/d/1awQoajq_DLmz2AIU9iweC0zEYlVuHCvEIRQepeYyx... AudioOutputs and RendererAudioServices cannot be held in StrongBindingSets, since they may encounter errors other than binding errors, in which case they must be able to signal that they should be destroyed. BUG=425368 Add mojo interface+impl for audio stream control. ========== to ========== Add mojo interface+impl creation of audio streams. Summary: * RendererAudioOutputStreamFactory is an interface for creating AudioOutputProviders. * Since a lot of data is common for all factories of a single renderer process, it is held in a context object. (AudioOutputStreamFactoryContext is per-process and RendererAudioOutputStreamFactory is per-frame). * Replaces AudioRendererHost (eventually, after field trials). * content/common/media/audio_output_factory.mojom defines the IPC interface. * Follow-up on https://codereview.chromium.org/2697793002/. * Design doc at https://docs.google.com/document/d/1awQoajq_DLmz2AIU9iweC0zEYlVuHCvEIRQepeYyx... * audio_output_stream_factory_context_impl_unittest.cc is actually an integration test, but it seems common to suffix test files with "_unittest" anyways, so I did that. RendererAudioOutputStreamFactory cannot be held in StrongBindingSets, since they may encounter errors other than binding errors, in which case they must be able to signal that they should be destroyed. BUG=425368 ==========
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
The CQ bit was checked by maxmorin@chromium.org
The CQ bit was unchecked by maxmorin@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Olga: PTAL. Ossu, Solis: If you like, you can also have a look, especially regarding what should be better documented.
Some high-level comments/nits so far. https://codereview.chromium.org/2697663003/diff/140001/content/browser/render... File content/browser/renderer_host/media/audio_output_stream_factory_context.h (right): https://codereview.chromium.org/2697663003/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_stream_factory_context.h:27: class CONTENT_EXPORT AudioOutputStreamFactoryContext { I'd prefer RendererAudioOutputStreamFactoryContext for the name, it would be more descriptive. https://codereview.chromium.org/2697663003/diff/140001/content/browser/render... File content/browser/renderer_host/media/audio_output_stream_factory_context_impl.h (right): https://codereview.chromium.org/2697663003/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_stream_factory_context_impl.h:24: // handles requests for mojom::RendererAudioOutputStreamFactory instances. This sounds confusing, since "request" is also a Mojo term. "Serves as a factory for.." maybe? https://codereview.chromium.org/2697663003/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_stream_factory_context_impl.h:35: void CreateFactory( Could you add a comment on its usage? https://codereview.chromium.org/2697663003/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_stream_factory_context_impl.h:39: // AudioOutputStreamFactoryContext impl: implementation. https://codereview.chromium.org/2697663003/diff/140001/content/browser/render... File content/browser/renderer_host/media/render_frame_audio_output_stream_factory.cc (right): https://codereview.chromium.org/2697663003/diff/140001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory.cc:66: return; Could we substitute this pattern with a strong binding set here? https://codereview.chromium.org/2697663003/diff/140001/content/browser/render... File content/browser/renderer_host/media/render_frame_audio_output_stream_factory.h (right): https://codereview.chromium.org/2697663003/diff/140001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory.h:58: AudioOutputStreamFactoryContext* context_; const pointer? https://codereview.chromium.org/2697663003/diff/140001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory.h:59: OutputStreamProviderVector streams_; |stream_providers_|? Could you add a comment on that stream providers own actual streams? Could we have a pseudo-graphic scheme of ownership of all the objects somewhere? https://codereview.chromium.org/2697663003/diff/140001/content/common/media/a... File content/common/media/audio_output_stream_factory.mojom (right): https://codereview.chromium.org/2697663003/diff/140001/content/common/media/a... content/common/media/audio_output_stream_factory.mojom:1: // Copyright 2016 The Chromium Authors. All rights reserved. 2017 https://codereview.chromium.org/2697663003/diff/140001/content/common/media/a... content/common/media/audio_output_stream_factory.mojom:12: interface RendererAudioOutputStreamFactory { Should the file be named "renderer_audio_output_stream_factory.mojom" maybe? We'll have other stream factories in the future (the one in AudioProcess, for example) https://codereview.chromium.org/2697663003/diff/140001/content/common/media/a... content/common/media/audio_output_stream_factory.mojom:17: media.mojom.AudioOutputStreamProvider&? stream_provider_request, Could you clarify in the comment the usecase when AudioOutputStreamProviderRequest is not provided?
Some high-level comments/nits so far.
CC: guidou@ for device authorization logic (see audio_output_stream_factory_context_impl.*)
Description was changed from ========== Add mojo interface+impl creation of audio streams. Summary: * RendererAudioOutputStreamFactory is an interface for creating AudioOutputProviders. * Since a lot of data is common for all factories of a single renderer process, it is held in a context object. (AudioOutputStreamFactoryContext is per-process and RendererAudioOutputStreamFactory is per-frame). * Replaces AudioRendererHost (eventually, after field trials). * content/common/media/audio_output_factory.mojom defines the IPC interface. * Follow-up on https://codereview.chromium.org/2697793002/. * Design doc at https://docs.google.com/document/d/1awQoajq_DLmz2AIU9iweC0zEYlVuHCvEIRQepeYyx... * audio_output_stream_factory_context_impl_unittest.cc is actually an integration test, but it seems common to suffix test files with "_unittest" anyways, so I did that. RendererAudioOutputStreamFactory cannot be held in StrongBindingSets, since they may encounter errors other than binding errors, in which case they must be able to signal that they should be destroyed. BUG=425368 ========== to ========== Add mojo interface+impl creation of audio streams. Summary: * RendererAudioOutputStreamFactory is an interface for creating AudioOutputProviders. * Since a lot of data is common for all factories of a single renderer process, it is held in a context object. (AudioOutputStreamFactoryContext is per-process and RendererAudioOutputStreamFactory is per-frame). * Replaces AudioRendererHost (eventually, after field trials). * content/common/media/audio_output_factory.mojom defines the IPC interface. * Follow-up on https://codereview.chromium.org/2697793002/. * Design doc at https://docs.google.com/document/d/1awQoajq_DLmz2AIU9iweC0zEYlVuHCvEIRQepeYyx... * audio_output_stream_factory_context_impl_unittest.cc is actually an integration test, but it seems common to suffix test files with "_unittest" anyways, so I did that. BUG=425368 ==========
https://codereview.chromium.org/2697663003/diff/140001/content/browser/render... File content/browser/renderer_host/media/audio_output_stream_factory_context.h (right): https://codereview.chromium.org/2697663003/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_stream_factory_context.h:27: class CONTENT_EXPORT AudioOutputStreamFactoryContext { On 2017/03/21 16:20:17, o1ka wrote: > I'd prefer RendererAudioOutputStreamFactoryContext for the name, it would be > more descriptive. Done. https://codereview.chromium.org/2697663003/diff/140001/content/browser/render... File content/browser/renderer_host/media/audio_output_stream_factory_context_impl.h (right): https://codereview.chromium.org/2697663003/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_stream_factory_context_impl.h:24: // handles requests for mojom::RendererAudioOutputStreamFactory instances. On 2017/03/21 16:20:17, o1ka wrote: > This sounds confusing, since "request" is also a Mojo term. "Serves as a factory > for.." maybe? "request" means mojo request. https://codereview.chromium.org/2697663003/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_stream_factory_context_impl.h:35: void CreateFactory( On 2017/03/21 16:20:17, o1ka wrote: > Could you add a comment on its usage? Done. https://codereview.chromium.org/2697663003/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_stream_factory_context_impl.h:39: // AudioOutputStreamFactoryContext impl: On 2017/03/21 16:20:17, o1ka wrote: > implementation. Done. https://codereview.chromium.org/2697663003/diff/140001/content/browser/render... File content/browser/renderer_host/media/render_frame_audio_output_stream_factory.cc (right): https://codereview.chromium.org/2697663003/diff/140001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory.cc:66: return; On 2017/03/21 16:20:17, o1ka wrote: > Could we substitute this pattern with a strong binding set here? Done. https://codereview.chromium.org/2697663003/diff/140001/content/browser/render... File content/browser/renderer_host/media/render_frame_audio_output_stream_factory.h (right): https://codereview.chromium.org/2697663003/diff/140001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory.h:58: AudioOutputStreamFactoryContext* context_; On 2017/03/21 16:20:17, o1ka wrote: > const pointer? Done. https://codereview.chromium.org/2697663003/diff/140001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory.h:59: OutputStreamProviderVector streams_; On 2017/03/21 16:20:17, o1ka wrote: > |stream_providers_|? > Could you add a comment on that stream providers own actual streams? Done. > Could we have a pseudo-graphic scheme of ownership of all the objects somewhere? You want something like https://docs.google.com/drawings/d/1KsGJnLhNY54EaNcOKVhtzKEpoPrpqy2BanA62vGlu... with ascii boxes? https://codereview.chromium.org/2697663003/diff/140001/content/common/media/a... File content/common/media/audio_output_stream_factory.mojom (right): https://codereview.chromium.org/2697663003/diff/140001/content/common/media/a... content/common/media/audio_output_stream_factory.mojom:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/03/21 16:20:17, o1ka wrote: > 2017 Done. https://codereview.chromium.org/2697663003/diff/140001/content/common/media/a... content/common/media/audio_output_stream_factory.mojom:12: interface RendererAudioOutputStreamFactory { On 2017/03/21 16:20:17, o1ka wrote: > Should the file be named "renderer_audio_output_stream_factory.mojom" maybe? > We'll have other stream factories in the future (the one in AudioProcess, for > example) Done. https://codereview.chromium.org/2697663003/diff/140001/content/common/media/a... content/common/media/audio_output_stream_factory.mojom:17: media.mojom.AudioOutputStreamProvider&? stream_provider_request, On 2017/03/21 16:20:17, o1ka wrote: > Could you clarify in the comment the usecase when > AudioOutputStreamProviderRequest is not provided? Changed so that request must always be provided.
Great job! Testing a epic. https://codereview.chromium.org/2697663003/diff/140001/content/browser/render... File content/browser/renderer_host/media/audio_output_stream_factory_context_impl.h (right): https://codereview.chromium.org/2697663003/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_output_stream_factory_context_impl.h:24: // handles requests for mojom::RendererAudioOutputStreamFactory instances. On 2017/03/22 13:18:29, Max Morin wrote: > On 2017/03/21 16:20:17, o1ka wrote: > > This sounds confusing, since "request" is also a Mojo term. "Serves as a > factory > > for.." maybe? > > "request" means mojo request. Ah, now I see it when you clarified the usage. https://codereview.chromium.org/2697663003/diff/140001/content/browser/render... File content/browser/renderer_host/media/render_frame_audio_output_stream_factory.h (right): https://codereview.chromium.org/2697663003/diff/140001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory.h:59: OutputStreamProviderVector streams_; On 2017/03/22 13:18:29, Max Morin wrote: > On 2017/03/21 16:20:17, o1ka wrote: > > |stream_providers_|? > > Could you add a comment on that stream providers own actual streams? > Done. > > > Could we have a pseudo-graphic scheme of ownership of all the objects > somewhere? > > You want something like > https://docs.google.com/drawings/d/1KsGJnLhNY54EaNcOKVhtzKEpoPrpqy2BanA62vGlu... > with ascii boxes? Yes, something like this https://cs.chromium.org/chromium/src/media/audio/audio_debug_recording_manage... Context is probably a better header for that. https://codereview.chromium.org/2697663003/diff/160001/content/browser/render... File content/browser/renderer_host/media/render_frame_audio_output_stream_factory.cc (right): https://codereview.chromium.org/2697663003/diff/160001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory.cc:12: #include "content/browser/media/capture/audio_mirroring_manager.h" Is it needed? https://codereview.chromium.org/2697663003/diff/160001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory.cc:15: #include "media/audio/audio_manager.h" Is it needed? https://codereview.chromium.org/2697663003/diff/160001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory.cc:104: context_->GetSalt(), origin, raw_device_id) Instead of exposing salt, can we make context method for getting HMAC? We don;t really need to know about MediaStreamManager here, do we? https://codereview.chromium.org/2697663003/diff/160001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory.cc:111: stream_providers_.begin(), stream_providers_.end(), Thread check? https://codereview.chromium.org/2697663003/diff/160001/content/browser/render... File content/browser/renderer_host/media/render_frame_audio_output_stream_factory_unittest.cc (right): https://codereview.chromium.org/2697663003/diff/160001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory_unittest.cc:59: const media::AudioParameters params( non-POD https://codereview.chromium.org/2697663003/diff/160001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory_unittest.cc:197: TEST_F(RenderFrameAudioOutputStreamFactoryTest, AuthWithStreamRequest) { The name is not very descriptive. Could you add a comment explaining what is being tested? https://codereview.chromium.org/2697663003/diff/160001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory_unittest.cc:243: media::AudioParameters params2; could we not count them? :) https://codereview.chromium.org/2697663003/diff/160001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory_unittest.cc:260: if (sizeof(int) >= sizeof(int64_t)) { Alternatively, we may skip compiling it. https://codereview.chromium.org/2697663003/diff/160001/content/browser/render... File content/browser/renderer_host/media/renderer_audio_output_stream_factory_context_impl_unittest.cc (right): https://codereview.chromium.org/2697663003/diff/160001/content/browser/render... content/browser/renderer_host/media/renderer_audio_output_stream_factory_context_impl_unittest.cc:66: const media::AudioParameters params( AudioParameters is non-POD. https://codereview.chromium.org/2697663003/diff/160001/content/browser/render... content/browser/renderer_host/media/renderer_audio_output_stream_factory_context_impl_unittest.cc:213: void AuthCallback2(base::OnceClosure sync_closure, What "2" stands for here? https://codereview.chromium.org/2697663003/diff/160001/content/browser/render... content/browser/renderer_host/media/renderer_audio_output_stream_factory_context_impl_unittest.cc:226: // "Renderer-side" audio client. Sends a signal from a dedicated thread. "Sends" is a bit confusing, since we are in a pull model. Something like "Fulfills audio output stream callbacks by rendering sine wave"? https://codereview.chromium.org/2697663003/diff/160001/content/browser/render... content/browser/renderer_host/media/renderer_audio_output_stream_factory_context_impl_unittest.cc:228: class TestClient : public base::PlatformThread::Delegate { Some more descriptive name maybe? https://codereview.chromium.org/2697663003/diff/160001/content/browser/render... content/browser/renderer_host/media/renderer_audio_output_stream_factory_context_impl_unittest.cc:278: PCHECK(sizeof(pending_data) == bytes_read); output diagnostics? https://codereview.chromium.org/2697663003/diff/160001/content/browser/render... content/browser/renderer_host/media/renderer_audio_output_stream_factory_context_impl_unittest.cc:286: PCHECK(sizeof(buffer_index) == bytes_written); ditto https://codereview.chromium.org/2697663003/diff/160001/content/browser/render... content/browser/renderer_host/media/renderer_audio_output_stream_factory_context_impl_unittest.cc:292: Signal s_; a more descriptive name? https://codereview.chromium.org/2697663003/diff/160001/content/browser/render... content/browser/renderer_host/media/renderer_audio_output_stream_factory_context_impl_unittest.cc:358: media::AudioParameters params2; Could you give it a more descriptive name if possible? https://codereview.chromium.org/2697663003/diff/160001/content/browser/render... content/browser/renderer_host/media/renderer_audio_output_stream_factory_context_impl_unittest.cc:384: } Should we in a follow-up CL extend this with play/pause tests? Add a todo?
Patchset #3 (id:180001) has been deleted
Patchset #3 (id:200001) has been deleted
Description was changed from ========== Add mojo interface+impl creation of audio streams. Summary: * RendererAudioOutputStreamFactory is an interface for creating AudioOutputProviders. * Since a lot of data is common for all factories of a single renderer process, it is held in a context object. (AudioOutputStreamFactoryContext is per-process and RendererAudioOutputStreamFactory is per-frame). * Replaces AudioRendererHost (eventually, after field trials). * content/common/media/audio_output_factory.mojom defines the IPC interface. * Follow-up on https://codereview.chromium.org/2697793002/. * Design doc at https://docs.google.com/document/d/1awQoajq_DLmz2AIU9iweC0zEYlVuHCvEIRQepeYyx... * audio_output_stream_factory_context_impl_unittest.cc is actually an integration test, but it seems common to suffix test files with "_unittest" anyways, so I did that. BUG=425368 ========== to ========== Add mojo interface+impl creation of audio streams. Summary: * RendererAudioOutputStreamFactory is an interface for creating AudioOutputProviders. * Since a lot of data is common for all factories of a single renderer process, it is held in a context object. (AudioOutputStreamFactoryContext is per-process and RendererAudioOutputStreamFactory is per-frame). * Replaces AudioRendererHost (eventually, after field trials). * Follow-up on https://codereview.chromium.org/2697793002/. * Design doc at https://docs.google.com/document/d/1awQoajq_DLmz2AIU9iweC0zEYlVuHCvEIRQepeYyx... * audio_output_stream_factory_context_impl_unittest.cc is actually an integration test, but it seems common to suffix test files with "_unittest" anyways, so I did that. BUG=425368 ==========
Thank you Olga. Since you didn't have major comments, I'm adding some more reviewers. https://codereview.chromium.org/2697663003/diff/160001/content/browser/render... File content/browser/renderer_host/media/render_frame_audio_output_stream_factory.cc (right): https://codereview.chromium.org/2697663003/diff/160001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory.cc:12: #include "content/browser/media/capture/audio_mirroring_manager.h" On 2017/03/23 09:39:43, o1ka wrote: > Is it needed? Done. https://codereview.chromium.org/2697663003/diff/160001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory.cc:15: #include "media/audio/audio_manager.h" On 2017/03/23 09:39:43, o1ka wrote: > Is it needed? Done. https://codereview.chromium.org/2697663003/diff/160001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory.cc:104: context_->GetSalt(), origin, raw_device_id) On 2017/03/23 09:39:44, o1ka wrote: > Instead of exposing salt, can we make context method for getting HMAC? > We don;t really need to know about MediaStreamManager here, do we? Done. https://codereview.chromium.org/2697663003/diff/160001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory.cc:111: stream_providers_.begin(), stream_providers_.end(), On 2017/03/23 09:39:44, o1ka wrote: > Thread check? Done. https://codereview.chromium.org/2697663003/diff/160001/content/browser/render... File content/browser/renderer_host/media/render_frame_audio_output_stream_factory_unittest.cc (right): https://codereview.chromium.org/2697663003/diff/160001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory_unittest.cc:59: const media::AudioParameters params( On 2017/03/23 09:39:44, o1ka wrote: > non-POD Done. https://codereview.chromium.org/2697663003/diff/160001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory_unittest.cc:197: TEST_F(RenderFrameAudioOutputStreamFactoryTest, AuthWithStreamRequest) { On 2017/03/23 09:39:44, o1ka wrote: > The name is not very descriptive. Could you add a comment explaining what is > being tested? Done. https://codereview.chromium.org/2697663003/diff/160001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory_unittest.cc:243: media::AudioParameters params2; On 2017/03/23 09:39:44, o1ka wrote: > could we not count them? :) Done. https://codereview.chromium.org/2697663003/diff/160001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory_unittest.cc:260: if (sizeof(int) >= sizeof(int64_t)) { On 2017/03/23 09:39:44, o1ka wrote: > Alternatively, we may skip compiling it. I don't think it's a common case, so I'd rather not mess with preprocessor nonsense. https://codereview.chromium.org/2697663003/diff/160001/content/browser/render... File content/browser/renderer_host/media/renderer_audio_output_stream_factory_context_impl_unittest.cc (right): https://codereview.chromium.org/2697663003/diff/160001/content/browser/render... content/browser/renderer_host/media/renderer_audio_output_stream_factory_context_impl_unittest.cc:66: const media::AudioParameters params( On 2017/03/23 09:39:44, o1ka wrote: > AudioParameters is non-POD. Done. https://codereview.chromium.org/2697663003/diff/160001/content/browser/render... content/browser/renderer_host/media/renderer_audio_output_stream_factory_context_impl_unittest.cc:213: void AuthCallback2(base::OnceClosure sync_closure, On 2017/03/23 09:39:44, o1ka wrote: > What "2" stands for here? Removed https://codereview.chromium.org/2697663003/diff/160001/content/browser/render... content/browser/renderer_host/media/renderer_audio_output_stream_factory_context_impl_unittest.cc:226: // "Renderer-side" audio client. Sends a signal from a dedicated thread. On 2017/03/23 09:39:45, o1ka wrote: > "Sends" is a bit confusing, since we are in a pull model. Something like > "Fulfills audio output stream callbacks by rendering sine wave"? Done. https://codereview.chromium.org/2697663003/diff/160001/content/browser/render... content/browser/renderer_host/media/renderer_audio_output_stream_factory_context_impl_unittest.cc:228: class TestClient : public base::PlatformThread::Delegate { On 2017/03/23 09:39:44, o1ka wrote: > Some more descriptive name maybe? Done. https://codereview.chromium.org/2697663003/diff/160001/content/browser/render... content/browser/renderer_host/media/renderer_audio_output_stream_factory_context_impl_unittest.cc:278: PCHECK(sizeof(pending_data) == bytes_read); On 2017/03/23 09:39:45, o1ka wrote: > output diagnostics? PTAL https://codereview.chromium.org/2697663003/diff/160001/content/browser/render... content/browser/renderer_host/media/renderer_audio_output_stream_factory_context_impl_unittest.cc:286: PCHECK(sizeof(buffer_index) == bytes_written); On 2017/03/23 09:39:44, o1ka wrote: > ditto Done. https://codereview.chromium.org/2697663003/diff/160001/content/browser/render... content/browser/renderer_host/media/renderer_audio_output_stream_factory_context_impl_unittest.cc:292: Signal s_; On 2017/03/23 09:39:44, o1ka wrote: > a more descriptive name? Done. https://codereview.chromium.org/2697663003/diff/160001/content/browser/render... content/browser/renderer_host/media/renderer_audio_output_stream_factory_context_impl_unittest.cc:358: media::AudioParameters params2; On 2017/03/23 09:39:45, o1ka wrote: > Could you give it a more descriptive name if possible? Done. https://codereview.chromium.org/2697663003/diff/160001/content/browser/render... content/browser/renderer_host/media/renderer_audio_output_stream_factory_context_impl_unittest.cc:384: } On 2017/03/23 09:39:44, o1ka wrote: > Should we in a follow-up CL extend this with play/pause tests? Add a todo? Done. https://codereview.chromium.org/2697663003/diff/220001/content/browser/BUILD.gn File content/browser/BUILD.gn (left): https://codereview.chromium.org/2697663003/diff/220001/content/browser/BUILD.... content/browser/BUILD.gn:1907: deps += [ "//media/mojo/interfaces" ] This is unconditionally included above anyways.
maxmorin@chromium.org changed reviewers: + dalecurtis@chromium.org, rockot@chromium.org
Dale: PTAL.
Ken: PTAL at mojo stuff
in content/common/media/renderer_audio_output_stream_factory.mojom and
content/browser/renderer_host/media/render_frame_audio_output_stream_factory.{cc,h}.
Thanks!
Said files LGTM https://codereview.chromium.org/2697663003/diff/220001/content/browser/render... File content/browser/renderer_host/media/render_frame_audio_output_stream_factory.cc (right): https://codereview.chromium.org/2697663003/diff/220001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory.cc:67: base::Passed(std::move(stream_provider_request)), callback)); nit: base::Passed(&stream_provider_request) is much more common https://codereview.chromium.org/2697663003/diff/220001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory.cc:90: base::WrapUnique<media::mojom::AudioOutputStreamProvider>( nit: Please use base::MakeUnique<Foo>(args...) instead of base::WrapUnique(new Foo(args...));
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...
https://codereview.chromium.org/2697663003/diff/240001/content/browser/bad_me... File content/browser/bad_message.h (right): https://codereview.chromium.org/2697663003/diff/240001/content/browser/bad_me... content/browser/bad_message.h:188: RFAOSF_OUT_OF_RANGE_INTEGER = 164, Quite the acronym stew here :) I guess it's no worse than some of the above though. https://codereview.chromium.org/2697663003/diff/240001/content/browser/render... File content/browser/renderer_host/media/render_frame_audio_output_stream_factory.cc (right): https://codereview.chromium.org/2697663003/diff/240001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory.cc:34: DCHECK(thread_checker_.CalledOnValidThread()); Meaningless right? It's bound to the thread first used on. https://codereview.chromium.org/2697663003/diff/240001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory.cc:40: } DCHECK(stream_providers_.empty()) ? Otherwise you may have dangling callbacks holding unretained refs to this. https://codereview.chromium.org/2697663003/diff/240001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory.cc:52: UMALogDeviceAuthorizationTime(auth_start_time); We don't log this in the existing ARH, so it will skew metrics if you add it here. Do we want that? https://codereview.chromium.org/2697663003/diff/240001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory.cc:64: render_frame_id_, static_cast<int>(session_id), device_id, origin, Hmm, why do we bother sending int64_t across if we only want an int? Are you planning to change the message later to just be an int? https://codereview.chromium.org/2697663003/diff/240001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory.cc:96: base::Unretained(this)))); See note above; seems like this could lead to callbacks with bad refs. https://codereview.chromium.org/2697663003/diff/240001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory.cc:108: auto it = std::find_if( base::EraseIf, if you want. https://codereview.chromium.org/2697663003/diff/240001/content/browser/render... File content/browser/renderer_host/media/render_frame_audio_output_stream_factory.h (right): https://codereview.chromium.org/2697663003/diff/240001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory.h:20: // Handles a RendererAudioOutputStreamFactory request for a render frame host, Again technically descriptive, but doesn't really say what it does. https://codereview.chromium.org/2697663003/diff/240001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory.h:38: int64_t session_id, For session_id, should we start using base::UnguessableToken? https://codereview.chromium.org/2697663003/diff/240001/content/browser/render... File content/browser/renderer_host/media/render_frame_audio_output_stream_factory_unittest.cc (right): https://codereview.chromium.org/2697663003/diff/240001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory_unittest.cc:1: // Copyright 2017 The Chromium Authors. All rights reserved. Don't see a test which utilizes the RemoveStream path? https://codereview.chromium.org/2697663003/diff/240001/content/browser/render... File content/browser/renderer_host/media/renderer_audio_output_stream_factory_context.h (right): https://codereview.chromium.org/2697663003/diff/240001/content/browser/render... content/browser/renderer_host/media/renderer_audio_output_stream_factory_context.h:25: // RendererAudioOutputStreamFactoryContext provides functions common to all This is technically descriptive but doesn't really talk about the purpose of the class. https://codereview.chromium.org/2697663003/diff/240001/content/browser/render... content/browser/renderer_host/media/renderer_audio_output_stream_factory_context.h:36: virtual std::string GetHMACForDeviceId( Seems like this should also be UnguesableToken in the future. https://codereview.chromium.org/2697663003/diff/240001/content/browser/render... File content/browser/renderer_host/media/renderer_audio_output_stream_factory_context_impl.h (right): https://codereview.chromium.org/2697663003/diff/240001/content/browser/render... content/browser/renderer_host/media/renderer_audio_output_stream_factory_context_impl.h:5: #ifndef CONTENT_BROWSER_RENDERER_HOST_MEDIA_RENDERER_AUDIO_OUTPUT_STREAM_FACTORY_CONTEXT_IMPL_H_ Hmm, naming is getting a bit java... we have a factory context impl :p https://codereview.chromium.org/2697663003/diff/240001/content/browser/render... content/browser/renderer_host/media/renderer_audio_output_stream_factory_context_impl.h:81: const int render_process_id_; Add some docs for member variables? https://codereview.chromium.org/2697663003/diff/240001/content/browser/render... content/browser/renderer_host/media/renderer_audio_output_stream_factory_context_impl.h:88: }; DISALLOW_COPY_AND ASSIGN. https://codereview.chromium.org/2697663003/diff/240001/content/browser/render... File content/browser/renderer_host/media/renderer_audio_output_stream_factory_context_impl_unittest.cc (right): https://codereview.chromium.org/2697663003/diff/240001/content/browser/render... content/browser/renderer_host/media/renderer_audio_output_stream_factory_context_impl_unittest.cc:73: Signal MakeSineWave() { See https://cs.chromium.org/chromium/src/media/audio/simple_sources.h -- or one of the clients which uses it. https://codereview.chromium.org/2697663003/diff/240001/content/common/media/r... File content/common/media/renderer_audio_output_stream_factory.mojom (right): https://codereview.chromium.org/2697663003/diff/240001/content/common/media/r... content/common/media/renderer_audio_output_stream_factory.mojom:12: interface RendererAudioOutputStreamFactory { Probably could use some more details here. A la purpose and expected usage.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #5 (id:260001) has been deleted
Thanks Ken and Dale. Dale: I don't love the naming, so if you have specific naming suggestions, please tell me. Otherwise, I'll keep the names :). https://codereview.chromium.org/2697663003/diff/240001/content/browser/render... File content/browser/renderer_host/media/render_frame_audio_output_stream_factory.cc (right): https://codereview.chromium.org/2697663003/diff/240001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory.cc:34: DCHECK(thread_checker_.CalledOnValidThread()); On 2017/03/24 17:47:12, DaleCurtis wrote: > Meaningless right? It's bound to the thread first used on. Done. https://codereview.chromium.org/2697663003/diff/240001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory.cc:40: } On 2017/03/24 17:47:12, DaleCurtis wrote: > DCHECK(stream_providers_.empty()) ? Otherwise you may have dangling callbacks > holding unretained refs to this. I changed this to explicitly clear |stream_providers_| (as intended, we don't really have any guarantee that it's empty). https://codereview.chromium.org/2697663003/diff/240001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory.cc:52: UMALogDeviceAuthorizationTime(auth_start_time); On 2017/03/24 17:47:12, DaleCurtis wrote: > We don't log this in the existing ARH, so it will skew metrics if you add it > here. Do we want that? In practice, this branch will hopefully never happen, but I see your point, so I removed it. https://codereview.chromium.org/2697663003/diff/240001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory.cc:64: render_frame_id_, static_cast<int>(session_id), device_id, origin, On 2017/03/24 17:47:12, DaleCurtis wrote: > Hmm, why do we bother sending int64_t across if we only want an int? Are you > planning to change the message later to just be an int? Since mojom files are cross-platform, we cannot use int (which is platform-specific) in them, hence this ugly mess. It's rather silly :). I'd like to get rid of session_id (the IPC interfaces returning session_id should return an Audio(Output|Input)StreamProvider directly instead). Alternatively, session_id could be an int32, UnguessableToken or some other fixed-width type. But that is outside of the scope of this CL. https://codereview.chromium.org/2697663003/diff/240001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory.cc:96: base::Unretained(this)))); On 2017/03/24 17:47:12, DaleCurtis wrote: > See note above; seems like this could lead to callbacks with bad refs. I clarified the lifetime of all stream providers above. https://codereview.chromium.org/2697663003/diff/240001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory.cc:108: auto it = std::find_if( On 2017/03/24 17:47:12, DaleCurtis wrote: > base::EraseIf, if you want. Done. Since I found flat_set, I also changed to using that so I can stop reinventing the wheel :). https://codereview.chromium.org/2697663003/diff/240001/content/browser/render... File content/browser/renderer_host/media/render_frame_audio_output_stream_factory.h (right): https://codereview.chromium.org/2697663003/diff/240001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory.h:38: int64_t session_id, On 2017/03/24 17:47:12, DaleCurtis wrote: > For session_id, should we start using base::UnguessableToken? Replied to a comment in cc file. https://codereview.chromium.org/2697663003/diff/240001/content/browser/render... File content/browser/renderer_host/media/render_frame_audio_output_stream_factory_unittest.cc (right): https://codereview.chromium.org/2697663003/diff/240001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory_unittest.cc:1: // Copyright 2017 The Chromium Authors. All rights reserved. On 2017/03/24 17:47:12, DaleCurtis wrote: > Don't see a test which utilizes the RemoveStream path? I added RenderFrameAudioOutputStreamFactoryTest.{Connection,Delegate}Error_DeletesStream. https://codereview.chromium.org/2697663003/diff/240001/content/browser/render... File content/browser/renderer_host/media/renderer_audio_output_stream_factory_context.h (right): https://codereview.chromium.org/2697663003/diff/240001/content/browser/render... content/browser/renderer_host/media/renderer_audio_output_stream_factory_context.h:36: virtual std::string GetHMACForDeviceId( On 2017/03/24 17:47:12, DaleCurtis wrote: > Seems like this should also be UnguesableToken in the future. It makes sense, but this would require us to store a map (origin, device_id) -> (token). It must also be stored between browser sessions (not sure how this works today). On the plus side, the device_id as visible to the renderer will be of a different type from the device_id we use when interacting with the audio manager, which would reduce confusion. https://codereview.chromium.org/2697663003/diff/240001/content/browser/render... File content/browser/renderer_host/media/renderer_audio_output_stream_factory_context_impl.h (right): https://codereview.chromium.org/2697663003/diff/240001/content/browser/render... content/browser/renderer_host/media/renderer_audio_output_stream_factory_context_impl.h:81: const int render_process_id_; On 2017/03/24 17:47:12, DaleCurtis wrote: > Add some docs for member variables? Added some. Was there anything specific you wanted? https://codereview.chromium.org/2697663003/diff/240001/content/browser/render... content/browser/renderer_host/media/renderer_audio_output_stream_factory_context_impl.h:88: }; On 2017/03/24 17:47:12, DaleCurtis wrote: > DISALLOW_COPY_AND ASSIGN. Done. https://codereview.chromium.org/2697663003/diff/240001/content/browser/render... File content/browser/renderer_host/media/renderer_audio_output_stream_factory_context_impl_unittest.cc (right): https://codereview.chromium.org/2697663003/diff/240001/content/browser/render... content/browser/renderer_host/media/renderer_audio_output_stream_factory_context_impl_unittest.cc:73: Signal MakeSineWave() { On 2017/03/24 17:47:12, DaleCurtis wrote: > See https://cs.chromium.org/chromium/src/media/audio/simple_sources.h -- or one > of the clients which uses it. Oh, nice. I should've looked for this :). https://codereview.chromium.org/2697663003/diff/240001/content/common/media/r... File content/common/media/renderer_audio_output_stream_factory.mojom (right): https://codereview.chromium.org/2697663003/diff/240001/content/common/media/r... content/common/media/renderer_audio_output_stream_factory.mojom:12: interface RendererAudioOutputStreamFactory { On 2017/03/24 17:47:13, DaleCurtis wrote: > Probably could use some more details here. A la purpose and expected usage. Better now?
On naming, I wonde rif we actually need render_frame and renderer_ as prefixes for these classes? https://codereview.chromium.org/2697663003/diff/280001/content/browser/render... File content/browser/renderer_host/media/render_frame_audio_output_stream_factory.cc (right): https://codereview.chromium.org/2697663003/diff/280001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory.cc:31: RendererAudioOutputStreamFactoryContext* context, Generally parameter order is input params, then i/o params: https://google.github.io/styleguide/cppguide.html#Function_Parameter_Ordering Up to you though. https://codereview.chromium.org/2697663003/diff/280001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory.cc:94: base::Unretained(context_), raw_device_id, render_frame_id_), Commentary on safety? Seems risky since this is a value passed in during construction. Header should document lifetime then. https://codereview.chromium.org/2697663003/diff/280001/content/browser/render... File content/browser/renderer_host/media/render_frame_audio_output_stream_factory.h (right): https://codereview.chromium.org/2697663003/diff/280001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory.h:57: // The stream providers will contain the corresponding streams. Empty line above comments. https://codereview.chromium.org/2697663003/diff/280001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory.h:59: const int render_frame_id_; I think it's easier to read if you put the const variables at the top of the variables section. https://codereview.chromium.org/2697663003/diff/280001/content/browser/render... File content/browser/renderer_host/media/render_frame_audio_output_stream_factory_unittest.cc (right): https://codereview.chromium.org/2697663003/diff/280001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory_unittest.cc:72: MockAudioOutputDelegate( explicit? https://codereview.chromium.org/2697663003/diff/280001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory_unittest.cc:90: }; DISALLOW_ https://codereview.chromium.org/2697663003/diff/280001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory_unittest.cc:94: explicit MockContext(bool auth_ok, int render_process_id = kRenderProcessId) No explicit. https://codereview.chromium.org/2697663003/diff/280001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory_unittest.cc:178: }; DISALLOW. https://codereview.chromium.org/2697663003/diff/280001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory_unittest.cc:180: class MockClient { Could probably just use a MOCK with an expectation that sets a value. https://codereview.chromium.org/2697663003/diff/280001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory_unittest.cc:194: }; Ditto. https://codereview.chromium.org/2697663003/diff/280001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory_unittest.cc:209: class RenderFrameAudioOutputStreamFactoryTest : public Test { Hmm, seems like there should already be a test class you can extend for this? https://codereview.chromium.org/2697663003/diff/280001/content/browser/render... File content/browser/renderer_host/media/renderer_audio_output_stream_factory_context_impl.h (right): https://codereview.chromium.org/2697663003/diff/280001/content/browser/render... content/browser/renderer_host/media/renderer_audio_output_stream_factory_context_impl.h:83: // Used for hashing the device_id. Lines above comments.
We are concerned that dropping renderer/render_frame will make the names ambiguous when we have stream factories between browser process and audio process (we don't want content::AudioOutputStreamFactory for renderer->browser and media::AudioOutputStreamFactory for browser->audio since they are too similar). Maybe there is a nicer way to disambiguate though. https://codereview.chromium.org/2697663003/diff/280001/content/browser/render... File content/browser/renderer_host/media/render_frame_audio_output_stream_factory.cc (right): https://codereview.chromium.org/2697663003/diff/280001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory.cc:31: RendererAudioOutputStreamFactoryContext* context, On 2017/03/27 19:07:49, DaleCurtis wrote: > Generally parameter order is input params, then i/o params: > > https://google.github.io/styleguide/cppguide.html#Function_Parameter_Ordering > > Up to you though. Done. https://codereview.chromium.org/2697663003/diff/280001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory.cc:94: base::Unretained(context_), raw_device_id, render_frame_id_), On 2017/03/27 19:07:49, DaleCurtis wrote: > Commentary on safety? Seems risky since this is a value passed in during > construction. Header should document lifetime then. Done. https://codereview.chromium.org/2697663003/diff/280001/content/browser/render... File content/browser/renderer_host/media/render_frame_audio_output_stream_factory.h (right): https://codereview.chromium.org/2697663003/diff/280001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory.h:57: // The stream providers will contain the corresponding streams. On 2017/03/27 19:07:49, DaleCurtis wrote: > Empty line above comments. Done. https://codereview.chromium.org/2697663003/diff/280001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory.h:59: const int render_frame_id_; On 2017/03/27 19:07:50, DaleCurtis wrote: > I think it's easier to read if you put the const variables at the top of the > variables section. Done. https://codereview.chromium.org/2697663003/diff/280001/content/browser/render... File content/browser/renderer_host/media/render_frame_audio_output_stream_factory_unittest.cc (right): https://codereview.chromium.org/2697663003/diff/280001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory_unittest.cc:72: MockAudioOutputDelegate( On 2017/03/27 19:07:50, DaleCurtis wrote: > explicit? Done. https://codereview.chromium.org/2697663003/diff/280001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory_unittest.cc:90: }; On 2017/03/27 19:07:50, DaleCurtis wrote: > DISALLOW_ Done. https://codereview.chromium.org/2697663003/diff/280001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory_unittest.cc:94: explicit MockContext(bool auth_ok, int render_process_id = kRenderProcessId) On 2017/03/27 19:07:50, DaleCurtis wrote: > No explicit. Done. https://codereview.chromium.org/2697663003/diff/280001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory_unittest.cc:178: }; On 2017/03/27 19:07:50, DaleCurtis wrote: > DISALLOW. Done. https://codereview.chromium.org/2697663003/diff/280001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory_unittest.cc:180: class MockClient { On 2017/03/27 19:07:50, DaleCurtis wrote: > Could probably just use a MOCK with an expectation that sets a value. Gmock has issues with move-only types like mojo::Scoped*. https://codereview.chromium.org/2697663003/diff/280001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory_unittest.cc:194: }; On 2017/03/27 19:07:50, DaleCurtis wrote: > Ditto. Done. https://codereview.chromium.org/2697663003/diff/280001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory_unittest.cc:209: class RenderFrameAudioOutputStreamFactoryTest : public Test { On 2017/03/27 19:07:50, DaleCurtis wrote: > Hmm, seems like there should already be a test class you can extend for this? I cannot find one, but as the fixture is unnecessary, I removed it. https://codereview.chromium.org/2697663003/diff/280001/content/browser/render... File content/browser/renderer_host/media/renderer_audio_output_stream_factory_context_impl.h (right): https://codereview.chromium.org/2697663003/diff/280001/content/browser/render... content/browser/renderer_host/media/renderer_audio_output_stream_factory_context_impl.h:83: // Used for hashing the device_id. On 2017/03/27 19:07:50, DaleCurtis wrote: > Lines above comments. Done.
I thought mojo had some patterns for naming browser vs renderer components? I'm not sure what the current best practices are, perhaps rockot@ can comment, but we've used service or host in the past for browser side components while leaving the renderer side ones unspecialized.
On Tue, Mar 28, 2017 at 10:07 AM, <dalecurtis@chromium.org> wrote: > I thought mojo had some patterns for naming browser vs renderer > components? I'm > not sure what the current best practices are, perhaps rockot@ can > comment, but > we've used service or host in the past for browser side components while > leaving > the renderer side ones unspecialized. > We have explicitly avoided such conventions for Mojo interfaces. They cannot sufficiently model the relationships between endpoints in the broader service model, i.e. there isn't a clear "host" vs "client" like there is with one-big-renderer vs many-untrusted-renderers -- rather, an InterfacePtr<T> is the client regardless of where it lives or what interface T is, and the Binding<T> is similarly the host. T is the protocol spoken from client to host. Another way to think of it is that all interfaces are implicitly "host" interfaces, but that doesn't tell you anything interesting about the relative trust or privilege on either end of the pipe. > https://codereview.chromium.org/2697663003/ > -- 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.
Thanks for clarifying rockot@. Current names seem reasonable then unfortunately. We can always rename later, so lgtm. Thanks Max!
maxmorin@chromium.org changed reviewers: + asvitkine@chromium.org, avi@chromium.org, mkwst@chromium.org
Cool, thank you. Avi: PTAL at content/ excluding media subdirectories. Mike: PTAL at mojo interface at content/common/media/renderer_audio_output_stream_factory.mojom. asvitkine: PTAL at histograms.xml. Olga: Do you have any outstanding concerns?
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...
Thanks Max! LGTM https://codereview.chromium.org/2697663003/diff/300001/content/browser/render... File content/browser/renderer_host/media/renderer_audio_output_stream_factory_context_impl.cc (right): https://codereview.chromium.org/2697663003/diff/300001/content/browser/render... content/browser/renderer_host/media/renderer_audio_output_stream_factory_context_impl.cc:94: handler, audio_system_->GetAudioManager(), std::move(audio_log), Could we switch to passing AudioManager interface here? We'll be getting rid of GetAudioManager() https://codereview.chromium.org/2697663003/diff/300001/content/browser/render... File content/browser/renderer_host/media/renderer_audio_output_stream_factory_context_impl.h (right): https://codereview.chromium.org/2697663003/diff/300001/content/browser/render... content/browser/renderer_host/media/renderer_audio_output_stream_factory_context_impl.h:40: // media::MojoAudioOutputStream Great, thanks! (an arrow is a bit confusing though since it looks more like inheritance)
https://codereview.chromium.org/2697663003/diff/300001/content/common/media/r... File content/common/media/renderer_audio_output_stream_factory.mojom (right): https://codereview.chromium.org/2697663003/diff/300001/content/common/media/r... content/common/media/renderer_audio_output_stream_factory.mojom:23: url.mojom.Origin origin) => Why are you passing in an origin here? Shouldn't the browser know the origin of the frame that's making the request?
On 2017/03/29 08:22:07, Mike West wrote: > https://codereview.chromium.org/2697663003/diff/300001/content/common/media/r... > File content/common/media/renderer_audio_output_stream_factory.mojom (right): > > https://codereview.chromium.org/2697663003/diff/300001/content/common/media/r... > content/common/media/renderer_audio_output_stream_factory.mojom:23: > url.mojom.Origin origin) => > Why are you passing in an origin here? Shouldn't the browser know the origin of > the frame that's making the request? Hmm, right. Makes me wonder why AudioRendererHost takes both frame id and origin as arguments. Guess I'll need to make a hop to the UI thread to get the origin. I'll do that. Does the ugly way to pass session_id over mojo look ok to you? I'd like to get rid of the session_id, but that depends on mojofying MediaStreamDispatcherHost and also on the changes corresponding to these for input streams, so I can't do that yet.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
On 2017/03/29 at 08:35:22, maxmorin wrote: > On 2017/03/29 08:22:07, Mike West wrote: > > https://codereview.chromium.org/2697663003/diff/300001/content/common/media/r... > > File content/common/media/renderer_audio_output_stream_factory.mojom (right): > > > > https://codereview.chromium.org/2697663003/diff/300001/content/common/media/r... > > content/common/media/renderer_audio_output_stream_factory.mojom:23: > > url.mojom.Origin origin) => > > Why are you passing in an origin here? Shouldn't the browser know the origin of > > the frame that's making the request? > > Hmm, right. Makes me wonder why AudioRendererHost takes both frame id and origin as arguments. Guess I'll need to make a hop to the UI thread to get the origin. I'll do that. Does the ugly way to pass session_id over mojo look ok to you? I'd like to get rid of the session_id, but that depends on mojofying MediaStreamDispatcherHost and also on the changes corresponding to these for input streams, so I can't do that yet. I think that bit looks fine. If you take care of the origin verification, I'm happy. :)
lgtm content stamp
histograms lgtm
Dale: I introduced weak pointers to manage callbacks now. Does CL still look good to you? Mike: PTAL. https://codereview.chromium.org/2697663003/diff/300001/content/browser/render... File content/browser/renderer_host/media/renderer_audio_output_stream_factory_context_impl.cc (right): https://codereview.chromium.org/2697663003/diff/300001/content/browser/render... content/browser/renderer_host/media/renderer_audio_output_stream_factory_context_impl.cc:94: handler, audio_system_->GetAudioManager(), std::move(audio_log), On 2017/03/29 08:13:48, o1ka wrote: > Could we switch to passing AudioManager interface here? We'll be getting rid of > GetAudioManager() Done. https://codereview.chromium.org/2697663003/diff/300001/content/browser/render... File content/browser/renderer_host/media/renderer_audio_output_stream_factory_context_impl.h (right): https://codereview.chromium.org/2697663003/diff/300001/content/browser/render... content/browser/renderer_host/media/renderer_audio_output_stream_factory_context_impl.h:40: // media::MojoAudioOutputStream On 2017/03/29 08:13:48, o1ka wrote: > Great, thanks! (an arrow is a bit confusing though since it looks more like > inheritance) Hopefully, the text clears it up :). https://codereview.chromium.org/2697663003/diff/300001/content/common/media/r... File content/common/media/renderer_audio_output_stream_factory.mojom (right): https://codereview.chromium.org/2697663003/diff/300001/content/common/media/r... content/common/media/renderer_audio_output_stream_factory.mojom:23: url.mojom.Origin origin) => On 2017/03/29 08:22:07, Mike West wrote: > Why are you passing in an origin here? Shouldn't the browser know the origin of > the frame that's making the request? I changed to getting it from RFH::GetLastCommittedOrigin.
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...
mojo lgtm, thanks!
https://codereview.chromium.org/2697663003/diff/340001/content/browser/render... File content/browser/renderer_host/media/render_frame_audio_output_stream_factory.cc (right): https://codereview.chromium.org/2697663003/diff/340001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory.cc:63: bad_message::ReceivedBadMessage(context_->GetRenderProcessId(), It would be nice to avoid introducing new bad_message enum values. If you simply call mojo::ReportBadMessage("arbitrary string"), you'll get a renderer crash report which includes the string argument in a crash key. Does that seem sufficient?
On 2017/03/30 at 14:54:00, Ken Rockot wrote: > https://codereview.chromium.org/2697663003/diff/340001/content/browser/render... > File content/browser/renderer_host/media/render_frame_audio_output_stream_factory.cc (right): > > https://codereview.chromium.org/2697663003/diff/340001/content/browser/render... > content/browser/renderer_host/media/render_frame_audio_output_stream_factory.cc:63: bad_message::ReceivedBadMessage(context_->GetRenderProcessId(), > It would be nice to avoid introducing new bad_message enum values. > > If you simply call mojo::ReportBadMessage("arbitrary string"), you'll get a renderer crash report which includes the string argument in a crash key. > Correction: We'll kill the renderer, and you'll get a browser crash report (without crashing of course) which include the string argument in a crash key. > Does that seem sufficient?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
WeakPtr usage lgtm assuming the threading is as labeled. https://codereview.chromium.org/2697663003/diff/340001/content/browser/render... File content/browser/renderer_host/media/render_frame_audio_output_stream_factory.cc (right): https://codereview.chromium.org/2697663003/diff/340001/content/browser/render... content/browser/renderer_host/media/render_frame_audio_output_stream_factory.cc:90: context_->RequestDeviceAuthorization( Thread checker?
On 2017/03/30 14:54:54, Ken Rockot wrote: > On 2017/03/30 at 14:54:00, Ken Rockot wrote: > > > https://codereview.chromium.org/2697663003/diff/340001/content/browser/render... > > File > content/browser/renderer_host/media/render_frame_audio_output_stream_factory.cc > (right): > > > > > https://codereview.chromium.org/2697663003/diff/340001/content/browser/render... > > > content/browser/renderer_host/media/render_frame_audio_output_stream_factory.cc:63: > bad_message::ReceivedBadMessage(context_->GetRenderProcessId(), > > It would be nice to avoid introducing new bad_message enum values. > > > > If you simply call mojo::ReportBadMessage("arbitrary string"), you'll get a > renderer crash report which includes the string argument in a crash key. > > > > Correction: We'll kill the renderer, and you'll get a browser crash report > (without crashing of course) which include the string argument in a crash key. > > > Does that seem sufficient? Oh, I didn't know of ReportBadMessage. I'll switch to that, add the thread check, and commit this. Thanks everyone for the review!
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, avi@chromium.org, asvitkine@chromium.org, olka@chromium.org, dalecurtis@chromium.org, mkwst@chromium.org Link to the patchset: https://codereview.chromium.org/2697663003/#ps360001 (title: "Use mojo::ReportBadMessage and add a thread check.")
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
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by maxmorin@chromium.org
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": 360001, "attempt_start_ts": 1490962631136250,
"parent_rev": "755d274e7b3e893719322489b07a5d929328caa0", "commit_rev":
"8757b429c30f14d20697f7c6fe4dfdd510cf0b7f"}
Message was sent while issue was closed.
Description was changed from ========== Add mojo interface+impl creation of audio streams. Summary: * RendererAudioOutputStreamFactory is an interface for creating AudioOutputProviders. * Since a lot of data is common for all factories of a single renderer process, it is held in a context object. (AudioOutputStreamFactoryContext is per-process and RendererAudioOutputStreamFactory is per-frame). * Replaces AudioRendererHost (eventually, after field trials). * Follow-up on https://codereview.chromium.org/2697793002/. * Design doc at https://docs.google.com/document/d/1awQoajq_DLmz2AIU9iweC0zEYlVuHCvEIRQepeYyx... * audio_output_stream_factory_context_impl_unittest.cc is actually an integration test, but it seems common to suffix test files with "_unittest" anyways, so I did that. BUG=425368 ========== to ========== Add mojo interface+impl creation of audio streams. Summary: * RendererAudioOutputStreamFactory is an interface for creating AudioOutputProviders. * Since a lot of data is common for all factories of a single renderer process, it is held in a context object. (AudioOutputStreamFactoryContext is per-process and RendererAudioOutputStreamFactory is per-frame). * Replaces AudioRendererHost (eventually, after field trials). * Follow-up on https://codereview.chromium.org/2697793002/. * Design doc at https://docs.google.com/document/d/1awQoajq_DLmz2AIU9iweC0zEYlVuHCvEIRQepeYyx... * audio_output_stream_factory_context_impl_unittest.cc is actually an integration test, but it seems common to suffix test files with "_unittest" anyways, so I did that. BUG=425368 Review-Url: https://codereview.chromium.org/2697663003 Cr-Commit-Position: refs/heads/master@{#461103} Committed: https://chromium.googlesource.com/chromium/src/+/8757b429c30f14d20697f7c6fe4d... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:360001) as https://chromium.googlesource.com/chromium/src/+/8757b429c30f14d20697f7c6fe4d...
Message was sent while issue was closed.
A revert of this CL (patchset #9 id:360001) has been created in https://codereview.chromium.org/2788173002/ by caseq@chromium.org. The reason for reverting is: Broke sizes test on linux x64: https://build.chromium.org/p/chromium/builders/Linux%20x64/builds/36560. |
