[asterisk-dev] [Code Review] 3687: media format improvements: Update packetization handling; improve rtp_engine's ast_rtp_codecs handling

[Corey Farrell]

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



/team/group/media_formats-reviewed-trunk/main/rtp_engine.c
<https://reviewboard.asterisk.org/r/3687/#comment22623>

    I think no locking &codecs->codecs_lock from destroy.  If another thread is accessing the same codecs, this is going to crash under certain timing (even with the lock).  Locking here only makes such issues very difficult to reproduce.



/team/group/media_formats-reviewed-trunk/main/rtp_engine.c
<https://reviewboard.asterisk.org/r/3687/#comment22622>

    Do we need the temp variable or can we just ao2_cleanup(AST_VECTOR_GET())?
    
    Also is it safe to cleanup the vector items during iteration without removing them?  I'm not sure this is an issue when we are about to free, but wanted to mention anyhow.



/team/group/media_formats-reviewed-trunk/main/rtp_engine.c
<https://reviewboard.asterisk.org/r/3687/#comment22644>

    Since we're changing ABI's anyhow, I think this function should return an error code.  If we have such extreme memory shortage then the best thing to do is fail, and tell the caller that it should fail too.



/team/group/media_formats-reviewed-trunk/main/rtp_engine.c
<https://reviewboard.asterisk.org/r/3687/#comment22643>

    We need to remove from the vector.  Also why not ao2_cleanup?



/team/group/media_formats-reviewed-trunk/main/rtp_engine.c
<https://reviewboard.asterisk.org/r/3687/#comment22642>

    We need to remove from the vector.  Also why not ao2_cleanup?



/team/group/media_formats-reviewed-trunk/main/rtp_engine.c
<https://reviewboard.asterisk.org/r/3687/#comment22641>

    We need to remove from the vector.



/team/group/media_formats-reviewed-trunk/main/rtp_engine.c
<https://reviewboard.asterisk.org/r/3687/#comment22646>

    All code assumes that ast_rwlock_(rd|wr)lock(&codecs->codec_lock) will succeed.  Should we be checking the return code?  This question applies through-out rtp_engine.c.


- Corey Farrell


On July 1, 2014, 12:19 p.m., Matt Jordan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3687/
> -----------------------------------------------------------
> 
> (Updated July 1, 2014, 12:19 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This patch started out as an attempt to fix the BUGBUGs left over packetization calls into rtp_engine; it got a little bit bigger. Things now compile and work (see Testing), so this is a good place to stop before the renaming effort.
> 
> Primarily, this patch does the following:
> (1) Removes ast_rtp_codecs_packetization_set. This call was effectively a NoOp with res_rtp_asterisk/res_rtp_multicast. The various channel drivers now call ast_rtp_codecs_set_framing where appropriate.
> (2) A major overhaul of ast_rtp_codec was done. This includes:
>     (a) Storing the framing on the structure. This allows for the smoother in res_rtp_asterisk to easily get the framing specified without having to do major gyrations.
>     (b) Payload types (which are ao2 ref counted objects) are no longer stored in an ao2_container. This container had two patterns of usage: lookups by an integer key value and iteration. Vectors work well for this type of access and - for relatively small numbers of items (which is generally the case for payload types), are much faster on both counts.
> (3) The 'use_ptime' setting in res_pjsip_sdp_rtp now works. Packetization is also handled a little bit better, as both the RTP engine and format_cap API already do the job of managing the framing.
> 
> A variety of ref leaks were cleaned up as well along the way.
> 
> 
> Diffs
> -----
> 
>   /team/group/media_formats-reviewed-trunk/tests/test_format_cap.c 417724 
>   /team/group/media_formats-reviewed-trunk/res/res_speech.c 417724 
>   /team/group/media_formats-reviewed-trunk/res/res_rtp_asterisk.c 417724 
>   /team/group/media_formats-reviewed-trunk/res/res_pjsip_sdp_rtp.c 417724 
>   /team/group/media_formats-reviewed-trunk/res/res_fax.c 417724 
>   /team/group/media_formats-reviewed-trunk/main/rtp_engine.c 417724 
>   /team/group/media_formats-reviewed-trunk/main/format_cap.c 417724 
>   /team/group/media_formats-reviewed-trunk/main/format.c 417724 
>   /team/group/media_formats-reviewed-trunk/include/asterisk/vector.h 417724 
>   /team/group/media_formats-reviewed-trunk/include/asterisk/rtp_engine.h 417724 
>   /team/group/media_formats-reviewed-trunk/include/asterisk/frame.h 417724 
>   /team/group/media_formats-reviewed-trunk/include/asterisk/format_cap.h 417724 
>   /team/group/media_formats-reviewed-trunk/include/asterisk/format.h 417724 
>   /team/group/media_formats-reviewed-trunk/formats/format_h264.c 417724 
>   /team/group/media_formats-reviewed-trunk/formats/format_h263.c 417724 
>   /team/group/media_formats-reviewed-trunk/channels/chan_skinny.c 417724 
>   /team/group/media_formats-reviewed-trunk/channels/chan_sip.c 417724 
>   /team/group/media_formats-reviewed-trunk/channels/chan_motif.c 417724 
>   /team/group/media_formats-reviewed-trunk/channels/chan_jingle.c 417724 
>   /team/group/media_formats-reviewed-trunk/channels/chan_iax2.c 417724 
>   /team/group/media_formats-reviewed-trunk/channels/chan_h323.c 417724 
>   /team/group/media_formats-reviewed-trunk/channels/chan_gtalk.c 417724 
>   /team/group/media_formats-reviewed-trunk/bridges/bridge_softmix.c 417724 
>   /team/group/media_formats-reviewed-trunk/bridges/bridge_native_rtp.c 417724 
>   /team/group/media_formats-reviewed-trunk/addons/ooh323cDriver.c 417724 
>   /team/group/media_formats-reviewed-trunk/addons/chan_ooh323.c 417724 
> 
> Diff: https://reviewboard.asterisk.org/r/3687/diff/
> 
> 
> Testing
> -------
> 
> Back in February, I wrote a number of single audio stream tests for the PJSIP channel driver. Eventually these will get posted up for review, but the tests cover:
>  * Basic Offer/Answer of different sets of codecs (using a variety of patterns, including allow=all (ew))
>  * Packetization, including use_ptime=yes|no.
>  * AVPF
>  * Preferred codec only (by only specifying a single supported codec), subsets of offers, etc.
> 
> These tests will eventually get put up on another review, but they gave some confidence that the mucking around in the rtp_engine that is done on this patch works.
> 
> 
> Thanks,
> 
> Matt Jordan
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140701/f8df30e3/attachment-0001.html>