[asterisk-dev] [Code Review] Media Project Phase 2: SILK 8khz-24khz, SLINEAR 8khz-192khz, SPEEX 32khz, Custom format creation in codecs.conf

Russell Bryant reviewboard at asterisk.org
Mon Feb 21 21:47:09 CST 2011 

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/1104/#review3202
-----------------------------------------------------------



/trunk/bridges/bridge_multiplexed.c
<https://reviewboard.asterisk.org/r/1104/#comment6638>

    For consistency, ao2_unlock(bridge), since the surrounding code is using the bridge pointer directly.



/trunk/bridges/bridge_softmix.c
<https://reviewboard.asterisk.org/r/1104/#comment6615>

    why this value?  (Just add a comment above it)



/trunk/bridges/bridge_softmix.c
<https://reviewboard.asterisk.org/r/1104/#comment6639>

    unsigned for the sample rate



/trunk/bridges/bridge_softmix.c
<https://reviewboard.asterisk.org/r/1104/#comment6640>

    As a matter of style, I would use sizeof(*bridge_data) instead of sizeof(struct softmix_bridge_data)



/trunk/bridges/bridge_softmix.c
<https://reviewboard.asterisk.org/r/1104/#comment6616>

    Should anything be done here if there is a failure?



/trunk/bridges/bridge_softmix.c
<https://reviewboard.asterisk.org/r/1104/#comment6641>

    It looks like this isn't doing exactly what the comment says.  The intention is to update the native bridge sample rate if at least 2 people are at a higher rate.  It does that part.  However, the rate it chooses is the highest rate that was encountered.  It may be that there is only 1 channel at that rate, though.
    
    I think there is some tweaking needed here to ensure that the rate jumped to is one where there are at least 2 channels.



/trunk/channels/iax2.h
<https://reviewboard.asterisk.org/r/1104/#comment6642>

    Yay constification!



/trunk/codecs/codec_resample.c
<https://reviewboard.asterisk.org/r/1104/#comment6643>

    Are any of these used outside of this file?  If not, make them static.



/trunk/codecs/codec_resample.c
<https://reviewboard.asterisk.org/r/1104/#comment6644>

    Do you need the resamp_pvt struct here?  It looks like ast_trans_pvt->pvt could just be a SpeexResamplerState pointer directly.



/trunk/codecs/codec_resample.c
<https://reviewboard.asterisk.org/r/1104/#comment6645>

    Wow, this API sure is easier.  :-)
    
    Is it guaranteed to process all of the data?  As you could probably tell from the old code, I had to call the resampler in a loop to ensure it processed everything.  I don't know, it seemed silly, but that's what I had to do to make it work.



/trunk/codecs/codec_resample.c
<https://reviewboard.asterisk.org/r/1104/#comment6646>

    or if you want to be overly explicit, * sizeof(int16_t).



/trunk/formats/format_attr_silk.c
<https://reviewboard.asterisk.org/r/1104/#comment6647>

    unsigned?



/trunk/formats/format_attr_silk.c
<https://reviewboard.asterisk.org/r/1104/#comment6648>

    The placement of the declaration of val looks bizarre.  How about just putting it at the beginning of the function?
    
    Also, it looks like just setting val = result when val is declared would be sufficient.  It's set to the same thing inside of each case in the switch.



/trunk/formats/format_attr_silk.c
<https://reviewboard.asterisk.org/r/1104/#comment6649>

    AST_MODULE_LOAD_DECLINE is probably a better choice



/trunk/funcs/func_pitchshift.c
<https://reviewboard.asterisk.org/r/1104/#comment6650>

    Dude, this might need to be covered in the CHANGES file.  The fact that PITCH_SHIFT() can now operate on _ANY_ sample rate is kind of a big deal.



