[Ffmpeg-devel] [PATCH] TIFF encoder (Google SoC qualification task)

[Michael Niedermayer]

Hi

On Mon, Apr 02, 2007 at 03:01:29PM +0200, Kamil Nowosad wrote:
> Hi
> 
> On Sun, Apr 01, 2007 at 08:23:41PM +0200, Michael Niedermayer wrote:
> > > -                *out++ = 0x80 | (count - 1);
> > > +                if (style == FF_RLE_SETBIT)
> > > +                    *out++ = 0x80 | (count - 1);
> > > +                else
> > > +                    *out++ = -count + 1;
> > 
> > this can be done without a branch, also it can be done more generically
> 
> Is this sufficent:
> 
> a = (uint8_t[]{   1,-1})[style];
> b = (uint8_t[]{0x80, 0})[style];

this is unneeded a/b can just be passed as parameters to the function


> 
> and in loop:
> 
> *out++ = ((count - 1) | b) * a
> ?
> Or maybe I don't see a more simple solution? (I see only two analogous
> with +, and ^ instead of |)

well your solution is close to what i had in mind but it can be simplified
further (your needs 2 simply arithmetic operations and 1 multiply, it can
be done with only 2 simple arithmetic operations too)


[...]
> > > Index: libavcodec/tiffenc.c
> > > ===================================================================
> > > --- libavcodec/tiffenc.c	(wersja 0)
> > > +++ libavcodec/tiffenc.c	(wersja 0)
> > [...]
> > 
> > > +    uint16_t *color_map;
> > 
> > this could as well be a local array
> 
> Shouldn't I rather allocate the memory for that in tiff_init and free
> in tiff_end ? I forgot about moving it there...

why? its not needed after encoding the current frame or?


[...]

> > > +    case 0:                    // XXX: when a user chose vlc it also assigns the default
> > > +        s->compression = TIFF_DEFAULT;
> > > +        break;
> > 
> > hmm maybe its better to fail with an error if a unsupported compression is
> > choosen
> 
> Yes, but the coder_type is 0 both in cases when user uses "-coder vlc" and 
> when he doesn't use -coder at all. My idea is to redefine:
> 
> #define FF_CODER_TYPE_VLC       0
> #define FF_CODER_TYPE_AC        1
> #define FF_CODER_TYPE_RAW       2  // no coder
> #define FF_CODER_TYPE_RLE       3  // run-length
> #define FF_CODER_TYPE_DEFLATE   4 
> 
> in avcodec.h to something like that
> 
> #define FF_CODER_TYPE_DEFAULT   0
> #define FF_CODER_TYPE_VLC       1
> #define FF_CODER_TYPE_AC        2
> #define FF_CODER_TYPE_RAW       3  // no coder
> #define FF_CODER_TYPE_RLE       4  // run-length
> #define FF_CODER_TYPE_DEFLATE   5
> 
> However, it needs changes in ffv1.c. I guess, that the separate patch is
> needed ;-). Should I do it in such way? 

well either leave them as is and let the user specifiy the coder or
add a
FF_CODER_TYPE_DEFAULT   -1
your suggestion might break applications which dont use AVOption to set the
parameters ...

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The worst form of inequality is to try to make unequal things equal.
-- Aristotle
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070402/ad08751e/attachment.pgp>