Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(39)

Issue 2903153004: [Chromoting] Implement down mixing in AudioPump (Closed)

Created:
3 years, 1 month ago by Hzj_jie
Modified:
3 years, 1 month ago
CC:
chromium-reviews, chromoting-reviews_chromium.org, tommi (sloooow) - chröme
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

[Chromoting] Implement down mixing in AudioPump This change implements down mixing logic in AudioPump. It adds 3 / 4 / 5 / 6 / 7 / 8 channels support in AudioPacket and down mixes the packet into stereo before encoding. The newly added logic will only be executed once multichannel output is returned by Windows API. R=SergeyU@chromium.org, JoeDow@chromium.org BUG=669070 Review-Url: https://codereview.chromium.org/2903153004 Cr-Commit-Position: refs/heads/master@{#478488} Committed: https://chromium.googlesource.com/chromium/src/+/c331598675bb5e25fd8bf064d161e05e4b1b6477

Patch Set 1 #

Total comments: 42

Patch Set 2 : Resolve review comments #

Patch Set 3 : Resolve review comments #

Patch Set 4 : Resolve review comments #

Total comments: 4

Patch Set 5 : Add more comments to explain the downmix and layout selection logic #

Total comments: 27

Patch Set 6 : Resolve review comments #

Total comments: 2

Patch Set 7 : Resovle review comments #

Total comments: 6

Patch Set 8 : Resolve review comments #

Total comments: 12
Unified diffs Side-by-side diffs Delta from patch set Stats (+234 lines, -60 lines) Patch
M remoting/host/BUILD.gn View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M remoting/host/audio_capturer_win.h View 1 2 3 4 1 chunk +4 lines, -0 lines 0 comments Download
M remoting/host/audio_capturer_win.cc View 1 2 3 4 5 6 5 chunks +50 lines, -54 lines 8 comments Download
M remoting/proto/audio.proto View 1 2 3 4 5 1 chunk +6 lines, -0 lines 2 comments Download
M remoting/protocol/DEPS View 1 chunk +1 line, -0 lines 0 comments Download
M remoting/protocol/audio_pump.h View 1 2 3 4 1 chunk +3 lines, -2 lines 0 comments Download
M remoting/protocol/audio_pump.cc View 1 2 3 4 5 6 7 5 chunks +109 lines, -2 lines 2 comments Download
M remoting/protocol/audio_pump_unittest.cc View 1 3 chunks +60 lines, -2 lines 0 comments Download

Messages

