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

David Vossel reviewboard at asterisk.org
Tue Feb 22 14:56:08 CST 2011 


> On 2011-02-21 21:47:09, Russell Bryant wrote:
> > /trunk/include/asterisk/time.h, lines 171-175
> > <https://reviewboard.asterisk.org/r/1104/diff/3/?file=15596#file15596line171>
> >
> >     It's not immediately obvious to me that this is correct.  I just wanted to make sure you tested it.

This is correct. I made this exact same patch a couple of years ago and someone suggested the (4000000 / _rate )) / 4 trick to avoid using floating point variables.  I didn't like it at the time because I knew it would change but did it anyway since it was technically ok.  Now that we support a ridiculous number of sample rates this no longer holds true and floating point calculations must be used.


> On 2011-02-21 21:47:09, Russell Bryant wrote:
> > /trunk/main/format.c, lines 1122-1125
> > <https://reviewboard.asterisk.org/r/1104/diff/3/?file=15602#file15602line1122>
> >
> >     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.

Umm, It does reload every time a new attribute interface is introduced, but there is no way to reload it after startup is complete.  Attribute modules are set to load before channel drivers, that was the primary reason that reloads after startup are not possible.  Its easy to reload the config, but it appeared to me that lots of channel drivers wanted to know about all possible formats on load for the channel_tech object's capabilities.  I made the load function remove all dynamic formats on reload just in case we could use reload after start up someday.  So, this is not a no-op.


> On 2011-02-21 21:47:09, Russell Bryant wrote:
> > /trunk/bridges/bridge_softmix.c, line 55
> > <https://reviewboard.asterisk.org/r/1104/diff/3/?file=15568#file15568line55>
> >
> >     why this value?  (Just add a comment above it)

Fixed.


> On 2011-02-21 21:47:09, Russell Bryant wrote:
> > /trunk/bridges/bridge_multiplexed.c, line 248
> > <https://reviewboard.asterisk.org/r/1104/diff/3/?file=15567#file15567line248>
> >
> >     For consistency, ao2_unlock(bridge), since the surrounding code is using the bridge pointer directly.

fixed


> On 2011-02-21 21:47:09, Russell Bryant wrote:
> > /trunk/bridges/bridge_softmix.c, lines 87-90
> > <https://reviewboard.asterisk.org/r/1104/diff/3/?file=15568#file15568line87>
> >
> >     unsigned for the sample rate

fixed


> On 2011-02-21 21:47:09, Russell Bryant wrote:
> > /trunk/bridges/bridge_softmix.c, lines 97-99
> > <https://reviewboard.asterisk.org/r/1104/diff/3/?file=15568#file15568line97>
> >
> >     As a matter of style, I would use sizeof(*bridge_data) instead of sizeof(struct softmix_bridge_data)

fixed


> On 2011-02-21 21:47:09, Russell Bryant wrote:
> > /trunk/bridges/bridge_softmix.c, lines 138-142
> > <https://reviewboard.asterisk.org/r/1104/diff/3/?file=15568#file15568line138>
> >
> >     Should anything be done here if there is a failure?

It won't fail here.


> On 2011-02-21 21:47:09, Russell Bryant wrote:
> > /trunk/bridges/bridge_softmix.c, lines 335-346
> > <https://reviewboard.asterisk.org/r/1104/diff/3/?file=15568#file15568line335>
> >
> >     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.

There are problems with both of these approaches.

The current approach always picks the highest sample rate if two or more channels exist that are above the current internal sample rate.  This means if a 16khz and 32khz person joins while the internal rate is 8khz, the conference will jump to 32khz even though 16khz would have been fine.

If we limit the jump to occur only when two people at the same sample rate join then the internal rate would stay at 8khz even though both a 16khz and 32khz person are present.  This is not ideal either.

I fixed this with a new algorithm that first picks the highest rate two or more channels support over the internal rate, or second if multiple channels are above the internal rate but no two channels have the same sample rate, it takes the lowest one supported over the internal rate.  In the case of picking the lowest sample rate above the internal sample rate, it is possible two sample rate adjustments may occur to find the optimal rate, but this would be incredibly rare.


> On 2011-02-21 21:47:09, Russell Bryant wrote:
> > /trunk/codecs/codec_resample.c, lines 60-72
> > <https://reviewboard.asterisk.org/r/1104/diff/3/?file=15576#file15576line60>
> >
> >     Are any of these used outside of this file?  If not, make them static.

fixed


> On 2011-02-21 21:47:09, Russell Bryant wrote:
> > /trunk/codecs/codec_resample.c, lines 74-77
> > <https://reviewboard.asterisk.org/r/1104/diff/3/?file=15576#file15576line74>
> >
> >     Do you need the resamp_pvt struct here?  It looks like ast_trans_pvt->pvt could just be a SpeexResamplerState pointer directly.

fixed


