Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2840773004:
[Chromoting] Add AudioVolumeApplier to reduce the complexity and the dependency of kChannels (Closed)
Description[Chromoting] Add AudioVolumeApplier to reduce the complexity and the dependency of kChannels
AudioVolumeApplier is a component to apply audio level to the samples, it also
has an AudioSilenceDetector internally to detect whether the samples are silent
before or after applying the level.
It does not rely on a fixed kChannels, which is good to support 5.1 / 7.1
channels.
BUG=669070
Review-Url: https://codereview.chromium.org/2840773004
Cr-Commit-Position: refs/heads/master@{#473809}
Committed: https://chromium.googlesource.com/chromium/src/+/26fe0dac88cb99bc94ce4d5f1ed9bc6cfff2f88e
Patch Set 1 #
Total comments: 6
Patch Set 2 : Resolve review comments #
Total comments: 4
Patch Set 3 : Resolve review comments #Patch Set 4 : Sync latest changes #
Messages
Total messages: 85 (78 generated)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by zijiehe@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 checked by zijiehe@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 checked by zijiehe@chromium.org to run a CQ dry run
Patchset #3 (id:60001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:40001) has been deleted
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by zijiehe@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...
Patchset #1 (id:80001) has been deleted
Description was changed from ========== [Chromoting] Add AudioVolumeApplier to reduce the complexity and the dependency of kChannels AudioVolumeApplier is a component to apply audio level to the samples, it also has an AudioSilenceDetector internally to detect whether the samples are silent before or after applying the level. It does not rely on a fixed kChannels, which is good to support 5.1 / 7.1 channels. BUG=669070 ========== to ========== [Chromoting] Add AudioVolumeApplier to reduce the complexity and the dependency of kChannels AudioVolumeApplier is a component to apply audio level to the samples, it also has an AudioSilenceDetector internally to detect whether the samples are silent before or after applying the level. It does not rely on a fixed kChannels, which is good to support 5.1 / 7.1 channels. BUG=669070 ==========
zijiehe@chromium.org changed reviewers: + joedow@chromium.org, sergeyu@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2840773004/diff/90001/remoting/host/audio_vol... File remoting/host/audio_volume_applier.cc (right): https://codereview.chromium.org/2840773004/diff/90001/remoting/host/audio_vol... remoting/host/audio_volume_applier.cc:11: silence_detector_after_(silence_threshold) {} I don't think we need the post-processing silence detection. It's not likely to be useful in many cases, but it makes this code more complicated. I think it's better to apply silence detection on the input only and mute the stream when volume = 0. https://codereview.chromium.org/2840773004/diff/90001/remoting/host/audio_vol... File remoting/host/audio_volume_applier.h (right): https://codereview.chromium.org/2840773004/diff/90001/remoting/host/audio_vol... remoting/host/audio_volume_applier.h:15: class AudioVolumeApplier { AudioVolumeFilter? https://codereview.chromium.org/2840773004/diff/90001/remoting/host/audio_vol... remoting/host/audio_volume_applier.h:15: class AudioVolumeApplier { What's the plan to integrate this class with downmixer? IMO it would be the best to structure it as follows: SilenceDetector -> Downmixer -> VolumeFilter This will allow to avoid downmixing when stream is inactive, while volume would be applied at the end when it's cheaper. But then it doesn't make sense to put silence detection and volume in this class. Also the interface doesn't makes it easy to connect these parts. Potentially we could implement it as a chain of filters, similar to how we connect InputFilter's for input pipeline - that would allow to make each part independent and easy to connect with each other. Encoder would be just another filter.
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Patchset #2 (id:110001) has been deleted
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_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Patchset #2 (id:130001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:130001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by zijiehe@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...
Patchset #2 (id:150001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by zijiehe@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 checked by zijiehe@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_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Patchset #2 (id:170001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2840773004/diff/90001/remoting/host/audio_vol... File remoting/host/audio_volume_applier.cc (right): https://codereview.chromium.org/2840773004/diff/90001/remoting/host/audio_vol... remoting/host/audio_volume_applier.cc:11: silence_detector_after_(silence_threshold) {} On 2017/04/26 19:57:35, Sergey Ulanov wrote: > I don't think we need the post-processing silence detection. It's not likely to > be useful in many cases, but it makes this code more complicated. I think it's > better to apply silence detection on the input only and mute the stream when > volume = 0. Done. https://codereview.chromium.org/2840773004/diff/90001/remoting/host/audio_vol... File remoting/host/audio_volume_applier.h (right): https://codereview.chromium.org/2840773004/diff/90001/remoting/host/audio_vol... remoting/host/audio_volume_applier.h:15: class AudioVolumeApplier { On 2017/04/26 19:57:36, Sergey Ulanov wrote: > What's the plan to integrate this class with downmixer? IMO it would be the best > to structure it as follows: > SilenceDetector -> Downmixer -> VolumeFilter > > This will allow to avoid downmixing when stream is inactive, while volume would > be applied at the end when it's cheaper. But then it doesn't make sense to put > silence detection and volume in this class. Also the interface doesn't makes it > easy to connect these parts. Potentially we could implement it as a chain of > filters, similar to how we connect InputFilter's for input pipeline - that would > allow to make each part independent and easy to connect with each other. Encoder > would be just another filter. IMO, downmixer should be placed out of AudioCapturer itself, but volume filter should be part of AudioCapturer. Silence detector can be part of AudioCapturer (as what we have now), or after downmixer (placing it in AudioPump should be a good choice AFAICT). Because applying volume level to the audio packet captured can make the behaviors of AudioCapturerWin and AudioCapturerLinux consistent. And downmixing and silence detector are platform independent. But considering current implementation, and the cost of applying volume level in AudioCapturerWin, leaving AudioSilenceDetector in AudioCapturer (or AudioVolumeApplier) is still a reasonable choice to trade-off between sharing logic across platforms and consuming less processor power. I tend to agree your opinion that checking silence after applying volume is not necessary in most of the cases. But the downmixer looks like an O(n*m) algorithm instead of O(n), https://cs.chromium.org/chromium/src/media/base/channel_mixer.cc?q=media/base.... So checking for the silence after applying volume level should not be a bad choice. Due to the design of AudioSilenceDetector::Reset() and the AudioSilenceDetector::silence_length_, chaining filters may not be able to work smoothly. AudioSilenceDetector can only work if all packets go through it, and any parameters after initialization are passed to it. My plan is to modify AudioSource to return AudioBus instead of AudioPacket, and down-mix it in AudioPump. https://codereview.chromium.org/2840773004/diff/90001/remoting/host/audio_vol... remoting/host/audio_volume_applier.h:15: class AudioVolumeApplier { On 2017/04/26 19:57:35, Sergey Ulanov wrote: > AudioVolumeFilter? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2840773004/diff/190001/remoting/host/audio_vo... File remoting/host/audio_volume_filter.cc (right): https://codereview.chromium.org/2840773004/diff/190001/remoting/host/audio_vo... remoting/host/audio_volume_filter.cc:14: if (frames == 0) { I think this can be replaced with a DCHECK. IAudioCaptureClient::GetBuffer() returns S_OK only when *pNumFramesToRead > 0. See https://msdn.microsoft.com/en-us/library/windows/desktop/dd370859(v=vs.85).aspx https://codereview.chromium.org/2840773004/diff/190001/remoting/host/audio_vo... File remoting/host/audio_volume_filter.h (right): https://codereview.chromium.org/2840773004/diff/190001/remoting/host/audio_vo... remoting/host/audio_volume_filter.h:30: // Returns the volume level in [0, 1]. I think it's important to make it clear that this is a normalized scalar value, i.e. sample values can be simply multiplied by the result of this function to apply volume. Otherwise one might assume that it's a volume slider position, which is different.
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2840773004/diff/190001/remoting/host/audio_vo... File remoting/host/audio_volume_filter.cc (right): https://codereview.chromium.org/2840773004/diff/190001/remoting/host/audio_vo... remoting/host/audio_volume_filter.cc:14: if (frames == 0) { On 2017/05/23 00:02:14, Sergey Ulanov wrote: > I think this can be replaced with a DCHECK. IAudioCaptureClient::GetBuffer() > returns S_OK only when *pNumFramesToRead > 0. > > See > https://msdn.microsoft.com/en-us/library/windows/desktop/dd370859(v=vs.85).aspx But if it's used on other platforms, assuming it's always larger than 0 may not that safe. https://codereview.chromium.org/2840773004/diff/190001/remoting/host/audio_vo... File remoting/host/audio_volume_filter.h (right): https://codereview.chromium.org/2840773004/diff/190001/remoting/host/audio_vo... remoting/host/audio_volume_filter.h:30: // Returns the volume level in [0, 1]. On 2017/05/23 00:02:14, Sergey Ulanov wrote: > I think it's important to make it clear that this is a normalized scalar value, > i.e. sample values can be simply multiplied by the result of this function to > apply volume. Otherwise one might assume that it's a volume slider position, > which is different. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by zijiehe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org Link to the patchset: https://codereview.chromium.org/2840773004/#ps230001 (title: "Sync latest changes")
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": 230001, "attempt_start_ts": 1495513761245960,
"parent_rev": "bafb106ad2c293fac86c9865952e78d5b17ac921", "commit_rev":
"26fe0dac88cb99bc94ce4d5f1ed9bc6cfff2f88e"}
Message was sent while issue was closed.
Description was changed from ========== [Chromoting] Add AudioVolumeApplier to reduce the complexity and the dependency of kChannels AudioVolumeApplier is a component to apply audio level to the samples, it also has an AudioSilenceDetector internally to detect whether the samples are silent before or after applying the level. It does not rely on a fixed kChannels, which is good to support 5.1 / 7.1 channels. BUG=669070 ========== to ========== [Chromoting] Add AudioVolumeApplier to reduce the complexity and the dependency of kChannels AudioVolumeApplier is a component to apply audio level to the samples, it also has an AudioSilenceDetector internally to detect whether the samples are silent before or after applying the level. It does not rely on a fixed kChannels, which is good to support 5.1 / 7.1 channels. BUG=669070 Review-Url: https://codereview.chromium.org/2840773004 Cr-Commit-Position: refs/heads/master@{#473809} Committed: https://chromium.googlesource.com/chromium/src/+/26fe0dac88cb99bc94ce4d5f1ed9... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:230001) as https://chromium.googlesource.com/chromium/src/+/26fe0dac88cb99bc94ce4d5f1ed9... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
