Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2016053003:
Add audio_device_id in mojo media ServiceFactory CreateRenderer api. (Closed)
|
Created:
4 years, 6 months ago by tianyuwang1 Modified:
4 years, 5 months ago CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, chromium-reviews, darin (slow to review), feature-media-reviews_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionaudio_device_id is needed for audio volume ducking. audio_device_id is defined in
media/audio/audio_device_description.h. The renderer needs a way to specify which audio device the sound should be play out.
Bug=b/28193093
Committed: https://crrev.com/f3c4a58fcc70cd3384020708ce3ff50d847654ba
Cr-Commit-Position: refs/heads/master@{#405213}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Add device_id to CreateRenderer api. #Patch Set 3 : Change all places that uses CreateRenderer api to pass in a device_id. #
Total comments: 5
Patch Set 4 : Change Create RendererFactory api to CreateRender api and removed CreateAudio[Video]Sink apis. #Patch Set 5 : Rebased #Patch Set 6 : Fix compile error #Patch Set 7 : Fix clang format #Patch Set 8 : Fix compile error #
Total comments: 10
Patch Set 9 : Fix comments. #
Total comments: 6
Patch Set 10 : Fixes according to comments. #
Total comments: 2
Patch Set 11 : Fix compile error #Patch Set 12 : Fix clang format #
Total comments: 7
Patch Set 13 : Minor changes according to the code review. #
Total comments: 6
Patch Set 14 : Small fixes according to code review comments. #Patch Set 15 : Add implementation to use audio_device_id in CastRender. #
Total comments: 6
Patch Set 16 : Fix comments #Patch Set 17 : Fix an error #Patch Set 18 : Fix test. #Patch Set 19 : Fix an error #
Total comments: 2
Patch Set 20 : Fix an error #
Total comments: 2
Patch Set 21 : fix comment #
Total comments: 2
Patch Set 22 : Minor change based on the comment #Messages
Total messages: 159 (66 generated)
tianyuwang@google.com changed reviewers: + alokp@chromium.org, kmackay@chromium.org, xhwang@chromium.org
alokp@chromium.org changed reviewers: + dalecurtis@chromium.org, tommi@chromium.org
https://codereview.chromium.org/2016053003/diff/1/media/mojo/interfaces/rende... File media/mojo/interfaces/renderer.mojom (right): https://codereview.chromium.org/2016053003/diff/1/media/mojo/interfaces/rende... media/mojo/interfaces/renderer.mojom:19: string device_id) => (bool success); It is possible to target a particular device when using AudioManager::MakeAudioOutputStream*. In that sense I think it is reasonable to specify the audio device. The only weird thing is that unlike AudioManager, this interface does not allow querying for available device IDs. Perhaps it may not be that weird once mojo:audio service is available? I understand WebRTC intern is working on bringing it up. Tommi?
https://chromiumcodereview.appspot.com/2016053003/diff/1/media/mojo/interface... File media/mojo/interfaces/renderer.mojom (right): https://chromiumcodereview.appspot.com/2016053003/diff/1/media/mojo/interface... media/mojo/interfaces/renderer.mojom:19: string device_id) => (bool success); On 2016/05/27 04:43:08, alokp wrote: > It is possible to target a particular device when using > AudioManager::MakeAudioOutputStream*. In that sense I think it is reasonable to > specify the audio device. The only weird thing is that unlike AudioManager, this > interface does not allow querying for available device IDs. > > Perhaps it may not be that weird once mojo:audio service is available? I > understand WebRTC intern is working on bringing it up. Tommi? +dalecurtis I am not super familiar with audio, but it seems a bit odd for this interface. Let me ask this question, if everything lives in the same process and we don't need mojo. How will we choose which audio stream will be played on which device_id. This will help me better understand how things should work. My gut feeling is that we should have some separate channel to pass this information, but I could be wrong.
https://chromiumcodereview.appspot.com/2016053003/diff/1/media/mojo/interface... File media/mojo/interfaces/renderer.mojom (right): https://chromiumcodereview.appspot.com/2016053003/diff/1/media/mojo/interface... media/mojo/interfaces/renderer.mojom:19: string device_id) => (bool success); On 2016/05/27 06:06:35, xhwang wrote: > On 2016/05/27 04:43:08, alokp wrote: > > It is possible to target a particular device when using > > AudioManager::MakeAudioOutputStream*. In that sense I think it is reasonable > to > > specify the audio device. The only weird thing is that unlike AudioManager, > this > > interface does not allow querying for available device IDs. > > > > Perhaps it may not be that weird once mojo:audio service is available? I > > understand WebRTC intern is working on bringing it up. Tommi? > > +dalecurtis > > I am not super familiar with audio, but it seems a bit odd for this interface. > > Let me ask this question, if everything lives in the same process and we don't > need mojo. How will we choose which audio stream will be played on which > device_id. This will help me better understand how things should work. My gut > feeling is that we should have some separate channel to pass this information, > but I could be wrong. In that case you would provide an AudioRendererSink for the AudioRenderer to render to. The default subclass of AudioRendererSink is AudioOutputDevice, which lets you specify audio device id. Since the mojo interface does not deal with AudioRenderer or AudioRendererSink, it should let the clients specify the audio-device id somehow. This is not an issue for the renderer process because it always renders to the default device. But it is important for chirp - they want to render TTS sounds to communications device so that other sounds are attenuated.
Why do you need this? Your target process has access to AudioManager directly for stream creation. That's what AudioOutputStreamSink is for.
It would make more sense if we wanted to go down this path to pass over an AudioRendererSink still controllable from the renderer so that things like SwitchOutputDevice, etc still work.
On 2016/05/27 19:18:26, DaleCurtis wrote: > Why do you need this? Your target process has access to AudioManager directly > for stream creation. That's what AudioOutputStreamSink is for. We have a separate Utility Process that needs to play audio through Mojo Media service on a certain device_id. The utility process doesn't have access to AudioManager directly.
How are you intending to pass this from WMPI? WMPI doesn't know about this information anyways. Currently the only object that knows about this is the AudioRendererSink.
On 2016/05/27 20:25:40, DaleCurtis wrote: > How are you intending to pass this from WMPI? WMPI doesn't know about this > information anyways. Currently the only object that knows about this is the > AudioRendererSink. AudioRendererSink is passed into RendererImpl at construction time. This is also true for MojoRendererService: https://code.google.com/p/chromium/codesearch#chromium/src/media/mojo/service... So if we keep this model, probably we could pass some information about audio device at creation time: https://code.google.com/p/chromium/codesearch#chromium/src/media/mojo/interfa... Or, if we follow what this CL does, we should pass AudioRendererSink and VideoRendererSink in media::Renderer::Initialize(). Which is a much bigger change. Historically, we didn't put this in the media Renderer interface because in theory not all Renderers need them. So it seems they don't belong to the Renderer interface.
On 2016/05/27 20:38:59, xhwang wrote: > On 2016/05/27 20:25:40, DaleCurtis wrote: > > How are you intending to pass this from WMPI? WMPI doesn't know about this > > information anyways. Currently the only object that knows about this is the > > AudioRendererSink. > > AudioRendererSink is passed into RendererImpl at construction time. This is also > true for MojoRendererService: > > https://code.google.com/p/chromium/codesearch#chromium/src/media/mojo/service... > > So if we keep this model, probably we could pass some information about audio > device at creation time: > > https://code.google.com/p/chromium/codesearch#chromium/src/media/mojo/interfa... > > Or, if we follow what this CL does, we should pass AudioRendererSink and > VideoRendererSink in media::Renderer::Initialize(). Which is a much bigger > change. Historically, we didn't put this in the media Renderer interface because > in theory not all Renderers need them. So it seems they don't belong to the > Renderer interface. I like the idea of putting it in CreateRenderer call, so that it fits with the none mojo model too. https://code.google.com/p/chromium/codesearch#chromium/src/media/mojo/interfa... Do you think we should pass in a device_id directly into CreateRenderer? Or do we want to pack it in something data sturct? The none mojo flow has some other informations when creating a AudioRendererSink. Will we need any of the other params in the future? https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...
On 2016/05/27 19:18:26, DaleCurtis wrote: > Why do you need this? Your target process has access to AudioManager directly > for stream creation. That's what AudioOutputStreamSink is for. Right - assuming by "target" process you mean the process where mojo:media is hosted. The client still needs to be able to specify the device id though.
On 2016/05/27 19:19:32, DaleCurtis wrote: > It would make more sense if we wanted to go down this path to pass over an > AudioRendererSink still controllable from the renderer so that things like > SwitchOutputDevice, etc still work. Do you mean pass over an AudioRendererSink from client to the remote renderer? If you already have an AudioRendererSink, I do not see why anybody would want to use a remote renderer. They would probably use a software decoder or mojo decoder interfaces instead. In my mind, a remote renderer is only useful if you do not want to see the media data again.
On 2016/05/27 20:38:59, xhwang wrote: > On 2016/05/27 20:25:40, DaleCurtis wrote: > > How are you intending to pass this from WMPI? WMPI doesn't know about this > > information anyways. Currently the only object that knows about this is the > > AudioRendererSink. > > AudioRendererSink is passed into RendererImpl at construction time. This is also > true for MojoRendererService: > > https://code.google.com/p/chromium/codesearch#chromium/src/media/mojo/service... > > So if we keep this model, probably we could pass some information about audio > device at creation time: > > https://code.google.com/p/chromium/codesearch#chromium/src/media/mojo/interfa... This would work! > Or, if we follow what this CL does, we should pass AudioRendererSink and > VideoRendererSink in media::Renderer::Initialize(). Which is a much bigger > change. Historically, we didn't put this in the media Renderer interface because > in theory not all Renderers need them. So it seems they don't belong to the > Renderer interface. Yeah I would not recommend putting sink in Renderer::Initialize.
I've change the device_id from Renderer.Initialize() to CreateRenderer api. Can you take a look and see if we should wrap the audio device_id in some data struct? Also I don't think we are passing the audio_renderer_sink to the MojoRendererImpl right now. So we need to hook that up in the future too. https://code.google.com/p/chromium/codesearch#chromium/src/media/mojo/service...
On 2016/05/28 00:21:22, tianyuwang1 wrote: > I've change the device_id from Renderer.Initialize() to CreateRenderer api. Can > you take a look and see if we should wrap the audio device_id in some data > struct? > > Also I don't think we are passing the audio_renderer_sink to the > MojoRendererImpl right now. So we need to hook that up in the future too. > > https://code.google.com/p/chromium/codesearch#chromium/src/media/mojo/service... Putting this in CreateRenderer() looks much better. Maybe this new parameter should be optional such that when it's null, we'll use the default device. (+dalecurtis) BTW, I am not a fan of the name "device_id", as this is more like a name or a type than an ID. Also, why we are using string names instead of enums? I guess these questions are out of the scope of this CL though.
lgtm, device_id is typically a hashed identifier of a system device, because on some systems it's just a plain path like /dev/blah/blah which isn't something we should be leaking to the renderer; since we also have device_name which means something else, device_id it is :)
On 2016/05/31 17:36:55, DaleCurtis wrote: > lgtm, device_id is typically a hashed identifier of a system device, because on > some systems it's just a plain path like /dev/blah/blah which isn't something we > should be leaking to the renderer; since we also have device_name which means > something else, device_id it is :) Thanks for the explanation. On this interface, what do we expect the caller to use for |audio_device_id|? Will it always be one of kDefaultDeviceId, kCommunicationsDeviceId and kLoopbackInputDeviceId? I guess the caller (in utility or renderer process) will have no idea about the real hashed identifier of a system device. If so, should we actually define some enum IDs to be used by sandboxed processes (e.g. renderer/utility), which will be translated by the browser into real system device IDs?
Renderer has ways of getting the right hashed identifier, so it will be either one of those default values or a real value retrieved via other means.
On 2016/05/31 17:59:25, DaleCurtis wrote: > Renderer has ways of getting the right hashed identifier, so it will be either > one of those default values or a real value retrieved via other means. okay, got it. Tianyu: The current approach looks good to me. Could you please update your CL to make it complete for a full review? Thanks!
On 2016/05/31 18:16:42, xhwang wrote: > On 2016/05/31 17:59:25, DaleCurtis wrote: > > Renderer has ways of getting the right hashed identifier, so it will be either > > one of those default values or a real value retrieved via other means. > > okay, got it. > > Tianyu: The current approach looks good to me. Could you please update your CL > to make it complete for a full review? Thanks! Yep. Doing it right now.
Description was changed from ========== Propose to add device_id in mojo Renderer service initialize api. device_id is needed for audio volume ducking. device_id is defined in media/audio/audio_device_description.h. For example, when a voice call is started, sound will be played using kCommunicationsDeviceId. All other audio streams being played with kDefaultDeviceId will be ducked to a lower volume. BUG=internal 28193093 ========== to ========== device_id is needed for audio volume ducking. device_id is defined in media/audio/audio_device_description.h. For example, when a voice call is started, sound will be played using kCommunicationsDeviceId. All other audio streams being played with kDefaultDeviceId will be ducked to a lower volume. Bug: b/28193093 ==========
The CQ bit was checked by tianyuwang@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2016053003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2016053003/40001
tianyuwang@google.com changed reviewers: + dcheng@chromium.org
https://codereview.chromium.org/2016053003/diff/40001/media/mojo/interfaces/s... File media/mojo/interfaces/service_factory.mojom (right): https://codereview.chromium.org/2016053003/diff/40001/media/mojo/interfaces/s... media/mojo/interfaces/service_factory.mojom:26: string? audio_device_id); Since kDefaultDeviceId is already defined, may be it is better to make the device-id as required parameter? https://codereview.chromium.org/2016053003/diff/40001/media/mojo/services/moj... File media/mojo/services/mojo_media_client.h (right): https://codereview.chromium.org/2016053003/diff/40001/media/mojo/services/moj... media/mojo/services/mojo_media_client.h:55: virtual std::unique_ptr<RendererFactory> CreateRendererFactory( I think we would need to change this API to accept device id. We do not use Audio[Video]RendererSink for chromecast. I would also get rid of MojoMediaClient::CreateAudio[Video]RendererSink and make the usage of RendererFactory internal to MojoMediaClient implementation.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2016053003/diff/40001/media/mojo/services/moj... File media/mojo/services/mojo_media_client.h (right): https://codereview.chromium.org/2016053003/diff/40001/media/mojo/services/moj... media/mojo/services/mojo_media_client.h:55: virtual std::unique_ptr<RendererFactory> CreateRendererFactory( On 2016/06/01 00:06:14, alokp wrote: > I think we would need to change this API to accept device id. We do not use > Audio[Video]RendererSink for chromecast. > > I would also get rid of MojoMediaClient::CreateAudio[Video]RendererSink and make > the usage of RendererFactory internal to MojoMediaClient implementation. I agree it seems the time to address the comment/TODO at l.51 now.
The CQ bit was checked by tianyuwang@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2016053003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2016053003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by tianyuwang@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2016053003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2016053003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by tianyuwang@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2016053003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2016053003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) cast_shell_android on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by tianyuwang@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2016053003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2016053003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by tianyuwang@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2016053003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2016053003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
PTAL https://codereview.chromium.org/2016053003/diff/40001/media/mojo/interfaces/s... File media/mojo/interfaces/service_factory.mojom (right): https://codereview.chromium.org/2016053003/diff/40001/media/mojo/interfaces/s... media/mojo/interfaces/service_factory.mojom:26: string? audio_device_id); On 2016/06/01 00:06:14, alokp wrote: > Since kDefaultDeviceId is already defined, may be it is better to make the > device-id as required parameter? xhwang@ suggested that we should make this audio_device_id optional instead of required. so that when it's not set we can use the default device. This is what other places that use device_id is doing too. If device_id is empty then use default device. https://codereview.chromium.org/2016053003/diff/40001/media/mojo/services/moj... File media/mojo/services/mojo_media_client.h (right): https://codereview.chromium.org/2016053003/diff/40001/media/mojo/services/moj... media/mojo/services/mojo_media_client.h:55: virtual std::unique_ptr<RendererFactory> CreateRendererFactory( On 2016/06/01 05:26:27, xhwang wrote: > On 2016/06/01 00:06:14, alokp wrote: > > I think we would need to change this API to accept device id. We do not use > > Audio[Video]RendererSink for chromecast. > > > > I would also get rid of MojoMediaClient::CreateAudio[Video]RendererSink and > make > > the usage of RendererFactory internal to MojoMediaClient implementation. > > I agree it seems the time to address the comment/TODO at l.51 now. Done. Added CreateRender api. Removed CreateRendererFactory and CreateAudio[Video]RendererSink apis.
lgtm % nits https://codereview.chromium.org/2016053003/diff/140001/content/renderer/media... File content/renderer/media/media_interface_provider.cc (right): https://codereview.chromium.org/2016053003/diff/140001/content/renderer/media... content/renderer/media/media_interface_provider.cc:36: mojo::MakeRequest<media::mojom::Renderer>(std::move(pipe)), ""); since string is optional, may be we should pass a null string, i.e., mojo::String(nullptr). https://codereview.chromium.org/2016053003/diff/140001/media/mojo/services/me... File media/mojo/services/media_mojo_unittest.cc (right): https://codereview.chromium.org/2016053003/diff/140001/media/mojo/services/me... media/mojo/services/media_mojo_unittest.cc:103: service_factory_->CreateRenderer(mojo::GetProxy(&renderer_), ""); ditto https://codereview.chromium.org/2016053003/diff/140001/media/mojo/services/mo... File media/mojo/services/mojo_media_client.h (right): https://codereview.chromium.org/2016053003/diff/140001/media/mojo/services/mo... media/mojo/services/mojo_media_client.h:55: const scoped_refptr<base::SingleThreadTaskRunner>& media_task_runner, since we intend to share the ownership, scoped_refptr should be passed by copy for both media_task_runner and media_log. https://www.chromium.org/developers/coding-style#TOC-Object-ownership-and-cal... https://codereview.chromium.org/2016053003/diff/140001/media/mojo/services/se... File media/mojo/services/service_factory_impl.cc (right): https://codereview.chromium.org/2016053003/diff/140001/media/mojo/services/se... media/mojo/services/service_factory_impl.cc:90: std::string device_id = audio_device_id.get(); nit: device_id = audio_device_id.is_null() ? AudioDeviceDescription::kDefaultDeviceId : audio_device_id.get();
The CQ bit was checked by tianyuwang@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2016053003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2016053003/160001
https://codereview.chromium.org/2016053003/diff/140001/content/renderer/media... File content/renderer/media/media_interface_provider.cc (right): https://codereview.chromium.org/2016053003/diff/140001/content/renderer/media... content/renderer/media/media_interface_provider.cc:36: mojo::MakeRequest<media::mojom::Renderer>(std::move(pipe)), ""); On 2016/06/02 04:47:55, alokp wrote: > since string is optional, may be we should pass a null string, i.e., > mojo::String(nullptr). Done. https://codereview.chromium.org/2016053003/diff/140001/media/mojo/services/me... File media/mojo/services/media_mojo_unittest.cc (right): https://codereview.chromium.org/2016053003/diff/140001/media/mojo/services/me... media/mojo/services/media_mojo_unittest.cc:103: service_factory_->CreateRenderer(mojo::GetProxy(&renderer_), ""); On 2016/06/02 04:47:55, alokp wrote: > ditto Done. https://codereview.chromium.org/2016053003/diff/140001/media/mojo/services/mo... File media/mojo/services/mojo_media_client.h (right): https://codereview.chromium.org/2016053003/diff/140001/media/mojo/services/mo... media/mojo/services/mojo_media_client.h:55: const scoped_refptr<base::SingleThreadTaskRunner>& media_task_runner, On 2016/06/02 04:47:55, alokp wrote: > since we intend to share the ownership, scoped_refptr should be passed by copy > for both media_task_runner and media_log. > > https://www.chromium.org/developers/coding-style#TOC-Object-ownership-and-cal... According to the style guide above: "If the function (at least sometimes) takes a ref on a refcounted object, declare the param as scoped_refptr<T>. The caller can decide whether it wishes to transfer ownership (by calling std::move(t) when passing t) or retain its ref (by simply passing t directly)." So we don't need to pass by copy for media_task_runner or media_log? We can simply pass them directly? https://codereview.chromium.org/2016053003/diff/140001/media/mojo/services/se... File media/mojo/services/service_factory_impl.cc (right): https://codereview.chromium.org/2016053003/diff/140001/media/mojo/services/se... media/mojo/services/service_factory_impl.cc:90: std::string device_id = audio_device_id.get(); On 2016/06/02 04:47:55, alokp wrote: > nit: device_id = audio_device_id.is_null() ? > AudioDeviceDescription::kDefaultDeviceId : audio_device_id.get(); Done.
Also update patch title and description before landing. https://codereview.chromium.org/2016053003/diff/140001/media/mojo/services/mo... File media/mojo/services/mojo_media_client.h (right): https://codereview.chromium.org/2016053003/diff/140001/media/mojo/services/mo... media/mojo/services/mojo_media_client.h:55: const scoped_refptr<base::SingleThreadTaskRunner>& media_task_runner, On 2016/06/02 05:18:39, tianyuwang1 wrote: > On 2016/06/02 04:47:55, alokp wrote: > > since we intend to share the ownership, scoped_refptr should be passed by copy > > for both media_task_runner and media_log. > > > > > https://www.chromium.org/developers/coding-style#TOC-Object-ownership-and-cal... > > According to the style guide above: > "If the function (at least sometimes) takes a ref on a refcounted object, > declare the param as scoped_refptr<T>. The caller can decide whether it wishes > to transfer ownership (by calling std::move(t) when passing t) or retain its ref > (by simply passing t directly)." > > So we don't need to pass by copy for media_task_runner or media_log? We can > simply pass them directly? The function parameter should be scoped_refptr<base::SingleThreadTaskRunner> instead of "const scoped_refptr<base::SingleThreadTaskRunner>&". Individual callers will decide whether to use std::move or pass by copy.
Thanks for doing this! looking good with some bike shedding and % alokp's comments. https://chromiumcodereview.appspot.com/2016053003/diff/160001/media/mojo/inte... File media/mojo/interfaces/service_factory.mojom (right): https://chromiumcodereview.appspot.com/2016053003/diff/160001/media/mojo/inte... media/mojo/interfaces/service_factory.mojom:24: // is null, kDefaultDeviceId will be used. I wasn't aware that in C++ code when the device ID is empty then we'll choose the default device. In this case, maybe we should do the same on the mojo interface. Basically drop the "?" part to make this parameter mandatory, and update this comment s/null/empty. Then we can also get rid of the use of mojo::String(nullptr) which seems a bit odd. WDYT? https://chromiumcodereview.appspot.com/2016053003/diff/160001/media/mojo/inte... media/mojo/interfaces/service_factory.mojom:26: string? audio_device_id); +alop: Just FYI, this will make it harder for us to get rid of this mojom interface now. One solution is to add a Create(audio_device_id) -> (bool success) on the Renderer interface, so that the caller will do: ConnectToInterface(GetProxy(&renderer)); renderer->Create(create_cb); We have similar issues on the VideoDecoder mojo interface: https://code.google.com/p/chromium/codesearch#chromium/src/media/mojo/interfa... https://chromiumcodereview.appspot.com/2016053003/diff/160001/media/mojo/serv... File media/mojo/services/test_mojo_media_client.h (right): https://chromiumcodereview.appspot.com/2016053003/diff/160001/media/mojo/serv... media/mojo/services/test_mojo_media_client.h:47: VideoRendererSink* CreateVideoRendererSink( nit: s/Create/Get/ for these 3 functions which makes more sense.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://chromiumcodereview.appspot.com/2016053003/diff/160001/media/mojo/inte... File media/mojo/interfaces/service_factory.mojom (right): https://chromiumcodereview.appspot.com/2016053003/diff/160001/media/mojo/inte... media/mojo/interfaces/service_factory.mojom:26: string? audio_device_id); On 2016/06/02 06:09:29, xhwang wrote: > +alop: Just FYI, this will make it harder for us to get rid of this mojom > interface now. One solution is to add a > > Create(audio_device_id) -> (bool success) > > on the Renderer interface, so that the caller will do: > > ConnectToInterface(GetProxy(&renderer)); > renderer->Create(create_cb); > > We have similar issues on the VideoDecoder mojo interface: > https://code.google.com/p/chromium/codesearch#chromium/src/media/mojo/interfa... Hmm. It would be weird to call Create on an already created Interface . It would be nice if ConnectToInterface allowed a struct to be passed to the Interface implementation. At the same time I also do not like an Initialize method on an abstract interface. I do not have a better suggestion at this time. I will keep thinking about this.
https://chromiumcodereview.appspot.com/2016053003/diff/140001/media/mojo/serv... File media/mojo/services/mojo_media_client.h (right): https://chromiumcodereview.appspot.com/2016053003/diff/140001/media/mojo/serv... media/mojo/services/mojo_media_client.h:55: const scoped_refptr<base::SingleThreadTaskRunner>& media_task_runner, On 2016/06/02 05:26:06, alokp wrote: > On 2016/06/02 05:18:39, tianyuwang1 wrote: > > On 2016/06/02 04:47:55, alokp wrote: > > > since we intend to share the ownership, scoped_refptr should be passed by > copy > > > for both media_task_runner and media_log. > > > > > > > > > https://www.chromium.org/developers/coding-style#TOC-Object-ownership-and-cal... > > > > According to the style guide above: > > "If the function (at least sometimes) takes a ref on a refcounted object, > > declare the param as scoped_refptr<T>. The caller can decide whether it wishes > > to transfer ownership (by calling std::move(t) when passing t) or retain its > ref > > (by simply passing t directly)." > > > > So we don't need to pass by copy for media_task_runner or media_log? We can > > simply pass them directly? > > The function parameter should be scoped_refptr<base::SingleThreadTaskRunner> > instead of "const scoped_refptr<base::SingleThreadTaskRunner>&". Individual > callers will decide whether to use std::move or pass by copy. Done. https://chromiumcodereview.appspot.com/2016053003/diff/160001/media/mojo/inte... File media/mojo/interfaces/service_factory.mojom (right): https://chromiumcodereview.appspot.com/2016053003/diff/160001/media/mojo/inte... media/mojo/interfaces/service_factory.mojom:24: // is null, kDefaultDeviceId will be used. On 2016/06/02 06:09:29, xhwang wrote: > I wasn't aware that in C++ code when the device ID is empty then we'll choose > the default device. In this case, maybe we should do the same on the mojo > interface. Basically drop the "?" part to make this parameter mandatory, and > update this comment s/null/empty. > > Then we can also get rid of the use of mojo::String(nullptr) which seems a bit > odd. > > WDYT? Done. https://chromiumcodereview.appspot.com/2016053003/diff/160001/media/mojo/serv... File media/mojo/services/test_mojo_media_client.h (right): https://chromiumcodereview.appspot.com/2016053003/diff/160001/media/mojo/serv... media/mojo/services/test_mojo_media_client.h:47: VideoRendererSink* CreateVideoRendererSink( On 2016/06/02 06:09:29, xhwang wrote: > nit: s/Create/Get/ for these 3 functions which makes more sense. Done.
The CQ bit was checked by tianyuwang@google.com to run a CQ dry run
Description was changed from ========== device_id is needed for audio volume ducking. device_id is defined in media/audio/audio_device_description.h. For example, when a voice call is started, sound will be played using kCommunicationsDeviceId. All other audio streams being played with kDefaultDeviceId will be ducked to a lower volume. Bug: b/28193093 ========== to ========== audio_device_id is needed for audio volume ducking. audio_device_id is defined in media/audio/audio_device_description.h. For example, when a voice call is started, sound will be played using kCommunicationsDeviceId. All other audio streams being played with kDefaultDeviceId will be ducked to a lower volume. Bug: b/28193093 ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2016053003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2016053003/180001
lgtm with one question https://chromiumcodereview.appspot.com/2016053003/diff/180001/media/mojo/serv... File media/mojo/services/service_factory_impl.h (right): https://chromiumcodereview.appspot.com/2016053003/diff/180001/media/mojo/serv... media/mojo/services/service_factory_impl.h:44: const std::string& audio_device_id) final; Is this what the service_factory.mojom.h file is generating?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
Description was changed from ========== audio_device_id is needed for audio volume ducking. audio_device_id is defined in media/audio/audio_device_description.h. For example, when a voice call is started, sound will be played using kCommunicationsDeviceId. All other audio streams being played with kDefaultDeviceId will be ducked to a lower volume. Bug: b/28193093 ========== to ========== audio_device_id is needed for audio volume ducking. audio_device_id is defined in media/audio/audio_device_description.h. The renderer needs a way to specify which audio device the sound should be play out. Bug: b/28193093 ==========
The CQ bit was checked by tianyuwang@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org, alokp@chromium.org, xhwang@chromium.org Link to the patchset: https://codereview.chromium.org/2016053003/#ps200001 (title: "Fix compile error")
https://codereview.chromium.org/2016053003/diff/180001/media/mojo/services/se... File media/mojo/services/service_factory_impl.h (right): https://codereview.chromium.org/2016053003/diff/180001/media/mojo/services/se... media/mojo/services/service_factory_impl.h:44: const std::string& audio_device_id) final; On 2016/06/02 21:53:50, xhwang wrote: > Is this what the service_factory.mojom.h file is generating? Fixed. service_factory.mojom.h is generating mojo::String.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2016053003/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2016053003/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_android on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Description was changed from ========== audio_device_id is needed for audio volume ducking. audio_device_id is defined in media/audio/audio_device_description.h. The renderer needs a way to specify which audio device the sound should be play out. Bug: b/28193093 ========== to ========== audio_device_id is needed for audio volume ducking. audio_device_id is defined in media/audio/audio_device_description.h. The renderer needs a way to specify which audio device the sound should be play out. Bug=b/28193093 ==========
The CQ bit was checked by tianyuwang@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2016053003/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2016053003/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by tianyuwang@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2016053003/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2016053003/220001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2016053003/diff/220001/chromecast/browser/med... File chromecast/browser/media/cast_mojo_media_client.cc (right): https://codereview.chromium.org/2016053003/diff/220001/chromecast/browser/med... chromecast/browser/media/cast_mojo_media_client.cc:26: return base::WrapUnique(new chromecast::media::CastRenderer( return base::MakeUnique<chromecast::media::CastRenderer>(create_backend_cb_, std::move(media_task_runner)); https://codereview.chromium.org/2016053003/diff/220001/content/renderer/media... File content/renderer/media/media_interface_provider.cc (right): https://codereview.chromium.org/2016053003/diff/220001/content/renderer/media... content/renderer/media/media_interface_provider.cc:36: mojo::MakeRequest<media::mojom::Renderer>(std::move(pipe)), ""); std::string() instead of "", here and elsewhere. https://codereview.chromium.org/2016053003/diff/220001/media/mojo/interfaces/... File media/mojo/interfaces/service_factory.mojom (right): https://codereview.chromium.org/2016053003/diff/220001/media/mojo/interfaces/... media/mojo/interfaces/service_factory.mojom:24: // is empty, kDefaultDeviceId will be used. Is the device_id structured data of any kind? Or is it an opaque handle? What is a Renderer service? I looked at renderer.mojom but there's no comments on the interface... What process does ServiceFactory live in? And who calls into ServiceFactory?
https://codereview.chromium.org/2016053003/diff/220001/media/mojo/interfaces/... File media/mojo/interfaces/service_factory.mojom (right): https://codereview.chromium.org/2016053003/diff/220001/media/mojo/interfaces/... media/mojo/interfaces/service_factory.mojom:24: // is empty, kDefaultDeviceId will be used. On 2016/06/03 04:29:48, dcheng wrote: > Is the device_id structured data of any kind? Or is it an opaque handle? device_id is just a string. See dalecurtis's explanation here: https://codereview.chromium.org/2016053003/#msg18 > What is > a Renderer service? I looked at renderer.mojom but there's no comments on the > interface... A Renderer service is the mojo version of media::Renderer interface: https://code.google.com/p/chromium/codesearch#chromium/src/media/base/rendere... Basically it reads demuxed compressed audio/video buffers, and try to decode them and render them. Agreed that documentation can be improved. > What process does ServiceFactory live in? And who calls into ServiceFactory? ServiceFactory implementation lives in the process where these services (CDM, Renderer, etc) are provided. Based on different configuration, this process could be the Browser process, GPU process or Utility process. In the ChromeCast case, it's in the Browser process but they do plan to move it to the Utility process in the future. Note that currently the ChromeCast media pipeline is living in the Browser process as well. So the current work is mostly replace IPC with mojo without changing the process model. ServiceFactory typically will be called by the Render process, where the media pipeline in Render frames lives. In this particular case, it'll be called by some ChromeCast code in the utility process. tianyuwang@ will know more details.
The CQ bit was checked by tianyuwang@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2016053003/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2016053003/240001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2016053003/diff/220001/chromecast/browser/med... File chromecast/browser/media/cast_mojo_media_client.cc (right): https://codereview.chromium.org/2016053003/diff/220001/chromecast/browser/med... chromecast/browser/media/cast_mojo_media_client.cc:26: return base::WrapUnique(new chromecast::media::CastRenderer( On 2016/06/03 04:29:48, dcheng wrote: > return base::MakeUnique<chromecast::media::CastRenderer>(create_backend_cb_, > std::move(media_task_runner)); Done. https://codereview.chromium.org/2016053003/diff/220001/content/renderer/media... File content/renderer/media/media_interface_provider.cc (right): https://codereview.chromium.org/2016053003/diff/220001/content/renderer/media... content/renderer/media/media_interface_provider.cc:36: mojo::MakeRequest<media::mojom::Renderer>(std::move(pipe)), ""); On 2016/06/03 04:29:48, dcheng wrote: > std::string() instead of "", here and elsewhere. Done. https://codereview.chromium.org/2016053003/diff/220001/media/mojo/interfaces/... File media/mojo/interfaces/service_factory.mojom (right): https://codereview.chromium.org/2016053003/diff/220001/media/mojo/interfaces/... media/mojo/interfaces/service_factory.mojom:24: // is empty, kDefaultDeviceId will be used. On 2016/06/03 04:53:28, xhwang wrote: > On 2016/06/03 04:29:48, dcheng wrote: > > Is the device_id structured data of any kind? Or is it an opaque handle? > > device_id is just a string. See dalecurtis's explanation here: > https://codereview.chromium.org/2016053003/#msg18 > > > What is > > a Renderer service? I looked at renderer.mojom but there's no comments on the > > interface... > > A Renderer service is the mojo version of media::Renderer interface: > https://code.google.com/p/chromium/codesearch#chromium/src/media/base/rendere... > > Basically it reads demuxed compressed audio/video buffers, and try to decode > them and render them. > > Agreed that documentation can be improved. > > > What process does ServiceFactory live in? And who calls into ServiceFactory? > > ServiceFactory implementation lives in the process where these services (CDM, > Renderer, etc) are provided. Based on different configuration, this process > could be the Browser process, GPU process or Utility process. In the ChromeCast > case, it's in the Browser process but they do plan to move it to the Utility > process in the future. Note that currently the ChromeCast media pipeline is > living in the Browser process as well. So the current work is mostly replace IPC > with mojo without changing the process model. > > ServiceFactory typically will be called by the Render process, where the media > pipeline in Render frames lives. In this particular case, it'll be called by > some ChromeCast code in the utility process. tianyuwang@ will know more details. On ChromeCast, ServiceFactory is called by Renderer process as Xiaohan mentioned. ServiceFactory will also be called from the utility process in order to play audio.
I'm honestly a bit confused about how media's mojo services are supposed to be distributed, and the mojoms (and replies on here) are not really helping clear up that confusion. Let's chat on Monday.
Have you started working on the followup CL that actually consumes |audio_device_id|? In general, we should try to land mojom changes with the code that actually uses the new parameter. https://codereview.chromium.org/2016053003/diff/240001/media/mojo/services/te... File media/mojo/services/test_mojo_media_client.cc (right): https://codereview.chromium.org/2016053003/diff/240001/media/mojo/services/te... media/mojo/services/test_mojo_media_client.cc:64: RendererFactory* renderer_factory = GetRendererFactory(media_log); So here we can std::move(). https://codereview.chromium.org/2016053003/diff/240001/media/mojo/services/te... media/mojo/services/test_mojo_media_client.cc:77: renderer_factory_ = base::WrapUnique(new DefaultRendererFactory( And then std::move() ownership here into the DefaultRendererFactory. As a minor nit, base::MakeUnique<DefaultRendererFactory(...) here as well. https://codereview.chromium.org/2016053003/diff/240001/media/mojo/services/te... File media/mojo/services/test_mojo_media_client.h (right): https://codereview.chromium.org/2016053003/diff/240001/media/mojo/services/te... media/mojo/services/test_mojo_media_client.h:44: RendererFactory* GetRendererFactory(const scoped_refptr<MediaLog>& media_log); In general, we prefer scoped_refptr<T> over const scoped_refptr<T>&, using std::move() to transfer ownership when applicable.
On 2016/06/06 21:24:08, dcheng wrote: > Have you started working on the followup CL that actually consumes > |audio_device_id|? In general, we should try to land mojom changes with the code > that actually uses the new parameter. I don't have cl right now that consumes |audio_device_id|. I'll work on it later this week. We will consume |audio_device_id| in chromecast/browser/media/cast_mojo_media_client.cc and pass it into CastRenderer. Do you think we need to have that cl ready before this API change can be submitted? Or I can send out a follow up cl to you later for the actually implementation of CastRenderer. > > https://codereview.chromium.org/2016053003/diff/240001/media/mojo/services/te... > File media/mojo/services/test_mojo_media_client.cc (right): > > https://codereview.chromium.org/2016053003/diff/240001/media/mojo/services/te... > media/mojo/services/test_mojo_media_client.cc:64: RendererFactory* > renderer_factory = GetRendererFactory(media_log); > So here we can std::move(). > > https://codereview.chromium.org/2016053003/diff/240001/media/mojo/services/te... > media/mojo/services/test_mojo_media_client.cc:77: renderer_factory_ = > base::WrapUnique(new DefaultRendererFactory( > And then std::move() ownership here into the DefaultRendererFactory. > > As a minor nit, base::MakeUnique<DefaultRendererFactory(...) here as well. > > https://codereview.chromium.org/2016053003/diff/240001/media/mojo/services/te... > File media/mojo/services/test_mojo_media_client.h (right): > > https://codereview.chromium.org/2016053003/diff/240001/media/mojo/services/te... > media/mojo/services/test_mojo_media_client.h:44: RendererFactory* > GetRendererFactory(const scoped_refptr<MediaLog>& media_log); > In general, we prefer scoped_refptr<T> over const scoped_refptr<T>&, using > std::move() to transfer ownership when applicable.
https://codereview.chromium.org/2016053003/diff/240001/media/mojo/services/te... File media/mojo/services/test_mojo_media_client.cc (right): https://codereview.chromium.org/2016053003/diff/240001/media/mojo/services/te... media/mojo/services/test_mojo_media_client.cc:64: RendererFactory* renderer_factory = GetRendererFactory(media_log); On 2016/06/06 21:24:08, dcheng wrote: > So here we can std::move(). Done. https://codereview.chromium.org/2016053003/diff/240001/media/mojo/services/te... media/mojo/services/test_mojo_media_client.cc:77: renderer_factory_ = base::WrapUnique(new DefaultRendererFactory( On 2016/06/06 21:24:08, dcheng wrote: > And then std::move() ownership here into the DefaultRendererFactory. > > As a minor nit, base::MakeUnique<DefaultRendererFactory(...) here as well. Done. https://codereview.chromium.org/2016053003/diff/240001/media/mojo/services/te... File media/mojo/services/test_mojo_media_client.h (right): https://codereview.chromium.org/2016053003/diff/240001/media/mojo/services/te... media/mojo/services/test_mojo_media_client.h:44: RendererFactory* GetRendererFactory(const scoped_refptr<MediaLog>& media_log); On 2016/06/06 21:24:08, dcheng wrote: > In general, we prefer scoped_refptr<T> over const scoped_refptr<T>&, using > std::move() to transfer ownership when applicable. Done.
I would prefer to see the followup CL that actually consumes it merged into this. It gives better context for the security review, since part of the security review for ipc changes is how the new values are consumed (important in this case, since the device id is coming from a less-trusted process to a more-trusted process).
On 2016/06/07 02:28:39, dcheng wrote: > I would prefer to see the followup CL that actually consumes it merged into > this. It gives better context for the security review, since part of the > security review for ipc changes is how the new values are consumed (important in > this case, since the device id is coming from a less-trusted process to a > more-trusted process). dcheng: A general question. In the future, do you prefer we have a large CL with interface changes and the full implementation ready so reviewers will know how the new interfaces will be used, or do you prefer incremental CLs, where by looking at later CLs uploaded, you'll have the full context to review earlier CLs. I would prefer the latter model to make the process easier for everybody. The later CLs don't even need to complete; it could be just some prototype to show the direction and intention of the interface change. WDYT?
On 2016/06/07 23:38:49, xhwang wrote: > On 2016/06/07 02:28:39, dcheng wrote: > > I would prefer to see the followup CL that actually consumes it merged into > > this. It gives better context for the security review, since part of the > > security review for ipc changes is how the new values are consumed (important > in > > this case, since the device id is coming from a less-trusted process to a > > more-trusted process). > > dcheng: A general question. In the future, do you prefer we have a large CL with > interface changes and the full implementation ready so reviewers will know how > the new interfaces will be used, or do you prefer incremental CLs, where by > looking at later CLs uploaded, you'll have the full context to review earlier > CLs. I would prefer the latter model to make the process easier for everybody. > The later CLs don't even need to complete; it could be just some prototype to > show the direction and intention of the interface change. WDYT? Obviously, giant patches are better to split up if possible. But I think splitting up something into several incremental CLs actually makes security reviewing harder, since I have to jump between several different CLs just to see what the final picture is going to be.
On 2016/06/13 17:37:56, dcheng wrote: > On 2016/06/07 23:38:49, xhwang wrote: > > On 2016/06/07 02:28:39, dcheng wrote: > > > I would prefer to see the followup CL that actually consumes it merged into > > > this. It gives better context for the security review, since part of the > > > security review for ipc changes is how the new values are consumed > (important > > in > > > this case, since the device id is coming from a less-trusted process to a > > > more-trusted process). > > > > dcheng: A general question. In the future, do you prefer we have a large CL > with > > interface changes and the full implementation ready so reviewers will know how > > the new interfaces will be used, or do you prefer incremental CLs, where by > > looking at later CLs uploaded, you'll have the full context to review earlier > > CLs. I would prefer the latter model to make the process easier for everybody. > > The later CLs don't even need to complete; it could be just some prototype to > > show the direction and intention of the interface change. WDYT? > > Obviously, giant patches are better to split up if possible. But I think > splitting up something into several incremental CLs actually makes security > reviewing harder, since I have to jump between several different CLs just to see > what the final picture is going to be. Sorry for the delay. I've been dragged into a more urgent production issue. I'll try to finish this cl later this week.
On 2016/06/13 17:37:56, dcheng wrote: > On 2016/06/07 23:38:49, xhwang wrote: > > On 2016/06/07 02:28:39, dcheng wrote: > > > I would prefer to see the followup CL that actually consumes it merged into > > > this. It gives better context for the security review, since part of the > > > security review for ipc changes is how the new values are consumed > (important > > in > > > this case, since the device id is coming from a less-trusted process to a > > > more-trusted process). > > > > dcheng: A general question. In the future, do you prefer we have a large CL > with > > interface changes and the full implementation ready so reviewers will know how > > the new interfaces will be used, or do you prefer incremental CLs, where by > > looking at later CLs uploaded, you'll have the full context to review earlier > > CLs. I would prefer the latter model to make the process easier for everybody. > > The later CLs don't even need to complete; it could be just some prototype to > > show the direction and intention of the interface change. WDYT? > > Obviously, giant patches are better to split up if possible. But I think > splitting up something into several incremental CLs actually makes security > reviewing harder, since I have to jump between several different CLs just to see > what the final picture is going to be. Thanks! Would design doc help in this case? I am a big fan of incremental CLs but recently I have seen several cases where we are required to implement everything in a big CL by security reviewers. The problem with this approach is that security review comments often requires non-trivial changes to the design and API, which would require a lot of changes in a big CL (interface, implementation, test etc), which would be big pain for the author and reviewers. At least we should have some way to get security reviewers kicking in early so that we don't have big surprises later in the process. Would prototype CLs (e.g. working prototype without tests), and/or design docs help in these cases?
tianyuwang: Actually this CL contains some refactoring that is not related to audio_device_id. Does it make sense to extract the refactoring part out and land it first as a separate CL?
The CQ bit was checked by tianyuwang@google.com 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...
Sorry for the long delay. Just finished another more urgent task. I've added the implementation to use audio_device_id in CastRenderer. @Alok and Daniel, can you guys take another look at the CastRenderer implementation? The API part didn't change.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by tianyuwang@google.com 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_...)
https://codereview.chromium.org/2016053003/diff/280001/chromecast/browser/med... File chromecast/browser/media/cast_renderer.cc (right): https://codereview.chromium.org/2016053003/diff/280001/chromecast/browser/med... chromecast/browser/media/cast_renderer.cc:32: int GetStreamType(const std::string& audio_device_id) { This is weird. Where are these stream-type integers defined? https://codereview.chromium.org/2016053003/diff/280001/media/mojo/interfaces/... File media/mojo/interfaces/service_factory.mojom (right): https://codereview.chromium.org/2016053003/diff/280001/media/mojo/interfaces/... media/mojo/interfaces/service_factory.mojom:25: CreateRenderer(Renderer& renderer, nit: Input parameters should be placed before the output ones.
https://codereview.chromium.org/2016053003/diff/280001/chromecast/browser/med... File chromecast/browser/media/media_pipeline_backend_factory.h (right): https://codereview.chromium.org/2016053003/diff/280001/chromecast/browser/med... chromecast/browser/media/media_pipeline_backend_factory.h:19: int stream_type)> CreateMediaPipelineBackendCB; I think it would be better to take the device ID string here rather than an integer stream type. Then we can do the mapping as appropriate in CastContentBrowserClientInternal.
The CQ bit was checked by tianyuwang@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from alokp@chromium.org, dalecurtis@chromium.org, xhwang@chromium.org Link to the patchset: https://codereview.chromium.org/2016053003/#ps300001 (title: "Fix comments")
The CQ bit was unchecked by tianyuwang@google.com
The CQ bit was checked by tianyuwang@google.com to run a CQ dry run
PTAL https://codereview.chromium.org/2016053003/diff/280001/chromecast/browser/med... File chromecast/browser/media/cast_renderer.cc (right): https://codereview.chromium.org/2016053003/diff/280001/chromecast/browser/med... chromecast/browser/media/cast_renderer.cc:32: int GetStreamType(const std::string& audio_device_id) { On 2016/07/12 05:01:30, alokp wrote: > This is weird. Where are these stream-type integers defined? They are not defined in cast_shell, because we didn't want to leak the enums. That's why we were using an integer. They are defined in the caller right now (Assistant code). We can switch over to use audio_device_id directly in MediaBackendPipelineManager. But I would prefer to do it in a separate cl, since we need to change multiple places in order to switch from stream_type to audio_device_id. I've changed the code so that we can do the mapping between audio_device_id to stream_type in CastContentBrowserInternal for now. https://codereview.chromium.org/2016053003/diff/280001/chromecast/browser/med... File chromecast/browser/media/media_pipeline_backend_factory.h (right): https://codereview.chromium.org/2016053003/diff/280001/chromecast/browser/med... chromecast/browser/media/media_pipeline_backend_factory.h:19: int stream_type)> CreateMediaPipelineBackendCB; On 2016/07/12 05:20:31, kmackay wrote: > I think it would be better to take the device ID string here rather than an > integer stream type. Then we can do the mapping as appropriate in > CastContentBrowserClientInternal. Done. https://codereview.chromium.org/2016053003/diff/280001/media/mojo/interfaces/... File media/mojo/interfaces/service_factory.mojom (right): https://codereview.chromium.org/2016053003/diff/280001/media/mojo/interfaces/... media/mojo/interfaces/service_factory.mojom:25: CreateRenderer(Renderer& renderer, On 2016/07/12 05:01:30, alokp wrote: > nit: Input parameters should be placed before the output ones. Done.
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 checked by tianyuwang@google.com 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 checked by tianyuwang@google.com 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 to run a CQ dry run
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
https://codereview.chromium.org/2016053003/diff/360001/chromecast/browser/cas... File chromecast/browser/cast_content_browser_client.cc (right): https://codereview.chromium.org/2016053003/diff/360001/chromecast/browser/cas... chromecast/browser/cast_content_browser_client.cc:160: params, audio_device_id); Don't pass the audio_device_id to media_pipeline_backend_manager()->CreateMediaPipelineBackend() here (just pass params).
The CQ bit was checked by tianyuwang@google.com to run a CQ dry run
Fixed an error https://codereview.chromium.org/2016053003/diff/360001/chromecast/browser/cas... File chromecast/browser/cast_content_browser_client.cc (right): https://codereview.chromium.org/2016053003/diff/360001/chromecast/browser/cas... chromecast/browser/cast_content_browser_client.cc:160: params, audio_device_id); On 2016/07/12 16:22:08, kmackay wrote: > Don't pass the audio_device_id to > media_pipeline_backend_manager()->CreateMediaPipelineBackend() here (just pass > params). Oops. Done.
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/2016053003/diff/380001/chromecast/browser/med... File chromecast/browser/media/cast_mojo_media_client.h (left): https://codereview.chromium.org/2016053003/diff/380001/chromecast/browser/med... chromecast/browser/media/cast_mojo_media_client.h:26: std::unique_ptr<::media::CdmFactory> CreateCdmFactory( Is CreateCdmFactory() supposed to be deleted?
The CQ bit was unchecked by commit-bot@chromium.org to run a CQ dry run
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2016053003/diff/380001/chromecast/browser/med... File chromecast/browser/media/cast_mojo_media_client.h (left): https://codereview.chromium.org/2016053003/diff/380001/chromecast/browser/med... chromecast/browser/media/cast_mojo_media_client.h:26: std::unique_ptr<::media::CdmFactory> CreateCdmFactory( On 2016/07/12 16:55:45, kmackay wrote: > Is CreateCdmFactory() supposed to be deleted? Nope. Added it back. Caused by a bad merge.
The CQ bit was checked by tianyuwang@google.com 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 % nit https://codereview.chromium.org/2016053003/diff/400001/chromecast/browser/cas... File chromecast/browser/cast_content_browser_client.cc (right): https://codereview.chromium.org/2016053003/diff/400001/chromecast/browser/cas... chromecast/browser/cast_content_browser_client.cc:160: params, 0); nit: I'd prefer to leave this as before (ie, don't pass in 0)
The CQ bit was checked by tianyuwang@google.com 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/2016053003/diff/400001/chromecast/browser/cas... File chromecast/browser/cast_content_browser_client.cc (right): https://codereview.chromium.org/2016053003/diff/400001/chromecast/browser/cas... chromecast/browser/cast_content_browser_client.cc:160: params, 0); On 2016/07/12 18:35:52, kmackay wrote: > nit: I'd prefer to leave this as before (ie, don't pass in 0) Done.
tianyuwang@google.com changed reviewers: + kenrb@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org to run a CQ dry run
Dry run: This issue passed the CQ dry run.
As dcheng mentioned, security IPC reviews are tricky when the handling code isn't implemented. Is it accurate to say that audio_device_id is used just to compare against special pre-defined values, and then possibly to be compared against hashes of device names? If that's true, it isn't especially high risk, although we'd want to have a look at anything more complicated than that. There is a history of renderer sandbox escapes via IPC handlers that don't account for all edge cases.
On 2016/07/13 00:57:42, kenrb wrote: > As dcheng mentioned, security IPC reviews are tricky when the handling code > isn't implemented. > > Is it accurate to say that audio_device_id is used just to compare against > special pre-defined values, and then possibly to be compared against hashes of > device names? This is correct. Right now chromecast is the only one using this, and in chromecast implementation we only compare audio_device_id against special pre-defined values. > > If that's true, it isn't especially high risk, although we'd want to have a look > at anything more complicated than that. There is a history of renderer sandbox > escapes via IPC handlers that don't account for all edge cases. There shouldn't be any more complicated logic.
On 2016/07/13 01:08:50, tianyuwang1 wrote: > On 2016/07/13 00:57:42, kenrb wrote: > > As dcheng mentioned, security IPC reviews are tricky when the handling code > > isn't implemented. > > > > Is it accurate to say that audio_device_id is used just to compare against > > special pre-defined values, and then possibly to be compared against hashes of > > device names? > > This is correct. Right now chromecast is the only one using this, and in > chromecast implementation we only compare audio_device_id against special > pre-defined values. > > > > > If that's true, it isn't especially high risk, although we'd want to have a > look > > at anything more complicated than that. There is a history of renderer sandbox > > escapes via IPC handlers that don't account for all edge cases. > > There shouldn't be any more complicated logic. Here is how we compare audio_device_id with a special pre-defined values: https://codereview.chromium.org/2016053003/patch/280001/290005
ipc lgtm
On 2016/07/13 01:11:02, tianyuwang1 wrote: > On 2016/07/13 01:08:50, tianyuwang1 wrote: > > On 2016/07/13 00:57:42, kenrb wrote: > > > As dcheng mentioned, security IPC reviews are tricky when the handling code > > > isn't implemented. > > > > > > Is it accurate to say that audio_device_id is used just to compare against > > > special pre-defined values, and then possibly to be compared against hashes > of > > > device names? > > > > This is correct. Right now chromecast is the only one using this, and in > > chromecast implementation we only compare audio_device_id against special > > pre-defined values. > > > > > > > > If that's true, it isn't especially high risk, although we'd want to have a > > look > > > at anything more complicated than that. There is a history of renderer > sandbox > > > escapes via IPC handlers that don't account for all edge cases. > > > > There shouldn't be any more complicated logic. > > Here is how we compare audio_device_id with a special pre-defined values: > https://codereview.chromium.org/2016053003/patch/280001/290005 FWIW, if it's just special pre-defined values, why not use an enum? =)
On 2016/07/13 13:25:37, dcheng wrote: > On 2016/07/13 01:11:02, tianyuwang1 wrote: > > On 2016/07/13 01:08:50, tianyuwang1 wrote: > > > On 2016/07/13 00:57:42, kenrb wrote: > > > > As dcheng mentioned, security IPC reviews are tricky when the handling > code > > > > isn't implemented. > > > > > > > > Is it accurate to say that audio_device_id is used just to compare against > > > > special pre-defined values, and then possibly to be compared against > hashes > > of > > > > device names? > > > > > > This is correct. Right now chromecast is the only one using this, and in > > > chromecast implementation we only compare audio_device_id against special > > > pre-defined values. > > > > > > > > > > > If that's true, it isn't especially high risk, although we'd want to have > a > > > look > > > > at anything more complicated than that. There is a history of renderer > > sandbox > > > > escapes via IPC handlers that don't account for all edge cases. > > > > > > There shouldn't be any more complicated logic. > > > > Here is how we compare audio_device_id with a special pre-defined values: > > https://codereview.chromium.org/2016053003/patch/280001/290005 > > FWIW, if it's just special pre-defined values, why not use an enum? =) On Chromecast it only uses special pre-defined values. But may be in other platform, they have some use case where they want to compare it against hashes.
The CQ bit was checked by tianyuwang@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from alokp@chromium.org, dalecurtis@chromium.org, xhwang@chromium.org, kmackay@chromium.org Link to the patchset: https://codereview.chromium.org/2016053003/#ps420001 (title: "Minor change based on the comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== audio_device_id is needed for audio volume ducking. audio_device_id is defined in media/audio/audio_device_description.h. The renderer needs a way to specify which audio device the sound should be play out. Bug=b/28193093 ========== to ========== audio_device_id is needed for audio volume ducking. audio_device_id is defined in media/audio/audio_device_description.h. The renderer needs a way to specify which audio device the sound should be play out. Bug=b/28193093 ==========
Message was sent while issue was closed.
Committed patchset #22 (id:420001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== audio_device_id is needed for audio volume ducking. audio_device_id is defined in media/audio/audio_device_description.h. The renderer needs a way to specify which audio device the sound should be play out. Bug=b/28193093 ========== to ========== audio_device_id is needed for audio volume ducking. audio_device_id is defined in media/audio/audio_device_description.h. The renderer needs a way to specify which audio device the sound should be play out. Bug=b/28193093 Committed: https://crrev.com/f3c4a58fcc70cd3384020708ce3ff50d847654ba Cr-Commit-Position: refs/heads/master@{#405213} ==========
Message was sent while issue was closed.
Patchset 22 (id:??) landed as https://crrev.com/f3c4a58fcc70cd3384020708ce3ff50d847654ba Cr-Commit-Position: refs/heads/master@{#405213}
Message was sent while issue was closed.
On 2016/07/13 17:30:57, tianyuwang1 wrote: > On 2016/07/13 13:25:37, dcheng wrote: > > On 2016/07/13 01:11:02, tianyuwang1 wrote: > > > On 2016/07/13 01:08:50, tianyuwang1 wrote: > > > > On 2016/07/13 00:57:42, kenrb wrote: > > > > > As dcheng mentioned, security IPC reviews are tricky when the handling > > code > > > > > isn't implemented. > > > > > > > > > > Is it accurate to say that audio_device_id is used just to compare > against > > > > > special pre-defined values, and then possibly to be compared against > > hashes > > > of > > > > > device names? > > > > > > > > This is correct. Right now chromecast is the only one using this, and in > > > > chromecast implementation we only compare audio_device_id against special > > > > pre-defined values. > > > > > > > > > > > > > > If that's true, it isn't especially high risk, although we'd want to > have > > a > > > > look > > > > > at anything more complicated than that. There is a history of renderer > > > sandbox > > > > > escapes via IPC handlers that don't account for all edge cases. > > > > > > > > There shouldn't be any more complicated logic. > > > > > > Here is how we compare audio_device_id with a special pre-defined values: > > > https://codereview.chromium.org/2016053003/patch/280001/290005 > > > > FWIW, if it's just special pre-defined values, why not use an enum? =) > > On Chromecast it only uses special pre-defined values. But may be in other > platform, they have some use case where they want to compare it against hashes. If it's just a hypothetical use case, it still seems better to start with the more narrowly scoped type (restricted to enums) rather than allowing freeform strings. If it's something that will actually be needed, then this is fine.
Message was sent while issue was closed.
> > If it's just a hypothetical use case, it still seems better to start with the > more narrowly scoped type (restricted to enums) rather than allowing freeform > strings. If it's something that will actually be needed, then this is fine. I think we don't necessarily want to expose the enum names publicly. |
