[Ffmpeg-devel] [PATCH] TIFF encoder (Google SoC qualification task)

Michael Niedermayer michaelni
Thu Mar 29 16:51:25 CEST 2007


Hi

On Thu, Mar 29, 2007 at 09:55:30AM +0000, Kamil Nowosad wrote:
> On Thu, Mar 29, 2007 at 01:39:17AM +0200, Michael Niedermayer wrote:
> > Hi
> > 
> > are you going to send a second revission of your tiff encoder patch, which
> > takes care of the review comments?
> > or did you decide to work on some other task?
> 
> Hi,
> Yes, I do. I have made some changes, according to the comments. However, I have planned to submit more extended version (this week I have two big tests on my studies, and it was impossible for me to do more, but I have planned to start work on Saturday (the whole time from Sat. until the deadline is free for me, so I can do my best :-) )) and then make the final fixes, after the review. I have forgotten what you have written in the "Patch review process" section in the documentation, sorry.
> 
> I have attached the patch. The next steps, according to the reviews,
> are:
> 
> - YUV support
> - support for horizontal predictor (and maybe LZW compression)
> - support for JPEG compression
> - optional choice of the best compression algorithm for every image
> - maybe some others ;-)

i would recommand against adding LZW and JPEG compression simply because iam
afraid that this could get tricky with googles deadlines, you can always
submit these as seperate patches if you like after the tiff encoder passed
review
and additional features should be in seperate patches ideally anyway ...


> 
> You have written that new files (like tiff.h) have to be diffed against
> the parent file. Do you mean doing svn copy tiff.c tiff.h insted of svn
> add tiff.h ?

yes


[...]
> Index: libavcodec/utils.c
> ===================================================================
> --- libavcodec/utils.c	(wersja 8542)
> +++ libavcodec/utils.c	(kopia robocza)
> @@ -636,6 +636,9 @@
>  {"coder", NULL, OFFSET(coder_type), FF_OPT_TYPE_INT, DEFAULT, INT_MIN, INT_MAX, V|E, "coder"},
>  {"vlc", "variable length coder / huffman coder", 0, FF_OPT_TYPE_CONST, FF_CODER_TYPE_VLC, INT_MIN, INT_MAX, V|E, "coder"},
>  {"ac", "arithmetic coder", 0, FF_OPT_TYPE_CONST, FF_CODER_TYPE_AC, INT_MIN, INT_MAX, V|E, "coder"},
> +{"raw", "raw (no encoding)", 0, FF_OPT_TYPE_CONST, FF_CODER_TYPE_RAW, INT_MIN, INT_MAX, V|E, "coder"},
> +{"rle", "run-lenghth coder", 0, FF_OPT_TYPE_CONST, FF_CODER_TYPE_RL, INT_MIN, INT_MAX, V|E, "coder"},
> +{"def", "deflate", 0, FF_OPT_TYPE_CONST, FF_CODER_TYPE_DEFLATE, INT_MIN, INT_MAX, V|E, "coder"},

i would prefer if this where called "deflate" becasue "def" could be
missunderstood as "default"


>  {"context", "context model", OFFSET(context_model), FF_OPT_TYPE_INT, DEFAULT, INT_MIN, INT_MAX, V|E},
>  {"slice_flags", NULL, OFFSET(slice_flags), FF_OPT_TYPE_INT, DEFAULT, INT_MIN, INT_MAX},
>  {"xvmc_acceleration", NULL, OFFSET(xvmc_acceleration), FF_OPT_TYPE_INT, DEFAULT, INT_MIN, INT_MAX},
[...]
> Index: libavcodec/avcodec.h
> ===================================================================
> --- libavcodec/avcodec.h	(wersja 8542)
> +++ libavcodec/avcodec.h	(kopia robocza)
> @@ -1565,8 +1565,11 @@
>       */
>      int global_quality;
> 
> -#define FF_CODER_TYPE_VLC   0
> -#define FF_CODER_TYPE_AC    1
> +#define FF_CODER_TYPE_VLC       0
> +#define FF_CODER_TYPE_AC        1
> +#define FF_CODER_TYPE_RAW       2  // no coder
> +#define FF_CODER_TYPE_RL        3  // run-length

id prefer FF_CODER_TYPE_RLE as RLE is a well known term


[...]

> +#ifdef CONFIG_ZLIB
> +const enum TiffCompr tiff_default_compression = TIFF_DEFLATE;
> +#else
> +const enum TiffCompr tiff_default_compression = TIFF_PACKBITS;
> +#endif

a #define TIFF_DEFAULT might be a better idea


