Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1753663002:
Add volume control for windows host (Closed)
DescriptionAdd volume control for windows host
BUG=276753
Committed: https://crrev.com/3ba26c009b0b2583562b695c426391f71b626f33
Cr-Commit-Position: refs/heads/master@{#380458}
Patch Set 1 #Patch Set 2 : Slightly refector logic, kBytesPerSample is not so useful, we cannot change it without impacting ot… #Patch Set 3 : fixup a warning (signed / unsigned mismatch) #
Total comments: 19
Patch Set 4 : Add IsMuted function to separate logic in DoCapture #Patch Set 5 : COMPILE_ASSERT is not supported by chromium codebase #Patch Set 6 : A typo #Patch Set 7 : Use static_assert, since this file is for windows only (we are using vs 2013) #Patch Set 8 : sync latest code #
Total comments: 23
Patch Set 9 : Resolve code review comments #
Total comments: 19
Patch Set 10 : Resolve review comments #
Total comments: 2
Patch Set 11 : Executing silence detector before applying volume #
Total comments: 4
Patch Set 12 : Do not call GetAudioLevel unless SilenceDetector does not mark the samples as silence #Patch Set 13 : Sync latest code #
Messages
Total messages: 22 (5 generated)
sergeyu@chromium.org changed reviewers: + sergeyu@chromium.org
https://codereview.chromium.org/1753663002/diff/40001/remoting/host/audio_cap... File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/1753663002/diff/40001/remoting/host/audio_cap... remoting/host/audio_capturer_win.cc:21: // Following logic expects kBytesPerSample always be 2. I don't think you need this comment here. See my suggestion below. https://codereview.chromium.org/1753663002/diff/40001/remoting/host/audio_cap... remoting/host/audio_capturer_win.cc:242: if (FAILED(audio_volume_->GetMute(&mute)) || !mute) { Suggest moving this code to a separate function to make it more readable. https://codereview.chromium.org/1753663002/diff/40001/remoting/host/audio_cap... remoting/host/audio_capturer_win.cc:245: level > 1 || level < -1)) { is the level allowed to be negative? https://codereview.chromium.org/1753663002/diff/40001/remoting/host/audio_cap... remoting/host/audio_capturer_win.cc:251: // the combination of one sample of two channels. I don't understand this comment. We always get data in 16-bit format, so samples[x] is always a whole sample from one of the channels. If the point is that this code assumes 16-bit encoding then maybe replace this comment with a compile assert, e.g.: COMPILE_ASSERT(kBytesPerSample==sizeof(samples[0]), expect_16_bits_per_sample). https://codereview.chromium.org/1753663002/diff/40001/remoting/host/audio_cap... remoting/host/audio_capturer_win.cc:255: * sizeof(BYTE) / sizeof(int16_t); nit: don't need sizeof(BYTE). kBytesPerSample already returns size in bytes. https://codereview.chromium.org/1753663002/diff/40001/remoting/host/audio_cap... remoting/host/audio_capturer_win.cc:260: samples[i] *= level; This would be many times faster if you avoid floating point arithmetic in the loop. E.g. int32_t level_int = static_cast<int>(level * 65536); for (size_t i = 0; i < sample_count; i++) { samples[i] = (static_cast<int32_t>(samples[i]) * level_int) >> 16; }
joedow@chromium.org changed reviewers: + joedow@chromium.org
https://codereview.chromium.org/1753663002/diff/40001/remoting/host/audio_cap... File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/1753663002/diff/40001/remoting/host/audio_cap... remoting/host/audio_capturer_win.cc:198: // Initialize ISampleAudioVolume. TODO(zijiehe): Do we need to control per s/ISampleAudioVolume/ISimpleAudioVolume. The TODO should be on the next line. https://codereview.chromium.org/1753663002/diff/40001/remoting/host/audio_cap... remoting/host/audio_capturer_win.cc:242: if (FAILED(audio_volume_->GetMute(&mute)) || !mute) { Agree with Sergey, also GetMute looks like it would fail for the same reasons as GetMasterVolume. If calling these APIs fails, do we want to default to 'full volume'? Or do we receive failed HRs for common scenarios such as headphones being unplugged? https://codereview.chromium.org/1753663002/diff/40001/remoting/host/audio_cap... remoting/host/audio_capturer_win.cc:246: level = 1; Should we default to 1 if the call above fails? The hresults returned generally point to audio hardware errors or the audio service not running so I wonder if 'full volume' makes more sense here.
I believe android_chromium_gn_compile_rel fatal totally does not relate to this CL, would you mind to confirm? https://codereview.chromium.org/1753663002/diff/40001/remoting/host/audio_cap... File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/1753663002/diff/40001/remoting/host/audio_cap... remoting/host/audio_capturer_win.cc:21: // Following logic expects kBytesPerSample always be 2. On 2016/03/01 21:48:17, Sergey Ulanov wrote: > I don't think you need this comment here. See my suggestion below. Done. https://codereview.chromium.org/1753663002/diff/40001/remoting/host/audio_cap... remoting/host/audio_capturer_win.cc:198: // Initialize ISampleAudioVolume. TODO(zijiehe): Do we need to control per On 2016/03/01 22:38:40, joedow wrote: > s/ISampleAudioVolume/ISimpleAudioVolume. > > The TODO should be on the next line. Done. https://codereview.chromium.org/1753663002/diff/40001/remoting/host/audio_cap... remoting/host/audio_capturer_win.cc:242: if (FAILED(audio_volume_->GetMute(&mute)) || !mute) { On 2016/03/01 22:38:40, joedow wrote: > Agree with Sergey, also GetMute looks like it would fail for the same reasons as > GetMasterVolume. If calling these APIs fails, do we want to default to 'full > volume'? Or do we receive failed HRs for common scenarios such as headphones > being unplugged? I just want to make sure this change won't break existing logic even something bad happened. https://codereview.chromium.org/1753663002/diff/40001/remoting/host/audio_cap... remoting/host/audio_capturer_win.cc:245: level > 1 || level < -1)) { On 2016/03/01 21:48:16, Sergey Ulanov wrote: > is the level allowed to be negative? No, the value should be [0.0, 1.0], but this check is to make sure an int16_t * level won't exceed int16_t. https://codereview.chromium.org/1753663002/diff/40001/remoting/host/audio_cap... remoting/host/audio_capturer_win.cc:246: level = 1; On 2016/03/01 22:38:40, joedow wrote: > Should we default to 1 if the call above fails? The hresults returned generally > point to audio hardware errors or the audio service not running so I wonder if > 'full volume' makes more sense here. The reason is same as GetMute call, i.e. make sure the behavior won't change even something bad happened. Currently the logic has been moved to IsMuted function, we can discuss if the principle is correct, and make necessary changes there. https://codereview.chromium.org/1753663002/diff/40001/remoting/host/audio_cap... remoting/host/audio_capturer_win.cc:251: // the combination of one sample of two channels. On 2016/03/01 21:48:16, Sergey Ulanov wrote: > I don't understand this comment. We always get data in 16-bit format, so > samples[x] is always a whole sample from one of the channels. If the point is > that this code assumes 16-bit encoding then maybe replace this comment with a > compile assert, e.g.: > COMPILE_ASSERT(kBytesPerSample==sizeof(samples[0]), > expect_16_bits_per_sample). Yes, 'We always get data in 16-bit format' is the assumption, i.e. kBytesPerSample must be 2, it's not changeable. This assumption impacts both AudioSilenceDetector and the logic here. I agree a compile assert should be better. https://codereview.chromium.org/1753663002/diff/40001/remoting/host/audio_cap... remoting/host/audio_capturer_win.cc:255: * sizeof(BYTE) / sizeof(int16_t); On 2016/03/01 21:48:16, Sergey Ulanov wrote: > nit: don't need sizeof(BYTE). kBytesPerSample already returns size in bytes. Though less possibility, what if sizeof(BYTE) != 1 :) https://codereview.chromium.org/1753663002/diff/40001/remoting/host/audio_cap... remoting/host/audio_capturer_win.cc:260: samples[i] *= level; On 2016/03/01 21:48:16, Sergey Ulanov wrote: > This would be many times faster if you avoid floating point arithmetic in the > loop. E.g. > > int32_t level_int = static_cast<int>(level * 65536); > for (size_t i = 0; i < sample_count; i++) { > samples[i] = (static_cast<int32_t>(samples[i]) * level_int) >> 16; > } Done. But the performance impact would not be many times.
https://codereview.chromium.org/1753663002/diff/140001/remoting/host/audio_ca... File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/1753663002/diff/140001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:216: bool AudioCapturerWin::IsMuted(float* level) { Couldn't this function return a float (just return 0.0f if the audio is muted) then you wouldn't need to pass a level pointer and you could rename the function GetAudioLevel() or similar? The current logic handles mute vs. 0 volume but I'm wondering if we actually want to send audio packets if the volume is 0.0 (i.e. should we treat mute and 0 volume the same?). https://codereview.chromium.org/1753663002/diff/140001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:224: if (FAILED(hr) || *level > 1 || *level < 0) { If the level is less than zero, should you clamp it to 0 instead of 1? https://codereview.chromium.org/1753663002/diff/140001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:225: *level = 1; Per my previous comment, I think the decision to default to full volume should be documented. Can you add a comment on why this is the right behavior. https://codereview.chromium.org/1753663002/diff/140001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:258: // BYTE to int16_t I don't think this comment adds much value, remove it? https://codereview.chromium.org/1753663002/diff/140001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:263: if (level < 1) { A comment might be good here to describe why we need to manually adjust the level of each sample. https://codereview.chromium.org/1753663002/diff/140001/remoting/host/audio_ca... File remoting/host/audio_capturer_win.h (right): https://codereview.chromium.org/1753663002/diff/140001/remoting/host/audio_ca... remoting/host/audio_capturer_win.h:38: bool IsMuted(float* level); It seems like this function is doing too much for its name, I wouldn't expect IsMuted to also set audio volume. It probably makes sense to break this into two function or rename it to indicate that it will determine the audio level.
https://codereview.chromium.org/1753663002/diff/40001/remoting/host/audio_cap... File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/1753663002/diff/40001/remoting/host/audio_cap... remoting/host/audio_capturer_win.cc:260: samples[i] *= level; On 2016/03/02 08:16:32, Hzj_jie wrote: > On 2016/03/01 21:48:16, Sergey Ulanov wrote: > > This would be many times faster if you avoid floating point arithmetic in the > > loop. E.g. > > > > int32_t level_int = static_cast<int>(level * 65536); > > for (size_t i = 0; i < sample_count; i++) { > > samples[i] = (static_cast<int32_t>(samples[i]) * level_int) >> 16; > > } > > Done. But the performance impact would not be many times. FWIW I see 2.5x-3x difference on ia64 and on ia32 with GCC: $ cat apply_volume.cc #include <array> #include <cstdint> #include <iostream> #include <time.h> int main() { const int kNumSamples = 100*1000; const int kIterations = 100; std::array<int16_t, kNumSamples> samples; float volume = 0.3; clock_t started; started = clock(); for (size_t j = 0; j < kIterations; ++j) { for (size_t i = 0; i < kNumSamples; ++i) { samples[i] *= volume; } } std::cerr << static_cast<double>(clock() - started) / CLOCKS_PER_SEC << std::endl; started = clock(); int32_t level_int = static_cast<int>(volume * 65536); for (size_t j = 0; j < kIterations; ++j) { for (size_t i = 0; i < kNumSamples; ++i) { samples[i] = (static_cast<int32_t>(samples[i]) * level_int) >> 16; } } std::cerr << static_cast<double>(clock() - started) / CLOCKS_PER_SEC << std::endl; int sum = 0; for (size_t i = 0; i < kNumSamples; ++i) { sum += samples[i]; } std::cerr << sum << std::endl; } $ g++ apply_volume.cc -std=c++11 -g -O2 $ ./a.out 0.037469 0.01429 0 $ g++ apply_volume.cc -std=c++11 -g -O2 -m32 $ ./a.out 0.034593 0.012743 0 It's not that floating point multiplication is slow (it may be even faster than int). It's int->float->int conversion that kills it. https://codereview.chromium.org/1753663002/diff/140001/remoting/host/audio_ca... File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/1753663002/diff/140001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:216: bool AudioCapturerWin::IsMuted(float* level) { On 2016/03/02 15:25:52, joedow wrote: > Couldn't this function return a float (just return 0.0f if the audio is muted) > then you wouldn't need to pass a level pointer and you could rename the function > GetAudioLevel() or similar? > > The current logic handles mute vs. 0 volume but I'm wondering if we actually > want to send audio packets if the volume is 0.0 (i.e. should we treat mute and 0 > volume the same?). +1 https://codereview.chromium.org/1753663002/diff/140001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:225: *level = 1; On 2016/03/02 15:25:52, joedow wrote: > Per my previous comment, I think the decision to default to full volume should > be documented. Can you add a comment on why this is the right behavior. look at it as the host's volume setting being ignored, not as we are defaulting to full volume. The client also has volume control that gets applied there. https://codereview.chromium.org/1753663002/diff/140001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:256: if (!IsMuted(&level)) { Sorry for not being clear about it on my previous comment. I was suggesting to move this whole block to a separate method. This would help to avoid 3 levels of nested if blocks. I.e. void ProcessSamples(uint8_t* data, size_t num_frames, uint32_t flags) { if (!num_frames) return; if (flags & AUDCLNT_BUFFERFLAGS_SILENT) != 0) return; if (silence_detector_.IsSilence(dat, num_frames * kChannels)) return; float volume = GetVolume(); if (volume <= 0) return; .. apply volume .. generate packet and call callback_. } https://codereview.chromium.org/1753663002/diff/140001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:260: * sizeof(BYTE) / sizeof(int16_t); I still think sizeof(BYTE) is useless here. Something must go terribly wrong for sizeof(BYTE) to be different than 1. And even if it was different, I don't think how it would apply here. frames is number of frames. sizeof(int16_t) would still be 2. I.e. size of the type used for data pointer has no relation to how many many samples we have. Also kBytesPerSample / sizeof(int16_t) is useless given that you have the static_assert() below now. Number of samples equals number of frames times number of channels. That's it. There is no reason we need to care about sample size in this expression. https://codereview.chromium.org/1753663002/diff/140001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:269: if (!silence_detector_.IsSilence(samples, sample_count)) { Here almost all data is being dropped (as audio is not played all the time over CRD session). I think ideally this should be done before we waste time applying volume. https://codereview.chromium.org/1753663002/diff/140001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:271: packet->add_data(data, frames * wave_format_ex_->nBlockAlign); So here the data is copied from data to a new buffer. Maybe do it one with the loop that updates the volume? I.e. the loop will be copying data from frames to packet->mutable_data().
Resolve code review comments https://codereview.chromium.org/1753663002/diff/40001/remoting/host/audio_cap... File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/1753663002/diff/40001/remoting/host/audio_cap... remoting/host/audio_capturer_win.cc:260: samples[i] *= level; On 2016/03/02 20:09:39, Sergey Ulanov wrote: > On 2016/03/02 08:16:32, Hzj_jie wrote: > > On 2016/03/01 21:48:16, Sergey Ulanov wrote: > > > This would be many times faster if you avoid floating point arithmetic in > the > > > loop. E.g. > > > > > > int32_t level_int = static_cast<int>(level * 65536); > > > for (size_t i = 0; i < sample_count; i++) { > > > samples[i] = (static_cast<int32_t>(samples[i]) * level_int) >> 16; > > > } > > > > Done. But the performance impact would not be many times. > > FWIW I see 2.5x-3x difference on ia64 and on ia32 with GCC: > > $ cat apply_volume.cc > #include <array> > #include <cstdint> > #include <iostream> > #include <time.h> > int main() { > const int kNumSamples = 100*1000; > const int kIterations = 100; > std::array<int16_t, kNumSamples> samples; > float volume = 0.3; > clock_t started; > > started = clock(); > for (size_t j = 0; j < kIterations; ++j) { > for (size_t i = 0; i < kNumSamples; ++i) { > samples[i] *= volume; > } > } > std::cerr << static_cast<double>(clock() - started) / CLOCKS_PER_SEC << > std::endl; > > started = clock(); > int32_t level_int = static_cast<int>(volume * 65536); > for (size_t j = 0; j < kIterations; ++j) { > for (size_t i = 0; i < kNumSamples; ++i) { > samples[i] = (static_cast<int32_t>(samples[i]) * level_int) >> 16; > } > } > std::cerr << static_cast<double>(clock() - started) / CLOCKS_PER_SEC > << std::endl; > > int sum = 0; > for (size_t i = 0; i < kNumSamples; ++i) { > sum += samples[i]; > } > std::cerr << sum << std::endl; > } > $ g++ apply_volume.cc -std=c++11 -g -O2 > $ ./a.out > 0.037469 > 0.01429 > 0 > $ g++ apply_volume.cc -std=c++11 -g -O2 -m32 > $ ./a.out > 0.034593 > 0.012743 > 0 > > It's not that floating point multiplication is slow (it may be even faster than > int). It's int->float->int conversion that kills it. I tried with only multiplication (which is on-par). Thank you for the detail analysis. https://codereview.chromium.org/1753663002/diff/140001/remoting/host/audio_ca... File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/1753663002/diff/140001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:216: bool AudioCapturerWin::IsMuted(float* level) { On 2016/03/02 20:09:40, Sergey Ulanov wrote: > On 2016/03/02 15:25:52, joedow wrote: > > Couldn't this function return a float (just return 0.0f if the audio is muted) > > then you wouldn't need to pass a level pointer and you could rename the > function > > GetAudioLevel() or similar? > > > > The current logic handles mute vs. 0 volume but I'm wondering if we actually > > want to send audio packets if the volume is 0.0 (i.e. should we treat mute and > 0 > > volume the same?). > > +1 Yes, current logic treat 0 volume exactly the same as mute. And audio packets will be ignored if volume is 0.0 or audio is muted. I have updated the function into float GetAudioLevel(). https://codereview.chromium.org/1753663002/diff/140001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:224: if (FAILED(hr) || *level > 1 || *level < 0) { On 2016/03/02 15:25:52, joedow wrote: > If the level is less than zero, should you clamp it to 0 instead of 1? I am OK with both values, we definitely should not receive <0 value from OS. Updated to 0. https://codereview.chromium.org/1753663002/diff/140001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:225: *level = 1; On 2016/03/02 20:09:40, Sergey Ulanov wrote: > On 2016/03/02 15:25:52, joedow wrote: > > Per my previous comment, I think the decision to default to full volume should > > be documented. Can you add a comment on why this is the right behavior. > > look at it as the host's volume setting being ignored, not as we are defaulting > to full volume. The client also has volume control that gets applied there. Yes, if anything wrong, current design ignores host volume settings, and returns (false, 1.0). I have added comments for the function in .h file. https://codereview.chromium.org/1753663002/diff/140001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:256: if (!IsMuted(&level)) { On 2016/03/02 20:09:40, Sergey Ulanov wrote: > Sorry for not being clear about it on my previous comment. I was suggesting to > move this whole block to a separate method. This would help to avoid 3 levels of > nested if blocks. I.e. > > void ProcessSamples(uint8_t* data, size_t num_frames, uint32_t flags) { > if (!num_frames) > return; > > if (flags & AUDCLNT_BUFFERFLAGS_SILENT) != 0) > return; > > if (silence_detector_.IsSilence(dat, num_frames * kChannels)) > return; > > float volume = GetVolume(); > if (volume <= 0) > return; > > .. apply volume > .. generate packet and call callback_. > } > Done. https://codereview.chromium.org/1753663002/diff/140001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:258: // BYTE to int16_t On 2016/03/02 15:25:51, joedow wrote: > I don't think this comment adds much value, remove it? Done. https://codereview.chromium.org/1753663002/diff/140001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:260: * sizeof(BYTE) / sizeof(int16_t); On 2016/03/02 20:09:39, Sergey Ulanov wrote: > I still think sizeof(BYTE) is useless here. Something must go terribly wrong for > sizeof(BYTE) to be different than 1. And even if it was different, I don't think > how it would apply here. frames is number of frames. sizeof(int16_t) would still > be 2. I.e. size of the type used for data pointer has no relation to how many > many samples we have. > Also kBytesPerSample / sizeof(int16_t) is useless given that you have the > static_assert() below now. Number of samples equals number of frames times > number of channels. That's it. There is no reason we need to care about sample > size in this expression. Good point, thank you. https://codereview.chromium.org/1753663002/diff/140001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:263: if (level < 1) { On 2016/03/02 15:25:52, joedow wrote: > A comment might be good here to describe why we need to manually adjust the > level of each sample. Done. https://codereview.chromium.org/1753663002/diff/140001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:269: if (!silence_detector_.IsSilence(samples, sample_count)) { On 2016/03/02 20:09:40, Sergey Ulanov wrote: > Here almost all data is being dropped (as audio is not played all the time over > CRD session). I think ideally this should be done before we waste time applying > volume. I believe network is more precious than processor, i.e. if we apply volume after silence detector, we may send silent samples to the client (level is always <= 1). Or do you prefer to do silence check twice? https://codereview.chromium.org/1753663002/diff/140001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:271: packet->add_data(data, frames * wave_format_ex_->nBlockAlign); On 2016/03/02 20:09:39, Sergey Ulanov wrote: > So here the data is copied from data to a new buffer. Maybe do it one with the > loop that updates the volume? I.e. the loop will be copying data from frames to > packet->mutable_data(). Same as comment above.
https://codereview.chromium.org/1753663002/diff/160001/remoting/host/audio_ca... File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/1753663002/diff/160001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:220: return 1; Is there an advantage to returning an int here instead of a float (i.e. 1 vs. 1.0f)? Same for the rest of the return statements. I think it would be cleaner to actually return a float value. https://codereview.chromium.org/1753663002/diff/160001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:223: } nit: add a newline here to break up the mute and volume level logic blocks. https://codereview.chromium.org/1753663002/diff/160001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:245: if (level == 0) { Are there problems here if the level is 0.01 or 0.06? I'm wondering if there might be cast problems when comparing to an integer which might be compiler specific (whether it rounds or truncates). The safest seems like a float comparison with a small delta for tolerance. https://codereview.chromium.org/1753663002/diff/160001/remoting/host/audio_ca... File remoting/host/audio_capturer_win.h (right): https://codereview.chromium.org/1753663002/diff/160001/remoting/host/audio_ca... remoting/host/audio_capturer_win.h:40: float GetAudioLevel(); Neither of the new methods change internal state so they can be marked const.
https://codereview.chromium.org/1753663002/diff/140001/remoting/host/audio_ca... File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/1753663002/diff/140001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:269: if (!silence_detector_.IsSilence(samples, sample_count)) { On 2016/03/03 09:33:39, Hzj_jie wrote: > On 2016/03/02 20:09:40, Sergey Ulanov wrote: > > Here almost all data is being dropped (as audio is not played all the time > over > > CRD session). I think ideally this should be done before we waste time > applying > > volume. > > I believe network is more precious than processor, i.e. if we apply volume after > silence detector, we may send silent samples to the client (level is always <= > 1). Or do you prefer to do silence check twice? You already have explicit check for volume=0, so samples may become silent after you apply volume only when volume is close to 0 and all samples are also close to 0, but above threshold used by silence_detector_. This is extremely rare scenario and I don't think we need to worry about it. And even if that was happening frequently I'd still argue that it's better to keep sending data because we know that there is some application that tries to play back something. https://codereview.chromium.org/1753663002/diff/160001/remoting/host/audio_ca... File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/1753663002/diff/160001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:220: return 1; On 2016/03/04 16:46:10, joedow wrote: > Is there an advantage to returning an int here instead of a float (i.e. 1 vs. > 1.0f)? Same for the rest of the return statements. I think it would be cleaner > to actually return a float value. FWIW IMO it makes no difference. Compiler will generate the same code in both cases. "return 1;" may be easier to read. https://codereview.chromium.org/1753663002/diff/160001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:221: } else if (mute) { don't need this else. See https://www.chromium.org/developers/coding-style#TOC-Code-formatting The last item in that section: "Don't use else after return" https://codereview.chromium.org/1753663002/diff/160001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:230: } else { don't need this else https://codereview.chromium.org/1753663002/diff/160001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:245: if (level == 0) { On 2016/03/04 16:46:10, joedow wrote: > Are there problems here if the level is 0.01 or 0.06? > I'm wondering if there > might be cast problems when comparing to an integer which might be compiler > specific (whether it rounds or truncates). The safest seems like a float > comparison with a small delta for tolerance. Compiler is required to convert ints to floats for mixed-type operations like this, so I don't think there a problem here. https://codereview.chromium.org/1753663002/diff/160001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:261: if (!silence_detector_.IsSilence(samples, sample_count)) { this can be reformatted as follows if (silence_detector_.IsSilence(samples, sample_count)) return; ...send data. https://codereview.chromium.org/1753663002/diff/160001/remoting/host/audio_ca... File remoting/host/audio_capturer_win.h (right): https://codereview.chromium.org/1753663002/diff/160001/remoting/host/audio_ca... remoting/host/audio_capturer_win.h:40: float GetAudioLevel(); On 2016/03/04 16:46:10, joedow wrote: > Neither of the new methods change internal state so they can be marked const. Actually GetAudioLevel() doesn't even need to be a method - it can be a static function. Move it out of this class? https://codereview.chromium.org/1753663002/diff/160001/remoting/host/audio_ca... remoting/host/audio_capturer_win.h:44: void ProcessSamples(BYTE* data, UINT32 frames, DWORD flags); BYTE, UINT32, DWORD are all windows specific types, and it's best to avoid using them when possible, except when you call windows APIs. in this case it's better to use uint8_t, size_t and int32_t
https://codereview.chromium.org/1753663002/diff/140001/remoting/host/audio_ca... File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/1753663002/diff/140001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:269: if (!silence_detector_.IsSilence(samples, sample_count)) { On 2016/03/04 21:30:03, Sergey Ulanov wrote: > On 2016/03/03 09:33:39, Hzj_jie wrote: > > On 2016/03/02 20:09:40, Sergey Ulanov wrote: > > > Here almost all data is being dropped (as audio is not played all the time > > over > > > CRD session). I think ideally this should be done before we waste time > > applying > > > volume. > > > > I believe network is more precious than processor, i.e. if we apply volume > after > > silence detector, we may send silent samples to the client (level is always <= > > 1). Or do you prefer to do silence check twice? > > You already have explicit check for volume=0, so samples may become silent after > you apply volume only when volume is close to 0 and all samples are also close > to 0, but above threshold used by silence_detector_. This is extremely rare > scenario and I don't think we need to worry about it. And even if that was > happening frequently I'd still argue that it's better to keep sending data > because we know that there is some application that tries to play back > something. > Not really, if the volume is close to 0, but samples aren't, they may be detected as silence. Since windows has a specific threshold 2. The most important thing is, if we check silence detector before attaching volume to the samples, the behavior is not consistent as what we are doing on linux. https://codereview.chromium.org/1753663002/diff/160001/remoting/host/audio_ca... File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/1753663002/diff/160001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:220: return 1; On 2016/03/04 21:30:03, Sergey Ulanov wrote: > On 2016/03/04 16:46:10, joedow wrote: > > Is there an advantage to returning an int here instead of a float (i.e. 1 vs. > > 1.0f)? Same for the rest of the return statements. I think it would be > cleaner > > to actually return a float value. > > FWIW IMO it makes no difference. Compiler will generate the same code in both > cases. "return 1;" may be easier to read. Done. https://codereview.chromium.org/1753663002/diff/160001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:221: } else if (mute) { On 2016/03/04 21:30:03, Sergey Ulanov wrote: > don't need this else. > See https://www.chromium.org/developers/coding-style#TOC-Code-formatting > The last item in that section: "Don't use else after return" Done. https://codereview.chromium.org/1753663002/diff/160001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:223: } On 2016/03/04 16:46:10, joedow wrote: > nit: add a newline here to break up the mute and volume level logic blocks. Done. https://codereview.chromium.org/1753663002/diff/160001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:230: } else { On 2016/03/04 21:30:03, Sergey Ulanov wrote: > don't need this else Done. https://codereview.chromium.org/1753663002/diff/160001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:245: if (level == 0) { On 2016/03/04 21:30:03, Sergey Ulanov wrote: > On 2016/03/04 16:46:10, joedow wrote: > > Are there problems here if the level is 0.01 or 0.06? > > I'm wondering if there > > might be cast problems when comparing to an integer which might be compiler > > specific (whether it rounds or truncates). The safest seems like a float > > comparison with a small delta for tolerance. > > Compiler is required to convert ints to floats for mixed-type operations like > this, so I don't think there a problem here. > I believe what Joe mentioned is the string display limitation of float point numbers. i.e. what you see for by calling a cout or printf is not what you get in a float number. A description can be found in Python tutorial (https://docs.python.org/3/tutorial/floatingpoint.html). But 0 is definitely safe. It's always 0. http://cpp.sh/9irk https://codereview.chromium.org/1753663002/diff/160001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:261: if (!silence_detector_.IsSilence(samples, sample_count)) { On 2016/03/04 21:30:03, Sergey Ulanov wrote: > this can be reformatted as follows > > if (silence_detector_.IsSilence(samples, sample_count)) > return; > > ...send data. Done. https://codereview.chromium.org/1753663002/diff/160001/remoting/host/audio_ca... File remoting/host/audio_capturer_win.h (right): https://codereview.chromium.org/1753663002/diff/160001/remoting/host/audio_ca... remoting/host/audio_capturer_win.h:40: float GetAudioLevel(); On 2016/03/04 21:30:03, Sergey Ulanov wrote: > On 2016/03/04 16:46:10, joedow wrote: > > Neither of the new methods change internal state so they can be marked const. > > Actually GetAudioLevel() doesn't even need to be a method - it can be a static > function. Move it out of this class? GetAudioLevel uses audio_volume_, ScopedComPtr<ISimpleAudioVolume>. Both GetMute and GetMasterVolume are not const declared according to MSDN. (https://msdn.microsoft.com/en-us/library/windows/desktop/dd316535(v=vs.85).aspx, https://msdn.microsoft.com/en-us/library/windows/desktop/dd316533(v=vs.85).aspx) https://codereview.chromium.org/1753663002/diff/160001/remoting/host/audio_ca... remoting/host/audio_capturer_win.h:44: void ProcessSamples(BYTE* data, UINT32 frames, DWORD flags); On 2016/03/04 21:30:03, Sergey Ulanov wrote: > BYTE, UINT32, DWORD are all windows specific types, and it's best to avoid using > them when possible, except when you call windows APIs. in this case it's better > to use uint8_t, size_t and int32_t Done.
https://codereview.chromium.org/1753663002/diff/180001/remoting/host/audio_ca... File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/1753663002/diff/180001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:265: if (silence_detector_.IsSilence(samples, sample_count)) { I still think this should be done before volume is applied for the same reasons I explained in my previous comment (i.e. this condition should apper before GetAudioLevel()). In 99% of calls this function will return true and the data is not going to be sent. In that case we don't want to waste time applying volume. If you are worried about the case when the volume is close to 0, but no 0, then maybe set low threshold for volume in line 249, e.g.: if (level < 0.01) return; But IMO this is not going to make much difference.
https://codereview.chromium.org/1753663002/diff/180001/remoting/host/audio_ca... File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/1753663002/diff/180001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:265: if (silence_detector_.IsSilence(samples, sample_count)) { On 2016/03/10 01:13:40, Sergey Ulanov wrote: > I still think this should be done before volume is applied for the same reasons > I explained in my previous comment (i.e. this condition should apper before > GetAudioLevel()). In 99% of calls this function will return true and the data is > not going to be sent. In that case we don't want to waste time applying volume. > If you are worried about the case when the volume is close to 0, but no 0, then > maybe set low threshold for volume in line 249, e.g.: > if (level < 0.01) > return; > But IMO this is not going to make much difference. Emm, I see your point clearly. Most of time, we will have silent samples. Reasonable. I have moved silence detector above the logic to apply volume.
LGTM, but see my nits https://codereview.chromium.org/1753663002/diff/200001/remoting/host/audio_ca... File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/1753663002/diff/200001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:256: size_t sample_count = frames * kChannels; nit: move samples and sample_count inside the if block below where they are used. https://codereview.chromium.org/1753663002/diff/200001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:257: if (silence_detector_.IsSilence(samples, sample_count)) { nit: move this above GetAudioLevel() call to avoid calling system APIs to get volume when we get silence. It most cases we will get silence here but audio is not muted. To optimize for that case we want to detect silence as soon as possible, i.e. before GetAudioLevel()
https://codereview.chromium.org/1753663002/diff/200001/remoting/host/audio_ca... File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/1753663002/diff/200001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:256: size_t sample_count = frames * kChannels; On 2016/03/10 06:04:28, Sergey Ulanov wrote: > nit: move samples and sample_count inside the if block below where they are > used. Both silence_detector_.IsSilence and following logic use samples and sample_count. https://codereview.chromium.org/1753663002/diff/200001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:257: if (silence_detector_.IsSilence(samples, sample_count)) { On 2016/03/10 06:04:28, Sergey Ulanov wrote: > nit: move this above GetAudioLevel() call to avoid calling system APIs to get > volume when we get silence. It most cases we will get silence here but audio is > not muted. To optimize for that case we want to detect silence as soon as > possible, i.e. before GetAudioLevel() Done.
lgtm
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/1753663002/#ps240001 (title: "Sync latest code")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1753663002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1753663002/240001
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Add volume control for windows host BUG=276753 ========== to ========== Add volume control for windows host BUG=276753 Committed: https://crrev.com/3ba26c009b0b2583562b695c426391f71b626f33 Cr-Commit-Position: refs/heads/master@{#380458} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/3ba26c009b0b2583562b695c426391f71b626f33 Cr-Commit-Position: refs/heads/master@{#380458} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
