----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4475/#review14640 -----------------------------------------------------------
Ship it! Ship It! - Joshua Colp On March 11, 2015, 5:30 p.m., Matt Jordan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/4475/ > ----------------------------------------------------------- > > (Updated March 11, 2015, 5:30 p.m.) > > > Review request for Asterisk Developers and Joshua Colp. > > > Repository: Asterisk > > > Description > ------- > > Currently, the Asterisk 12+ testsuite test 'snoop_id' is failing due to a > stack overflow on 32-bit machines. This occurred after a patch was made that > enabled all formats on originated channels, including 192khz SLIN audio. > Since it is theoretically possible to create Local channels along with > ChanSpy in Asterisk 11 in a similar fashion, this issue is being fixed in > that branch as well. > > When an audiohook is created (which is used by the various Spy applications > and Snoop channel), it initially is given a sample rate of 8kHz. It is > expected, however, that this rate may change based on the media that passes > through the audiohook. However, the read/write operations on the audiohook > behave very differently. > > When a frame is written to the audiohook, the format of the frame is checked > against the internal sample rate. If the rate of the format does not match > the internal sample rate, the internal sample rate is updated and a new SLIN > format is chosen based on that sample rate. This works just fine. > > When a frame is read, however, we do somehting quite different. If the format > rate matches the internal sample rate, all is fine. However, if the rates > don't match, the audiohook attempts to "fix up" the number of samples that > were requested. This can result in some seriously large number of samples > being requested from the read/write factories. > > Consider the worst case - 192kHz SLIN. If we attempt to read 20ms worth of > audio produced at that rate, we'd request 3840 samples (192000 / (1000 / > 20)). However, if the audiohook is still expecting an internal sample rate of > 8000, we'll attempt to "fix up" the requested samples to: > > samples_converted = samples * (ast_format_get_sample_rate(format) / (float) > audiohook->hook_internal_samp_rate); > > or: > > 92160 = 3840 * (192000 / 8000) > > This results in us attempting to read 92160 samples from our factories, as > opposed to the 3840 that we actually wanted. On a 64-bit machine, this > miraculously survives - despite allocating up to two buffers of length 92160 > on the stack. The 32-bit machines aren't quite so lucky. Even in the case > where this works, we will either (a) get way more samples than we wanted; or > (b) get about 3840 samples, assuming the timing is pretty good on the machine. > > Either way, this is kind of wrong. > > My first inclination was to allocate the buffers on the heap. As it is, > however, there's at least two drawbacks with doing this: > (1) It's a bit complicated, as the size of the buffers may change during the > lifetime of the audiohook (ew). > (2) The stack is faster (yay); the heap is slower (boo). > > Since our calculation is flat out wrong in the first place, I've opted to fix > this issue that way instead. Rather than attempting to 'massage' the samples > requested, we now instead re-calculate our internal sample rate based on the > format requested. This causes us to read the correct number of samples, and > has the added benefit of setting the audihook with the right SLIN format. > > > Diffs > ----- > > /branches/11/main/audiohook.c 432788 > > Diff: https://reviewboard.asterisk.org/r/4475/diff/ > > > Testing > ------- > > (1) Tested ChanSpy in 11 and 13 manually. Still works. Yay. > (2) Ran the chanspy tests in 11/13. Those still work too. > (3) Ran the snoop tests. They stopped crashing, and also still work. > > > Thanks, > > Matt Jordan > >
-- _____________________________________________________________________ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
