[Ffmpeg-devel] tiff encoder (qualification task for GSoC)
Bartlomiej Wolowiec
b.wolowiec
Fri Mar 30 18:32:16 CEST 2007
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.
> 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
> --------
ok
>
>
> [...]
>
> > 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
I do "svn copy libavcodec/tiff.c libavcodec/tiff.h".
> [...]
>
> > +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;
>
Done
> [...]
>
> > + case TIFF_LONGLONG:
> > + bytestream_put_le32(p, *((uint32_t *) val));
> > + val += 4;
> > + bytestream_put_le32(p, *((uint32_t *) val));
> > + val += 4;
>
> this is wrong
>
TIFF_LONGLONG changed to TIFF_RATIONAL. Its compatible witch documentation.
>
> [...]
>
> > +/**
> > + * 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
>
I add one function add_entry. Now add_entryp and add_entryv simply use it.
>
> [...]
>
> > + 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
>
It's corrected.
> > +
> > + 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
>
Done.
>
> [...]
>
> > + uint8_t *ptr = (uint8_t *) buf;
>
> unneeded cast
>
>
> [...]
Corrected.
>
> > + 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
>
Done.
>
> [...]
>
> > + 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
In TiffEncoderContext I've changed uint8_t *bpp_tab to uint8_t bpp_tab[4].
> 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
After considering it, I found that s->bps and s->bpr are redundant in the
code. So I deleted them. Needed values come from bpp and other parametres.
> > +
> > + 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
>
Corrected.
>
> [> + 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
I must have missed it and then forgot to delete. :)
>
> [...]
>
> > Index: libavformat/tiff.c
> > ===================================================================
> > --- libavformat/tiff.c (wersja 0)
> > +++ libavformat/tiff.c (wersja 0)
I don't know what I should add, because I created the file from its basis.
> [...]
>
> > +#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. 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: tiff.patch
Type: text/x-diff
Size: 40685 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070330/f9b2dddc/attachment.patch>
More information about the ffmpeg-devel
mailing list