[asterisk-dev] [Code Review] 2692: chan_pjsip: T.38 Fax Support

[Mark Michelson]

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



/trunk/include/asterisk/res_sip_session.h
<https://reviewboard.asterisk.org/r/2692/#comment18172>

    This could use a better explanation than the brief description given. What does it mean to "defer"  a re-invited stream? What is this callback expected to do?



/trunk/include/asterisk/res_sip_session.h
<https://reviewboard.asterisk.org/r/2692/#comment18171>

    This comment could use some clarification. "yet again" is not indicative of where the defer_incoming_sdp_stream callback may originally be called.



/trunk/res/res_sip_session.c
<https://reviewboard.asterisk.org/r/2692/#comment18174>

    The return values of this function need to be documented.



/trunk/res/res_sip_session.c
<https://reviewboard.asterisk.org/r/2692/#comment18175>

    Why do we return if res = 0 here?



/trunk/res/res_sip_session.c
<https://reviewboard.asterisk.org/r/2692/#comment18176>

    Hm, seeing this really makes me think the distributor should be caching the dialog on the rdata since the distributor has already had to look up the dialog before the request reaches this point.
    
    Not opening an issue since that's outside the scope of FAX support, but I'm noting it anyway.



/trunk/res/res_sip_session.c
<https://reviewboard.asterisk.org/r/2692/#comment18177>

    I'm puzzled about the handling of the return value of handle_deferring_sdp() here. My interpretation of handle_deferring_sdp() was that a zero return was successful. Is a successful return here supposed to result in the request bubbling up the stack further? It seems a bit backwards.



/trunk/res/res_sip_t38.c
<https://reviewboard.asterisk.org/r/2692/#comment18180>

    Also return early if session_media is NULL.



/trunk/res/res_sip_t38.c
<https://reviewboard.asterisk.org/r/2692/#comment18181>

    Are you supposed to ast_frfree(f) before setting it to a new value?


- Mark Michelson


On July 25, 2013, 4:34 p.m., Joshua Colp wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2692/
> -----------------------------------------------------------
> 
> (Updated July 25, 2013, 4:34 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This change adds T.38 support to chan_pjsip. A seperate optional module has been created which contains the T.38 state machine, SDP parsing/generation, and UDPTL sending/receiving.
> 
> 
> Diffs
> -----
> 
>   /trunk/channels/chan_gulp.c 395397 
>   /trunk/include/asterisk/res_sip.h 395397 
>   /trunk/include/asterisk/res_sip_session.h 395397 
>   /trunk/res/res_sip.c 395397 
>   /trunk/res/res_sip/sip_configuration.c 395397 
>   /trunk/res/res_sip_session.c 395397 
>   /trunk/res/res_sip_session.exports.in 395397 
>   /trunk/res/res_sip_t38.c PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/2692/diff/
> 
> 
> Testing
> -------
> 
> Tested sending and receiving using Zoiper.
> 
> 
> Thanks,
> 
> Joshua Colp
> 
>

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