<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/1083/">https://reviewboard.asterisk.org/r/1083/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On January 25th, 2011, 12:31 p.m., <b>Russell Bryant</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://reviewboard.asterisk.org/r/1083/diff/3/?file=15424#file15424line133" style="color: black; font-weight: bold; text-decoration: underline;">/main/format.c</a>
<span style="font-weight: normal;">
(Diff revision 3)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">static int format_set_helper(struct ast_format *format, va_list ap)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">133</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">struct ast_format *ast_format_set(struct ast_format *format, enum ast_format_id id, int set_attributes, ... )</pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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);</pre>
</blockquote>
<p>On January 25th, 2011, 12:45 p.m., <b>David Vossel</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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?</pre>
</blockquote>
<p>On January 25th, 2011, 3:21 p.m., <b>Russell Bryant</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
<p>On January 25th, 2011, 5:06 p.m., <b>David Vossel</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.
</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.
</pre>
<br />
<p>- Russell</p>
<br />
<p>On January 24th, 2011, 3:25 p.m., David Vossel wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.orgrb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers.</div>
<div>By David Vossel.</div>
<p style="color: grey;"><i>Updated 2011-01-24 15:25:45</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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
</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/trunk/addons/chan_mobile.c <span style="color: grey">(303557)</span></li>
<li>/trunk/addons/chan_ooh323.h <span style="color: grey">(303557)</span></li>
<li>/trunk/addons/chan_ooh323.c <span style="color: grey">(303557)</span></li>
<li>/trunk/addons/format_mp3.c <span style="color: grey">(303557)</span></li>
<li>/trunk/addons/ooh323cDriver.h <span style="color: grey">(303557)</span></li>
<li>/trunk/addons/ooh323cDriver.c <span style="color: grey">(303557)</span></li>
<li>/trunk/apps/app_alarmreceiver.c <span style="color: grey">(303557)</span></li>
<li>/trunk/apps/app_amd.c <span style="color: grey">(303557)</span></li>
<li>/trunk/apps/app_chanspy.c <span style="color: grey">(303557)</span></li>
<li>/trunk/apps/app_confbridge.c <span style="color: grey">(303557)</span></li>
<li>/trunk/apps/app_dahdibarge.c <span style="color: grey">(303557)</span></li>
<li>/trunk/apps/app_dictate.c <span style="color: grey">(303557)</span></li>
<li>/trunk/apps/app_dumpchan.c <span style="color: grey">(303557)</span></li>
<li>/trunk/apps/app_echo.c <span style="color: grey">(303557)</span></li>
<li>/trunk/apps/app_fax.c <span style="color: grey">(303557)</span></li>
<li>/trunk/apps/app_festival.c <span style="color: grey">(303557)</span></li>
<li>/trunk/apps/app_followme.c <span style="color: grey">(303557)</span></li>
<li>/trunk/apps/app_ices.c <span style="color: grey">(303557)</span></li>
<li>/trunk/apps/app_jack.c <span style="color: grey">(303557)</span></li>
<li>/trunk/apps/app_meetme.c <span style="color: grey">(303557)</span></li>
<li>/trunk/apps/app_milliwatt.c <span style="color: grey">(303557)</span></li>
<li>/trunk/apps/app_mixmonitor.c <span style="color: grey">(303557)</span></li>
<li>/trunk/apps/app_mp3.c <span style="color: grey">(303557)</span></li>
<li>/trunk/apps/app_nbscat.c <span style="color: grey">(303557)</span></li>
<li>/trunk/apps/app_originate.c <span style="color: grey">(303557)</span></li>
<li>/trunk/apps/app_parkandannounce.c <span style="color: grey">(303557)</span></li>
<li>/trunk/apps/app_record.c <span style="color: grey">(303557)</span></li>
<li>/trunk/apps/app_rpt.c <span style="color: grey">(303557)</span></li>
<li>/trunk/apps/app_sms.c <span style="color: grey">(303557)</span></li>
<li>/trunk/apps/app_speech_utils.c <span style="color: grey">(303557)</span></li>
<li>/trunk/apps/app_talkdetect.c <span style="color: grey">(303557)</span></li>
<li>/trunk/apps/app_test.c <span style="color: grey">(303557)</span></li>
<li>/trunk/apps/app_voicemail.c <span style="color: grey">(303557)</span></li>
<li>/trunk/apps/app_waitforsilence.c <span style="color: grey">(303557)</span></li>
<li>/trunk/bridges/bridge_multiplexed.c <span style="color: grey">(303557)</span></li>
<li>/trunk/bridges/bridge_simple.c <span style="color: grey">(303557)</span></li>
<li>/trunk/bridges/bridge_softmix.c <span style="color: grey">(303557)</span></li>
<li>/trunk/channels/chan_agent.c <span style="color: grey">(303557)</span></li>
<li>/trunk/channels/chan_alsa.c <span style="color: grey">(303557)</span></li>
<li>/trunk/channels/chan_bridge.c <span style="color: grey">(303557)</span></li>
<li>/trunk/channels/chan_console.c <span style="color: grey">(303557)</span></li>
<li>/trunk/channels/chan_dahdi.c <span style="color: grey">(303557)</span></li>
<li>/trunk/channels/chan_gtalk.c <span style="color: grey">(303557)</span></li>
<li>/trunk/channels/chan_h323.c <span style="color: grey">(303557)</span></li>
<li>/trunk/channels/chan_iax2.c <span style="color: grey">(303557)</span></li>
<li>/trunk/channels/chan_jingle.c <span style="color: grey">(303557)</span></li>
<li>/trunk/channels/chan_local.c <span style="color: grey">(303557)</span></li>
<li>/trunk/channels/chan_mgcp.c <span style="color: grey">(303557)</span></li>
<li>/trunk/channels/chan_misdn.c <span style="color: grey">(303557)</span></li>
<li>/trunk/channels/chan_multicast_rtp.c <span style="color: grey">(303557)</span></li>
<li>/trunk/channels/chan_nbs.c <span style="color: grey">(303557)</span></li>
<li>/trunk/channels/chan_oss.c <span style="color: grey">(303557)</span></li>
<li>/trunk/channels/chan_phone.c <span style="color: grey">(303557)</span></li>
<li>/trunk/channels/chan_sip.c <span style="color: grey">(303557)</span></li>
<li>/trunk/channels/chan_skinny.c <span style="color: grey">(303557)</span></li>
<li>/trunk/channels/chan_unistim.c <span style="color: grey">(303557)</span></li>
<li>/trunk/channels/chan_usbradio.c <span style="color: grey">(303557)</span></li>
<li>/trunk/channels/chan_vpb.cc <span style="color: grey">(303557)</span></li>
<li>/trunk/channels/h323/ast_h323.cxx <span style="color: grey">(303557)</span></li>
<li>/trunk/channels/h323/chan_h323.h <span style="color: grey">(303557)</span></li>
<li>/trunk/channels/iax2-parser.h <span style="color: grey">(303557)</span></li>
<li>/trunk/channels/iax2-parser.c <span style="color: grey">(303557)</span></li>
<li>/trunk/channels/iax2-provision.c <span style="color: grey">(303557)</span></li>
<li>/trunk/channels/iax2.h <span style="color: grey">(303557)</span></li>
<li>/trunk/channels/sip/include/globals.h <span style="color: grey">(303557)</span></li>
<li>/trunk/channels/sip/include/sip.h <span style="color: grey">(303557)</span></li>
<li>/trunk/codecs/codec_a_mu.c <span style="color: grey">(303557)</span></li>
<li>/trunk/codecs/codec_adpcm.c <span style="color: grey">(303557)</span></li>
<li>/trunk/codecs/codec_alaw.c <span style="color: grey">(303557)</span></li>
<li>/trunk/codecs/codec_dahdi.c <span style="color: grey">(303557)</span></li>
<li>/trunk/codecs/codec_g722.c <span style="color: grey">(303557)</span></li>
<li>/trunk/codecs/codec_g726.c <span style="color: grey">(303557)</span></li>
<li>/trunk/codecs/codec_gsm.c <span style="color: grey">(303557)</span></li>
<li>/trunk/codecs/codec_ilbc.c <span style="color: grey">(303557)</span></li>
<li>/trunk/codecs/codec_lpc10.c <span style="color: grey">(303557)</span></li>
<li>/trunk/codecs/codec_resample.c <span style="color: grey">(303557)</span></li>
<li>/trunk/codecs/codec_speex.c <span style="color: grey">(303557)</span></li>
<li>/trunk/codecs/codec_ulaw.c <span style="color: grey">(303557)</span></li>
<li>/trunk/codecs/ex_adpcm.h <span style="color: grey">(303557)</span></li>
<li>/trunk/codecs/ex_alaw.h <span style="color: grey">(303557)</span></li>
<li>/trunk/codecs/ex_g722.h <span style="color: grey">(303557)</span></li>
<li>/trunk/codecs/ex_g726.h <span style="color: grey">(303557)</span></li>
<li>/trunk/codecs/ex_gsm.h <span style="color: grey">(303557)</span></li>
<li>/trunk/codecs/ex_lpc10.h <span style="color: grey">(303557)</span></li>
<li>/trunk/codecs/ex_speex.h <span style="color: grey">(303557)</span></li>
<li>/trunk/codecs/ex_ulaw.h <span style="color: grey">(303557)</span></li>
<li>/trunk/formats/format_g719.c <span style="color: grey">(303557)</span></li>
<li>/trunk/formats/format_g723.c <span style="color: grey">(303557)</span></li>
<li>/trunk/formats/format_g726.c <span style="color: grey">(303557)</span></li>
<li>/trunk/formats/format_g729.c <span style="color: grey">(303557)</span></li>
<li>/trunk/formats/format_gsm.c <span style="color: grey">(303557)</span></li>
<li>/trunk/formats/format_h263.c <span style="color: grey">(303557)</span></li>
<li>/trunk/formats/format_h264.c <span style="color: grey">(303557)</span></li>
<li>/trunk/formats/format_ilbc.c <span style="color: grey">(303557)</span></li>
<li>/trunk/formats/format_jpeg.c <span style="color: grey">(303557)</span></li>
<li>/trunk/formats/format_ogg_vorbis.c <span style="color: grey">(303557)</span></li>
<li>/trunk/formats/format_pcm.c <span style="color: grey">(303557)</span></li>
<li>/trunk/formats/format_siren14.c <span style="color: grey">(303557)</span></li>
<li>/trunk/formats/format_siren7.c <span style="color: grey">(303557)</span></li>
<li>/trunk/formats/format_sln.c <span style="color: grey">(303557)</span></li>
<li>/trunk/formats/format_sln16.c <span style="color: grey">(303557)</span></li>
<li>/trunk/formats/format_vox.c <span style="color: grey">(303557)</span></li>
<li>/trunk/formats/format_wav.c <span style="color: grey">(303557)</span></li>
<li>/trunk/formats/format_wav_gsm.c <span style="color: grey">(303557)</span></li>
<li>/trunk/funcs/func_channel.c <span style="color: grey">(303557)</span></li>
<li>/trunk/funcs/func_frame_trace.c <span style="color: grey">(303557)</span></li>
<li>/trunk/funcs/func_pitchshift.c <span style="color: grey">(303557)</span></li>
<li>/trunk/include/asterisk/abstract_jb.h <span style="color: grey">(303557)</span></li>
<li>/trunk/include/asterisk/astobj2.h <span style="color: grey">(303557)</span></li>
<li>/trunk/include/asterisk/audiohook.h <span style="color: grey">(303557)</span></li>
<li>/trunk/include/asterisk/bridging.h <span style="color: grey">(303557)</span></li>
<li>/trunk/include/asterisk/bridging_technology.h <span style="color: grey">(303557)</span></li>
<li>/trunk/include/asterisk/callerid.h <span style="color: grey">(303557)</span></li>
<li>/trunk/include/asterisk/channel.h <span style="color: grey">(303557)</span></li>
<li>/trunk/include/asterisk/data.h <span style="color: grey">(303557)</span></li>
<li>/trunk/include/asterisk/file.h <span style="color: grey">(303557)</span></li>
<li>/trunk/include/asterisk/format.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>/trunk/include/asterisk/format_cap.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>/trunk/include/asterisk/format_pref.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>/trunk/include/asterisk/frame.h <span style="color: grey">(303557)</span></li>
<li>/trunk/include/asterisk/frame_defs.h <span style="color: grey">(303557)</span></li>
<li>/trunk/include/asterisk/image.h <span style="color: grey">(303557)</span></li>
<li>/trunk/include/asterisk/mod_format.h <span style="color: grey">(303557)</span></li>
<li>/trunk/include/asterisk/pbx.h <span style="color: grey">(303557)</span></li>
<li>/trunk/include/asterisk/rtp_engine.h <span style="color: grey">(303557)</span></li>
<li>/trunk/include/asterisk/slin.h <span style="color: grey">(303557)</span></li>
<li>/trunk/include/asterisk/slinfactory.h <span style="color: grey">(303557)</span></li>
<li>/trunk/include/asterisk/speech.h <span style="color: grey">(303557)</span></li>
<li>/trunk/include/asterisk/translate.h <span style="color: grey">(303557)</span></li>
<li>/trunk/main/abstract_jb.c <span style="color: grey">(303557)</span></li>
<li>/trunk/main/app.c <span style="color: grey">(303557)</span></li>
<li>/trunk/main/asterisk.c <span style="color: grey">(303557)</span></li>
<li>/trunk/main/astobj2.c <span style="color: grey">(303557)</span></li>
<li>/trunk/main/audiohook.c <span style="color: grey">(303557)</span></li>
<li>/trunk/main/bridging.c <span style="color: grey">(303557)</span></li>
<li>/trunk/main/callerid.c <span style="color: grey">(303557)</span></li>
<li>/trunk/main/ccss.c <span style="color: grey">(303557)</span></li>
<li>/trunk/main/channel.c <span style="color: grey">(303557)</span></li>
<li>/trunk/main/cli.c <span style="color: grey">(303557)</span></li>
<li>/trunk/main/data.c <span style="color: grey">(303557)</span></li>
<li>/trunk/main/dial.c <span style="color: grey">(303557)</span></li>
<li>/trunk/main/dsp.c <span style="color: grey">(303557)</span></li>
<li>/trunk/main/features.c <span style="color: grey">(303557)</span></li>
<li>/trunk/main/file.c <span style="color: grey">(303557)</span></li>
<li>/trunk/main/format.c <span style="color: grey">(PRE-CREATION)</span></li>
<li>/trunk/main/format_cap.c <span style="color: grey">(PRE-CREATION)</span></li>
<li>/trunk/main/format_pref.c <span style="color: grey">(PRE-CREATION)</span></li>
<li>/trunk/main/frame.c <span style="color: grey">(303557)</span></li>
<li>/trunk/main/image.c <span style="color: grey">(303557)</span></li>
<li>/trunk/main/indications.c <span style="color: grey">(303557)</span></li>
<li>/trunk/main/manager.c <span style="color: grey">(303557)</span></li>
<li>/trunk/main/pbx.c <span style="color: grey">(303557)</span></li>
<li>/trunk/main/rtp_engine.c <span style="color: grey">(303557)</span></li>
<li>/trunk/main/slinfactory.c <span style="color: grey">(303557)</span></li>
<li>/trunk/main/translate.c <span style="color: grey">(303557)</span></li>
<li>/trunk/main/udptl.c <span style="color: grey">(303557)</span></li>
<li>/trunk/pbx/pbx_spool.c <span style="color: grey">(303557)</span></li>
<li>/trunk/res/res_adsi.c <span style="color: grey">(303557)</span></li>
<li>/trunk/res/res_agi.c <span style="color: grey">(303557)</span></li>
<li>/trunk/res/res_calendar.c <span style="color: grey">(303557)</span></li>
<li>/trunk/res/res_clioriginate.c <span style="color: grey">(303557)</span></li>
<li>/trunk/res/res_fax.c <span style="color: grey">(303557)</span></li>
<li>/trunk/res/res_fax_spandsp.c <span style="color: grey">(303557)</span></li>
<li>/trunk/res/res_musiconhold.c <span style="color: grey">(303557)</span></li>
<li>/trunk/res/res_rtp_asterisk.c <span style="color: grey">(303557)</span></li>
<li>/trunk/res/res_rtp_multicast.c <span style="color: grey">(303557)</span></li>
<li>/trunk/res/res_speech.c <span style="color: grey">(303557)</span></li>
<li>/trunk/tests/test_format_api.c <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/1083/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>