|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|
|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
This is all documented and in one place so changing the algorithm is
+ 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 
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
i am running with it at the moment, but only a few formats have
Overall the code is a lot cleaner. Hopefully someone will have time
to test it in addition to me.
 this needs to be fixed for those modules with multiple
formats within the same file.
 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
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.