[asterisk-dev] [Code Review] 3512: media formats: Convert the translation core over

[Mark Michelson]

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



team/group/media_formats-reviewed/include/asterisk/codec.h
<https://reviewboard.asterisk.org/r/3512/#comment21732>

    1) Document that this function appends the names onto the buf rather than just setting the input buf to the codec names.
    2) Returning an ast_str ** is awkward and unnecessary. If you wanted to make this return something, a more natural way might be to do something like:
    
    struct ast_str *ast_codec_get_names(const struct ast_codec *codec, struct ast_str *buf);
    
    And then require that it gets called like:
    
    str = ast_codec_get_names(codec, str);
    
    As it is, I'm fine with it just returning void (or possibly a pass/fail int).



team/group/media_formats-reviewed/main/codec.c
<https://reviewboard.asterisk.org/r/3512/#comment21727>

    This won't compile since buf is undeclared.
    
    I can see two ways of fixing this.
    1) Switch to using ao2_callback_data() and pass the buf into this callback.
    
    2) Since you're doing a straight pointer comparison, will the same codec ever exist in the container more than once? If not, then instead of performing an ao2_callback to append codec names to an ast_str, change the ao2_callback to find the single matching codec and perform the string append in ast_codec_get_names().



team/group/media_formats-reviewed/main/translate.c
<https://reviewboard.asterisk.org/r/3512/#comment21729>

    Does a comparison between different sample rates of signed linear codecs return AST_FORMAT_CMP_EQUAL? If not, then this functions differently from the old code.


- Mark Michelson


On May 8, 2014, 5:16 p.m., Kevin Harwell wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3512/
> -----------------------------------------------------------
> 
> (Updated May 8, 2014, 5:16 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: SWP-6725
>     https://issues.asterisk.org/jira/browse/SWP-6725
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Updated the translation core (translate.c) to use the new media format API.
> 
> 
> Diffs
> -----
> 
>   team/group/media_formats-reviewed/main/translate.c 413539 
>   team/group/media_formats-reviewed/main/format.c 413539 
>   team/group/media_formats-reviewed/main/codec.c 413539 
>   team/group/media_formats-reviewed/include/asterisk/format.h 413539 
>   team/group/media_formats-reviewed/include/asterisk/codec.h 413539 
> 
> Diff: https://reviewboard.asterisk.org/r/3512/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Kevin Harwell
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140508/8d162490/attachment.html>