<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/3665/">https://reviewboard.asterisk.org/r/3665/</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 23rd, 2014, 1:10 a.m. EDT, <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/3665/diff/3/?file=60235#file60235line87" style="color: black; font-weight: bold; text-decoration: underline;">/team/group/media_formats-reviewed/main/codec.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; ">static int codec_cmp(void *obj, void *arg, int flags)</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">87</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">if</span> <span class="p">(</span><span class="n">right</span><span class="o">-></span><span class="n">sample_rate</span><span class="p">)</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;">Why?</pre>
</blockquote>
<p>On June 23rd, 2014, 10:32 a.m. EDT, <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;">Because there are times when a user wants to look up a codec by name only, and we currently don't have a mechanism to do that.
Take, for example, "Opus". If I perform a 'core show translation path opus', it will fail without this check. Without providing the sample rate, we don't get a hit on the codec. The fact that Opus only has a single sample rate doesn't matter. We can't really infer what we should provide the lookup from the CLI command either.
I think forcing the sample rate is a little pessimistic: 90% of the time, you don't need to provide a sample rate to find the codec you're looking for. If you do have to provide the sample rate, this comparison function will still work: it will match on the sample rate as well if you provide it (and if you provide a sample rate and there is no codec that matches, it will return NULL as well).</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;">What about situations where multiple sample rate's exist for a codec? This seems to be related to my question about completion for sample rate's from the CLI command.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On June 23rd, 2014, 1:10 a.m. EDT, <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/3665/diff/3/?file=60240#file60240line297" style="color: black; font-weight: bold; text-decoration: underline;">/team/group/media_formats-reviewed/main/format_cache.c</a>
<span style="font-weight: normal;">
(Diff revision 3)
</span>
</th>
</tr>
</thead>
<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">289</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cp">#define SET_CACHED_FORMAT(cached_format, new_format) do { \</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'd prefer we just use George's ao2_replace.</pre>
</blockquote>
<p>On June 23rd, 2014, 10:32 a.m. EDT, <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;">Now that it is available, sure. I'll merge it into this review.</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;">Well if you are merging to media_formats-reviewed then it's not actually available (that branch has never merged new code from 12). It is in media_formats-reviewed-trunk. If needed I'm thinking we can either cherry-pick ao2_replace or just define it here instead of SET_CACHED_FORMAT (with a BUGBUG).</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On June 23rd, 2014, 1:10 a.m. EDT, <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/3665/diff/3/?file=60243#file60243line669" style="color: black; font-weight: bold; text-decoration: underline;">/team/group/media_formats-reviewed/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_set_m_type(struct ast_rtp_codecs *codecs, struct ast_rtp_instance *instance, int payload)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">663</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="n">ast_format_copy</span><span class="p">(</span><span class="o"><span class="hl">&</span></span><span class="n"><span class="hl">type</span></span><span class="o"><span class="hl">-></span></span><span class="n"><span class="hl">format</span></span><span class="p"><span class="hl">,</span></span><span class="hl"> </span><span class="o"><span class="hl">&</span></span><span class="n">static_RTP_PT</span><span class="p">[</span><span class="n">payload</span><span class="p">].</span><span class="n">format</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">669</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="hl"><span class="tb"> </span></span><span class="n"><span class="hl">type</span></span><span class="o"><span class="hl">-></span></span><span class="n"><span class="hl">format</span></span><span class="hl"> </span><span class="o"><span class="hl">=</span></span><span class="hl"> </span><span class="n">ast_format_copy</span><span class="p">(</span><span class="n">static_RTP_PT</span><span class="p">[</span><span class="n">payload</span><span class="p">].</span><span class="n">format</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;">type->format could be leaking a reference here.</pre>
</blockquote>
<p>On June 23rd, 2014, 10:32 a.m. EDT, <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;">If this pattern becomes common, we may want to consider a version of ast_format_copy that cleans up the destination, similar to ao2_replace. (ao2_replace won't work here, as ast_format_copy already bumps the reference of the copied format)</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 think ast_format_copy is a bad function, it should be a macro. Being a function makes it so when chan_iax2 copies a format we see format.c:ast_format_copy in the refs log (not helpful).
#define ast_format_copy(f) ao2_bump(f)
#define ast_format_replace(dst,src) ao2_replace(dst,src)
Also I think maybe ast_format_copy is a "legacy name", and ast_format_bump would be more appropriate. This would make it clearer that the purpose is to increase the ao2 ref.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On June 23rd, 2014, 1:10 a.m. EDT, <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/3665/diff/3/?file=60245#file60245line1080" style="color: black; font-weight: bold; text-decoration: underline;">/team/group/media_formats-reviewed/main/translate.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; ">static char *handle_cli_core_show_translation(struct ast_cli_entry *e, int cmd, struct ast_cli_args *a)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">1059</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="k">return</span> <span class="nb">NULL</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">1075</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="k">return</span> <span class="nb">NULL</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;">Looks like we're missing completion for sample_rate. Let's BUGBUG it so we remember to consider adding it later.</pre>
</blockquote>
<p>On June 23rd, 2014, 10:32 a.m. EDT, <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 thought about that, except I'm not sure how you would do completion for sample rate. The only way we could feasibly do it would be to expose an iterator over all currently registered codecs, and I'm a little hesitant over whether or not that is strictly necessary. Generally, if you want the translation paths for a specific sample rate of slin, you have a general idea of the sample rates you can use.
On the other hand, the fact that sample rate isn't tab completable is partly why I made sample rate optional in the lookup for codecs, so I'm open to suggestions here.</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 think a BUGBUG comment would be best so we can explore this later. Even if I'm right this is not something that we should hold up the review for.
My reason for thinking we should provide this completion is from the CLI help string:
If a codec has multiple sample rates, the sample rate must be provided as well.</pre>
<br />
<p>- Corey</p>
<br />
<p>On June 23rd, 2014, 10:51 a.m. EDT, 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, Corey Farrell and Joshua Colp.</div>
<div>By Matt Jordan.</div>
<p style="color: grey;"><i>Updated June 23, 2014, 10:51 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 patch includes all of Corey's fine work on r3625, more that he did in channel/rtp_engine/dsp, and enough work in format_cache/elsewhere to get Asterisk's core to compile, along with some improvements in translate.
With this patch, Asterisk (with very little loaded) should run and generally display the codec path translations. I'm still not convinced we're computing computational complexity correctly for everything - particularly translations provided by codec_resample - but the table produced matches Asterisk 11/12, so that's a good step.
Major changes made in this patch:
* Removed ast_best_codec, as it was a farce [1]. All channel drivers will now use the first codec listed in their configured set of codecs as their preferred codec.
* Formats now store their name, as it can differ from the codec. This now has the accessor ast_format_get_name; codecs get the new ast_format_get_codec_name. Similarly, formats can now be constructed either entirely from the codec, or from a codec + name.
* Updated the format_cache with the expected short-hand pointers to the cached formats.
* channel.c was updated. That's large. Note that this was done mostly by Corey Farrell
* Codecs can do an explicit name match without their sample rate. This is done to make it a bit easier for CLI commands to query codecs with singular but odd sample rates (looking at you Opus)
* CLI commands in translate.c should now mostly work. translate.c will now correctly register translation paths - previously, it used the passed in codecs, which did not contain the codec->id field.
[1] http://lists.digium.com/pipermail/asterisk-dev/2014-June/068133.html</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/tests/test_format_cache.c <span style="color: grey">(417074)</span></li>
<li>/team/group/media_formats-reviewed/res/res_pjsip_sdp_rtp.c <span style="color: grey">(417074)</span></li>
<li>/team/group/media_formats-reviewed/main/translate.c <span style="color: grey">(417074)</span></li>
<li>/team/group/media_formats-reviewed/main/slinfactory.c <span style="color: grey">(417074)</span></li>
<li>/team/group/media_formats-reviewed/main/rtp_engine.c <span style="color: grey">(417074)</span></li>
<li>/team/group/media_formats-reviewed/main/frame.c <span style="color: grey">(417074)</span></li>
<li>/team/group/media_formats-reviewed/main/format_cap.c <span style="color: grey">(417074)</span></li>
<li>/team/group/media_formats-reviewed/main/format_cache.c <span style="color: grey">(417074)</span></li>
<li>/team/group/media_formats-reviewed/main/format.c <span style="color: grey">(417074)</span></li>
<li>/team/group/media_formats-reviewed/main/dsp.c <span style="color: grey">(417074)</span></li>
<li>/team/group/media_formats-reviewed/main/core_unreal.c <span style="color: grey">(417074)</span></li>
<li>/team/group/media_formats-reviewed/main/codec_builtin.c <span style="color: grey">(417074)</span></li>
<li>/team/group/media_formats-reviewed/main/codec.c <span style="color: grey">(417074)</span></li>
<li>/team/group/media_formats-reviewed/main/channel.c <span style="color: grey">(417074)</span></li>
<li>/team/group/media_formats-reviewed/main/asterisk.c <span style="color: grey">(417074)</span></li>
<li>/team/group/media_formats-reviewed/include/asterisk/rtp_engine.h <span style="color: grey">(417074)</span></li>
<li>/team/group/media_formats-reviewed/include/asterisk/format_cache.h <span style="color: grey">(417074)</span></li>
<li>/team/group/media_formats-reviewed/include/asterisk/format.h <span style="color: grey">(417074)</span></li>
<li>/team/group/media_formats-reviewed/include/asterisk/channel.h <span style="color: grey">(417074)</span></li>
<li>/team/group/media_formats-reviewed/include/asterisk/astobj2.h <span style="color: grey">(417074)</span></li>
<li>/team/group/media_formats-reviewed/include/asterisk/_private.h <span style="color: grey">(417074)</span></li>
<li>/team/group/media_formats-reviewed/channels/chan_unistim.c <span style="color: grey">(417074)</span></li>
<li>/team/group/media_formats-reviewed/channels/chan_skinny.c <span style="color: grey">(417074)</span></li>
<li>/team/group/media_formats-reviewed/channels/chan_sip.c <span style="color: grey">(417074)</span></li>
<li>/team/group/media_formats-reviewed/channels/chan_phone.c <span style="color: grey">(417074)</span></li>
<li>/team/group/media_formats-reviewed/channels/chan_multicast_rtp.c <span style="color: grey">(417074)</span></li>
<li>/team/group/media_formats-reviewed/channels/chan_misdn.c <span style="color: grey">(417074)</span></li>
<li>/team/group/media_formats-reviewed/channels/chan_mgcp.c <span style="color: grey">(417074)</span></li>
<li>/team/group/media_formats-reviewed/channels/chan_jingle.c <span style="color: grey">(417074)</span></li>
<li>/team/group/media_formats-reviewed/channels/chan_iax2.c <span style="color: grey">(417074)</span></li>
<li>/team/group/media_formats-reviewed/channels/chan_h323.c <span style="color: grey">(417074)</span></li>
<li>/team/group/media_formats-reviewed/channels/chan_gtalk.c <span style="color: grey">(417074)</span></li>
<li>/team/group/media_formats-reviewed/addons/chan_ooh323.c <span style="color: grey">(417074)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/3665/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>