> On 2011-02-21 21:47:09, Russell Bryant wrote:
> > /trunk/codecs/codec_resample.c, lines 143-152
> > <https://reviewboard.asterisk.org/r/1104/diff/3/?file=15576#file15576line143>
> >
> >     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.

Yes, this appears to be the case as long as the out buffer is large enough to contain all the samples which it is.


> On 2011-02-21 21:47:09, Russell Bryant wrote:
> > /trunk/formats/format_attr_silk.c, lines 39-43
> > <https://reviewboard.asterisk.org/r/1104/diff/3/?file=15585#file15585line39>
> >
> >     unsigned?

fixed


> On 2011-02-21 21:47:09, Russell Bryant wrote:
> > /trunk/formats/format_attr_silk.c, lines 57-66
> > <https://reviewboard.asterisk.org/r/1104/diff/3/?file=15585#file15585line57>
> >
> >     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.

fixed, and i have no idea why this was written like this.


> On 2011-02-21 21:47:09, Russell Bryant wrote:
> > /trunk/formats/format_attr_silk.c, lines 203-205
> > <https://reviewboard.asterisk.org/r/1104/diff/3/?file=15585#file15585line203>
> >
> >     AST_MODULE_LOAD_DECLINE is probably a better choice

fixed


> On 2011-02-21 21:47:09, Russell Bryant wrote:
> > /trunk/funcs/func_pitchshift.c, line 212
> > <https://reviewboard.asterisk.org/r/1104/diff/3/?file=15586#file15586line212>
> >
> >     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.

Blahah


> On 2011-02-21 21:47:09, Russell Bryant wrote:
> > /trunk/include/asterisk/audiohook.h, lines 69-71
> > <https://reviewboard.asterisk.org/r/1104/diff/3/?file=15589#file15589line69>
> >
> >     Add /*! to doxygenize this

fixed


> On 2011-02-21 21:47:09, Russell Bryant wrote:
> > /trunk/include/asterisk/audiohook.h, lines 75-76
> > <https://reviewboard.asterisk.org/r/1104/diff/3/?file=15589#file15589line75>
> >
> >     Can these just be moved to audiohook.c ?

fixed


> On 2011-02-21 21:47:09, Russell Bryant wrote:
> > /trunk/include/asterisk/audiohook.h, line 119
> > <https://reviewboard.asterisk.org/r/1104/diff/3/?file=15589#file15589line119>
> >
> >     unsigned?

fixed


> On 2011-02-21 21:47:09, Russell Bryant wrote:
> > /trunk/include/asterisk/format.h, lines 104-105
> > <https://reviewboard.asterisk.org/r/1104/diff/3/?file=15590#file15590line104>
> >
> >     taking this too far?  Maybe we should add something even more ridiculous just so we can claim to support it.  Marketing!

3072khz audio!


> On 2011-02-21 21:47:09, Russell Bryant wrote:
> > /trunk/include/asterisk/format.h, lines 435-439
> > <https://reviewboard.asterisk.org/r/1104/diff/3/?file=15590#file15590line435>
> >
> >     This should be in _private.h.

fixed


> On 2011-02-21 21:47:09, Russell Bryant wrote:
> > /trunk/include/asterisk/format.h, lines 440-464
> > <https://reviewboard.asterisk.org/r/1104/diff/3/?file=15590#file15590line440>
> >
> >     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.

uhh, ok.   I don't really care.  I'm just making these normal functions.  They are not used enough to matter.


> On 2011-02-21 21:47:09, Russell Bryant wrote:
> > /trunk/include/asterisk/format.h, line 462
> > <https://reviewboard.asterisk.org/r/1104/diff/3/?file=15590#file15590line462>
> >
> >     make rate unsigned
> >     
> >     move { to next line

fixed


> On 2011-02-21 21:47:09, Russell Bryant wrote:
> > /trunk/include/asterisk/rtp_engine.h, lines 1823-1824
> > <https://reviewboard.asterisk.org/r/1104/diff/3/?file=15593#file15593line1823>
> >
> >     move to _private.h

fixed


> On 2011-02-21 21:47:09, Russell Bryant wrote:
> > /trunk/main/format.c, lines 610-611
> > <https://reviewboard.asterisk.org/r/1104/diff/3/?file=15602#file15602line610>
> >
> >     const?

fixxxxxed


> On 2011-02-21 21:47:09, Russell Bryant wrote:
> > /trunk/main/format.c, line 1127
> > <https://reviewboard.asterisk.org/r/1104/diff/3/?file=15602#file15602line1127>
> >
> >     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.

fixed


> On 2011-02-21 21:47:09, Russell Bryant wrote:
> > /trunk/main/format_cap.c, lines 299-303
> > <https://reviewboard.asterisk.org/r/1104/diff/3/?file=15603#file15603line299>
> >
> >     I think the unref needs to be moved to right before the return.

and this is what review is for :)


- David


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


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/1916030b/attachment-0001.htm>

More information about the asterisk-dev mailing list