[asterisk-dev] [Code Review] 2507: Add support for indexing installed sounds and for Stasis-HTTP sounds queries

[Matt Jordan]

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



/trunk/res/res_sounds.c
<https://reviewboard.asterisk.org/r/2507/#comment16939>

    This is a fairly costly operation. Imagine someone writing something like this in python:
    
    foreach (sound_file in sounds):
      if (language in asterisk.get_sound_languages()):
        ...
    
    What they don't know is that each call to 'get_sound_languages()' is iterating over the entire directory of sound files. That can be a pretty big hit.
    
    Since this is a costly operation, you should at a minimum cache the result of this after the first call. Alternatively, you could get all the language on module load and return that container.
    
    On a module reload, destroy the cached version and either reprocess on the next call, or rebuild the cached version.



/trunk/res/res_sounds.c
<https://reviewboard.asterisk.org/r/2507/#comment16940>

    I'm really not a big fan of combining allocation and search. If you're going to create things, create them. If you need to find things, find them. The two operations are not the same however.



/trunk/res/res_sounds.c
<https://reviewboard.asterisk.org/r/2507/#comment16941>

    Use ast_strlen_zero here



/trunk/res/res_sounds.c
<https://reviewboard.asterisk.org/r/2507/#comment16942>

    Use ast_strlen_zero here



/trunk/res/res_sounds.c
<https://reviewboard.asterisk.org/r/2507/#comment16944>

    If ferror is set, you should print that here



/trunk/res/res_sounds.c
<https://reviewboard.asterisk.org/r/2507/#comment16945>

    And here as well



/trunk/res/res_sounds.c
<https://reviewboard.asterisk.org/r/2507/#comment16949>

    This isn't needed - a module reload res_sounds would take care of it.
    
    On the other hand, registering a 'sounds show' function would be appropriate.



/trunk/res/res_sounds.c
<https://reviewboard.asterisk.org/r/2507/#comment16950>

    I'm not really sure what benefit we get here by having this be a resource module.
    
    (1) Sounds are a relatively 'core' concept
    (2) The ability to show the loaded sounds on the system feels like a core ability, and would be nice to always have available
    (3) You already have a sounds indexer API in core.
    
    Other than 'not loading it if you don't need it', why not have this in core?


- Matt Jordan


On May 7, 2013, 2:16 p.m., opticron wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2507/
> -----------------------------------------------------------
> 
> (Updated May 7, 2013, 2:16 p.m.)
> 
> 
> Review request for Asterisk Developers and David Lee.
> 
> 
> Bugs: ASTERISK-21584 and ASTERISK-21585
>     https://issues.asterisk.org/jira/browse/ASTERISK-21584
>     https://issues.asterisk.org/jira/browse/ASTERISK-21585
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This adds support for stasis/sounds and stasis/sounds/{ID} queries via the Stasis-HTTP interface.
> 
> The following changes have been made to accomplish this:
> * A modular indexer was created for local sound files.
> * A new function to get an ast_format associated with a file extension was added.  
> * Modifications were made to the built-in HTTP server so that URI decoding could be deferred to the URI handler when necessary.
> * The Stasis-HTTP sounds JSON documentation was modified to handle cases where multiple languages are installed in different formats.
> 
> This requires the additions to astobj2.c/h present in https://reviewboard.asterisk.org/r/2428/
> 
> 
> Diffs
> -----
> 
>   /trunk/main/sounds.c PRE-CREATION 
>   /trunk/main/http.c 387798 
>   /trunk/main/file.c 387798 
>   /trunk/include/asterisk/sounds.h PRE-CREATION 
>   /trunk/include/asterisk/http.h 387798 
>   /trunk/include/asterisk/file.h 387798 
>   /trunk/res/res_sounds.c PRE-CREATION 
>   /trunk/res/res_stasis_http.c 387798 
>   /trunk/res/stasis_http/resource_sounds.h 387798 
>   /trunk/res/stasis_http/resource_sounds.c 387798 
>   /trunk/rest-api/api-docs/sounds.json 387798 
> 
> Diff: https://reviewboard.asterisk.org/r/2507/diff/
> 
> 
> Testing
> -------
> 
> Tested using a browser and HTTP GET requests using the Stasis-HTTP API.  Currently contemplating how to create automated tests.
> 
> 
> Thanks,
> 
> opticron
> 
>

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