Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(6)

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

Created:
7 months, 1 week ago by aleloi
Modified:
6 months, 1 week ago
Reviewers:
AleBzk, peah-webrtc
CC:
webrtc-reviews_webrtc.org, peah-webrtc, Andrew MacDonald, aleloi, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, kwiberg-webrtc, minyue-webrtc(BackIn2018March), the sun, aluebs-webrtc, bjornv1
Target Ref:
refs/heads/master
Project:
webrtc
Visibility:
Public.

Description

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/BUILD.gn 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/analog_volume_mapper_unittest.cc View 1 2 1 chunk +30 lines, -0 lines 0 comments Download
Trybot results:

Messages

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

https://codereview.webrtc.org/2837553002/diff/80001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/test/analog_volume_mapper.h (right):

https://codereview.webrtc.org/2837553002/diff/80001/webrtc/modules/audio_proc...
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
used?

Powered by Google App Engine
This is Rietveld efc10ee0f