[asterisk-dev] [Code Review] Asterisk media architecture conversion - no more format bitfields
Russell Bryant
reviewboard at asterisk.org
Tue Jan 25 17:17:17 CST 2011
> 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.
>
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.
- Russell
-----------------------------------------------------------
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/20110125/8deeba55/attachment-0001.htm>
More information about the asterisk-dev
mailing list