[asterisk-dev] [Code Review] 3618: App_jack: more than 8Khz

[Dennis Guse]

> On June 13, 2014, 6:28 p.m., Mark Michelson wrote:
> > /trunk/apps/app_jack.c, line 411
> > <https://reviewboard.asterisk.org/r/3618/diff/1/?file=59647#file59647line411>
> >
> >     You brought this up as a concern in your description, but considering that app_jack had been using a constant 160 previously, you are not introducing any new incorrect behavior by dividing the rate by 50. I say it's fine to go with that, at least for the time being.

I am ok with that - it's just not very beautiful.


> On June 13, 2014, 6:28 p.m., Mark Michelson wrote:
> > /trunk/apps/app_jack.c, line 190
> > <https://reviewboard.asterisk.org/r/3618/diff/1/?file=59647#file59647line190>
> >
> >     There is no reason for this to be an error message.

In my case I need to know immediately, if something with the Asterisk-Jack connection is wrong...
I would at least provide it as warning.


> On June 13, 2014, 6:28 p.m., Mark Michelson wrote:
> > /trunk/apps/app_jack.c, line 64
> > <https://reviewboard.asterisk.org/r/3618/diff/1/?file=59647#file59647line64>
> >
> >     I saw your comment about requiring a large ringbuffer for dealing with larger sampling rates, but I don't think a 128K allocation is the right way to go here.
> >     
> >     I think instead, you can start by using a smaller allocation. When you want to write to the ringbuffer, use the jack_ringbuffer_write_space() function to determine if you have room to write the bytes. If you do not, you can free the current ringbuffer and create a new larger one to take its place. This way, the #define value acts as the basis for ringbuffer allocations but does not have to be so large as to dictate a maximum size.
> >     
> >     I'm not sure if this would result in lost data, though, so feel free to let me know if this idea would not work.

Thanks for your review.
You are right defining the ringbuffer size in that way is awful.

I thought about your proposed solution with re-allocation new ringbuffers.
It might work, but is probably very tricky to implement as jack and asterisk are sharing them.
And I have no clue, if it will work at all since I have no experience with Jack.

I would propose another solution: define number of frames the ringbuffer needs be able to hold.
During initialization, when the frame_datalen is available, just use frame_datalen * frame_count.
In that way app_jack will be able to cope with higher sampling rates without always allocating such a large buffer.

What do you think?


- Dennis


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


On June 14, 2014, 10:12 p.m., Dennis Guse wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3618/
> -----------------------------------------------------------
> 
> (Updated June 14, 2014, 10:12 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-20696 and ASTERISK-23836
>     https://issues.asterisk.org/jira/browse/ASTERISK-20696
>     https://issues.asterisk.org/jira/browse/ASTERISK-23836
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Target: app_jack.c
> 
> Enables the jack-audiohook to cope with dynamic sampling rates from and to Asterisk.
> Information from channel is taken to derive the channel's sampling rate, suiting SLINxx format and frame->datalen.
> 
> Limitations:
> * Required information is taken from channel during initialization as audiohook does not provide this information then. Audiohook.internal_sampl_rate(...) is set later, but no callback is available to inform app_jack.
> 
> * Frame.datalen is computed using "rate / 50" assuming a ptime of 20ms.
> There is no internal API available to determine datalen for a SLINxx.
> 
> * Ringbuffer size is a DEFINE and thus must be quite big to be able to cope with 16Khz+
> 
> 
> Diffs
> -----
> 
>   /trunk/apps/app_jack.c 415578 
> 
> Diff: https://reviewboard.asterisk.org/r/3618/diff/
> 
> 
> Testing
> -------
> 
> Checked with jackd and puredata using G.711 and G.722 on Ubuntu 14.0.4 64bit using Linphone and Ekiga.
> 
> 
> Thanks,
> 
> Dennis Guse
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140614/a7c26044/attachment-0001.html>