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

Wed Feb 5 15:52:49 CST 2014

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

For my initial pass, I only looked at the .h files (with the exception of vector.h) and made comments based on questions I'd have as a user of the API based on the documentation provided. I imagine that my next pass, which will be looking at the .c files, will answer some of my questions. I figure that posting these can help to solidify the API documentation though and lets people avoid having to look at the implementation to figure out what's going on.

As far as usability of the API is concerned, it seems pretty similar to the old format APIs, so adjusting wouldn't be too difficult. Probably the biggest difference is the immutability requirements on ast_format structures. That may need some further explanation, like what is specifically unsafe to do with an "immutable" format.

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

    So since we're in the early stages of this, I'm going to request that documentation for these fields be more specific.

    sample_rate: What are the units for this? Is this samples per second or something else?

    minimum_ms and maximum_ms: Is this the maximum number of milliseconds per sample or for some other time slice?

    increment_ms: I'm completely in the dark on what this means just from the description. The length of what, exactly? Under what circumstances are we talking about increasing the number of milliseconds of something?

    default_ms: When you say "length of media" is that the length per sample?

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

    I suggest calling this "num_samples" or "samples_count" or something else since "get_samples" sounds like it would actually be used to get media samples as opposed to a count.

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

    What are the units for this length?

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

    Similar suggestion as the ast_codec get_samples() callback. Rename this to better reflect that you're retrieving a count of samples as opposed to retrieving actual samples.

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

    Does "ng" stand for "next generation"? If this ends up replacing format_cap.h, would the "ng" be dropped from all these function names?

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

    Does this framing value override the global framing value of the cap?

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

    Semantically, is this a copy or a move operation? Is src still usable after this operation?

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

    What is meant by "position"? Is that a zero-based index in the cap structure or is it something else?

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

    What is meant by an exact match? name, sample rate, and media type? Or are there more criteria?

    Or does an exact match mean a pointer address match?

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

    Since the add_all_bytype function allows for you to specify the "unknown" format type as a way of indicating to add all formats of all types, why not allow the same idiom on the remove_bytype() function and get rid of remove_all() altogether?

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

    Is it possible that there could be multiple compatible formats?

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

    Add [out] after \param to indicate that result is an output parameter.

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

    The use of "structure 1" and "structure 2" here makes it difficult to interpret the results when this type is returned from ast_format_cap_ng_iscompatible_format() since it's unclear whether the passed-in format is structure 1 or 2.

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

    I don't really understand the \brief explanation. When you say "same format type", does that mean that format1 and format2 have to be the same format type? And what does "same format type" mean? Does it mean that both formats are audio formats, or is it more specific than that?

    Also, there's red here.

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

    Is this essentially a more "raw" form of the format_attribute_set() callback?

    Is an entire fmtp SDP line expected or just the attributes from that line?

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

    Can this be enforced by making the return const?

    (Same question applies for other places where this note exists)

- 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/20140205/c9f3d774/attachment-0001.html>