[asterisk-dev] [Code Review] 3519: media_formats: Add legacy format API and move chan_iax2, chan_h323, and chan_misdn over.

[Kevin Harwell]

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



/team/group/media_formats-reviewed/channels/chan_h323.c
<https://reviewboard.asterisk.org/r/3519/#comment21616>

    If no longer needed then delete, or did you mean to come back to this?



/team/group/media_formats-reviewed/channels/chan_h323.c
<https://reviewboard.asterisk.org/r/3519/#comment21619>

    More of the same commented code.



/team/group/media_formats-reviewed/channels/chan_iax2.c
<https://reviewboard.asterisk.org/r/3519/#comment21623>

    May not be a problem, but what happens if/when these formats get unref'ed since it points to a global object?  If the incoming frame is on the stack the format won't be unref'ed so should be fine, otherwise these might need a ref bump.



/team/group/media_formats-reviewed/channels/h323/ast_h323.cxx
<https://reviewboard.asterisk.org/r/3519/#comment21629>

    This should be unreachable now.  Should be ast_format_get_type(tmpfmt) != AST_MEDIA_TYPE_AUDIO



/team/group/media_formats-reviewed/channels/h323/ast_h323.cxx
<https://reviewboard.asterisk.org/r/3519/#comment21637>

    Same here for codec->default_ms, but can use "ast_format_get_default_ms "



/team/group/media_formats-reviewed/main/format_compatibility.c
<https://reviewboard.asterisk.org/r/3519/#comment21633>

    What about #defining the various (1ULL << num) as I see them repeated in several spots and it might make sense to have a name to them?



/team/group/media_formats-reviewed/main/format_compatibility.c
<https://reviewboard.asterisk.org/r/3519/#comment21634>

    It looks possible that this could return NULL.  Looking through some of the code I saw a few spots where this function was called but the NULL result was not checked for and there would be a possibility of a NULL pointer being dereffed.  So do those places need a NULL check, should this return some kind of empty format representing NULL, or is this something that really should never happen or are all the cases calling this know based on the passed in params that NULL won't be returned?


- Kevin Harwell


On April 30, 2014, 5:54 p.m., Joshua Colp wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3519/
> -----------------------------------------------------------
> 
> (Updated April 30, 2014, 5:54 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This change adds a legacy legacy format compatibility API which is used by chan_iax2, chan_h323, and chan_misdn to work in new media formats land.
> 
> 
> Diffs
> -----
> 
>   /team/group/media_formats-reviewed/main/format_compatibility.c PRE-CREATION 
>   /team/group/media_formats-reviewed/main/codec_builtin.c 413144 
>   /team/group/media_formats-reviewed/include/asterisk/translate.h 413144 
>   /team/group/media_formats-reviewed/include/asterisk/format_compatibility.h PRE-CREATION 
>   /team/group/media_formats-reviewed/include/asterisk/format_cache.h 413144 
>   /team/group/media_formats-reviewed/include/asterisk/codec.h 413144 
>   /team/group/media_formats-reviewed/codecs/codec_dahdi.c 413144 
>   /team/group/media_formats-reviewed/channels/iax2/provision.c 413144 
>   /team/group/media_formats-reviewed/channels/iax2/parser.c 413144 
>   /team/group/media_formats-reviewed/channels/h323/chan_h323.h 413144 
>   /team/group/media_formats-reviewed/channels/h323/ast_h323.cxx 413144 
>   /team/group/media_formats-reviewed/channels/chan_phone.c 413144 
>   /team/group/media_formats-reviewed/channels/chan_misdn.c 413144 
>   /team/group/media_formats-reviewed/channels/chan_iax2.c 413144 
>   /team/group/media_formats-reviewed/channels/chan_h323.c 413144 
>   /team/group/media_formats-reviewed/apps/app_meetme.c 413144 
> 
> Diff: https://reviewboard.asterisk.org/r/3519/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Joshua Colp
> 
>

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