> +
> +static int value_size(uint16_t type, uint32_t count, uint8_t * value)
> +{
> +    switch (type) {
> +    case TIFF_BYTE:
> +        return count;
> +    case TIFF_SHORT:
> +        return 2 * count;
> +    case TIFF_LONG:
> +        return 4 * count;
> +    case TIFF_LONGLONG:
> +        return 8 * count;
> +    case TIFF_STRING:
> +        return strlen(value) + 1;
> +    }
> +    return 0;                   // never reached
> +}

this can be done much simpler with a static const array mapping the 5 types
to their size


> +
> +/**
> + * adds an entry to the IFD (referenced by a pointer)
> + */
> +static int tiff_add_ifd_entryp(AVCodecContext * avctx, uint16_t tag,
> +                                uint16_t type, uint32_t count,
> +                                uint8_t * value)
> +{
> +    TiffEncoderContext *s = avctx->priv_data;
> +    int size = value_size(type, count, value);
> +    TiffIfdEntry *entr;
> +
> +    if (s->num_ifd_entries == s->max_ifd_entries) {     // time to enlarge the buffer
> +        s->max_ifd_entries *= 2;
> +        s->ifd_entries =
> +            av_realloc(s->ifd_entries,
> +                       s->max_ifd_entries * sizeof(TiffIfdEntry));
> +        if (s->ifd_entries == NULL) {
> +            av_log(avctx, AV_LOG_ERROR, "not enough memory\n");
> +            return -1;
> +        }
> +    }

as the number of entries seems constant dynamic alocation and reallocation
isnt needed


> +
> +    entr = s->ifd_entries + s->num_ifd_entries;
> +    entr->tag = tag;
> +    entr->type = type;
> +    entr->count = count;
> +    if (size > 4) {
> +        if (s->buf + size >= s->buf_end){
> +            av_log(avctx, AV_LOG_ERROR, "the buffer given is too short\n");
> +            return -1;
> +        }
> +        entr->value = CURR_OFFSET(s);
> +        memcpy(s->buf, value, size);
> +        s->buf += size;
> +    } else {
> +        entr->value = 0;
> +        memcpy((uint8_t *) & entr->value, value, size);

the cast is unneeded


> +    }
> +    s->num_ifd_entries++;
> +    return 0;
> +}

also why do you build the IFD in memory as TiffIfdEntry objects and then
write it out, why not write each entry directly?


> +/**
> + * adds an entry to the IFD (referenced by a value)
> + * [it is helpful when passing constants]
> + */
> +static int tiff_add_ifd_entryv(AVCodecContext * avctx, uint16_t tag,
> +                                uint16_t type, uint32_t count,
> +                                uint32_t value)
> +{
> +    if (type == TIFF_BYTE) {
> +        uint8_t x = value;
> +        return tiff_add_ifd_entryp(avctx, tag, type, count, &x);
> +    } else if (type == TIFF_SHORT) {
> +        uint16_t x = value;
> +        return tiff_add_ifd_entryp(avctx, tag, type, count, &x);
> +    } else if (type == TIFF_LONG) {
> +        uint32_t x = value;
> +        return tiff_add_ifd_entryp(avctx, tag, type, count, &x);
> +    }
> +    return -1;
> +}

this function is unneeded

a simple
tiff_add_ifd_entryp(avctx, tag, type, count, (uint16_t[]){value});
will work too


[...]
> +/**
> + * packbits compression implementation
> + */
> +static int tiff_pack_bits(uint8_t * dst, int *dst_len, uint8_t * src,
> +                          int src_len)
> +{
> +    uint8_t *dst_begin = dst, *last_literal, special_case_ch;
> +    enum { START, RUN, LITERAL, SPECIAL } last;
> +    last = START;
> +    while (src_len > 0) {
> +        uint8_t *sp;
> +        int num = 1;
> +
> +        for (sp = src + 1, src_len--; (*sp == *src) && (src_len > 0);
> +             src++, src_len--)
> +            num++;
> +
> +        if (dst - dst_begin + (num + 127) / 128 + 2 >= *dst_len)        // check if has enough space
> +            return -1;
> +
> +
> +#define CHECK_AND_ADD(x, expr){ \
> +            if ((expr) || *last_literal == 127){\
> +                *dst = -1;\
> +                last_literal = dst++;\
> +            }\
> +            (*last_literal)++;\
> +            *dst++ = x;\
> +        }
> +        if (num == 1) {
> +            if (last == SPECIAL) {
> +                CHECK_AND_ADD(special_case_ch, 0);
> +                CHECK_AND_ADD(special_case_ch, 0);
> +                CHECK_AND_ADD(*sp, 0);
> +            } else
> +                CHECK_AND_ADD(*sp, last != LITERAL);
> +            last = LITERAL;
> +        } else if (num == 2) {
> +            switch (last) {
> +            case LITERAL:
> +                special_case_ch = *sp;
> +                last = SPECIAL;
> +                break;
> +            case SPECIAL:
> +                CHECK_AND_ADD(special_case_ch, 0);
> +                CHECK_AND_ADD(special_case_ch, 0);
> +                special_case_ch = *sp;
> +            default:
> +                *dst++ = -1;
> +                *dst++ = *sp;
> +                last = RUN;
> +            }
> +        } else {
> +            if (last == SPECIAL) {
> +                *dst++ = -1;
> +                *dst++ = special_case_ch;
> +            }
> +            while (num > 0) {
> +                *dst++ = (uint8_t) (-FFMIN(num, 128) + 1);
> +                *dst++ = *sp;
> +                num -= 128;
> +            }
> +            last = RUN;
> +        }
> +    }

