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

[Bartlomiej Wolowiec]

On Friday 30 March 2007 23:09, Michael Niedermayer wrote:
> no, the code is much more complex than needed, there are many ways this
> can be implemeneted, one is to reserve space for the IFD and write
> everything immedeatly, this will need 2 lines of code to calculate the IFD
> size
> another is to store data immedeatly and store the IFD in a seperate array
> like s->entries (but not in this odd custom format) then at the end just
> memcpy it to its final place

Ok, I have chosen second option. 

> > +/// Array containing the number of bytes of luma corresponding to
> > chrominance +static const int YUV_Y[PIX_FMT_NB] = {
> > +    [PIX_FMT_YUV411P] = 4,
> > +    [PIX_FMT_YUV422P] = 2,
> > +    [PIX_FMT_YUV444P] = 1,
> > +};
> > +
> > +/// Horizontal subsampling
> > +static const int YUV_SUBSAMPLING_HORIZ[PIX_FMT_NB] = {
> > +    [PIX_FMT_YUV411P] = 4,
> > +    [PIX_FMT_YUV422P] = 2,
> > +    [PIX_FMT_YUV444P] = 1,
> > +};
> > +
> > +/// Vertical subsampling
> > +static const int YUV_SUBSAMPLING_VERT[PIX_FMT_NB] = {
> > +    [PIX_FMT_YUV411P] = 1,
> > +    [PIX_FMT_YUV422P] = 1,
> > +    [PIX_FMT_YUV444P] = 1,
> > +};
>
> duplicate table, int8_t is enough for them, no need to waste
> PIX_FMT_NB*sizeof(int) space
>
> PIX_FMT_YUV420P is missing
>
> see avcodec_get_chroma_sub_sample()
>

Ok, I have removed these tables from code.

> > +
> > +/**
> > + * Put n values to buffer
> > + *
> > + * @param p Pointer to pointer to output buffer
> > + * @param n Number of values
> > + * @param val Pointer to values
> > + * @param type type of values
> > + */
> > +static void tnput(uint8_t ** p, int n, uint8_t * val, enum TiffTypes
> > type) +{
> > +    int i;
> > +    for (i = 0; i < n; i++) {
> > +        switch (type) {
> > +        case TIFF_STRING:
> > +        case TIFF_BYTE:
> > +            bytestream_put_byte(p, *val);
> > +            val++;
> > +            break;
> > +        case TIFF_SHORT:
> > +            bytestream_put_le16(p, *((uint16_t *) val));
> > +            val += 2;
> > +            break;
> > +        case TIFF_LONG:
> > +            bytestream_put_le32(p, *((uint32_t *) val));
> > +            val += 4;
> > +            break;
> > +        case TIFF_RATIONAL:
> > +            bytestream_put_le32(p, *((uint32_t *) val));
> > +            val += 4;
> > +            bytestream_put_le32(p, *((uint32_t *) val));
> > +            val += 4;
> > +            break;
> > +        default:
> > +            assert(0 && "not implemented");
> > +        }
> > +    }
> > +}
>
> this function can easily be implemented in a quarter of the code

hmm.. I remove val+=, but I don't know how can I simplify it more.

>
>
> [...]
>
> > +            //reverse (LE)
> > +            for (i = 0; i < entry->count / 2; i++) {
> > +                for (j = 0; j < ts; j++) {
> > +                    tmp = *(ptr + i * ts + j);
> > +                    *(ptr + i * ts + j) =
> > +                        *(ptr + ((entry->count - i - 1) * ts) + j);
> > +                    *(ptr + ((entry->count - i - 1) * ts) + j) = tmp;
> > +                }
> > +            }
> > +            tnput(buf, entry->count, entry->val, entry->type);
>
> you reshuffle the data twice here

I don't exactly understand what do you mean. If data size is less than four 
bytes, the data is written in offset field, but firstly I should reverse it.

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

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

> > +        for (i = 0; i < strips; i++) {
> > +            strip_offsets[i] = s->offset + ptr - buf;
> > +            zn = 0;
> > +            for (j = 0; j < s->rps && i * s->rps + j < s->height; j++) {
> > +                memcpy(zbuf + j * bpr,
> > +                       p->data[0] + (i * s->rps + j) * p->linesize[0],
> > +                       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(strip_sizes);
> > +                av_free(strip_offsets);
> > +                av_free(zbuf);
> > +                return -1;
> > +            }
> > +            ptr += n;
> > +            strip_sizes[i] = ptr - buf - strip_offsets[i] + s->offset;
> > +        }
>
> whats the strip loop good for? deflate is encoded as single strip
>
> also the av_free(strip*) return -1 occurs 3 times in the code

I moved strip* to TiffContext and now av_free is in tiff_encoder_end.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: tiff.patch
Type: text/x-diff
Size: 36504 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070331/9664080d/attachment.patch>