Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2903153004:
[Chromoting] Implement down mixing in AudioPump (Closed)
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
Messages
Total messages: 85 (49 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_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
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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2903153004/diff/20001/remoting/host/audio_cap... File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/2903153004/diff/20001/remoting/host/audio_cap... remoting/host/audio_capturer_win.cc:42: int ChooseChannels(int channel_count) { This function doesn't choose a channel, it converts an unsupported channel number into stereo (supported values are passed through as-is). Is that safe to do? i.e. if the channel count was 10 (9.1 audio) you would convert it to 2 which doesn't seem right. https://codereview.chromium.org/2903153004/diff/20001/remoting/protocol/audio... File remoting/protocol/audio_pump.cc (right): https://codereview.chromium.org/2903153004/diff/20001/remoting/protocol/audio... remoting/protocol/audio_pump.cc:29: return packet.data(0).size() / packet.channels() / packet.bytes_per_sample(); Could an empty packet be passed in here (with the result being 0 / 0 / 0)? would it make sense for some DCHECKs here? https://codereview.chromium.org/2903153004/diff/20001/remoting/protocol/audio... remoting/protocol/audio_pump.cc:32: std::unique_ptr<media::AudioBus> ToAudioBus( nit: Rename to 'PacketToAudioBus' ? https://codereview.chromium.org/2903153004/diff/20001/remoting/protocol/audio... remoting/protocol/audio_pump.cc:34: using namespace media; The inline using directives are a bit repetitive. If you need them, please move them out of the method so all of the methods in the file can use them. Repeating them in each method hurts readability IMO and isn't consistent with the rest of the codebase. https://codereview.chromium.org/2903153004/diff/20001/remoting/protocol/audio... remoting/protocol/audio_pump.cc:36: if (frame_count <= 0) { nit: 'frame_count < 1' is simpler https://codereview.chromium.org/2903153004/diff/20001/remoting/protocol/audio... remoting/protocol/audio_pump.cc:42: // float*, meanwhile WrapMemory() function. Can you fix this sentence fragment. The first bit sounds like a TODO and the second doesn't complete the thought. https://codereview.chromium.org/2903153004/diff/20001/remoting/protocol/audio... remoting/protocol/audio_pump.cc:114: std::unique_ptr<remoting::AudioPacket> packet); You don't need the remote prefix here since it is inside the remoting namespace already. https://codereview.chromium.org/2903153004/diff/20001/remoting/protocol/audio... remoting/protocol/audio_pump.cc:133: std::unique_ptr<media::ChannelMixer> mixer_ = nullptr; No need to assign nullptr here since unique_ptr is initialized to nullptr in its c'tor. https://codereview.chromium.org/2903153004/diff/20001/remoting/protocol/audio... remoting/protocol/audio_pump.cc:230: std::unique_ptr<AudioBus> output = AudioBus::Create(2, input->frames()); Can you replace the magic '2' with AudioPacket::CHANNELS_STEREO? https://codereview.chromium.org/2903153004/diff/20001/remoting/protocol/audio... remoting/protocol/audio_pump.cc:234: DCHECK(result); I don't think this DCHECK is needed here as the next line derefs the pointer since the * is deref'd on the next line. https://codereview.chromium.org/2903153004/diff/20001/remoting/protocol/audio... File remoting/protocol/audio_pump_unittest.cc (right): https://codereview.chromium.org/2903153004/diff/20001/remoting/protocol/audio... remoting/protocol/audio_pump_unittest.cc:137: base::RunLoop().RunUntilIdle(); Please add a comment here to note why this is needed. I had to look at another test to figure out what this is for (comment from test above: '// Run message loop to let the pump start the capturer.'). https://codereview.chromium.org/2903153004/diff/20001/remoting/protocol/audio... remoting/protocol/audio_pump_unittest.cc:140: // Randomly generate several audio packets with different channel counts. nit: This isn't random if you define a static list of channel numbers. Please fix the comment. https://codereview.chromium.org/2903153004/diff/20001/remoting/protocol/audio... remoting/protocol/audio_pump_unittest.cc:143: }; Would it make sense to use the enum names here instead? I suppose it makes sense that 8 == 7.1 audio, but if I saw 'AUDIO_7_1' I think it would be easier to scan. https://codereview.chromium.org/2903153004/diff/20001/remoting/protocol/audio... remoting/protocol/audio_pump_unittest.cc:147: base::RunLoop().RunUntilIdle(); Please add comments (as in the test above) to explain why you are running the run loops.
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/2903153004/diff/20001/remoting/host/audio_cap... File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/2903153004/diff/20001/remoting/host/audio_cap... remoting/host/audio_capturer_win.cc:42: int ChooseChannels(int channel_count) { On 2017/05/26 16:01:50, joedow wrote: > This function doesn't choose a channel, it converts an unsupported channel > number into stereo (supported values are passed through as-is). > > Is that safe to do? i.e. if the channel count was 10 (9.1 audio) you would > convert it to 2 which doesn't seem right. Before this change, we always select stereo. But this change add supports to mono / 5.1 / 6.1 / 7.1. We can surely add more channel choices, but AFAICT, Windows supports only 1 / 2 / 6 / 8. While 6.1 is for testing purpose only. https://codereview.chromium.org/2903153004/diff/20001/remoting/protocol/audio... File remoting/protocol/audio_pump.cc (right): https://codereview.chromium.org/2903153004/diff/20001/remoting/protocol/audio... remoting/protocol/audio_pump.cc:29: return packet.data(0).size() / packet.channels() / packet.bytes_per_sample(); On 2017/05/26 16:01:50, joedow wrote: > Could an empty packet be passed in here (with the result being 0 / 0 / 0)? > would it make sense for some DCHECKs here? AudioPump uses an AudioSource interface. So assuming the input looks dangerous to me. https://codereview.chromium.org/2903153004/diff/20001/remoting/protocol/audio... remoting/protocol/audio_pump.cc:32: std::unique_ptr<media::AudioBus> ToAudioBus( On 2017/05/26 16:01:50, joedow wrote: > nit: Rename to 'PacketToAudioBus' ? Done. https://codereview.chromium.org/2903153004/diff/20001/remoting/protocol/audio... remoting/protocol/audio_pump.cc:34: using namespace media; On 2017/05/26 16:01:50, joedow wrote: > The inline using directives are a bit repetitive. If you need them, please move > them out of the method so all of the methods in the file can use them. > Repeating them in each method hurts readability IMO and isn't consistent with > the rest of the codebase. Done. https://codereview.chromium.org/2903153004/diff/20001/remoting/protocol/audio... remoting/protocol/audio_pump.cc:36: if (frame_count <= 0) { On 2017/05/26 16:01:50, joedow wrote: > nit: 'frame_count < 1' is simpler Done. https://codereview.chromium.org/2903153004/diff/20001/remoting/protocol/audio... remoting/protocol/audio_pump.cc:42: // float*, meanwhile WrapMemory() function. On 2017/05/26 16:01:50, joedow wrote: > Can you fix this sentence fragment. The first bit sounds like a TODO and the > second doesn't complete the thought. Done. https://codereview.chromium.org/2903153004/diff/20001/remoting/protocol/audio... remoting/protocol/audio_pump.cc:114: std::unique_ptr<remoting::AudioPacket> packet); On 2017/05/26 16:01:50, joedow wrote: > You don't need the remote prefix here since it is inside the remoting namespace > already. Done. https://codereview.chromium.org/2903153004/diff/20001/remoting/protocol/audio... remoting/protocol/audio_pump.cc:133: std::unique_ptr<media::ChannelMixer> mixer_ = nullptr; On 2017/05/26 16:01:50, joedow wrote: > No need to assign nullptr here since unique_ptr is initialized to nullptr in its > c'tor. Done. https://codereview.chromium.org/2903153004/diff/20001/remoting/protocol/audio... remoting/protocol/audio_pump.cc:230: std::unique_ptr<AudioBus> output = AudioBus::Create(2, input->frames()); On 2017/05/26 16:01:50, joedow wrote: > Can you replace the magic '2' with AudioPacket::CHANNELS_STEREO? Done. https://codereview.chromium.org/2903153004/diff/20001/remoting/protocol/audio... remoting/protocol/audio_pump.cc:234: DCHECK(result); On 2017/05/26 16:01:50, joedow wrote: > I don't think this DCHECK is needed here as the next line derefs the pointer > since the * is deref'd on the next line. Done. https://codereview.chromium.org/2903153004/diff/20001/remoting/protocol/audio... File remoting/protocol/audio_pump_unittest.cc (right): https://codereview.chromium.org/2903153004/diff/20001/remoting/protocol/audio... remoting/protocol/audio_pump_unittest.cc:137: base::RunLoop().RunUntilIdle(); On 2017/05/26 16:01:51, joedow wrote: > Please add a comment here to note why this is needed. I had to look at another > test to figure out what this is for (comment from test above: '// Run message > loop to let the pump start the capturer.'). Done. https://codereview.chromium.org/2903153004/diff/20001/remoting/protocol/audio... remoting/protocol/audio_pump_unittest.cc:140: // Randomly generate several audio packets with different channel counts. On 2017/05/26 16:01:51, joedow wrote: > nit: This isn't random if you define a static list of channel numbers. Please > fix the comment. Done. https://codereview.chromium.org/2903153004/diff/20001/remoting/protocol/audio... remoting/protocol/audio_pump_unittest.cc:143: }; On 2017/05/26 16:01:51, joedow wrote: > Would it make sense to use the enum names here instead? I suppose it makes > sense that 8 == 7.1 audio, but if I saw 'AUDIO_7_1' I think it would be easier > to scan. Done. https://codereview.chromium.org/2903153004/diff/20001/remoting/protocol/audio... remoting/protocol/audio_pump_unittest.cc:147: base::RunLoop().RunUntilIdle(); On 2017/05/26 16:01:51, joedow wrote: > Please add comments (as in the test above) to explain why you are running the > run loops. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2903153004/diff/20001/remoting/host/audio_cap... File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/2903153004/diff/20001/remoting/host/audio_cap... remoting/host/audio_capturer_win.cc:42: int ChooseChannels(int channel_count) { On 2017/05/26 20:08:03, Hzj_jie wrote: > On 2017/05/26 16:01:50, joedow wrote: > > This function doesn't choose a channel, it converts an unsupported channel > > number into stereo (supported values are passed through as-is). > > > > Is that safe to do? i.e. if the channel count was 10 (9.1 audio) you would > > convert it to 2 which doesn't seem right. > > Before this change, we always select stereo. But this change add supports to > mono / 5.1 / 6.1 / 7.1. > We can surely add more channel choices, but AFAICT, Windows supports only 1 / 2 > / 6 / 8. While 6.1 is for testing purpose only. If I understand correctly this function defines channel configuration that the request from a system. But if the channel count is not one of the supported set, does it still make sense to request STEREO? Most likely it will fail anyway - we saw that Windows doesn't downmix captured streams, so I'm not sure it's worth trying initialize the stream anyway. I'm not sure these are the only configurations that windows supports. Is it documented anywhere? At very least should support 2.1 and Quad configurations. E.g. this page shows two different 4 channel configs https://msdn.microsoft.com/en-us/library/windows/desktop/dd390971(v=vs.85).aspx https://codereview.chromium.org/2903153004/diff/20001/remoting/host/audio_cap... remoting/host/audio_capturer_win.cc:204: ChooseChannels(wave_format_extensible->Format.nChannels); If you change number of channels here then you probably want to change configuration in wave_format_extensible->dwChannelMask as well. Could it be the reason windows doesn't downmix the stream currently?
Description was changed from ========== [Chromoting] Implement down mixing in AudioPump This change implements down mixing logic in AudioPump. It adds 5.1 and 7.1 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 ========== to ========== [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 ==========
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/2903153004/diff/20001/remoting/host/audio_cap... File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/2903153004/diff/20001/remoting/host/audio_cap... remoting/host/audio_capturer_win.cc:42: int ChooseChannels(int channel_count) { On 2017/05/26 22:24:45, Sergey Ulanov wrote: > On 2017/05/26 20:08:03, Hzj_jie wrote: > > On 2017/05/26 16:01:50, joedow wrote: > > > This function doesn't choose a channel, it converts an unsupported channel > > > number into stereo (supported values are passed through as-is). > > > > > > Is that safe to do? i.e. if the channel count was 10 (9.1 audio) you would > > > convert it to 2 which doesn't seem right. > > > > Before this change, we always select stereo. But this change add supports to > > mono / 5.1 / 6.1 / 7.1. > > We can surely add more channel choices, but AFAICT, Windows supports only 1 / > 2 > > / 6 / 8. While 6.1 is for testing purpose only. > > If I understand correctly this function defines channel configuration that the > request from a system. But if the channel count is not one of the supported set, > does it still make sense to request STEREO? Most likely it will fail anyway - we > saw that Windows doesn't downmix captured streams, so I'm not sure it's worth > trying initialize the stream anyway. IMO, it's still worthy to have a shot: it won't waste too many resources as only one more function call is needed but it ensures no regression will ever be triggered. > > I'm not sure these are the only configurations that windows supports. Is it > documented anywhere? At very least should support 2.1 and Quad configurations. > E.g. this page shows two different 4 channel configs > https://msdn.microsoft.com/en-us/library/windows/desktop/dd390971(v=vs.85).aspx > > No, I found no full lists. But searching on web leads me to the wiki page https://en.wikipedia.org/wiki/Sound_card and ebay or amazon: we do have 2.1, 4 channels and 4.1 channels. But 7.1 / 8 channels is the top level for PC. https://codereview.chromium.org/2903153004/diff/20001/remoting/host/audio_cap... remoting/host/audio_capturer_win.cc:204: ChooseChannels(wave_format_extensible->Format.nChannels); On 2017/05/26 22:24:45, Sergey Ulanov wrote: > If you change number of channels here then you probably want to change > configuration in wave_format_extensible->dwChannelMask as well. Could it be the > reason windows doesn't downmix the stream currently? According to MSDN, https://social.msdn.microsoft.com/Forums/vstudio/en-US/3a5d0748-fb7f-4de9-80f..., WASAPI (IAudioClient) does not do any format conversion. I have also tried to set the dwChannelMask, but it won't make any difference, IAudioClient::Initialize() function still returns 2004287480. But the answer on MSDN also mentioned, "you have to stream in the mix format, or at least something fairly close to it." So always trying stereo is not a bad idea for unknown channels. P.S. Most higher-level APIs will do format conversion for you: waveOut / waveIn Media Foundation MediaCapture etc. We can also try waveOut.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2903153004/diff/20001/remoting/host/audio_cap... File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/2903153004/diff/20001/remoting/host/audio_cap... 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 22:24:45, Sergey Ulanov wrote: > > If you change number of channels here then you probably want to change > > configuration in wave_format_extensible->dwChannelMask as well. Could it be > the > > reason windows doesn't downmix the stream currently? > > According to MSDN, > https://social.msdn.microsoft.com/Forums/vstudio/en-US/3a5d0748-fb7f-4de9-80f..., > WASAPI (IAudioClient) does not do any format conversion. I have also tried to > set the dwChannelMask, but it won't make any difference, > IAudioClient::Initialize() function still returns 2004287480. > But the answer on MSDN also mentioned, "you have to stream in the mix format, or > at least something fairly close to it." So always trying stereo is not a bad > idea for unknown channels. > > P.S. Most higher-level APIs will do format conversion for you: > waveOut / waveIn > Media Foundation > MediaCapture > etc. > > We can also try waveOut. My point was that, if the API will never downmix the stream, then it makes no sense to set nChannels to 2 here. If changing nChannels is useful, then you also want to change wave_format_extensible->dwChannelMask.
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/2903153004/diff/20001/remoting/host/audio_cap... File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/2903153004/diff/20001/remoting/host/audio_cap... remoting/host/audio_capturer_win.cc:204: ChooseChannels(wave_format_extensible->Format.nChannels); On 2017/05/30 19:18:19, Sergey Ulanov wrote: > On 2017/05/28 20:58:59, Hzj_jie wrote: > > On 2017/05/26 22:24:45, Sergey Ulanov wrote: > > > If you change number of channels here then you probably want to change > > > configuration in wave_format_extensible->dwChannelMask as well. Could it be > > the > > > reason windows doesn't downmix the stream currently? > > > > According to MSDN, > > > https://social.msdn.microsoft.com/Forums/vstudio/en-US/3a5d0748-fb7f-4de9-80f..., > > WASAPI (IAudioClient) does not do any format conversion. I have also tried to > > set the dwChannelMask, but it won't make any difference, > > IAudioClient::Initialize() function still returns 2004287480. > > But the answer on MSDN also mentioned, "you have to stream in the mix format, > or > > at least something fairly close to it." So always trying stereo is not a bad > > idea for unknown channels. > > > > P.S. Most higher-level APIs will do format conversion for you: > > waveOut / waveIn > > Media Foundation > > MediaCapture > > etc. > > > > We can also try waveOut. > > My point was that, if the API will never downmix the stream, then it makes no > sense to set nChannels to 2 here. > If changing nChannels is useful, then you also want to change > wave_format_extensible->dwChannelMask. Got you. I will update both nChannels and dwChannelMask.
https://codereview.chromium.org/2903153004/diff/20001/remoting/host/audio_cap... File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/2903153004/diff/20001/remoting/host/audio_cap... 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 19:18:19, Sergey Ulanov wrote: > > My point was that, if the API will never downmix the stream, then it makes no > > sense to set nChannels to 2 here. > > If changing nChannels is useful, then you also want to change > > wave_format_extensible->dwChannelMask. > > Got you. I will update both nChannels and dwChannelMask. Is this useful given that we know that windows will not downmix the stream anyway? I think it's better to just return error here when nChannels is outside of the supported range instead of trying to initialize in stereo. 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.pr... remoting/proto/audio.proto:51: CHANNELS_QUAD = 4; There are several different speaker layouts for 3 or 4 speakers. See https://codesearch.chromium.org/chromium/src/media/base/channel_layout.h E.g. for 4 channels it can be (L, R, C, LFE), or (L, R, BL, BR), or (L, R, C, BC) So speaker layout is not uniquely identified by number of channels. There are several approaches we could take 1. Extend this struct to identify broader set of speaker layouts, either by adding a layout field, or by renaming channels field to layout (channels count can be deduced from layout). 2. Limit support to widely used speaker layouts, i.e. 5.1 and 7.1 in addition to stereo. 3. Use a different struct for non-encoded frames. Then it will be possible to use media::ChannelLayout enum to identify various layouts. I think (2) is close what you had initially, and it seems reasonable to me. We just need to have clear expectations on how the capturer will work on systems with other speaker configurations. Or if you think we want to support all possible layouts then (3) is the right approach.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2903153004/diff/20001/remoting/host/audio_cap... File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/2903153004/diff/20001/remoting/host/audio_cap... remoting/host/audio_capturer_win.cc:204: ChooseChannels(wave_format_extensible->Format.nChannels); On 2017/05/31 00:44:43, Sergey Ulanov wrote: > On 2017/05/31 00:12:08, Hzj_jie wrote: > > On 2017/05/30 19:18:19, Sergey Ulanov wrote: > > > My point was that, if the API will never downmix the stream, then it makes > no > > > sense to set nChannels to 2 here. > > > If changing nChannels is useful, then you also want to change > > > wave_format_extensible->dwChannelMask. > > > > Got you. I will update both nChannels and dwChannelMask. > > Is this useful given that we know that windows will not downmix the stream > anyway? I think it's better to just return error here when nChannels is outside > of the supported range instead of trying to initialize in stereo. Trying to initialize in stereo does not really waste too much resources, and because of the hidden source character of Windows, we cannot predict what will happen if we simply drop the initialization attempt here. I tend to avoid changes to existing logic path when handling edge cases. 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.pr... remoting/proto/audio.proto:51: CHANNELS_QUAD = 4; On 2017/05/31 00:44:43, Sergey Ulanov wrote: > There are several different speaker layouts for 3 or 4 speakers. See > https://codesearch.chromium.org/chromium/src/media/base/channel_layout.h > > E.g. for 4 channels it can be (L, R, C, LFE), or (L, R, BL, BR), or (L, R, C, > BC) > > So speaker layout is not uniquely identified by number of channels. > > There are several approaches we could take > 1. Extend this struct to identify broader set of speaker layouts, either by > adding a layout field, or by renaming channels field to layout (channels count > can be deduced from layout). > 2. Limit support to widely used speaker layouts, i.e. 5.1 and 7.1 in addition to > stereo. > 3. Use a different struct for non-encoded frames. Then it will be possible to > use media::ChannelLayout enum to identify various layouts. > > I think (2) is close what you had initially, and it seems reasonable to me. We > just need to have clear expectations on how the capturer will work on systems > with other speaker configurations. > > Or if you think we want to support all possible layouts then (3) is the right > approach. Yes, I simply choose the most common used configurations. And both 5.1 and 7.1 also have different configurations. Windows supports from 1 to 8 channels, while supporting 3 / 4 / 5 / 7 channels in CRD is still better than nothing. Since we will downmix anyway, layouts is not that important. In ChannelMixingMatrix (https://cs.chromium.org/chromium/src/media/base/channel_mixing_matrix.cc?type...), the layout impacts only which channels are merged. Using 4 channels as an example, L / R / C / BC will be merged to, (L + C + BC) / (R + C + BC) L / R / BL / BR will be merged to, (L + BL) / (R + BR) If we always use L / R / C / BC, we will merge L / R / BL / BR layout into, (L + BL + BR) / (R + BL + BR) which IMO is still acceptable for remote desktop usage. Better than nothing. I also believe the count of users who require multiple channels is limited, using approach (3) is not worthy.
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.pr... remoting/proto/audio.proto:51: CHANNELS_QUAD = 4; On 2017/05/31 02:49:28, Hzj_jie wrote: > On 2017/05/31 00:44:43, Sergey Ulanov wrote: > > There are several different speaker layouts for 3 or 4 speakers. See > > https://codesearch.chromium.org/chromium/src/media/base/channel_layout.h > > > > E.g. for 4 channels it can be (L, R, C, LFE), or (L, R, BL, BR), or (L, R, C, > > BC) > > > > So speaker layout is not uniquely identified by number of channels. > > > > There are several approaches we could take > > 1. Extend this struct to identify broader set of speaker layouts, either by > > adding a layout field, or by renaming channels field to layout (channels count > > can be deduced from layout). > > 2. Limit support to widely used speaker layouts, i.e. 5.1 and 7.1 in addition > to > > stereo. > > 3. Use a different struct for non-encoded frames. Then it will be possible to > > use media::ChannelLayout enum to identify various layouts. > > > > I think (2) is close what you had initially, and it seems reasonable to me. We > > just need to have clear expectations on how the capturer will work on systems > > with other speaker configurations. > > > > Or if you think we want to support all possible layouts then (3) is the right > > approach. > > Yes, I simply choose the most common used configurations. And both 5.1 and 7.1 > also have different configurations. Windows supports from 1 to 8 channels, while > supporting 3 / 4 / 5 / 7 channels in CRD is still better than nothing. Since we > will downmix anyway, layouts is not that important. In ChannelMixingMatrix > (https://cs.chromium.org/chromium/src/media/base/channel_mixing_matrix.cc?type...), > the layout impacts only which channels are merged. Using 4 channels as an > example, > L / R / C / BC will be merged to, > (L + C + BC) / (R + C + BC) > L / R / BL / BR will be merged to, > (L + BL) / (R + BR) > If we always use L / R / C / BC, we will merge L / R / BL / BR layout into, > (L + BL + BR) / (R + BL + BR) > which IMO is still acceptable for remote desktop usage. Better than nothing. This may work (though it's not ideal because it gives suboptimal experience for a more common quadraphonic configuration). But it needs to be documented somewhere, here or in the capturer. Also, if we treat all 4-channel layouts as L/R/C/BC, then it shouldn't be called CHANNELS_QUAD. Quadraphonic layout is L/R/BL/BR. Maybe call these CHANNELS_SURROUND_3 and CHANNELS_SURROUND_4? > > I also believe the count of users who require multiple channels is limited, > using approach (3) is not worthy. As we discussed before it is suboptimal that we currently use protobufs outside of the protocol layer, regardless of multichannel audio support. BTW, potentially we could use media::AudioBus to represent unecoded frames. That would also make the encoder more efficient by avoiding unnecessary float->int conversion. The encoder already uses AudioBus internally for resampling, WDYT?
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/2903153004/diff/80001/remoting/proto/audio.proto File remoting/proto/audio.proto (right): https://codereview.chromium.org/2903153004/diff/80001/remoting/proto/audio.pr... remoting/proto/audio.proto:51: CHANNELS_QUAD = 4; On 2017/05/31 18:37:23, Sergey Ulanov wrote: > On 2017/05/31 02:49:28, Hzj_jie wrote: > > On 2017/05/31 00:44:43, Sergey Ulanov wrote: > > > There are several different speaker layouts for 3 or 4 speakers. See > > > https://codesearch.chromium.org/chromium/src/media/base/channel_layout.h > > > > > > E.g. for 4 channels it can be (L, R, C, LFE), or (L, R, BL, BR), or (L, R, > C, > > > BC) > > > > > > So speaker layout is not uniquely identified by number of channels. > > > > > > There are several approaches we could take > > > 1. Extend this struct to identify broader set of speaker layouts, either by > > > adding a layout field, or by renaming channels field to layout (channels > count > > > can be deduced from layout). > > > 2. Limit support to widely used speaker layouts, i.e. 5.1 and 7.1 in > addition > > to > > > stereo. > > > 3. Use a different struct for non-encoded frames. Then it will be possible > to > > > use media::ChannelLayout enum to identify various layouts. > > > > > > I think (2) is close what you had initially, and it seems reasonable to me. > We > > > just need to have clear expectations on how the capturer will work on > systems > > > with other speaker configurations. > > > > > > Or if you think we want to support all possible layouts then (3) is the > right > > > approach. > > > > Yes, I simply choose the most common used configurations. And both 5.1 and 7.1 > > also have different configurations. Windows supports from 1 to 8 channels, > while > > supporting 3 / 4 / 5 / 7 channels in CRD is still better than nothing. Since > we > > will downmix anyway, layouts is not that important. In ChannelMixingMatrix > > > (https://cs.chromium.org/chromium/src/media/base/channel_mixing_matrix.cc?type...), > > the layout impacts only which channels are merged. Using 4 channels as an > > example, > > L / R / C / BC will be merged to, > > (L + C + BC) / (R + C + BC) > > L / R / BL / BR will be merged to, > > (L + BL) / (R + BR) > > If we always use L / R / C / BC, we will merge L / R / BL / BR layout into, > > (L + BL + BR) / (R + BL + BR) > > which IMO is still acceptable for remote desktop usage. Better than nothing. > > This may work (though it's not ideal because it gives suboptimal experience for > a more common quadraphonic configuration). But > it needs to be documented somewhere, here or in the capturer. Also, if we treat > all 4-channel layouts as L/R/C/BC, then it shouldn't be called CHANNELS_QUAD. > Quadraphonic layout is L/R/BL/BR. > Maybe call these CHANNELS_SURROUND_3 and CHANNELS_SURROUND_4? > I will add comments to both AudioCapturerWin and AudioPump: the capturer treats all the 4-channel configurations as QUAD, while the pump does the downmixing. > > > > I also believe the count of users who require multiple channels is limited, > > using approach (3) is not worthy. > > As we discussed before it is suboptimal that we currently use protobufs outside > of the protocol layer, regardless of multichannel audio support. > BTW, potentially we could use media::AudioBus to represent unecoded frames. That > would also make the encoder more efficient by avoiding unnecessary float->int > conversion. The encoder already uses AudioBus internally for resampling, WDYT? Yes, it was my original plan (indeed the branch name of this change is called "return-audio-bus"). But then I found it was not worthy due to the very limited user cases. But for the performance concern, yes, I think it's still a good solution. IMO, this change is still reasonable to solve the users' need. How about let's submit this change first and optimize in a different change? Then we may not be that urgent: after all, converting 48k * 8 int16s from or to floats per second is not a big deal for a modern CPU.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2903153004/diff/100001/remoting/protocol/audi... File remoting/protocol/audio_pump.cc (right): https://codereview.chromium.org/2903153004/diff/100001/remoting/protocol/audi... remoting/protocol/audio_pump.cc:35: if (frame_count < 1) { packet.data(0).is_empty() https://codereview.chromium.org/2903153004/diff/100001/remoting/protocol/audi... remoting/protocol/audio_pump.cc:35: if (frame_count < 1) { Why would we get an empty packet here? Can it be replaced with a DCHECK? https://codereview.chromium.org/2903153004/diff/100001/remoting/protocol/audi... remoting/protocol/audio_pump.cc:43: return media::AudioBus::WrapMemory( Does this work correctly remoting::AudioPacket always uses 16-bit signed integers, while AudioBus::WrapMemory() expects 32-bit FP (i.e. float) samples. https://codereview.chromium.org/2903153004/diff/100001/remoting/protocol/audi... remoting/protocol/audio_pump.cc:46: reinterpret_cast<int16_t*>(const_cast<char*>(packet.data(0).data()))); You can use string_as_array() from base/stl_util.h instead of the const_cast const_cast<char*> https://codereview.chromium.org/2903153004/diff/100001/remoting/protocol/audi... remoting/protocol/audio_pump.cc:62: sizeof(float) * sizeof(int16_t)); I don't think we want to use AudioBus::CalculateMemorySize() here. This code is correct only with the assumption that AudioBus uses float internally, and there is no reason we need to depend on it here. "packet.channels() * packet.frames() * AudioPacket::BYTES_PER_SAMPLE_2" would be simpler and clearer https://codereview.chromium.org/2903153004/diff/100001/remoting/protocol/audi... remoting/protocol/audio_pump.cc:81: return media::CHANNEL_LAYOUT_QUAD; I don't think that downmixing all 4-channel layouts as quadraphonic is acceptable. In case the system is using 3.1 layout we would have center channel mixed to the left and LFE mixed to the right, which may severely affect stereo imaging (voice in the left ear, special effects in the right). My understanding from your previous comment was that you were suggesting to treat all 4-speaker layouts as surround (i.e. L / R / C / BC). That would mix the two additional channels to the left and the right channels equally, which is acceptable even if the system uses quad layout. https://codereview.chromium.org/2903153004/diff/100001/remoting/protocol/audi... remoting/protocol/audio_pump.cc:212: packet->bytes_per_sample() != AudioPacket::BYTES_PER_SAMPLE_2) { I think all these checks can be DCHECKs https://codereview.chromium.org/2903153004/diff/100001/remoting/protocol/audi... remoting/protocol/audio_pump.cc:216: if (input_layout == media::CHANNEL_LAYOUT_UNSUPPORTED || I think in case the layout is unsupported it doesn't make sense to continue with the packet. Encoder won't be able to handle it. Return nullptr in that case? https://codereview.chromium.org/2903153004/diff/100001/remoting/protocol/audi... remoting/protocol/audio_pump.cc:222: if (!mixer_ || last_input_layout_ != input_layout) { It's better to move this code above mono/stereo check. Otherwise last_input_layout_ won't be set to MONO/STEREO, which is confusing. https://codereview.chromium.org/2903153004/diff/100001/remoting/protocol/audi... remoting/protocol/audio_pump.cc:230: return packet; Why would AudioPacketToAudioBus() fail? If it does fail, do we want pass the packet to the encoder anyway? https://codereview.chromium.org/2903153004/diff/100001/remoting/protocol/audi... remoting/protocol/audio_pump.cc:240: result->set_channels(AudioPacket::CHANNELS_STEREO); I think it would be best to set all these properties in AudioBusToAudioPacket(). Otherwise it looks wrong that AudioBusToAudioPacket() copies data, but doesn't set other properties.
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/2903153004/diff/100001/remoting/protocol/audi... File remoting/protocol/audio_pump.cc (right): https://codereview.chromium.org/2903153004/diff/100001/remoting/protocol/audi... remoting/protocol/audio_pump.cc:35: if (frame_count < 1) { On 2017/06/02 18:34:36, Sergey Ulanov wrote: > packet.data(0).is_empty() (frame_count == 0) != (packet.data(0).empty()) Considering a crappy AudioCapturer returns two channels frames without enough data: data(0).size() < 1 * packet.channels() * packet.bytes_per_sample(). https://codereview.chromium.org/2903153004/diff/100001/remoting/protocol/audi... remoting/protocol/audio_pump.cc:35: if (frame_count < 1) { On 2017/06/02 18:34:36, Sergey Ulanov wrote: > Why would we get an empty packet here? Can it be replaced with a DCHECK? It's not an empty packet, but a defeated packet. https://codereview.chromium.org/2903153004/diff/100001/remoting/protocol/audi... remoting/protocol/audio_pump.cc:43: return media::AudioBus::WrapMemory( On 2017/06/02 18:34:36, Sergey Ulanov wrote: > Does this work correctly > remoting::AudioPacket always uses 16-bit signed integers, while > AudioBus::WrapMemory() expects 32-bit FP (i.e. float) samples. I just realize this condition won't meet anyway. I will remove it. https://codereview.chromium.org/2903153004/diff/100001/remoting/protocol/audi... remoting/protocol/audio_pump.cc:46: reinterpret_cast<int16_t*>(const_cast<char*>(packet.data(0).data()))); On 2017/06/02 18:34:37, Sergey Ulanov wrote: > You can use string_as_array() from base/stl_util.h instead of the const_cast > const_cast<char*> Done. https://codereview.chromium.org/2903153004/diff/100001/remoting/protocol/audi... remoting/protocol/audio_pump.cc:62: sizeof(float) * sizeof(int16_t)); On 2017/06/02 18:34:36, Sergey Ulanov wrote: > I don't think we want to use AudioBus::CalculateMemorySize() here. This code is > correct only with the assumption that AudioBus uses float internally, and there > is no reason we need to depend on it here. "packet.channels() * packet.frames() > * AudioPacket::BYTES_PER_SAMPLE_2" would be simpler and clearer Done. https://codereview.chromium.org/2903153004/diff/100001/remoting/protocol/audi... remoting/protocol/audio_pump.cc:81: return media::CHANNEL_LAYOUT_QUAD; On 2017/06/02 18:34:36, Sergey Ulanov wrote: > I don't think that downmixing all 4-channel layouts as quadraphonic is > acceptable. In case the system is using 3.1 layout we would have center channel > mixed to the left and LFE mixed to the right, which may severely affect stereo > imaging (voice in the left ear, special effects in the right). > > My understanding from your previous comment was that you were suggesting to > treat all 4-speaker layouts as surround (i.e. L / R / C / BC). That would mix > the two additional channels to the left and the right channels equally, which is > acceptable even if the system uses quad layout. Done. https://codereview.chromium.org/2903153004/diff/100001/remoting/protocol/audi... remoting/protocol/audio_pump.cc:212: packet->bytes_per_sample() != AudioPacket::BYTES_PER_SAMPLE_2) { On 2017/06/02 18:34:36, Sergey Ulanov wrote: > I think all these checks can be DCHECKs I have removed packet->data(0).empty() check, it will be covered by AudioPacketToAudioBus() anyway. IMO, dchecking return value of an interface is not the right thing to do as I have replied in Joe's comment. p.s. The DCHECK(packet) in line 209 does not help in production: if by any reason an AudioCapturer returns nullptr on users' environment but cannot be produced on our test environment, the following logic will crash the service anyway. https://codereview.chromium.org/2903153004/diff/100001/remoting/protocol/audi... remoting/protocol/audio_pump.cc:216: if (input_layout == media::CHANNEL_LAYOUT_UNSUPPORTED || On 2017/06/02 18:34:37, Sergey Ulanov wrote: > I think in case the layout is unsupported it doesn't make sense to continue with > the packet. Encoder won't be able to handle it. Return nullptr in that case? Done. But IMO, unsupported by media::ChannelMixer does not need to mean it cannot be supported by Encoder: Encoder does not use media::CHANNEL_LAYOUT at all. https://codereview.chromium.org/2903153004/diff/100001/remoting/protocol/audi... remoting/protocol/audio_pump.cc:222: if (!mixer_ || last_input_layout_ != input_layout) { On 2017/06/02 18:34:36, Sergey Ulanov wrote: > It's better to move this code above mono/stereo check. Otherwise > last_input_layout_ won't be set to MONO/STEREO, which is confusing. If the input_layout is mono or stereo, we won't need to initialize |mixer_|. Maybe I should change |last_input_layout_| into |mixer_input_layout_|. https://codereview.chromium.org/2903153004/diff/100001/remoting/protocol/audi... remoting/protocol/audio_pump.cc:230: return packet; On 2017/06/02 18:34:36, Sergey Ulanov wrote: > Why would AudioPacketToAudioBus() fail? If it does fail, do we want pass the > packet to the encoder anyway? A corrupted data without a frame. https://codereview.chromium.org/2903153004/diff/100001/remoting/protocol/audi... remoting/protocol/audio_pump.cc:240: result->set_channels(AudioPacket::CHANNELS_STEREO); On 2017/06/02 18:34:37, Sergey Ulanov wrote: > I think it would be best to set all these properties in AudioBusToAudioPacket(). > Otherwise it looks wrong that AudioBusToAudioPacket() copies data, but doesn't > set other properties. There is no sampling rate in AudioBus, so I leave set_sampling_rate() still in this function.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2903153004/diff/20001/remoting/host/audio_cap... File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/2903153004/diff/20001/remoting/host/audio_cap... remoting/host/audio_capturer_win.cc:42: int ChooseChannels(int channel_count) { On 2017/05/28 20:58:58, Hzj_jie wrote: > On 2017/05/26 22:24:45, Sergey Ulanov wrote: > > If I understand correctly this function defines channel configuration that the > > request from a system. But if the channel count is not one of the supported > set, > > does it still make sense to request STEREO? Most likely it will fail anyway - > we > > saw that Windows doesn't downmix captured streams, so I'm not sure it's worth > > trying initialize the stream anyway. > IMO, it's still worthy to have a shot: it won't waste too many resources as only > one more function call is needed but it ensures no regression will ever be > triggered. I still don't think this is necessary. The problem is not any resources being wasted, but that this logic makes this code more complicated, while we don't know if it will ever make any difference. https://codereview.chromium.org/2903153004/diff/20001/remoting/host/audio_cap... remoting/host/audio_capturer_win.cc:204: ChooseChannels(wave_format_extensible->Format.nChannels); On 2017/05/31 02:49:28, Hzj_jie wrote: > On 2017/05/31 00:44:43, Sergey Ulanov wrote: > > On 2017/05/31 00:12:08, Hzj_jie wrote: > > > On 2017/05/30 19:18:19, Sergey Ulanov wrote: > > > > My point was that, if the API will never downmix the stream, then it makes > > no > > > > sense to set nChannels to 2 here. > > > > If changing nChannels is useful, then you also want to change > > > > wave_format_extensible->dwChannelMask. > > > > > > Got you. I will update both nChannels and dwChannelMask. > > > > Is this useful given that we know that windows will not downmix the stream > > anyway? I think it's better to just return error here when nChannels is > outside > > of the supported range instead of trying to initialize in stereo. > > Trying to initialize in stereo does not really waste too much resources, and > because of the hidden source character of Windows, we cannot predict what will > happen if we simply drop the initialization attempt here. I don't think it can cause any problems. It's reasonable for an application to drop the object without calling Initialize() after it has called GetMixFormat() and determined that it can't support the format. > I tend to avoid changes to existing logic path when handling edge cases. In this change you are changing how multi-channel layouts are handled. It seems strange to change it only for 1-8 channel layouts, but keep the old behavior for layouts with more than 8 channels, even though we know it won't work properly. Also in general we want to optimize for readability and simplicity of the code overall, not for simplicity of each individual CL. https://codereview.chromium.org/2903153004/diff/100001/remoting/protocol/audi... File remoting/protocol/audio_pump.cc (right): https://codereview.chromium.org/2903153004/diff/100001/remoting/protocol/audi... remoting/protocol/audio_pump.cc:35: if (frame_count < 1) { On 2017/06/02 22:11:25, Hzj_jie wrote: > On 2017/06/02 18:34:36, Sergey Ulanov wrote: > > packet.data(0).is_empty() > > (frame_count == 0) != (packet.data(0).empty()) > Considering a crappy AudioCapturer returns two channels frames without enough > data: data(0).size() < 1 * packet.channels() * packet.bytes_per_sample(). Then this should be a DCHECK. We own AudioCapturer and if there is any scenario in which any of the capturers returns fractional invalid frames it should be fixed in the capturer instead of working around it here. If you are concerned about partial frames returned by the capturer, then this should be a broader DCHECK, e.g.: DCHECK(packet.data(0).size() % (packet.channels() * packet.bytes_per_sample()) == 0); If the problem is caused by system API it should be handled at the capturer layer, not here. https://codereview.chromium.org/2903153004/diff/120001/remoting/host/audio_ca... File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/2903153004/diff/120001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:300: packet->set_channels(static_cast<AudioPacket::Channels>( I think this would be the right place to add a comment that we only take into account number of channels, but ignore speaker layout. Also add a TODO to fix it, taking into account dwChannelMask.
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-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
https://codereview.chromium.org/2903153004/diff/20001/remoting/host/audio_cap... File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/2903153004/diff/20001/remoting/host/audio_cap... remoting/host/audio_capturer_win.cc:42: int ChooseChannels(int channel_count) { On 2017/06/05 18:03:19, Sergey Ulanov wrote: > On 2017/05/28 20:58:58, Hzj_jie wrote: > > On 2017/05/26 22:24:45, Sergey Ulanov wrote: > > > If I understand correctly this function defines channel configuration that > the > > > request from a system. But if the channel count is not one of the supported > > set, > > > does it still make sense to request STEREO? Most likely it will fail anyway > - > > we > > > saw that Windows doesn't downmix captured streams, so I'm not sure it's > worth > > > trying initialize the stream anyway. > > IMO, it's still worthy to have a shot: it won't waste too many resources as > only > > one more function call is needed but it ensures no regression will ever be > > triggered. > > I still don't think this is necessary. The problem is not any resources being > wasted, but that this logic makes this code more complicated, while we don't > know if it will ever make any difference. > Then I would prefer to rewrite the wave_format_ex_->wFormatTag switch in Initialize() function: fast-return here indeed makes the logic more complex. https://codereview.chromium.org/2903153004/diff/20001/remoting/host/audio_cap... remoting/host/audio_capturer_win.cc:204: ChooseChannels(wave_format_extensible->Format.nChannels); On 2017/06/05 18:03:19, Sergey Ulanov wrote: > On 2017/05/31 02:49:28, Hzj_jie wrote: > > On 2017/05/31 00:44:43, Sergey Ulanov wrote: > > > On 2017/05/31 00:12:08, Hzj_jie wrote: > > > > On 2017/05/30 19:18:19, Sergey Ulanov wrote: > > > > > My point was that, if the API will never downmix the stream, then it > makes > > > no > > > > > sense to set nChannels to 2 here. > > > > > If changing nChannels is useful, then you also want to change > > > > > wave_format_extensible->dwChannelMask. > > > > > > > > Got you. I will update both nChannels and dwChannelMask. > > > > > > Is this useful given that we know that windows will not downmix the stream > > > anyway? I think it's better to just return error here when nChannels is > > outside > > > of the supported range instead of trying to initialize in stereo. > > > > Trying to initialize in stereo does not really waste too much resources, and > > because of the hidden source character of Windows, we cannot predict what will > > happen if we simply drop the initialization attempt here. > > I don't think it can cause any problems. It's reasonable for an application to > drop the object without calling Initialize() after it has called GetMixFormat() > and determined that it can't support the format. > > > > I tend to avoid changes to existing logic path when handling edge cases. > > In this change you are changing how multi-channel layouts are handled. It seems > strange to change it only for 1-8 channel layouts, but keep the old behavior for > layouts with more than 8 channels, even though we know it won't work properly. > Also in general we want to optimize for readability and simplicity of the code > overall, not for simplicity of each individual CL. Done. https://codereview.chromium.org/2903153004/diff/100001/remoting/protocol/audi... File remoting/protocol/audio_pump.cc (right): https://codereview.chromium.org/2903153004/diff/100001/remoting/protocol/audi... remoting/protocol/audio_pump.cc:35: if (frame_count < 1) { On 2017/06/05 18:03:19, Sergey Ulanov wrote: > On 2017/06/02 22:11:25, Hzj_jie wrote: > > On 2017/06/02 18:34:36, Sergey Ulanov wrote: > > > packet.data(0).is_empty() > > > > (frame_count == 0) != (packet.data(0).empty()) > > Considering a crappy AudioCapturer returns two channels frames without enough > > data: data(0).size() < 1 * packet.channels() * packet.bytes_per_sample(). > > Then this should be a DCHECK. We own AudioCapturer and if there is any scenario > in which any of the capturers returns fractional invalid frames it should be > fixed in the capturer instead of working around it here. If you are concerned > about partial frames returned by the capturer, then this should be a broader > DCHECK, e.g.: > DCHECK(packet.data(0).size() % (packet.channels() * packet.bytes_per_sample()) > == 0); > > If the problem is caused by system API it should be handled at the capturer > layer, not here. I still doubt this decision, especially the DCHECK() takes no effect on users' machines, and may cause unexpected behavior. https://codereview.chromium.org/2903153004/diff/120001/remoting/host/audio_ca... File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/2903153004/diff/120001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:300: packet->set_channels(static_cast<AudioPacket::Channels>( On 2017/06/05 18:03:19, Sergey Ulanov wrote: > I think this would be the right place to add a comment that we only take into > account number of channels, but ignore speaker layout. Also add a TODO to fix > it, taking into account dwChannelMask. Done.
lgtm when my comments are addressed https://codereview.chromium.org/2903153004/diff/100001/remoting/protocol/audi... File remoting/protocol/audio_pump.cc (right): https://codereview.chromium.org/2903153004/diff/100001/remoting/protocol/audi... remoting/protocol/audio_pump.cc:35: if (frame_count < 1) { On 2017/06/06 03:21:42, Hzj_jie wrote: > I still doubt this decision, especially the DCHECK() takes no effect on users' > machines, and may cause unexpected behavior. Do you have an examples when that would make a difference? https://codereview.chromium.org/2903153004/diff/140001/remoting/protocol/audi... File remoting/protocol/audio_pump.cc (right): https://codereview.chromium.org/2903153004/diff/140001/remoting/protocol/audi... remoting/protocol/audio_pump.cc:177: if (packet->channels() > AudioPacket::CHANNELS_STEREO) { Do we need this check given that Downmix() already handles mono and stereo packets? https://codereview.chromium.org/2903153004/diff/140001/remoting/protocol/audi... remoting/protocol/audio_pump.cc:178: packet = Downmix(std::move(packet)); Downmix() currently may return nullptr, but this case isn't handled here. Looking at Downmix() I don't think will actually ever fail. The input_layout=media::CHANNEL_LAYOUT_UNSUPPORTED case can be replaced with a DCHECK (we never expect a packet with unsupported layout). AudioPacketToAudioBus() can never return nullptr, so that check can be cleaned up as well. https://codereview.chromium.org/2903153004/diff/140001/remoting/protocol/audi... remoting/protocol/audio_pump.cc:185: if (!encoded_packet) please add {} here for consistency
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: This issue passed the CQ dry run.
https://codereview.chromium.org/2903153004/diff/100001/remoting/protocol/audi... File remoting/protocol/audio_pump.cc (right): https://codereview.chromium.org/2903153004/diff/100001/remoting/protocol/audi... remoting/protocol/audio_pump.cc:35: if (frame_count < 1) { On 2017/06/07 06:24:59, Sergey Ulanov wrote: > On 2017/06/06 03:21:42, Hzj_jie wrote: > > I still doubt this decision, especially the DCHECK() takes no effect on users' > > machines, and may cause unexpected behavior. > > Do you have an examples when that would make a difference? IMO, changing the following or similar pattern: ======== DCHECK(p.IsInValidState()); p.DoSomething(); ======== into ======== DCHECK(p.IsInValidState()); if (!p.IsInValidState()) { return; } p.DoSomething(); ======== or ======== CHECK(p.IsInValidState()); p.DoSomething(); ======== would be safer. Considering if DoSomething() is an operation to modify important users' data: working in invalid state may break the entire data set, using DCHECK() here won't actually help on users' environments. https://codereview.chromium.org/2903153004/diff/140001/remoting/protocol/audi... File remoting/protocol/audio_pump.cc (right): https://codereview.chromium.org/2903153004/diff/140001/remoting/protocol/audi... remoting/protocol/audio_pump.cc:177: if (packet->channels() > AudioPacket::CHANNELS_STEREO) { On 2017/06/07 06:24:59, Sergey Ulanov wrote: > Do we need this check given that Downmix() already handles mono and stereo > packets? Done. I have removed the if-condition in Downmix() function. https://codereview.chromium.org/2903153004/diff/140001/remoting/protocol/audi... remoting/protocol/audio_pump.cc:178: packet = Downmix(std::move(packet)); On 2017/06/07 06:24:59, Sergey Ulanov wrote: > Downmix() currently may return nullptr, but this case isn't handled here. > Looking at Downmix() I don't think will actually ever fail. The > input_layout=media::CHANNEL_LAYOUT_UNSUPPORTED case can be replaced with a > DCHECK (we never expect a packet with unsupported layout). > AudioPacketToAudioBus() can never return nullptr, so that check can be cleaned > up as well. Done. https://codereview.chromium.org/2903153004/diff/140001/remoting/protocol/audi... remoting/protocol/audio_pump.cc:185: if (!encoded_packet) On 2017/06/07 06:24:59, Sergey Ulanov wrote: > please add {} here for consistency Done.
https://codereview.chromium.org/2903153004/diff/100001/remoting/protocol/audi... File remoting/protocol/audio_pump.cc (right): https://codereview.chromium.org/2903153004/diff/100001/remoting/protocol/audi... remoting/protocol/audio_pump.cc:35: if (frame_count < 1) { On 2017/06/07 20:27:18, Hzj_jie wrote: > On 2017/06/07 06:24:59, Sergey Ulanov wrote: > > On 2017/06/06 03:21:42, Hzj_jie wrote: > > > I still doubt this decision, especially the DCHECK() takes no effect on > users' > > > machines, and may cause unexpected behavior. > > > > Do you have an examples when that would make a difference? > > IMO, changing the following or similar pattern: > ======== > DCHECK(p.IsInValidState()); > p.DoSomething(); > ======== > into > ======== > DCHECK(p.IsInValidState()); > if (!p.IsInValidState()) { return; } > p.DoSomething(); > ======== > or > ======== > CHECK(p.IsInValidState()); > p.DoSomething(); > ======== > would be safer. See https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... . It addressed both of your suggestions. CHECK() may be appropriate when violation of the value would result in security vulnerability. I don't think this is one of these cases. > > Considering if DoSomething() is an operation to modify important users' data: > working in invalid state may break the entire data set, using DCHECK() here > won't actually help on users' environments. There are million things that can go wrong. This CHECK() can't guarantee that we are in a valid state and that DoSomething() will work correctly.
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/2903153004/#ps160001 (title: "Resolve review comments")
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
zijiehe@chromium.org changed reviewers: + chcunningham@chromium.org
Chris, Chrome Remote Desktop now uses media::ChannelMixer to downmix 2+ channels into stereo, and depends on media/base. Would you please have a look at this change?
chcunningham@chromium.org changed reviewers: + dalecurtis@chromium.org
Some questions, mostly about the mask. I'm not sure how busted things will be. Have you tested locally? How does it sound? https://codereview.chromium.org/2903153004/diff/160001/remoting/host/audio_ca... File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/2903153004/diff/160001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:276: // consider dwChannelMask. as-is, what happens if you have a some mask (that you don't consider)? Will the captured packets just be padded with empty bytes for channels not in the mask? I'm reading the docs for the first time, but It seems like you may only get data for the masked channels without padding. And the order of the channels appears to be dependent on the mask. Internally, media::ChannelMixer assumes the orders will match those from FFmpeg. Its hard coded here. https://cs.chromium.org/chromium/src/media/base/channel_layout.cc?type=cs&sq=... Seems like things could go pretty wrong if we fail to consider the mask and reconcile the order with that from ffmpeg. I don't know how often this will bite you. Like, how often is a mask provided... How standard is ffmpeg's ordering... +DaleCurtis to advise. https://codereview.chromium.org/2903153004/diff/160001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:277: // TODO(zijiehe): Support also layouts. Not sure what this TODO means. Do you mean "Support masked layouts"? https://codereview.chromium.org/2903153004/diff/160001/remoting/protocol/audi... File remoting/protocol/audio_pump.cc (right): https://codereview.chromium.org/2903153004/diff/160001/remoting/protocol/audi... remoting/protocol/audio_pump.cc:29: return packet.data(0).size() / packet.channels() / packet.bytes_per_sample(); I think this calculation could go sideways if you have a channel mask. My read of the API docs are they wont send you data for channels that aren't in the mask.
https://codereview.chromium.org/2903153004/diff/160001/remoting/host/audio_ca... File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/2903153004/diff/160001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:276: // consider dwChannelMask. On 2017/06/08 at 16:39:45, chcunningham wrote: > as-is, what happens if you have a some mask (that you don't consider)? Will the captured packets just be padded with empty bytes for channels not in the mask? > > I'm reading the docs for the first time, but It seems like you may only get data for the masked channels without padding. And the order of the channels appears to be dependent on the mask. Internally, media::ChannelMixer assumes the orders will match those from FFmpeg. Its hard coded here. > > https://cs.chromium.org/chromium/src/media/base/channel_layout.cc?type=cs&sq=... > > Seems like things could go pretty wrong if we fail to consider the mask and reconcile the order with that from ffmpeg. I don't know how often this will bite you. Like, how often is a mask provided... How standard is ffmpeg's ordering... > > +DaleCurtis to advise. I don't remember the details around channel masks, but we do have code for converting to media::ChannelLayout here https://cs.chromium.org/chromium/src/media/audio/win/core_audio_util_win.cc?l=39
cc:tommi in case he wants to chime in with any advice on Windows input capture and the dwChannelMask. Also he wrote code to handle similar channel conversion in our input driver for windows when we can't open at the requested format: https://cs.chromium.org/chromium/src/media/audio/win/audio_low_latency_input_...
https://codereview.chromium.org/2903153004/diff/160001/remoting/proto/audio.p... File remoting/proto/audio.proto (right): https://codereview.chromium.org/2903153004/diff/160001/remoting/proto/audio.p... remoting/proto/audio.proto:50: CHANNELS_SURROUND = 3; what about 2.1?
Folks, please read my and Zijie's comments first. The gist of these comments: 1. we want to downmix multi-channel audio to stereo to stream from chromoting host to the client. 2. to avoid significant refactoring and complicating this CL we decided to ignore speaker layout and only take into account speaker count. It's not ideal, but there are only few cases when this approach makes difference and the current behavior will be acceptable (e.g. in case of quad layout the two back channels will be mixed to both front channels). 3. In future we plan to refactor AudioCapturer interface to use media::AudioBus instead of remoting::AudioPacket. When this happens it will be easier to propagate speaker layout from the capturer to the mixer. https://codereview.chromium.org/2903153004/diff/160001/remoting/host/audio_ca... File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/2903153004/diff/160001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:276: // consider dwChannelMask. On 2017/06/08 16:39:45, chcunningham wrote: > as-is, what happens if you have a some mask (that you don't consider)? Will the > captured packets just be padded with empty bytes for channels not in the mask? We do take into account nChannels, and number of bits set in dwChannelMask should be equal nChannels. > > I'm reading the docs for the first time, but It seems like you may only get data > for the masked channels without padding. And the order of the channels appears > to be dependent on the mask. Internally, media::ChannelMixer assumes the orders > will match those from FFmpeg. Its hard coded here. This order matches the order used by windows, see https://msdn.microsoft.com/en-us/library/windows/hardware/ff538802(v=vs.85).aspx > > https://cs.chromium.org/chromium/src/media/base/channel_layout.cc?type=cs&sq=... > > Seems like things could go pretty wrong if we fail to consider the mask and > reconcile the order with that from ffmpeg. I don't know how often this will bite > you. Like, how often is a mask provided... How standard is ffmpeg's ordering... ffmpeg's channel order matches channel order used by Windows as far as I can tell. Please let me know if I'm wrong. > > +DaleCurtis to advise.
On 2017/06/08 17:09:46, DaleCurtis wrote: > cc:tommi in case he wants to chime in with any advice on Windows input capture > and the dwChannelMask. Also he wrote code to handle similar channel conversion > in our input driver for windows when we can't open at the requested format: > > https://cs.chromium.org/chromium/src/media/audio/win/audio_low_latency_input_... This looks we have lots of duplicate logic in remoting/host/audio_capturer*. Sergey, should we migrate to the implementations of media? An immediate benefit is the capturer on Mac (https://cs.chromium.org/chromium/src/media/audio/mac/audio_low_latency_input_...).
https://codereview.chromium.org/2903153004/diff/160001/remoting/host/audio_ca... File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/2903153004/diff/160001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:276: // consider dwChannelMask. On 2017/06/08 17:05:47, DaleCurtis wrote: > On 2017/06/08 at 16:39:45, chcunningham wrote: > > as-is, what happens if you have a some mask (that you don't consider)? Will > the captured packets just be padded with empty bytes for channels not in the > mask? > > > > I'm reading the docs for the first time, but It seems like you may only get > data for the masked channels without padding. And the order of the channels > appears to be dependent on the mask. Internally, media::ChannelMixer assumes the > orders will match those from FFmpeg. Its hard coded here. > > > > > https://cs.chromium.org/chromium/src/media/base/channel_layout.cc?type=cs&sq=... > > > > Seems like things could go pretty wrong if we fail to consider the mask and > reconcile the order with that from ffmpeg. I don't know how often this will bite > you. Like, how often is a mask provided... How standard is ffmpeg's ordering... > > > > +DaleCurtis to advise. > > I don't remember the details around channel masks, but we do have code for > converting to media::ChannelLayout here > https://cs.chromium.org/chromium/src/media/audio/win/core_audio_util_win.cc?l=39 Thank you for the suggestion, but we do not always have dwChannelMask. I have no idea when will WAVE_FORMAT_EXTENSIBLE be returned, but we still need to support WAVE_FORMAT_PCM and WAVE_FORMAT_IEEE_FLOAT. Meanwhile by using dwChannelMask for WAVE_FORMAT_EXTENSIBLE would make the logic more complex, and I believe in most of the cases it does not make any difference. https://codereview.chromium.org/2903153004/diff/160001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:277: // TODO(zijiehe): Support also layouts. On 2017/06/08 16:39:45, chcunningham wrote: > Not sure what this TODO means. Do you mean "Support masked layouts"? Yes. Updated. https://codereview.chromium.org/2903153004/diff/160001/remoting/proto/audio.p... File remoting/proto/audio.proto (right): https://codereview.chromium.org/2903153004/diff/160001/remoting/proto/audio.p... remoting/proto/audio.proto:50: CHANNELS_SURROUND = 3; On 2017/06/08 17:58:07, tommi (sloooow) - chröme wrote: > what about 2.1? We do not support 2.1 for now: all 3 channels layout will be treated as surround. Because we always downmix the audio to stereo. AFAICT, downmixing 2.1 or surround to stereo makes very limited differences here. https://codereview.chromium.org/2903153004/diff/160001/remoting/protocol/audi... File remoting/protocol/audio_pump.cc (right): https://codereview.chromium.org/2903153004/diff/160001/remoting/protocol/audi... remoting/protocol/audio_pump.cc:29: return packet.data(0).size() / packet.channels() / packet.bytes_per_sample(); On 2017/06/08 16:39:45, chcunningham wrote: > I think this calculation could go sideways if you have a channel mask. My read > of the API docs are they wont send you data for channels that aren't in the > mask. We do not change the dwChannelMask returned by GetMixFormat(), so all channels should be returned from Windows. Do you mean this?
On 2017/06/08 16:39:45, chcunningham wrote: > Some questions, mostly about the mask. I'm not sure how busted things will be. > Have you tested locally? How does it sound? > > https://codereview.chromium.org/2903153004/diff/160001/remoting/host/audio_ca... > File remoting/host/audio_capturer_win.cc (right): > > https://codereview.chromium.org/2903153004/diff/160001/remoting/host/audio_ca... > remoting/host/audio_capturer_win.cc:276: // consider dwChannelMask. > as-is, what happens if you have a some mask (that you don't consider)? Will the > captured packets just be padded with empty bytes for channels not in the mask? > > I'm reading the docs for the first time, but It seems like you may only get data > for the masked channels without padding. And the order of the channels appears > to be dependent on the mask. Internally, media::ChannelMixer assumes the orders > will match those from FFmpeg. Its hard coded here. > > https://cs.chromium.org/chromium/src/media/base/channel_layout.cc?type=cs&sq=... > > Seems like things could go pretty wrong if we fail to consider the mask and > reconcile the order with that from ffmpeg. I don't know how often this will bite > you. Like, how often is a mask provided... How standard is ffmpeg's ordering... > > +DaleCurtis to advise. > > https://codereview.chromium.org/2903153004/diff/160001/remoting/host/audio_ca... > remoting/host/audio_capturer_win.cc:277: // TODO(zijiehe): Support also layouts. > Not sure what this TODO means. Do you mean "Support masked layouts"? > > https://codereview.chromium.org/2903153004/diff/160001/remoting/protocol/audi... > File remoting/protocol/audio_pump.cc (right): > > https://codereview.chromium.org/2903153004/diff/160001/remoting/protocol/audi... > remoting/protocol/audio_pump.cc:29: return packet.data(0).size() / > packet.channels() / packet.bytes_per_sample(); > I think this calculation could go sideways if you have a channel mask. My read > of the API docs are they wont send you data for channels that aren't in the > mask. Since we have downmixed the audio output, it's (personally) acceptable: at least I have not found the disorder of left / right channels.
LGTM > Folks, please read my and Zijie's comments first. Apologies. https://codereview.chromium.org/2903153004/diff/20001/remoting/host/audio_cap... File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/2903153004/diff/20001/remoting/host/audio_cap... remoting/host/audio_capturer_win.cc:42: int ChooseChannels(int channel_count) { This is fine for this CL, but we have seen more than 8 channels for people with elaborate setups (fancy mixers). Down the road you can introduce a notion of "discrete" layouts (basically a big shrug) and map that to media's notion for down mixing to stereo. https://codereview.chromium.org/2903153004/diff/160001/remoting/host/audio_ca... File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/2903153004/diff/160001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:276: // consider dwChannelMask. > We do take into account nChannels, and number of bits set in dwChannelMask > should be equal nChannels. > This is a key insight that I overlooked. I now appreciate that, in the worst case you may put something in the wrong ear, but at least you're not going to read out of bounds or anything. > ffmpeg's channel order matches channel order used by Windows as far as I can > tell. Please let me know if I'm wrong. You're right, looks the same to me.
https://codereview.chromium.org/2903153004/diff/20001/remoting/host/audio_cap... File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/2903153004/diff/20001/remoting/host/audio_cap... remoting/host/audio_capturer_win.cc:42: int ChooseChannels(int channel_count) { On 2017/06/09 19:25:28, chcunningham wrote: > This is fine for this CL, but we have seen more than 8 channels for people with > elaborate setups (fancy mixers). Down the road you can introduce a notion of > "discrete" layouts (basically a big shrug) and map that to media's notion for > down mixing to stereo. IMO, a long-term solution is to migrate to AudioInputStream, so we won't need to care about these details any more :) https://codereview.chromium.org/2903153004/diff/160001/remoting/host/audio_ca... File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/2903153004/diff/160001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:276: // consider dwChannelMask. On 2017/06/09 19:25:28, chcunningham wrote: > > We do take into account nChannels, and number of bits set in dwChannelMask > > should be equal nChannels. > > > > This is a key insight that I overlooked. I now appreciate that, in the worst > case you may put something in the wrong ear, but at least you're not going to > read out of bounds or anything. > > > ffmpeg's channel order matches channel order used by Windows as far as I can > > tell. Please let me know if I'm wrong. > > You're right, looks the same to me. Done.
The CQ bit was checked by zijiehe@chromium.org
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: linux_chromium_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
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": 160001, "attempt_start_ts": 1497054194752760,
"parent_rev": "281c950299514c5bb47241eec0e5892a8dc0d6cf", "commit_rev":
"3b1a63bcced72479291504d57846dbce721f3e67"}
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1497054194752760,
"parent_rev": "742eb78657ac9d110f591624bc096e83b304681b", "commit_rev":
"c331598675bb5e25fd8bf064d161e05e4b1b6477"}
Message was sent while issue was closed.
Description was changed from ========== [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 ========== to ========== [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/+/c331598675bb5e25fd8bf064d161... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:160001) as https://chromium.googlesource.com/chromium/src/+/c331598675bb5e25fd8bf064d161... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
