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

Michael Niedermayer michaelni
Sun Mar 25 21:00:07 CEST 2007


Hi

On Sat, Mar 24, 2007 at 04:29:16PM +0100, Bartlomiej Wolowiec wrote:
> Thanks for suggestions about my code. I think that helps me in any future 
> task.
> I improved my implementation packbits, and I to compared performance my 
> packbit and packbit from targacom.c, I set bpp=1 and then my implementation 
> is faster, so in future I rewrite targa_encode_rle, afterwards I checked time 
> differences for another bpp.


> I need to create each entry in memory, because I dont know which is offset. 
> For simple files I can calculate it, but if I want add any entry but i must 
> recalculate it (for example in grayscale and colors file have another 
> entries).

just write each entry immedeatly, and keep a single array of their offsets,
at the end write the IFD and update the pointer in the header which points to
the IFD


> I also add suport for:
> -RGB24, PAL8, GRAY, MONOBLACK and MONOWHITE,
> -many pictures in one tiff file
> 

> How can I get image resolution in ffmpeg api ? In file I set it to 72dpi.

hmm, 72dpi will do for the moment, we can change that later


also when replying to a review please inline your comments after the 
quoted comments from the review that way i can much easier keep track
of things ...
that would look like:
--------
> please change code to xyz

xyz is impossible due to abc
--------


[...]
> Index: libavcodec/tiff.h
> ===================================================================
> --- libavcodec/tiff.h	(wersja 0)
> +++ libavcodec/tiff.h	(wersja 0)

files should always be diffed against their "parent" file,  that makes review
easier, here that would have been tiff.c


[...]

> +typedef struct TiffDirEntry {
> +    /// tag code
> +    enum TiffTags tag;
> +    /// values type
> +    enum TiffTypes type;
> +    /// position of values in file or value if and only if the value fits into 4 bytes
> +    int off;
> +    /// pointer to values
> +    void *val;
> +    /// number of values
> +    int count;
> +} TiffDirEntry;

putting the comments to the right would be more readable

typedef struct TiffDirEntry {
    enum TiffTags tag;  ///< tag code
    enum TiffTypes type;///< values type
    int off;            ///< position of values in file or value if and only if the value fits into 4 bytes
    void *val;          ///< pointer to values
    int count;          ///< number of values
} TiffDirEntry;



[...]

> +        case TIFF_LONGLONG:
> +            bytestream_put_le32(p, *((uint32_t *) val));
> +            val += 4;
> +            bytestream_put_le32(p, *((uint32_t *) val));
> +            val += 4;

this is wrong


[...]

> +/**
> + * Add entry to directory in tiff header (pointer version)
> + *
> + * @param s Tiff context
> + * @param tag Tag that identifies the entry
> + * @param type Entry type
> + * @param count The number of values
> + * @param ptr_val Poitner to values
> + */
> +static void add_entryp(TiffEncoderContext * s, enum TiffTags tag,
> +                       enum TiffTypes type, int count, void *ptr_val)
> +{
> +    if (s->num_entries == TIFF_MAX_ENTRY) {
> +        av_log(s->avctx, AV_LOG_ERROR, "Too many entries in tiff dir!!\n");
> +    } else {
> +        s->entries[s->num_entries].tag = tag;
> +        s->entries[s->num_entries].type = type;
> +        s->entries[s->num_entries].count = count;
> +        s->entries[s->num_entries].off = -1;
> +        s->entries[s->num_entries].val = ptr_val;
> +        s->num_entries++;
> +    }
> +}
> +
> +/**
> + * Add entry to directory in tiff header (value version)
> + *
> + * @param s Tiff context
> + * @param tag Tag that identifies the entry
> + * @param type Entry type
> + * @param count The number of values
> + * @param val Value
> + */
> +static void add_entryv(TiffEncoderContext * s, enum TiffTags tag,
> +                       enum TiffTypes type, int count, int val)
> +{
> +    if (s->num_entries == TIFF_MAX_ENTRY) {
> +        av_log(s->avctx, AV_LOG_ERROR, "Too many entries in tiff dir!!\n");
> +    } else {
> +        s->entries[s->num_entries].tag = tag;
> +        s->entries[s->num_entries].type = type;
> +        s->entries[s->num_entries].count = count;
> +        s->entries[s->num_entries].off = val;
> +        s->entries[s->num_entries].val = NULL;
> +        s->num_entries++;
> +    }
> +}

please avoid code duplication


[...]

> +                while (*run_end == val && run_end - from < 128
> +                       && run_end < end)
> +                    run_end++;

the checks for run_end being within the array should be before
dereferencing 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;
> +            }
> +        }
> +
> +        return dst - org_dst;
> +    default:
> +        av_log(s->avctx, AV_LOG_ERROR,
> +               "Chosen compression is not supported\n");
> +        return -1;
> +    }

> +    return 0;

the return is useless as every case of the switch() ends in a return


[...]

> +    uint8_t *ptr = (uint8_t *) buf;

unneeded cast


