[asterisk-dev] [Code Review] 3665: media_formats: Core updates, plus format_cache/channel/and some other stuff

Mon Jun 23 09:32:41 CDT 2014

> On June 23, 2014, 12:10 a.m., Corey Farrell wrote:
> > Sorry to hit so many issues, I did tell you I was not so confident with the changes I made to the last 3 files.  Looking at it a second time has helped find stuff.  Feel free to BUGBUG things where possible.  I've clicked "open an issue" on a couple findings that aren't necessarily issues, I did this so they could be fixed or dropped and nothing will be missed.
> > 
> > I also checked this against /team/group/media_formats-reviewed-trunk, it only has 3 patch chunks that failed (1 in rtp_engine.c and 3 in channel.c).  Your call if you want to switch the branch for this change or not.

No worries - thanks for the review!

> On June 23, 2014, 12:10 a.m., Corey Farrell wrote:
> > /team/group/media_formats-reviewed/include/asterisk/format_cache.h, lines 280-281
> > <https://reviewboard.asterisk.org/r/3665/diff/3/?file=60232#file60232line280>
> >
> >     If SLIN is an acronym for signed, abbreviation for linear (S. lin), then it's "an" SLIN format.  a/an is used based on the immediate next sound, like an FBI agent or a Federal Agent.
> >     
> >     On the other hand maybe I've been pronouncing slin wrong and it actually sounds similar to slim?

Boy, I have no idea! When I read 'SLIN' in my head, I always read it the full way - 'Signed Linear'. So 'a Signed Linear format' - in my head at any rate - is "sounds" correct.

But I'm fine with 'an' as well.

Anyone else have any objections?

> On June 23, 2014, 12:10 a.m., Corey Farrell wrote:
> > /team/group/media_formats-reviewed/main/channel.c, line 5168
> > <https://reviewboard.asterisk.org/r/3665/diff/3/?file=60234#file60234line5168>
> >
> >     Please split line (I know it was already too big but now it's huge).

Side tangent: I can't wait to remove the monitor code out of here. But that's a problem for another day.

> On June 23, 2014, 12:10 a.m., Corey Farrell wrote:
> > /team/group/media_formats-reviewed/main/channel.c, lines 6119-6120
> > <https://reviewboard.asterisk.org/r/3665/diff/3/?file=60234#file60234line6119>
> >
> >     Candidates for RAII_VAR's?

Yes, since we're leaking like a sieve here

> On June 23, 2014, 12:10 a.m., Corey Farrell wrote:
> > /team/group/media_formats-reviewed/main/codec.c, line 87
> > <https://reviewboard.asterisk.org/r/3665/diff/3/?file=60235#file60235line87>
> >
> >     Why?

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).

> On June 23, 2014, 12:10 a.m., Corey Farrell wrote:
> > /team/group/media_formats-reviewed/main/format_cache.c, line 297
> > <https://reviewboard.asterisk.org/r/3665/diff/3/?file=60240#file60240line297>
> >
> >     I'd prefer we just use George's ao2_replace.

Now that it is available, sure. I'll merge it into this review.

> On June 23, 2014, 12:10 a.m., Corey Farrell wrote:
> > /team/group/media_formats-reviewed/main/format_cache.c, line 449
> > <https://reviewboard.asterisk.org/r/3665/diff/3/?file=60240#file60240line449>
> >
> >     Do we still want to block ast_format_cache_set(name, NULL) ?

Probably not. Fixed.

> On June 23, 2014, 12:10 a.m., Corey Farrell wrote:
> > /team/group/media_formats-reviewed/main/format_cache.c, line 524
> > <https://reviewboard.asterisk.org/r/3665/diff/3/?file=60240#file60240line524>
> >
> >     Is this good enough or do we need ast_format_cmp?

It's probably better to use ast_format_cmp.

> On June 23, 2014, 12:10 a.m., Corey Farrell wrote:
> > /team/group/media_formats-reviewed/main/rtp_engine.c, line 588
> > <https://reviewboard.asterisk.org/r/3665/diff/3/?file=60243#file60243line588>
> >
> >     Need a destructor to handle format ref

And since this is a public struct, the allocation routine should be public as well to guarantee that consumers who create said struct get proper destruction. Added.

