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

[Kevin Harwell]

> On May 8, 2014, 1 p.m., Mark Michelson wrote:
> > team/group/media_formats-reviewed/include/asterisk/codec.h, lines 180-189
> > <https://reviewboard.asterisk.org/r/3512/diff/3/?file=58318#file58318line180>
> >
> >     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).

This method has been dropped.


> On May 8, 2014, 1 p.m., Mark Michelson wrote:
> > team/group/media_formats-reviewed/main/codec.c, lines 355-364
> > <https://reviewboard.asterisk.org/r/3512/diff/3/?file=58320#file58320line355>
> >
> >     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().

After talking it over with Josh it turns out that there is no need for the name lookup at all.  This was an artifact of the old code that did not store the name with the codec, so it had to be looked up.   Also Josh mentioned it might be helpful to print out the sample rate along with the name, so I I'll add that in as well.


> On May 8, 2014, 1 p.m., Mark Michelson wrote:
> > team/group/media_formats-reviewed/main/translate.c, lines 714-715
> > <https://reviewboard.asterisk.org/r/3512/diff/3/?file=58322#file58322line714>
> >
> >     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.

It doesn't properly check.  I should be able to check against the codec name since all slin codecs have the name "slin".


- Kevin


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


On May 8, 2014, 12: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, 12: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/b80b8887/attachment.html>