[asterisk-dev] [Code Review] Add basic (passthrough, playback, record) support for ITU G.722.1 and G.722.1C (also known as Siren7 and Siren14)

[Kevin Fleming]

> On 2009-02-12 16:40:35, Russell Bryant wrote:
> > /trunk/formats/format_siren14.c, lines 128-129
> > <http://reviewboard.digium.com/r/158/diff/2/?file=2644#file2644line128>
> >
> >     Most modules return AST_MODULE_LOAD_DECLINE on a failure to register a handler.

Will fix.


> On 2009-02-12 16:40:35, Russell Bryant wrote:
> > /trunk/formats/format_siren7.c, lines 35-36
> > <http://reviewboard.digium.com/r/158/diff/2/?file=2645#file2645line35>
> >
> >     The same macros in your other format module had more magic to them.  Should they be the same?

No; that magic was required there because the samples-to-bytes ratio is not an integer, so the math must be done as floating point then cast back to an integer.


> On 2009-02-12 16:40:35, Russell Bryant wrote:
> > /trunk/include/asterisk/rtp.h, lines 229-230
> > <http://reviewboard.digium.com/r/158/diff/2/?file=2647#file2647line229>
> >
> >     The special tag is actually "\return", not "\returns".
> >     
> >     Also, in a case like this when you're documenting individual return values, there is a nice doxygen tag for that, as well:
> >     
> >     \retval 0 success
> >     \retval -1 the payload type is out of range
> >     \retval -2 the mimeType/mimeSubtype combination was not found

Burned by copy-and-paste again.


> On 2009-02-12 16:40:35, Russell Bryant wrote:
> > /trunk/formats/format_siren14.c, line 84
> > <http://reviewboard.digium.com/r/158/diff/2/?file=2644#file2644line84>
> >
> >     Should you check for an error here?

Probably, but all of this code was copied straight from format_sln16.c, so I didn't pay a lot of attention to it.


- Kevin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/158/#review407
-----------------------------------------------------------


On 2009-02-12 17:28:57, Kevin Fleming wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/158/
> -----------------------------------------------------------
> 
> (Updated 2009-02-12 17:28:57)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This patch adds passthrough, file recording and file playback support for the codecs listed above, with negotiation over SIP/SDP supported. Due to Asterisk's current limitation of treating a codec/bitrate combination as a unique codec, only G.722.1 at 32 kbps and G.722.1C at 48 kbps are supported.
> 
> Along the way, some related work was done:
> 
> 1) The rtpPayloadType structure definition, used as a return result for an API call in rtp.h, was moved from rtp.c to rtp.h so that the API call was actually usable. The only previous used of the API all was chan_h323.c, which had a duplicate of the structure definition instead of doing it the right way.
> 
> 2) The hardcoded SDP sample rates for various codecs in chan_sip.c were removed, in favor of storing these sample rates in rtp.c along with the codec definitions there. A new API call was added to allow retrieval of the sample rate for a given codec.
> 
> 3) Some basic 'a=fmtp' parsing for SDP was added to chan_sip, because chan_sip *must* decline any media streams offered for these codecs that are not at the bitrates that we support (otherwise Bad Things (TM) would result).
> 
> 
> Diffs
> -----
> 
>   /trunk/channels/chan_h323.c 175409 
>   /trunk/channels/chan_sip.c 175409 
>   /trunk/formats/format_siren14.c PRE-CREATION 
>   /trunk/formats/format_siren7.c PRE-CREATION 
>   /trunk/include/asterisk/frame.h 175409 
>   /trunk/include/asterisk/rtp.h 175409 
>   /trunk/main/frame.c 175409 
>   /trunk/main/rtp.c 175409 
> 
> Diff: http://reviewboard.digium.com/r/158/diff
> 
> 
> Testing
> -------
> 
> Compiled, installed, tested with Polycom IP6000 and IP7000 phones in Siren7 and Siren14 mode.
> 
> 
> Thanks,
> 
> Kevin
> 
>