<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/3687/">https://reviewboard.asterisk.org/r/3687/</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 1st, 2014, 12:14 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/3687/diff/3/?file=61632#file61632line704" style="color: black; font-weight: bold; text-decoration: underline;">/team/group/media_formats-reviewed-trunk/main/rtp_engine.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="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">void ast_rtp_codecs_payloads_clear(struct ast_rtp_codecs *codecs, struct ast_rtp_instance *instance)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">680</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="n">type</span> <span class="o">=</span> <span class="n">ast_rtp_engine_alloc_payload_type</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">677</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="n">old_type</span> <span class="o">=</span> <span class="n">AST_VECTOR_GET</span><span class="p">(</span><span class="o">&</span><span class="n">codecs</span><span class="o">-></span><span class="n">payloads</span><span class="p">,</span> <span class="n">payload</span><span class="p">);</span></pre></td>
</tr>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">681</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="k">if</span> <span class="p">(</span><span class="o"><span class="hl">!</span></span><span class="n">type</span><span class="p">)</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">678</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="k">if</span> <span class="p">(</span><span class="n"><span class="hl">old_</span>type</span><span class="p">)</span> <span class="p">{</span></pre></td>
</tr>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">682</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="n">ast_rwlock_unlock</span><span class="p">(</span><span class="o">&</span><span class="n">static_RTP_PT_lock</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">679</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="n">ao2_ref</span><span class="p">(</span><span class="n">old_type</span><span class="p">,</span> <span class="o">-</span><span class="mi">1</span><span class="p">);</span></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#ebb1ba" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">683</font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="k">return</span><span class="p">;</span></pre></td>
<th bgcolor="#ebb1ba" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">684</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><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">680</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><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;">We need to remove from the vector. Also why not ao2_cleanup?</pre>
</blockquote>
<p>On July 1st, 2014, 12:29 p.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;">A related thought, maybe we need an AST_VECTOR_EXTRACT(vector, idx) to handle get + remove?</pre>
</blockquote>
<p>On July 1st, 2014, 12:32 p.m. CDT, <b>Matt Jordan</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;">We don't need to remove from the vector. We are explicitly blowing away our reference to the item on the vector, then replacing it with a new item. No removal is necessary, as the item on the vector is just a pointer to the ao2 object.</pre>
</blockquote>
<p>On July 1st, 2014, 12:38 p.m. CDT, <b>Matt Jordan</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;">Keep in mind that while we're doing this, we're holding the wrlock to the codecs. That means no one else can come around and snag the codec payload while we're swapping it out.
Assuming the payload is in slot 3, you have:
write lock
- deref item in slot 3
- build now item (slot 3 may be garbage here)
- put new item into slot 3
unlock
If a consumer of the codecs had a reference to the payload type at slot 3, they're fine. The object will be valid until they release it. If they went to obtain the codecs in slot 3 while this operation occurred, they'd hit the write lock. Once the write lock was released, they'd get the new codec information in slot 3.</pre>
</blockquote>
<p>On July 1st, 2014, 12:43 p.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;">Not removing element at index 'payload' from the vector means it will point to a possibly destroyed ast_rtp_payload_type. Down a few lines in the code you are not "setting" element at index 'payload' to new_type, you are "inserting" new_type. So I think the end result would be:
AST_VECTOR_GET(&codecs->payloads, payload) == new_type
AST_VECTOR_GET(&codecs->payloads, payload + 1) == old_type (pointer to possibly freed object)</pre>
</blockquote>
<p>On July 1st, 2014, 12:45 p.m. CDT, <b>Matt Jordan</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;">No, we are inserting at that particular location. Look at AST_VECTOR_INSERT:
#define AST_VECTOR_INSERT(vec, idx, elem) ({ \
int res = 0; \
do { \
if (((idx) + 1) > (vec)->max) { \
size_t new_max = ((idx) + 1) * 2; \
typeof((vec)->elems) new_elems = ast_calloc(1, \
new_max * sizeof(*new_elems)); \
if (new_elems) { \
memcpy(new_elems, (vec)->elems, \
(vec)->current * sizeof(*new_elems)); \
ast_free((vec)->elems); \
(vec)->elems = new_elems; \
(vec)->max = new_max; \
} else { \
res = -1; \
break; \
} \
} \
(vec)->elems[(idx)] = (elem); \
if (((idx) + 1) > (vec)->current) { \
(vec)->current = (idx) + 1; \
} \
} while(0); \
res; \
})
Specifically this part:
(vec)->elems[(idx)] = (elem); \
if (((idx) + 1) > (vec)->current) { \
(vec)->current = (idx) + 1; \
}
The exact same index as the last element is set to the new element. This insert the new type at exactly the same position in the array as the old element.
Unless I'm missing something...?</pre>
</blockquote>
<p>On July 1st, 2014, 12:51 p.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;">Maybe I should just do a better job reading the comments:
* \warning This macro will overwrite anything already present at the position provided.
Not exactly what I would call AST_VECTOR_INSERT, more like AST_VECTOR_SET with auto-expansion. Oh well you are right sorry!</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;">No worries! I'm glad you did a thorough review of the changes in here, since I got a bit invasive in the rtp_engine.</pre>
<br />
<p>- Matt</p>
<br />
<p>On July 1st, 2014, 12:43 p.m. CDT, Matt Jordan 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 Matt Jordan.</div>
<p style="color: grey;"><i>Updated July 1, 2014, 12:43 p.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 patch started out as an attempt to fix the BUGBUGs left over packetization calls into rtp_engine; it got a little bit bigger. Things now compile and work (see Testing), so this is a good place to stop before the renaming effort.
Primarily, this patch does the following:
(1) Removes ast_rtp_codecs_packetization_set. This call was effectively a NoOp with res_rtp_asterisk/res_rtp_multicast. The various channel drivers now call ast_rtp_codecs_set_framing where appropriate.
(2) A major overhaul of ast_rtp_codec was done. This includes:
(a) Storing the framing on the structure. This allows for the smoother in res_rtp_asterisk to easily get the framing specified without having to do major gyrations.
(b) Payload types (which are ao2 ref counted objects) are no longer stored in an ao2_container. This container had two patterns of usage: lookups by an integer key value and iteration. Vectors work well for this type of access and - for relatively small numbers of items (which is generally the case for payload types), are much faster on both counts.
(3) The 'use_ptime' setting in res_pjsip_sdp_rtp now works. Packetization is also handled a little bit better, as both the RTP engine and format_cap API already do the job of managing the framing.
A variety of ref leaks were cleaned up as well along the way.</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;">Back in February, I wrote a number of single audio stream tests for the PJSIP channel driver. Eventually these will get posted up for review, but the tests cover:
* Basic Offer/Answer of different sets of codecs (using a variety of patterns, including allow=all (ew))
* Packetization, including use_ptime=yes|no.
* AVPF
* Preferred codec only (by only specifying a single supported codec), subsets of offers, etc.
These tests will eventually get put up on another review, but they gave some confidence that the mucking around in the rtp_engine that is done on this patch works.</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/tests/test_format_cap.c <span style="color: grey">(417724)</span></li>
<li>/team/group/media_formats-reviewed-trunk/res/res_speech.c <span style="color: grey">(417724)</span></li>
<li>/team/group/media_formats-reviewed-trunk/res/res_rtp_asterisk.c <span style="color: grey">(417724)</span></li>
<li>/team/group/media_formats-reviewed-trunk/res/res_pjsip_sdp_rtp.c <span style="color: grey">(417724)</span></li>
<li>/team/group/media_formats-reviewed-trunk/res/res_fax.c <span style="color: grey">(417724)</span></li>
<li>/team/group/media_formats-reviewed-trunk/main/rtp_engine.c <span style="color: grey">(417724)</span></li>
<li>/team/group/media_formats-reviewed-trunk/main/format_cap.c <span style="color: grey">(417724)</span></li>
<li>/team/group/media_formats-reviewed-trunk/main/format.c <span style="color: grey">(417724)</span></li>
<li>/team/group/media_formats-reviewed-trunk/include/asterisk/vector.h <span style="color: grey">(417724)</span></li>
<li>/team/group/media_formats-reviewed-trunk/include/asterisk/rtp_engine.h <span style="color: grey">(417724)</span></li>
<li>/team/group/media_formats-reviewed-trunk/include/asterisk/frame.h <span style="color: grey">(417724)</span></li>
<li>/team/group/media_formats-reviewed-trunk/include/asterisk/format_cap.h <span style="color: grey">(417724)</span></li>
<li>/team/group/media_formats-reviewed-trunk/include/asterisk/format.h <span style="color: grey">(417724)</span></li>
<li>/team/group/media_formats-reviewed-trunk/formats/format_h264.c <span style="color: grey">(417724)</span></li>
<li>/team/group/media_formats-reviewed-trunk/formats/format_h263.c <span style="color: grey">(417724)</span></li>
<li>/team/group/media_formats-reviewed-trunk/channels/chan_skinny.c <span style="color: grey">(417724)</span></li>
<li>/team/group/media_formats-reviewed-trunk/channels/chan_sip.c <span style="color: grey">(417724)</span></li>
<li>/team/group/media_formats-reviewed-trunk/channels/chan_motif.c <span style="color: grey">(417724)</span></li>
<li>/team/group/media_formats-reviewed-trunk/channels/chan_jingle.c <span style="color: grey">(417724)</span></li>
<li>/team/group/media_formats-reviewed-trunk/channels/chan_iax2.c <span style="color: grey">(417724)</span></li>
<li>/team/group/media_formats-reviewed-trunk/channels/chan_h323.c <span style="color: grey">(417724)</span></li>
<li>/team/group/media_formats-reviewed-trunk/channels/chan_gtalk.c <span style="color: grey">(417724)</span></li>
<li>/team/group/media_formats-reviewed-trunk/bridges/bridge_softmix.c <span style="color: grey">(417724)</span></li>
<li>/team/group/media_formats-reviewed-trunk/bridges/bridge_native_rtp.c <span style="color: grey">(417724)</span></li>
<li>/team/group/media_formats-reviewed-trunk/addons/ooh323cDriver.c <span style="color: grey">(417724)</span></li>
<li>/team/group/media_formats-reviewed-trunk/addons/chan_ooh323.c <span style="color: grey">(417724)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/3687/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>