[Home]

Summary:ASTERISK-15435: [patch] [OpenSolaris] wav format produces garbage files
Reporter:Ben Klang (bklang)Labels:
Date Opened:2010-01-14 20:34:34.000-0600Date Closed:2011-01-25 19:27:41.000-0600
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Formats/format_wav
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) 1263522240.57.wav
( 1) setvbuf.patch
Description:I was toying with changing the recording format for voicemail files on my server.  The GSM format had been working fine to both record and playback files.  However switching voicemail.conf to create wav files results in garbage data.  The files do not play back at all on several platforms, including the server that recorded it.

I also attempted to record wav audio using the Record() dialplan application.  The same garbage files resulted.  Here is what the console says when trying to playback one of the files:

[Jan 14 21:27:53] WARNING[14811]: app_playback.c:471 playback_exec: ast_streamfile failed on SIP/F27564A6-8B0D-408E-00000028 for /tmp/1263522240.57
[Jan 14 21:27:53] WARNING[14811]: format_wav.c:95 check_header: Does not begin with RIFF
[Jan 14 21:27:53] WARNING[14811]: file.c:370 fn_wrapper: Unable to open format wav

The size of the file produced varies approximately according to duration, so some data is getting in there.

This does not seem to impact other formats.  Playback of known-good wav files works fine so it only seems to be a problem writing the wav format.


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

A few times I would get a segfault within Asterisk after recording wav files, so I suspect there may also be memory corruption.  Unfortunately, the segfaults were not consistent and I have not yet succeeded in capturing a coredump.
Comments:By: Ben Klang (bklang) 2010-01-14 20:35:50.000-0600

One other thing to mention:  I tried both wav and wav49 in voicemail.conf.  Neither format produced a good recording.
By: Tilghman Lesher (tilghman) 2010-01-15 10:45:42.000-0600

An upload of a small written wav file might help us track down the problem.
By: Ben Klang (bklang) 2010-01-15 10:55:51.000-0600

Example file attached.
By: Leif Madsen (lmadsen) 2010-01-15 13:11:47.000-0600

If you're getting segfaults as well, you may wish to try and upload a backtrace for reference.
By: Leif Madsen (lmadsen) 2010-01-15 14:21:13.000-0600

It was mentioned in a call this may be a problem to do with little vs. big endien.
By: Tilghman Lesher (tilghman) 2010-01-15 15:07:28.000-0600

That would be most odd, given that bklang specifically noted that his platform is i386.  Perhaps something in OpenSolaris is doing something funky in regards to offset calculations?
By: Ben Klang (bklang) 2010-01-15 15:14:13.000-0600

Yes, my platform is i386, so endianness should not be the issue.  I checked and it's actually a Pentium D with 64-bit extensions, so it's x86-64 to be specific.
By: Sean Bright (seanbright) 2010-01-15 18:30:35.000-0600

Sounds file to me... just doesn't have a wav header.

PCM signed 16 bit (LE) mono @ 8000Hz

<i>"Testing 1... 2... 3... 4... 5..."</i>


By: Ben Klang (bklang) 2010-01-21 18:54:38.000-0600

To comment further on seanbright's comment, the wav header is the proper length, it's just all NULLs.  Interestingly, the two fields for size information are updated.  Probably not coincidentally, those two fields are set in a separate function in format_wav.c.  The function write_header() is responsible for writing a bunch of constants to the header of the wav file.  These are the fields that are NULL in the example file.  A second function named update_header() is responsible for filling in the two size fields.  Since those fields are present, it might appear that somehow write_header() is failing while update_header() is succeeding.  

Like seanbright I was able to get the audio to play by filling in the missing static bytes from the header.

For sanity's sake, I tested this on Linux and it is not an issue.  Somehow this is only an issue (so far) on OpenSolaris.  Verified in both 1.6.1 and 1.6.2 branches.
By: Michi (michaesc) 2010-01-28 06:12:39.000-0600

To be exact, this problem appears in Solaris 11 x86 (nv-b91) as well, not only OpenSolaris. It could very well be a problem that exists on all SVR5 Unix OSs, or maybe it is due to a nonstandard (non Posix/ISO C) programming construct. That last sentence is a speculation, not a observation.

It would be nice to be able to construct PCM wav files with the proper headers again as it was possible in Asterisk 1.4. It's not enough of course, to be able to take a recorded file to another computer which can play back wav files with defective headers. This would mean that people can leave messages for you, and you can never hear your voicemail messages = a very absurd concept.
By: Ben Klang (bklang) 2010-02-15 14:14:09.000-0600

I have found a workaround, though I'm not certain this is the correct fix.

Adding fflush(f); to the end of the write_header() function in both files products working wav files.
By: Chris Walton (crjw) 2010-04-30 00:43:44

The problem lies in main/file.c.

Stuff happens in this order:
1. A stream is opened for the output wav file.
2. The "RIFF" header is written to the stream via "rewrite_wrapper".
3. A new buffer is assigned to the stream using "setvbuf" on line 110 of main/file.c.

Both the Linux and Solaris man pages indicate that "setvbuf" should be used "before" any read or write operations are done on the stream.
I think it is just fluke that the code happens to work under Linux.

Everything seems to work perfectly if the setvbuf is done prior to "rewrite_wrapper".
By: Chris Walton (crjw) 2010-04-30 21:40:51

