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

[Matt Jordan]

> On June 23, 2014, 12:41 p.m., Corey Farrell wrote:
> > /team/group/media_formats-reviewed/main/rtp_engine.c, line 234
> > <https://reviewboard.asterisk.org/r/3665/diff/3-4/?file=60243#file60243line234>
> >
> >     For a static procedure shouldn't we omit the "ast_" namespace?

I didn't here simply because the name of the structure is 'ast_rtp_payload_type'. However, I didn't like it when I did it ... and I think I'd prefer no ast_ (generally) in static declarations.

Removed.


> On June 23, 2014, 12:41 p.m., Corey Farrell wrote:
> > /team/group/media_formats-reviewed/main/rtp_engine.c, line 756
> > <https://reviewboard.asterisk.org/r/3665/diff/3-4/?file=60243#file60243line756>
> >
> >     Still need to bump g726_aal2.

Thanks; fixed.


> On June 23, 2014, 12:41 p.m., Corey Farrell wrote:
> > /team/group/media_formats-reviewed/main/rtp_engine.c, lines 1978-1992
> > <https://reviewboard.asterisk.org/r/3665/diff/3-4/?file=60243#file60243line1978>
> >
> >     These differ slightly from ast_rtp_engine_unload_format.  Might be better to create:
> >     static void rtp_engine_static_RTP_PT_cleanup(int i);
> >     static void rtp_engine_mime_type_cleanup(int i);

It was slightly different as in unload_format, we explicitly check that the format exists.

I'm fine with some helper functions however. It would certainly make the code more consistent, and the cost is negligible.

It also concerns me that we memset out the ast_rtp_payload_type explicitly in static_RTP_PT, but leave values lying around in ast_rtp_mime_types. Granted, one has a fixed size and the other uses a flexible 'current max size', but leaving stale values could result in shenanigans if a new format is added to ast_rtp_mime_types - which can happen when certain codecs are loaded.


- Matt


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


On June 23, 2014, 9: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, 9: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/51b621dd/attachment-0001.html>