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

[Michael Niedermayer]

Hi

On Sun, Apr 01, 2007 at 05:20:57PM +0200, Kamil Nowosad 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


[...]
> +/* list of TIFF compression types */
>  enum TiffCompr{

not doxygen compatible

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


[...]

> +    uLongf zlen;

what is uLongf?


[...]

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


[...]

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


[...]
> + * 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


[...]

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

these should be const static


[...]

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


[...]
> +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


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

No great genius has ever existed without some touch of madness. -- 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/20070401/a8b095db/attachment.pgp>