Total messages: 85 (49 generated)
Hzj_jie
3 years, 1 month ago (2017-05-26 03:14:33 UTC) #8
joedow
https://codereview.chromium.org/2903153004/diff/20001/remoting/host/audio_capturer_win.cc File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/2903153004/diff/20001/remoting/host/audio_capturer_win.cc#newcode42 remoting/host/audio_capturer_win.cc:42: int ChooseChannels(int channel_count) { This function doesn't choose a ...
3 years, 1 month ago (2017-05-26 16:01:51 UTC) #11
Hzj_jie
https://codereview.chromium.org/2903153004/diff/20001/remoting/host/audio_capturer_win.cc File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/2903153004/diff/20001/remoting/host/audio_capturer_win.cc#newcode42 remoting/host/audio_capturer_win.cc:42: int ChooseChannels(int channel_count) { On 2017/05/26 16:01:50, joedow wrote: ...
3 years, 1 month ago (2017-05-26 20:08:04 UTC) #14
Sergey Ulanov
https://codereview.chromium.org/2903153004/diff/20001/remoting/host/audio_capturer_win.cc File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/2903153004/diff/20001/remoting/host/audio_capturer_win.cc#newcode42 remoting/host/audio_capturer_win.cc:42: int ChooseChannels(int channel_count) { On 2017/05/26 20:08:03, Hzj_jie wrote: ...
3 years, 1 month ago (2017-05-26 22:24:45 UTC) #17
Hzj_jie
https://codereview.chromium.org/2903153004/diff/20001/remoting/host/audio_capturer_win.cc File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/2903153004/diff/20001/remoting/host/audio_capturer_win.cc#newcode42 remoting/host/audio_capturer_win.cc:42: int ChooseChannels(int channel_count) { On 2017/05/26 22:24:45, Sergey Ulanov ...
3 years, 1 month ago (2017-05-28 20:59:02 UTC) #21
Sergey Ulanov
https://codereview.chromium.org/2903153004/diff/20001/remoting/host/audio_capturer_win.cc File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/2903153004/diff/20001/remoting/host/audio_capturer_win.cc#newcode204 remoting/host/audio_capturer_win.cc:204: ChooseChannels(wave_format_extensible->Format.nChannels); On 2017/05/28 20:58:59, Hzj_jie wrote: > On 2017/05/26 ...
3 years, 1 month ago (2017-05-30 19:18:19 UTC) #24
Hzj_jie
https://codereview.chromium.org/2903153004/diff/20001/remoting/host/audio_capturer_win.cc File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/2903153004/diff/20001/remoting/host/audio_capturer_win.cc#newcode204 remoting/host/audio_capturer_win.cc:204: ChooseChannels(wave_format_extensible->Format.nChannels); On 2017/05/30 19:18:19, Sergey Ulanov wrote: > On ...
3 years, 1 month ago (2017-05-31 00:12:09 UTC) #27
Sergey Ulanov
https://codereview.chromium.org/2903153004/diff/20001/remoting/host/audio_capturer_win.cc File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/2903153004/diff/20001/remoting/host/audio_capturer_win.cc#newcode204 remoting/host/audio_capturer_win.cc:204: ChooseChannels(wave_format_extensible->Format.nChannels); On 2017/05/31 00:12:08, Hzj_jie wrote: > On 2017/05/30 ...
3 years, 1 month ago (2017-05-31 00:44:43 UTC) #28
Hzj_jie
https://codereview.chromium.org/2903153004/diff/20001/remoting/host/audio_capturer_win.cc File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/2903153004/diff/20001/remoting/host/audio_capturer_win.cc#newcode204 remoting/host/audio_capturer_win.cc:204: ChooseChannels(wave_format_extensible->Format.nChannels); On 2017/05/31 00:44:43, Sergey Ulanov wrote: > On ...
3 years, 1 month ago (2017-05-31 02:49:28 UTC) #31
Sergey Ulanov
https://codereview.chromium.org/2903153004/diff/80001/remoting/proto/audio.proto File remoting/proto/audio.proto (right): https://codereview.chromium.org/2903153004/diff/80001/remoting/proto/audio.proto#newcode51 remoting/proto/audio.proto:51: CHANNELS_QUAD = 4; On 2017/05/31 02:49:28, Hzj_jie wrote: > ...
3 years, 1 month ago (2017-05-31 18:37:23 UTC) #32
Hzj_jie
https://codereview.chromium.org/2903153004/diff/80001/remoting/proto/audio.proto File remoting/proto/audio.proto (right): https://codereview.chromium.org/2903153004/diff/80001/remoting/proto/audio.proto#newcode51 remoting/proto/audio.proto:51: CHANNELS_QUAD = 4; On 2017/05/31 18:37:23, Sergey Ulanov wrote: ...
3 years, 1 month ago (2017-05-31 19:23:19 UTC) #35
Sergey Ulanov
https://codereview.chromium.org/2903153004/diff/100001/remoting/protocol/audio_pump.cc File remoting/protocol/audio_pump.cc (right): https://codereview.chromium.org/2903153004/diff/100001/remoting/protocol/audio_pump.cc#newcode35 remoting/protocol/audio_pump.cc:35: if (frame_count < 1) { packet.data(0).is_empty() https://codereview.chromium.org/2903153004/diff/100001/remoting/protocol/audio_pump.cc#newcode35 remoting/protocol/audio_pump.cc:35: if ...
3 years, 1 month ago (2017-06-02 18:34:37 UTC) #38
Hzj_jie
https://codereview.chromium.org/2903153004/diff/100001/remoting/protocol/audio_pump.cc File remoting/protocol/audio_pump.cc (right): https://codereview.chromium.org/2903153004/diff/100001/remoting/protocol/audio_pump.cc#newcode35 remoting/protocol/audio_pump.cc:35: if (frame_count < 1) { On 2017/06/02 18:34:36, Sergey ...
3 years, 1 month ago (2017-06-02 22:11:25 UTC) #41
Sergey Ulanov
https://codereview.chromium.org/2903153004/diff/20001/remoting/host/audio_capturer_win.cc File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/2903153004/diff/20001/remoting/host/audio_capturer_win.cc#newcode42 remoting/host/audio_capturer_win.cc:42: int ChooseChannels(int channel_count) { On 2017/05/28 20:58:58, Hzj_jie wrote: ...
3 years, 1 month ago (2017-06-05 18:03:19 UTC) #44
Hzj_jie
https://codereview.chromium.org/2903153004/diff/20001/remoting/host/audio_capturer_win.cc File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/2903153004/diff/20001/remoting/host/audio_capturer_win.cc#newcode42 remoting/host/audio_capturer_win.cc:42: int ChooseChannels(int channel_count) { On 2017/06/05 18:03:19, Sergey Ulanov ...
3 years, 1 month ago (2017-06-06 03:21:43 UTC) #49
Sergey Ulanov
lgtm when my comments are addressed https://codereview.chromium.org/2903153004/diff/100001/remoting/protocol/audio_pump.cc File remoting/protocol/audio_pump.cc (right): https://codereview.chromium.org/2903153004/diff/100001/remoting/protocol/audio_pump.cc#newcode35 remoting/protocol/audio_pump.cc:35: if (frame_count < ...
3 years, 1 month ago (2017-06-07 06:25:00 UTC) #50
Sergey Ulanov
3 years, 1 month ago (2017-06-07 06:41:08 UTC) #51
Hzj_jie
https://codereview.chromium.org/2903153004/diff/100001/remoting/protocol/audio_pump.cc File remoting/protocol/audio_pump.cc (right): https://codereview.chromium.org/2903153004/diff/100001/remoting/protocol/audio_pump.cc#newcode35 remoting/protocol/audio_pump.cc:35: if (frame_count < 1) { On 2017/06/07 06:24:59, Sergey ...
3 years, 1 month ago (2017-06-07 20:27:18 UTC) #56
Sergey Ulanov
https://codereview.chromium.org/2903153004/diff/100001/remoting/protocol/audio_pump.cc File remoting/protocol/audio_pump.cc (right): https://codereview.chromium.org/2903153004/diff/100001/remoting/protocol/audio_pump.cc#newcode35 remoting/protocol/audio_pump.cc:35: if (frame_count < 1) { On 2017/06/07 20:27:18, Hzj_jie ...
3 years, 1 month ago (2017-06-07 21:16:29 UTC) #57
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2903153004/160001
3 years, 1 month ago (2017-06-08 00:17:34 UTC) #60
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/458526)
3 years, 1 month ago (2017-06-08 00:26:56 UTC) #62
Hzj_jie
Chris, Chrome Remote Desktop now uses media::ChannelMixer to downmix 2+ channels into stereo, and depends ...
3 years, 1 month ago (2017-06-08 03:46:37 UTC) #64
chcunningham
Some questions, mostly about the mask. I'm not sure how busted things will be. Have ...
3 years, 1 month ago (2017-06-08 16:39:45 UTC) #66
DaleCurtis
https://codereview.chromium.org/2903153004/diff/160001/remoting/host/audio_capturer_win.cc File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/2903153004/diff/160001/remoting/host/audio_capturer_win.cc#newcode276 remoting/host/audio_capturer_win.cc:276: // consider dwChannelMask. On 2017/06/08 at 16:39:45, chcunningham wrote: ...
3 years, 1 month ago (2017-06-08 17:05:47 UTC) #67
DaleCurtis
cc:tommi in case he wants to chime in with any advice on Windows input capture ...
3 years, 1 month ago (2017-06-08 17:09:46 UTC) #68
tommi (sloooow) - chröme
https://codereview.chromium.org/2903153004/diff/160001/remoting/proto/audio.proto File remoting/proto/audio.proto (right): https://codereview.chromium.org/2903153004/diff/160001/remoting/proto/audio.proto#newcode50 remoting/proto/audio.proto:50: CHANNELS_SURROUND = 3; what about 2.1?
3 years, 1 month ago (2017-06-08 17:58:08 UTC) #69
Sergey Ulanov
Folks, please read my and Zijie's comments first. The gist of these comments: 1. we ...
3 years, 1 month ago (2017-06-08 18:27:59 UTC) #70
Hzj_jie
On 2017/06/08 17:09:46, DaleCurtis wrote: > cc:tommi in case he wants to chime in with ...
3 years, 1 month ago (2017-06-08 19:07:05 UTC) #71
Hzj_jie
https://codereview.chromium.org/2903153004/diff/160001/remoting/host/audio_capturer_win.cc File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/2903153004/diff/160001/remoting/host/audio_capturer_win.cc#newcode276 remoting/host/audio_capturer_win.cc:276: // consider dwChannelMask. On 2017/06/08 17:05:47, DaleCurtis wrote: > ...
3 years, 1 month ago (2017-06-08 19:07:49 UTC) #72
Hzj_jie
On 2017/06/08 16:39:45, chcunningham wrote: > Some questions, mostly about the mask. I'm not sure ...
3 years, 1 month ago (2017-06-08 19:10:04 UTC) #73
chcunningham
LGTM > Folks, please read my and Zijie's comments first. Apologies. https://codereview.chromium.org/2903153004/diff/20001/remoting/host/audio_capturer_win.cc File remoting/host/audio_capturer_win.cc (right): ...
3 years, 1 month ago (2017-06-09 19:25:28 UTC) #74
Hzj_jie
https://codereview.chromium.org/2903153004/diff/20001/remoting/host/audio_capturer_win.cc File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/2903153004/diff/20001/remoting/host/audio_capturer_win.cc#newcode42 remoting/host/audio_capturer_win.cc:42: int ChooseChannels(int channel_count) { On 2017/06/09 19:25:28, chcunningham wrote: ...
3 years, 1 month ago (2017-06-09 21:33:47 UTC) #75
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2903153004/160001
3 years, 1 month ago (2017-06-09 21:34:25 UTC) #77
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/475917)
3 years, 1 month ago (2017-06-09 23:21:40 UTC) #79
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2903153004/160001
3 years, 1 month ago (2017-06-10 00:23:38 UTC) #81
commit-bot: I haz the power
3 years, 1 month ago (2017-06-10 01:22:39 UTC) #85
Message was sent while issue was closed.
Committed patchset #8 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/c331598675bb5e25fd8bf064d161...

Powered by Google App Engine
This is Rietveld 408576698