[Home]

Summary:ASTERISK-16035: [patch] audiohook: translation to slinear 8khz regardless of samplerate
Reporter:Simon Sester (ssester)Labels:
Date Opened:2010-04-30 07:17:25Date Closed:2011-04-06 11:58:48
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Core/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) issue17267_proposed_patch_audiohook.c.txt
( 1) issue17267_proposed_patch_audiohook.h.txt
( 2) issue17267_quick_fix_diff.txt
( 3) _issue17267_quick_fix_unified_diff.txt
Description:If an audiohook is attached to a channel that uses a codec with a samplerate of 16khz (e.g. G722) the frames are always downgraded to 8khz 16bit signed linear. So if we have 16khz G722 with 320 samples, that will be downgraded to 160 samples.

The expected behaviour would be to translate the frames to 16khz 16bit signed linear (SLINEAR16) with 320 samples.

****** ADDITIONAL INFORMATION ******

When hooked to a channel (nativeformats = G722) the frame contains the
following information:
frame->datalen = 320
frame->samples = 160
frame->len = 20
ast_format_rate(chan->nativeformats) = 16000

The expected behaviour would be:
frame->datalen = 640
frame->samples = 320
frame->len = 20
Comments:By: Simon Sester (ssester) 2010-04-30 07:26:10

The file issue17267_quick_fix_diff.txt contains the changes I made to make the audiohook pass 16khz 16bit signed linear (AST_FORMAT_SLINEAR16) to the manipulation_callback (if necessary).
The hack works fine for me, but as far as I can tell from looking at the code it only works for TYPE-MANIPULATE audiohooks.


By: Leif Madsen (lmadsen) 2010-04-30 11:02:56

Thanks for the submission! Just waiting for your license to be approved.
By: Simon Sester (ssester) 2010-05-05 08:05:54

The license has been approved!
By: Leif Madsen (lmadsen) 2010-05-10 10:58:14

Yay license! Marked as Ready for Testing!
By: Leif Madsen (lmadsen) 2010-05-11 10:44:38

Any chance you could submit the patch as a unified diff?  (svn diff)
By: Simon Sester (ssester) 2010-05-11 11:23:32

A unified diff is now attached as issue17267_quick_fix_unified_diff.txt
By: David Vossel (dvossel) 2010-05-19 10:01:22

I'm afraid this won't be this easy.  I'm betting that there are lots of applications that assume that the audiohook is always going to be providing 8khz audio.  Perhaps an option in the ast_audiohook_init() function to tell what sample rate is expected would work.  Whatever it is, I think the audiohook should be guaranteed to consistently provide the same sample rate after initialization.
By: Paul Belanger (pabelanger) 2010-06-25 09:08:19

Ping, tracker clean up. And status updates?
By: Simon Sester (ssester) 2010-06-28 06:20:48

I'll begin working on a better solution soon.
By: Leif Madsen (lmadsen) 2010-07-06 10:45:38

Leaving this set to Feedback while we await on the wonderous solution provided by ssester :)
By: Paul Belanger (pabelanger) 2010-08-04 11:53:17

Suspended due to lack of activity. Please request a bug marshal in #asterisk-bugs on the IRC network irc.freenode.net to reopen the issue should you have the additional information requested.

Further information can be found at http://www.asterisk.org/developers/bug-guidelines
By: Tzafrir Cohen (tzafrir) 2010-08-19 06:56:29

Reopepned, as per request of ssester on #asterisk-bugs .
By: Simon Sester (ssester) 2010-08-19 08:05:51

The two new diffs contain a proposed solution. To deal with the concern from dvossel an audiohook now has to set a flag to be given SLINEAR16:
ast_set_flag(data->audiohook, AST_AUDIOHOOK_WANTS_SLINEAR16);
This way, older audiohook applications won't see any changes to the audiohook API.

Tested with Asterisk 1.6.2.10 and only one audiohook (with and without 16khz audio).
Still no SLINEAR16 support for spy and whisper audiohooks, but this patch shouldn't break them.
Also new in this patch: if there is one (or more) audiohook requesting SLINEAR and one (ore more) audiohook requesting SLINEAR16, both audiohooks get the requested format and the result will be combined into one SLINEAR16 frame before being transcoded back to the original format(untested).
By: Leif Madsen (lmadsen) 2010-08-24 12:45:18

Setting to Confirmed. This seems like a good thing to bring up on the asterisk-dev mailing list.
By: David Vossel (dvossel) 2010-09-13 12:55:41

I've briefly looked over the new patch and have a few comments about the direction it is taking.

It does appear to address the problem I mentioned earlier about being about to configure if 16khz is wanted or not, but the way it is done feels rather limiting.  If we are going to take the time to expand audiohooks from 8khz, it should be done in a way that is completely scalable, meaning nothing in the code hard codes checks for 16khz.  Instead of passing a flag to the init function telling it we what 16khz audio, we should be able to pass a number representing the sample rate we want.  Then internally the code should be written in a way that allows any sample rate Asterisk is capable of producing to be acceptable.  Doing it this way would allow us do have 64khz audiohooks for free once 64khz audio is supported, which isn't far off. We already have support for 32khz audio.
By: Leif Madsen (lmadsen) 2011-04-06 11:58:47

Closing this as dvossel's work in trunk resolves this issue. Will be part of Asterisk 1.10. Thanks!