Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2837553002:
First try on a fake analog volume control interface for use in audioproc_f. (Closed)
|
Created:
3 years, 8 months ago by aleloi Modified:
3 years, 7 months ago CC:
webrtc-reviews_webrtc.org, 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. |
DescriptionFirst 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
Messages
Total messages: 15 (6 generated)
aleloi@webrtc.org changed reviewers: + alessiob@webrtc.org
WDYT, was this roughly what we wanted?
https://codereview.webrtc.org/2837553002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/test/analog_volume_mapper_unittest.cc (right): https://codereview.webrtc.org/2837553002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/analog_volume_mapper_unittest.cc:13: The idea is to submit all subclasses of the VolumeMapper to this test. Perhaps it would be better to make the test call a function that takes an *AnalogVolumeMapper instead.
alessiob@webrtc.org changed reviewers: + peah@webrtc.org
LGTM with a few comments + I added Per to check if he's fine with the interface name. https://codereview.webrtc.org/2837553002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/test/analog_volume_mapper.h (right): https://codereview.webrtc.org/2837553002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/analog_volume_mapper.h:24: class AnalogLevelMapper { I thought about the class name and personally I'm ok with it, but I'd like to hear Per's opinion. https://codereview.webrtc.org/2837553002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/test/analog_volume_mapper_unittest.cc (right): https://codereview.webrtc.org/2837553002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/analog_volume_mapper_unittest.cc:13: On 2017/04/21 09:39:14, aleloi wrote: > The idea is to submit all subclasses of the VolumeMapper to this test. Perhaps > it would be better to make the test call a function that takes an > *AnalogVolumeMapper instead. Acknowledged. https://codereview.webrtc.org/2837553002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/test/identity_analog_volume_mapper.h (right): https://codereview.webrtc.org/2837553002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/identity_analog_volume_mapper.h:21: class IdentityAnalogLevelMapper : public AnalogLevelMapper { Great to have this! We might want to use it in audioproc_f when the simulation is disabled (only if this improves readability).
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/...
Patchset #2 (id:20001) has been deleted
Hi, Thanks for the CL. I have added some comments. https://codereview.webrtc.org/2837553002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/test/analog_volume_mapper.h (right): https://codereview.webrtc.org/2837553002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/analog_volume_mapper.h:24: class AnalogLevelMapper { I definitely think that this CL should include the code hooking this up into audioproc_f. It is hard to properly review this code without seeing the vision for how it is to be used. https://codereview.webrtc.org/2837553002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/analog_volume_mapper.h:24: class AnalogLevelMapper { An alternative to an interface would be to have a class that implements a number of mappings. The actual mapping is typically very short, sometimes even a one-liner, and having a separate class for each mapping causes a lot of boilerplate code to be re-created just to add a new mapping. My suggestion is to wait with creating an interface until you have two implementations of the interface. That is, go with a proper class for the first implementation, and then change to having an interface when you add the second implementation. Then you will know what overlap there is on the code. https://codereview.webrtc.org/2837553002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/analog_volume_mapper.h:29: // be within [0, 65535]. Is the maximum level really that high, I thought it was lower, maybe 128 or 256. https://codereview.webrtc.org/2837553002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/test/identity_analog_volume_mapper.h (right): https://codereview.webrtc.org/2837553002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/identity_analog_volume_mapper.h:21: class IdentityAnalogLevelMapper : public AnalogLevelMapper { I don't see the purpose of this class. I don't think we should simulate the mic-gain, if we don't need to. If we want to have an example implementation of the analog mapper I'd rather have a usable one, like a linear mapping, or a dB mapping.
Patchset #2 (id:40001) has been deleted
PTAL next patch set! https://codereview.webrtc.org/2837553002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/test/analog_volume_mapper.h (right): https://codereview.webrtc.org/2837553002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/analog_volume_mapper.h:24: class AnalogLevelMapper { On 2017/04/21 12:04:39, peah-webrtc wrote: > I definitely think that this CL should include the code hooking this up into > audioproc_f. It is hard to properly review this code without seeing the vision > for how it is to be used. Can I make a dependent CL for that after https://codereview.webrtc.org/2834643002/ has landed? https://codereview.webrtc.org/2837553002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/analog_volume_mapper.h:24: class AnalogLevelMapper { On 2017/04/21 12:04:39, peah-webrtc wrote: > An alternative to an interface would be to have a class that implements a number > of mappings. > > The actual mapping is typically very short, sometimes even a one-liner, and > having a separate class for each mapping causes a lot of boilerplate code to be > re-created just to add a new mapping. > > My suggestion is to wait with creating an interface until you have two > implementations of the interface. That is, go with a proper class for the first > implementation, and then change to having an interface when you add the second > implementation. Then you will know what overlap there is on the code. That makes sense. I've changed into const enum to choose the mapping. The enum is set in the c-tor. https://codereview.webrtc.org/2837553002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/analog_volume_mapper.h:29: // be within [0, 65535]. On 2017/04/21 12:04:39, peah-webrtc wrote: > Is the maximum level really that high, I thought it was lower, maybe 128 or 256. In the documentation to set_analog_level_limits it says [0, 65535]. In practice, 0 to 255 seems to be used: k{Max, Min}VolumeLevel in voice_engine_defines.h is 0 and 255. In unit tests the level is set to larger values. https://codereview.webrtc.org/2837553002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/test/identity_analog_volume_mapper.h (right): https://codereview.webrtc.org/2837553002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/identity_analog_volume_mapper.h:21: class IdentityAnalogLevelMapper : public AnalogLevelMapper { On 2017/04/21 12:04:39, peah-webrtc wrote: > I don't see the purpose of this class. I don't think we should simulate the > mic-gain, if we don't need to. If we want to have an example implementation of > the analog mapper I'd rather have a usable one, like a linear mapping, or a dB > mapping. I wanted to always use an AnalogLevelMapper instance in audioproc_f, even when mic simulation is disabled. That way there would be less special cases between mic simulation on and off. One way to do that was to use a fake mic that doesn't affect the signal. Hence the Identity..LevelMapper.
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.
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 On 2017/04/24 14:54:30, AleBzk wrote: > The comment looks incomplete. Thanks, done. 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) { On 2017/04/24 14:54:30, AleBzk wrote: > 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. I couldn't find a range outside 0, 255 except in tests. I've changed it to [0, 255]. We can make the range larger later if needed. 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_; On 2017/04/24 14:54:30, AleBzk wrote: > 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). Oh, good catch! Done. 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_; On 2017/04/24 14:54:30, AleBzk wrote: > Same here Done. 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) { On 2017/04/24 14:54:30, AleBzk wrote: > 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. You are correct. I forgot to run the tests. See next patch (with tests running on bots).
On 2017/04/25 08:23:58, aleloi wrote: > 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 > On 2017/04/24 14:54:30, AleBzk wrote: > > The comment looks incomplete. > > Thanks, done. > > 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) { > On 2017/04/24 14:54:30, AleBzk wrote: > > 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. > > I couldn't find a range outside 0, 255 except in tests. I've changed it to [0, > 255]. We can make the range larger later if needed. > > 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_; > On 2017/04/24 14:54:30, AleBzk wrote: > > 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). > > Oh, good catch! Done. > > 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_; > On 2017/04/24 14:54:30, AleBzk wrote: > > Same here > > Done. > > 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) { > On 2017/04/24 14:54:30, AleBzk wrote: > > 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. > > You are correct. I forgot to run the tests. See next patch (with tests running > on bots). lgtm
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? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
