[asterisk-commits] mjordan: branch 13 r432811 - in /branches/13: ./ main/audiohook.c

[SVN commits to the Asterisk project]

Author: mjordan
Date: Thu Mar 12 07:58:11 2015
New Revision: 432811

URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=432811
Log:
main/audiohook: Update internal sample rate on reads

When an audiohook is created (which is used by the various Spy applications
and Snoop channel in Asterisk 13+), 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 something 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);

  which is:

  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, the calculation being performed is wrong, based on the API users
expectations.

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, this patch fixes
this issue by instead updating the internal sample rate based on the format
passed into the read operation. This causes us to read the correct number of
samples, and has the added benefit of setting the audihook with the right
SLIN format.

Note that this issue was caught by the Asterisk Test Suite as a result of
r432195 in the 13 branch. Because this issue is also theoretically possible
in Asterisk 11, the change is being made here as well.

Review: https://reviewboard.asterisk.org/r/4475/
........

Merged revisions 432810 from http://svn.asterisk.org/svn/asterisk/branches/11

Modified:
    branches/13/   (props changed)
    branches/13/main/audiohook.c

Propchange: branches/13/
------------------------------------------------------------------------------
Binary property 'branch-11-merged' - no diff available.

Modified: branches/13/main/audiohook.c
URL: http://svnview.digium.com/svn/asterisk/branches/13/main/audiohook.c?view=diff&rev=432811&r1=432810&r2=432811
==============================================================================
--- branches/13/main/audiohook.c (original)
+++ branches/13/main/audiohook.c Thu Mar 12 07:58:11 2015
@@ -360,21 +360,12 @@
 {
 	struct ast_frame *read_frame = NULL, *final_frame = NULL;
 	struct ast_format *slin;
-	int samples_converted;
-
-	/* the number of samples requested is based on the format they are requesting.  Inorder
-	 * to process this correctly samples must be converted to our internal sample rate */
-	if (audiohook->hook_internal_samp_rate == ast_format_get_sample_rate(format)) {
-		samples_converted = samples;
-	} else if (audiohook->hook_internal_samp_rate > ast_format_get_sample_rate(format)) {
-		samples_converted = samples * (audiohook->hook_internal_samp_rate / (float) ast_format_get_sample_rate(format));
-	} else {
-		samples_converted = samples * (ast_format_get_sample_rate(format) / (float) audiohook->hook_internal_samp_rate);
-	}
+
+	audiohook_set_internal_rate(audiohook, ast_format_get_sample_rate(format), 1);
 
 	if (!(read_frame = (direction == AST_AUDIOHOOK_DIRECTION_BOTH ?
-		audiohook_read_frame_both(audiohook, samples_converted, read_reference, write_reference) :
-		audiohook_read_frame_single(audiohook, samples_converted, direction)))) {
+		audiohook_read_frame_both(audiohook, samples, read_reference, write_reference) :
+		audiohook_read_frame_single(audiohook, samples, direction)))) {
 		return NULL;
 	}