I just uploaded a simple patch that will make setbuf run before the header is written.
By: Leif Madsen (lmadsen) 2010-05-04 15:33:25

Can someone test this patch to determine if it resolves the issue? Thanks!
By: Ben Klang (bklang) 2010-06-24 10:40:31

I have tested crjw's patch and can confirm that the ability to record .wav audio files sounds fixed.  Thanks, crjw, for the patch.
By: Bruce McAlister (asgaroth) 2010-07-19 04:17:23

I can also confirm that this patch is working against Asterisk 1.6.2.9 running on Solaris 10 Update 5:

core show version
Asterisk 1.6.2.9 built by vzhsxn @ soldev on a i86pc running SunOS on 2010-07-19 08:56:33 UTC

uname -a
SunOS flamingo 5.10 Generic_142901-11 i86pc i386 i86pc
By: Paul Belanger (pabelanger) 2010-07-22 09:21:58

Looks like this patch will need some work. Here is a snip from #asterisk-dev this morning.

---
<putnopvut> Well, the patch has the potential to segfault if get_filestream() returns NULL. That's the one thing that stands out to me. Aside from that, if stuff appears to be working, then I give my "ship it!"
<pabelanger> Ok, just confirmed get_filestream() will return NULL, so segfault is possible.
<putnopvut> allocate fs. If it's allocated, then allocate the write_buffer and then move onto the rewrite_wrapper() call. If either allocation fails, then do some cleanup and exit with a failure code.
By: Chris Walton (crjw) 2010-07-22 10:39:55

I wrote the original patch which seems to work under "normal" conditions.
I doubt my C skills are good enough to do all the error checking and logging suggested by pabelanger and putnopvut.  Also I won't have time to code or test for the next few weeks.
I am hoping somebody else can take this on!
By: Eric Futch (efutch) 2010-10-21 17:15:19

Confirmed that this patch fixes the wav file bug for me as well on Solaris 10 10/09.
By: Sean Bright (seanbright) 2011-01-25 19:14:10.000-0600

In response to pabelanger's comment:

I don't see setvbuf() failing as a fatal error (and neither does the existing code), so we just need a check to make sure that the allocation of the filestream doesn't fail.

Committing a patch to this effect now.


By: Digium Subversion (svnbot) 2011-01-25 19:24:59.000-0600

Repository: asterisk
Revision: 304096

U   branches/1.6.2/main/file.c

------------------------------------------------------------------------
r304096 | seanbright | 2011-01-25 19:24:58 -0600 (Tue, 25 Jan 2011) | 12 lines

Per the man page, setvbuf() must be called before any other operation on an open file.

We use setvbuf() to associate a buffer with a stream, but we have already written
to the open file.  This works (by chance) on Linux, but fails on other platforms,
such as OpenSolaris.

(closes issue ASTERISK-15435)
Reported by: bklang
Patches:
     setvbuf.patch uploaded by crjw (license 963)
Tested by: bklang, asgaroth, efutch

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=304096
By: Digium Subversion (svnbot) 2011-01-25 19:26:27.000-0600

Repository: asterisk
Revision: 304097

_U  branches/1.8/
U   branches/1.8/main/file.c

------------------------------------------------------------------------
r304097 | seanbright | 2011-01-25 19:26:26 -0600 (Tue, 25 Jan 2011) | 19 lines

Merged revisions 304096 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.6.2

........
 r304096 | seanbright | 2011-01-25 20:24:58 -0500 (Tue, 25 Jan 2011) | 12 lines
 
 Per the man page, setvbuf() must be called before any other operation on an open file.
 
 We use setvbuf() to associate a buffer with a stream, but we have already written
 to the open file.  This works (by chance) on Linux, but fails on other platforms,
 such as OpenSolaris.
 
 (closes issue ASTERISK-15435)
 Reported by: bklang
 Patches:
       setvbuf.patch uploaded by crjw (license 963)
 Tested by: bklang, asgaroth, efutch
........

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=304097
By: Digium Subversion (svnbot) 2011-01-25 19:27:40.000-0600

Repository: asterisk
Revision: 304098

_U  trunk/
U   trunk/main/file.c

------------------------------------------------------------------------
r304098 | seanbright | 2011-01-25 19:27:40 -0600 (Tue, 25 Jan 2011) | 26 lines

Merged revisions 304097 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.8

................
 r304097 | seanbright | 2011-01-25 20:26:26 -0500 (Tue, 25 Jan 2011) | 19 lines
 
 Merged revisions 304096 via svnmerge from
 https://origsvn.digium.com/svn/asterisk/branches/1.6.2
 
 ........
   r304096 | seanbright | 2011-01-25 20:24:58 -0500 (Tue, 25 Jan 2011) | 12 lines
   
   Per the man page, setvbuf() must be called before any other operation on an open file.
   
   We use setvbuf() to associate a buffer with a stream, but we have already written
   to the open file.  This works (by chance) on Linux, but fails on other platforms,
   such as OpenSolaris.
   
   (closes issue ASTERISK-15435)
   Reported by: bklang
   Patches:
         setvbuf.patch uploaded by crjw (license 963)
   Tested by: bklang, asgaroth, efutch
 ........
................

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=304098