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

Corey Farrell reviewboard at asterisk.org
Mon Jun 23 11:48:00 CDT 2014 


> On June 23, 2014, 1: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.
> 
> Matt Jordan wrote:
>     Now that it is available, sure. I'll merge it into this review.

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


> On June 23, 2014, 1: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.
> 
> Matt Jordan wrote:
>     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.

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.


> On June 23, 2014, 1: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.
> 
> Matt Jordan wrote:
>     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)

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.


> On June 23, 2014, 1: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?
> 
> Matt Jordan wrote:
>     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).

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.


- Corey


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


On June 23, 2014, 10:51 a.m., Matt Jordan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3665/
> -----------------------------------------------------------
> 
> (Updated June 23, 2014, 10:51 a.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 417074 
>   /team/group/media_formats-reviewed/res/res_pjsip_sdp_rtp.c 417074 
>   /team/group/media_formats-reviewed/main/translate.c 417074 
>   /team/group/media_formats-reviewed/main/slinfactory.c 417074 
>   /team/group/media_formats-reviewed/main/rtp_engine.c 417074 
>   /team/group/media_formats-reviewed/main/frame.c 417074 
>   /team/group/media_formats-reviewed/main/format_cap.c 417074 
>   /team/group/media_formats-reviewed/main/format_cache.c 417074 
>   /team/group/media_formats-reviewed/main/format.c 417074 
>   /team/group/media_formats-reviewed/main/dsp.c 417074 
>   /team/group/media_formats-reviewed/main/core_unreal.c 417074 
>   /team/group/media_formats-reviewed/main/codec_builtin.c 417074 
>   /team/group/media_formats-reviewed/main/codec.c 417074 
>   /team/group/media_formats-reviewed/main/channel.c 417074 
>   /team/group/media_formats-reviewed/main/asterisk.c 417074 
>   /team/group/media_formats-reviewed/include/asterisk/rtp_engine.h 417074 
>   /team/group/media_formats-reviewed/include/asterisk/format_cache.h 417074 
>   /team/group/media_formats-reviewed/include/asterisk/format.h 417074 
>   /team/group/media_formats-reviewed/include/asterisk/channel.h 417074 
>   /team/group/media_formats-reviewed/include/asterisk/astobj2.h 417074 
>   /team/group/media_formats-reviewed/include/asterisk/_private.h 417074 
>   /team/group/media_formats-reviewed/channels/chan_unistim.c 417074 
>   /team/group/media_formats-reviewed/channels/chan_skinny.c 417074 
>   /team/group/media_formats-reviewed/channels/chan_sip.c 417074 
>   /team/group/media_formats-reviewed/channels/chan_phone.c 417074 
>   /team/group/media_formats-reviewed/channels/chan_multicast_rtp.c 417074 
>   /team/group/media_formats-reviewed/channels/chan_misdn.c 417074 
>   /team/group/media_formats-reviewed/channels/chan_mgcp.c 417074 
>   /team/group/media_formats-reviewed/channels/chan_jingle.c 417074 
>   /team/group/media_formats-reviewed/channels/chan_iax2.c 417074 
>   /team/group/media_formats-reviewed/channels/chan_h323.c 417074 
>   /team/group/media_formats-reviewed/channels/chan_gtalk.c 417074 
>   /team/group/media_formats-reviewed/addons/chan_ooh323.c 417074 
> 
> 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/327f817e/attachment-0001.html>

More information about the asterisk-dev mailing list