Summary:	ASTERISK-04194: [patch] Native support for OGG/Vorbis
Reporter:	Jeffrey C. Ollie (jcollie)	Labels:
Date Opened:	2005-05-16 16:09:40	Date Closed:	2008-01-15 15:42:03.000-0600
Priority:	Major	Regression?	No
Status:	Closed/Complete	Components:	Core/General
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) asterisk-ogg-2.patch.txt ( 1) COPYING ( 2) format_ogg_vorbis.c ( 3) voicemail-ogg-mime-type.patch.txt
Description:	This patch adds support for OGG/Vorbis audio.  It needs libogg and libvorbis (tested with libogg-1.1.2 and libvorbis-1.1.0).  It will currently only read 8Khz 1 channel OGG/Vorbis files.  Once there has been some more testing, I plan on adding the ability to downmix/downsample OGG/Vorbis audio files that were recorded with more channels and at a higher rate.  The new format can also record OGG/Vorbis audio at 8Khz and 1 channel. Note that this has only received light testing so far. ****** ADDITIONAL INFORMATION ****** Disclaimer on file.
Comments:	By: Jeffrey C. Ollie (jcollie) 2005-05-17 13:27:39 I have uploaded a new version that won't cut off the end of the audio when playing back.  It also has some more commenting of the code (yay!). By: Jeffrey C. Ollie (jcollie) 2005-05-17 13:34:41 Also, I have tested the new format with voicemail, Playback, and Record. By: Serge Vecher (serge-v) 2005-05-17 13:47:21 Looks like the header in format_ogg_vorbis.c could use some updating. Just my 2c. By: Russell Bryant (russell) 2005-05-19 01:36:07 Maybe you should post this to -dev to get some others testing it.  Maybe you already have ... if so, ignore me.  :) By: Kevin P. Fleming (kpfleming) 2005-05-19 01:37:22 Are there any licensing issues to be concerned with here? What license(s) are the relevant libraries distributed under? By: Jeffrey C. Ollie (jcollie) 2005-05-19 08:10:52 The license for libogg and libvorbis is OSS-friendly.  I've uploaded the COPYING file from the version of libogg that will ship with Fedora Core 4.  The license for libvorbis is identical except for the copyright dates. By: Kevin P. Fleming (kpfleming) 2005-05-19 11:28:54 This appears to be "BSD with advertising" or at least very similar to it, right? I don't know if the "reproduce this copyright with binary distributions" will be an issue or not... By: Kevin P. Fleming (kpfleming) 2005-05-19 11:29:34 Can you comment on the license compatibility aspects of this one? Thanks. By: Kevin P. Fleming (kpfleming) 2005-05-19 11:30:53 Actually, I guess that only applies if these modules are linked against static Xiph.org libraries... if not, then we are not redistributing any Xiph.org code. By: Jeffrey C. Ollie (jcollie) 2005-05-19 13:40:52 Posted a note to -dev to attract more testers. By: critch (critch) 2005-05-19 14:39:04 getcomment should have an appropriate log message, minor issue. tell probably shouldn't be to hard to develop. If the format isn't a variable bit rate format, it can be computed from the tell on the file pointer. Else Maybe even just store a counter of frames shoved out and then do the appropriate multiplication on request. By: Jeffrey C. Ollie (jcollie) 2005-05-19 14:58:18 Uploaded a new version that fixes the log message in getcomment.  As far as tell goes, OGG/Vorbis is a variable rate format, so implementing tell/seek isn't as easy with WAV or some other fixed rate format. By: David Woodhouse (dwmw2) 2005-05-20 08:07:21 Works very nicely -- voicemail can now send ogg files instead of .wav. Thanks. It sends email messages with the wrong MIME type though. Ideally the format converters would each tell us their own MIME type, but given the precedent set elsewhere in the same file (look for 'wav49') this hack (voicemail-ogg-mime-type.patch.txt) will probably suffice for now. By: Jeffrey C. Ollie (jcollie) 2005-06-06 13:53:43 Mark, you marked the formatting review as failed... Can you elaborate as to why? By: Michael Jerris (mikej) 2005-06-06 19:36:30 From a quick look: space after if i.e. if (.... Using spaces instead of tabs. I will try to review this more fully later this evening. By: Jason Parker (jparker) 2005-06-06 20:45:59 Repeated hardcoded numbers (4096) suck too.  Should make the heavily used ones constant. By: Michael Jerris (mikej) 2005-06-19 13:50:11 2 weeks no response.  Close if no response by 6/25. By: Clod Patry (junky) 2005-06-20 18:48:10 I disagree with you MikeJ. I really like that patch and i think that should be merged, cause right now there's nothing about ogg. The problem is just about license i think, markster is the best one to decide if that patch is okay or not. By: Michael Jerris (mikej) 2005-06-20 21:12:28 there has been no response to code review for 2 weeks.  If somone is willing to take responsibility for following this one through to completion, then it can stay open.  If no one is willing to respond, then there is no point in keeping it open. By: Jeffrey C. Ollie (jcollie) 2005-06-21 08:44:46 Uploaded a new version that should address some of the comments. By: Kevin P. Fleming (kpfleming) 2005-06-21 16:12:00 Code review notes: - there is an unused variable in 'ogg_vorbis_read' - it seems to me that a proper use of 'goto' could considerably shorten ogg_vorbis_open() and ensure that the exit/failure cases are handled consistently - there is no need to lock/unlock in usecount(), since reading an int is always atomic By: Clod Patry (junky) 2005-06-22 09:35:06 you said this: "there is no need to lock/unlock in usecount(), since reading an int is always atomic" . But if i check in formats/format_*.c , i see there's a lock/unlock everywhere in these files. Are you proposing to do something like: int usecount() {      return glistcnt; } If yes, ask me, i'll write a patch for formats/format_*.c. By: Kevin P. Fleming (kpfleming) 2005-06-22 16:39:41 Yes, that's how it is in all the modules, but it isn't necessary in any of them, unless the usecounter is being stored in something larger than an int (which wouldn't work anyway, since the function only returns an int). By: Clod Patry (junky) 2005-06-22 21:00:42 If jcollie wants to modify his patch, i'll provide a patch soon for all formats to clean the usecount function like you want, kpflemming. By: Jeffrey C. Ollie (jcollie) 2005-06-24 23:42:52 I've uploaded a new version that addresses two of Kevin's points.  As far as restructuring ogg_vorbis_open, I'm rather hesitant unless there have been specific bugs identified that don't have easy, quick fixes.  It took quite a bit of staring at example code and the library code itself to figure out how the OGG/Vorbis libraries work (of course, I've completely forgotten everything that I learned in the month or so since I originally wrote the code). By: Olle Johansson (oej) 2005-07-18 06:22:19 Gentle reminder from housekeeping - where are we with Ogg? /O By: pupfuzz (pupfuzz) 2005-07-19 03:01:11 Feedback from a user: This patch has worked well for me and is also provides important functionality. Some of the smartphones cannot play GSM or compressed WAV voicemail files, but they do play OGG. By: Clod Patry (junky) 2005-07-19 06:47:33 oej: jcollie made changes, we are waiting for kpfleming's review now. I really wish we'll could see format_ogg_vorbis.c in HEAD eventually. By: Kevin P. Fleming (kpfleming) 2005-07-19 20:18:42 Committed to CVS HEAD, thanks! By: Digium Subversion (svnbot) 2008-01-15 15:42:03.000-0600 Repository: asterisk Revision: 6173 U   trunk/apps/app_voicemail.c U   trunk/formats/Makefile A   trunk/formats/format_ogg_vorbis.c ------------------------------------------------------------------------ r6173 | kpfleming | 2008-01-15 15:42:02 -0600 (Tue, 15 Jan 2008) | 2 lines add OGG/Vorbis file format support (bug ASTERISK-4194) ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=6173