[Ffmpeg-devel] tiff encoder (qualification task for GSoC)

[Bartlomiej Wolowiec]

On Sunday 01 April 2007 20:52, Michael Niedermayer wrote:
> > -/** sizes of various TIFF field types */
> > +/** sizes of various TIFF field types (string size = 100)*/
> >  static const int type_sizes[6] = {
> >      0, 1, 100, 2, 4, 8
> >  };
> >
> > -typedef struct TiffContext {
> > +/** sizes of various TIFF field types (string size = 1)*/
> > +static const int type_sizes2[6] = {
> > +    0, 1, 1, 2, 4, 8
> > +};
>
> uint8_t or unsigned char would safe a little space for these tables
>
> also they should ideally not end up duplicated in both encoder and
> decoder but i think its better to leave it as is for now and change
> that later so i dont have to reread the 2 tiff encoder patches,
> another 100 times, i just wanted to mention it

ok, I've changed in tiff.h from static arrays to only extern. Arrays are in 
tiff.c.

> > +
> > +typedef struct TiffEncoderContext {
> >      AVCodecContext *avctx;
> >      AVFrame picture;
> >
> > -    int width, height;
> > -    unsigned int bpp;
> > -    int le;
> > -    int compr;
> > -    int invert;
> > +    int width;                  ///< picture width
> > +    int height;                 ///< picture height
> > +    unsigned int bpp;           ///< bits per pixel
> > +    int compr;                  ///< compression level
> > +    int bpp_tab_size;           ///< bpp_tab size
> > +    int invert;                 ///< photometric interpretation
> > +    int strips;                 ///< number of strips
> > +    int rps;                    ///< row per strip
> > +    uint8_t *entries;           ///< entires in header
> > +    int num_entries;            ///< number of entires
> > +    uint8_t **buf;              ///< actual position in buffer
> > +    uint8_t *buf_start;         ///< pointer to first byte in buffer
> > +    int buf_size;               ///< buffer size
> > +} TiffEncoderContext;
> >
> > -    int strips, rps;
> > -    int sot;
> > -    uint8_t* stripdata;
> > -    uint8_t* stripsizes;
> > -    int stripsize, stripoff;
> > -    LZWState *lzw;
> > -} TiffContext;
>
> well, either put the encoder struct in tiffenc.c or use the same context
> struct for both encoder and decoder, they seem fairly similiar anyway

it is now in tiffenc.c and I think it's a better solution because it uses less 
memory in decoder.

> > +/**
> > + * Encode one strip in tiff file
> > + *
> > + * @param s Tiff context
> > + * @param src Input buffer
> > + * @param dst Output buffer
> > + * @param n Size of input buffer
> > + * @param compr Compression method
> > + * @return Number of output bytes. If an output error is encountered, -1
> > returned + */
> > +
> > +static int encode_strip(TiffEncoderContext * s, const int8_t * src,
> > +                        uint8_t * dst, int n, int compr)
> > +{
> > +
> > +    switch (compr) {
> > +#ifdef CONFIG_ZLIB
> > +    case TIFF_DEFLATE:
> > +    case TIFF_ADOBE_DEFLATE:
> > +        {
> > +            unsigned long zlen = n + 128;
> > +            if (check_size(s, zlen))
> > +                return -1;
> > +            if (compress(dst, &zlen, src, zlen) != Z_OK) {
> > +                av_log(s->avctx, AV_LOG_ERROR, "Compressing failed\n");
> > +                return -1;
> > +            }
> > +            return zlen;
> > +        }
> > +#endif
> > +    case TIFF_RAW:
> > +        if (check_size(s, n))
> > +            return -1;
> > +        memcpy(dst, src, n);
> > +        return n;
> > +    case TIFF_PACKBITS:
> > +        if (check_size(s, ((n >> 7) + 1) * 129))
> > +            return -1;
> > +
> > +        return ff_rle_encode(dst, ((n >> 7) + 1) * 129, src, 1, n, 0);
> > +    default:
> > +        return -1;
> > +    }
> > +}
>
> the encode_strip() function could be inlined, this would simplify the code
> though iam not sure if its a good idea maybe its cleaner as is so
> leave it if you prefer

I will leave it as it is now..

>
>
> [...]
>
> > +static int tiff_encoder_init(AVCodecContext * avctx)
> > +{
> > +    TiffEncoderContext *s = avctx->priv_data;
> > +    s->entries = av_malloc(TIFF_MAX_ENTRY * 12);
> > +    return 0;
> > +}
> > +
> > +static int tiff_encoder_end(AVCodecContext * avctx)
> > +{
> > +    TiffEncoderContext *s = avctx->priv_data;
> > +    av_free(s->entries);
> > +    return 0;
> > +}
>
> s->entries doesnt need to be allocated by malloc(), it seems ive missed
> that also these 2 functions become useless after that ...

ok, array has now constant size and it isn't allocated by malloc().

>
> [...]
>
> >          for(x = 0; x < w; x += count) {
> >              /* see if we can encode the next set of pixels with RLE */
> > -            if((count = count_pixels(ptr, w-x, bpp, 1)) > 1) {
> > +            if((count = count_pixels(inbuf, w-x, bpp, 1)) > 1) {
>
> please dont rename ptr to inbuf

ok

> >                  if(out + bpp + 1 > outbuf + out_size) return -1;
> > +
> > +                if(targa_style)
> >                  *out++ = 0x80 | (count - 1);
> > -                memcpy(out, ptr, bpp);
> > +                else
> > +                *out++ = -(count - 1);
>
> well, as i already said to the other student, this can be done without the
> branch and more generically, actually i could think of 3 ways to do this
> simpler and the look up table variant is not the one i want because theres
> a much simpler solution which should be both fast and support all sane
> variants to store this byte

Well, I added two arguments to the function and the value is calculated that 
way: (count ^ xor) + add. I think that this solution is fast and universal. 

Best Regards,
Bartek
-------------- next part --------------
A non-text attachment was scrubbed...
Name: rle.patch
Type: text/x-diff
Size: 10270 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070402/36e0e8c6/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: tiff.patch
Type: text/x-diff
Size: 31175 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070402/36e0e8c6/attachment-0001.patch>