Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2230823004:
Added a level indicator to new mixer. (Closed)
DescriptionAdded a level indicator to new mixer.
Added a level indicator to the new mixer. The level indicator is
webrtc::voe::AudioLevel. It computes the current audio level, which is
used all the way up to peerconnection.
This is part of the project to rewrite the old conference mixer and
output mixer.
NOTRY=True
Committed: https://crrev.com/616df1e95c5ac70d288d83e694eefc8f78da6245
Cr-Commit-Position: refs/heads/master@{#13878}
Patch Set 1 #Patch Set 2 : Added tests for volume. #Patch Set 3 : Added call to AudioLimiter. #
Total comments: 2
Patch Set 4 : Updated comments in volume test. #
Total comments: 28
Patch Set 5 : Fixed audio mixer volume tests, inited SPL. #
Total comments: 1
Patch Set 6 : Updated comments, renamed output level function and tests, formatted level indicator. #Patch Set 7 : Fake participant returns frame copy. #
Total comments: 7
Patch Set 8 : Renamed audio level functions. #Patch Set 9 : Removed format changes to level_indicator.cc. #
Total comments: 16
Patch Set 10 : Minor changes in response to reviewer comments. #
Total comments: 8
Patch Set 11 : Made local variable const. #
Dependent Patchsets: Messages
Total messages: 48 (16 generated)
The CQ bit was checked by aleloi@webrtc.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.webrtc.org/...
aleloi@webrtc.org changed reviewers: + minyue@webrtc.org
Could you take a look at this? I added the capacity to measure audio volume to the new mixer. https://codereview.webrtc.org/2230823004/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/new_audio_conference_mixer_impl.cc (left): https://codereview.webrtc.org/2230823004/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/new_audio_conference_mixer_impl.cc:76: // TODO(andrew): consider not modifying |frame| here. Removed stale comment. We're planning to remove AudioFrames completely from here anyway. https://codereview.webrtc.org/2230823004/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/new_audio_conference_mixer_impl.cc:241: output_frequency_ <= AudioProcessing::kMaxNativeSampleRateHz; Two problems with this line: output_frequency_ is now set to exactly kMaxNativeSampleRate; doesn't make sense to not use limiter for high rates. I asked hlundin@ about this and am now removing it.
Description was changed from ========== Added a level indicator to new mixer. Added a level indicator to the new mixer. The level indicator is webrtc::voe::AudioLevel. It computes the current audio level, which is used all the way up to peerconnection. This is part of the project to rewrite the old conference mixer and output mixer. ========== to ========== Added a level indicator to new mixer. Added a level indicator to the new mixer. The level indicator is webrtc::voe::AudioLevel. It computes the current audio level, which is used all the way up to peerconnection. This is part of the project to rewrite the old conference mixer and output mixer. NOTE to aleloi@: do not commit this without ensuring that modules_unittests run. Locally, there is some config flag error that results in the function pointer WebRtcSpl_MaxAbsValueW16 to 0x0 instead of WebRtcSpl_MaxAbsValueW16C or WebRtcSpl_MaxAbsValueW16_mips. ==========
Description was changed from ========== Added a level indicator to new mixer. Added a level indicator to the new mixer. The level indicator is webrtc::voe::AudioLevel. It computes the current audio level, which is used all the way up to peerconnection. This is part of the project to rewrite the old conference mixer and output mixer. NOTE to aleloi@: do not commit this without ensuring that modules_unittests run. Locally, there is some config flag error that results in the function pointer WebRtcSpl_MaxAbsValueW16 to 0x0 instead of WebRtcSpl_MaxAbsValueW16C or WebRtcSpl_MaxAbsValueW16_mips. ========== to ========== Added a level indicator to new mixer. Added a level indicator to the new mixer. The level indicator is webrtc::voe::AudioLevel. It computes the current audio level, which is used all the way up to peerconnection. This is part of the project to rewrite the old conference mixer and output mixer. NOTE to aleloi@: do not commit this without ensuring that modules_unittests run. Locally, there is some config flag error that results in the function pointer WebRtcSpl_MaxAbsValueW16 to 0x0 instead of WebRtcSpl_MaxAbsValueW16C or WebRtcSpl_MaxAbsValueW16_mips. (Seems that it doesn't only happen locally...) ==========
Description was changed from ========== Added a level indicator to new mixer. Added a level indicator to the new mixer. The level indicator is webrtc::voe::AudioLevel. It computes the current audio level, which is used all the way up to peerconnection. This is part of the project to rewrite the old conference mixer and output mixer. NOTE to aleloi@: do not commit this without ensuring that modules_unittests run. Locally, there is some config flag error that results in the function pointer WebRtcSpl_MaxAbsValueW16 to 0x0 instead of WebRtcSpl_MaxAbsValueW16C or WebRtcSpl_MaxAbsValueW16_mips. (Seems that it doesn't only happen locally...) ========== to ========== Added a level indicator to new mixer. Added a level indicator to the new mixer. The level indicator is webrtc::voe::AudioLevel. It computes the current audio level, which is used all the way up to peerconnection. This is part of the project to rewrite the old conference mixer and output mixer. ==========
aleloi@webrtc.org changed reviewers: + mflodman@webrtc.org
mflodman@, can you also take a look at this? In particular voice_engine/level_indicator.cc, which I change to call WebRtcSpl_Init(); https://codereview.webrtc.org/2230823004/diff/80001/webrtc/voice_engine/level... File webrtc/voice_engine/level_indicator.cc (right): https://codereview.webrtc.org/2230823004/diff/80001/webrtc/voice_engine/level... webrtc/voice_engine/level_indicator.cc:31: WebRtcSpl_Init(); Is this the right place to init SPL? Context: level_indicator (AudioLeves) uses WebRtcSpl_MaxAbsValueW16, which is a function pointer that is set to either WebRtcSpl_MaxAbsValueW16C, WebRtcSpl_MaxAbsValueW16Neon or WebRtcSpl_MaxAbsValueW16_mips when WebRtcSpl_Init() is called. Earlier, this target was used solely in voice_engine, and Spl_Init() was called in a VoEBaseImpl::Init(). Now, I have broken out this target from :voice_engine and added it as a dependency for the audio mixer rewrite. WebRtcSplInit() needs to be run at some point.
thanks and see my comments https://codereview.webrtc.org/2230823004/diff/60001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/new_audio_conference_mixer.h (right): https://codereview.webrtc.org/2230823004/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/new_audio_conference_mixer.h:62: // Output level functions from VoEVolumeControl would be good to note the unit of level somewhere, but yes, you only take the value that AudioLevel returns, then may be good to refer AudioLevel in the comment. https://codereview.webrtc.org/2230823004/diff/60001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/new_audio_conference_mixer_impl.cc (left): https://codereview.webrtc.org/2230823004/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/new_audio_conference_mixer_impl.cc:76: // TODO(andrew): consider not modifying |frame| here. you think there is no need to consider this anymore? https://codereview.webrtc.org/2230823004/diff/60001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/new_audio_conference_mixer_impl.h (right): https://codereview.webrtc.org/2230823004/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/new_audio_conference_mixer_impl.h:129: // Output level functions from VoEVolumeControl comment is a bit misleading, revise the wording. And add a full stop to sentence. https://codereview.webrtc.org/2230823004/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/new_audio_conference_mixer_impl.h:130: uint32_t GetSpeechOutputLevel() override; why does this have to be "speech"? https://codereview.webrtc.org/2230823004/diff/60001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc (right): https://codereview.webrtc.org/2230823004/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:373: // Test that the volume is reported as zero when the mixer input is is -> comprises only https://codereview.webrtc.org/2230823004/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:374: // frames with no sound. no sound -> zero values https://codereview.webrtc.org/2230823004/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:375: TEST(AudioMixer, ZeroVolume) { ZeroVolume -> LevelIsZeroWhenMixingZeros https://codereview.webrtc.org/2230823004/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:382: participant.fake_frame()->sample_rate_hz_ = 8000; define kSampleRate and use it to replace 8000 https://codereview.webrtc.org/2230823004/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:398: // Test that the volume is reported as full when the mixer input is maximum https://codereview.webrtc.org/2230823004/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:398: // Test that the volume is reported as full when the mixer input is comprises frames with max values of int16_t. https://codereview.webrtc.org/2230823004/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:400: TEST(AudioMixer, FullVolume) { FullVolume -> LevelIsMaxWhenMixingMaxValues https://codereview.webrtc.org/2230823004/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:400: TEST(AudioMixer, FullVolume) { curious, what if one participant is zero and another is max
BTW, the description does not say who will be using the new APIs
I addressed your comments, minyue@! The linter complained on level_indicator, so I did a git cl format. The only semantical change is still that WebRtcSpl_Init(); is run in the constructor. https://codereview.webrtc.org/2230823004/diff/60001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/new_audio_conference_mixer.h (right): https://codereview.webrtc.org/2230823004/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/new_audio_conference_mixer.h:62: // Output level functions from VoEVolumeControl On 2016/08/11 10:27:33, minyue-webrtc wrote: > would be good to note the unit of level somewhere, but yes, you only take the > value that AudioLevel returns, then may be good to refer AudioLevel in the > comment. Thank you! Updated documentation in next patch set. https://codereview.webrtc.org/2230823004/diff/60001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/new_audio_conference_mixer_impl.cc (left): https://codereview.webrtc.org/2230823004/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/new_audio_conference_mixer_impl.cc:76: // TODO(andrew): consider not modifying |frame| here. On 2016/08/11 10:27:33, minyue-webrtc wrote: > you think there is no need to consider this anymore? I wrote that in a comment in one of the earlier patch sets. We will replace the AudioFrame with something else and rewrite the mixer code that touches it. Besides, I don't see a reason for why one would not modify the frame. Please tell me if you can think of one :) https://codereview.webrtc.org/2230823004/diff/60001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/new_audio_conference_mixer_impl.h (right): https://codereview.webrtc.org/2230823004/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/new_audio_conference_mixer_impl.h:129: // Output level functions from VoEVolumeControl On 2016/08/11 10:27:33, minyue-webrtc wrote: > comment is a bit misleading, revise the wording. > > And add a full stop to sentence. Done. https://codereview.webrtc.org/2230823004/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/new_audio_conference_mixer_impl.h:130: uint32_t GetSpeechOutputLevel() override; On 2016/08/11 10:27:33, minyue-webrtc wrote: > why does this have to be "speech"? Historical reasons, I'd say. The function name in VoEVolumeControl is GetSpeechOutputLevel https://cs.chromium.org/chromium/src/third_party/webrtc/voice_engine/include/... I'll call it GetAudioOutputLevel instead, since that's what it is. https://codereview.webrtc.org/2230823004/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/new_audio_conference_mixer_impl.h:163: voe::AudioLevel audio_level_; Will add capitalization and sentence break (sorry, missed). https://codereview.webrtc.org/2230823004/diff/60001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc (right): https://codereview.webrtc.org/2230823004/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:373: // Test that the volume is reported as zero when the mixer input is On 2016/08/11 10:27:33, minyue-webrtc wrote: > is -> comprises only Done. https://codereview.webrtc.org/2230823004/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:374: // frames with no sound. On 2016/08/11 10:27:33, minyue-webrtc wrote: > no sound -> zero values Done. https://codereview.webrtc.org/2230823004/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:375: TEST(AudioMixer, ZeroVolume) { On 2016/08/11 10:27:33, minyue-webrtc wrote: > ZeroVolume -> LevelIsZeroWhenMixingZeros Done. https://codereview.webrtc.org/2230823004/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:382: participant.fake_frame()->sample_rate_hz_ = 8000; On 2016/08/11 10:27:33, minyue-webrtc wrote: > define kSampleRate and use it to replace 8000 Done. https://codereview.webrtc.org/2230823004/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:382: participant.fake_frame()->sample_rate_hz_ = 8000; On 2016/08/11 10:27:33, minyue-webrtc wrote: > define kSampleRate and use it to replace 8000 Done. https://codereview.webrtc.org/2230823004/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:398: // Test that the volume is reported as full when the mixer input is On 2016/08/11 10:27:33, minyue-webrtc wrote: > comprises frames with max values of int16_t. Done. https://codereview.webrtc.org/2230823004/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:398: // Test that the volume is reported as full when the mixer input is On 2016/08/11 10:27:33, minyue-webrtc wrote: > maximum Done. https://codereview.webrtc.org/2230823004/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:400: TEST(AudioMixer, FullVolume) { On 2016/08/11 10:27:33, minyue-webrtc wrote: > curious, what if one participant is zero and another is max It should be max, or one off from max due to rounding and limiting. Do you think it's meaningful to test? https://codereview.webrtc.org/2230823004/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:400: TEST(AudioMixer, FullVolume) { On 2016/08/11 10:27:33, minyue-webrtc wrote: > FullVolume -> LevelIsMaxWhenMixingMaxValues Thanks! I thought about how to name the tests, but I only looked at my other tests for inspiration (which would also benefit from better names).
The CQ bit was checked by aleloi@webrtc.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.webrtc.org/...
Re volume with two participants https://codereview.webrtc.org/2230823004/diff/60001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc (right): https://codereview.webrtc.org/2230823004/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:400: TEST(AudioMixer, FullVolume) { On 2016/08/11 12:15:20, aleloi wrote: > On 2016/08/11 10:27:33, minyue-webrtc wrote: > > curious, what if one participant is zero and another is max > > It should be max, or one off from max due to rounding and limiting. Do you think > it's meaningful to test? Thinking on it again, I see that I was probably wrong. Since there are two participants, the limiter kicks in. We first reduce the volume by shifting down both frames, so the max frame is now full of 0x1fff and the other is zero. Then we tell the limiter to limit to -7dBFS, which is just below half max volume. The frame should get a little more quiet. Then we multiply it by two, so we get almost the full volume back. I will run a test to see if this is correct!
On 2016/08/11 12:25:22, aleloi wrote: > Re volume with two participants > > https://codereview.webrtc.org/2230823004/diff/60001/webrtc/modules/audio_mixe... > File webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc (right): > > https://codereview.webrtc.org/2230823004/diff/60001/webrtc/modules/audio_mixe... > webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:400: TEST(AudioMixer, > FullVolume) { > On 2016/08/11 12:15:20, aleloi wrote: > > On 2016/08/11 10:27:33, minyue-webrtc wrote: > > > curious, what if one participant is zero and another is max > > > > It should be max, or one off from max due to rounding and limiting. Do you > think > > it's meaningful to test? > > Thinking on it again, I see that I was probably wrong. Since there are two > participants, the limiter kicks in. > > We first reduce the volume by shifting down both frames, so the max frame is now > full of 0x1fff and the other is zero. > > Then we tell the limiter to limit to -7dBFS, which is just below half max > volume. The frame should get a little more quiet. Then we multiply it by two, so > we get almost the full volume back. I will run a test to see if this is correct! Re re volume with two participants: First, huge thank you, minyue@, for asking this question. It uncovered unintended behavior in the tests. It may also have answered the reason behind the "don't touch the mixing frame" comment. The mock participants work by giving the mixer the same frame over and over. The mixer invalidates that frame by e.g. halving it (which is intended behavior and IIRC documented). This means that you can't set the mock participant frame once and expect it to be the same. Which is not how I designed the tests, which means that my tests are insufficient, since they didn't break. It wasn't noticed in the volume tests because there was only one participant and the frame was never halved. Next patch set will be a test update!
After writing the mixer update, I tried to figure out why it wasn't detected earlier. E.g. by the test that is supposed to run the new mixer against the old one and compare results. It turns out there was a bug in that test. When I fixed the bug, the test failed. I will make another CL with a fixed test. Thus your question helped me to find a serious issue in the new mixer. BTW, minyue@, the frame you should get after mixing a source outputting max frames with one outputting zero frames should be a frame filled with something very close to MAX_FRAME * sqrt(10 ** (-7 / 10) ) * 2. sqrt since we are looking at the amplitude and *2 since we double the frame after limiting. -7 because we limit to -7dBFS. It's 29272.96252802122. When performing this experiment in the real mixer, I got 29534, which is a little off. Don't know why. I don't think the math is wrong.
good job, please take a look at my small comments https://codereview.webrtc.org/2230823004/diff/120001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/new_audio_conference_mixer.h (right): https://codereview.webrtc.org/2230823004/diff/120001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/new_audio_conference_mixer.h:64: virtual uint32_t GetAudioOutputLevel() = 0; maybe better to use "GetOutputAudioLevel", since AudioLevel is one word. https://codereview.webrtc.org/2230823004/diff/120001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/new_audio_conference_mixer.h:67: virtual uint32_t GetAudioOutputLevelFullRange() = 0; same here https://codereview.webrtc.org/2230823004/diff/120001/webrtc/voice_engine/leve... File webrtc/voice_engine/level_indicator.cc (right): https://codereview.webrtc.org/2230823004/diff/120001/webrtc/voice_engine/leve... webrtc/voice_engine/level_indicator.cc:31: WebRtcSpl_Init(); I think clang format made irrelevant changes to the CL and hard to discern the actual change.
and would you cc me the CL that fixes to bug. https://codereview.webrtc.org/2230823004/diff/60001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/new_audio_conference_mixer_impl.cc (left): https://codereview.webrtc.org/2230823004/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/new_audio_conference_mixer_impl.cc:76: // TODO(andrew): consider not modifying |frame| here. On 2016/08/11 12:15:19, aleloi wrote: > On 2016/08/11 10:27:33, minyue-webrtc wrote: > > you think there is no need to consider this anymore? > > I wrote that in a comment in one of the earlier patch sets. We will replace the > AudioFrame with something else and rewrite the mixer code that touches it. > Besides, I don't see a reason for why one would not modify the frame. Please > tell me if you can think of one :) I do not see any case, it is up to you to decide whether to keep the comment
Updated according to comments. https://codereview.webrtc.org/2230823004/diff/120001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/new_audio_conference_mixer.h (right): https://codereview.webrtc.org/2230823004/diff/120001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/new_audio_conference_mixer.h:64: virtual uint32_t GetAudioOutputLevel() = 0; On 2016/08/15 06:26:15, minyue-webrtc wrote: > maybe better to use "GetOutputAudioLevel", since AudioLevel is one word. Done. https://codereview.webrtc.org/2230823004/diff/120001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/new_audio_conference_mixer.h:67: virtual uint32_t GetAudioOutputLevelFullRange() = 0; On 2016/08/15 06:26:15, minyue-webrtc wrote: > same here Done. https://codereview.webrtc.org/2230823004/diff/120001/webrtc/voice_engine/leve... File webrtc/voice_engine/level_indicator.cc (right): https://codereview.webrtc.org/2230823004/diff/120001/webrtc/voice_engine/leve... webrtc/voice_engine/level_indicator.cc:31: WebRtcSpl_Init(); On 2016/08/15 06:26:15, minyue-webrtc wrote: > I think clang format made irrelevant changes to the CL and hard to discern the > actual change. Acknowledged.
https://codereview.webrtc.org/2230823004/diff/120001/webrtc/voice_engine/leve... File webrtc/voice_engine/level_indicator.cc (right): https://codereview.webrtc.org/2230823004/diff/120001/webrtc/voice_engine/leve... webrtc/voice_engine/level_indicator.cc:31: WebRtcSpl_Init(); On 2016/08/15 11:52:13, aleloi wrote: > On 2016/08/15 06:26:15, minyue-webrtc wrote: > > I think clang format made irrelevant changes to the CL and hard to discern the > > actual change. > > Acknowledged. I think there are still too many lines of changes.
On 2016/08/15 15:58:37, minyue-webrtc wrote: > https://codereview.webrtc.org/2230823004/diff/120001/webrtc/voice_engine/leve... > File webrtc/voice_engine/level_indicator.cc (right): > > https://codereview.webrtc.org/2230823004/diff/120001/webrtc/voice_engine/leve... > webrtc/voice_engine/level_indicator.cc:31: WebRtcSpl_Init(); > On 2016/08/15 11:52:13, aleloi wrote: > > On 2016/08/15 06:26:15, minyue-webrtc wrote: > > > I think clang format made irrelevant changes to the CL and hard to discern > the > > > actual change. > > > > Acknowledged. > > I think there are still too many lines of changes. Sorry, I reverted the formatting.
lgtm
mflodman@webrtc.org changed reviewers: + kwiberg@webrtc.org
I don't really have the knowledge to review sp lib functions and how it's used. I'll add Karl as reviewer instead of me. Karl, Please review or hand over to someone in the audio team that can review this properly. https://codereview.webrtc.org/2230823004/diff/160001/webrtc/voice_engine/leve... File webrtc/voice_engine/level_indicator.cc (right): https://codereview.webrtc.org/2230823004/diff/160001/webrtc/voice_engine/leve... webrtc/voice_engine/level_indicator.cc:31: WebRtcSpl_Init(); This has already been called in VoEBaseImpl::Init, should this really be done here?
https://codereview.webrtc.org/2230823004/diff/160001/webrtc/voice_engine/leve... File webrtc/voice_engine/level_indicator.cc (right): https://codereview.webrtc.org/2230823004/diff/160001/webrtc/voice_engine/leve... webrtc/voice_engine/level_indicator.cc:31: WebRtcSpl_Init(); On 2016/08/18 09:34:13, mflodman wrote: > This has already been called in VoEBaseImpl::Init, should this really be done > here? I wanted to use the level indicator as a separate dependency in the new audio mixer without having to depend on the whole of VoE. But then the mixer crashed in unit tests, because the SPL functions (which this component uses) were not initialized. My understanding is that if this component is going to be a separate build target, it should initialize its dependencies by itself.
https://codereview.webrtc.org/2230823004/diff/160001/webrtc/voice_engine/leve... File webrtc/voice_engine/level_indicator.cc (right): https://codereview.webrtc.org/2230823004/diff/160001/webrtc/voice_engine/leve... webrtc/voice_engine/level_indicator.cc:31: WebRtcSpl_Init(); On 2016/08/18 09:40:33, aleloi wrote: > On 2016/08/18 09:34:13, mflodman wrote: > > This has already been called in VoEBaseImpl::Init, should this really be done > > here? > > I wanted to use the level indicator as a separate dependency in the new audio > mixer without having to depend on the whole of VoE. But then the mixer crashed > in unit tests, because the SPL functions (which this component uses) were not > initialized. > > My understanding is that if this component is going to be a separate build > target, it should initialize its dependencies by itself. But is it ok to do this twice in the case you run this in VoiceEngine? Should this rather be done in the test itself? This might be right, but I don't know how SPLib initialization is supposed to work and I think it's better if someone in the audio team looks at this then me.
https://codereview.webrtc.org/2230823004/diff/160001/webrtc/voice_engine/leve... File webrtc/voice_engine/level_indicator.cc (right): https://codereview.webrtc.org/2230823004/diff/160001/webrtc/voice_engine/leve... webrtc/voice_engine/level_indicator.cc:31: WebRtcSpl_Init(); On 2016/08/18 09:45:28, mflodman wrote: > On 2016/08/18 09:40:33, aleloi wrote: > > On 2016/08/18 09:34:13, mflodman wrote: > > > This has already been called in VoEBaseImpl::Init, should this really be > done > > > here? > > > > I wanted to use the level indicator as a separate dependency in the new audio > > mixer without having to depend on the whole of VoE. But then the mixer crashed > > in unit tests, because the SPL functions (which this component uses) were not > > initialized. > > > > My understanding is that if this component is going to be a separate build > > target, it should initialize its dependencies by itself. > > But is it ok to do this twice in the case you run this in VoiceEngine? Should > this rather be done in the test itself? > > This might be right, but I don't know how SPLib initialization is supposed to > work and I think it's better if someone in the audio team looks at this then me. It reads that WebRtcSpl_Init() should be called only once (see below). so we should not go wrong with it. But true, it should be called on a high level. If possible, do it in the test? https://cs.chromium.org/chromium/src/third_party/webrtc/common_audio/signal_p...
On 2016/08/18 09:53:09, minyue-webrtc wrote: > https://codereview.webrtc.org/2230823004/diff/160001/webrtc/voice_engine/leve... > File webrtc/voice_engine/level_indicator.cc (right): > > https://codereview.webrtc.org/2230823004/diff/160001/webrtc/voice_engine/leve... > webrtc/voice_engine/level_indicator.cc:31: WebRtcSpl_Init(); > On 2016/08/18 09:45:28, mflodman wrote: > > On 2016/08/18 09:40:33, aleloi wrote: > > > On 2016/08/18 09:34:13, mflodman wrote: > > > > This has already been called in VoEBaseImpl::Init, should this really be > > done > > > > here? > > > > > > I wanted to use the level indicator as a separate dependency in the new > audio > > > mixer without having to depend on the whole of VoE. But then the mixer > crashed > > > in unit tests, because the SPL functions (which this component uses) were > not > > > initialized. > > > > > > My understanding is that if this component is going to be a separate build > > > target, it should initialize its dependencies by itself. > > > > But is it ok to do this twice in the case you run this in VoiceEngine? Should > > this rather be done in the test itself? > > > > This might be right, but I don't know how SPLib initialization is supposed to > > work and I think it's better if someone in the audio team looks at this then > me. > > > It reads that WebRtcSpl_Init() should be called only once (see below). so we > should not go wrong with it. But true, it should be called on a high level. If > possible, do it in the test? > > https://cs.chromium.org/chromium/src/third_party/webrtc/common_audio/signal_p... The implementation of the init function also ensures that InitFunctionPointers is only called once, no matter how many times you call WebRtcSpl_Init(). So we should be safe. But I agree that someone who knows for sure better take look at it.
On 2016/08/18 10:45:25, aleloi wrote: > On 2016/08/18 09:53:09, minyue-webrtc wrote: > > > https://codereview.webrtc.org/2230823004/diff/160001/webrtc/voice_engine/leve... > > File webrtc/voice_engine/level_indicator.cc (right): > > > > > https://codereview.webrtc.org/2230823004/diff/160001/webrtc/voice_engine/leve... > > webrtc/voice_engine/level_indicator.cc:31: WebRtcSpl_Init(); > > On 2016/08/18 09:45:28, mflodman wrote: > > > On 2016/08/18 09:40:33, aleloi wrote: > > > > On 2016/08/18 09:34:13, mflodman wrote: > > > > > This has already been called in VoEBaseImpl::Init, should this really be > > > done > > > > > here? > > > > > > > > I wanted to use the level indicator as a separate dependency in the new > > audio > > > > mixer without having to depend on the whole of VoE. But then the mixer > > crashed > > > > in unit tests, because the SPL functions (which this component uses) were > > not > > > > initialized. > > > > > > > > My understanding is that if this component is going to be a separate build > > > > target, it should initialize its dependencies by itself. > > > > > > But is it ok to do this twice in the case you run this in VoiceEngine? > Should > > > this rather be done in the test itself? > > > > > > This might be right, but I don't know how SPLib initialization is supposed > to > > > work and I think it's better if someone in the audio team looks at this then > > me. > > > > > > It reads that WebRtcSpl_Init() should be called only once (see below). so we > > should not go wrong with it. But true, it should be called on a high level. If > > possible, do it in the test? > > > > > https://cs.chromium.org/chromium/src/third_party/webrtc/common_audio/signal_p... > > The implementation of the init function also ensures that InitFunctionPointers > is only called once, no matter how many times you call WebRtcSpl_Init(). So we > should be safe. But I agree that someone who knows for sure better take look at > it. I have a confirmation from Karl: WebRtcSpl_Init is safe to call multiple times. kwiberg@, please correct me if I misinterpreted :)
https://codereview.webrtc.org/2230823004/diff/160001/webrtc/voice_engine/leve... File webrtc/voice_engine/level_indicator.cc (right): https://codereview.webrtc.org/2230823004/diff/160001/webrtc/voice_engine/leve... webrtc/voice_engine/level_indicator.cc:31: WebRtcSpl_Init(); On 2016/08/18 09:53:08, minyue-webrtc wrote: > On 2016/08/18 09:45:28, mflodman wrote: > > On 2016/08/18 09:40:33, aleloi wrote: > > > On 2016/08/18 09:34:13, mflodman wrote: > > > > This has already been called in VoEBaseImpl::Init, should this really be > > done > > > > here? > > > > > > I wanted to use the level indicator as a separate dependency in the new > audio > > > mixer without having to depend on the whole of VoE. But then the mixer > crashed > > > in unit tests, because the SPL functions (which this component uses) were > not > > > initialized. > > > > > > My understanding is that if this component is going to be a separate build > > > target, it should initialize its dependencies by itself. > > > > But is it ok to do this twice in the case you run this in VoiceEngine? Should > > this rather be done in the test itself? > > > > This might be right, but I don't know how SPLib initialization is supposed to > > work and I think it's better if someone in the audio team looks at this then > me. > > > It reads that WebRtcSpl_Init() should be called only once (see below). so we > should not go wrong with it. But true, it should be called on a high level. If > possible, do it in the test? > > https://cs.chromium.org/chromium/src/third_party/webrtc/common_audio/signal_p... No, it's implemented so that it should be safe to call multiple times, even from several different threads.
On 2016/08/19 11:24:44, kwiberg-webrtc wrote: > https://codereview.webrtc.org/2230823004/diff/160001/webrtc/voice_engine/leve... > File webrtc/voice_engine/level_indicator.cc (right): > > https://codereview.webrtc.org/2230823004/diff/160001/webrtc/voice_engine/leve... > webrtc/voice_engine/level_indicator.cc:31: WebRtcSpl_Init(); > On 2016/08/18 09:53:08, minyue-webrtc wrote: > > On 2016/08/18 09:45:28, mflodman wrote: > > > On 2016/08/18 09:40:33, aleloi wrote: > > > > On 2016/08/18 09:34:13, mflodman wrote: > > > > > This has already been called in VoEBaseImpl::Init, should this really be > > > done > > > > > here? > > > > > > > > I wanted to use the level indicator as a separate dependency in the new > > audio > > > > mixer without having to depend on the whole of VoE. But then the mixer > > crashed > > > > in unit tests, because the SPL functions (which this component uses) were > > not > > > > initialized. > > > > > > > > My understanding is that if this component is going to be a separate build > > > > target, it should initialize its dependencies by itself. > > > > > > But is it ok to do this twice in the case you run this in VoiceEngine? > Should > > > this rather be done in the test itself? > > > > > > This might be right, but I don't know how SPLib initialization is supposed > to > > > work and I think it's better if someone in the audio team looks at this then > > me. > > > > > > It reads that WebRtcSpl_Init() should be called only once (see below). so we > > should not go wrong with it. But true, it should be called on a high level. If > > possible, do it in the test? > > > > > https://cs.chromium.org/chromium/src/third_party/webrtc/common_audio/signal_p... > > No, it's implemented so that it should be safe to call multiple times, even from > several different threads. Ah, that was what I meant: safe to call multiple times. once() does it automatically. But, my first sentence in that comment sounds truly wrong.
On 2016/08/19 11:34:38, minyue-webrtc wrote: > On 2016/08/19 11:24:44, kwiberg-webrtc wrote: > > > https://codereview.webrtc.org/2230823004/diff/160001/webrtc/voice_engine/leve... > > File webrtc/voice_engine/level_indicator.cc (right): > > > > > https://codereview.webrtc.org/2230823004/diff/160001/webrtc/voice_engine/leve... > > webrtc/voice_engine/level_indicator.cc:31: WebRtcSpl_Init(); > > On 2016/08/18 09:53:08, minyue-webrtc wrote: > > > On 2016/08/18 09:45:28, mflodman wrote: > > > > On 2016/08/18 09:40:33, aleloi wrote: > > > > > On 2016/08/18 09:34:13, mflodman wrote: > > > > > > This has already been called in VoEBaseImpl::Init, should this really > be > > > > done > > > > > > here? > > > > > > > > > > I wanted to use the level indicator as a separate dependency in the new > > > audio > > > > > mixer without having to depend on the whole of VoE. But then the mixer > > > crashed > > > > > in unit tests, because the SPL functions (which this component uses) > were > > > not > > > > > initialized. > > > > > > > > > > My understanding is that if this component is going to be a separate > build > > > > > target, it should initialize its dependencies by itself. > > > > > > > > But is it ok to do this twice in the case you run this in VoiceEngine? > > Should > > > > this rather be done in the test itself? > > > > > > > > This might be right, but I don't know how SPLib initialization is supposed > > to > > > > work and I think it's better if someone in the audio team looks at this > then > > > me. > > > > > > > > > It reads that WebRtcSpl_Init() should be called only once (see below). so we > > > should not go wrong with it. But true, it should be called on a high level. > If > > > possible, do it in the test? > > > > > > > > > https://cs.chromium.org/chromium/src/third_party/webrtc/common_audio/signal_p... > > > > No, it's implemented so that it should be safe to call multiple times, even > from > > several different threads. > > Ah, that was what I meant: safe to call multiple times. once() does it > automatically. But, my first sentence in that comment sounds truly wrong. Friendly ping. I'd like an LG from a VoE owner.
On 2016/08/23 12:16:26, aleloi wrote: > On 2016/08/19 11:34:38, minyue-webrtc wrote: > > On 2016/08/19 11:24:44, kwiberg-webrtc wrote: > > > > > > https://codereview.webrtc.org/2230823004/diff/160001/webrtc/voice_engine/leve... > > > File webrtc/voice_engine/level_indicator.cc (right): > > > > > > > > > https://codereview.webrtc.org/2230823004/diff/160001/webrtc/voice_engine/leve... > > > webrtc/voice_engine/level_indicator.cc:31: WebRtcSpl_Init(); > > > On 2016/08/18 09:53:08, minyue-webrtc wrote: > > > > On 2016/08/18 09:45:28, mflodman wrote: > > > > > On 2016/08/18 09:40:33, aleloi wrote: > > > > > > On 2016/08/18 09:34:13, mflodman wrote: > > > > > > > This has already been called in VoEBaseImpl::Init, should this > really > > be > > > > > done > > > > > > > here? > > > > > > > > > > > > I wanted to use the level indicator as a separate dependency in the > new > > > > audio > > > > > > mixer without having to depend on the whole of VoE. But then the mixer > > > > crashed > > > > > > in unit tests, because the SPL functions (which this component uses) > > were > > > > not > > > > > > initialized. > > > > > > > > > > > > My understanding is that if this component is going to be a separate > > build > > > > > > target, it should initialize its dependencies by itself. > > > > > > > > > > But is it ok to do this twice in the case you run this in VoiceEngine? > > > Should > > > > > this rather be done in the test itself? > > > > > > > > > > This might be right, but I don't know how SPLib initialization is > supposed > > > to > > > > > work and I think it's better if someone in the audio team looks at this > > then > > > > me. > > > > > > > > > > > > It reads that WebRtcSpl_Init() should be called only once (see below). so > we > > > > should not go wrong with it. But true, it should be called on a high > level. > > If > > > > possible, do it in the test? > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/third_party/webrtc/common_audio/signal_p... > > > > > > No, it's implemented so that it should be safe to call multiple times, even > > from > > > several different threads. > > > > Ah, that was what I meant: safe to call multiple times. once() does it > > automatically. But, my first sentence in that comment sounds truly wrong. > > Friendly ping. I'd like an LG from a VoE owner. I thought I removed myself as a reviewer in favor of kwiberg, who knows this code and would be a better approver than me.
mflodman@webrtc.org changed reviewers: - mflodman@webrtc.org
Removing myself for real.
Sorry, I'd missed that I was supposed to be reviewing this... https://codereview.webrtc.org/2230823004/diff/160001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/new_audio_conference_mixer.h (right): https://codereview.webrtc.org/2230823004/diff/160001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/new_audio_conference_mixer.h:67: virtual uint32_t GetOutputAudioLevelFullRange() = 0; Style guide says to use regular int for small integers like these. https://codereview.webrtc.org/2230823004/diff/160001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/new_audio_conference_mixer_impl.cc (right): https://codereview.webrtc.org/2230823004/diff/160001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/new_audio_conference_mixer_impl.cc:599: uint32_t level = static_cast<uint32_t>(current_level); Why two kinds of integers? And again, style guide says to use int unless you have a good reason not to. https://codereview.webrtc.org/2230823004/diff/160001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/new_audio_conference_mixer_impl.cc:607: uint32_t level = static_cast<uint32_t>(current_level); Same. https://codereview.webrtc.org/2230823004/diff/160001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc (right): https://codereview.webrtc.org/2230823004/diff/160001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:378: const int kSampleRateHz = 8000; Consider using constexpr instead of const when you can. https://codereview.webrtc.org/2230823004/diff/160001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:402: // input comprises frames with maximal values. Broken grammar. https://codereview.webrtc.org/2230823004/diff/160001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:405: const int kSampleRateHz = 8000; constexpr
Another pset. Ptal :) https://codereview.webrtc.org/2230823004/diff/160001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/new_audio_conference_mixer.h (right): https://codereview.webrtc.org/2230823004/diff/160001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/new_audio_conference_mixer.h:67: virtual uint32_t GetOutputAudioLevelFullRange() = 0; On 2016/08/23 12:58:26, kwiberg-webrtc wrote: > Style guide says to use regular int for small integers like these. Done. https://codereview.webrtc.org/2230823004/diff/160001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/new_audio_conference_mixer_impl.cc (right): https://codereview.webrtc.org/2230823004/diff/160001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/new_audio_conference_mixer_impl.cc:599: uint32_t level = static_cast<uint32_t>(current_level); On 2016/08/23 12:58:26, kwiberg-webrtc wrote: > Why two kinds of integers? And again, style guide says to use int unless you > have a good reason not to. Now fixed. Originally, the methods had similar signatures to GetSpeechOutputLevel*(...) from voe_volume_control. But that's not a good enough reason to not follow the style guide. https://codereview.webrtc.org/2230823004/diff/160001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/new_audio_conference_mixer_impl.cc:607: uint32_t level = static_cast<uint32_t>(current_level); On 2016/08/23 12:58:26, kwiberg-webrtc wrote: > Same. Done. https://codereview.webrtc.org/2230823004/diff/160001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc (right): https://codereview.webrtc.org/2230823004/diff/160001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:402: // input comprises frames with maximal values. On 2016/08/23 12:58:26, kwiberg-webrtc wrote: > Broken grammar. Thanks, fixed! https://codereview.webrtc.org/2230823004/diff/160001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:405: const int kSampleRateHz = 8000; On 2016/08/23 12:58:26, kwiberg-webrtc wrote: > constexpr Done.
lgtm, with a handful of optional nits https://codereview.webrtc.org/2230823004/diff/180001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/new_audio_conference_mixer_impl.cc (right): https://codereview.webrtc.org/2230823004/diff/180001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/new_audio_conference_mixer_impl.cc:598: int level = audio_level_.Level(); It's a good habit to make local variables const if possible (because it makes the code easier to read), the more so the longer the function is. https://codereview.webrtc.org/2230823004/diff/180001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc (right): https://codereview.webrtc.org/2230823004/diff/180001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:379: std::unique_ptr<NewAudioConferenceMixer> mixer( You can make mixer const (i.e., const std::unique_ptr<NewAudioConferenceMixer>), since you never reassign it. https://codereview.webrtc.org/2230823004/diff/180001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:391: for (size_t i = 0; i < 11; i++) { I'm pretty sure these indexes will fit in an int... https://codereview.webrtc.org/2230823004/diff/180001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:407: NewAudioConferenceMixer::Create(kId)); const here too.
Description was changed from ========== Added a level indicator to new mixer. Added a level indicator to the new mixer. The level indicator is webrtc::voe::AudioLevel. It computes the current audio level, which is used all the way up to peerconnection. This is part of the project to rewrite the old conference mixer and output mixer. ========== to ========== Added a level indicator to new mixer. Added a level indicator to the new mixer. The level indicator is webrtc::voe::AudioLevel. It computes the current audio level, which is used all the way up to peerconnection. This is part of the project to rewrite the old conference mixer and output mixer. NOTRY=True ==========
Thanks for the comments, everyone! https://codereview.webrtc.org/2230823004/diff/180001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/new_audio_conference_mixer_impl.cc (right): https://codereview.webrtc.org/2230823004/diff/180001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/new_audio_conference_mixer_impl.cc:598: int level = audio_level_.Level(); On 2016/08/24 08:04:38, kwiberg-webrtc wrote: > It's a good habit to make local variables const if possible (because it makes > the code easier to read), the more so the longer the function is. Done, thanks for the tip! https://codereview.webrtc.org/2230823004/diff/180001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc (right): https://codereview.webrtc.org/2230823004/diff/180001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:379: std::unique_ptr<NewAudioConferenceMixer> mixer( On 2016/08/24 08:04:39, kwiberg-webrtc wrote: > You can make mixer const (i.e., const std::unique_ptr<NewAudioConferenceMixer>), > since you never reassign it. I'll do it in one of the dependent CLs. I've changed names and moved things around, so it will be less manual merging this way. https://codereview.webrtc.org/2230823004/diff/180001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:391: for (size_t i = 0; i < 11; i++) { On 2016/08/24 08:04:39, kwiberg-webrtc wrote: > I'm pretty sure these indexes will fit in an int... This has been pointed out by ivoc@ in one of the dependent CLs. https://codereview.webrtc.org/2230823004/diff/180001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:407: NewAudioConferenceMixer::Create(kId)); On 2016/08/24 08:04:39, kwiberg-webrtc wrote: > const here too. Same as above.
The CQ bit was checked by aleloi@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from minyue@webrtc.org, kwiberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/2230823004/#ps200001 (title: "Made local variable const.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Added a level indicator to new mixer. Added a level indicator to the new mixer. The level indicator is webrtc::voe::AudioLevel. It computes the current audio level, which is used all the way up to peerconnection. This is part of the project to rewrite the old conference mixer and output mixer. NOTRY=True ========== to ========== Added a level indicator to new mixer. Added a level indicator to the new mixer. The level indicator is webrtc::voe::AudioLevel. It computes the current audio level, which is used all the way up to peerconnection. This is part of the project to rewrite the old conference mixer and output mixer. NOTRY=True ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Added a level indicator to new mixer. Added a level indicator to the new mixer. The level indicator is webrtc::voe::AudioLevel. It computes the current audio level, which is used all the way up to peerconnection. This is part of the project to rewrite the old conference mixer and output mixer. NOTRY=True ========== to ========== Added a level indicator to new mixer. Added a level indicator to the new mixer. The level indicator is webrtc::voe::AudioLevel. It computes the current audio level, which is used all the way up to peerconnection. This is part of the project to rewrite the old conference mixer and output mixer. NOTRY=True Committed: https://crrev.com/616df1e95c5ac70d288d83e694eefc8f78da6245 Cr-Commit-Position: refs/heads/master@{#13878} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/616df1e95c5ac70d288d83e694eefc8f78da6245 Cr-Commit-Position: refs/heads/master@{#13878} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
