[asterisk-dev] [Code Review] app_swift - streaming text-to-speech interface for the Cepstral Swift engine

[paul.belanger at polybeacon.com]

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


Good work, plan to give this a try.  Some formatting comments below.  Specifically, the 'red blobs' should be removed.


/trunk/apps/app_swift.c
<https://reviewboard.asterisk.org/r/691/#comment4552>

    Red blob like this should be removed.  They represent wasted white space.
    
    There are more examples in your code, but will not point them out this time around.



/trunk/apps/app_swift.c
<https://reviewboard.asterisk.org/r/691/#comment4553>

    Path should not be hardcoded.



/trunk/apps/app_swift.c
<https://reviewboard.asterisk.org/r/691/#comment4554>

    C++ comments (//) should be changed to /* */



/trunk/apps/app_swift.c
<https://reviewboard.asterisk.org/r/691/#comment4555>

    same as above.



/trunk/apps/app_swift.c
<https://reviewboard.asterisk.org/r/691/#comment4556>

    Would you not need to ASTOBJ_UNLOCK(ps) here?



/trunk/apps/app_swift.c
<https://reviewboard.asterisk.org/r/691/#comment4557>

    dtmf_search_counter += 1;



/trunk/apps/app_swift.c
<https://reviewboard.asterisk.org/r/691/#comment4558>

    i += 1;



/trunk/apps/app_swift.c
<https://reviewboard.asterisk.org/r/691/#comment4559>

    wouldn't break; be more appropriate?



/trunk/apps/app_swift.c
<https://reviewboard.asterisk.org/r/691/#comment4562>

    It seems you are defaulting to ulaw, is there a reason?  What if the user had gsm, or alaw? Does the engine support SLN?



/trunk/apps/app_swift.c
<https://reviewboard.asterisk.org/r/691/#comment4560>

    /* */ comments, or to be removed?



/trunk/apps/app_swift.c
<https://reviewboard.asterisk.org/r/691/#comment4561>

    same as above.



/trunk/apps/app_swift.c
<https://reviewboard.asterisk.org/r/691/#comment4563>

    space before and after /



/trunk/apps/app_swift.c
<https://reviewboard.asterisk.org/r/691/#comment4564>

    Same as above.



/trunk/apps/app_swift.c
<https://reviewboard.asterisk.org/r/691/#comment4565>

    You should be able to change this code to:
    
         }
    ast_ffree(f);



/trunk/apps/app_swift.c
<https://reviewboard.asterisk.org/r/691/#comment4566>

    Seems unnecessary.



/trunk/apps/app_swift.c
<https://reviewboard.asterisk.org/r/691/#comment4567>

    Same as above.



/trunk/apps/app_swift.c
<https://reviewboard.asterisk.org/r/691/#comment4569>

    ast_free()



/trunk/apps/app_swift.c
<https://reviewboard.asterisk.org/r/691/#comment4568>

    Same as above


- pabelanger


On 2010-06-03 12:34:47, Darren Sessions wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/691/
> -----------------------------------------------------------
> 
> (Updated 2010-06-03 12:34:47)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> app_swift uses the Cepstral text-to-speech engine and copies audio frames straight from the Cepstral libs to the call channel which avoids the need to write the generated audio to a file first and then read it. Additionally, DTMF recognition code was integrated into the module to prevent delays in previous usage scenarios where the app was called and dtmf listened for afterward.
> 
> 
> Diffs
> -----
> 
>   /trunk/apps/app_swift.c PRE-CREATION 
>   /trunk/autoconf/ast_ext_lib.m4 266734 
>   /trunk/build_tools/menuselect-deps.in 266734 
>   /trunk/configs/swift.conf.sample PRE-CREATION 
>   /trunk/configure.ac 266828 
>   /trunk/include/asterisk/autoconfig.h.in 266828 
>   /trunk/makeopts.in 266734 
> 
> Diff: https://reviewboard.asterisk.org/r/691/diff
> 
> 
> Testing
> -------
> 
> I've built against trunk successfully now (thanks to Tilghman for getting the build tweaks squared away) without any errors and made dozens of test calls using different usage scenarios.
> 
> 
> Thanks,
> 
> Darren
> 
>