[Ffmpeg-devel] TIFF LZW decoder (was: [PATCH] change gif demuxer to gif decoder)

[Michael Niedermayer]

Hi

On Wed, Oct 25, 2006 at 04:07:56PM +0300, Kostya wrote:
> On Wed, Oct 25, 2006 at 01:11:15PM +0200, Michael Niedermayer wrote:
> > Hi
> > 
> > On Wed, Oct 25, 2006 at 07:34:18AM +0300, Kostya wrote:
> > > On Tue, Oct 24, 2006 at 12:51:02PM +0200, Michael Niedermayer wrote:
> > > > Hi
> > > > 
> > > > On Tue, Oct 24, 2006 at 12:24:15PM +0300, Kostya wrote:
> > > > > On Tue, Oct 24, 2006 at 07:35:01AM +0300, Kostya wrote:
> > > > > > 
> > > > > > As I understand, it may not need the wrapper, just two additional fields in
> > > > > > structure: GetCode() pointer and dict_limit (when dictionary size reaches it,
> > > > > > it should be expanded). I also tend to pass buffer and size to the structure
> > > > > > instead of using bytestream.
> > > > > > 
> > > > > > I intend to present this scheme to the end of week.
> > > > >  
> > > > > Here it is. 
> > > > 
> > > > i think that the LZWState struct should not be in the "public"
> > > > header for lzw, but rather in either lzw.c (or lzw_internal.h)
> > > > a "typedef void LZWState;" in lzw.h should be enough ...
> > > 
> > > I think that would cause unneeded complexity (decoders should know
> > > the size of LZWState anyway).
> > 
> > the problem with knowing the size is that code which uses lzw becomes 
> > dependant on the specific size, for libavcodec it doesnt matter much, just
> > 2-3 files more which need to be compiled if the size changes but if lzw would
> > be used from outside libavcodec (or libavutil if we move it there) then
> > we couldnt change LZWState in any way without breaking binary compatibility
> > 
> > iam not sure is there some interrest in having lzw in libavutil and useable
> > from outside? if no then id accept the LZWState in lzw.h even though i dont
> > think its good design, its not like 1 pointer dereference per compressed
> > block would have a meassureable impact on speed or that 1 av_malloc in
> > ff_lzw_decode_init() and a av_free() call at the end would be alot of
> > complexity (all IMHO)
> 
> Changed as you suggested. Please note that allocation and initialization are
> two different functions as TIFF requires LZW decoder to be reinited for each line
> and placing alloc/free to codec_decode_init/end seems logical too.
> 
> As for moving it to lavutil - it can be done any time but LZW is tied to decoders
> and I don't see any sense in doing that (except as a new feature of lavutil).
> 
> > and about complexity ....
> > id suggest to replace the get_code function pointer by a simple
> > get_code(...){
> >     if(TIFF){
> >     }else{
> >     }
> > }
> > 
> > not only is it simpler but it also should be faster as it can be
> > inlined while a function pointed to by a function pointer can not (easily)
> > which means copy arguments to the stack, read function pointer, call it,
> > save clobbered registers, read arguments from the stack, ..., restore
> > clobbered registers, return
> 
> Done

thanks, and feel free to commit it (and dont forget svn cp)

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

In the past you could go to a library and read, borrow or copy any book
Today you'd get arrested for mere telling someone where the library is