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

Bartlomiej Wolowiec b.wolowiec
Sat Mar 31 21:00:01 CEST 2007


Hi,
On Saturday 31 March 2007 14:42, Michael Niedermayer wrote:
> something like:
>
> static void tnput(uint8_t ** p, int n, uint8_t * val, enum TiffTypes type,
> int flip){ int i;
> #ifdef WORDS_BIGENDIAN
>     flip ^= ((int[]){0,0,0,1,3,3})[type];
> #endif
>     for(i=0; i < n*type_sizes2[type]; i++)
>         *(*p)++ = val[i ^ flip];
> }

ok.

> > > > +    case TIFF_PACKBITS:
> > > > +
> > > > +        from = src;
> > > > +        val = *(src++);
> > > > +        while (src < end) {
> > > > +            // max 128 bytes in direct copy
> > > > +            if (src - from >= 127) {
> > > > +                *(dst++) = src - from - 1;
> > > > +                memcpy(dst, from, src - from);
> > > > +                dst += src - from;
> > > > +                from = src;
> > > > +            }
> > > > +            if (val == *src) {
> > > > +                // how long is run ?
> > > > +                run_end = src;
> > > > +                while (run_end < end && *run_end == val
> > > > +                       && run_end - from < 128)
> > > > +                    run_end++;
> > > > +
> > > > +                if (run_end - src == 1 && src - from >= 2) {
> > > > +                    src++;
> > > > +                } else {
> > > > +
> > > > +                    // write bytes from buffer
> > > > +                    if (src - from >= 2) {
> > > > +                        *(dst++) = src - from - 2;
> > > > +                        memcpy(dst, from, src - from - 1);
> > > > +                        dst += src - from - 1;
> > > > +                        from = src - 1;
> > > > +                    }
> > > > +
> > > > +                    src = run_end;
> > > > +
> > > > +                    *(dst++) = from - src + 1;
> > > > +                    *(dst++) = val;
> > > > +                    from = src;
> > > > +                }
> > > > +            }
> > > > +            val = *src;
> > > > +            src++;
> > > > +        }
> > > > +        // write buffer
> > > > +        src = FFMIN(src, end);
> > > > +        if (src - from > 0) {
> > > > +            *(dst++) = src - from - 1;
> > > > +            for (; from != src; from++) {
> > > > +                *(dst++) = *from;
> > > > +            }
> > > > +        }
> > >
> > > this is duplicate relative to targaenc.c
> >
> > But targa_encode_rle use a little different coding. It takes the bpp
> > argument and my tests showed that it is slower for bpp=1...
>
> well, merge both codes into a generic rle encoder in rle.c/h and use that
> there are many codecs which use rle duplicating it for each is not ok
> and dont hesitate to optimize rle.c/h if you want though iam not insisting
> on this, what i insist on though is that code duplication is not ok

Ok, I added rle.c/h files, modified targaenc.c and my code (I know that it 
should be in different patch, but as a qualification task I send it in one).

> > > [...]
> > >
> > > > +    if (buf_size <
> > > > +        (((s->height * s->width * s->bpp >> 3) * 129) >> 7) + 128 +
> > > > +        TIFF_MAX_ENTRY * 12 + strips * 8) {
> > > > +        av_log(s->avctx, AV_LOG_ERROR, "Buffer is too small\n");
> > > > +        return -1;
> > > > +    }
> > >
> > > this check is buggy
> >
> > Ok, I added check_size to my code.
>
> which is more complex and too looks buggy though i didnt check it
> carefull so i could be wrong ...
>

I think that it works correctly, I've made tests checking how much memory is 
allocated in comparison with how much is used. For RAW the value was exactly 
the same, of course for packbits and deflate there was diferrence.

> [...]
>
> > +    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 +        uint32_t *strip_offsets;        ///< strips
> > offsets in file +        uint32_t *strip_sizes;          ///< strips
> > sizes
>
> indention looks wrong
> also the ///< are not aligned nicely

hmm.. I have accidentally formatted it by tab and clean-diff spoiled 
indention...

