[Ffmpeg-devel] tiff encoder (qualification task for GSoC)
Michael Niedermayer
michaelni
Sun Apr 1 20:52:28 CEST 2007
Hi
On Sun, Apr 01, 2007 at 05:32:55PM +0200, Bartlomiej Wolowiec wrote:
> Hi,
> I hope that all that you've suggested is already in code.
>
> Best Regards,
> Bartek
[...]
> -/** sizes of various TIFF field types */
> +/** sizes of various TIFF field types (string size = 100)*/
> static const int type_sizes[6] = {
> 0, 1, 100, 2, 4, 8
> };
>
> -typedef struct TiffContext {
> +/** sizes of various TIFF field types (string size = 1)*/
> +static const int type_sizes2[6] = {
> + 0, 1, 1, 2, 4, 8
> +};
uint8_t or unsigned char would safe a little space for these tables
also they should ideally not end up duplicated in both encoder and
decoder but i think its better to leave it as is for now and change
that later so i dont have to reread the 2 tiff encoder patches,
another 100 times, i just wanted to mention it
> +
> +typedef struct TiffEncoderContext {
> AVCodecContext *avctx;
> AVFrame picture;
>
> - int width, height;
> - unsigned int bpp;
> - int le;
> - int compr;
> - int invert;
> + int width; ///< picture width
> + int height; ///< picture height
> + unsigned int bpp; ///< bits per pixel
> + int compr; ///< compression level
> + int bpp_tab_size; ///< bpp_tab size
> + int invert; ///< photometric interpretation
> + int strips; ///< number of strips
> + int rps; ///< row per strip
> + uint8_t *entries; ///< entires in header
> + int num_entries; ///< number of entires
> + uint8_t **buf; ///< actual position in buffer
> + uint8_t *buf_start; ///< pointer to first byte in buffer
> + int buf_size; ///< buffer size
> +} TiffEncoderContext;
>
> - int strips, rps;
> - int sot;
> - uint8_t* stripdata;
> - uint8_t* stripsizes;
> - int stripsize, stripoff;
> - LZWState *lzw;
> -} TiffContext;
well, either put the encoder struct in tiffenc.c or use the same context
struct for both encoder and decoder, they seem fairly similiar anyway
[...]
> +/**
> + * Encode one strip in tiff file
> + *
> + * @param s Tiff context
> + * @param src Input buffer
> + * @param dst Output buffer
> + * @param n Size of input buffer
> + * @param compr Compression method
> + * @return Number of output bytes. If an output error is encountered, -1 returned
> + */
> +
> +static int encode_strip(TiffEncoderContext * s, const int8_t * src,
> + uint8_t * dst, int n, int compr)
> +{
> +
> + switch (compr) {
> +#ifdef CONFIG_ZLIB
> + case TIFF_DEFLATE:
> + case TIFF_ADOBE_DEFLATE:
> + {
> + unsigned long zlen = n + 128;
> + if (check_size(s, zlen))
> + return -1;
> + if (compress(dst, &zlen, src, zlen) != Z_OK) {
> + av_log(s->avctx, AV_LOG_ERROR, "Compressing failed\n");
> + return -1;
> + }
> + return zlen;
> + }
> +#endif
> + case TIFF_RAW:
> + if (check_size(s, n))
> + return -1;
> + memcpy(dst, src, n);
> + return n;
> + case TIFF_PACKBITS:
> + if (check_size(s, ((n >> 7) + 1) * 129))
> + return -1;
> +
> + return ff_rle_encode(dst, ((n >> 7) + 1) * 129, src, 1, n, 0);
> + default:
> + return -1;
> + }
> +}
the encode_strip() function could be inlined, this would simplify the code
though iam not sure if its a good idea maybe its cleaner as is so
leave it if you prefer
[...]
> + if (avctx->pix_fmt == PIX_FMT_PAL8) {
> + uint16_t pal[256 * 3];
> + for (i = 0; i < 256; i++) {
> + uint32_t rgb = *(uint32_t *) (p->data[1] + i * 4);
> + pal[i] = ((rgb >> 16) & 0xff) * 257;
> + pal[i + 256] = ((rgb >> 8) & 0xff) * 257;
> + pal[i + 512] = (rgb & 0xff) * 257;
the & 0xff) could be aligned (another silly nitpick i know ...)
[...]
> +static int tiff_encoder_init(AVCodecContext * avctx)
> +{
> + TiffEncoderContext *s = avctx->priv_data;
> + s->entries = av_malloc(TIFF_MAX_ENTRY * 12);
> + return 0;
> +}
> +
> +static int tiff_encoder_end(AVCodecContext * avctx)
> +{
> + TiffEncoderContext *s = avctx->priv_data;
> + av_free(s->entries);
> + return 0;
> +}
s->entries doesnt need to be allocated by malloc(), it seems ive missed that
also these 2 functions become useless after that ...
[...]
> for(x = 0; x < w; x += count) {
> /* see if we can encode the next set of pixels with RLE */
> - if((count = count_pixels(ptr, w-x, bpp, 1)) > 1) {
> + if((count = count_pixels(inbuf, w-x, bpp, 1)) > 1) {
please dont rename ptr to inbuf
> if(out + bpp + 1 > outbuf + out_size) return -1;
> +
> + if(targa_style)
> *out++ = 0x80 | (count - 1);
> - memcpy(out, ptr, bpp);
> + else
> + *out++ = -(count - 1);
well, as i already said to the other student, this can be done without the
branch and more generically, actually i could think of 3 ways to do this simpler
and the look up table variant is not the one i want because theres a much
simpler solution which should be both fast and support all sane variants
to store this byte
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
In a rich man's house there is no place to spit but his face.
-- Diogenes of Sinope
-------------- 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/20070401/ae80343e/attachment.pgp>
More information about the ffmpeg-devel
mailing list