[asterisk-dev] [Code Review] Automatic Gain Normalization in meetme

[Mark Michelson]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/146/#review410
-----------------------------------------------------------


Unfortunately, I don't have much knowledge about the math involved here for calculating or correcting gains, so I can't comment on the correctness of the logic. I can, however, make comments about the actual C code, though, so that's what I'm doing :)


trunk/apps/app_meetme.c
<http://reviewboard.digium.com/r/146/#comment972>

    When I tried compiling this, I got a warning that the shift count was >= to the width of the type.
    
    If you need to add a new flag, it seems like what will need to be done is what is done now in app_dial, where flags beyond 1 << 31 are added as #defines and cast as uint64_t. Luckily, this will be a bit easier to correct than app_dial since it appears that app_meetme does not use the ast_flags macros. You can probably change the confflags variable to be a uint64_t to accommodate the addition of new flags.



trunk/apps/app_meetme.c
<http://reviewboard.digium.com/r/146/#comment973>

    Move variable declarations to the top of a code block.



trunk/main/dsp.c
<http://reviewboard.digium.com/r/146/#comment975>

    Since you are setting each element of the array to the same value and know the size of the array, you can replace the entire body of this function with:
    
    memset(dsp->gain_buckets, 0, NUMGAINBUCKETS * sizeof(dsp->gain_buckets[0]);
    
    (note: you could use sizeof(int) instead, but this is less error-prone in case the type of dsp->gainbuckets changes)



trunk/main/dsp.c
<http://reviewboard.digium.com/r/146/#comment977>

    To conform to coding guidelines, be sure that you place spaces around assignment operators and any comparison operators inside a for loop. This one would look like this:
    
    for (i = 0; i < NUMGAINBUCKETS; i++) {



trunk/main/dsp.c
<http://reviewboard.digium.com/r/146/#comment976>

    I think this line would read more clearly as
    
    len = f->samples;



trunk/main/dsp.c
<http://reviewboard.digium.com/r/146/#comment979>

    These two lines could be replaced with a call to ast_dsp_gain_reset(dsp)


- Mark


On 2009-02-08 13:24:27, chetanv wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/146/
> -----------------------------------------------------------
> 
> (Updated 2009-02-08 13:24:27)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> Often during an audio conference, one of the participants has set a high mic volume. Whenever he speaks, the other listeners hear his relatively high volume. Similarly, someone has a low voice or has set a low mic volume. Other participants have to strain to hear him.
> 
> This patch tries to overcome this problem by estimating the volume of the participants and adjusting their gain. This adjustment is done every 2 seconds in the meetme app.
> 
> The patch adds a "g" flag to the meetme app. This option is tied up with the "T" flag - talker detection. Both have to be on for this to work.
> 
> Changes are in apps/app_meetme.c, include/asterisk/dsp.h, main/dsp.c
> 
> In app_meetme, if the user is talking, his frame is used for gain related calculations (ast_dsp_gain()) and his volume is adjusted using the current value of the gain multiplication factor.
> The current gain multiplication factor is based on the past 100 frames of that user (past 2 seconds).
> 
> Three new fields have been added to ast_dsp structure:
>   * gain_mult_factor: The gain multiplication factor
>   * gain_buckets[]: The buckets array used in estimating the value of the gain multiplication factor
>   * numgainframes: Number of gain frames which have been processed
> 
> For every frame, the RMS gain (actually - square of RMS) is calculated and bucketed into 10 buckets (histogram). After 100 frames are bucketed this way, the histogram is used to estimate the gain multiplication factor. Starting from the right side of the histogram, the bucket with at-least 4 frames is picked and the corresponding gain multiplication factor is chosen. (There is a on-to-one mapping between the buckets of the histogram and the gain multiplication factor - GMF_VALUES[])
> 
> 
> Diffs
> -----
> 
>   trunk/apps/app_meetme.c 169716 
>   trunk/include/asterisk/dsp.h 169716 
>   trunk/main/dsp.c 169716 
> 
> Diff: http://reviewboard.digium.com/r/146/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> chetanv
> 
>