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

Michael Niedermayer michaelni
Fri Mar 30 23:09:43 CEST 2007


Hi

On Fri, Mar 30, 2007 at 06:32:16PM +0200, Bartlomiej Wolowiec wrote:
> On Sunday 25 March 2007 21:00, Michael Niedermayer wrote:
> > 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
> 
> Currently I write everything which can be written and I add to s->entries 
> tag in which data should be written and offset corrected. I hope, that this 
> solution is satisfying.

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


[...]
> > [...]
> >
> > > +#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
> >
> > [...]
> According to the suggestion refering to the second tiff encoder implementation 
> I decided that currently I shouldn't engage myself with the case of putting 
> many pictures in one file. Furthermore, I'm not working on jpeg and LZW 
> because I try to improve my code, that it could be accepted. 

good


> In the future I 
> would like to write a patch adding this functions. In new version of patch I 
> only added support for YUV444P, YUV422P i YUV411P.
> 
> Best wishes.

> Index: libavcodec/allcodecs.c
> ===================================================================
> --- libavcodec/allcodecs.c	(wersja 8556)
> +++ libavcodec/allcodecs.c	(kopia robocza)
> @@ -131,7 +131,7 @@
>      REGISTER_ENCDEC (TARGA, targa);
>      REGISTER_DECODER(THEORA, theora);
>      REGISTER_DECODER(TIERTEXSEQVIDEO, tiertexseqvideo);
> -    REGISTER_DECODER(TIFF, tiff);
> +    REGISTER_ENCDEC(TIFF, tiff);
>      REGISTER_DECODER(TRUEMOTION1, truemotion1);

