[Ffmpeg-devel] [PATCH] TIFF encoder (Google SoC qualification task)

Justin Ruggles justinruggles
Fri Mar 23 05:28:54 CET 2007


Kamil Nowosad wrote:
> Writing a TIFF encoder is one of your SoC qualification tasks, and I am
> now submitting what i have yet done.

It's great to see more students getting involved.  Just make sure to get
your application in to Google by Saturday.

> Apart from libavcodec, I have also updated the libavformat, so that it
> is possible to include many images in one TIFF file.
> Each image is divided into strips [of size ~8kB] and then compressed
> using zlib or [when configured without zlib] packbits compression.
> 
> My TiffEncoderContext is small. I don't really need [yet] more than this
> one field [file_offset]. Is it ok, or should I extend the structure?
> 
> It supports now only rgb24 pixel format, but i'm going to change that
> until the deadline.
> 
> Are there any features that you find especially important to be
> implemented here? I can also improve the decoder. I think I could add
> the multiple_imares_per_one_file support.
> 
> I'm waiting for your suggestions.

I mentioned these in the review of the other TIFF patch, but I'll
mention them here, too.  Features which might be good to have:
horizontal predictor
grayscale and 1-bit
yuv
adobe-style jpeg-in-tiff with ljpeg option

Palette image support might be interesting.  There are a couple variants
though, the original TIFF6 and the Adobe technical note.  The latter
would be preferred for encoding since it's newer and can be used with
colorspaces other than RGB.  This gets me reminiscing about experimental
color quantizers...fun stuff.

> +static int tiff_pack_bits(uint8_t *dst, int *dst_len, uint8_t *src, int src_len)
> +{
> +#define MIN(a, b) ((a)<(b) ? (a):(b))
> +    uint8_t *dst_begin = dst,
> +	    *last_literal,
> +	     special_case_ch;
> +    enum {START, RUN, LITERAL, SPECIAL} last;
> +    last = START;
> +    while (src_len > 0){
> +	uint8_t *sp;
> +	int num = 0;
> +	
> +	for (sp = src; (*sp == *src) && (src_len > 0); src++, src_len--)
> +	    num++;
> +
> +	if (dst - dst_begin + (num+127)/128 + 2 >= *dst_len) // check if has enough space
> +	    return -1;
> +
> +	if (num > 2){
> +	    if (last == SPECIAL){
> +		*dst++ = -1;
> +		*dst++ = special_case_ch;
> +	    }
> +	    while (num > 2){ 
> +		*dst++ = (uint8_t)(-MIN(num, 128)+1);
> +		*dst++ = *sp;
> +		num -= 128;
> +	    }
> +	    last = RUN;
> +	}
> +	if (num == 2){ 
> +	    if (last == LITERAL && src_len){
> +		special_case_ch = *sp;
> +		last = SPECIAL;
> +	    }
> +	    else{
> +		if (last == SPECIAL){
> +		    *dst++ = -1;
> +		    *dst++ = special_case_ch;
> +		}
> +		*dst++ = -1;
> +		*dst++ = *sp;
> +		last = RUN;
> +	    }
> +	    num = 0;
> +	}
> +	if (num > 0){
> +#define CHECK_AND_ADD(x, expr){ \ 
> +	    if ((expr) || *last_literal == 127){\
> +		*dst = -1;\
> +		last_literal = dst++;\
> +	    }\
> +	    (*last_literal)++;\
> +	    *dst++ = x;\
> +	    }
> +
> +	    if (last == SPECIAL){
> +		CHECK_AND_ADD(special_case_ch, 0);
> +		CHECK_AND_ADD(special_case_ch, 0);
> +		CHECK_AND_ADD(*sp, 0);
> +	    }
> +	    else 
> +		CHECK_AND_ADD(*sp, last != LITERAL);
> +	    last = LITERAL;
> +#undef CHECK_AND_ADD
> +	}
> +    }
> +#undef MIN 
> +    *dst_len = dst - dst_begin;
> +    return 0;
> +}

It seems to me like your packbits implementation could be simplified a
lot.  None of the implementations I've seen or written have been so complex.

>[...]
> +    /* BPP info: */
> +    bpp_info_offset = CURR_OFFSET;
> +    tiff_put_short(&buf, 8);
> +    tiff_put_short(&buf, 8);
> +    tiff_put_short(&buf, 8);
> +
> +    /* image file directory starts here:  */
> +    tiff_put_long(&entry_point, CURR_OFFSET); 
> +    
> +    /* IFD entries (9): */
> +    tiff_put_short(&buf, 9);  // the number of entries

You might want to think about making this more adaptable.  Say, by
creating a structure for the IFD, adding entries to it, then writing it
to output.  Also, hardcoding the 1 value array currently used seems
simple now, but makes it hard to add more features later.

> +    tiff_add_ifd_entry(&buf, TIFF_WIDTH, TIFF_LONG, 1, avctx->width);
> +    tiff_add_ifd_entry(&buf, TIFF_HEIGHT, TIFF_LONG, 1, avctx->height);
> +    tiff_add_ifd_entry(&buf, TIFF_BPP, TIFF_SHORT, 3, bpp_info_offset); 
> +#ifdef CONFIG_ZLIB
> +    tiff_add_ifd_entry(&buf, TIFF_COMPR, TIFF_SHORT, 1, TIFF_DEFLATE);
> +#else
> +    tiff_add_ifd_entry(&buf, TIFF_COMPR, TIFF_SHORT, 1, TIFF_PACKBITS);
> +#endif
> +    tiff_add_ifd_entry(&buf, TIFF_INVERT, TIFF_SHORT, 1, 2);
> +    if (strip > 1)
> +	tiff_add_ifd_entry(&buf, TIFF_STRIP_OFFS, TIFF_LONG, strip, strip_offsets_offset);
> +    else
> +	tiff_add_ifd_entry(&buf, TIFF_STRIP_OFFS, TIFF_LONG, 1, strip_offset[0]);
> +    tiff_add_ifd_entry(&buf, TIFF_SAMPLESPERPIX, TIFF_SHORT, 1, 3);
> +    tiff_add_ifd_entry(&buf, TIFF_ROWSPERSTRIP, TIFF_LONG, 1, lines_per_strip);
> +    if (strip > 1)
> +	tiff_add_ifd_entry(&buf, TIFF_STRIP_SIZE, TIFF_LONG, strip, strip_sizes_offset);
> +    else
> +	tiff_add_ifd_entry(&buf, TIFF_STRIP_SIZE, TIFF_LONG, 1, avctx->width * avctx->height * 3);
> +
> +    /* end writing */
> +    s->file_offset = CURR_OFFSET;
> +    ret = buf - buf_start;
> +cleanup:
> +    av_free(strip_offset);
> +#undef CURR_OFFSET
> +    return ret;
> +}

You added tag definitions for XRexolution, YResolution, and
ResolutionUnit but aren't using them.  The images might display ok
without them, but they are explicitly required by the TIFF6 spec.

-Justin




More information about the ffmpeg-devel mailing list