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

[Kamil Nowosad]

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];

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 |)

> > +/* list of TIFF compression types */
> >  enum TiffCompr{
> 
> not doxygen compatible

It is done (I just wanted to ask some questions before submitting my
patch).

> > 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...

> > +    uLongf zlen;
> 
> what is uLongf?

It is a type used by zlib. Sorry, I haven't precisely checked, how is zlib 
used in other codecs. Now I've changed it to uint32_t.

> > +    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? 

> > +#define COMPRESS(func) \
> > +        for (y = 0; y < avctx->height; y += s->lines_per_strip) {\
> > +            int i;\
> > +            s->strip_offset[s->strip++] = CURR_OFFSET(s);\
> > +            for (i = 0; (i < s->lines_per_strip) && (y + i < avctx->height); i++) {\
> > +                func;\
> > +                src += p->linesize[0];\
> > +            }\
> > +        }
> > +    switch (s->compression) {
> > +    case TIFF_RAW:
> > +        if (s->buf_end - s->buf < avctx->height * s->bytes_per_line) {
> > +            av_log(avctx, AV_LOG_ERROR, "the buffer given is too short\n");
> > +            return -1;
> > +        }
> > +        COMPRESS(bytestream_put_buffer(&s->buf, src, s->bytes_per_line));
> > +        break;
> > +
> > +    case TIFF_PACKBITS:
> > +        COMPRESS(ret =
> > +                 ff_encode_rle(s->buf, s->buf_end - s->buf, src,
> > +                               s->bytes_per_line, 1, FF_RLE_NEG);
> > +                 if (ret == -1) {
> > +                     av_log(avctx, AV_LOG_ERROR,
> > +                        "the buffer given is too short\n");
> > +                     return -1;
> > +                 }
> > +                 s->buf += ret;);
> > +        break;
> 
> hmm isnt this a little bit obfuscated? if theres no way to cleanly simplify
> it then it might be better to leave it ...

I'll try to something with that.

> > + * encoding scheme:
> > + * @li [if the client has not set it] TIFF signature
> > + * @li offset of the IFD
> > + * @li image data [divided into strips]
> > + * @li IFD data which doesn't fit inside the IFD
> > + * @li IFD, sorted by tag
> > + * @li [if the client will not set it] 32 zero bits
> 
> the comment in [] is wrong

Fixed.

> > +    const uint8_t header[4] = { 0x49, 0x49, 42, 0 };
> > +    const uint8_t trailer[4] = { 0, 0, 0, 0 };
> 
> these should be const static

Changed.

> > +    if (tiff_add_ifd_entry(avctx, TIFF_WIDTH,         TIFF_LONG,     1,           &avctx->width)||
> > +        tiff_add_ifd_entry(avctx, TIFF_HEIGHT,        TIFF_LONG,     1,           &avctx->height)||
> > +        tiff_add_ifd_entry(avctx, TIFF_BPP,           TIFF_SHORT,    s->nsamples, s->bpp_info)||
> > +        tiff_add_ifd_entry(avctx, TIFF_COMPR,         TIFF_SHORT,    1,           &s->compression)||
> > +        tiff_add_ifd_entry(avctx, TIFF_INVERT,        TIFF_SHORT,    1,           &s->photometric_interpretation)||
> > +        tiff_add_ifd_entry(avctx, TIFF_STRIP_OFFS,    TIFF_LONG,     s->strip,    s->strip_offset)||
> > +        tiff_add_ifd_entry(avctx, TIFF_SAMPLESPERPIX, TIFF_SHORT,    1,           &s->nsamples)||
> > +        tiff_add_ifd_entry(avctx, TIFF_ROWSPERSTRIP,  TIFF_LONG,     1,           &s->lines_per_strip)||
> > +        tiff_add_ifd_entry(avctx, TIFF_STRIP_SIZE,    TIFF_LONG,     s->strip,    s->strip_size)||
> > +        tiff_add_ifd_entry(avctx, TIFF_XRES,          TIFF_RATIONAL, 1,           xres)||
> > +        tiff_add_ifd_entry(avctx, TIFF_YRES,          TIFF_RATIONAL, 1,           yres)||
> > +        tiff_add_ifd_entry(avctx, TIFF_RES_UNIT,      TIFF_SHORT,    1,           (uint16_t[]){2})||
> > +        ((s->has_color_map) &&
> > +        tiff_add_ifd_entry(avctx, TIFF_PAL,           TIFF_SHORT,    3 << s->bpp, s->color_map))
> > +        )
> > +        goto cleanup;
> 
> this is the longest if() ive seen in a patch i think, cant that be avoided?

I've changed that.

> > +static int tiff_init(AVCodecContext * avctx)
> > +{
> > +    return 0;
> > +}
> > +
> > +static int tiff_end(AVCodecContext * avctx)
> > +{
> > +    return 0;
> > +}
> 
> hmm, these can be removed if theres nothing to allocate and cleanup but
> then why are strip_offset/strip_size/color_map in the context at all
> they could as well be local variables

Ok, I'll remove them if they won't be needed.

Sorry to bother you with such kind of stupid shortcomings... I'm now getting 
out of habits gained during the contest programming ;-) 

-- 
Best regards,
Kamil Nowosad