Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2330393002:
Change sync primitives ownership for audio rendering. (Closed)
|
Created:
4 years, 3 months ago by Max Morin Modified:
4 years, 2 months ago CC:
chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, miu+watch_chromium.org, posciak+watch_chromium.org, audio-mojo-cl_google.com Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionChange sync primitives ownership for audio rendering. See blue (new code) and magenta (old code) in https://docs.google.com/drawings/d/1QDNHnCENITeH7RFnrokOls1Bbm8ydvTJie1lx7Q7V2I/edit.
Also fixed the lint warnings.
BUG=425368
Committed: https://crrev.com/b2230c4434368728ecd7a5888754204ed451d986
Cr-Commit-Position: refs/heads/master@{#419427}
Patch Set 1 #Patch Set 2 : Fix. #Patch Set 3 : Refactor. Fix unit test. #Patch Set 4 : New design. #Patch Set 5 : Small fix. #Patch Set 6 : Fixes. #Patch Set 7 : Remove incorrect comment. #
Total comments: 18
Patch Set 8 : Comments. #
Total comments: 8
Patch Set 9 : Comments #
Messages
Total messages: 22 (11 generated)
Description was changed from ========== Change sync primitives ownership for audio rendering. BUG=425368 ========== to ========== Change sync primitives ownership for audio rendering. See blue (new code) and magenta (old code) in https://docs.google.com/drawings/d/1QDNHnCENITeH7RFnrokOls1Bbm8ydvTJie1lx7Q7V.... Some logic regarding the implementation of the audio stream IPC was also moved from AudioRendererHost to AudioEntry. BUG=425368 ==========
maxmorin@chromium.org changed reviewers: + grunell@chromium.org, olka@chromium.org
Description was changed from ========== Change sync primitives ownership for audio rendering. See blue (new code) and magenta (old code) in https://docs.google.com/drawings/d/1QDNHnCENITeH7RFnrokOls1Bbm8ydvTJie1lx7Q7V.... Some logic regarding the implementation of the audio stream IPC was also moved from AudioRendererHost to AudioEntry. BUG=425368 ========== to ========== Change sync primitives ownership for audio rendering. See blue (new code) and magenta (old code) in https://docs.google.com/drawings/d/1QDNHnCENITeH7RFnrokOls1Bbm8ydvTJie1lx7Q7V.... Some logic regarding the implementation of the audio stream IPC was also moved from AudioRendererHost to AudioEntry. Also fixed the lint warnings. BUG=425368 ==========
maxmorin@chromium.org changed reviewers: - grunell@chromium.org
Olga: PTAL.
How about this?
Nice! https://codereview.chromium.org/2330393002/diff/120001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host.cc (left): https://codereview.chromium.org/2330393002/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:330: // mapping shared memory and sharing with the renderer process. Could you transit all the comments to the new code? https://codereview.chromium.org/2330393002/diff/120001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/2330393002/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:226: new AudioEntry(host, stream_id, render_frame_id, params, output_device_id, (you can use base::WrapUnique instead as well https://cs.chromium.org/chromium/src/base/memory/ptr_util.h?type=cs&sq=packag...) https://codereview.chromium.org/2330393002/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:348: base::SharedMemoryHandle memory_handle; nit: I think foreign_memory_handle was more descriptive https://codereview.chromium.org/2330393002/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:360: static_cast<uint32_t>(shared_memory_size))); checked_cast? (https://cs.chromium.org/chromium/src/base/numerics/safe_conversions.h?l=48) also see https://www.chromium.org/Home/chromium-security/education/security-tips-for-ipc https://codereview.chromium.org/2330393002/diff/120001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host_unittest.cc (right): https://codereview.chromium.org/2330393002/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host_unittest.cc:113: // leading to double-close errors from SyncSocket. Can you file a bug and refer it here? https://codereview.chromium.org/2330393002/diff/120001/content/browser/render... File content/browser/renderer_host/media/audio_sync_reader.cc (right): https://codereview.chromium.org/2330393002/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_sync_reader.cc:126: size_t memory_size = sizeof(media::AudioOutputBufferParameters) + const https://codereview.chromium.org/2330393002/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_sync_reader.cc:127: AudioBus::CalculateMemorySize(params); Since this is the value to be sent over IPC in the end, we may probably want to use CheckedNumeric here? https://cs.chromium.org/chromium/src/base/numerics/safe_math.h?l=50 https://codereview.chromium.org/2330393002/diff/120001/content/browser/render... File content/browser/renderer_host/media/audio_sync_reader.h (right): https://codereview.chromium.org/2330393002/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_sync_reader.h:43: base::SharedMemory* shared_memory() { return shared_memory_.get(); } const method https://codereview.chromium.org/2330393002/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_sync_reader.h:44: base::CancelableSyncSocket* foreign_socket() { return foreign_socket_.get(); } const method
Olga: PTAL https://codereview.chromium.org/2330393002/diff/120001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host.cc (left): https://codereview.chromium.org/2330393002/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:330: // mapping shared memory and sharing with the renderer process. On 2016/09/16 10:51:59, o1ka wrote: > Could you transit all the comments to the new code? Done. https://codereview.chromium.org/2330393002/diff/120001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/2330393002/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:226: new AudioEntry(host, stream_id, render_frame_id, params, output_device_id, On 2016/09/16 10:51:59, o1ka wrote: > (you can use base::WrapUnique instead as well > https://cs.chromium.org/chromium/src/base/memory/ptr_util.h?type=cs&sq=packag...) Done. https://codereview.chromium.org/2330393002/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:348: base::SharedMemoryHandle memory_handle; On 2016/09/16 10:51:59, o1ka wrote: > nit: I think foreign_memory_handle was more descriptive Done. https://codereview.chromium.org/2330393002/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:360: static_cast<uint32_t>(shared_memory_size))); On 2016/09/16 10:51:59, o1ka wrote: > checked_cast? > (https://cs.chromium.org/chromium/src/base/numerics/safe_conversions.h?l=48) > also see > https://www.chromium.org/Home/chromium-security/education/security-tips-for-ipc Done. https://codereview.chromium.org/2330393002/diff/120001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host_unittest.cc (right): https://codereview.chromium.org/2330393002/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host_unittest.cc:113: // leading to double-close errors from SyncSocket. On 2016/09/16 10:51:59, o1ka wrote: > Can you file a bug and refer it here? Done. https://codereview.chromium.org/2330393002/diff/120001/content/browser/render... File content/browser/renderer_host/media/audio_sync_reader.cc (right): https://codereview.chromium.org/2330393002/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_sync_reader.cc:126: size_t memory_size = sizeof(media::AudioOutputBufferParameters) + On 2016/09/16 10:51:59, o1ka wrote: > const Done. https://codereview.chromium.org/2330393002/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_sync_reader.cc:127: AudioBus::CalculateMemorySize(params); On 2016/09/16 10:51:59, o1ka wrote: > Since this is the value to be sent over IPC in the end, we may probably want to > use CheckedNumeric here? > https://cs.chromium.org/chromium/src/base/numerics/safe_math.h?l=50 Yes. Thinking about it, I'm not sure there's anything stopping the renderer from asking for a month-long audio buffer, but there is probably something in the IPC system that checks this. I'll add a note to myself to also fix this for mojo. Fixed. https://codereview.chromium.org/2330393002/diff/120001/content/browser/render... File content/browser/renderer_host/media/audio_sync_reader.h (right): https://codereview.chromium.org/2330393002/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_sync_reader.h:43: base::SharedMemory* shared_memory() { return shared_memory_.get(); } On 2016/09/16 10:51:59, o1ka wrote: > const method Done. https://codereview.chromium.org/2330393002/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_sync_reader.h:44: base::CancelableSyncSocket* foreign_socket() { return foreign_socket_.get(); } On 2016/09/16 10:51:59, o1ka wrote: > const method Done.
lgtm
Description was changed from ========== Change sync primitives ownership for audio rendering. See blue (new code) and magenta (old code) in https://docs.google.com/drawings/d/1QDNHnCENITeH7RFnrokOls1Bbm8ydvTJie1lx7Q7V.... Some logic regarding the implementation of the audio stream IPC was also moved from AudioRendererHost to AudioEntry. Also fixed the lint warnings. BUG=425368 ========== to ========== Change sync primitives ownership for audio rendering. See blue (new code) and magenta (old code) in https://docs.google.com/drawings/d/1QDNHnCENITeH7RFnrokOls1Bbm8ydvTJie1lx7Q7V.... Also fixed the lint warnings. BUG=425368 ==========
maxmorin@chromium.org changed reviewers: + dalecurtis@chromium.org
Dale: PTAL.
lgtm https://codereview.chromium.org/2330393002/diff/140001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/2330393002/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:224: return {}; Just return nullptr? https://codereview.chromium.org/2330393002/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:354: size_t shared_memory_size = shared_memory->requested_size(); Do we sanitize this anywhere else? Otherwise seems like renderer could request arbitrary blocks? https://codereview.chromium.org/2330393002/diff/140001/content/browser/render... File content/browser/renderer_host/media/audio_sync_reader.cc (right): https://codereview.chromium.org/2330393002/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_sync_reader.cc:127: base::CheckedNumeric<size_t> memory_size = Move above the unique_ptr allocations. https://codereview.chromium.org/2330393002/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_sync_reader.cc:135: return {}; Ditto return nullptr.
https://codereview.chromium.org/2330393002/diff/140001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/2330393002/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:224: return {}; On 2016/09/16 23:29:12, DaleCurtis wrote: > Just return nullptr? Done. https://codereview.chromium.org/2330393002/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:354: size_t shared_memory_size = shared_memory->requested_size(); On 2016/09/16 23:29:12, DaleCurtis wrote: > Do we sanitize this anywhere else? Otherwise seems like renderer could request > arbitrary blocks? This is sanitized in the IPC system: https://cs.chromium.org/chromium/src/media/base/ipc/media_param_traits.cc?sq=.... https://codereview.chromium.org/2330393002/diff/140001/content/browser/render... File content/browser/renderer_host/media/audio_sync_reader.cc (right): https://codereview.chromium.org/2330393002/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_sync_reader.cc:127: base::CheckedNumeric<size_t> memory_size = On 2016/09/16 23:29:12, DaleCurtis wrote: > Move above the unique_ptr allocations. Done. https://codereview.chromium.org/2330393002/diff/140001/content/browser/render... content/browser/renderer_host/media/audio_sync_reader.cc:135: return {}; On 2016/09/16 23:29:12, DaleCurtis wrote: > Ditto return nullptr. Done.
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 Link to the patchset: https://codereview.chromium.org/2330393002/#ps160001 (title: "Comments")
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 ========== Change sync primitives ownership for audio rendering. See blue (new code) and magenta (old code) in https://docs.google.com/drawings/d/1QDNHnCENITeH7RFnrokOls1Bbm8ydvTJie1lx7Q7V.... Also fixed the lint warnings. BUG=425368 ========== to ========== Change sync primitives ownership for audio rendering. See blue (new code) and magenta (old code) in https://docs.google.com/drawings/d/1QDNHnCENITeH7RFnrokOls1Bbm8ydvTJie1lx7Q7V.... Also fixed the lint warnings. BUG=425368 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Change sync primitives ownership for audio rendering. See blue (new code) and magenta (old code) in https://docs.google.com/drawings/d/1QDNHnCENITeH7RFnrokOls1Bbm8ydvTJie1lx7Q7V.... Also fixed the lint warnings. BUG=425368 ========== to ========== Change sync primitives ownership for audio rendering. See blue (new code) and magenta (old code) in https://docs.google.com/drawings/d/1QDNHnCENITeH7RFnrokOls1Bbm8ydvTJie1lx7Q7V.... Also fixed the lint warnings. BUG=425368 Committed: https://crrev.com/b2230c4434368728ecd7a5888754204ed451d986 Cr-Commit-Position: refs/heads/master@{#419427} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/b2230c4434368728ecd7a5888754204ed451d986 Cr-Commit-Position: refs/heads/master@{#419427}
Message was sent while issue was closed.
Description was changed from ========== Change sync primitives ownership for audio rendering. See blue (new code) and magenta (old code) in https://docs.google.com/drawings/d/1QDNHnCENITeH7RFnrokOls1Bbm8ydvTJie1lx7Q7V.... Also fixed the lint warnings. BUG=425368 Committed: https://crrev.com/b2230c4434368728ecd7a5888754204ed451d986 Cr-Commit-Position: refs/heads/master@{#419427} ========== to ========== Change sync primitives ownership for audio rendering. See blue (new code) and magenta (old code) in https://docs.google.com/drawings/d/1QDNHnCENITeH7RFnrokOls1Bbm8ydvTJie1lx7Q7V.... Also fixed the lint warnings. BUG=425368 Committed: https://crrev.com/b2230c4434368728ecd7a5888754204ed451d986 Cr-Commit-Position: refs/heads/master@{#419427} ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
