<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/3671/">https://reviewboard.asterisk.org/r/3671/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On June 25th, 2014, 6:45 a.m. CDT, <b>Joshua Colp</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/3671/diff/1/?file=60620#file60620line200" style="color: black; font-weight: bold; text-decoration: underline;">/team/group/media_formats-reviewed-trunk/main/format.c</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">struct ast_format *ast_format_create(struct ast_codec *codec)</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">200</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="k">if</span> <span class="p">(</span><span class="n">format1</span> <span class="o">==</span> <span class="nb">NULL</span> <span class="o">||</span> <span class="n">format2</span> <span class="o">==</span> <span class="nb">NULL</span><span class="p">)</span> <span class="p">{</span></pre></td>
</tr>
<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">201</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="k">return</span> <span class="n">AST_FORMAT_CMP_NOT_EQUAL</span><span class="p">;</span></pre></td>
</tr>
<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">202</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="p">}</span></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;">Just curious - does this happen?</pre>
</blockquote>
<p>On June 25th, 2014, 7:14 a.m. CDT, <b>Corey Farrell</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;">This could happen if someone configures a blank format_cap (disallow=all) - the first format would be NULL (ast_best_codec).
Maybe an assert should go here until we get a better handle on things? If NULL's to this procedure are possible we probably want to check this second (if both are NULL then they are equal).</pre>
</blockquote>
<p>On June 25th, 2014, 7:26 a.m. CDT, <b>Joshua Colp</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;">My question becomes: Should this accept NULLs or should the caller see that something they expected to be non-NULL is NULL and thus should print out an error/warning/something.</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;">The reason why it is useful to accept NULLs here is explicitly because of res_rtp_asterisk and its silliness. res_rtp_asterisk has to deal both with 'rtp-isms' - things like Cisco specific DTMF - as well Asterisk formats. It stores both of these in the same static array. As such, there are times when it has a format pointer that can be NULL (because the 'format' is Cisco DTMF or something equally silly) but it is running through checks to see if the format is T.140, or G722, or other such things. Having this function gracefully handle NULLs keeps some code in res_rtp_asterisk slightly more sane.
Since this is a heavily used function, it's probably good for it to be as 'safe' as possible. The NULL check here is unlikely to be a performance bottleneck.</pre>
<br />
<p>- Matt</p>
<br />
<p>On June 25th, 2014, 7:14 a.m. CDT, Corey Farrell wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers, Joshua Colp and Matt Jordan.</div>
<div>By Corey Farrell.</div>
<p style="color: grey;"><i>Updated June 25, 2014, 7:14 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
Asterisk
</div>
<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 update gives media_formats the ability to receive a call using chan_sip. Possibly other channel drivers might work, I haven't tried them.
* ast_format_cap_is_compatible_format needs to be checked against AST_FORMAT_CMP_NOT_EQUAL, not zero/non-zero. All calls to ast_format_cap_is_compatible_format were fixed.
* res_rtp_asterisk was updated by Matt Jordan, along with related changes to codec.c, codec.h, format.c, format.c and codec_builtin.c.
* Switch ast_format_copy from function to macro to ao2_bump. This allows REF_DEBUG to give better results.</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;">Called from Asterisk 11 to a test server with this code, I was able to hear the 'invalid' message, everything seemed during the call. I received TONS of ao2 frack's when stopping Asterisk. The sip.conf peer on both Asterisk servers was setup for disallow=all / allow=ulaw.</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>/team/group/media_formats-reviewed-trunk/res/res_speech.c <span style="color: grey">(417190)</span></li>
<li>/team/group/media_formats-reviewed-trunk/res/res_rtp_asterisk.c <span style="color: grey">(417190)</span></li>
<li>/team/group/media_formats-reviewed-trunk/main/translate.c <span style="color: grey">(417190)</span></li>
<li>/team/group/media_formats-reviewed-trunk/main/frame.c <span style="color: grey">(417190)</span></li>
<li>/team/group/media_formats-reviewed-trunk/main/format.c <span style="color: grey">(417190)</span></li>
<li>/team/group/media_formats-reviewed-trunk/main/codec_builtin.c <span style="color: grey">(417190)</span></li>
<li>/team/group/media_formats-reviewed-trunk/main/codec.c <span style="color: grey">(417190)</span></li>
<li>/team/group/media_formats-reviewed-trunk/main/channel.c <span style="color: grey">(417190)</span></li>
<li>/team/group/media_formats-reviewed-trunk/main/bridge.c <span style="color: grey">(417190)</span></li>
<li>/team/group/media_formats-reviewed-trunk/include/asterisk/format.h <span style="color: grey">(417190)</span></li>
<li>/team/group/media_formats-reviewed-trunk/include/asterisk/codec.h <span style="color: grey">(417190)</span></li>
<li>/team/group/media_formats-reviewed-trunk/channels/chan_unistim.c <span style="color: grey">(417190)</span></li>
<li>/team/group/media_formats-reviewed-trunk/channels/chan_skinny.c <span style="color: grey">(417190)</span></li>
<li>/team/group/media_formats-reviewed-trunk/channels/chan_sip.c <span style="color: grey">(417190)</span></li>
<li>/team/group/media_formats-reviewed-trunk/channels/chan_pjsip.c <span style="color: grey">(417190)</span></li>
<li>/team/group/media_formats-reviewed-trunk/channels/chan_oss.c <span style="color: grey">(417190)</span></li>
<li>/team/group/media_formats-reviewed-trunk/channels/chan_nbs.c <span style="color: grey">(417190)</span></li>
<li>/team/group/media_formats-reviewed-trunk/channels/chan_motif.c <span style="color: grey">(417190)</span></li>
<li>/team/group/media_formats-reviewed-trunk/channels/chan_mgcp.c <span style="color: grey">(417190)</span></li>
<li>/team/group/media_formats-reviewed-trunk/channels/chan_jingle.c <span style="color: grey">(417190)</span></li>
<li>/team/group/media_formats-reviewed-trunk/channels/chan_h323.c <span style="color: grey">(417190)</span></li>
<li>/team/group/media_formats-reviewed-trunk/channels/chan_gtalk.c <span style="color: grey">(417190)</span></li>
<li>/team/group/media_formats-reviewed-trunk/channels/chan_alsa.c <span style="color: grey">(417190)</span></li>
<li>/team/group/media_formats-reviewed-trunk/addons/chan_ooh323.c <span style="color: grey">(417190)</span></li>
<li>/team/group/media_formats-reviewed-trunk/addons/chan_mobile.c <span style="color: grey">(417190)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/3671/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>