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

[Michael Niedermayer]

Hi

On Sun, Apr 01, 2007 at 01:08:52PM +0200, Bartlomiej Wolowiec wrote:
[...]
> > >   *
> > >   * 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.

functional changes and indention changes must be in seperate patches (see our
svn policy at http://ffmpeg.mplayerhq.hu/ffmpeg-doc.html#SEC39)
(and iam fine if you send the indention change patch later ...)


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

no, problem iam used to it, its quite rare that patches are perfect
in their first revission


[...]

> +    TIFF_TYPE_NB // max of TiffTypes

unused


[...]
> +static void add_entry(TiffEncoderContext * s,
> +                      enum TiffTags tag, enum TiffTypes type, int count,
> +                      const void *ptr_val)
> +{
> +    uint8_t *entries_ptr = s->entries + 12 * s->num_entries;
> +

> +    assert(s->num_entrie < TIFF_MAX_ENTRY);

typo s->num_entrieS


[...]
> +    if (type_sizes[type] * count <= 4) {
> +        tnput(&entries_ptr, count, ptr_val, type, 0);

> +        entries_ptr += 4 - type_sizes[type] * count;

this line seems unneeded


[...]
> +#ifdef CONFIG_ZLIB
> +    if (s->compr == TIFF_DEFLATE || s->compr == TIFF_ADOBE_DEFLATE) {
> +        uint8_t *zbuf;
> +        int zlen, zn;
> +        int j;
> +
> +        zlen = bytes_per_row * s->rps;
> +        zbuf = av_malloc(zlen);
> +        strip_offsets[0] = ptr - buf;
> +        zn = 0;
> +        for (j = 0; j < s->rps; j++) {
> +            memcpy(zbuf + j * bytes_per_row,
> +                   p->data[0] + j * p->linesize[0], bytes_per_row);
> +            zn += bytes_per_row;
> +        }
> +        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);
> +            goto fail;
> +        }
> +        ptr += n;
> +        strip_sizes[0] = ptr - buf - strip_offsets[0];
> +        av_free(zbuf);

you could factorize av_free() like

n = encode_strip(s, zbuf, ptr, zn, s->compr);
av_free(zbuf);
if(n<0){
    av_log(s->avctx, AV_LOG_ERROR, "Encode strip failed\n");
    goto fail;
}

but thats nitpicking i guess ...

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

Those who are too smart to engage in politics are punished by being
governed by those who are dumber. -- Plato 
-------------- 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/a5152705/attachment.pgp>