Chromium Code Reviews
Help | Chromium Project | Sign in

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

Can't Edit
Can't Publish+Mail
Start Review
3 days, 7 hours ago by aleloi
2 hours, 12 minutes ago
AleBzk, peah-webrtc
CC:, AleBzk, 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: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+115 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 1 chunk +73 lines, -0 lines 4 comments Download
A webrtc/modules/audio_processing/test/ View 1 1 chunk +30 lines, -0 lines 1 comment Download
Trybot results: Sign in to try more bots
Commit queue not available (can’t edit this change).


Total messages: 12 (6 generated)
WDYT, was this roughly what we wanted?
3 days, 7 hours 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 ...
3 days, 7 hours ago (2017-04-21 09:39:14 UTC) #3
LGTM with a few comments + I added Per to check if he's fine with ...
3 days, 6 hours 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): ...
3 days, 5 hours 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 ...
5 hours, 31 minutes ago (2017-04-24 11:34:45 UTC) #11
2 hours, 12 minutes ago (2017-04-24 14:54:31 UTC) #12
Hi Alex,

Thanks for the changes.
Please, see my comments.
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
The comment looks incomplete.
webrtc/modules/audio_processing/test/analog_volume_mapper.h:35: bool
AnalogLevelIsValid(int level) {
From this method, I understand that different mappings require different ranges
for level. If I'm right, to me it looks like we're adding a coupling with
existing code that works within a specific range - which may be unavoidable.
In fact, once we get the mappings from actual devices, it could happen that
different devices have different ranges.

If this is the case, I would explain everything when you define
LevelToScalingMappingKind, otherwise the code may look too hacky/unclear.
webrtc/modules/audio_processing/test/analog_volume_mapper.h:38: return level_ <=
65535 && 0 <= level_;
You probably want to check level and not level_.

Just a personal taste, 0 <= level_ && level_ <= 65535 (which reads more like the
familiar 0 <= level_ <= 65535).
webrtc/modules/audio_processing/test/analog_volume_mapper.h:41: return level_ <=
255 && 0 <= level_;
Same here
File webrtc/modules/audio_processing/test/
webrtc/modules/audio_processing/test/ for
(int i = 0; i < 65535; ++i) {
Does this test pass? When you call set_analog_level(i),
RTC_DCHECK(AnalogLevelIsValid(level)) is called and it fails for
MappingKind::kLinear when i > 255.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cc6ac46