[asterisk-dev] [Code Review] Asterisk media architecture conversion - no more format bitfields

David Vossel reviewboard at asterisk.org
Wed Jan 26 09:34:18 CST 2011 


> On 2011-01-25 12:31:07, Russell Bryant wrote:
> > /trunk/main/dial.c, line 260
> > <https://reviewboard.asterisk.org/r/1083/diff/3/?file=15420#file15420line260>
> >
> >     I wonder if it would be better to have just one alloc function and have it take a bit field of allocation options.
> 
> David Vossel wrote:
>     This is similar to the way we initialize linked lists I believe.  I chose this instead of passing an argument because it was more explicit.  Its much more obvious if some sort of crash occurs in a channel driver during access of a capabilities structure that locking may be an issue when the function allocating the container explicitly has "nolock" in the name.
> 
> Russell Bryant wrote:
>     how is that more explicit than ast_format_cap_alloc(AST_FORMAT_CAP_ALLOC_NOLOCK); ?

I can go for this.


> On 2011-01-25 12:31:07, Russell Bryant wrote:
> > /trunk/main/format.c, line 133
> > <https://reviewboard.asterisk.org/r/1083/diff/3/?file=15424#file15424line133>
> >
> >     I'm thinking that you can just get rid of the set_attributes argument here.  You can infer that there are no attributes if the attributes section begins with AST_FORMAT_ATTR_END;
> >     
> >     So ...
> >     
> >     ast_format_set(format, AST_FORMAT_SLINEAR, AST_FORMAT_ATTR_END);
> >     
> >     or ...
> >     
> >     ast_format_set(format, AST_FORMAT_SLINEAR,
> >         AST_FORMAT_ATTR_FOO, 1234,
> >         AST_FORMAT_ATTR_END);
> 
> David Vossel wrote:
>     I chose not to do this because I didn't want to write AST_FORMAT_ATTR_END  1000 times, '0' was easier.  That is my only reasoning.   It would not be hard to make this change using sed.  I do not feel strongly about this one way or the other.  What do you think?
> 
> Russell Bryant wrote:
>     0 is easier, but I just don't like an argument that doesn't provide value.  If it's reasonably quick to search and replace, I'd like to change it.
> 
> David Vossel wrote:
>     I went back to look at this implementation and remembered the primary reason this argument was introduced.  The va_list is never accessed by the format.c API, it is passed to its corresponding format attribute interface where the registered interface's set function handles it.  By having the set_attributes argument we can determine whether or not the attribute interface needs to be looked up at all.  Otherwise, the format interface container must be accessed for every ast_format_set regardless if attributes are required or not.
>     
>     I suppose it is possible to take a look at the va_list and then create a new va_list to send to the attribute interface if AST_FORMAT_ATTR_END is not the first argument.  My thought process while writing this was to try and make this function as fast as possible since it is used on every media frame.
>
> 
> Russell Bryant wrote:
>     I wonder if it's actually faster, though.  There is a cost to passing an argument, too.
>     
>     Here's an alternative that I think resolves both of our concerns:
>     
>     struct ast_format *ast_format_set(struct ast_format *format, enum ast_format_id id, int first_attr, ...);
>     
>     Instead of a "set_attributes" flag, you could have a "first attribute" field.  In the case of no attributes, it's just AST_FORMAT_ATTR_END, and you can check that directly without messing with the va_*() functions.  If there are attributes, that's just where the first attribute identifier goes.
>

I'll do this.


- David


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


