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

[Joshua Colp]

> On Feb. 5, 2014, 9:52 p.m., Mark Michelson wrote:
> > /branches/12/include/asterisk/format_cap_ng.h, line 49
> > <https://reviewboard.asterisk.org/r/3178/diff/1/?file=53400#file53400line49>
> >
> >     Does "ng" stand for "next generation"? If this ends up replacing format_cap.h, would the "ng" be dropped from all these function names?

Yes.


> On Feb. 5, 2014, 9:52 p.m., Mark Michelson wrote:
> > /branches/12/include/asterisk/codec.h, lines 50-59
> > <https://reviewboard.asterisk.org/r/3178/diff/1/?file=53397#file53397line50>
> >
> >     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?

I've removed increment_ms. It was actually duplicating minimum_ms and didn't add any value, except confusion.


> On Feb. 5, 2014, 9:52 p.m., Mark Michelson wrote:
> > /branches/12/include/asterisk/format.h, lines 458-459
> > <https://reviewboard.asterisk.org/r/3178/diff/1/?file=53398#file53398line458>
> >
> >     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.


> On Feb. 5, 2014, 9:52 p.m., Mark Michelson wrote:
> > /branches/12/include/asterisk/format_ng.h, lines 135-137
> > <https://reviewboard.asterisk.org/r/3178/diff/1/?file=53401#file53401line135>
> >
> >     Can this be enforced by making the return const?
> >     
> >     (Same question applies for other places where this note exists)

It's possible but it would require wrapping ao2_ref and ao2_cleanup and casting the const away. I was uncertain whether I wanted to go down that path.


> On Feb. 5, 2014, 9:52 p.m., Mark Michelson wrote:
> > /branches/12/include/asterisk/format_cap_ng.h, lines 163-173
> > <https://reviewboard.asterisk.org/r/3178/diff/1/?file=53400#file53400line163>
> >
> >     Is it possible that there could be multiple compatible formats?

The answer is... yes. When attributes are in play you may have multiple formats of same codec with different attributes, and one may be an exact match while others may be a subset. Right now the code is written to only immediately return if there's an exact match. If subsets exist then only one of them will be returned.


- Joshua


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


On Feb. 7, 2014, 2:59 p.m., Joshua Colp wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3178/
> -----------------------------------------------------------
> 
> (Updated Feb. 7, 2014, 2:59 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 407438 
>   /branches/12/res/res_fax.c 407438 
>   /branches/12/main/smoother.c PRE-CREATION 
>   /branches/12/main/frame.c 407438 
>   /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 407438 
>   /branches/12/main/codec_builtin.c PRE-CREATION 
>   /branches/12/main/codec.c PRE-CREATION 
>   /branches/12/main/asterisk.c 407438 
>   /branches/12/include/asterisk/vector.h 407438 
>   /branches/12/include/asterisk/smoother.h PRE-CREATION 
>   /branches/12/include/asterisk/frame.h 407438 
>   /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 407438 
>   /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/20140207/634d3040/attachment-0001.html>