[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