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

[Michael Niedermayer]

Hi

On Sun, Apr 01, 2007 at 05:32:55PM +0200, Bartlomiej Wolowiec wrote:
> Hi,
> I hope that all that you've suggested is already in code.
> 
> Best Regards,
> Bartek

[...]

> -/** 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


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


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


[...]
> +    if (avctx->pix_fmt == PIX_FMT_PAL8) {
> +        uint16_t pal[256 * 3];
> +        for (i = 0; i < 256; i++) {
> +            uint32_t rgb = *(uint32_t *) (p->data[1] + i * 4);
> +            pal[i]       = ((rgb >> 16) & 0xff) * 257;
> +            pal[i + 256] = ((rgb >> 8)  & 0xff) * 257;
> +            pal[i + 512] = (rgb & 0xff)         * 257;

the & 0xff) could be aligned (another silly nitpick i know ...)


[...]

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


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


>                  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


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

In a rich man's house there is no place to spit but his face.
-- Diogenes of Sinope
-------------- 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/ae80343e/attachment.pgp>