> On June 23, 2014, 12:10 a.m., Corey Farrell wrote:
> > /team/group/media_formats-reviewed/main/rtp_engine.c, line 669
> > <https://reviewboard.asterisk.org/r/3665/diff/3/?file=60243#file60243line669>
> >
> >     type->format could be leaking a reference here.

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)

> On June 23, 2014, 12:10 a.m., Corey Farrell wrote:
> > /team/group/media_formats-reviewed/main/rtp_engine.c, line 801
> > <https://reviewboard.asterisk.org/r/3665/diff/3/?file=60243#file60243line801>
> >
> >     This is probably my doing, but I'm not sure we should directly use ao2_bump instead of ast_format_copy.

Agreed; we should use the library specific functions where possible (who knows, some day it may be more than just a ref bump).

Users of this function are also leaking the returned format; fixed that as well.

> On June 23, 2014, 12:10 a.m., Corey Farrell wrote:
> > /team/group/media_formats-reviewed/main/rtp_engine.c, line 1604
> > <https://reviewboard.asterisk.org/r/3665/diff/3/?file=60243#file60243line1604>
> >
> >     We will need a shutdown procedure to clear these references

I'm glad you caught this - as your other finding pointed out, ast_rtp_engine_unload_format is blowing away the references currently. Fixed that there; updated the shutdown routine to clean up any formats that are left over.

> On June 23, 2014, 12:10 a.m., Corey Farrell wrote:
> > /team/group/media_formats-reviewed/main/translate.c, line 1080
> > <https://reviewboard.asterisk.org/r/3665/diff/3/?file=60245#file60245line1080>
> >
> >     Looks like we're missing completion for sample_rate.  Let's BUGBUG it so we remember to consider adding it later.

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.

- Matt

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3665/#review12269
-----------------------------------------------------------

On June 22, 2014, 8:35 p.m., Matt Jordan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3665/
> -----------------------------------------------------------
> 
> (Updated June 22, 2014, 8:35 p.m.)
> 
> 
> Review request for Asterisk Developers, Corey Farrell and Joshua Colp.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> 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
> 
> 
> Diffs
> -----
> 
>   /team/group/media_formats-reviewed/tests/test_format_cache.c 417056 
>   /team/group/media_formats-reviewed/main/translate.c 417056 
>   /team/group/media_formats-reviewed/main/slinfactory.c 417056 
>   /team/group/media_formats-reviewed/main/rtp_engine.c 417056 
>   /team/group/media_formats-reviewed/main/frame.c 417056 
>   /team/group/media_formats-reviewed/main/format_cap.c 417056 
>   /team/group/media_formats-reviewed/main/format_cache.c 417056 
>   /team/group/media_formats-reviewed/main/format.c 417056 
>   /team/group/media_formats-reviewed/main/dsp.c 417056 
>   /team/group/media_formats-reviewed/main/core_unreal.c 417056 
>   /team/group/media_formats-reviewed/main/codec_builtin.c 417056 
>   /team/group/media_formats-reviewed/main/codec.c 417056 
>   /team/group/media_formats-reviewed/main/channel.c 417056 
>   /team/group/media_formats-reviewed/main/asterisk.c 417056 
>   /team/group/media_formats-reviewed/include/asterisk/format_cache.h 417056 
>   /team/group/media_formats-reviewed/include/asterisk/format.h 417056 
>   /team/group/media_formats-reviewed/include/asterisk/channel.h 417056 
>   /team/group/media_formats-reviewed/channels/chan_unistim.c 417056 
>   /team/group/media_formats-reviewed/channels/chan_skinny.c 417056 
>   /team/group/media_formats-reviewed/channels/chan_phone.c 417056 
>   /team/group/media_formats-reviewed/channels/chan_multicast_rtp.c 417056 
>   /team/group/media_formats-reviewed/channels/chan_misdn.c 417056 
>   /team/group/media_formats-reviewed/channels/chan_mgcp.c 417056 
>   /team/group/media_formats-reviewed/channels/chan_jingle.c 417056 
>   /team/group/media_formats-reviewed/channels/chan_iax2.c 417056 
>   /team/group/media_formats-reviewed/channels/chan_h323.c 417056 
>   /team/group/media_formats-reviewed/channels/chan_gtalk.c 417056 
>   /team/group/media_formats-reviewed/addons/chan_ooh323.c 417056 
> 
> Diff: https://reviewboard.asterisk.org/r/3665/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Matt Jordan
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140623/a4479dc4/attachment-0001.html>