this code would encode 0 0 1 to -2 0 0 1
which is wrong as that doesnt decode to 0 0 1

furthermore its a duplicate of the rle encoder in targaenc.c
please use the code from targaenc.c or if you prefer replace the code in
targaenc.c with a better implementation and use that then in both targa and
tiff (this of course would need benchmarks)


[...]
> +static int tiff_prepare_color_map(AVCodecContext * avctx)
> +{
> +    TiffEncoderContext *s = avctx->priv_data;
> +    AVFrame *p = s->p;
> +    int i;
> +    uint32_t *src = p->data[1];
> +
> +    s->color_map = av_malloc((1 << s->bpp) * 3 * sizeof(uint16_t));
> +    if (s->color_map == NULL) {
> +        av_log(avctx, AV_LOG_ERROR, "not enough memory\n");
> +        return -1;
> +    }
> +
> +    for (i = 0; i < (1 << s->bpp); i++) {
> +        uint32_t sample = *src;

*src++;
avoids the seperate ++ later


> +        s->color_map[2 * (1 << s->bpp) + i] = (sample & 255) << 8;      // B

2 * (1 << s->bpp) == 2<<s->bpp

> +        sample = sample >> 8;
> +        s->color_map[(1 << s->bpp) + i] = (sample & 255) << 8;  // G
> +        sample = sample >> 8;
> +        s->color_map[i] = (sample & 255) << 8;  // R

white is 65535 for short but 8bit white of 255 with the code above that
reaches 65280 only


[...]
> +    /* compute the number of strips */
> +    if (avctx->width == 0 || avctx->height == 0) {
> +        av_log(avctx, AV_LOG_ERROR, "invalid image size\n");
> +        return -1;

i dont think a encoder init function can be called with width or height
0 that is it will fail some checks before ...


> +    } else if (s->compression == TIFF_DEFLATE) {
> +        s->lines_per_strip = avctx->height;
> +        s->num_of_strips = 1;
> +    } else {
> +        s->lines_per_strip = (8192 + s->bytes_per_line - 1) / (s->bytes_per_line);      // =ceil(height/num_of_strips)

superfluous ()


[...]
> +    case TIFF_RAW:
> +        if (s->buf + avctx->height * s->bytes_per_line >= s->buf_end){
> +            av_log(avctx, AV_LOG_ERROR, "the buffer given is too short\n");
> +            return -1;
> +        }
> +        for (y = 0; y < avctx->height; y += s->lines_per_strip) {
> +            uint8_t *src;
> +            int i;
> +
> +            s->strip_offset[s->strip++] = CURR_OFFSET(s);
> +            for (i = 0;
> +                 (i < s->lines_per_strip) && (y + i < avctx->height);
> +                 i++) {
> +                src = p->data[0] + (y + i) * p->linesize[0];

uint8_t *src= p->data[0] + y * p->linesize[0];
src += p->linesize[0];
moves some opertaions out of the loop


> +                memcpy(s->buf, src, s->bytes_per_line);
> +                s->buf += s->bytes_per_line;

bytestream_put_buffer()


> +            }
> +        }
> +        break;
> +    case TIFF_PACKBITS:
> +        for (y = 0; y < avctx->height; y += s->lines_per_strip) {
> +            uint8_t *src;
> +            int i;
> +
> +            s->strip_offset[s->strip++] = CURR_OFFSET(s);
> +            for (i = 0;
> +                 (i < s->lines_per_strip) && (y + i < avctx->height);
> +                 i++) {
> +                int plen = s->buf_end - s->buf;
> +                src = p->data[0] + (y + i) * p->linesize[0];
> +                if (tiff_pack_bits(s->buf, &plen, src, s->bytes_per_line)
> +                    == -1) {
> +                    av_log(avctx, AV_LOG_ERROR,
> +                           "packbits compression error\n");
> +                    return -1;
> +                }
> +                s->buf += plen;
> +            }
> +        }

this and the TIFF_RAW case contain some duplicate code


[...]
> +/**
> + * encoding scheme:
> + * @li [if the client has not set it] TIFF signature
> + * @li offset of the IFD
> + * @li image data [divided into strips]
> + * @li IFD data which doesn't fit inside the IFD
> + * @li IFD, sorted by tag
> + * @li [if the client will not set it] 32 zero bits
> + */
> +static int tiff_encode_frame(AVCodecContext * avctx, unsigned char *buf,
> +                             int buf_size, void *data)
> +{
> +    TiffEncoderContext *s = avctx->priv_data;
> +    const uint8_t header[4] = {0x49, 0x49, 42, 0},
> +                  trailer[4] = {0, 0, 0, 0};
> +    uint32_t xres[2] = {72, 1},
> +             yres[2] = {72, 1};
> +    int ret;
> +
> +    s->buf_start = s->buf = buf;
> +    s->buf_end = s->buf + buf_size;
> +    s->p = data;
> +
> +    if (avctx->frame_number == 0) {
> +        /* do we have to write the header? (the TIFF muxer does it
> +         * for us - it is useful when creating TIFFs with multiple images in one file) */
> +        if (!avctx->extradata_size) { // i'm sure that there exists a nicer solution - maybe adding flags?
> +            memcpy(s->buf, header, 4);
> +            s->buf += 4;
> +            s->offset = 0;
> +            s->write_trailer = 1;
> +        } else
> +            s->offset = *(int *) avctx->extradata;
> +    }

how is this supposed to work with stream copy? that is if i take a series
of tiff images and stream copy them (no decoder, no encoder) into a
multi image tiff file?
how is this supposed to work if i take a multi image tiff file and stream
copy that into individual tiff files (no decoder no encoder)?
how is a decoder supposed to decode such individual tiff images, considering
that a decoder sees exactly what the encoder outputs, that is a packet with
offsets pointing relative to the filebegin which is not known 
or accessible to the decoder ...

the easiest solution is to drop multi image support, implementing it cleanly
could take more time than you think, the same is of course true for the
other student working on tiff encoding

you can always send multi tiff support as seperate patch later if you like
and find a way how to cleanly implement it


> +
> +    switch (avctx->pix_fmt) {
> +    case PIX_FMT_PAL8:
> +        s->bpp_info[0] = 8;
> +        s->nsamples = 1;
> +        s->bpp = 8;
> +        s->photometric_interpretation = 3;
> +        s->has_color_map = 1;
> +        break;
> +    case PIX_FMT_RGB24:
> +        s->bpp_info[0] = 8;
> +        s->bpp_info[1] = 8;
> +        s->bpp_info[2] = 8;
> +        s->nsamples = 3;
> +        s->bpp = 24;
> +        s->photometric_interpretation = 2;
> +        break;
> +    case PIX_FMT_GRAY8:
> +        s->bpp_info[0] = 8;
> +        s->nsamples = 1;
> +        s->bpp = 8;
> +        s->photometric_interpretation = 1;
> +        break;
> +    case PIX_FMT_MONOBLACK:
> +        s->bpp_info[0] = 1;
> +        s->nsamples = 1;
> +        s->bpp = 1;
> +        s->photometric_interpretation = 1;
> +        break;
> +    case PIX_FMT_MONOWHITE:
> +        s->bpp_info[0] = 1;
> +        s->nsamples = 1;
> +        s->bpp = 1;
> +        s->photometric_interpretation = 0;
> +        break;
> +    default:
> +        av_log(avctx, AV_LOG_ERROR, "this pix_fmt (%d) is not supported\n",
> +               avctx->pix_fmt);
> +        return -1;
> +    }

this can be simplified


> +
> +    s->bytes_per_line = (avctx->width * s->bpp + 7) / 8;
> +
> +    s->entry_point = s->buf;
> +    s->buf += 4;                // a place for next IFD offset [later updated]
> +
> +    if (s->has_color_map)
> +        if (tiff_prepare_color_map(avctx) < 0) {
> +            return -1;
> +        }
> +
> +    /* write the image data */
> +    if (tiff_write_image_data(avctx)) {
> +        if (s->strip_offset)
> +            av_free(s->strip_offset);
> +        if (s->strip_size)
> +            av_free(s->strip_size);
> +        if (s->has_color_map)
> +            av_free(s->color_map);
> +        return -1;

the NULL checks before av_free are unneeded
also this is duplicate code, the code at the end does the same


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I do not agree with what you have to say, but I'll defend to the death your
right to say it. -- Voltaire
-------------- 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/20070329/423be26d/attachment.pgp>



More information about the ffmpeg-devel mailing list