[...]
> +    s->compr = TIFF_PACKBITS;
> +    if (avctx->compression_level == 0) {
> +        s->compr = TIFF_RAW;
> +    } else if (avctx->compression_level == 1) {
> +        s->compr = TIFF_PACKBITS;

redundant, compr is already at that value


[...]
> +    switch (avctx->pix_fmt) {
> +    case PIX_FMT_RGB24:
> +        s->bpp = 24;
> +        s->bps = 3 * s->width;
> +        s->bpr = 3 * s->width;
> +        s->bpp_tab_size = 3;
> +        s->bpp_tab = av_malloc(3 * sizeof(short));
> +        s->bpp_tab[0] = 8;
> +        s->bpp_tab[1] = 8;
> +        s->bpp_tab[2] = 8;
> +        s->invert = 2;
> +        break;
> +    case PIX_FMT_GRAY8:
> +        s->bpp = 8;
> +        s->bps = s->width;
> +        s->bpr = s->width;
> +        s->bpp_tab_size = 1;
> +        s->bpp_tab = av_malloc(1 * sizeof(short));
> +        s->bpp_tab[0] = 8;
> +        s->invert = 1;
> +        break;
> +    case PIX_FMT_PAL8:
> +        s->bpp = 8;
> +        s->bps = s->width;
> +        s->bpr = s->width;
> +        s->bpp_tab_size = 1;
> +        s->bpp_tab = av_malloc(1 * sizeof(short));
> +        s->bpp_tab[0] = 8;
> +        s->invert = 3;
> +        break;
> +    case PIX_FMT_MONOBLACK:
> +        s->bpp = 1;
> +        s->bps = s->width;
> +        s->bpr = s->width;
> +        s->bpp_tab_size = 0;
> +        s->bpp_tab = NULL;
> +        s->invert = 1;
> +        break;
> +    case PIX_FMT_MONOWHITE:
> +        s->bpp = 1;
> +        s->bps = s->width;
> +        s->bpr = s->width;
> +        s->bpp_tab_size = 0;
> +        s->bpp_tab = NULL;
> +        s->invert = 0;
> +        break;
> +    default:
> +        av_log(s->avctx, AV_LOG_ERROR,
> +               "This colors format is not supported\n");
> +        return -1;
> +    }

you dont need av_malloc() for allocating a small fixed size buffer, simply
add the array to the stack, also the type short vs. uint16_t missmatches

the s->bps = s->width; s->bpr = s->width; code can be factored out of the
switch

bits per pixel=1 and bytes per line=width seem to missmatch

bps=bpr so one of them can be removed


> +
> +    if (s->compr == TIFF_DEFLATE || s->compr == TIFF_ADOBE_DEFLATE)
> +        //best choose for DEFLATE
> +        s->rps = s->height;
> +    else
> +        s->rps = FFMAX(8192 / (((s->width * s->bpp) >> 3) + 1), 1);     // suggest size of strip
> +
> +    s->bps *= s->rps;
> +
> +    strips = (s->height - 1) / s->rps + 1;
> +
> +    strip_sizes = av_mallocz(4 * strips);
> +    strip_offsets = av_mallocz(4 * strips);
> +
> +    if (buf_size <
> +        (s->height * s->width * s->bpp >> 3) + 128 + TIFF_MAX_ENTRY * 12 +
> +        strips * 8) {
> +        av_log(s->avctx, AV_LOG_ERROR, "Buffer is too small\n");
> +        return -1;
> +    }

i do think that with the worst case overhead from packbits this is not large
enough

also the strip_sizes / strip_offsets leak in the error case


[> +    if (avctx->pix_fmt == PIX_FMT_PAL8) {
> +        uint16_t *pal;
> +        uint16_t *ps;
> +        int rgb;
> +        pal = av_malloc(256 * 3 * sizeof(short));
> +        ps = pal;
> +        for (i = 0; i < 256; i++) {
> +            rgb = *(int *) (p->data[1] + i * 4);
> +            *(pal + i) = ((rgb >> 16) & 0xff) * 257;
> +            *(pal + i + 256) = ((rgb >> 8) & 0xff) * 257;
> +            *(pal + i + 512) = (rgb & 0xff) * 257;
> +        }
> +
> +        rgb = *(int *) (p->data[1] + (0xd7) * 4);
> +        add_entryp(s, TIFF_PAL, TIFF_SHORT, 256 * 3, pal);

the rgb = ... line doesnt seem to do anything

[...]
> Index: libavformat/tiff.c
> ===================================================================
> --- libavformat/tiff.c	(wersja 0)
> +++ libavformat/tiff.c	(wersja 0)
[...]
> +#include "avformat.h"
> +#include "tiff.h"
> +
> +static int tiff_write_header(struct AVFormatContext *s)
> +{
> +    const uint8_t header[] = { 0x49, 0x49, 42, 0 };
> +    TiffEncoderContext *c =
> +        (TiffEncoderContext *) s->streams[0]->codec->priv_data;
> +    c->tiff_format = 1;

libavformat MUST NOT, under ANY circumstances touch the private context of a
codec

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

Opposition brings concord. Out of discord comes the fairest harmony.
-- Heraclitus
-------------- 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/20070325/c60047e3/attachment.pgp>



More information about the ffmpeg-devel mailing list