On 2011-01-24 15:25:45, David Vossel wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1083/
> -----------------------------------------------------------
> 
> (Updated 2011-01-24 15:25:45)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This patch is the foundation of an entire new way of looking at media in Asterisk.  The code present in this review is everything required to complete phase1 of my Media Architecture proposal.
> 
> For more information about this project visit the link below.
> https://wiki.asterisk.org/wiki/display/AST/Media+Architecture+Proposal
> 
> 
> Diffs
> -----
> 
>   /trunk/addons/chan_mobile.c 303557 
>   /trunk/addons/chan_ooh323.h 303557 
>   /trunk/addons/chan_ooh323.c 303557 
>   /trunk/addons/format_mp3.c 303557 
>   /trunk/addons/ooh323cDriver.h 303557 
>   /trunk/addons/ooh323cDriver.c 303557 
>   /trunk/apps/app_alarmreceiver.c 303557 
>   /trunk/apps/app_amd.c 303557 
>   /trunk/apps/app_chanspy.c 303557 
>   /trunk/apps/app_confbridge.c 303557 
>   /trunk/apps/app_dahdibarge.c 303557 
>   /trunk/apps/app_dictate.c 303557 
>   /trunk/apps/app_dumpchan.c 303557 
>   /trunk/apps/app_echo.c 303557 
>   /trunk/apps/app_fax.c 303557 
>   /trunk/apps/app_festival.c 303557 
>   /trunk/apps/app_followme.c 303557 
>   /trunk/apps/app_ices.c 303557 
>   /trunk/apps/app_jack.c 303557 
>   /trunk/apps/app_meetme.c 303557 
>   /trunk/apps/app_milliwatt.c 303557 
>   /trunk/apps/app_mixmonitor.c 303557 
>   /trunk/apps/app_mp3.c 303557 
>   /trunk/apps/app_nbscat.c 303557 
>   /trunk/apps/app_originate.c 303557 
>   /trunk/apps/app_parkandannounce.c 303557 
>   /trunk/apps/app_record.c 303557 
>   /trunk/apps/app_rpt.c 303557 
>   /trunk/apps/app_sms.c 303557 
>   /trunk/apps/app_speech_utils.c 303557 
>   /trunk/apps/app_talkdetect.c 303557 
>   /trunk/apps/app_test.c 303557 
>   /trunk/apps/app_voicemail.c 303557 
>   /trunk/apps/app_waitforsilence.c 303557 
>   /trunk/bridges/bridge_multiplexed.c 303557 
>   /trunk/bridges/bridge_simple.c 303557 
>   /trunk/bridges/bridge_softmix.c 303557 
>   /trunk/channels/chan_agent.c 303557 
>   /trunk/channels/chan_alsa.c 303557 
>   /trunk/channels/chan_bridge.c 303557 
>   /trunk/channels/chan_console.c 303557 
>   /trunk/channels/chan_dahdi.c 303557 
>   /trunk/channels/chan_gtalk.c 303557 
>   /trunk/channels/chan_h323.c 303557 
>   /trunk/channels/chan_iax2.c 303557 
>   /trunk/channels/chan_jingle.c 303557 
>   /trunk/channels/chan_local.c 303557 
>   /trunk/channels/chan_mgcp.c 303557 
>   /trunk/channels/chan_misdn.c 303557 
>   /trunk/channels/chan_multicast_rtp.c 303557 
>   /trunk/channels/chan_nbs.c 303557 
>   /trunk/channels/chan_oss.c 303557 
>   /trunk/channels/chan_phone.c 303557 
>   /trunk/channels/chan_sip.c 303557 
>   /trunk/channels/chan_skinny.c 303557 
>   /trunk/channels/chan_unistim.c 303557 
>   /trunk/channels/chan_usbradio.c 303557 
>   /trunk/channels/chan_vpb.cc 303557 
>   /trunk/channels/h323/ast_h323.cxx 303557 
>   /trunk/channels/h323/chan_h323.h 303557 
>   /trunk/channels/iax2-parser.h 303557 
>   /trunk/channels/iax2-parser.c 303557 
>   /trunk/channels/iax2-provision.c 303557 
>   /trunk/channels/iax2.h 303557 
>   /trunk/channels/sip/include/globals.h 303557 
>   /trunk/channels/sip/include/sip.h 303557 
>   /trunk/codecs/codec_a_mu.c 303557 
>   /trunk/codecs/codec_adpcm.c 303557 
>   /trunk/codecs/codec_alaw.c 303557 
>   /trunk/codecs/codec_dahdi.c 303557 
>   /trunk/codecs/codec_g722.c 303557 
>   /trunk/codecs/codec_g726.c 303557 
>   /trunk/codecs/codec_gsm.c 303557 
>   /trunk/codecs/codec_ilbc.c 303557 
>   /trunk/codecs/codec_lpc10.c 303557 
>   /trunk/codecs/codec_resample.c 303557 
>   /trunk/codecs/codec_speex.c 303557 
>   /trunk/codecs/codec_ulaw.c 303557 
>   /trunk/codecs/ex_adpcm.h 303557 
>   /trunk/codecs/ex_alaw.h 303557 
>   /trunk/codecs/ex_g722.h 303557 
>   /trunk/codecs/ex_g726.h 303557 
>   /trunk/codecs/ex_gsm.h 303557 
>   /trunk/codecs/ex_lpc10.h 303557 
>   /trunk/codecs/ex_speex.h 303557 
>   /trunk/codecs/ex_ulaw.h 303557 
>   /trunk/formats/format_g719.c 303557 
>   /trunk/formats/format_g723.c 303557 
>   /trunk/formats/format_g726.c 303557 
>   /trunk/formats/format_g729.c 303557 
>   /trunk/formats/format_gsm.c 303557 
>   /trunk/formats/format_h263.c 303557 
>   /trunk/formats/format_h264.c 303557 
>   /trunk/formats/format_ilbc.c 303557 
>   /trunk/formats/format_jpeg.c 303557 
>   /trunk/formats/format_ogg_vorbis.c 303557 
>   /trunk/formats/format_pcm.c 303557 
>   /trunk/formats/format_siren14.c 303557 
>   /trunk/formats/format_siren7.c 303557 
>   /trunk/formats/format_sln.c 303557 
>   /trunk/formats/format_sln16.c 303557 
>   /trunk/formats/format_vox.c 303557 
>   /trunk/formats/format_wav.c 303557 
>   /trunk/formats/format_wav_gsm.c 303557 
>   /trunk/funcs/func_channel.c 303557 
>   /trunk/funcs/func_frame_trace.c 303557 
>   /trunk/funcs/func_pitchshift.c 303557 
>   /trunk/include/asterisk/abstract_jb.h 303557 
>   /trunk/include/asterisk/astobj2.h 303557 
>   /trunk/include/asterisk/audiohook.h 303557 
>   /trunk/include/asterisk/bridging.h 303557 
>   /trunk/include/asterisk/bridging_technology.h 303557 
>   /trunk/include/asterisk/callerid.h 303557 
>   /trunk/include/asterisk/channel.h 303557 
>   /trunk/include/asterisk/data.h 303557 
>   /trunk/include/asterisk/file.h 303557 
>   /trunk/include/asterisk/format.h PRE-CREATION 
>   /trunk/include/asterisk/format_cap.h PRE-CREATION 
>   /trunk/include/asterisk/format_pref.h PRE-CREATION 
>   /trunk/include/asterisk/frame.h 303557 
>   /trunk/include/asterisk/frame_defs.h 303557 
>   /trunk/include/asterisk/image.h 303557 
>   /trunk/include/asterisk/mod_format.h 303557 
>   /trunk/include/asterisk/pbx.h 303557 
>   /trunk/include/asterisk/rtp_engine.h 303557 
>   /trunk/include/asterisk/slin.h 303557 
>   /trunk/include/asterisk/slinfactory.h 303557 
>   /trunk/include/asterisk/speech.h 303557 
>   /trunk/include/asterisk/translate.h 303557 
>   /trunk/main/abstract_jb.c 303557 
>   /trunk/main/app.c 303557 
>   /trunk/main/asterisk.c 303557 
>   /trunk/main/astobj2.c 303557 
>   /trunk/main/audiohook.c 303557 
>   /trunk/main/bridging.c 303557 
>   /trunk/main/callerid.c 303557 
>   /trunk/main/ccss.c 303557 
>   /trunk/main/channel.c 303557 
>   /trunk/main/cli.c 303557 
>   /trunk/main/data.c 303557 
>   /trunk/main/dial.c 303557 
>   /trunk/main/dsp.c 303557 
>   /trunk/main/features.c 303557 
>   /trunk/main/file.c 303557 
>   /trunk/main/format.c PRE-CREATION 
>   /trunk/main/format_cap.c PRE-CREATION 
>   /trunk/main/format_pref.c PRE-CREATION 
>   /trunk/main/frame.c 303557 
>   /trunk/main/image.c 303557 
>   /trunk/main/indications.c 303557 
>   /trunk/main/manager.c 303557 
>   /trunk/main/pbx.c 303557 
>   /trunk/main/rtp_engine.c 303557 
>   /trunk/main/slinfactory.c 303557 
>   /trunk/main/translate.c 303557 
>   /trunk/main/udptl.c 303557 
>   /trunk/pbx/pbx_spool.c 303557 
>   /trunk/res/res_adsi.c 303557 
>   /trunk/res/res_agi.c 303557 
>   /trunk/res/res_calendar.c 303557 
>   /trunk/res/res_clioriginate.c 303557 
>   /trunk/res/res_fax.c 303557 
>   /trunk/res/res_fax_spandsp.c 303557 
>   /trunk/res/res_musiconhold.c 303557 
>   /trunk/res/res_rtp_asterisk.c 303557 
>   /trunk/res/res_rtp_multicast.c 303557 
>   /trunk/res/res_speech.c 303557 
>   /trunk/tests/test_format_api.c PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/1083/diff
> 
> 
> Testing
> -------
> 
> Below are the major areas I tested during development.  I will continue testing as this patch is being reviewed.
> -Local Channel + IAX2 channel load testing
> -SIP Calls with and without video
> -IAX2 Calls
> -AudioHooks and apps using audiohooks
> -Masquerades
> -DTMF Attended Transfers
> -SIP Transfers
> -Gtalk
> 
> 
> Thanks,
> 
> David
> 
>

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

More information about the asterisk-dev mailing list