[asterisk-dev] [Code Review] Expand availability of codec bits from 32 to 64

Mark Michelson mmichelson at digium.com
Tue Nov 3 19:15:32 CST 2009 

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


In general, there are some good things done here, and there are some things I disagree with.

I like the fact that the frame subclass was made into a union so that it is more clear what its purpose is.
I like that the format has been placed into a type called format_t in an attempt to provide some abstraction.

Now, what I don't like is the fact that the format_t is not very well abstracted; All users of format_t know and exploit the fact that the format_t is an integral type and is a bitmask. If we should desire to change the format_t into a struct or non-integral type, an equal amount of work would be required to make that change down the road. I would have liked to see an API developed which would allow for users of formats to not have to know or care about the underlying type of the format_t. This would allow future changes to be more centralized.

Of course, the concerns I express here are based on my desire to have good abstraction. As far as the actual code changes go, I didn't see anything immediately alarming in the time I spent looking. Although I will admit that I did not get around to reviewing every file in great detail.

- Mark


On 2009-11-03 17:46:03, Tilghman Lesher wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/416/
> -----------------------------------------------------------
> 
> (Updated 2009-11-03 17:46:03)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> Since the addition of SIREN7 and SIREN14 codecs, there are 0 audio codec bits left in which to allocate more codecs.  This implementation adds an additional 16 audio bits.
> 
> 
> Diffs
> -----
> 
>   /trunk/apps/app_alarmreceiver.c 227508 
>   /trunk/apps/app_amd.c 227508 
>   /trunk/apps/app_chanspy.c 227508 
>   /trunk/apps/app_dahdibarge.c 227508 
>   /trunk/apps/app_dial.c 227508 
>   /trunk/apps/app_dictate.c 227508 
>   /trunk/apps/app_disa.c 227508 
>   /trunk/apps/app_echo.c 227508 
>   /trunk/apps/app_externalivr.c 227508 
>   /trunk/apps/app_fax.c 227508 
>   /trunk/apps/app_festival.c 227508 
>   /trunk/apps/app_followme.c 227508 
>   /trunk/apps/app_jack.c 227508 
>   /trunk/apps/app_meetme.c 227508 
>   /trunk/apps/app_milliwatt.c 227508 
>   /trunk/apps/app_mp3.c 227508 
>   /trunk/apps/app_nbscat.c 227508 
>   /trunk/apps/app_queue.c 227508 
>   /trunk/apps/app_record.c 227508 
>   /trunk/apps/app_sms.c 227508 
>   /trunk/apps/app_speech_utils.c 227508 
>   /trunk/apps/app_talkdetect.c 227508 
>   /trunk/apps/app_test.c 227508 
>   /trunk/apps/app_url.c 227508 
>   /trunk/apps/app_waitforring.c 227508 
>   /trunk/bridges/bridge_softmix.c 227508 
>   /trunk/channels/chan_agent.c 227508 
>   /trunk/channels/chan_alsa.c 227508 
>   /trunk/channels/chan_bridge.c 227508 
>   /trunk/channels/chan_console.c 227508 
>   /trunk/channels/chan_dahdi.c 227508 
>   /trunk/channels/chan_gtalk.c 227508 
>   /trunk/channels/chan_h323.c 227508 
>   /trunk/channels/chan_iax2.c 227508 
>   /trunk/channels/chan_jingle.c 227508 
>   /trunk/channels/chan_local.c 227508 
>   /trunk/channels/chan_mgcp.c 227508 
>   /trunk/channels/chan_misdn.c 227508 
>   /trunk/channels/chan_multicast_rtp.c 227508 
>   /trunk/channels/chan_oss.c 227508 
>   /trunk/channels/chan_phone.c 227508 
>   /trunk/channels/chan_sip.c 227508 
>   /trunk/channels/chan_skinny.c 227508 
>   /trunk/channels/chan_unistim.c 227508 
>   /trunk/channels/chan_vpb.cc 227508 
>   /trunk/channels/h323/chan_h323.h 227508 
>   /trunk/channels/iax2-parser.h 227508 
>   /trunk/channels/iax2-parser.c 227508 
>   /trunk/channels/iax2.h 227508 
>   /trunk/channels/sig_analog.c 227508 
>   /trunk/channels/sig_pri.c 227508 
>   /trunk/codecs/codec_dahdi.c 227508 
>   /trunk/codecs/codec_ulaw.c 227508 
>   /trunk/codecs/ex_adpcm.h 227508 
>   /trunk/codecs/ex_alaw.h 227508 
>   /trunk/codecs/ex_g722.h 227508 
>   /trunk/codecs/ex_g726.h 227508 
>   /trunk/codecs/ex_gsm.h 227508 
>   /trunk/codecs/ex_ilbc.h 227508 
>   /trunk/codecs/ex_lpc10.h 227508 
>   /trunk/codecs/ex_speex.h 227508 
>   /trunk/codecs/ex_ulaw.h 227508 
>   /trunk/configure UNKNOWN 
>   /trunk/configure.ac 227508 
>   /trunk/doc/codec-64bit.txt PRE-CREATION 
>   /trunk/formats/format_g723.c 227508 
>   /trunk/formats/format_g726.c 227508 
>   /trunk/formats/format_g729.c 227508 
>   /trunk/formats/format_gsm.c 227508 
>   /trunk/formats/format_h263.c 227508 
>   /trunk/formats/format_h264.c 227508 
>   /trunk/formats/format_ilbc.c 227508 
>   /trunk/formats/format_jpeg.c 227508 
>   /trunk/formats/format_ogg_vorbis.c 227508 
>   /trunk/formats/format_pcm.c 227508 
>   /trunk/formats/format_siren14.c 227508 
>   /trunk/formats/format_siren7.c 227508 
>   /trunk/formats/format_sln.c 227508 
>   /trunk/formats/format_sln16.c 227508 
>   /trunk/formats/format_vox.c 227508 
>   /trunk/formats/format_wav.c 227508 
>   /trunk/formats/format_wav_gsm.c 227508 
>   /trunk/funcs/func_volume.c 227508 
>   /trunk/include/asterisk/abstract_jb.h 227508 
>   /trunk/include/asterisk/audiohook.h 227508 
>   /trunk/include/asterisk/autoconfig.h.in 227508 
>   /trunk/include/asterisk/bridging.h 227508 
>   /trunk/include/asterisk/bridging_technology.h 227508 
>   /trunk/include/asterisk/channel.h 227508 
>   /trunk/include/asterisk/compat.h 227508 
>   /trunk/include/asterisk/frame.h 227508 
>   /trunk/include/asterisk/frame_defs.h PRE-CREATION 
>   /trunk/include/asterisk/pbx.h 227508 
>   /trunk/include/asterisk/rtp_engine.h 227508 
>   /trunk/include/asterisk/slin.h 227508 
>   /trunk/include/asterisk/slinfactory.h 227508 
>   /trunk/include/asterisk/translate.h 227508 
>   /trunk/include/asterisk/unaligned.h 227508 
>   /trunk/main/abstract_jb.c 227508 
>   /trunk/main/app.c 227508 
>   /trunk/main/asterisk.exports 227508 
>   /trunk/main/audiohook.c 227508 
>   /trunk/main/autoservice.c 227508 
>   /trunk/main/bridging.c 227508 
>   /trunk/main/channel.c 227508 
>   /trunk/main/dial.c 227508 
>   /trunk/main/dsp.c 227508 
>   /trunk/main/features.c 227508 
>   /trunk/main/file.c 227508 
>   /trunk/main/frame.c 227508 
>   /trunk/main/indications.c 227508 
>   /trunk/main/manager.c 227508 
>   /trunk/main/pbx.c 227508 
>   /trunk/main/rtp_engine.c 227508 
>   /trunk/main/slinfactory.c 227508 
>   /trunk/main/strcompat.c 227508 
>   /trunk/main/translate.c 227508 
>   /trunk/main/udptl.c 227508 
>   /trunk/pbx/pbx_spool.c 227508 
>   /trunk/res/res_adsi.c 227508 
>   /trunk/res/res_agi.c 227508 
>   /trunk/res/res_musiconhold.c 227508 
>   /trunk/res/res_rtp_asterisk.c 227508 
>   /trunk/res/res_rtp_multicast.c 227508 
> 
> Diff: https://reviewboard.asterisk.org/r/416/diff
> 
> 
> Testing
> -------
> 
> Works with all existing codecs.  Still working on getting IAX2 to pass voice with a codec in the extended range (testlaw, which is identical to ulaw).
> 
> 
> Thanks,
> 
> Tilghman
> 
>

More information about the asterisk-dev mailing list