[asterisk-dev] [Code Review] 3178: media_formats: Moving of existing code around, implementation, and unit tests

Wed Feb 5 18:18:37 CST 2014

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

All of my comments have to do with implementation details as opposed to the validity of the API itself.

/branches/12/include/asterisk/vector.h
<https://reviewboard.asterisk.org/r/3178/#comment20289>

    This macro needs to have some sort of warning attached to it because if you use this, and you actually expect the inserted element to remain at the position indicated, then it basically means that removing items from the vector is prohibited. Otherwise, it's possible for your item to change positions in the vector (especially if you perform an ordered removal).

/branches/12/include/asterisk/vector.h
<https://reviewboard.asterisk.org/r/3178/#comment20287>

    It's recommended that you place parentheses around idx each time it is referenced in the macro. Either that or do like other macros do in this file and have

    size_t __idx = (idx);

    at the beginning and then reference __idx for the duration of the macro.

/branches/12/include/asterisk/vector.h
<https://reviewboard.asterisk.org/r/3178/#comment20312>

    There's tons of red in this macro. I'm not sure what's up with that.

/branches/12/main/asterisk.c
<https://reviewboard.asterisk.org/r/3178/#comment20290>

    red

/branches/12/main/format.c
<https://reviewboard.asterisk.org/r/3178/#comment20291>

    There's some redundancy between this file and codec.c. This enum is repeated, and some functions that get sample size are repeated.

/branches/12/main/format_cap_ng.c
<https://reviewboard.asterisk.org/r/3178/#comment20292>

    Set this equal to AST_LIST_HEAD_NOLOCK_INIT_VALUE instead of { NULL, };

/branches/12/main/format_cap_ng.c
<https://reviewboard.asterisk.org/r/3178/#comment20307>

    This has a sort-of side effect. The codecs are added in order of their IDs, which makes the preference order end up in that order as well.

    That may have been your intention, but if not, I figured I'd point it out.

/branches/12/main/format_cap_ng.c
<https://reviewboard.asterisk.org/r/3178/#comment20299>

    Documentation in codec.h says that codec IDs start with 1 and not to start at 0 if iterating over them.

/branches/12/main/format_cap_ng.c
<https://reviewboard.asterisk.org/r/3178/#comment20308>

    If src and dst have overlapping formats, this will mean doubling up on formats in the dst structure. Is that intentional?

/branches/12/main/format_cap_ng.c
<https://reviewboard.asterisk.org/r/3178/#comment20302>

    Rather than referring to cap->preference_order.current, you should use AST_VECTOR_SIZE()

/branches/12/main/format_cap_ng.c
<https://reviewboard.asterisk.org/r/3178/#comment20309>

    Use AST_VECTOR_SIZE instead of referring to cap->formats.current

/branches/12/main/format_cap_ng.c
<https://reviewboard.asterisk.org/r/3178/#comment20310>

    Another place where you should use AST_VECTOR_SIZE()

/branches/12/main/format_cap_ng.c
<https://reviewboard.asterisk.org/r/3178/#comment20311>

    Should the call to AST_VECTOR_REMOVE_CMP_ORDERED be outside the list traversal?

- Mark Michelson

On Feb. 4, 2014, 3:57 p.m., Joshua Colp wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3178/
> -----------------------------------------------------------
> 
> (Updated Feb. 4, 2014, 3:57 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This change has a few things in it:
> 
> 1. Some media related things have been moved around to more logical places or their own parts (smoothers).
> 
> 2. A new implementation of media formats according to https://wiki.asterisk.org/wiki/display/AST/Media+Format+Rewrite. The implementation doesn't completely adhere to the design since I tweaked things but it mostly conforms.
> 
> 3. Unit tests for the above implementation.
> 
> What I'd like feedback on is the actual media formats implementation and the API design itself. Is this something you would be comfortable using?
> 
> 
> Diffs
> -----
> 
>   /branches/12/tests/test_format_cap.c PRE-CREATION 
>   /branches/12/tests/test_format_cache.c PRE-CREATION 
>   /branches/12/tests/test_core_format.c PRE-CREATION 
>   /branches/12/tests/test_core_codec.c PRE-CREATION 
>   /branches/12/res/res_rtp_asterisk.c 406006 
>   /branches/12/res/res_fax.c 406006 
>   /branches/12/main/smoother.c PRE-CREATION 
>   /branches/12/main/frame.c 406006 
>   /branches/12/main/format_ng.c PRE-CREATION 
>   /branches/12/main/format_cap_ng.c PRE-CREATION 
>   /branches/12/main/format_cache.c PRE-CREATION 
>   /branches/12/main/format.c 406006 
>   /branches/12/main/codec_builtin.c PRE-CREATION 
>   /branches/12/main/codec.c PRE-CREATION 
>   /branches/12/main/asterisk.c 406006 
>   /branches/12/include/asterisk/vector.h 406006 
>   /branches/12/include/asterisk/smoother.h PRE-CREATION 
>   /branches/12/include/asterisk/frame.h 406006 
>   /branches/12/include/asterisk/format_ng.h PRE-CREATION 
>   /branches/12/include/asterisk/format_cap_ng.h PRE-CREATION 
>   /branches/12/include/asterisk/format_cache.h PRE-CREATION 
>   /branches/12/include/asterisk/format.h 406006 
>   /branches/12/include/asterisk/codec.h PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/3178/diff/
> 
> 
> Testing
> -------
> 
> Ran unit tests, all passed.
> 
> Note: I know AO2 throws a fit and it's because a container isn't getting initialized. Getting said container initialized requires beginning the hacking apart process.
> 
> 
> Thanks,
> 
> Joshua Colp
> 
>

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