<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/4505/">https://reviewboard.asterisk.org/r/4505/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 17th, 2015, 11:57 a.m. CDT, <b>Matt Jordan</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/4505/diff/1/?file=72588#file72588line77" style="color: black; font-weight: bold; text-decoration: underline;">/tags/13.2.0/include/asterisk/codec.h</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_codec {</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">77</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="kt">unsigned</span> <span class="kt">int</span> <span class="n">smooth</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">77</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="kt">unsigned</span> <span class="kt">int</span> <span class="n">smooth</span><span class="p">;</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">78</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="cm">/*! \brief Whether the smoother is operated in BE byte order */</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">79</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="kt">unsigned</span> <span class="kt">int</span> <span class="n">smoother_be</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">78</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="cm">/*! \brief The module that registered this codec */</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">80</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="cm">/*! \brief The module that registered this codec */</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;">I don't think you can trust that the codec will know its endianness. Looking at the resample code, I don't _think_ it actually determines the endianness of its encoding/decoding, and instead relies on the underlying machine to make that determination. As such, I don't think this should be a property on the codec structure.</pre>
</blockquote>
<p>On March 17th, 2015, 8:20 p.m. CDT, <b>Frankie Chin</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;">Matt, I actually followed the implementation in Asterisk 12.8.1 where the AST_SMOOTHER_FLAG_BE was defined for all the SLIN codecs in main/format.c under the format_list_init() method. Do you mean this implementation back in 12.8.1 was inappropriate? FYI, slin codec used to work fine in Asterisk 12.8.1 for our application.</pre>
</blockquote>
<p>On March 19th, 2015, 4:48 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;">Apologies; you are absolutely correct about that.
I'll need a day or two to think about whether or not this is the right way to handle passing smoother flags in. A part of me dislikes having an explicit BE byte order integer, but I'm also not a giant fan of storing a bunch of smoother properties directly on the codec. There may just not be a better place for it.</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;">After looking at 12 some more, I think handling this in the same manner as Asterisk 12 is probably the best way forward. That would mean adding a 'smoother_flags' option
struct ast_codec {
...
unsigned int smooth;
unsigned int smoother_flags;
}
That would change the API call from ast_format_smoothed_with_be to ast_format_get_smoother_flags (or something similar). That way, any additional smoother flags can be obtained without needing additional APIs added.</pre>
<br />
<p>- Matt</p>
<br />
<p>On March 16th, 2015, 10:36 p.m. CDT, Frankie Chin 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 Frankie Chin.</div>
<p style="color: grey;"><i>Updated March 16, 2015, 10:36 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-24858">ASTERISK-24858</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;">In Asterisk 13.2.0 when SLIN codec is used in two Asterisk servers registered to one another via PJSIP, the RTP payload is sent in the wrong byte order. The patch addresses the following based on the correct behavior in Asterisk 12.8.1:
1) Save ptime = 20 as the framing in the ast_rtp_codecs structure when creating outgoing SDP packet (res_pjsip_sdp_rtp.c)
2) Do not copy the framing when copying the payload (rtp_engine.c)
3) Introduce the new "smoother_be" flagin the ast_codec structure. Set this flag = 1 for all the SLIN codecs (codec_builtin.c).
4) Check for this "smoother_be" flag before using the smoother on the data (res_rtp_asterisk.c)</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;">The patch was tested using the scenario described in ASTERISK-24858</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>/tags/13.2.0/res/res_rtp_asterisk.c <span style="color: grey">(433002)</span></li>
<li>/tags/13.2.0/res/res_pjsip_sdp_rtp.c <span style="color: grey">(433002)</span></li>
<li>/tags/13.2.0/main/rtp_engine.c <span style="color: grey">(433002)</span></li>
<li>/tags/13.2.0/main/format.c <span style="color: grey">(433002)</span></li>
<li>/tags/13.2.0/main/codec_builtin.c <span style="color: grey">(433002)</span></li>
<li>/tags/13.2.0/include/asterisk/format.h <span style="color: grey">(433002)</span></li>
<li>/tags/13.2.0/include/asterisk/codec.h <span style="color: grey">(433002)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/4505/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>