<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/2723/">https://reviewboard.asterisk.org/r/2723/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On August 20th, 2013, 11:10 a.m. UTC, <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/2723/diff/4/?file=44220#file44220line12855" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/channels/chan_sip.c</a>
<span style="font-weight: normal;">
(Diff revision 4)
</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; ">static void add_codec_to_sdp(const struct sip_pvt *p,</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">12833</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span>if (fmt.<span class="hl">cur</span>_ms && (fmt.<span class="hl">cur</span>_ms <span class="hl"><</span> *m<span class="hl">in</span>_packet_size))</pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">12855</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span>if (<span class="hl">max_packet_size && </span>fmt.<span class="hl">max</span>_ms && (fmt.<span class="hl">max</span>_ms <span class="hl">></span> *m<span class="hl">ax</span>_packet_size))<span class="hl"> {</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;">As the max packet size is a combination of all formats involved it should be the *lowest* not the highest.</pre>
</blockquote>
<p>On August 21st, 2013, 1:43 p.m. UTC, <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;">I was thinking that what you were informing the remote endpoint was the largest packet size you may send them, which would be the highest of all of the combined formats.
{quote}
a=maxptime:<maximum packet time>
This gives the maximum amount of media that can be encapsulated
in each packet, expressed as time in milliseconds.
{quote}
Wouldn't we want to tell the endpoints that amongst codecs A, B, and C, the largest packet size is max(A,B,C), not min(A,B,C)?</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;">Messy attribute is messy. Since this applies to all codecs at media level if I am using codec A which has a max of 60ms and codec B which has a max of 120ms then if codec A is used the maximum amount of media we accept that can be encapsulated is not 120ms, it's 60ms. Now... would the Asterisk core actually CARE if we received 120ms? Probably not. That's my thinking.</pre>
<br />
<p>- Joshua</p>
<br />
<p>On August 19th, 2013, 11:05 p.m. UTC, 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.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers, Joshua Colp and Mark Michelson.</div>
<div>By Matt Jordan.</div>
<p style="color: grey;"><i>Updated Aug. 19, 2013, 11:05 p.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-21981">ASTERISK-21981</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;">Note: This patch was written by Lorenzo Miniero. I know he's at the IETF this week, but I figured we could get the formal code review going for him :-)
This patch adds pass through support for Opus and VP8. That includes:
* Format attribute negotiation for Opus. Note that unlike some other codecs, the draft RFC specifies having spaces delimiting the attributes in addition to ';', so you have "attra=X; attrb=Y". This broke the attribute parsing in chan_sip, so a small tweak was also included in this patch for that.
* A format attribute negotiation module for Opus
* Fast picture update for VP8. Since VP8 uses a different RTCP packet number than FIR, this really is specific to VP8 at this time. Ideally this would be more generic and flexible for user preferences and other video codecs, but that could be done at a latter date.
The only part of this work that I did was port over the fast picture update code to chan_pjsip. I *think* that chan_pjsip will still suck out the attributes in res_pjsip_sdp_rtp, but I could be mistaken (Josh?)</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/channels/chan_pjsip.c <span style="color: grey">(396942)</span></li>
<li>/trunk/channels/chan_sip.c <span style="color: grey">(396942)</span></li>
<li>/trunk/include/asterisk/format.h <span style="color: grey">(396942)</span></li>
<li>/trunk/include/asterisk/opus.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>/trunk/main/channel.c <span style="color: grey">(396942)</span></li>
<li>/trunk/main/format.c <span style="color: grey">(396942)</span></li>
<li>/trunk/main/frame.c <span style="color: grey">(396942)</span></li>
<li>/trunk/main/rtp_engine.c <span style="color: grey">(396942)</span></li>
<li>/trunk/res/res_format_attr_opus.c <span style="color: grey">(PRE-CREATION)</span></li>
<li>/trunk/res/res_pjsip_sdp_rtp.c <span style="color: grey">(396942)</span></li>
<li>/trunk/res/res_rtp_asterisk.c <span style="color: grey">(396942)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/2723/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>