[asterisk-dev] [Code Review] Asterisk media architecture conversion - no more format bitfields

David Vossel reviewboard at asterisk.org
Mon Jan 31 11:44:48 CST 2011 


> On 2011-01-29 11:09:20, Terry Wilson wrote:
> > /trunk/channels/chan_gtalk.c, line 1046
> > <https://reviewboard.asterisk.org/r/1083/diff/3/?file=15322#file15322line1046>
> >
> >     With all of the other format API having copy functions, do you think that it would be more consistent to hide this behind an API in case the underlying structure changes to include a pointer for some reason?

Perhaps in the future.  I did not write this particular API, I only isolated it from frame.c.


> On 2011-01-29 11:09:20, Terry Wilson wrote:
> > /trunk/channels/chan_gtalk.c, line 2165
> > <https://reviewboard.asterisk.org/r/1083/diff/3/?file=15322#file15322line2165>
> >
> >     Like the copy comment above, should we hide this behind an API call? It looks like format_pref.h actually declares and documents ast_format_pref_init, but it isn't actually defined anywhere. Pretty easy to find most of the mem(set|cpy) instances with `grep -nIr "mem\w\{3\}\s*(.*pref" *|grep -v svn`

Since this is not a new err... "problem" if you can call it that and is not introduced by this patch, I'd prefer addressing this in a later patch if we decide to address it.  Fixing this right now doesn't seem like a good ROI to me.


> On 2011-01-29 11:09:20, Terry Wilson wrote:
> > /trunk/channels/chan_h323.c, line 1051
> > <https://reviewboard.asterisk.org/r/1083/diff/3/?file=15323#file15323line1051>
> >
> >     The commented out code would no longer work. :-p

This channel driver needs to die.


> On 2011-01-29 11:09:20, Terry Wilson wrote:
> > /trunk/channels/chan_h323.c, lines 2058-2064
> > <https://reviewboard.asterisk.org/r/1083/diff/3/?file=15323#file15323line2058>
> >
> >     the if(a) { if (b) {}} could be simplified to if (a && b). Did you mean to nix the debug statement as well?

Yep, I killed the debug.  This could be simplified, but I have no desire to touch anything in this channel driver unless it doesn't work.


> On 2011-01-29 11:09:20, Terry Wilson wrote:
> > /trunk/channels/chan_iax2.c, lines 1708-1709
> > <https://reviewboard.asterisk.org/r/1083/diff/3/?file=15324#file15324line1708>
> >
> >     newline
> >     return
> >     }
> >     instead of
> >     return
> >     newline
> >     }

fixed


> On 2011-01-29 11:09:20, Terry Wilson wrote:
> > /trunk/channels/chan_jingle.c, lines 1220-1221
> > <https://reviewboard.asterisk.org/r/1083/diff/3/?file=15325#file15325line1220>
> >
> >     I'm not sure about this one. It looks like the original code specifically tries to keep any video nativeformats, whereas the new version removes all nativeformats and replaces them with the format of the frame. Is this intentional?

oops, fixed.


> On 2011-01-29 11:09:20, Terry Wilson wrote:
> > /trunk/channels/chan_local.c, lines 1053-1054
> > <https://reviewboard.asterisk.org/r/1083/diff/3/?file=15326#file15326line1053>
> >
> >     The original code looks like it replaces nativeformats completely while the new code uses an append? Intentional?

not intentional.  This was before I had the copy function.  Since the capabilities structure is empty at this point it is the same as a copy, but the fact that I use "append" is really confusing. 

fixed.


> On 2011-01-29 11:09:20, Terry Wilson wrote:
> > /trunk/channels/chan_mgcp.c, line 2244
> > <https://reviewboard.asterisk.org/r/1083/diff/3/?file=15327#file15327line2244>
> >
> >     extra newline

fixed


> On 2011-01-29 11:09:20, Terry Wilson wrote:
> > /trunk/channels/chan_oss.c, lines 803-804
> > <https://reviewboard.asterisk.org/r/1083/diff/3/?file=15331#file15331line803>
> >
> >     C++-style comments.

fixed


> On 2011-01-29 11:09:20, Terry Wilson wrote:
> > /trunk/channels/chan_phone.c, lines 213-215
> > <https://reviewboard.asterisk.org/r/1083/diff/3/?file=15332#file15332line213>
> >
> >     weird spacing (though so was the original)

fixed


> On 2011-01-29 11:09:20, Terry Wilson wrote:
> > /trunk/channels/chan_phone.c, lines 1389-1395
> > <https://reviewboard.asterisk.org/r/1083/diff/3/?file=15332#file15332line1389>
> >
> >     If we fail, here, we don't destroy the prior capabilities allocs.