> > +    }
> > +}
> > +
> > +/**
> > + * Write yuv line in tiffs order.
> > + *
> > + * @param s Tiff context
> > + * @param line Line buffer
> > + * @param y Line position
> > + * @pram yuv_y Number of bytes of luma corresponding to chrominance
> > + */
> > +
> > +static void make_yuv_line(TiffEncoderContext * s, uint8_t * line, int y,
> > +                          int yuv_y)
> > +{
> > +    uint8_t *line_ptr = line;
> > +    AVFrame *const p = (AVFrame *) & s->picture;
> > +    int j, k;
> > +
> > +    for (j = 0; j < s->width / yuv_y; j++) {
> > +        memcpy(line_ptr, p->data[0] + y * p->linesize[0] + j * yuv_y,
> > +               yuv_y);
> > +        line_ptr += yuv_y;
> > +        *line_ptr++ = p->data[1][y * p->linesize[1] + j];
> > +        *line_ptr++ = p->data[2][y * p->linesize[2] + j];
> > +
> > +    }
> > +
> > +    // write end bytes
> > +    for (k = 0; k < yuv_y; k++) {
> > +        if (j * yuv_y + k < s->width)
> > +            *line_ptr++ = p->data[0][y * p->linesize[0] + j * yuv_y +
> > k]; +        else
> > +            *line_ptr++ = 0;
> > +    }
> > +    *line_ptr++ = p->data[1][y * p->linesize[1] + j - 1];
> > +    *line_ptr = p->data[2][y * p->linesize[2] + j - 1];
>
> thinking about this a little, this looks wrong, its converting planar
> yuv to packed, you rather should support the packed format used in tiff
> directly and not support the planar ones, and if ffmpeg happens not to
> support the exact packed format (i didnt check) then the correct action
> would be either add support in a seperate patch or drop yuv in tiff
> support for now
>

tiff uses Y0 Y1 Cb Cr format, the information from libavutil/avutil.h shows 
that ffmpeg doesn't support that YUV format.  Currently, I'm deleting support 
for YUV from my implementation of TIFF. In the future I'm willing to write 
the proper patch.

> [...]
>
> > +    memcpy(s->bpp_tab, (uint16_t[]) {8, 8, 8, 8}, 8);
>
> now as bpp_tab is always constant it doesnt need to be in the context
> anymore unless its clear that it will have to be changed in the future for
> other pixel formats? (which i dont know so maybe its better like it is)

I changed it into constant. I've look through the documentation and I saw that  
almost always 8,8,8,8 is used.

> > +
> > +        zlen = bpr * s->rps;
> > +        zbuf = av_malloc(zlen);
> > +        s->strip_offsets[0] = ptr - buf;
> > +        zn = 0;
> > +        for (j = 0; j < s->rps; j++) {
> > +            if (isYUV(avctx->pix_fmt)) {
> > +                make_yuv_line(s, line, j, yuv_y);
> > +            } else {
> > +                line = p->data[0] + j * p->linesize[0];
> > +            }
> > +
> > +            memcpy(zbuf + j * bpr, line, bpr);
> > +            zn += bpr;
> > +        }
> > +        if ((n = encode_strip(s, zbuf, ptr, zn, s->compr)) < 0) {
> > +            av_log(s->avctx, AV_LOG_ERROR, "Encode strip failed\n");
> > +            av_free(zbuf);
> > +            if (isYUV(avctx->pix_fmt)) {
> > +                av_free(line);
> > +            }
> > +            return -1;
> > +        }
> > +        ptr += n;
> > +        s->strip_sizes[0] = ptr - buf - s->strip_offsets[0];
> > +        av_free(zbuf);
> > +    } else
> > +#endif
> > +    {
> > +        start_strip = ptr;
> > +
> > +
> > +        for (i = 0; i < s->height; i++) {
> > +            if (s->strip_sizes[i / s->rps] == 0) {
> > +                s->strip_offsets[i / s->rps] = ptr - buf;
> > +                start_strip = ptr;
> > +            }
> > +            if (isYUV(avctx->pix_fmt)) {
> > +                make_yuv_line(s, line, i, yuv_y);
> > +            } else {
> > +                line = p->data[0] + i * p->linesize[0];
> > +            }
> > +
> > +            if ((n = encode_strip(s, line, ptr, b_width, s->compr)) < 0)
> > { +                av_log(s->avctx, AV_LOG_ERROR, "Encode strip
> > failed\n"); +                if (isYUV(avctx->pix_fmt)) {
> > +                    av_free(line);
> > +                }
> > +                return -1;
> > +            }
> > +            ptr += n;
>
> the code here and in the deflate case above looks somewhat similar, cant
> they be merged?

I' ve considered it, but I haven't found rational method of joining it.  
DEFLATE packs everything (firstly it must be copied to buffer), but the rest 
of methods pack every line separately. Moreover, DEFLATE code will be easier 
to adapt to add LZW compression later.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: tiff.patch
Type: text/x-diff
Size: 45623 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070331/04743228/attachment.patch>



More information about the ffmpeg-devel mailing list