[asterisk-dev] [Code Review]: Pinequeue: Play queue prompts in the background - making call available to agent faster

Fri Apr 27 01:51:45 CDT 2012

> On April 26, 2012, 5:48 p.m., Mark Michelson wrote:
> > Unfortunately I've run out of time to be able to look at this today. I'll look at it more tomorrow. Here are some thoughts:
> > 
> > In app_queue.c, how does the channel get serviced once the generator is created? In the previous incarnation of play_file(), there was a call to ast_waitstream() that would service the channel (and thus play the file) to the channel. When you put a generator on the channel, something needs to call ast_waitfor() and ast_read() in order to have the generator generate data for it. In say.c, it looks like you call ast_autoservice_start() at one point in order to have the autoservice thread do this for you, but in app_queue, this does not happen.
> > 
> > The code is a bit disorganized. play_file() in app_queue should be broken up into at least two functions (one to play the file in the background and one to play and wait until the file completes plus more for other logical operations). In addition, the code be organized a bit better, so that, for instance, all operations that get done when now_playing == 0 are done in one block instead of having it done in disparate blocks. This will improve readability and make it a bit easier to review.
> > 
> > Some of the locations of data seems odd to me. For instance, the generator that has been added to app_queue.c seems like it should go into file.c instead, since it is potentially useful for other applications to use. This way you could add some sort of ast_streamfile_background() call that would allow you to play files in the background from any application. I already mention down below that the datastore information and callbacks can be moved into the global_datastores.h and global_datastores.c files.
> > 
> > This seems like a very nice addition since this is how most queuing systems work. The implementation details are a bit off though. I will look at this more tomorrow.

As I stated earlier, I was considering moving the generator stuff out of app_queue, but did not know where to. Thanks for the feedback, I'll spend time reformatting. This time around, I wanted more of an approval of the concept - if it was interesting or not, so whether it was worthwhile pursuing further.

I will probably need help with some of the details.

/O

> On April 26, 2012, 5:48 p.m., Mark Michelson wrote:
> > /trunk/apps/app_queue.c, line 6444
> > <https://reviewboard.asterisk.org/r/1887/diff/2/?file=27573#file27573line6444>
> >
> >     When was autoservice started?

The channel could already have something running from earlier. Or?

> On April 26, 2012, 5:48 p.m., Mark Michelson wrote:
> > /trunk/apps/app_queue.c, line 6659
> > <https://reviewboard.asterisk.org/r/1887/diff/2/?file=27573#file27573line6659>
> >
> >     Check to be sure that ast_datastore_alloc() succeeds. Also, remove the parentheses after ast_prompt_list

Why remove parenthesis when calling a function?

> On April 26, 2012, 5:48 p.m., Mark Michelson wrote:
> > /trunk/apps/app_queue.c, line 6704
> > <https://reviewboard.asterisk.org/r/1887/diff/2/?file=27573#file27573line6704>
> >
> >     Please don't add initials to code comments. See the third bullet point here: https://wiki.asterisk.org/wiki/display/AST/Coding+Guidelines#CodingGuidelines-GeneralRules

I thought I remove all of those. Just a mistake. 

> On April 26, 2012, 5:48 p.m., Mark Michelson wrote:
> > /trunk/include/asterisk/channel.h, lines 3516-3535
> > <https://reviewboard.asterisk.org/r/1887/diff/2/?file=27575#file27575line3516>
> >
> >     One thing that may simplify your life here is to check out global_datastores.h and global_datastores.c. We store datastore information in those files if the datastore may be accessed by multiple source files. An example of one is the datastore that keeps track of all the interfaces that have been dialed by a channel to ensure that the channel does not get caught in a forwarding loop.
> >     
> >     Using those files, you would not have to spread out the datastore information between channel.h, file.h, and file.c

Great. Thanks.

- Olle E

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

On April 26, 2012, 9:42 a.m., Olle E Johansson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1887/
> -----------------------------------------------------------
> 
> (Updated April 26, 2012, 9:42 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> Today, when a prompt is being played to a call waiting in the queue and an agent becomes available, the agent will not get the call until the prompt is finished. Both customer and agent is kept waiting.
> 
> With this rather big piece of code, we attach a generator to play prompts. The generator can, like all generators, be stopped at any time so that the agent (queue member) can get the call immediately.
> 
> The generator is now placed in app_queue, but could propably be moved somewhere else. It also changes functionality in main/say.c in order to be able to place those prompts in the same prompt queue for background processing. The same generator could be used to sevice conference bridges and maybe be added as a dialplan function at some point for background playlists.
> 
> 
> This addresses bug 19795.
>     https://issues.asterisk.org/jira/browse/19795
> 
> 
> Diffs
> -----
> 
>   /trunk/apps/app_queue.c 364000 
>   /trunk/configs/queues.conf.sample 364000 
>   /trunk/include/asterisk/channel.h 364000 
>   /trunk/include/asterisk/file.h 364000 
>   /trunk/main/asterisk.dynamics 364000 
>   /trunk/main/file.c 364000 
>   /trunk/main/say.c 364000 
> 
> Diff: https://reviewboard.asterisk.org/r/1887/diff
> 
> 
> Testing
> -------
> 
> Quite a lot of testing in our environment during the rather long time we've been testing this. Customer is happy.
> 
> 
> There was some complaints from a bowl of petunias, which made me a bit surprised.
> 
> 
> Thanks,
> 
> Olle E
> 
>

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