Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2697793002:
Add mojo interface+impl for audio stream control. (Closed)
|
Created:
3 years, 10 months ago by Max Morin Modified:
3 years, 9 months ago CC:
Aaron Boodman, abarth-chromium, alokp+watch_chromium.org, audio-mojo-cl_google.com, chromium-reviews, darin (slow to review), darin-cc_chromium.org, feature-media-reviews_chromium.org, 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/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd mojo interface+impl for audio stream control.
Summary:
* AudioOutputStream is an interface for control messages for a single stream, and AudioOutputStreamProvider is a single-use factory for AudioOutputStreams.
* The changes in media/mojo/interfaces/, media/base/ipc, and
content/common/media/audio_messages.h mojofy OutputDeviceStatus.
* media/mojo/interfaces/audio_output.mojom defines the IPC interface.
* AudioOutputDelegate is moved to media. This also causes some small
changes to related files
* It is not possible to keep the AudioOutputStreamProviders in a StrongBindingSet
since they need to be destroyed on error, but they cannot propagate
an error to the StrongBindingSet from the inside.
* Not hooked up anywhere yet.
This CL is split off from https://codereview.chromium.org/2319493002/.
Design doc at https://docs.google.com/document/d/1awQoajq_DLmz2AIU9iweC0zEYlVuHCvEIRQepeYyxh8/edit
The "Callback to be deleted on error" is used in many places. Here are some cases where it is used by storing a pointer to owner: https://cs.chromium.org/chromium/src/device/geolocation/geolocation_service_impl.cc?l=152, https://cs.chromium.org/chromium/src/content/browser/permissions/permission_service_impl.cc?l=100, https://cs.chromium.org/chromium/src/content/browser/payments/payment_app_manager.cc?l=61, https://cs.chromium.org/chromium/src/content/browser/background_sync/background_sync_service_impl.cc?l=91, https://cs.chromium.org/chromium/src/content/browser/notifications/blink_notification_service_impl.cc?l=66.
BUG=425368
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Review-Url: https://codereview.chromium.org/2697793002
Cr-Commit-Position: refs/heads/master@{#456795}
Committed: https://chromium.googlesource.com/chromium/src/+/aa3e611c9d8b00201fab5d36af2758b618e94b05
Patch Set 1 #Patch Set 2 : Move to media #
Total comments: 22
Patch Set 3 : Dale's comments and add MEDIA_MOJO_EXPORT #
Total comments: 6
Patch Set 4 : Introduce provider #
Total comments: 25
Patch Set 5 : review. #
Total comments: 5
Patch Set 6 : . #
Total comments: 2
Patch Set 7 : format mojom #Patch Set 8 : Fix linking #Patch Set 9 : Fix linking once and for all. #Patch Set 10 : update build files #Patch Set 11 : Don't inline AudioOutputDelegate(EventHandler) dtor. #Messages
Total messages: 94 (51 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 control. Summary: * AudioOutput is an interface for control messages for a single stream. * The changes in media/mojo/interfaces/, media/base/ipc, and content/common/media/audio_messages.h mojofy OutputDeviceStatus. * content/common/media/audio_output.mojom defines the IPC interface. * AFAIK sync sockets and shared memory is not welcome in media/, so AudioOutputImpl lives in content/. Feel free to tell me if I got this wrong, as the directory hierarchy is unclear to me :). This CL is split off https://codereview.chromium.org/2319493002/. Design doc at https://docs.google.com/document/d/1awQoajq_DLmz2AIU9iweC0zEYlVuHCvEIRQepeYyx... BUG=425368 ========== to ========== Add mojo interface+impl for audio stream control. Summary: * AudioOutput is an interface for control messages for a single stream. * The changes in media/mojo/interfaces/, media/base/ipc, and content/common/media/audio_messages.h mojofy OutputDeviceStatus. * content/common/media/audio_output.mojom defines the IPC interface. * AFAIK sync sockets and shared memory is not welcome in media/, so AudioOutputImpl lives in content/. Feel free to tell me if I got this wrong, as the directory hierarchy is unclear to me :). This CL is split off from https://codereview.chromium.org/2319493002/. Design doc at https://docs.google.com/document/d/1awQoajq_DLmz2AIU9iweC0zEYlVuHCvEIRQepeYyx... BUG=425368 ==========
maxmorin@chromium.org changed reviewers: + olka@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Olga: PTAL.
Description was changed from ========== Add mojo interface+impl for audio stream control. Summary: * AudioOutput is an interface for control messages for a single stream. * The changes in media/mojo/interfaces/, media/base/ipc, and content/common/media/audio_messages.h mojofy OutputDeviceStatus. * content/common/media/audio_output.mojom defines the IPC interface. * AFAIK sync sockets and shared memory is not welcome in media/, so AudioOutputImpl lives in content/. Feel free to tell me if I got this wrong, as the directory hierarchy is unclear to me :). This CL is split off from https://codereview.chromium.org/2319493002/. Design doc at https://docs.google.com/document/d/1awQoajq_DLmz2AIU9iweC0zEYlVuHCvEIRQepeYyx... BUG=425368 ========== to ========== Add mojo interface+impl for audio stream control. Summary: * AudioOutput is an interface for control messages for a single stream. * The changes in media/mojo/interfaces/, media/base/ipc, and content/common/media/audio_messages.h mojofy OutputDeviceStatus. * content/common/media/audio_output.mojom defines the IPC interface. * AFAIK sync sockets and shared memory is not welcome in media/, so AudioOutputImpl lives in content/. Feel free to tell me if I got this wrong, as the directory hierarchy is unclear to me :). This CL is split off from https://codereview.chromium.org/2319493002/. Design doc at https://docs.google.com/document/d/1awQoajq_DLmz2AIU9iweC0zEYlVuHCvEIRQepeYyx... The "Callback to be deleted on error" is used in many places. Here it is used by storing a pointer to owner: https://cs.chromium.org/chromium/src/device/geolocation/geolocation_service_i..., https://cs.chromium.org/chromium/src/content/browser/permissions/permission_s..., https://cs.chromium.org/chromium/src/content/browser/payments/payment_app_man..., https://cs.chromium.org/chromium/src/content/browser/background_sync/backgrou..., https://cs.chromium.org/chromium/src/content/browser/notifications/blink_noti.... BUG=425368 ==========
Description was changed from ========== Add mojo interface+impl for audio stream control. Summary: * AudioOutput is an interface for control messages for a single stream. * The changes in media/mojo/interfaces/, media/base/ipc, and content/common/media/audio_messages.h mojofy OutputDeviceStatus. * content/common/media/audio_output.mojom defines the IPC interface. * AFAIK sync sockets and shared memory is not welcome in media/, so AudioOutputImpl lives in content/. Feel free to tell me if I got this wrong, as the directory hierarchy is unclear to me :). This CL is split off from https://codereview.chromium.org/2319493002/. Design doc at https://docs.google.com/document/d/1awQoajq_DLmz2AIU9iweC0zEYlVuHCvEIRQepeYyx... The "Callback to be deleted on error" is used in many places. Here it is used by storing a pointer to owner: https://cs.chromium.org/chromium/src/device/geolocation/geolocation_service_i..., https://cs.chromium.org/chromium/src/content/browser/permissions/permission_s..., https://cs.chromium.org/chromium/src/content/browser/payments/payment_app_man..., https://cs.chromium.org/chromium/src/content/browser/background_sync/backgrou..., https://cs.chromium.org/chromium/src/content/browser/notifications/blink_noti.... BUG=425368 ========== to ========== Add mojo interface+impl for audio stream control. Summary: * AudioOutput is an interface for control messages for a single stream. * The changes in media/mojo/interfaces/, media/base/ipc, and content/common/media/audio_messages.h mojofy OutputDeviceStatus. * content/common/media/audio_output.mojom defines the IPC interface. * AFAIK sync sockets and shared memory is not welcome in media/, so AudioOutputImpl lives in content/. Feel free to tell me if I got this wrong, as the directory hierarchy is unclear to me :). This CL is split off from https://codereview.chromium.org/2319493002/. Design doc at https://docs.google.com/document/d/1awQoajq_DLmz2AIU9iweC0zEYlVuHCvEIRQepeYyx... The "Callback to be deleted on error" is used in many places. Here are some cases where it is used by storing a pointer to owner: https://cs.chromium.org/chromium/src/device/geolocation/geolocation_service_i..., https://cs.chromium.org/chromium/src/content/browser/permissions/permission_s..., https://cs.chromium.org/chromium/src/content/browser/payments/payment_app_man..., https://cs.chromium.org/chromium/src/content/browser/background_sync/backgrou..., https://cs.chromium.org/chromium/src/content/browser/notifications/blink_noti.... BUG=425368 ==========
Description was changed from ========== Add mojo interface+impl for audio stream control. Summary: * AudioOutput is an interface for control messages for a single stream. * The changes in media/mojo/interfaces/, media/base/ipc, and content/common/media/audio_messages.h mojofy OutputDeviceStatus. * content/common/media/audio_output.mojom defines the IPC interface. * AFAIK sync sockets and shared memory is not welcome in media/, so AudioOutputImpl lives in content/. Feel free to tell me if I got this wrong, as the directory hierarchy is unclear to me :). This CL is split off from https://codereview.chromium.org/2319493002/. Design doc at https://docs.google.com/document/d/1awQoajq_DLmz2AIU9iweC0zEYlVuHCvEIRQepeYyx... The "Callback to be deleted on error" is used in many places. Here are some cases where it is used by storing a pointer to owner: https://cs.chromium.org/chromium/src/device/geolocation/geolocation_service_i..., https://cs.chromium.org/chromium/src/content/browser/permissions/permission_s..., https://cs.chromium.org/chromium/src/content/browser/payments/payment_app_man..., https://cs.chromium.org/chromium/src/content/browser/background_sync/backgrou..., https://cs.chromium.org/chromium/src/content/browser/notifications/blink_noti.... BUG=425368 ========== to ========== Add mojo interface+impl for audio stream control. Summary: * AudioOutput is an interface for control messages for a single stream. * The changes in media/mojo/interfaces/, media/base/ipc, and content/common/media/audio_messages.h mojofy OutputDeviceStatus. * content/common/media/audio_output.mojom defines the IPC interface. * AFAIK sync sockets and shared memory is not welcome in media/, so AudioOutputImpl lives in content/. Feel free to tell me if I got this wrong, as the directory hierarchy is unclear to me :). * Not hooked up anywhere yet. This CL is split off from https://codereview.chromium.org/2319493002/. Design doc at https://docs.google.com/document/d/1awQoajq_DLmz2AIU9iweC0zEYlVuHCvEIRQepeYyx... The "Callback to be deleted on error" is used in many places. Here are some cases where it is used by storing a pointer to owner: https://cs.chromium.org/chromium/src/device/geolocation/geolocation_service_i..., https://cs.chromium.org/chromium/src/content/browser/permissions/permission_s..., https://cs.chromium.org/chromium/src/content/browser/payments/payment_app_man..., https://cs.chromium.org/chromium/src/content/browser/background_sync/backgrou..., https://cs.chromium.org/chromium/src/content/browser/notifications/blink_noti.... BUG=425368 ==========
Olga: as discussed offline, I added some references to classes which currently use the "callback for deletion on error" style of lifetime management. I didn't find any that take callbacks in the constructor, they take a * to some context/factory object and call a hard-coded method on that. Such a solution is also fine by me, if you prefer it. I also saw classes creating strong bindings and forgetting about them. This is not appropriate here since the audio should be stopped when the render frame is gone. Regarding keeping the AudioOutputs in a StrongBindingSet, it is not possible since they need to be destroyed on error, but they cannot propagate an error to the StrongBindingSet from the inside.
Description was changed from ========== Add mojo interface+impl for audio stream control. Summary: * AudioOutput is an interface for control messages for a single stream. * The changes in media/mojo/interfaces/, media/base/ipc, and content/common/media/audio_messages.h mojofy OutputDeviceStatus. * content/common/media/audio_output.mojom defines the IPC interface. * AFAIK sync sockets and shared memory is not welcome in media/, so AudioOutputImpl lives in content/. Feel free to tell me if I got this wrong, as the directory hierarchy is unclear to me :). * Not hooked up anywhere yet. This CL is split off from https://codereview.chromium.org/2319493002/. Design doc at https://docs.google.com/document/d/1awQoajq_DLmz2AIU9iweC0zEYlVuHCvEIRQepeYyx... The "Callback to be deleted on error" is used in many places. Here are some cases where it is used by storing a pointer to owner: https://cs.chromium.org/chromium/src/device/geolocation/geolocation_service_i..., https://cs.chromium.org/chromium/src/content/browser/permissions/permission_s..., https://cs.chromium.org/chromium/src/content/browser/payments/payment_app_man..., https://cs.chromium.org/chromium/src/content/browser/background_sync/backgrou..., https://cs.chromium.org/chromium/src/content/browser/notifications/blink_noti.... BUG=425368 ========== to ========== Add mojo interface+impl for audio stream control. Summary: * AudioOutput is an interface for control messages for a single stream. * The changes in media/mojo/interfaces/, media/base/ipc, and content/common/media/audio_messages.h mojofy OutputDeviceStatus. * content/common/media/audio_output.mojom defines the IPC interface. * AFAIK sync sockets and shared memory is not welcome in media/, so AudioOutputImpl lives in content/. Feel free to tell me if I got this wrong, as the directory hierarchy is unclear to me :). * Not hooked up anywhere yet. This CL is split off from https://codereview.chromium.org/2319493002/. Design doc at https://docs.google.com/document/d/1awQoajq_DLmz2AIU9iweC0zEYlVuHCvEIRQepeYyx... The "Callback to be deleted on error" is used in many places. Here are some cases where it is used by storing a pointer to owner: https://cs.chromium.org/chromium/src/device/geolocation/geolocation_service_i..., https://cs.chromium.org/chromium/src/content/browser/permissions/permission_s..., https://cs.chromium.org/chromium/src/content/browser/payments/payment_app_man..., https://cs.chromium.org/chromium/src/content/browser/background_sync/backgrou..., https://cs.chromium.org/chromium/src/content/browser/notifications/blink_noti.... BUG=425368 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Patchset #2 (id:20001) 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...
Description was changed from ========== Add mojo interface+impl for audio stream control. Summary: * AudioOutput is an interface for control messages for a single stream. * The changes in media/mojo/interfaces/, media/base/ipc, and content/common/media/audio_messages.h mojofy OutputDeviceStatus. * content/common/media/audio_output.mojom defines the IPC interface. * AFAIK sync sockets and shared memory is not welcome in media/, so AudioOutputImpl lives in content/. Feel free to tell me if I got this wrong, as the directory hierarchy is unclear to me :). * Not hooked up anywhere yet. This CL is split off from https://codereview.chromium.org/2319493002/. Design doc at https://docs.google.com/document/d/1awQoajq_DLmz2AIU9iweC0zEYlVuHCvEIRQepeYyx... The "Callback to be deleted on error" is used in many places. Here are some cases where it is used by storing a pointer to owner: https://cs.chromium.org/chromium/src/device/geolocation/geolocation_service_i..., https://cs.chromium.org/chromium/src/content/browser/permissions/permission_s..., https://cs.chromium.org/chromium/src/content/browser/payments/payment_app_man..., https://cs.chromium.org/chromium/src/content/browser/background_sync/backgrou..., https://cs.chromium.org/chromium/src/content/browser/notifications/blink_noti.... BUG=425368 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Add mojo interface+impl for audio stream control. Summary: * AudioOutput is an interface for control messages for a single stream. * The changes in media/mojo/interfaces/, media/base/ipc, and content/common/media/audio_messages.h mojofy OutputDeviceStatus. * media/mojo/interfaces/audio_output.mojom defines the IPC interface. * Not hooked up anywhere yet. This CL is split off from https://codereview.chromium.org/2319493002/. Design doc at https://docs.google.com/document/d/1awQoajq_DLmz2AIU9iweC0zEYlVuHCvEIRQepeYyx... The "Callback to be deleted on error" is used in many places. Here are some cases where it is used by storing a pointer to owner: https://cs.chromium.org/chromium/src/device/geolocation/geolocation_service_i..., https://cs.chromium.org/chromium/src/content/browser/permissions/permission_s..., https://cs.chromium.org/chromium/src/content/browser/payments/payment_app_man..., https://cs.chromium.org/chromium/src/content/browser/background_sync/backgrou..., https://cs.chromium.org/chromium/src/content/browser/notifications/blink_noti.... BUG=425368 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Description was changed from ========== Add mojo interface+impl for audio stream control. Summary: * AudioOutput is an interface for control messages for a single stream. * The changes in media/mojo/interfaces/, media/base/ipc, and content/common/media/audio_messages.h mojofy OutputDeviceStatus. * media/mojo/interfaces/audio_output.mojom defines the IPC interface. * Not hooked up anywhere yet. This CL is split off from https://codereview.chromium.org/2319493002/. Design doc at https://docs.google.com/document/d/1awQoajq_DLmz2AIU9iweC0zEYlVuHCvEIRQepeYyx... The "Callback to be deleted on error" is used in many places. Here are some cases where it is used by storing a pointer to owner: https://cs.chromium.org/chromium/src/device/geolocation/geolocation_service_i..., https://cs.chromium.org/chromium/src/content/browser/permissions/permission_s..., https://cs.chromium.org/chromium/src/content/browser/payments/payment_app_man..., https://cs.chromium.org/chromium/src/content/browser/background_sync/backgrou..., https://cs.chromium.org/chromium/src/content/browser/notifications/blink_noti.... BUG=425368 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Add mojo interface+impl for audio stream control. Summary: * AudioOutput is an interface for control messages for a single stream. * The changes in media/mojo/interfaces/, media/base/ipc, and content/common/media/audio_messages.h mojofy OutputDeviceStatus. * media/mojo/interfaces/audio_output.mojom defines the IPC interface. * It is not possible to keep the AudioOutputs in a StrongBindingSet since they need to be destroyed on error, but they cannot propagate an error to the StrongBindingSet from the inside. * Not hooked up anywhere yet. This CL is split off from https://codereview.chromium.org/2319493002/. Design doc at https://docs.google.com/document/d/1awQoajq_DLmz2AIU9iweC0zEYlVuHCvEIRQepeYyx... The "Callback to be deleted on error" is used in many places. Here are some cases where it is used by storing a pointer to owner: https://cs.chromium.org/chromium/src/device/geolocation/geolocation_service_i..., https://cs.chromium.org/chromium/src/content/browser/permissions/permission_s..., https://cs.chromium.org/chromium/src/content/browser/payments/payment_app_man..., https://cs.chromium.org/chromium/src/content/browser/background_sync/backgrou..., https://cs.chromium.org/chromium/src/content/browser/notifications/blink_noti.... BUG=425368 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) 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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
On 2017/02/17 12:02:47, Max Morin wrote: > Olga: as discussed offline, I added some references to classes which currently > use the "callback for deletion on error" style of lifetime management. I didn't > find any that take callbacks in the constructor, they take a * to some > context/factory object and call a hard-coded method on that. Such a solution is > also fine by me, if you prefer it. > > I also saw classes creating strong bindings and forgetting about them. This is > not appropriate here since the audio should be stopped when the render frame is > gone. > > Regarding keeping the AudioOutputs in a StrongBindingSet, it is not possible > since they need to be destroyed on error, but they cannot propagate an error to > the StrongBindingSet from the inside. I also removed the pausing of IPC message processing in AudioOutputImpl since it wasn't needed (as discussed offline). PTAL (working on windows compile failure). Also, I can add Dale now if you prefer.
On 2017/02/21 11:05:35, Max Morin wrote: > On 2017/02/17 12:02:47, Max Morin wrote: > > Olga: as discussed offline, I added some references to classes which currently > > use the "callback for deletion on error" style of lifetime management. I > didn't > > find any that take callbacks in the constructor, they take a * to some > > context/factory object and call a hard-coded method on that. Such a solution > is > > also fine by me, if you prefer it. > > > > I also saw classes creating strong bindings and forgetting about them. This is > > not appropriate here since the audio should be stopped when the render frame > is > > gone. > > > > Regarding keeping the AudioOutputs in a StrongBindingSet, it is not possible > > since they need to be destroyed on error, but they cannot propagate an error > to > > the StrongBindingSet from the inside. > > I also removed the pausing of IPC message processing in AudioOutputImpl since it > wasn't needed (as discussed offline). > PTAL (working on windows compile failure). Also, I can add Dale now if you > prefer. Sure, go ahead and add Dale - he has much better understanding of directory structure.
maxmorin@chromium.org changed reviewers: + dalecurtis@chromium.org
Dale: PTAL. Especially, advise on directory structure.
Description was changed from ========== Add mojo interface+impl for audio stream control. Summary: * AudioOutput is an interface for control messages for a single stream. * The changes in media/mojo/interfaces/, media/base/ipc, and content/common/media/audio_messages.h mojofy OutputDeviceStatus. * media/mojo/interfaces/audio_output.mojom defines the IPC interface. * It is not possible to keep the AudioOutputs in a StrongBindingSet since they need to be destroyed on error, but they cannot propagate an error to the StrongBindingSet from the inside. * Not hooked up anywhere yet. This CL is split off from https://codereview.chromium.org/2319493002/. Design doc at https://docs.google.com/document/d/1awQoajq_DLmz2AIU9iweC0zEYlVuHCvEIRQepeYyx... The "Callback to be deleted on error" is used in many places. Here are some cases where it is used by storing a pointer to owner: https://cs.chromium.org/chromium/src/device/geolocation/geolocation_service_i..., https://cs.chromium.org/chromium/src/content/browser/permissions/permission_s..., https://cs.chromium.org/chromium/src/content/browser/payments/payment_app_man..., https://cs.chromium.org/chromium/src/content/browser/background_sync/backgrou..., https://cs.chromium.org/chromium/src/content/browser/notifications/blink_noti.... BUG=425368 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Add mojo interface+impl for audio stream control. Summary: * AudioOutput is an interface for control messages for a single stream. * The changes in media/mojo/interfaces/, media/base/ipc, and content/common/media/audio_messages.h mojofy OutputDeviceStatus. * media/mojo/interfaces/audio_output.mojom defines the IPC interface. * AudioOutputDelegate is moved to media. This also causes some small changes to related files * It is not possible to keep the AudioOutputs in a StrongBindingSet since they need to be destroyed on error, but they cannot propagate an error to the StrongBindingSet from the inside. * Not hooked up anywhere yet. This CL is split off from https://codereview.chromium.org/2319493002/. Design doc at https://docs.google.com/document/d/1awQoajq_DLmz2AIU9iweC0zEYlVuHCvEIRQepeYyx... The "Callback to be deleted on error" is used in many places. Here are some cases where it is used by storing a pointer to owner: https://cs.chromium.org/chromium/src/device/geolocation/geolocation_service_i..., https://cs.chromium.org/chromium/src/content/browser/permissions/permission_s..., https://cs.chromium.org/chromium/src/content/browser/payments/payment_app_man..., https://cs.chromium.org/chromium/src/content/browser/background_sync/backgrou..., https://cs.chromium.org/chromium/src/content/browser/notifications/blink_noti.... BUG=425368 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
This is looking great! Nice work; just a couple Qs which reveal my ignorance I think :p https://codereview.chromium.org/2697793002/diff/80001/media/mojo/interfaces/a... File media/mojo/interfaces/audio_output.mojom (right): https://codereview.chromium.org/2697793002/diff/80001/media/mojo/interfaces/a... media/mojo/interfaces/audio_output.mojom:13: interface AudioOutput { Can you fill these comments out now? I.e., high level class comment, and per method comments describing the parameters and how they're used. Why we have Start,Play,Pause. That said, maybe we should rename Start to Initialize or Create? Just something that doesn't imply the stream will start rendering w/o a play call. https://codereview.chromium.org/2697793002/diff/80001/media/mojo/interfaces/m... File media/mojo/interfaces/media_types.mojom (right): https://codereview.chromium.org/2697793002/diff/80001/media/mojo/interfaces/m... media/mojo/interfaces/media_types.mojom:50: Extra line? https://codereview.chromium.org/2697793002/diff/80001/media/mojo/services/moj... File media/mojo/services/mojo_audio_output.cc (right): https://codereview.chromium.org/2697793002/diff/80001/media/mojo/services/moj... media/mojo/services/mojo_audio_output.cc:42: delegate_ = std::move(create_delegate_callback_).Run(this, params); Hmm, post std::move the contents of create_delegate_callback_ are undefined I thought? I notice this is how the examples in OnceCallback do it, so no problem, just surprising since we now potentially have a member variable in some weird state. https://codereview.chromium.org/2697793002/diff/80001/media/mojo/services/moj... media/mojo/services/mojo_audio_output.cc:103: start_callback_.Run(std::move(buffer_handle), std::move(socket_handle)); base::ResetAndReturn? https://codereview.chromium.org/2697793002/diff/80001/media/mojo/services/moj... media/mojo/services/mojo_audio_output.cc:114: if (!finished_callback_) { Hmm, this is relying on finished_callback_ being null, so my understanding of oncecallback post std::move above must be wrong?
Thanks Dale. https://codereview.chromium.org/2697793002/diff/80001/media/mojo/interfaces/a... File media/mojo/interfaces/audio_output.mojom (right): https://codereview.chromium.org/2697793002/diff/80001/media/mojo/interfaces/a... media/mojo/interfaces/audio_output.mojom:13: interface AudioOutput { On 2017/02/21 18:18:32, DaleCurtis wrote: > Can you fill these comments out now? I.e., high level class comment, and per > method comments describing the parameters and how they're used. Why we have > Start,Play,Pause. > > That said, maybe we should rename Start to Initialize or Create? Just something > that doesn't imply the stream will start rendering w/o a play call. Added comments, changed to Initialize. https://codereview.chromium.org/2697793002/diff/80001/media/mojo/interfaces/m... File media/mojo/interfaces/media_types.mojom (right): https://codereview.chromium.org/2697793002/diff/80001/media/mojo/interfaces/m... media/mojo/interfaces/media_types.mojom:50: On 2017/02/21 18:18:32, DaleCurtis wrote: > Extra line? Done. https://codereview.chromium.org/2697793002/diff/80001/media/mojo/services/moj... File media/mojo/services/mojo_audio_output.cc (right): https://codereview.chromium.org/2697793002/diff/80001/media/mojo/services/moj... media/mojo/services/mojo_audio_output.cc:42: delegate_ = std::move(create_delegate_callback_).Run(this, params); On 2017/02/21 18:18:32, DaleCurtis wrote: > Hmm, post std::move the contents of create_delegate_callback_ are undefined I > thought? I notice this is how the examples in OnceCallback do it, so no problem, > just surprising since we now potentially have a member variable in some weird > state. The C++ standard requires that it is in a "valid but unspecified state" after moving. This is only a minimal requirement, for example we know that std::unique_ptr is null after moving. I think it is implicit that a OnceCallback also become null/empty, though having it explicitly documented would be nice. I filed crbug.com/694945 to ask for clarification. https://codereview.chromium.org/2697793002/diff/80001/media/mojo/services/moj... media/mojo/services/mojo_audio_output.cc:103: start_callback_.Run(std::move(buffer_handle), std::move(socket_handle)); On 2017/02/21 18:18:32, DaleCurtis wrote: > base::ResetAndReturn? Done. https://codereview.chromium.org/2697793002/diff/80001/media/mojo/services/moj... media/mojo/services/mojo_audio_output.cc:114: if (!finished_callback_) { On 2017/02/21 18:18:32, DaleCurtis wrote: > Hmm, this is relying on finished_callback_ being null, so my understanding of > oncecallback post std::move above must be wrong? I'm pretty sure it's null after being moved from, but this check is not necessary anyways (since this object is synchronously destroyed when running |finished_callback_|, OnError cannot be called multiple times). I simplified this method and added a comment to the constructor stating that |this| must be destroyed by |finished_callback_| synchronously.
Patchset #3 (id:100001) has been deleted
Awesome! Just one question and some nits (commented both PSs, since started it yesterday) https://codereview.chromium.org/2697793002/diff/80001/media/base/audio_output... File media/base/audio_output_delegate.h (right): https://codereview.chromium.org/2697793002/diff/80001/media/base/audio_output... media/base/audio_output_delegate.h:5: #ifndef MEDIA_BASE_AUDIO_OUTPUT_DELEGATE_H_ To me media/audio looks like a better place for it. Side by side with audio controllers. https://codereview.chromium.org/2697793002/diff/80001/media/mojo/services/moj... File media/mojo/services/mojo_audio_output.cc (right): https://codereview.chromium.org/2697793002/diff/80001/media/mojo/services/moj... media/mojo/services/mojo_audio_output.cc:38: // Already started. Add logging on error in all places like this? Will help us to triage bugs. https://codereview.chromium.org/2697793002/diff/80001/media/mojo/services/moj... media/mojo/services/mojo_audio_output.cc:42: delegate_ = std::move(create_delegate_callback_).Run(this, params); On 2017/02/22 10:08:43, Max Morin wrote: > On 2017/02/21 18:18:32, DaleCurtis wrote: > > Hmm, post std::move the contents of create_delegate_callback_ are undefined I > > thought? I notice this is how the examples in OnceCallback do it, so no > problem, > > just surprising since we now potentially have a member variable in some weird > > state. > > The C++ standard requires that it is in a "valid but unspecified state" after > moving. This is only a minimal requirement, for example we know that > std::unique_ptr is null after moving. I think it is implicit that a OnceCallback > also become null/empty, though having it explicitly documented would be nice. I > filed crbug.com/694945 to ask for clarification. Here is what I found it unit tests: https://cs.chromium.org/chromium/src/base/bind_unittest.cc?type=cs&q=OnceCall.... So it is null pre implementation, and you are right that it should be documented. https://codereview.chromium.org/2697793002/diff/80001/media/mojo/services/moj... media/mojo/services/mojo_audio_output.cc:70: // Not started yet. ...or volume is out of range https://codereview.chromium.org/2697793002/diff/80001/media/mojo/services/moj... media/mojo/services/mojo_audio_output.cc:114: if (!finished_callback_) { On 2017/02/22 10:08:43, Max Morin wrote: > On 2017/02/21 18:18:32, DaleCurtis wrote: > > Hmm, this is relying on finished_callback_ being null, so my understanding of > > oncecallback post std::move above must be wrong? > > I'm pretty sure it's null after being moved from, but this check is not > necessary anyways (since this object is synchronously destroyed when running > |finished_callback_|, OnError cannot be called multiple times). I simplified > this method and added a comment to the constructor stating that |this| must be > destroyed by |finished_callback_| synchronously. Hmm... |finished_callback_| acts as a deleter for MojoAudioOutput, and will also remove the instance from some owner container. Something like what StrongBindingSet does (https://cs.chromium.org/chromium/src/media/mojo/services/strong_binding_set.h...). Won't Close() call below (https://cs.chromium.org/chromium/src/mojo/public/cpp/bindings/binding.h?type=...) result in disconnecting the pipe? If it will, do we need |finished_callback_|, or we can store a pointer to a strong binding and just close it (not sure, this may be unsafe)? BTW, I would rename the callback into |deleter_| - it's more clear https://codereview.chromium.org/2697793002/diff/80001/media/mojo/services/moj... File media/mojo/services/mojo_audio_output.h (right): https://codereview.chromium.org/2697793002/diff/80001/media/mojo/services/moj... media/mojo/services/mojo_audio_output.h:11: // This class handles IPC for single stream by delegating method calls to its nit "audio output stream" https://codereview.chromium.org/2697793002/diff/120001/media/mojo/interfaces/... File media/mojo/interfaces/audio_output.mojom (right): https://codereview.chromium.org/2697793002/diff/120001/media/mojo/interfaces/... media/mojo/interfaces/audio_output.mojom:16: // is used to write data to the stream as defined in AudioDeviceThread. nit: are used https://codereview.chromium.org/2697793002/diff/120001/media/mojo/interfaces/... media/mojo/interfaces/audio_output.mojom:26: // Sets volume. Volume is in the range [0, 1]. Describe what happens if volume if out of range? https://codereview.chromium.org/2697793002/diff/120001/media/mojo/services/mo... File media/mojo/services/mojo_audio_output.cc (right): https://codereview.chromium.org/2697793002/diff/120001/media/mojo/services/mo... media/mojo/services/mojo_audio_output.cc:45: initialize_callback_ = callback; Add a comment that it will be called when OnStreamCreated is received? (will help reading)
Dale: quick question: can something in media/mojo/services depend on something in media/audio? Looks like DEPS doesn't allow this, so I guess AudioOutputDelegate must be in media/base? https://codereview.chromium.org/2697793002/diff/80001/media/mojo/services/moj... File media/mojo/services/mojo_audio_output.cc (right): https://codereview.chromium.org/2697793002/diff/80001/media/mojo/services/moj... media/mojo/services/mojo_audio_output.cc:42: delegate_ = std::move(create_delegate_callback_).Run(this, params); On 2017/02/22 13:26:30, o1ka wrote: > On 2017/02/22 10:08:43, Max Morin wrote: > > On 2017/02/21 18:18:32, DaleCurtis wrote: > > > Hmm, post std::move the contents of create_delegate_callback_ are undefined > I > > > thought? I notice this is how the examples in OnceCallback do it, so no > > problem, > > > just surprising since we now potentially have a member variable in some > weird > > > state. > > > > The C++ standard requires that it is in a "valid but unspecified state" after > > moving. This is only a minimal requirement, for example we know that > > std::unique_ptr is null after moving. I think it is implicit that a > OnceCallback > > also become null/empty, though having it explicitly documented would be nice. > I > > filed crbug.com/694945 to ask for clarification. > > Here is what I found it unit tests: > https://cs.chromium.org/chromium/src/base/bind_unittest.cc?type=cs&q=OnceCall.... > So it is null pre implementation, and you are right that it should be > documented. The comment actually contradicts the test, since the comment says it's "undefined" but the test prescribes a certain state. :)
I think we just haven't had a need for it yet, you should be fine updating the DEPS to allow media/audio. https://codereview.chromium.org/2697793002/diff/80001/media/mojo/interfaces/a... File media/mojo/interfaces/audio_output.mojom (right): https://codereview.chromium.org/2697793002/diff/80001/media/mojo/interfaces/a... media/mojo/interfaces/audio_output.mojom:13: interface AudioOutput { On 2017/02/22 at 10:08:43, Max Morin wrote: > On 2017/02/21 18:18:32, DaleCurtis wrote: > > Can you fill these comments out now? I.e., high level class comment, and per > > method comments describing the parameters and how they're used. Why we have > > Start,Play,Pause. > > > > That said, maybe we should rename Start to Initialize or Create? Just something > > that doesn't imply the stream will start rendering w/o a play call. > > Added comments, changed to Initialize. After I wrote this, I got this feedback on my own mojo CL: https://codereview.chromium.org/2687583002#msg18 See c#18 if it doesn't link directly there. Essentially mojo folk prefer interfaces w/o an explicit initialize method so you can never do it wrong. They suggest An AudioOutputProvider interface which vends AudioOutputRequest objects.
Patchset #4 (id:140001) has been deleted
Patchset #4 (id:160001) has been deleted
Patchset #4 (id:180001) has been deleted
Added a Provider interface, PTAL. https://codereview.chromium.org/2697793002/diff/80001/media/base/audio_output... File media/base/audio_output_delegate.h (right): https://codereview.chromium.org/2697793002/diff/80001/media/base/audio_output... media/base/audio_output_delegate.h:5: #ifndef MEDIA_BASE_AUDIO_OUTPUT_DELEGATE_H_ On 2017/02/22 13:26:30, o1ka wrote: > To me media/audio looks like a better place for it. Side by side with audio > controllers. Done. https://codereview.chromium.org/2697793002/diff/80001/media/mojo/services/moj... File media/mojo/services/mojo_audio_output.cc (right): https://codereview.chromium.org/2697793002/diff/80001/media/mojo/services/moj... media/mojo/services/mojo_audio_output.cc:38: // Already started. On 2017/02/22 13:26:30, o1ka wrote: > Add logging on error in all places like this? Will help us to triage bugs. Done. https://codereview.chromium.org/2697793002/diff/80001/media/mojo/services/moj... media/mojo/services/mojo_audio_output.cc:114: if (!finished_callback_) { On 2017/02/22 13:26:30, o1ka wrote: > On 2017/02/22 10:08:43, Max Morin wrote: > > On 2017/02/21 18:18:32, DaleCurtis wrote: > > > Hmm, this is relying on finished_callback_ being null, so my understanding > of > > > oncecallback post std::move above must be wrong? > > > > I'm pretty sure it's null after being moved from, but this check is not > > necessary anyways (since this object is synchronously destroyed when running > > |finished_callback_|, OnError cannot be called multiple times). I simplified > > this method and added a comment to the constructor stating that |this| must be > > destroyed by |finished_callback_| synchronously. > > Hmm... |finished_callback_| acts as a deleter for MojoAudioOutput, and will also > remove the instance from some owner container. > Something like what StrongBindingSet does > (https://cs.chromium.org/chromium/src/media/mojo/services/strong_binding_set.h...). > Won't Close() call below > (https://cs.chromium.org/chromium/src/mojo/public/cpp/bindings/binding.h?type=...) > result in disconnecting the pipe? If it will, do we need |finished_callback_|, > or we can store a pointer to a strong binding and just close it (not sure, this > may be unsafe)? > > BTW, I would rename the callback into |deleter_| - it's more clear Renamed to deleter_. https://codereview.chromium.org/2697793002/diff/80001/media/mojo/services/moj... File media/mojo/services/mojo_audio_output.h (right): https://codereview.chromium.org/2697793002/diff/80001/media/mojo/services/moj... media/mojo/services/mojo_audio_output.h:11: // This class handles IPC for single stream by delegating method calls to its On 2017/02/22 13:26:30, o1ka wrote: > nit "audio output stream" Done. https://codereview.chromium.org/2697793002/diff/120001/media/mojo/interfaces/... File media/mojo/interfaces/audio_output.mojom (right): https://codereview.chromium.org/2697793002/diff/120001/media/mojo/interfaces/... media/mojo/interfaces/audio_output.mojom:16: // is used to write data to the stream as defined in AudioDeviceThread. On 2017/02/22 13:26:30, o1ka wrote: > nit: are used Done. https://codereview.chromium.org/2697793002/diff/120001/media/mojo/interfaces/... media/mojo/interfaces/audio_output.mojom:26: // Sets volume. Volume is in the range [0, 1]. On 2017/02/22 13:26:30, o1ka wrote: > Describe what happens if volume if out of range? It's not allowed. https://codereview.chromium.org/2697793002/diff/120001/media/mojo/services/mo... File media/mojo/services/mojo_audio_output.cc (right): https://codereview.chromium.org/2697793002/diff/120001/media/mojo/services/mo... media/mojo/services/mojo_audio_output.cc:45: initialize_callback_ = callback; On 2017/02/22 13:26:31, o1ka wrote: > Add a comment that it will be called when OnStreamCreated is received? (will > help reading) Done.
Dale, Olga: Ping
Demonstrating some of the gaps in my mojo knowledge here :) https://codereview.chromium.org/2697793002/diff/200001/media/mojo/interfaces/... File media/mojo/interfaces/audio_output.mojom (right): https://codereview.chromium.org/2697793002/diff/200001/media/mojo/interfaces/... media/mojo/interfaces/audio_output.mojom:13: interface AudioOutput { I think I prefer some sort of trailing differentiation; a la AudioOutputStream, etc. Don't want to bike-shed too much, but AudiOutput feels too generic given our overwhelming abundance of things with audio_output in the name. https://codereview.chromium.org/2697793002/diff/200001/media/mojo/interfaces/... media/mojo/interfaces/audio_output.mojom:16: // Stops rendering audio and sends a signal to the |socket_descriptor| Add blank lines between comments and methods. https://codereview.chromium.org/2697793002/diff/200001/media/mojo/interfaces/... media/mojo/interfaces/audio_output.mojom:27: Acquire(AudioOutput&? audio_output, media.mojom.AudioParameters params) => Hmm, why this signature versus: Acquire(params) => (AudioOutput, handle, handle) ? https://codereview.chromium.org/2697793002/diff/200001/media/mojo/services/mo... File media/mojo/services/mojo_audio_output.cc (right): https://codereview.chromium.org/2697793002/diff/200001/media/mojo/services/mo... media/mojo/services/mojo_audio_output.cc:49: if (volume < 0 || volume > 1) { Hmm, should this be a BadMessage instead which kills the renderer? I can't remember if we tried this and found some client sending bad values or not though. https://codereview.chromium.org/2697793002/diff/200001/media/mojo/services/mo... media/mojo/services/mojo_audio_output.cc:58: int stream_id, Unused? Still necessary? https://codereview.chromium.org/2697793002/diff/200001/media/mojo/services/mo... File media/mojo/services/mojo_audio_output.h (right): https://codereview.chromium.org/2697793002/diff/200001/media/mojo/services/mo... media/mojo/services/mojo_audio_output.h:11: // This class handles IPC for single audio output stream by delegating method Comments go on the class not between includes. https://codereview.chromium.org/2697793002/diff/200001/media/mojo/services/mo... File media/mojo/services/mojo_audio_output_provider.cc (right): https://codereview.chromium.org/2697793002/diff/200001/media/mojo/services/mo... media/mojo/services/mojo_audio_output_provider.cc:24: void MojoAudioOutputProvider::Acquire(AudioOutputRequest request, Is it possible to setup the provider object to repeatedly vend instances? Or are they 1-shot always? https://codereview.chromium.org/2697793002/diff/200001/media/mojo/services/mo... media/mojo/services/mojo_audio_output_provider.cc:34: base::Bind(deleter_callback, base::Unretained(this))); hmm, is Unretained necessary here? You're binding arguments here.
https://codereview.chromium.org/2697793002/diff/200001/media/mojo/services/mo... File media/mojo/services/mojo_audio_output_provider.cc (right): https://codereview.chromium.org/2697793002/diff/200001/media/mojo/services/mo... media/mojo/services/mojo_audio_output_provider.cc:28: LOG(ERROR) << "Output acquired twice."; Shouldn't |acquire_callback| run here? Or an error should be signaled. Otherwise a client does not get any reply. How common is the pattern like this with a factory interface which cannot be reused? https://codereview.chromium.org/2697793002/diff/200001/media/mojo/services/mo... File media/mojo/services/mojo_audio_output_provider.h (right): https://codereview.chromium.org/2697793002/diff/200001/media/mojo/services/mo... media/mojo/services/mojo_audio_output_provider.h:50: mojo::Binding<AudioOutputProvider*> binding_; How will delete AudioOutputProvider if a connection is closed before Acquire() is called? https://codereview.chromium.org/2697793002/diff/200001/media/mojo/services/mo... media/mojo/services/mojo_audio_output_provider.h:52: base::ThreadChecker thread_checker_; Not used?
Description was changed from ========== Add mojo interface+impl for audio stream control. Summary: * AudioOutput is an interface for control messages for a single stream. * The changes in media/mojo/interfaces/, media/base/ipc, and content/common/media/audio_messages.h mojofy OutputDeviceStatus. * media/mojo/interfaces/audio_output.mojom defines the IPC interface. * AudioOutputDelegate is moved to media. This also causes some small changes to related files * It is not possible to keep the AudioOutputs in a StrongBindingSet since they need to be destroyed on error, but they cannot propagate an error to the StrongBindingSet from the inside. * Not hooked up anywhere yet. This CL is split off from https://codereview.chromium.org/2319493002/. Design doc at https://docs.google.com/document/d/1awQoajq_DLmz2AIU9iweC0zEYlVuHCvEIRQepeYyx... The "Callback to be deleted on error" is used in many places. Here are some cases where it is used by storing a pointer to owner: https://cs.chromium.org/chromium/src/device/geolocation/geolocation_service_i..., https://cs.chromium.org/chromium/src/content/browser/permissions/permission_s..., https://cs.chromium.org/chromium/src/content/browser/payments/payment_app_man..., https://cs.chromium.org/chromium/src/content/browser/background_sync/backgrou..., https://cs.chromium.org/chromium/src/content/browser/notifications/blink_noti.... BUG=425368 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Add mojo interface+impl for audio stream control. Summary: * AudioOutput is an interface for control messages for a single stream. * The changes in media/mojo/interfaces/, media/base/ipc, and content/common/media/audio_messages.h mojofy OutputDeviceStatus. * media/mojo/interfaces/audio_output.mojom defines the IPC interface. * AudioOutputDelegate is moved to media. This also causes some small changes to related files * It is not possible to keep the AudioOutputs in a StrongBindingSet since they need to be destroyed on error, but they cannot propagate an error to the StrongBindingSet from the inside. * Not hooked up anywhere yet. This CL is split off from https://codereview.chromium.org/2319493002/. Design doc at https://docs.google.com/document/d/1awQoajq_DLmz2AIU9iweC0zEYlVuHCvEIRQepeYyx... The "Callback to be deleted on error" is used in many places. Here are some cases where it is used by storing a pointer to owner: https://cs.chromium.org/chromium/src/device/geolocation/geolocation_service_i..., https://cs.chromium.org/chromium/src/content/browser/permissions/permission_s..., https://cs.chromium.org/chromium/src/content/browser/payments/payment_app_man..., https://cs.chromium.org/chromium/src/content/browser/background_sync/backgrou..., https://cs.chromium.org/chromium/src/content/browser/notifications/blink_noti.... BUG=425368 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
https://codereview.chromium.org/2697793002/diff/200001/media/mojo/interfaces/... File media/mojo/interfaces/audio_output.mojom (right): https://codereview.chromium.org/2697793002/diff/200001/media/mojo/interfaces/... media/mojo/interfaces/audio_output.mojom:13: interface AudioOutput { On 2017/03/06 17:56:12, DaleCurtis wrote: > I think I prefer some sort of trailing differentiation; a la AudioOutputStream, > etc. Don't want to bike-shed too much, but AudiOutput feels too generic given > our overwhelming abundance of things with audio_output in the name. Makes sense. Now AudioOutputStream. https://codereview.chromium.org/2697793002/diff/200001/media/mojo/interfaces/... media/mojo/interfaces/audio_output.mojom:16: // Stops rendering audio and sends a signal to the |socket_descriptor| On 2017/03/06 17:56:12, DaleCurtis wrote: > Add blank lines between comments and methods. Done. https://codereview.chromium.org/2697793002/diff/200001/media/mojo/interfaces/... media/mojo/interfaces/audio_output.mojom:27: Acquire(AudioOutput&? audio_output, media.mojom.AudioParameters params) => On 2017/03/06 17:56:12, DaleCurtis wrote: > Hmm, why this signature versus: > > Acquire(params) => (AudioOutput, handle, handle) ? I prefer this one since it allows things like: AudioOutputPtr output; Acquire(mojo::MakeRequest(&output), params, callback); // No need to wait for callback: output->SetVolume(0.5); If we got the outputptr in the callback instead, we'd have to buffer the calls manually while waiting for the callback. https://codereview.chromium.org/2697793002/diff/200001/media/mojo/services/mo... File media/mojo/services/mojo_audio_output.cc (right): https://codereview.chromium.org/2697793002/diff/200001/media/mojo/services/mo... media/mojo/services/mojo_audio_output.cc:49: if (volume < 0 || volume > 1) { On 2017/03/06 17:56:12, DaleCurtis wrote: > Hmm, should this be a BadMessage instead which kills the renderer? I can't > remember if we tried this and found some client sending bad values or not > though. I don't know if we get any bad volumes, I recall bad device_ids (not 64 hex characters). However, we cannot use bad_message from the audio process later, so I'm not sure we gain anything from adding it now. It also adds a dependency on content. https://codereview.chromium.org/2697793002/diff/200001/media/mojo/services/mo... media/mojo/services/mojo_audio_output.cc:58: int stream_id, On 2017/03/06 17:56:12, DaleCurtis wrote: > Unused? Still necessary? It is part of the AudioOutputDelegate::EventHandler interface and AudioRendererHost still needs it. It will be removed when ARH is removed (same for the streamid in OnError). https://codereview.chromium.org/2697793002/diff/200001/media/mojo/services/mo... File media/mojo/services/mojo_audio_output.h (right): https://codereview.chromium.org/2697793002/diff/200001/media/mojo/services/mo... media/mojo/services/mojo_audio_output.h:11: // This class handles IPC for single audio output stream by delegating method On 2017/03/06 17:56:12, DaleCurtis wrote: > Comments go on the class not between includes. Done. https://codereview.chromium.org/2697793002/diff/200001/media/mojo/services/mo... File media/mojo/services/mojo_audio_output_provider.cc (right): https://codereview.chromium.org/2697793002/diff/200001/media/mojo/services/mo... media/mojo/services/mojo_audio_output_provider.cc:24: void MojoAudioOutputProvider::Acquire(AudioOutputRequest request, On 2017/03/06 17:56:12, DaleCurtis wrote: > Is it possible to setup the provider object to repeatedly vend instances? Or are > they 1-shot always? I don't think we want to skip the authorization check. I also don't think we'd make the renderer-side use a provider multiple times. I'll CC guido for implications the spec has on this. https://codereview.chromium.org/2697793002/diff/200001/media/mojo/services/mo... media/mojo/services/mojo_audio_output_provider.cc:28: LOG(ERROR) << "Output acquired twice."; On 2017/03/07 00:49:24, o1ka wrote: > Shouldn't |acquire_callback| run here? Or an error should be signaled. Otherwise > a client does not get any reply. Right, I changed this to unbinding the binder and calling the deleter. > How common is the pattern like this with a factory interface which cannot be > reused? Not common I think? https://codereview.chromium.org/2697793002/diff/200001/media/mojo/services/mo... media/mojo/services/mojo_audio_output_provider.cc:34: base::Bind(deleter_callback, base::Unretained(this))); On 2017/03/06 17:56:12, DaleCurtis wrote: > hmm, is Unretained necessary here? You're binding arguments here. It's actually not. I removed it. https://codereview.chromium.org/2697793002/diff/200001/media/mojo/services/mo... File media/mojo/services/mojo_audio_output_provider.h (right): https://codereview.chromium.org/2697793002/diff/200001/media/mojo/services/mo... media/mojo/services/mojo_audio_output_provider.h:50: mojo::Binding<AudioOutputProvider*> binding_; On 2017/03/07 00:49:24, o1ka wrote: > How will delete AudioOutputProvider if a connection is closed before Acquire() > is called? I fixed this (added connection error handler to binding). https://codereview.chromium.org/2697793002/diff/200001/media/mojo/services/mo... media/mojo/services/mojo_audio_output_provider.h:52: base::ThreadChecker thread_checker_; On 2017/03/07 00:49:24, o1ka wrote: > Not used? Done.
+CC Guido: Hello. I'm currently replacing AudioRendererHost. The replacement will return an AudioOutputStreamProvider after the permissions check. This AudioOutputStreamProvider can be used to get the actual stream. I'm wondering if it would ever make sense to generate several streams using an AudioOutputStreamProvider, only doing a single permissions check? Reading the spec, I got the impression that permissions can be revoked after being granted, so we need to re-check permissions if we want to open another stream. Is this correct?
Description was changed from ========== Add mojo interface+impl for audio stream control. Summary: * AudioOutput is an interface for control messages for a single stream. * The changes in media/mojo/interfaces/, media/base/ipc, and content/common/media/audio_messages.h mojofy OutputDeviceStatus. * media/mojo/interfaces/audio_output.mojom defines the IPC interface. * AudioOutputDelegate is moved to media. This also causes some small changes to related files * It is not possible to keep the AudioOutputs in a StrongBindingSet since they need to be destroyed on error, but they cannot propagate an error to the StrongBindingSet from the inside. * Not hooked up anywhere yet. This CL is split off from https://codereview.chromium.org/2319493002/. Design doc at https://docs.google.com/document/d/1awQoajq_DLmz2AIU9iweC0zEYlVuHCvEIRQepeYyx... The "Callback to be deleted on error" is used in many places. Here are some cases where it is used by storing a pointer to owner: https://cs.chromium.org/chromium/src/device/geolocation/geolocation_service_i..., https://cs.chromium.org/chromium/src/content/browser/permissions/permission_s..., https://cs.chromium.org/chromium/src/content/browser/payments/payment_app_man..., https://cs.chromium.org/chromium/src/content/browser/background_sync/backgrou..., https://cs.chromium.org/chromium/src/content/browser/notifications/blink_noti.... BUG=425368 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Add mojo interface+impl for audio stream control. Summary: * AudioOutputStream is an interface for control messages for a single stream, and AudioOutputStreamProvider is a single-use factory for AudioOutputStreams. * The changes in media/mojo/interfaces/, media/base/ipc, and content/common/media/audio_messages.h mojofy OutputDeviceStatus. * media/mojo/interfaces/audio_output.mojom defines the IPC interface. * AudioOutputDelegate is moved to media. This also causes some small changes to related files * It is not possible to keep the AudioOutputStreamProviders in a StrongBindingSet since they need to be destroyed on error, but they cannot propagate an error to the StrongBindingSet from the inside. * Not hooked up anywhere yet. This CL is split off from https://codereview.chromium.org/2319493002/. Design doc at https://docs.google.com/document/d/1awQoajq_DLmz2AIU9iweC0zEYlVuHCvEIRQepeYyx... The "Callback to be deleted on error" is used in many places. Here are some cases where it is used by storing a pointer to owner: https://cs.chromium.org/chromium/src/device/geolocation/geolocation_service_i..., https://cs.chromium.org/chromium/src/content/browser/permissions/permission_s..., https://cs.chromium.org/chromium/src/content/browser/payments/payment_app_man..., https://cs.chromium.org/chromium/src/content/browser/background_sync/backgrou..., https://cs.chromium.org/chromium/src/content/browser/notifications/blink_noti.... BUG=425368 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
https://codereview.chromium.org/2697793002/diff/200001/media/mojo/interfaces/... File media/mojo/interfaces/audio_output.mojom (right): https://codereview.chromium.org/2697793002/diff/200001/media/mojo/interfaces/... media/mojo/interfaces/audio_output.mojom:13: interface AudioOutput { On 2017/03/07 11:23:16, Max Morin wrote: > On 2017/03/06 17:56:12, DaleCurtis wrote: > > I think I prefer some sort of trailing differentiation; a la > AudioOutputStream, > > etc. Don't want to bike-shed too much, but AudiOutput feels too generic given > > our overwhelming abundance of things with audio_output in the name. > > Makes sense. Now AudioOutputStream. Don't we have one AudioOutputStream already?
On 2017/03/07 13:04:53, o1ka wrote: > https://codereview.chromium.org/2697793002/diff/200001/media/mojo/interfaces/... > File media/mojo/interfaces/audio_output.mojom (right): > > https://codereview.chromium.org/2697793002/diff/200001/media/mojo/interfaces/... > media/mojo/interfaces/audio_output.mojom:13: interface AudioOutput { > On 2017/03/07 11:23:16, Max Morin wrote: > > On 2017/03/06 17:56:12, DaleCurtis wrote: > > > I think I prefer some sort of trailing differentiation; a la > > AudioOutputStream, > > > etc. Don't want to bike-shed too much, but AudiOutput feels too generic > given > > > our overwhelming abundance of things with audio_output in the name. > > > > Makes sense. Now AudioOutputStream. > > Don't we have one AudioOutputStream already? This is mojom::AudioOutputStream, so I think it's ok. We could call it MojoAudioOutputStream like the implementation, but then we'd have mojom::MojoAudioOutputStream which is repetitive.
Ok, makes sense. On Mar 7, 2017 2:09 PM, <maxmorin@chromium.org> wrote: > On 2017/03/07 13:04:53, o1ka wrote: > > > https://codereview.chromium.org/2697793002/diff/200001/ > media/mojo/interfaces/audio_output.mojom > > File media/mojo/interfaces/audio_output.mojom (right): > > > > > https://codereview.chromium.org/2697793002/diff/200001/ > media/mojo/interfaces/audio_output.mojom#newcode13 > > media/mojo/interfaces/audio_output.mojom:13: interface AudioOutput { > > On 2017/03/07 11:23:16, Max Morin wrote: > > > On 2017/03/06 17:56:12, DaleCurtis wrote: > > > > I think I prefer some sort of trailing differentiation; a la > > > AudioOutputStream, > > > > etc. Don't want to bike-shed too much, but AudiOutput feels too > generic > > given > > > > our overwhelming abundance of things with audio_output in the name. > > > > > > Makes sense. Now AudioOutputStream. > > > > Don't we have one AudioOutputStream already? > > This is mojom::AudioOutputStream, so I think it's ok. We could call it > MojoAudioOutputStream like the implementation, but then we'd have > mojom::MojoAudioOutputStream which is repetitive. > > https://codereview.chromium.org/2697793002/ > -- 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.
lgtm - you'll still need a security reviewer and it might be good to add someone from mojo to take a glance over the design.
Awesome! lgtm https://codereview.chromium.org/2697793002/diff/200001/media/mojo/services/mo... File media/mojo/services/mojo_audio_output_provider.cc (right): https://codereview.chromium.org/2697793002/diff/200001/media/mojo/services/mo... media/mojo/services/mojo_audio_output_provider.cc:28: LOG(ERROR) << "Output acquired twice."; On 2017/03/07 11:23:16, Max Morin wrote: > On 2017/03/07 00:49:24, o1ka wrote: > > Shouldn't |acquire_callback| run here? Or an error should be signaled. > Otherwise > > a client does not get any reply. > Right, I changed this to unbinding the binder and calling the deleter. > > > How common is the pattern like this with a factory interface which cannot be > > reused? > > Not common I think? I mean - have you seen examples in mojo services implementation? It looks kind of odd and I do not really like it when shooting in the foot is so easy. But with your last change it will crush quickly and it's simple - that's good. https://codereview.chromium.org/2697793002/diff/220001/media/mojo/interfaces/... File media/mojo/interfaces/audio_output_stream.mojom (right): https://codereview.chromium.org/2697793002/diff/220001/media/mojo/interfaces/... media/mojo/interfaces/audio_output_stream.mojom:26: // Creates a new AudioOutputStream using |params|. |shared_buffer| and |socket_descriptor| are used to write data to the stream as defined in AudioDeviceThread. Can only be used for one output stream. "for one output stream" is not quite clear, as if I can pass the same |output_stream| into it multiple times. "Can only be used once"? and add "On error, the message pipe is closed."? https://codereview.chromium.org/2697793002/diff/220001/media/mojo/services/mo... File media/mojo/services/mojo_audio_output_stream_provider.cc (right): https://codereview.chromium.org/2697793002/diff/220001/media/mojo/services/mo... media/mojo/services/mojo_audio_output_stream_provider.cc:36: deleter_callback_.Run(); Add a comment that it's deleting |this|? (so that nobody in the future would try to add anything after that line)
Thank you! https://codereview.chromium.org/2697793002/diff/200001/media/mojo/services/mo... File media/mojo/services/mojo_audio_output_provider.cc (right): https://codereview.chromium.org/2697793002/diff/200001/media/mojo/services/mo... media/mojo/services/mojo_audio_output_provider.cc:28: LOG(ERROR) << "Output acquired twice."; On 2017/03/07 23:31:06, o1ka wrote: > On 2017/03/07 11:23:16, Max Morin wrote: > > On 2017/03/07 00:49:24, o1ka wrote: > > > Shouldn't |acquire_callback| run here? Or an error should be signaled. > > Otherwise > > > a client does not get any reply. > > Right, I changed this to unbinding the binder and calling the deleter. > > > > > How common is the pattern like this with a factory interface which cannot be > > > reused? > > > > Not common I think? > > I mean - have you seen examples in mojo services implementation? It looks kind > of odd and I do not really like it when shooting in the foot is so easy. But > with your last change it will crush quickly and it's simple - that's good. From what I gather from the spec (though I am no expert), I don't think it ever makes sense to reuse a permissions check. Because of this, I don't want to allow it. I agree it's ugly though. https://codereview.chromium.org/2697793002/diff/220001/media/mojo/interfaces/... File media/mojo/interfaces/audio_output_stream.mojom (right): https://codereview.chromium.org/2697793002/diff/220001/media/mojo/interfaces/... media/mojo/interfaces/audio_output_stream.mojom:26: // Creates a new AudioOutputStream using |params|. |shared_buffer| and |socket_descriptor| are used to write data to the stream as defined in AudioDeviceThread. Can only be used for one output stream. On 2017/03/07 23:31:07, o1ka wrote: > "for one output stream" is not quite clear, as if I can pass the same > |output_stream| into it multiple times. > "Can only be used once"? Done. > and add "On error, the message pipe is closed."? There aren't really any error that can occur (other than shutdown/removing frame, in which case I think it's implicit that the message pipe is closed). https://codereview.chromium.org/2697793002/diff/220001/media/mojo/services/mo... File media/mojo/services/mojo_audio_output_stream_provider.cc (right): https://codereview.chromium.org/2697793002/diff/220001/media/mojo/services/mo... media/mojo/services/mojo_audio_output_stream_provider.cc:36: deleter_callback_.Run(); On 2017/03/07 23:31:07, o1ka wrote: > Add a comment that it's deleting |this|? (so that nobody in the future would try > to add anything after that line) Done.
maxmorin@chromium.org changed reviewers: + mkwst@chromium.org, rockot@chromium.org
mkwst: PTAL at media/mojo/interfaces, media/base/ipc/media_param_traits_macros.h, and content/common/media/audio_messages.h. rockot: As mojo expert, PTAL at media/mojo/interfaces/audio_output_stream.mojom and implementations in media/mojo/services.
https://codereview.chromium.org/2697793002/diff/220001/media/mojo/interfaces/... File media/mojo/interfaces/audio_output_stream.mojom (right): https://codereview.chromium.org/2697793002/diff/220001/media/mojo/interfaces/... media/mojo/interfaces/audio_output_stream.mojom:26: // Creates a new AudioOutputStream using |params|. |shared_buffer| and |socket_descriptor| are used to write data to the stream as defined in AudioDeviceThread. Can only be used for one output stream. On 2017/03/08 07:38:29, Max Morin wrote: > On 2017/03/07 23:31:07, o1ka wrote: > > "for one output stream" is not quite clear, as if I can pass the same > > |output_stream| into it multiple times. > > "Can only be used once"? > > Done. > > > and add "On error, the message pipe is closed."? > > There aren't really any error that can occur (other than shutdown/removing > frame, in which case I think it's implicit that the message pipe is closed). Calling Acquire() twice?
On 2017/03/08 08:11:49, o1ka wrote: > https://codereview.chromium.org/2697793002/diff/220001/media/mojo/interfaces/... > File media/mojo/interfaces/audio_output_stream.mojom (right): > > https://codereview.chromium.org/2697793002/diff/220001/media/mojo/interfaces/... > media/mojo/interfaces/audio_output_stream.mojom:26: // Creates a new > AudioOutputStream using |params|. |shared_buffer| and |socket_descriptor| are > used to write data to the stream as defined in AudioDeviceThread. Can only be > used for one output stream. > On 2017/03/08 07:38:29, Max Morin wrote: > > On 2017/03/07 23:31:07, o1ka wrote: > > > "for one output stream" is not quite clear, as if I can pass the same > > > |output_stream| into it multiple times. > > > "Can only be used once"? > > > > Done. > > > > > and add "On error, the message pipe is closed."? > > > > There aren't really any error that can occur (other than shutdown/removing > > frame, in which case I think it's implicit that the message pipe is closed). > > Calling Acquire() twice? This violates the preconditions of the method, it doesn't make sense to specify a behavior for it.
maxmorin@chromium.org changed reviewers: + clamy@chromium.org
Clamy: PTAL at content/browser/BUILD.gn.
On 2017/03/08 08:14:11, Max Morin wrote: > On 2017/03/08 08:11:49, o1ka wrote: > > > https://codereview.chromium.org/2697793002/diff/220001/media/mojo/interfaces/... > > File media/mojo/interfaces/audio_output_stream.mojom (right): > > > > > https://codereview.chromium.org/2697793002/diff/220001/media/mojo/interfaces/... > > media/mojo/interfaces/audio_output_stream.mojom:26: // Creates a new > > AudioOutputStream using |params|. |shared_buffer| and |socket_descriptor| are > > used to write data to the stream as defined in AudioDeviceThread. Can only be > > used for one output stream. > > On 2017/03/08 07:38:29, Max Morin wrote: > > > On 2017/03/07 23:31:07, o1ka wrote: > > > > "for one output stream" is not quite clear, as if I can pass the same > > > > |output_stream| into it multiple times. > > > > "Can only be used once"? > > > > > > Done. > > > > > > > and add "On error, the message pipe is closed."? > > > > > > There aren't really any error that can occur (other than shutdown/removing > > > frame, in which case I think it's implicit that the message pipe is closed). > > > > Calling Acquire() twice? > > This violates the preconditions of the method, it doesn't make sense to specify > a behavior for it. If I am a user of this interface, I need to verify my code uses it correctly. I need to DCHECK(IsAquired()) or need to know that it will crush or the connection will be closed. Unspecified behavior does not help.
On 2017/03/08 08:28:15, o1ka wrote: > On 2017/03/08 08:14:11, Max Morin wrote: > > On 2017/03/08 08:11:49, o1ka wrote: > > > > > > https://codereview.chromium.org/2697793002/diff/220001/media/mojo/interfaces/... > > > File media/mojo/interfaces/audio_output_stream.mojom (right): > > > > > > > > > https://codereview.chromium.org/2697793002/diff/220001/media/mojo/interfaces/... > > > media/mojo/interfaces/audio_output_stream.mojom:26: // Creates a new > > > AudioOutputStream using |params|. |shared_buffer| and |socket_descriptor| > are > > > used to write data to the stream as defined in AudioDeviceThread. Can only > be > > > used for one output stream. > > > On 2017/03/08 07:38:29, Max Morin wrote: > > > > On 2017/03/07 23:31:07, o1ka wrote: > > > > > "for one output stream" is not quite clear, as if I can pass the same > > > > > |output_stream| into it multiple times. > > > > > "Can only be used once"? > > > > > > > > Done. > > > > > > > > > and add "On error, the message pipe is closed."? > > > > > > > > There aren't really any error that can occur (other than shutdown/removing > > > > frame, in which case I think it's implicit that the message pipe is > closed). > > > > > > Calling Acquire() twice? > > > > This violates the preconditions of the method, it doesn't make sense to > specify > > a behavior for it. > > If I am a user of this interface, I need to verify my code uses it correctly. I > need to DCHECK(IsAquired()) or need to know that it will crush or the connection > will be closed. Unspecified behavior does not help. I prefer if the interface only provides the functionality needed (create a stream) and not functionality not needed (an elaborate way for the client to close the connection). As it stands, clients are not allowed to expect some certain behavior if they fail to meet the preconditions, and that's a good thing! This allows us to be more flexible in the future. For example, if want to use bad_message to handle violated preconditions, we are free to do so and don't need to go and update clients that depend on being able to call Acquire twice.
mojo bits lgtm https://codereview.chromium.org/2697793002/diff/240001/media/mojo/interfaces/... File media/mojo/interfaces/audio_output_stream.mojom (right): https://codereview.chromium.org/2697793002/diff/240001/media/mojo/interfaces/... media/mojo/interfaces/audio_output_stream.mojom:26: // Creates a new AudioOutputStream using |params|. |shared_buffer| and |socket_descriptor| are used to write data to the stream as defined in AudioDeviceThread. Can only be called once. Formatting is all weird here. Sorry we don't have presubmit / autoformatting. Could you please fix this to 80 columns etc? Typically a method is wrapped like: Acquire(AudioOutputStream& output_stream, AudioParameters params) => (...); Also note that qualifying AudioParameters is unnecessary as you're already inside the media.mojom module.
Thank you Ken. https://codereview.chromium.org/2697793002/diff/240001/media/mojo/interfaces/... File media/mojo/interfaces/audio_output_stream.mojom (right): https://codereview.chromium.org/2697793002/diff/240001/media/mojo/interfaces/... media/mojo/interfaces/audio_output_stream.mojom:26: // Creates a new AudioOutputStream using |params|. |shared_buffer| and |socket_descriptor| are used to write data to the stream as defined in AudioDeviceThread. Can only be called once. On 2017/03/08 17:43:37, Ken Rockot wrote: > Formatting is all weird here. Sorry we don't have presubmit / autoformatting. > Could you please fix this to 80 columns etc? Typically a method is wrapped like: > > Acquire(AudioOutputStream& output_stream, AudioParameters params) > => (...); > > Also note that qualifying AudioParameters is unnecessary as you're already > inside the media.mojom module. Oops, I figured "git cl format" would fix this. :) Fixed now.
Thanks! content/ lgtm.
Mike West: Ping.
LGTM.
On 2017/03/07 11:39:38, Max Morin wrote: > +CC Guido: Hello. I'm currently replacing AudioRendererHost. The replacement > will return an AudioOutputStreamProvider after the permissions check. This > AudioOutputStreamProvider can be used to get the actual stream. I'm wondering if > it would ever make sense to generate several streams using an > AudioOutputStreamProvider, only doing a single permissions check? Reading the > spec, I got the impression that permissions can be revoked after being granted, > so we need to re-check permissions if we want to open another stream. Is this > correct? Sorry for the late reply. Yes, this is correct.
The CQ bit was checked by maxmorin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org, olka@chromium.org, rockot@chromium.org Link to the patchset: https://codereview.chromium.org/2697793002/#ps260001 (title: "format mojom")
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_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
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, clamy@chromium.org, mkwst@chromium.org, dalecurtis@chromium.org, olka@chromium.org Link to the patchset: https://codereview.chromium.org/2697793002/#ps280001 (title: "Fix linking")
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_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
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, clamy@chromium.org, mkwst@chromium.org, dalecurtis@chromium.org, olka@chromium.org Link to the patchset: https://codereview.chromium.org/2697793002/#ps300001 (title: "Fix linking once and for all.")
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_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
Patchset #11 (id:340001) has been deleted
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, clamy@chromium.org, mkwst@chromium.org, dalecurtis@chromium.org, olka@chromium.org Link to the patchset: https://codereview.chromium.org/2697793002/#ps360001 (title: "Don't inline AudioOutputDelegate(EventHandler) dtor.")
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": 1489513135447760,
"parent_rev": "0d90112e49ee6da473a129121b06199d86abd920", "commit_rev":
"aa3e611c9d8b00201fab5d36af2758b618e94b05"}
Message was sent while issue was closed.
Description was changed from ========== Add mojo interface+impl for audio stream control. Summary: * AudioOutputStream is an interface for control messages for a single stream, and AudioOutputStreamProvider is a single-use factory for AudioOutputStreams. * The changes in media/mojo/interfaces/, media/base/ipc, and content/common/media/audio_messages.h mojofy OutputDeviceStatus. * media/mojo/interfaces/audio_output.mojom defines the IPC interface. * AudioOutputDelegate is moved to media. This also causes some small changes to related files * It is not possible to keep the AudioOutputStreamProviders in a StrongBindingSet since they need to be destroyed on error, but they cannot propagate an error to the StrongBindingSet from the inside. * Not hooked up anywhere yet. This CL is split off from https://codereview.chromium.org/2319493002/. Design doc at https://docs.google.com/document/d/1awQoajq_DLmz2AIU9iweC0zEYlVuHCvEIRQepeYyx... The "Callback to be deleted on error" is used in many places. Here are some cases where it is used by storing a pointer to owner: https://cs.chromium.org/chromium/src/device/geolocation/geolocation_service_i..., https://cs.chromium.org/chromium/src/content/browser/permissions/permission_s..., https://cs.chromium.org/chromium/src/content/browser/payments/payment_app_man..., https://cs.chromium.org/chromium/src/content/browser/background_sync/backgrou..., https://cs.chromium.org/chromium/src/content/browser/notifications/blink_noti.... BUG=425368 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Add mojo interface+impl for audio stream control. Summary: * AudioOutputStream is an interface for control messages for a single stream, and AudioOutputStreamProvider is a single-use factory for AudioOutputStreams. * The changes in media/mojo/interfaces/, media/base/ipc, and content/common/media/audio_messages.h mojofy OutputDeviceStatus. * media/mojo/interfaces/audio_output.mojom defines the IPC interface. * AudioOutputDelegate is moved to media. This also causes some small changes to related files * It is not possible to keep the AudioOutputStreamProviders in a StrongBindingSet since they need to be destroyed on error, but they cannot propagate an error to the StrongBindingSet from the inside. * Not hooked up anywhere yet. This CL is split off from https://codereview.chromium.org/2319493002/. Design doc at https://docs.google.com/document/d/1awQoajq_DLmz2AIU9iweC0zEYlVuHCvEIRQepeYyx... The "Callback to be deleted on error" is used in many places. Here are some cases where it is used by storing a pointer to owner: https://cs.chromium.org/chromium/src/device/geolocation/geolocation_service_i..., https://cs.chromium.org/chromium/src/content/browser/permissions/permission_s..., https://cs.chromium.org/chromium/src/content/browser/payments/payment_app_man..., https://cs.chromium.org/chromium/src/content/browser/background_sync/backgrou..., https://cs.chromium.org/chromium/src/content/browser/notifications/blink_noti.... BUG=425368 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2697793002 Cr-Commit-Position: refs/heads/master@{#456795} Committed: https://chromium.googlesource.com/chromium/src/+/aa3e611c9d8b00201fab5d36af27... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:360001) as https://chromium.googlesource.com/chromium/src/+/aa3e611c9d8b00201fab5d36af27...
Message was sent while issue was closed.
Thanks everyone for the review, and also thank you Guido for confirming. |
