<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/3703/">https://reviewboard.asterisk.org/r/3703/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 2nd, 2014, 3:25 p.m. CDT, <b>Corey Farrell</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/3703/diff/2/?file=62050#file62050line48" style="color: black; font-weight: bold; text-decoration: underline;">/team/res/res_format_attr_celt.c</a>
<span style="font-weight: normal;">
(Diff revision 2)
</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; ">ASTERISK_FILE_VERSION(__FILE__, "$Revision$")</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">48</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="k">static</span> <span class="kt"><span class="hl">int</span></span> <span class="nf">celt_<span class="hl">sdp_parse</span></span><span class="p">(</span><span class="k">struct</span> <span class="n">ast_format<span class="hl">_attr</span></span> <span class="o">*</span><span class="n">format<span class="hl">_attr</span></span><span class="p"><span class="hl">,</span></span><span class="hl"> </span><span class="k"><span class="hl">const</span></span><span class="hl"> </span><span class="kt"><span class="hl">char</span></span><span class="hl"> </span><span class="o"><span class="hl">*</span></span><span class="n"><span class="hl">attributes</span></span><span class="p">)</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">48</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="k">static</span> <span class="kt"><span class="hl">void</span></span> <span class="nf">celt_<span class="hl">destroy</span></span><span class="p">(</span><span class="k">struct</span> <span class="n">ast_format</span> <span class="o">*</span><span class="n">format</span><span class="p">)</span></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">49</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="p">{</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">49</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="p">{</span></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">50</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="k">struct</span> <span class="n">celt_attr</span> <span class="o">*</span><span class="n">attr</span> <span class="o">=</span> <span class="p"><span class="hl">(</span></span><span class="k"><span class="hl">struct</span></span><span class="hl"> </span><span class="n"><span class="hl">celt_attr</span></span><span class="hl"> </span><span class="o"><span class="hl">*</span></span><span class="p"><span class="hl">)</span></span><span class="hl"> </span><span class="n">format_attr</span><span class="p">;</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">50</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="k">struct</span> <span class="n">celt_attr</span> <span class="o">*</span><span class="n">attr</span> <span class="o">=</span> <span class="n"><span class="hl">ast_</span>format_<span class="hl">get_</span>attr<span class="hl">ibute_data</span></span><span class="p"><span class="hl">(</span></span><span class="n"><span class="hl">format</span></span><span class="p"><span class="hl">)</span>;</span></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">51</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></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">52</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="n">ast_free</span><span class="p">(</span><span class="n">attr</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">53</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><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;">All format_destroy callbacks are identical. I think it would be better to make format.c responsible for freeing attribute data after calling the (optional) format_destroy callback. This callback would exist in-case the attributes contain a pointer that must be free'd or reference released. Maybe also rename the callback to format_cleanup?</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'm not sure I agree with this finding. Right now, the format attribute modules are completely responsible for managing attributes. That has the benefit of putting all the logic in a single module. Granted, right now, attributes are relatively simple structures, but there's no guarantee that will always be the case - and this design is more flexible for the future.</pre>
<br />
<p>- Matt</p>
<br />
<p>On July 3rd, 2014, 7:15 a.m. CDT, Joshua Colp 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.</div>
<div>By Joshua Colp.</div>
<p style="color: grey;"><i>Updated July 3, 2014, 7:15 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-23957">ASTERISK-23957</a>
</div>
<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 change does a few things:
1. Fixes an issue where direct format interfaces were treated as AO2 objects when they were not.
2. Added an ast_format_clone API call which clones and deep copies a format, returning one which can be safely modified.
3. Changed the format interface API so anything which manipulates the format returns a new format.
4. Added get/set functions for format attribute data.
5. Added an API call to replace the format in an RTP engine codecs structure.
6. Updated the res_format_attr_* modules to work with the new media formats work.
7. Added support for loading an interface module after a format has been created.</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;">Placed calls in and out, confirmed that the attribute stuff doesn't crash things and that received SDP is parsed and interpreted. What doesn't currently work is passing this information through so outgoing calls have the correct attributes.</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_pjsip_sdp_rtp.c <span style="color: grey">(417804)</span></li>
<li>/team/group/media_formats-reviewed-trunk/res/res_format_attr_silk.c <span style="color: grey">(417804)</span></li>
<li>/team/group/media_formats-reviewed-trunk/res/res_format_attr_opus.c <span style="color: grey">(417804)</span></li>
<li>/team/group/media_formats-reviewed-trunk/res/res_format_attr_h264.c <span style="color: grey">(417804)</span></li>
<li>/team/group/media_formats-reviewed-trunk/res/res_format_attr_h263.c <span style="color: grey">(417804)</span></li>
<li>/team/group/media_formats-reviewed-trunk/res/res_format_attr_celt.c <span style="color: grey">(417804)</span></li>
<li>/team/group/media_formats-reviewed-trunk/main/rtp_engine.c <span style="color: grey">(417804)</span></li>
<li>/team/group/media_formats-reviewed-trunk/main/format.c <span style="color: grey">(417804)</span></li>
<li>/team/group/media_formats-reviewed-trunk/include/asterisk/rtp_engine.h <span style="color: grey">(417804)</span></li>
<li>/team/group/media_formats-reviewed-trunk/include/asterisk/format.h <span style="color: grey">(417804)</span></li>
<li>/team/group/media_formats-reviewed-trunk/channels/chan_sip.c <span style="color: grey">(417804)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/3703/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>