Summary:	ASTERISK-06213: [patch] cleanup of file.c
Reporter:	Luigi Rizzo (rizzo)	Labels:
Date Opened:	2006-01-29 06:44:54.000-0600	Date Closed:	2011-06-07 14:10:03
Priority:	Minor	Regression?	No
Status:	Closed/Complete	Components:	Core/General
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) file.c ( 1) file.c.diff ( 2) file-v3.diff ( 3) new_file_v2.tgz ( 4) new_file.tgz
Description:	The attached patch implements a massive simplification and cleanup of file.c. The changes are completely self contained so you can replace the file and recompile without having to touch anything else. Detailed description in order of importance: + Move to function build_filename() replicated implementations of the  algorithm to build a pathname from a filename. + introduce a helper function, fileexists_core(), to replace the  multiple (and slightly different) implementations of the code  that determines the existence and name of a sound file with  optional language and format preferences.  In addition, interpret language lookup in the same way as it is done  in say.c, i.e., if a language is specified:  - first try the the specified language, 'preflang';  - if preflang has a _ suffix, e.g. "it_pi", try the language without    the suffix (i.e. just "it" in the example);  - if no match is found, try without any preference;  Additionally, support the 'ast_language_is_prefix' variable described  below.  This is all documented and in one place so changing the algorithm is  trivial. + replace the 4 variants of ast_waitstream*() with wrappers around a  single waitstream_core() function which does everything. + comment and largely simplify the function ast_filehelper(), using  continue statements to reduce the nesting depth by 3-4 levels; + add a global variable, ast_language_is_prefix, to control where the  language prefix gets put in constructing the pathname for sound files.  By default the variable is set to 0 and this gives the traditional  behaviour of adding the language just before the last component;  if set to 1 (with a separate patch that applies to asterisk.c and  the appropriate include file) the language is prepended to the  entire pathname, which makes it possible to store all files for a  given language in a separate  subtree.  The mechanism can be extended to support more variations e.g. add the  language wherever a special pattern (e.g. // or whatever) is found, etc. + function copy(): put calls to close() only in one place; + add a comment to exts_compare() to remember that we can use a  simpler algorithm that doesn't require a copy of the string; ****** ADDITIONAL INFORMATION ****** for review, i also enclose a full version of the file.
Comments:	By: Luigi Rizzo (rizzo) 2006-02-02 14:12:27.000-0600 here is the revised version of the format handlers that i announced on the -dev list. In addition to the previous changes, I have done a number of things listed below. - moved to the header include/asterisk/file.h  the common data structures (struct ast_format and ast_filestream) - defined a common ast_filestream structure with extra pointers for  buffers and local format descriptors. The clients declare,  at format install time, how large these structures should be,  and the common code takes care of allocation and deallocation; - put into ast_format both the format lock and the usecnt [1]  so the common code related to locking/unlocking can be put in  one place (file.c). - removed common code in open/rewrite/and close and left to  the format handlers only the format specific things; - normalized the code to format the ast_frame on reads; - changed the explicit numbers for frame sizes and number of samples  into symbolic constants; - removed a lot of useless structure fields and variables and  dead code; i am running with it at the moment, but only a few formats have been tested. Overall the code is a lot cleaner. Hopefully someone will have time to test it in addition to me. [1] this needs to be fixed for those modules with multiple    formats within the same file. [2] no diffs at this time, only the modified tarball, because one    should look at the architecture first, and the diffs are too    extensive. You should probably be able to just overwrite your    existing files (file.c, formats/format_*c, include/asterisk/file.h)    and rebuild cleanly.    I am going to merge this code soon in svn: team/rizzo/base [3] By: Luigi Rizzo (rizzo) 2006-02-03 07:05:37.000-0600 another update - new_file_v2.tgz has added documentation in file.h to show what the handlers are supposed to do, and includes fixes the usecnt issue (now there is a single lock+counter per module, irrespective of the number of formats it registers). I have also merged pcm, pcm_alaw and au support into one file because it is basically the same code. Ogg_vorbis is not fixed yet (haven't tried to compile it). The changes are rather mechanical though. On passing, the following bugs/issues remain in the code: + .au support does not work properly to extend files;   pcm and pcm_alaw do work, so it is trivial to fix; + the seek support in h263 and h264 is totally bogus. I don't  know how it is supposed to work, anyways; + there is some inconsistency in the computation of header sizes  in wav_gsm; in a place or two it says 60 bytes, in other places  it uses 52 bytes. Which one is correct ? By: Kevin P. Fleming (kpfleming) 2006-02-14 18:26:07.000-0600 Please put this all into a standalone branch (not merged with your other work), as that will make it much easier to review and test. I must say, this looks like an amazingly good set of work, though :-) By: Luigi Rizzo (rizzo) 2006-02-15 06:07:17.000-0600 file-v3.diff applies cleanly agains svn10193 and implements all of this. while i try to fix ogg_vorbis (trivial, mechanical) please try the rest. By: Olle Johansson (oej) 2006-03-08 14:20:18.000-0600 file-v3.diff does not apply cleanly to trunk. Maybe you should consider creating a separate branch for this patch to keep it up to date for Kevin. By: Luigi Rizzo (rizzo) 2006-03-29 06:59:25.000-0600 the branch team/group/formats-luigi contains an up-to-date version of this code. By: Luigi Rizzo (rizzo) 2006-04-04 11:17:51 committed to trunk SVN 17285 By: Jason Parker (jparker) 2006-04-04 20:14:54 I think format_mp3 in asterisk-addons needs to be fixed also.  :) By: Olle Johansson (oej) 2006-04-05 00:36:02 The Makefile needs to remove the removed format drivers from existing installations that upgrade. By: Tilghman Lesher (tilghman) 2006-04-06 09:22:49 This change has broken file deletes where the format is passed as NULL.  Please see issue ASTERISK-6715 By: paradise (paradise) 2006-04-10 09:29:24 format_mp3 in asterisk-addons fails to compile :-( By: Luigi Rizzo (rizzo) 2006-04-12 11:05:07 we know the change of API is breaking some things outside the main tree, these issues will be addressed when the API has stabilized in a couple of weeks, presumably.