diego will likely want the ( to be aligned vertically ...


[...]

> Index: libavcodec/avcodec.h
> ===================================================================
> --- libavcodec/avcodec.h	(wersja 8556)
> +++ libavcodec/avcodec.h	(kopia robocza)
> @@ -2331,6 +2331,7 @@
>  extern AVCodec theora_decoder;
>  extern AVCodec tiertexseqvideo_decoder;
>  extern AVCodec tiff_decoder;
> +extern AVCodec tiff_encoder;
>  extern AVCodec truemotion1_decoder;
>  extern AVCodec truemotion2_decoder;
>  extern AVCodec truespeech_decoder;

the list of encoders and decoders is seperate, yes i know this is silly
nitpicking


[...]

> @@ -17,32 +16,40 @@
>   * You should have received a copy of the GNU Lesser General Public
>   * License along with FFmpeg; if not, write to the Free Software
>   * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> - *
>   */

cosmetic change


[...]
>  /* abridged list of TIFF tags */
> -enum TiffTags{
> +enum TiffTags {

cosmetic change


[...]
>  /** sizes of various TIFF field types */
> -static const int type_sizes[6] = {
> -    0, 1, 100, 2, 4, 8
> +static const int type_sizes[TIFF_TYPE_NB] = {
> +        [TIFF_BYTE] = 1,
> +        [TIFF_STRING] = 100, // org from tiff.c
> +        [TIFF_SHORT] = 2,
> +        [TIFF_LONG] = 4,
> +        [TIFF_RATIONAL] = 8,
>  };

cosmetic

[...]
> +    int tiff_format;            ///< 0 - raw format, 1 - tiff format

unused

[...]
> +#include "avcodec.h"
> +#ifdef CONFIG_ZLIB
> +#include <zlib.h>
> +#endif
> +#include "bytestream.h"
> +#include "tiff.h"
> +
> +#define TIFF_MAX_ENTRY 32
> +
> +#define isYUV(x) ((x)==PIX_FMT_YUV410P || (x)==PIX_FMT_YUV420P    \
> +                            || (x)==PIX_FMT_YUV411P || (x)==PIX_FMT_YUV422P \
> +                            || (x)==PIX_FMT_YUV444P)
> +
> +/// 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()



> +
> +/**
> + * 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


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



> +        } else {
> +            switch (entry->type) {
> +            case TIFF_BYTE:
> +                bytestream_put_byte(buf, entry->off);
> +                break;
> +            case TIFF_SHORT:
> +                bytestream_put_le16(buf, entry->off);
> +                break;
> +            case TIFF_LONG:
> +                bytestream_put_le32(buf, entry->off);
> +                break;
> +            default:
> +                assert(0 && "not implemented");
> +            }

duplicate of tnput()


[...]

> +/**
> + * Add entry to directory in tiff header (pointer version)
> + *
> + * @param s Tiff context
> + * @param buf Pointer to pointer to actual position in buffer
> + * @param tag Tag that identifies the entry
> + * @param type Entry type
> + * @param count The number of values
> + * @param ptr_val Pointer to values
> + */
> +static void add_entryp(TiffEncoderContext * s, uint8_t ** buf,
> +                       enum TiffTags tag, enum TiffTypes type, int count,
> +                       void *ptr_val)
> +{
> +    add_entry(s, buf, tag, type, count, -1, ptr_val);
> +}
> +
> +/**
> + * Add entry to directory in tiff header (value version)
> + *
> + * @param s Tiff context
> + * @param buf Pointer to pointer to actual position in buffer
> + * @param tag Tag that identifies the entry
> + * @param type Entry type
> + * @param val Value
> + */
> +static void add_entryv(TiffEncoderContext * s, uint8_t ** buf,
> +                       enum TiffTags tag, enum TiffTypes type, int val)
> +{
> +    add_entry(s, buf, tag, type, 1, val, NULL);
> +}

these wraper functions are useless


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


[...]
> +    } else if ((avctx->compression_level > 2) && !isYUV(avctx->pix_fmt)) {

superfluous ()


[...]
> +    s->width = avctx->width;
> +    s->height = avctx->height;
> +
> +    if (!s->width || !s->height) {
> +        av_log(s->avctx, AV_LOG_ERROR, "Width or Height is zero\n");
> +        return -1;
> +    }
> +

this check is not usefull, such checks if they are missing in the
generic code have to be added to the generic code


> +    switch (avctx->pix_fmt) {
> +    case PIX_FMT_YUV411P:
> +    case PIX_FMT_YUV422P:
> +    case PIX_FMT_YUV444P:
> +        s->bpp =
> +            ((YUV_Y[avctx->pix_fmt] + 2) << 3) / YUV_Y[avctx->pix_fmt];
> +        s->bpp_tab_size = 3;
> +        s->bpp_tab[0] = 8;
> +        s->bpp_tab[1] = 8;
> +        s->bpp_tab[2] = 8;
> +        s->invert = 6;
> +        break;
> +    case PIX_FMT_RGB24:
> +        s->bpp = 24;
> +        s->bpp_tab_size = 3;
> +        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->bpp_tab_size = 1;
> +        s->bpp_tab[0] = 8;
> +        s->invert = 1;
> +        break;
> +    case PIX_FMT_PAL8:
> +        s->bpp = 8;
> +        s->bpp_tab_size = 1;
> +        s->bpp_tab[0] = 8;
> +        s->invert = 3;
> +        break;
> +    case PIX_FMT_MONOBLACK:
> +        s->bpp = 1;
> +        s->bpp_tab_size = 0;
> +        s->invert = 1;
> +        break;
> +    case PIX_FMT_MONOWHITE:
> +        s->bpp = 1;
> +        s->bpp_tab_size = 0;
> +        s->invert = 0;
> +        break;
> +    default:
> +        av_log(s->avctx, AV_LOG_ERROR,
> +               "This colors format is not supported\n");
> +        return -1;
> +    }

this still can be simplified


[...]

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


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


[...]

> +        b_width = (isYUV(avctx->pix_fmt)) ?
> +            ((((s->width + YUV_Y[avctx->pix_fmt] -
> +                1) / YUV_Y[avctx->pix_fmt]) * YUV_Y[avctx->pix_fmt]) +
> +             ((s->width + YUV_Y[avctx->pix_fmt] -
> +               1) / YUV_Y[avctx->pix_fmt]) * 2) : ((s->width * s->bpp +
> +                                                    7) >> 3);

obfusated, noone will know what this does


[...]
> +                        *(line_ptr++) =

superfluous ()


> +                            *(p->data[0] + i * p->linesize[0] + j * yuv_y +
> +                              k);

p->data[0][i * p->linesize[0] + j * yuv_y + k];



[...]

> +        uint32_t yuvt1[6] = { 299, 1000, 587, 1000, 114, 1000 };        //CCIR recomendation 600-1 (used in ffmpeg?)

static const


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

why do you use malloc() here ? a simple local array seems all that is needed


[...]
> +    s->offset += ptr - buf;

this will break encoding of more then 1 tiff image, that is several files
with one image in each


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

it is not once nor twice but times without number that the same ideas make
their appearance in the world. -- Aristotle
-------------- 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/20070330/05a7d6c4/attachment.pgp>



More information about the ffmpeg-devel mailing list