Summary:ASTERISK-06213: [patch] cleanup of file.c
Reporter:Luigi Rizzo (rizzo)Labels:
Date Opened:2006-01-29 06:44:54.000-0600Date Closed:2011-06-07 14:10:03
Versions:Frequency of
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;


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


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.