[asterisk-dev] [Code Review] Fax Gateway Implementation T30<->T38

[irroot]

> On 2011-05-11 15:44:58, Matthew Nicholson wrote:
> > /trunk/res/res_fax.c, line 2306
> > <https://reviewboard.asterisk.org/r/1116/diff/17/?file=16454#file16454line2306>
> >
> >     Where is this destroy_callback() function defined? I don't see it in this patch.

this is already used previously in the code.

static void destroy_callback(void *data)
{
        if (data) {
                ao2_ref(data, -1);
        }
}


> On 2011-05-11 15:44:58, Matthew Nicholson wrote:
> > /trunk/res/res_fax.c, line 2313
> > <https://reviewboard.asterisk.org/r/1116/diff/17/?file=16454#file16454line2313>
> >
> >     This function doesn't actually return the gateway datastore, perhaps it should be called find_session_gateway().

This has been done yip makes more sense.


> On 2011-05-11 15:44:58, Matthew Nicholson wrote:
> > /trunk/res/res_fax.c, lines 2333-2334
> > <https://reviewboard.asterisk.org/r/1116/diff/17/?file=16454#file16454line2333>
> >
> >     We need to add a ref for gateway here (as the comment says).

my bad did not take into account ao2_alloc returns with refcount held


> On 2011-05-11 15:44:58, Matthew Nicholson wrote:
> > /trunk/res/res_fax.c, line 2349
> > <https://reviewboard.asterisk.org/r/1116/diff/17/?file=16454#file16454line2349>
> >
> >     We don't need to ref gateway here if we allocated it on this function call.  Only ref if we are returning a reference we got form the datastore.

see above its incremented only once on creation or find


> On 2011-05-11 15:44:58, Matthew Nicholson wrote:
> > /trunk/res/res_fax.c, lines 3298-3299
> > <https://reviewboard.asterisk.org/r/1116/diff/17/?file=16454#file16454line3298>
> >
> >     The gateway should be removed from the channel here and shut down if it is running.

This has been done perhaps the audio formats need to be stored and restored at this point ??


- irroot


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


On 2011-05-13 05:36:52, irroot wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1116/
> -----------------------------------------------------------
> 
> (Updated 2011-05-13 05:36:52)
> 
> 
> Review request for Asterisk Developers, Russell Bryant, Matthew Nicholson, dimas, Leif Madsen, and dafe_von_cetin.
> 
> 
> Summary
> -------
> 
> Hi there the patch that was going around circa 2008 to implement this in 1.4/1.6 app_fax has been moved to trunk [1.10]
> ive made some cleanups and moved it into res_fax res_fax_spandsp this is the framework and not production code
> unfortunately i have no means of testing it at the moment and require help.
> 
> i have cleaned the code up substantially it is related to R459
> 
> hope this is found useful and aids in the goal to get it in 1.10.
> 
> Adds application FaxGateway / FaxDetect
> Adds alternate bridge to Dial with new option.
> 
> 
> This addresses bugs 13405, 19215 and 19251.
>     https://issues.asterisk.org/view.php?id=13405
>     https://issues.asterisk.org/view.php?id=19215
>     https://issues.asterisk.org/view.php?id=19251
> 
> 
> Diffs
> -----
> 
>   /trunk/channels/sip/include/sip.h 318833 
>   /trunk/include/asterisk/res_fax.h 318833 
>   /trunk/res/res_fax.c 318833 
>   /trunk/res/res_fax_spandsp.c 318833 
>   /trunk/channels/chan_sip.c 318833 
> 
> Diff: https://reviewboard.asterisk.org/r/1116/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> irroot
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20110513/9f72f943/attachment.htm>