Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2712743004:
Support 4 channel mic in Windows Core Audio (Closed)
|
Created:
3 years, 10 months ago by jens.nielsen Modified:
3 years, 9 months ago CC:
webrtc-reviews_webrtc.org, yujie_mao (webrtc), Andrew MacDonald, tlegrand-webrtc, tterriberry_mozilla.com, audio-team_agora.io, qiang.lu, niklas.enbom, sdk-team_agora.io, peah-webrtc, minyue-webrtc, the sun, aleloi Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionSupport 4 channel mic in Windows Core Audio
BUG=webrtc:7220
Review-Url: https://codereview.webrtc.org/2712743004
Cr-Commit-Position: refs/heads/master@{#16940}
Committed: https://chromium.googlesource.com/external/webrtc/+/228c268065bd74d8fc8bd92681d8f375a9f5781a
Patch Set 1 #Patch Set 2 : Fix formatting #
Total comments: 53
Patch Set 3 : Changes after code review comments #
Total comments: 4
Patch Set 4 : Fix compile error on mac #
Total comments: 2
Patch Set 5 : Follow up last comments and more cleanup #Patch Set 6 : Fix compile error on linux/mac #Patch Set 7 : Fix another non-Windows build error #
Total comments: 2
Messages
Total messages: 53 (24 generated)
Description was changed from ========== Support 4 channel mic in Windows Core Audio BUG=webrtc:7220 ========== to ========== Support 4 channel mic in Windows Core Audio BUG=webrtc:7220 ==========
jens.nielsen@berotec.se changed reviewers: + henrika@webrtc.org, solenberg@webrtc.org
Hi Patch for supporting 4 channel microphone on Windows ready for review Best regards Jens https://codereview.webrtc.org/2712743004/diff/20001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/win/audio_device_core_win.cc (right): https://codereview.webrtc.org/2712743004/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/win/audio_device_core_win.cc:2535: WAVEFORMATEXTENSIBLE Wfx = WAVEFORMATEXTENSIBLE(); Quite undocumented feature but from trial and error I get AUDCLNT_E_UNSUPPORTED_FORMAT from IsFormatSupported() even for supported multi-channel formats unless WAVEFORMATEXTENSIBLE is used https://codereview.webrtc.org/2712743004/diff/20001/webrtc/voice_engine/utili... File webrtc/voice_engine/utility.cc (right): https://codereview.webrtc.org/2712743004/diff/20001/webrtc/voice_engine/utili... webrtc/voice_engine/utility.cc:48: AudioFrameOperations::DownmixChannels( Maybe needs some error handling here for unsupported channels combinations but the old code also makes assumptions, and "Never fix more than one bug in each CL"
henrika@webrtc.org changed reviewers: + henrik.lundin@webrtc.org
+henrik.lundin for changes related to AudioFrame
Thanks for doing work in this area. I like it! Focusing on the Windows specific parts: The parts for WASAPI support in native WebRTC audio uses old code and can be hard to modify partly. If you now add support for multi-channel, the WAVEFORMATEXTENSIBLE structure is the way to go and I would prefer if it was used in all places to make it more clear that multi-channel support is added. Also, It feels odd that you don't use dwChannelMask in your code. Care to elaborate.
Thanks for the patch! See comments inline. https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi... File webrtc/audio/utility/audio_frame_operations.cc (right): https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi... webrtc/audio/utility/audio_frame_operations.cc:117: void AudioFrameOperations::QuadToMono(const int16_t* src_audio, The order of the methods in the .cc file and the .h file should be the same. https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi... webrtc/audio/utility/audio_frame_operations.cc:168: } I'd like to see some kind of DCHECK that the number of input and output channels are valid. One way to accomplish this is to add a return to each if statement, and then at the very bottom add RTC_NOTREACHED() << "src_channels: " << src_channels << ", dst_channels: " << dst_channels; https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi... webrtc/audio/utility/audio_frame_operations.cc:173: if (frame->num_channels_ >= dst_channels) { This is not needed. You are explicitly testing all valid combinations below, and return -1 if that fails. https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi... webrtc/audio/utility/audio_frame_operations.cc:183: } else { Delete the else statement and simply return -1 unconditionally. https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi... File webrtc/audio/utility/audio_frame_operations.h (right): https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi... webrtc/audio/utility/audio_frame_operations.h:79: // may point to the same buffer. Comment on which combinations of src_channels and dst_channels are supported. https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi... webrtc/audio/utility/audio_frame_operations.h:88: // downmix. Comment on which combinations of src_channels and dst_channels are supported. https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi... File webrtc/audio/utility/audio_frame_operations_unittest.cc (right): https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi... webrtc/audio/utility/audio_frame_operations_unittest.cc:30: void SetFrameData(AudioFrame* frame, int16_t ch1, int16_t ch2, Please, run "git cl format" on your patch. https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi... webrtc/audio/utility/audio_frame_operations_unittest.cc:178: SetFrameData(&frame_, 0); Let this be a separate TEST_F. This will also avoid the strange reuse of temp_frame and frame_. https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi... webrtc/audio/utility/audio_frame_operations_unittest.cc:219: AudioFrameOperations::QuadToStereo(temp_frame.data_, Let this be a separate TEST_F. https://codereview.webrtc.org/2712743004/diff/20001/webrtc/voice_engine/utili... File webrtc/voice_engine/utility.cc (right): https://codereview.webrtc.org/2712743004/diff/20001/webrtc/voice_engine/utili... webrtc/voice_engine/utility.cc:48: AudioFrameOperations::DownmixChannels( On 2017/02/23 16:29:18, jens.nielsen wrote: > Maybe needs some error handling here for unsupported channels combinations but > the old code also makes assumptions, and "Never fix more than one bug in each > CL" I think this leaves the field a little too open. I suggest you add some DCHECKs here. RTC_DCHECK(num_channels == 2 || num_channels == 4) << "num_channels: " << num_channels; RTC_DCHECK(dst_frame->num_channels_ == 1 || dst_frame->num_channels_ == 2) << "dst_frame->num_channels_: " << dst_frame->num_channels_; https://codereview.webrtc.org/2712743004/diff/20001/webrtc/voice_engine/utili... File webrtc/voice_engine/utility_unittest.cc (right): https://codereview.webrtc.org/2712743004/diff/20001/webrtc/voice_engine/utili... webrtc/voice_engine/utility_unittest.cc:245: src_channel < sizeof(kSrcChannels) / sizeof(*kSrcChannels); Use arraysize() from webrtc/base/arraysize.h.
aleloi@webrtc.org changed reviewers: + aleloi@webrtc.org
Nice work! Some drive-by comments. https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi... File webrtc/audio/utility/audio_frame_operations.cc (right): https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi... webrtc/audio/utility/audio_frame_operations.cc:122: src_audio[4 * i + 2] + src_audio[4 * i + 3]) >> 2; I think this can overflow. If so, can the sum be stored in a larger temporary variable? Or does the binary arithmetic work out anyway? https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi... webrtc/audio/utility/audio_frame_operations.cc:130: I think this is a good place to RTC_DCHECK_LE that samples_per_channel_ * 4 <= AudioFrame::kMaxDataSizeSamples. https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi... webrtc/audio/utility/audio_frame_operations.cc:142: dst_audio[i * 2 + 1] = (src_audio[4 * i + 2] + src_audio[4 * i + 3]) >> 1; I have concerns for overflows here as well. https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi... webrtc/audio/utility/audio_frame_operations.cc:150: Please add RTC_DCHECK_LE(samples_per_channel_ * 4, AudioFrame::kMaxDataSizeSamples) https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi... File webrtc/audio/utility/audio_frame_operations.h (right): https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi... webrtc/audio/utility/audio_frame_operations.h:62: int16_t* dst_audio); Add blank line https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi... webrtc/audio/utility/audio_frame_operations.h:72: int16_t* dst_audio); Add blank line https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi... webrtc/audio/utility/audio_frame_operations.h:74: // |num_channels_| is stereo. Shouldn't |num_channels_| be 4 here and not stereo? Or am I missing something? https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi... webrtc/audio/utility/audio_frame_operations.h:84: size_t dst_channels); Please move the dst_audio to the end. Quote from style guide: "When defining a function, parameter order is: inputs, then outputs." https://google.github.io/styleguide/cppguide.html#Function_Parameter_Ordering https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi... webrtc/audio/utility/audio_frame_operations.h:90: size_t dst_channels); Please swap function parameters here as well. |dst_channels| is input, |frame| is both, so it should be last. https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi... File webrtc/audio/utility/audio_frame_operations_unittest.cc (right): https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi... webrtc/audio/utility/audio_frame_operations_unittest.cc:31: int16_t ch3, int16_t ch4) { Please change parameter ordering: input params first, both input and output after. https://codereview.webrtc.org/2712743004/diff/20001/webrtc/voice_engine/utili... File webrtc/voice_engine/utility_unittest.cc (right): https://codereview.webrtc.org/2712743004/diff/20001/webrtc/voice_engine/utili... webrtc/voice_engine/utility_unittest.cc:78: float ch3, float ch4, int sample_rate_hz) { Please change param order. https://codereview.webrtc.org/2712743004/diff/20001/webrtc/voice_engine/utili... webrtc/voice_engine/utility_unittest.cc:82: frame->samples_per_channel_ = sample_rate_hz / 100; Please use rtc::CheckedDivExact<int>(sample_rate_hz, 100) from webrtc/base/checks.h
Thank you! I will upload a new patch set as soon as I have addressed your comments @henrika: Just to be clear, do you want me to change the other usages of WAVEFORMATEX to WAVEFORMATEXTENSIBLE as well in this CL or keep that as a separate work? Regarding dwChannelMask it's something I had in the back of my head that 0 is a special value that means basically "do whatever". (Maybe from this page, though it concerns playback, see "Details about dwChannelMask" section https://msdn.microsoft.com/en-us/library/windows/hardware/dn653308(v=vs.85).aspx ) It makes sense for a playback device but for capture it's a bit backward for the software to tell the device where its microphones are located... I'm pretty sure the order will be ignored anyway but just to be sure I could set it to 0x33 (left, right, left of center, right of center) just so the correct number of bits are set. https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi... File webrtc/audio/utility/audio_frame_operations.cc (right): https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi... webrtc/audio/utility/audio_frame_operations.cc:122: src_audio[4 * i + 2] + src_audio[4 * i + 3]) >> 2; On 2017/02/24 10:39:31, aleloi wrote: > I think this can overflow. If so, can the sum be stored in a larger temporary > variable? Or does the binary arithmetic work out anyway? I was too, and was about to fix that here as well as the StereoToMono case when I saw there's already a unit test to verify that it in fact does not overflow... In a spirit of "don't touch it if not needed" I just followed pattern here... :) https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi... File webrtc/audio/utility/audio_frame_operations_unittest.cc (right): https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi... webrtc/audio/utility/audio_frame_operations_unittest.cc:178: SetFrameData(&frame_, 0); On 2017/02/24 09:46:36, hlundin-webrtc wrote: > Let this be a separate TEST_F. This will also avoid the strange reuse of > temp_frame and frame_. Will do. Should I also change the StereoToMono and MonoToStereo tests this was copied from?
My recommendation is to start using WAVEFORMATEXTENSIBLE for the input side to make it more clear that we now support multi-channel and to also avoid error prone casts. I agree that usage of dwChannelMask is mainly for output. I was just curios about how it is used for input. No changes needed.
https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi... File webrtc/audio/utility/audio_frame_operations.cc (right): https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi... webrtc/audio/utility/audio_frame_operations.cc:122: src_audio[4 * i + 2] + src_audio[4 * i + 3]) >> 2; On 2017/02/24 11:11:18, jens.nielsen wrote: > On 2017/02/24 10:39:31, aleloi wrote: > > I think this can overflow. If so, can the sum be stored in a larger temporary > > variable? Or does the binary arithmetic work out anyway? > > I was too, and was about to fix that here as well as the StereoToMono case when > I saw there's already a unit test to verify that it in fact does not overflow... > In a spirit of "don't touch it if not needed" I just followed pattern here... :) I tried to look it up: http://en.cppreference.com/w/cpp/language/operator_arithmetic#Overflows When signed integer arithmetic operation overflows (the result does not fit in the result type), the behavior is undefined: it may wrap around according to the rules of the representation (typically 2's complement), it may trap on some platforms or due to compiler options (e.g. -ftrapv in GCC and Clang), or may be completely optimized out by the compiler. IIUC, this is undefined behavior. On the other hand, all the compilers seem to do integer promotion: https://godbolt.org/g/f0hiqu. Also, our undefined-behavior test bots don't catch it. But can we add a static_cast<int32_t> anyway? It will generate the same binary code, but I will sleep better at night :)
Hi all
Thank you again for your comments, I have uploaded a new patch set which should
address the comments so far.
I have one question about how to handle rebase though, the last time I worked
with gerrit we did git rebase to local master and then just squash/amend
everything into one commit before pushing to gerrit. Your system is different,
I've figured out my local branch is tracking origin/master and (I hope) I'm
supposed to do git pull to merge, I did this now (I was pretty far behind) and
this gave me a new commit ("Merge branch 'master'..."), am I supposed to do
something with this or is the rest handled by git cl upload? Sorry if this is a
stupid question but this is at a stage where I could potentially wreck the
commit if I do it wrong... :)
Best regards
Jens
https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi...
File webrtc/audio/utility/audio_frame_operations.cc (right):
https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi...
webrtc/audio/utility/audio_frame_operations.cc:117: void
AudioFrameOperations::QuadToMono(const int16_t* src_audio,
On 2017/02/24 09:46:35, hlundin-webrtc wrote:
> The order of the methods in the .cc file and the .h file should be the same.
Done.
https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi...
webrtc/audio/utility/audio_frame_operations.cc:122: src_audio[4 * i + 2] +
src_audio[4 * i + 3]) >> 2;
On 2017/02/24 13:32:52, aleloi wrote:
> On 2017/02/24 11:11:18, jens.nielsen wrote:
> > On 2017/02/24 10:39:31, aleloi wrote:
> > > I think this can overflow. If so, can the sum be stored in a larger
> temporary
> > > variable? Or does the binary arithmetic work out anyway?
> >
> > I was too, and was about to fix that here as well as the StereoToMono case
> when
> > I saw there's already a unit test to verify that it in fact does not
> overflow...
> > In a spirit of "don't touch it if not needed" I just followed pattern
here...
> :)
>
> I tried to look it up:
> http://en.cppreference.com/w/cpp/language/operator_arithmetic#Overflows
>
> When signed integer arithmetic operation overflows (the result does not fit in
> the result type), the behavior is undefined: it may wrap around according to
the
> rules of the representation (typically 2's complement), it may trap on some
> platforms or due to compiler options (e.g. -ftrapv in GCC and Clang), or may
be
> completely optimized out by the compiler.
>
>
> IIUC, this is undefined behavior. On the other hand, all the compilers seem to
> do integer promotion: https://godbolt.org/g/f0hiqu. Also, our
undefined-behavior
> test bots don't catch it.
>
> But can we add a static_cast<int32_t> anyway? It will generate the same binary
> code, but I will sleep better at night :)
Done.
https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi...
webrtc/audio/utility/audio_frame_operations.cc:130:
On 2017/02/24 10:39:31, aleloi wrote:
> I think this is a good place to RTC_DCHECK_LE that samples_per_channel_ * 4 <=
> AudioFrame::kMaxDataSizeSamples.
Done. But the damage is already done here if the buffer is too small, this
should be checked when the 4 channel AudioFrame is created and/or written to
(afaik never used)
https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi...
webrtc/audio/utility/audio_frame_operations.cc:142: dst_audio[i * 2 + 1] =
(src_audio[4 * i + 2] + src_audio[4 * i + 3]) >> 1;
On 2017/02/24 10:39:31, aleloi wrote:
> I have concerns for overflows here as well.
Done.
https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi...
webrtc/audio/utility/audio_frame_operations.cc:150:
On 2017/02/24 10:39:31, aleloi wrote:
> Please add
>
> RTC_DCHECK_LE(samples_per_channel_ * 4, AudioFrame::kMaxDataSizeSamples)
Done.
https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi...
webrtc/audio/utility/audio_frame_operations.cc:168: }
On 2017/02/24 09:46:36, hlundin-webrtc wrote:
> I'd like to see some kind of DCHECK that the number of input and output
channels
> are valid. One way to accomplish this is to add a return to each if statement,
> and then at the very bottom add
> RTC_NOTREACHED() << "src_channels: " << src_channels << ", dst_channels: " <<
> dst_channels;
Done.
https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi...
webrtc/audio/utility/audio_frame_operations.cc:173: if (frame->num_channels_ >=
dst_channels) {
On 2017/02/24 09:46:35, hlundin-webrtc wrote:
> This is not needed. You are explicitly testing all valid combinations below,
and
> return -1 if that fails.
Done.
https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi...
webrtc/audio/utility/audio_frame_operations.cc:183: } else {
On 2017/02/24 09:46:36, hlundin-webrtc wrote:
> Delete the else statement and simply return -1 unconditionally.
Done.
https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi...
File webrtc/audio/utility/audio_frame_operations.h (right):
https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi...
webrtc/audio/utility/audio_frame_operations.h:62: int16_t* dst_audio);
On 2017/02/24 10:39:31, aleloi wrote:
> Add blank line
Done.
https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi...
webrtc/audio/utility/audio_frame_operations.h:72: int16_t* dst_audio);
On 2017/02/24 10:39:31, aleloi wrote:
> Add blank line
Done.
https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi...
webrtc/audio/utility/audio_frame_operations.h:74: // |num_channels_| is stereo.
On 2017/02/24 10:39:31, aleloi wrote:
> Shouldn't |num_channels_| be 4 here and not stereo? Or am I missing something?
Done. (Silly copy-paste error, sorry about that)
https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi...
webrtc/audio/utility/audio_frame_operations.h:79: // may point to the same
buffer.
On 2017/02/24 09:46:36, hlundin-webrtc wrote:
> Comment on which combinations of src_channels and dst_channels are supported.
Done.
https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi...
webrtc/audio/utility/audio_frame_operations.h:84: size_t dst_channels);
On 2017/02/24 10:39:31, aleloi wrote:
> Please move the dst_audio to the end.
>
> Quote from style guide: "When defining a function, parameter order is: inputs,
> then outputs."
>
> https://google.github.io/styleguide/cppguide.html#Function_Parameter_Ordering
Done.
https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi...
webrtc/audio/utility/audio_frame_operations.h:88: // downmix.
On 2017/02/24 09:46:36, hlundin-webrtc wrote:
> Comment on which combinations of src_channels and dst_channels are supported.
Done.
https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi...
webrtc/audio/utility/audio_frame_operations.h:90: size_t dst_channels);
On 2017/02/24 10:39:31, aleloi wrote:
> Please swap function parameters here as well. |dst_channels| is input, |frame|
> is both, so it should be last.
Done.
https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi...
File webrtc/audio/utility/audio_frame_operations_unittest.cc (right):
https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi...
webrtc/audio/utility/audio_frame_operations_unittest.cc:30: void
SetFrameData(AudioFrame* frame, int16_t ch1, int16_t ch2,
On 2017/02/24 09:46:36, hlundin-webrtc wrote:
> Please, run "git cl format" on your patch.
Done.
https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi...
webrtc/audio/utility/audio_frame_operations_unittest.cc:31: int16_t ch3, int16_t
ch4) {
On 2017/02/24 10:39:32, aleloi wrote:
> Please change parameter ordering: input params first, both input and output
> after.
Done.
https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi...
webrtc/audio/utility/audio_frame_operations_unittest.cc:178:
SetFrameData(&frame_, 0);
On 2017/02/24 09:46:36, hlundin-webrtc wrote:
> Let this be a separate TEST_F. This will also avoid the strange reuse of
> temp_frame and frame_.
Done.
https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi...
webrtc/audio/utility/audio_frame_operations_unittest.cc:219:
AudioFrameOperations::QuadToStereo(temp_frame.data_,
On 2017/02/24 09:46:36, hlundin-webrtc wrote:
> Let this be a separate TEST_F.
Done.
https://codereview.webrtc.org/2712743004/diff/20001/webrtc/voice_engine/utili...
File webrtc/voice_engine/utility.cc (right):
https://codereview.webrtc.org/2712743004/diff/20001/webrtc/voice_engine/utili...
webrtc/voice_engine/utility.cc:48: AudioFrameOperations::DownmixChannels(
On 2017/02/24 09:46:36, hlundin-webrtc wrote:
> On 2017/02/23 16:29:18, jens.nielsen wrote:
> > Maybe needs some error handling here for unsupported channels combinations
but
> > the old code also makes assumptions, and "Never fix more than one bug in
each
> > CL"
>
> I think this leaves the field a little too open. I suggest you add some
DCHECKs
> here.
> RTC_DCHECK(num_channels == 2 || num_channels == 4) << "num_channels: " <<
> num_channels;
> RTC_DCHECK(dst_frame->num_channels_ == 1 || dst_frame->num_channels_ == 2) <<
> "dst_frame->num_channels_: " << dst_frame->num_channels_;
Done.
https://codereview.webrtc.org/2712743004/diff/20001/webrtc/voice_engine/utili...
File webrtc/voice_engine/utility_unittest.cc (right):
https://codereview.webrtc.org/2712743004/diff/20001/webrtc/voice_engine/utili...
webrtc/voice_engine/utility_unittest.cc:78: float ch3, float ch4, int
sample_rate_hz) {
On 2017/02/24 10:39:32, aleloi wrote:
> Please change param order.
Done.
https://codereview.webrtc.org/2712743004/diff/20001/webrtc/voice_engine/utili...
webrtc/voice_engine/utility_unittest.cc:82: frame->samples_per_channel_ =
sample_rate_hz / 100;
On 2017/02/24 10:39:32, aleloi wrote:
> Please use rtc::CheckedDivExact<int>(sample_rate_hz, 100) from
> webrtc/base/checks.h
Done.
https://codereview.webrtc.org/2712743004/diff/20001/webrtc/voice_engine/utili...
webrtc/voice_engine/utility_unittest.cc:245: src_channel < sizeof(kSrcChannels)
/ sizeof(*kSrcChannels);
On 2017/02/24 09:46:36, hlundin-webrtc wrote:
> Use arraysize() from webrtc/base/arraysize.h.
Done.
It is not clear to me if you are in a bad state now or not. I always do git pull on my branch for a given CL and resolve any merge conflicts (usually there are none) before uploading again using git cl upload.
On 2017/02/24 15:17:07, henrika_webrtc wrote: > It is not clear to me if you are in a bad state now or not. > > I always do git pull on my branch for a given CL and resolve any merge conflicts > (usually there are none) > before uploading again using git cl upload. Simple solution: run a test bot and check if it gets merge problems. LG on the files I've checked with the new patch.
The Windows parts start to look OK. Can you also add some information in the bug on how you have verified the new support. For the record. https://codereview.webrtc.org/2712743004/diff/40001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/win/audio_device_core_win.cc (right): https://codereview.webrtc.org/2712743004/diff/40001/webrtc/modules/audio_devi... webrtc/modules/audio_device/win/audio_device_core_win.cc:2595: (WAVEFORMATEX*)&Wfx, You don't need this now, right? If so, please explain why? https://codereview.webrtc.org/2712743004/diff/40001/webrtc/modules/audio_devi... webrtc/modules/audio_device/win/audio_device_core_win.cc:2640: (WAVEFORMATEX*)&Wfx, // selected wave format dito
Hi again Sorry for silly copy-paste thing causing build error on mac, this should be fixed now. Also did git pull and re-ran all tests And bug report is updated with some details regarding tests https://codereview.webrtc.org/2712743004/diff/40001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/win/audio_device_core_win.cc (right): https://codereview.webrtc.org/2712743004/diff/40001/webrtc/modules/audio_devi... webrtc/modules/audio_device/win/audio_device_core_win.cc:2595: (WAVEFORMATEX*)&Wfx, On 2017/02/24 15:25:03, henrika_webrtc wrote: > You don't need this now, right? If so, please explain why? The signature for IsFormatSupported() (and Initialize()) still has a WAVEFORMATEX* even though we're actually passing a WAVEFORMATEXTENSIBLE*. So the cast is needed now but wasn't before...
The CQ bit was checked by jens.nielsen@berotec.se 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.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-webrtc-committers". Note that this has nothing to do with OWNERS files.
LGTM (with one comment) on webrtc/audio/utility/* and webrtc/voice_engine/*. Although I have no formal owner's rights there. henrika@ should have sufficient powers to approve all of your changes. FTR: jens.nielsen@berotec.se has signed the required CLA for contributing code. https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi... File webrtc/audio/utility/audio_frame_operations_unittest.cc (right): https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi... webrtc/audio/utility/audio_frame_operations_unittest.cc:178: SetFrameData(&frame_, 0); On 2017/02/24 11:11:19, jens.nielsen wrote: > On 2017/02/24 09:46:36, hlundin-webrtc wrote: > > Let this be a separate TEST_F. This will also avoid the strange reuse of > > temp_frame and frame_. > > Will do. Should I also change the StereoToMono and MonoToStereo tests this was > copied from? That is up to you. I think you are living by the Boy Scout Rule anyway, leaving the code better than you found it. But if you want to improve it even further... :) https://codereview.webrtc.org/2712743004/diff/60001/webrtc/voice_engine/utili... File webrtc/voice_engine/utility_unittest.cc (right): https://codereview.webrtc.org/2712743004/diff/60001/webrtc/voice_engine/utili... webrtc/voice_engine/utility_unittest.cc:261: for (int src_channel = 0; src_channel < arraysize(kSrcChannels); Please, also #include "webrtc/base/arraysize.h". Include what you use.
LGTM. Thanks for doing this!
https://codereview.webrtc.org/2712743004/diff/40001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/win/audio_device_core_win.cc (right): https://codereview.webrtc.org/2712743004/diff/40001/webrtc/modules/audio_devi... webrtc/modules/audio_device/win/audio_device_core_win.cc:2595: (WAVEFORMATEX*)&Wfx, On 2017/02/24 16:41:30, jens.nielsen wrote: > On 2017/02/24 15:25:03, henrika_webrtc wrote: > > You don't need this now, right? If so, please explain why? > > The signature for IsFormatSupported() (and Initialize()) still has a > WAVEFORMATEX* even though we're actually passing a WAVEFORMATEXTENSIBLE*. So the > cast is needed now but wasn't before... Acknowledged.
Hi all This should hopefully be the final patch set for this CL. Thanks again! /Jens https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi... File webrtc/audio/utility/audio_frame_operations_unittest.cc (right): https://codereview.webrtc.org/2712743004/diff/20001/webrtc/audio/utility/audi... webrtc/audio/utility/audio_frame_operations_unittest.cc:178: SetFrameData(&frame_, 0); On 2017/02/27 07:52:02, hlundin-webrtc wrote: > On 2017/02/24 11:11:19, jens.nielsen wrote: > > On 2017/02/24 09:46:36, hlundin-webrtc wrote: > > > Let this be a separate TEST_F. This will also avoid the strange reuse of > > > temp_frame and frame_. > > > > Will do. Should I also change the StereoToMono and MonoToStereo tests this was > > copied from? > > That is up to you. I think you are living by the Boy Scout Rule anyway, leaving > the code better than you found it. But if you want to improve it even further... > :) Alright I was reluctant to change too much in one CL, but I couldn't stay away and applied the comments on the functions I added also to the already existing code :) https://codereview.webrtc.org/2712743004/diff/60001/webrtc/voice_engine/utili... File webrtc/voice_engine/utility_unittest.cc (right): https://codereview.webrtc.org/2712743004/diff/60001/webrtc/voice_engine/utili... webrtc/voice_engine/utility_unittest.cc:261: for (int src_channel = 0; src_channel < arraysize(kSrcChannels); On 2017/02/27 07:52:03, hlundin-webrtc wrote: > Please, also #include "webrtc/base/arraysize.h". Include what you use. Done.
Thanks! Still LGTM. Let's give The Sun and Aleloi until tomorrow morning to comment.
aleloi@google.com changed reviewers: + aleloi@google.com
Oh, sorry. LGTM!
The CQ bit was checked by henrik.lundin@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrika@webrtc.org Link to the patchset: https://codereview.webrtc.org/2712743004/#ps80001 (title: "Follow up last comments and more cleanup")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/19406)
On 2017/03/01 10:12:39, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > mac_baremetal on master.tryserver.webrtc (JOB_FAILED, > http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/19406) You have compilation errors on the bots. Please, fix and upload new patch sets. Let me know if you are not allowed to view the bot results.
The CQ bit was checked by jens.nielsen@berotec.se 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.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...) ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/22182) linux_more_configs on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_more_configs/buil...) mac_asan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/22550) mac_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/19410)
The CQ bit was checked by jens.nielsen@berotec.se 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.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi Alright, new patch sets uploaded and now also dry run passed (didn't know I could start one myself) Maybe I just don't find my way around this build system but it would have been much easier if I had the equivalent of -WError on Windows as well, or at least see the warnings somewhere. AFAIK I should have had at least the signed comparison warning but couldn't see it anywhere Best regards Jens
On 2017/03/01 12:57:29, jens.nielsen wrote: > Hi > > Alright, new patch sets uploaded and now also dry run passed (didn't know I > could start one myself) > > Maybe I just don't find my way around this build system but it would have been > much easier if I had the equivalent of -WError on Windows as well, or at least > see the warnings somewhere. AFAIK I should have had at least the signed > comparison warning but couldn't see it anywhere > > Best regards > Jens Thanks for fixing! Yeah, I know very little about the Windows tool chain. All I know is that I'm glad we have all these trybots on different platforms, since it would be a pain beyond imaginable to have to test everything manually on all variants. :) I'll give it a new go with the commit queue.
The CQ bit was checked by henrik.lundin@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from aleloi@google.com, henrik.lundin@webrtc.org, henrika@webrtc.org Link to the patchset: https://codereview.webrtc.org/2712743004/#ps120001 (title: "Fix another non-Windows build error")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1488373700716930,
"parent_rev": "c5726c1579358260a7587ed05943f01dba506fa6", "commit_rev":
"228c268065bd74d8fc8bd92681d8f375a9f5781a"}
Message was sent while issue was closed.
Description was changed from ========== Support 4 channel mic in Windows Core Audio BUG=webrtc:7220 ========== to ========== Support 4 channel mic in Windows Core Audio BUG=webrtc:7220 Review-Url: https://codereview.webrtc.org/2712743004 Cr-Commit-Position: refs/heads/master@{#16940} Committed: https://chromium.googlesource.com/external/webrtc/+/228c268065bd74d8fc8bd9268... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/external/webrtc/+/228c268065bd74d8fc8bd9268...
Message was sent while issue was closed.
And finally it got through, thanks! Now all that's left is to close the bug report, I don't think I have access to do that or at least I don't see anything to click... https://bugs.chromium.org/p/webrtc/issues/detail?id=7220
Message was sent while issue was closed.
https://codereview.webrtc.org/2712743004/diff/120001/webrtc/voice_engine/util... File webrtc/voice_engine/utility.cc (right): https://codereview.webrtc.org/2712743004/diff/120001/webrtc/voice_engine/util... webrtc/voice_engine/utility.cc:44: int16_t downsampled_audio[AudioFrame::kMaxDataSizeSamples]; nit: this isn't downsampled, it is downmixed.
Message was sent while issue was closed.
https://codereview.webrtc.org/2712743004/diff/120001/webrtc/voice_engine/util... File webrtc/voice_engine/utility.cc (right): https://codereview.webrtc.org/2712743004/diff/120001/webrtc/voice_engine/util... webrtc/voice_engine/utility.cc:44: int16_t downsampled_audio[AudioFrame::kMaxDataSizeSamples]; On 2017/03/02 00:57:29, the sun wrote: > nit: this isn't downsampled, it is downmixed. Fixing this in https://codereview.webrtc.org/2721123005. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
