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

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
Created:
3 days, 7 hours ago by aleloi
Modified:
2 hours, 12 minutes ago
Reviewers:
AleBzk, peah-webrtc
CC:
webrtc-reviews_webrtc.org, AleBzk, peah-webrtc, Andrew MacDonald, aleloi, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, kwiberg-webrtc, minyue-webrtc, 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: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+115 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 1 chunk +73 lines, -0 lines 4 comments Download
A webrtc/modules/audio_processing/test/analog_volume_mapper_unittest.cc 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).

Messages

Total messages: 12 (6 generated)
aleloi
WDYT, was this roughly what we wanted?
3 days, 7 hours 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 ...
3 days, 7 hours 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 ...
3 days, 6 hours 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 ...
3 days, 5 hours 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 ...
5 hours, 31 minutes ago (2017-04-24 11:34:45 UTC) #11
AleBzk
2 hours, 12 minutes ago (2017-04-24 14:54:31 UTC) #12
Hi Alex,

Thanks for the changes.
Please, see my comments.

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

https://codereview.webrtc.org/2837553002/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/analog_volume_mapper.h:27: kIdentity,  //
Any level produces a
The comment looks incomplete.

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

https://codereview.webrtc.org/2837553002/diff/60001/webrtc/modules/audio_proc...
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).

https://codereview.webrtc.org/2837553002/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/analog_volume_mapper.h:41: return level_ <=
255 && 0 <= level_;
Same here

https://codereview.webrtc.org/2837553002/diff/60001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/test/analog_volume_mapper_unittest.cc
(right):

https://codereview.webrtc.org/2837553002/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/analog_volume_mapper_unittest.cc:22: 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