/trunk/include/asterisk/audiohook.h
<https://reviewboard.asterisk.org/r/1104/#comment6635>

    Add /*! to doxygenize this



/trunk/include/asterisk/audiohook.h
<https://reviewboard.asterisk.org/r/1104/#comment6636>

    Can these just be moved to audiohook.c ?



/trunk/include/asterisk/audiohook.h
<https://reviewboard.asterisk.org/r/1104/#comment6637>

    unsigned?



/trunk/include/asterisk/format.h
<https://reviewboard.asterisk.org/r/1104/#comment6651>

    taking this too far?  Maybe we should add something even more ridiculous just so we can claim to support it.  Marketing!



/trunk/include/asterisk/format.h
<https://reviewboard.asterisk.org/r/1104/#comment6652>

    This should be in _private.h.



/trunk/include/asterisk/format.h
<https://reviewboard.asterisk.org/r/1104/#comment6653>

    I'm not sure that it's really necessary to have these functions as inline functions... but if they stay, they are good candidates for using the AST_INLINE_API() macro instead of force_inline.



/trunk/include/asterisk/format.h
<https://reviewboard.asterisk.org/r/1104/#comment6654>

    make rate unsigned
    
    move { to next line



/trunk/include/asterisk/format_cap.h
<https://reviewboard.asterisk.org/r/1104/#comment6655>

    \ in front of retval



/trunk/include/asterisk/rtp_engine.h
<https://reviewboard.asterisk.org/r/1104/#comment6656>

    move to _private.h



/trunk/include/asterisk/time.h
<https://reviewboard.asterisk.org/r/1104/#comment6657>

    It's not immediately obvious to me that this is correct.  I just wanted to make sure you tested it.



/trunk/main/format.c
<https://reviewboard.asterisk.org/r/1104/#comment6658>

    const?



/trunk/main/format.c
<https://reviewboard.asterisk.org/r/1104/#comment6659>

    reload?  Does this actually support reload?  If so, I think there is some additional work required here.  If not, this seems like a no-op.



/trunk/main/format.c
<https://reviewboard.asterisk.org/r/1104/#comment6660>

    This function is a little big and gets pretty deep indentation wise.  I think some of this could be broken out into some more functions such as add_format(cat), process_samprate(), to clean things up a bit.



/trunk/main/format_cap.c
<https://reviewboard.asterisk.org/r/1104/#comment6661>

    I think the unref needs to be moved to right before the return.


- Russell


On 2011-02-17 17:32:14, David Vossel wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1104/
> -----------------------------------------------------------
> 
> (Updated 2011-02-17 17:32:14)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> -Functional Changes
> Dynamic global format list build by codecs defined in codecs.conf
> SILK 8khz, 12khz, 16khz, and 24khz with custom attributes defined in codecs.conf
> Negotiation of SILK attributes in chan_sip.
> SPEEX 32khz with translation
> SLINEAR 8khz, 12khz, 24khz, 32khz, 44.1khz, 48khz, 96khz, 192khz with translation using codec_resample.c
> Various changes to RTP code required to properly handle the dynamic format list and formats with attributes.
> 
> -Organizational changes
> Global format list is moved from frame.c to format.c
> Various format specific functions moved from frame.c to format.c
> 
> -MIS
> There were several changes that I opted not to do during phase 1 that involved the proper use of formats with attributes.  I felt that the phase 1 patch was too complex to attempt these changes so I have included them here.  The primary change I held off doing the first time around was in the rtp_engine.c Payload and MIME type lists.  These lists were statically defined which made it impossible to set formats with attributes in them.  I have gone back and revised these lists to allow the flexibility required for the new architecture.
> 
> 
> Diffs
> -----
> 
>   /trunk/CHANGES 308241 
>   /trunk/apps/app_chanspy.c 308241 
>   /trunk/apps/app_jack.c 308241 
>   /trunk/apps/app_mixmonitor.c 308241 
>   /trunk/bridges/bridge_multiplexed.c 308241 
>   /trunk/bridges/bridge_softmix.c 308241 
>   /trunk/channels/chan_gtalk.c 308241 
>   /trunk/channels/chan_iax2.c 308241 
>   /trunk/channels/chan_jingle.c 308241 
>   /trunk/channels/chan_sip.c 308241 
>   /trunk/channels/chan_skinny.c 308241 
>   /trunk/channels/iax2.h 308241 
>   /trunk/codecs/Makefile 308241 
>   /trunk/codecs/codec_resample.c 308241 
>   /trunk/codecs/codec_speex.c 308241 
>   /trunk/codecs/speex/arch.h PRE-CREATION 
>   /trunk/codecs/speex/fixed_generic.h PRE-CREATION 
>   /trunk/codecs/speex/resample.c PRE-CREATION 
>   /trunk/codecs/speex/resample_sse.h PRE-CREATION 
>   /trunk/codecs/speex/speex_resampler.h PRE-CREATION 
>   /trunk/codecs/speex/stack_alloc.h PRE-CREATION 
>   /trunk/configs/codecs.conf.sample 308241 
>   /trunk/formats/format_attr_silk.c PRE-CREATION 
>   /trunk/funcs/func_pitchshift.c 308241 
>   /trunk/funcs/func_speex.c 308241 
>   /trunk/funcs/func_volume.c 308241 
>   /trunk/include/asterisk/audiohook.h 308241 
>   /trunk/include/asterisk/format.h 308241 
>   /trunk/include/asterisk/format_cap.h 308241 
>   /trunk/include/asterisk/frame.h 308241 
>   /trunk/include/asterisk/rtp_engine.h 308241 
>   /trunk/include/asterisk/silk.h PRE-CREATION 
>   /trunk/include/asterisk/slinfactory.h 308241 
>   /trunk/include/asterisk/time.h 308241 
>   /trunk/include/asterisk/translate.h 308241 
>   /trunk/main/asterisk.c 308241 
>   /trunk/main/audiohook.c 308241 
>   /trunk/main/channel.c 308241 
>   /trunk/main/data.c 308241 
>   /trunk/main/format.c 308241 
>   /trunk/main/format_cap.c 308241 
>   /trunk/main/format_pref.c 308241 
>   /trunk/main/frame.c 308241 
>   /trunk/main/rtp_engine.c 308241 
>   /trunk/main/slinfactory.c 308241 
>   /trunk/main/translate.c 308241 
>   /trunk/res/res_mutestream.c 308241 
>   /trunk/res/res_rtp_asterisk.c 308241 
> 
> Diff: https://reviewboard.asterisk.org/r/1104/diff
> 
> 
> Testing
> -------
> 
> I used the same testing I did for phase1 with phase2.  This includes load testing with various codec negotiation scenarios.  Speex 32 and its translators were verified using Asterisk back to back.  I verified SILK using a codec translator module I'm working with.  The new slinear resample code was exercised with both Speex32 and SILK 12khz/24khz translation.
> 
> 
> Thanks,
> 
> David
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20110222/e273187b/attachment-0001.htm>

More information about the asterisk-dev mailing list