Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in

Issue 2837553002: First try on a fake analog volume control interface for use in audioproc_f. (Closed)

1 year ago by aleloi
11 months ago
AleBzk, peah-webrtc
CC:, peah-webrtc, Andrew MacDonald, aleloi,,, hlundin-webrtc, kwiberg-webrtc, minyue-webrtc, the sun, aluebs-webrtc, bjornv1
Target Ref:


First try on a fake analog volume control interface for use in audioproc_f. [DO NOT SUBMIT until the description is written] Issues that I see: * Namings - class / method / member * Documentation - can the comments be made better? BUG=webrtc:7494

Patch Set 1 #

Total comments: 12

Patch Set 2 : Removed polymorphism, added linear gain curve, enum for mapping type. #

Total comments: 10

Patch Set 3 : Comments addressed: smaller range, simpler checks, working test. #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+103 lines, -0 lines) Patch
M webrtc/modules/audio_processing/ View 1 3 chunks +12 lines, -0 lines 0 comments Download
A webrtc/modules/audio_processing/test/analog_volume_mapper.h View 1 2 1 chunk +61 lines, -0 lines 1 comment Download
A webrtc/modules/audio_processing/test/ View 1 2 1 chunk +30 lines, -0 lines 0 comments Download


Total messages: 15 (6 generated)
WDYT, was this roughly what we wanted?
1 year ago (2017-04-21 09:37:30 UTC) #2
aleloi File webrtc/modules/audio_processing/test/ (right): webrtc/modules/audio_processing/test/ The idea is to submit all subclasses of the ...
1 year ago (2017-04-21 09:39:14 UTC) #3
LGTM with a few comments + I added Per to check if he's fine with ...
1 year ago (2017-04-21 10:35:50 UTC) #5
Hi, Thanks for the CL. I have added some comments. File webrtc/modules/audio_processing/test/analog_volume_mapper.h (right): ...
1 year ago (2017-04-21 12:04:39 UTC) #9
PTAL next patch set! File webrtc/modules/audio_processing/test/analog_volume_mapper.h (right): webrtc/modules/audio_processing/test/analog_volume_mapper.h:24: class AnalogLevelMapper { On 2017/04/21 ...
12 months ago (2017-04-24 11:34:45 UTC) #11
Hi Alex, Thanks for the changes. Please, see my comments. File webrtc/modules/audio_processing/test/analog_volume_mapper.h (right): ...
12 months ago (2017-04-24 14:54:31 UTC) #12
aleloi File webrtc/modules/audio_processing/test/analog_volume_mapper.h (right): webrtc/modules/audio_processing/test/analog_volume_mapper.h:27: kIdentity, // Any level produces a On 2017/04/24 14:54:30, ...
12 months ago (2017-04-25 08:23:58 UTC) #13
On 2017/04/25 08:23:58, aleloi wrote: > > File webrtc/modules/audio_processing/test/analog_volume_mapper.h (right): > > > ...
12 months ago (2017-04-25 14:07:46 UTC) #14
12 months ago (2017-04-26 07:20:03 UTC) #15
Thanks for the new patch. Here comes some more comments.
File webrtc/modules/audio_processing/test/analog_volume_mapper.h (right):
webrtc/modules/audio_processing/test/analog_volume_mapper.h:26: enum class
LevelToScalingMappingKind {
I really think it would be easier to see how to best implement it if is
integrated in the simulator.

This class describes a mapping between a gain value, and a scaling. I think I
see how you want to apply that but it seems to me that it is not as
straightforward as it could be. But since I don't see the usage of the class, it
is hard to tell.

I'd have imagined a class that modules the analog microphone gain in such a
manner that it receives the microphone signal, and the analog gain provided by
APM, and returns a scaled microphone signal, together with the gain that was
actually applied. That is exactly how the analog gain effect appears to APM and
to me it would make sense to model it like this.

This class may be used as part of such a class though. but it does not look like
that with the setting and getting methods.
There are some things that get quite tricky to model in this class. For
instance, how do you handle a gain curve that applies soft saturation? That is
both gain and signal dependent, and to apply that you need both the gain, the
gain mapping and the signal as this is signal dependent.

Seeing the integrations of class, this implementation may fit really well into
that though. 
If not in a CL, maybe you could present or describe how exactly this is to be

Powered by Google App Engine
This is Rietveld 408576698