I didn't address this because Asterisk will not load if this fails.


> On 2011-01-29 11:09:20, Terry Wilson wrote:
> > /trunk/channels/chan_sip.c, lines 6034-6036
> > <https://reviewboard.asterisk.org/r/1083/diff/3/?file=15333#file15333line6034>
> >
> >     Personally, I tend to like initializing like this. You tend to use ast_format_clear() everywhere else I've seen so far, though.

Ah, this is left over from before I started using ast_format_clear().  I like this way as well, but if I ever change the internals of ast_format, thats could result in a crazy amount of code to be altered.  That's why I chose to use the clear() function.  I'll change this to use it as well to be consistent. 


> On 2011-01-29 11:09:20, Terry Wilson wrote:
> > /trunk/channels/chan_skinny.c, line 4685
> > <https://reviewboard.asterisk.org/r/1083/diff/3/?file=15334#file15334line4685>
> >
> >     I think this should be if (!ast_format_cap_is_empty(tmp->nativeformats)

it should be if (ast_format_cap_is_empty(tmp->nativeformats))

fixed


> On 2011-01-29 11:09:20, Terry Wilson wrote:
> > /trunk/channels/chan_unistim.c, lines 1488-1493
> > <https://reviewboard.asterisk.org/r/1083/diff/3/?file=15335#file15335line1488>
> >
> >     A few things here. unistim_line_copy goes against the normal copy function argument order of (dst, src). This can sometimes lead to confusion/bugs. For instance, the memcpy in this function looks backward. I'm having a little trouble figuring out what should be going on. If the memcpy is backward, then the tmp=src->cap, src->cap = tmp is redundant. In any case, I wonder if the cap should be dup'd instead of just copying over the link.

There were a few problems here. I fixed them and reversed the function's arguments.


> On 2011-01-29 11:09:20, Terry Wilson wrote:
> > /trunk/channels/chan_unistim.c, lines 5676-5688
> > <https://reviewboard.asterisk.org/r/1083/diff/3/?file=15335#file15335line5676>
> >
> >     buff_failed: doesn't free the caps.

fixed


> On 2011-01-29 11:09:20, Terry Wilson wrote:
> > /trunk/channels/chan_vpb.cc, line 349
> > <https://reviewboard.asterisk.org/r/1083/diff/3/?file=15337#file15337line349>
> >
> >     Everywhere else you have just deleted the entry.

The compiler freaked out if I removed this initialization.


> On 2011-01-29 11:09:20, Terry Wilson wrote:
> > /trunk/channels/chan_vpb.cc, line 383
> > <https://reviewboard.asterisk.org/r/1083/diff/3/?file=15337#file15337line383>
> >
> >     here too.

same as above


> On 2011-01-29 11:09:20, Terry Wilson wrote:
> > /trunk/channels/chan_sip.c, line 16824
> > <https://reviewboard.asterisk.org/r/1083/diff/3/?file=15333#file15333line16824>
> >
> >     Is a hardcoded 63 here what we really want?

Nope, Fixed.


> On 2011-01-29 11:09:20, Terry Wilson wrote:
> > /trunk/channels/chan_h323.c, lines 784-785
> > <https://reviewboard.asterisk.org/r/1083/diff/3/?file=15323#file15323line784>
> >
> >     I think there are about 23 calls to ast_format_cap_remove_all() followed immediately by a call to ast_format_cap_add() in various parts of the code. It might be useful to add a ast_format_cap_set (set_format? replace?) for what seems like a pretty common case.

Good idea. I'll do this.


- David


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


On 2011-01-24 15:25:45, David Vossel wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1083/
> -----------------------------------------------------------
> 
> (Updated 2011-01-24 15:25:45)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This patch is the foundation of an entire new way of looking at media in Asterisk.  The code present in this review is everything required to complete phase1 of my Media Architecture proposal.
> 
> For more information about this project visit the link below.
> https://wiki.asterisk.org/wiki/display/AST/Media+Architecture+Proposal
> 
> 
> Diffs
> -----
> 
>   /trunk/addons/chan_mobile.c 303557 
>   /trunk/addons/chan_ooh323.h 303557 
>   /trunk/addons/chan_ooh323.c 303557 
>   /trunk/addons/format_mp3.c 303557 
>   /trunk/addons/ooh323cDriver.h 303557 
>   /trunk/addons/ooh323cDriver.c 303557 
>   /trunk/apps/app_alarmreceiver.c 303557 
>   /trunk/apps/app_amd.c 303557 
>   /trunk/apps/app_chanspy.c 303557 
>   /trunk/apps/app_confbridge.c 303557 
>   /trunk/apps/app_dahdibarge.c 303557 
>   /trunk/apps/app_dictate.c 303557 
>   /trunk/apps/app_dumpchan.c 303557 
>   /trunk/apps/app_echo.c 303557 
>   /trunk/apps/app_fax.c 303557 
>   /trunk/apps/app_festival.c 303557 
>   /trunk/apps/app_followme.c 303557 
>   /trunk/apps/app_ices.c 303557 
>   /trunk/apps/app_jack.c 303557 
>   /trunk/apps/app_meetme.c 303557 
>   /trunk/apps/app_milliwatt.c 303557 
>   /trunk/apps/app_mixmonitor.c 303557 
>   /trunk/apps/app_mp3.c 303557 
>   /trunk/apps/app_nbscat.c 303557 
>   /trunk/apps/app_originate.c 303557 
>   /trunk/apps/app_parkandannounce.c 303557 
>   /trunk/apps/app_record.c 303557 
>   /trunk/apps/app_rpt.c 303557 
>   /trunk/apps/app_sms.c 303557 
>   /trunk/apps/app_speech_utils.c 303557 
>   /trunk/apps/app_talkdetect.c 303557 
>   /trunk/apps/app_test.c 303557 
>   /trunk/apps/app_voicemail.c 303557 
>   /trunk/apps/app_waitforsilence.c 303557 
>   /trunk/bridges/bridge_multiplexed.c 303557 
>   /trunk/bridges/bridge_simple.c 303557 
>   /trunk/bridges/bridge_softmix.c 303557 
>   /trunk/channels/chan_agent.c 303557 
>   /trunk/channels/chan_alsa.c 303557 
>   /trunk/channels/chan_bridge.c 303557 
>   /trunk/channels/chan_console.c 303557 
>   /trunk/channels/chan_dahdi.c 303557 
>   /trunk/channels/chan_gtalk.c 303557 
>   /trunk/channels/chan_h323.c 303557 
>   /trunk/channels/chan_iax2.c 303557 
>   /trunk/channels/chan_jingle.c 303557 
>   /trunk/channels/chan_local.c 303557 
>   /trunk/channels/chan_mgcp.c 303557 
>   /trunk/channels/chan_misdn.c 303557 
>   /trunk/channels/chan_multicast_rtp.c 303557 
>   /trunk/channels/chan_nbs.c 303557 
>   /trunk/channels/chan_oss.c 303557 
>   /trunk/channels/chan_phone.c 303557 
>   /trunk/channels/chan_sip.c 303557 
>   /trunk/channels/chan_skinny.c 303557 
>   /trunk/channels/chan_unistim.c 303557 
>   /trunk/channels/chan_usbradio.c 303557 
>   /trunk/channels/chan_vpb.cc 303557 
>   /trunk/channels/h323/ast_h323.cxx 303557 
>   /trunk/channels/h323/chan_h323.h 303557 
>   /trunk/channels/iax2-parser.h 303557 
>   /trunk/channels/iax2-parser.c 303557 
>   /trunk/channels/iax2-provision.c 303557 
>   /trunk/channels/iax2.h 303557 
>   /trunk/channels/sip/include/globals.h 303557 
>   /trunk/channels/sip/include/sip.h 303557 
>   /trunk/codecs/codec_a_mu.c 303557 
>   /trunk/codecs/codec_adpcm.c 303557 
>   /trunk/codecs/codec_alaw.c 303557 
>   /trunk/codecs/codec_dahdi.c 303557 
>   /trunk/codecs/codec_g722.c 303557 
>   /trunk/codecs/codec_g726.c 303557 
>   /trunk/codecs/codec_gsm.c 303557 
>   /trunk/codecs/codec_ilbc.c 303557 
>   /trunk/codecs/codec_lpc10.c 303557 
>   /trunk/codecs/codec_resample.c 303557 
>   /trunk/codecs/codec_speex.c 303557 
>   /trunk/codecs/codec_ulaw.c 303557 
>   /trunk/codecs/ex_adpcm.h 303557 
>   /trunk/codecs/ex_alaw.h 303557 
>   /trunk/codecs/ex_g722.h 303557 
>   /trunk/codecs/ex_g726.h 303557 
>   /trunk/codecs/ex_gsm.h 303557 
>   /trunk/codecs/ex_lpc10.h 303557 
>   /trunk/codecs/ex_speex.h 303557 
>   /trunk/codecs/ex_ulaw.h 303557 
>   /trunk/formats/format_g719.c 303557 
>   /trunk/formats/format_g723.c 303557 
>   /trunk/formats/format_g726.c 303557 
>   /trunk/formats/format_g729.c 303557 
>   /trunk/formats/format_gsm.c 303557 
>   /trunk/formats/format_h263.c 303557 
>   /trunk/formats/format_h264.c 303557 
>   /trunk/formats/format_ilbc.c 303557 
>   /trunk/formats/format_jpeg.c 303557 
>   /trunk/formats/format_ogg_vorbis.c 303557 
>   /trunk/formats/format_pcm.c 303557 
>   /trunk/formats/format_siren14.c 303557 
>   /trunk/formats/format_siren7.c 303557 
>   /trunk/formats/format_sln.c 303557 
>   /trunk/formats/format_sln16.c 303557 
>   /trunk/formats/format_vox.c 303557 
>   /trunk/formats/format_wav.c 303557 
>   /trunk/formats/format_wav_gsm.c 303557 
>   /trunk/funcs/func_channel.c 303557 
>   /trunk/funcs/func_frame_trace.c 303557 
>   /trunk/funcs/func_pitchshift.c 303557 
>   /trunk/include/asterisk/abstract_jb.h 303557 
>   /trunk/include/asterisk/astobj2.h 303557 
>   /trunk/include/asterisk/audiohook.h 303557 
>   /trunk/include/asterisk/bridging.h 303557 
>   /trunk/include/asterisk/bridging_technology.h 303557 
>   /trunk/include/asterisk/callerid.h 303557 
>   /trunk/include/asterisk/channel.h 303557 
>   /trunk/include/asterisk/data.h 303557 
>   /trunk/include/asterisk/file.h 303557 
>   /trunk/include/asterisk/format.h PRE-CREATION 
>   /trunk/include/asterisk/format_cap.h PRE-CREATION 
>   /trunk/include/asterisk/format_pref.h PRE-CREATION 
>   /trunk/include/asterisk/frame.h 303557 
>   /trunk/include/asterisk/frame_defs.h 303557 
>   /trunk/include/asterisk/image.h 303557 
>   /trunk/include/asterisk/mod_format.h 303557 
>   /trunk/include/asterisk/pbx.h 303557 
>   /trunk/include/asterisk/rtp_engine.h 303557 
>   /trunk/include/asterisk/slin.h 303557 
>   /trunk/include/asterisk/slinfactory.h 303557 
>   /trunk/include/asterisk/speech.h 303557 
>   /trunk/include/asterisk/translate.h 303557 
>   /trunk/main/abstract_jb.c 303557 
>   /trunk/main/app.c 303557 
>   /trunk/main/asterisk.c 303557 
>   /trunk/main/astobj2.c 303557 
>   /trunk/main/audiohook.c 303557 
>   /trunk/main/bridging.c 303557 
>   /trunk/main/callerid.c 303557 
>   /trunk/main/ccss.c 303557 
>   /trunk/main/channel.c 303557 
>   /trunk/main/cli.c 303557 
>   /trunk/main/data.c 303557 
>   /trunk/main/dial.c 303557 
>   /trunk/main/dsp.c 303557 
>   /trunk/main/features.c 303557 
>   /trunk/main/file.c 303557 
>   /trunk/main/format.c PRE-CREATION 
>   /trunk/main/format_cap.c PRE-CREATION 
>   /trunk/main/format_pref.c PRE-CREATION 
>   /trunk/main/frame.c 303557 
>   /trunk/main/image.c 303557 
>   /trunk/main/indications.c 303557 
>   /trunk/main/manager.c 303557 
>   /trunk/main/pbx.c 303557 
>   /trunk/main/rtp_engine.c 303557 
>   /trunk/main/slinfactory.c 303557 
>   /trunk/main/translate.c 303557 
>   /trunk/main/udptl.c 303557 
>   /trunk/pbx/pbx_spool.c 303557 
>   /trunk/res/res_adsi.c 303557 
>   /trunk/res/res_agi.c 303557 
>   /trunk/res/res_calendar.c 303557 
>   /trunk/res/res_clioriginate.c 303557 
>   /trunk/res/res_fax.c 303557 
>   /trunk/res/res_fax_spandsp.c 303557 
>   /trunk/res/res_musiconhold.c 303557 
>   /trunk/res/res_rtp_asterisk.c 303557 
>   /trunk/res/res_rtp_multicast.c 303557 
>   /trunk/res/res_speech.c 303557 
>   /trunk/tests/test_format_api.c PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/1083/diff
> 
> 
> Testing
> -------
> 
> Below are the major areas I tested during development.  I will continue testing as this patch is being reviewed.
> -Local Channel + IAX2 channel load testing
> -SIP Calls with and without video
> -IAX2 Calls
> -AudioHooks and apps using audiohooks
> -Masquerades
> -DTMF Attended Transfers
> -SIP Transfers
> -Gtalk
> 
> 
> Thanks,
> 
> David
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20110131/db1fbc51/attachment-0001.htm>

More information about the asterisk-dev mailing list