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

Bartlomiej Wolowiec b.wolowiec
Sun Apr 1 13:08:52 CEST 2007


On Saturday 31 March 2007 23:10, Michael Niedermayer wrote:
> > > > > > +    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).
>
> ok, though i would prefer splited patches
> for the actual SOC having well seperated and reviewable changes is also
> important, otherwise i and the mentors would have alot of problems
> following development (that was one of many problems with last years SOC)

Ok, I divided it into two patches.

> >   *
> >   * This file is part of FFmpeg.
> >   *
> > @@ -20,6 +19,7 @@
> >   *
> >   */
> >  #include "avcodec.h"
> > +#include "rle.h"
> >
> >  /**
> >   * Count up to 127 consecutive pixels which are either all the same or
> > @@ -62,138 +62,37 @@
> >   * @param bpp Bytes per pixel
> >   * @param w Image width
> >   * @param h Image height
> > + * @param targa_style 1 - targa style ( repeat: 0x80 | (count - 1) ), 0
> > - tiff style ( repeat: -(count - 1) ) * @return Size of output in bytes,
> > or -1 if larger than out_size */
> > -static int targa_encode_rle(uint8_t *outbuf, int out_size, AVFrame *pic,
> > -                            int bpp, int w, int h)
> > +int rle_encode(uint8_t *outbuf, int out_size, uint8_t *inbuf, int bpp,
> > int w, int targa_style) {
> > -    int count, x, y;
> > -    uint8_t *ptr, *line, *out;
> > +    int count, x;
> > +    uint8_t *out;
> >
> >      out = outbuf;
> > -    line = pic->data[0];
> >
> > -    for(y = 0; y < h; y ++) {
> > -        ptr = line;
> > +        for(x = 0; x < w; x += count) {
> > +                /* see if we can encode the next set of pixels with RLE
> > */ +                if((count = count_pixels(inbuf, w-x, bpp, 1)) > 1) {
> > +                        if(out + bpp + 1 > outbuf + out_size) return -1;
> > +                        if(targa_style)
> > +                                *out++ = 0x80 | (count - 1);
> > +                        else
> > +                                *out++ = -(count - 1);
> > +                        memcpy(out, inbuf, bpp);
> > +                        out += bpp;
> > +                } else {
> > +                        /* fall back on uncompressed */
> > +                        count = count_pixels(inbuf, w-x, bpp, 0);
> > +                        *out++ = count - 1;
> >
> > -        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(out + bpp + 1 > outbuf + out_size) return -1;
>
> please dont reindent code, also please use 4 space indention like in the
> rest of libav*
> ... how should i know what you changed if diff cant match the lines up
> due to indention changes ...

The main problem is, that the oryginal code processed all lines, but the new 
function is processing single line, so almost all indentions moved 4 space 
back.

>
> > +        for (i = 0; i < (count * ts) / 2; i++) {
> > +            FFSWAP(uint8_t, *(ptr + i), *(ptr + count * ts - i - 1));
> > +        }
> > +        tnput(&entries_ptr, count, ptr_val, type, ts - 1);
>
> are you sure this is correct? i cant help but it looks quite odd to me
> also i cant find anything in the spec which would require that
> it rather looks a simple
> tnput(&entries_ptr, count, ptr_val, type, 0);
> might do the correct thing?

There, you're right. When I had problems with code, the answer from tiffdump 
from libtiff (http://www.remotesensing.org/libtiff/) suggested me the 
solution, but unfortunately there was a bug and the values were showed 
reverse.

>
>
> [...]
>
> > +    uint16_t bpp_tab[] = { 8, 8, 8, 8 };
>
> const static
>

Corrected. Earlier add_entry function couldn't have const argument.

Thank you for a great patience during correction of my bugs :)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: rle.patch
Type: text/x-diff
Size: 10423 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070401/1da7ef58/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: tiff.patch
Type: text/x-diff
Size: 30147 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070401/1da7ef58/attachment-0001.patch>



More information about the ffmpeg-devel mailing list