[Ffmpeg-devel] [PATCH] TIFF encoder (Google SoC qualification task)
Michael Niedermayer
michaelni
Thu Mar 22 20:32:31 CET 2007
Hi
On Thu, Mar 22, 2007 at 12:01:46PM +0000, Kamil Nowosad wrote:
> Writing a TIFF encoder is one of your SoC qualification tasks, and I am
> now submitting what i have yet done.
>
> 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.
the compression algorithm could be selected by using AVCodecContext.coder_type
also why is the image split into strips? is this needed for tiff spec
compliance or is there some other reason? iam asking simply because it would
be simpler to have just 1 strip ...
also it would be insterresting to encode an image with several different
variants (especially the TIFF_PREDICTOR stuff) and choose the smallest
one allthough this is optional and would have to be in a seperate patch
>
> 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?
please elaborate on what file_offset does, its certainly an odd name for a
variable in an encoder considering that an encoder has nothing to do with
files
>
> 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.
[...]
> diff -upNr ffmpeg/libavcodec/tiff.c ffmpeg-new/libavcodec/tiff.c
> --- ffmpeg/libavcodec/tiff.c 2007-03-22 10:43:09.000000000 +0000
> +++ ffmpeg-new/libavcodec/tiff.c 2007-03-22 11:05:59.000000000 +0000
encoder and decoder should be in seperate files
with common code in a tiff.h and if needed tiff.c while the
encoder would be in tiffenc.c and decoder possibly in tiffdec.c or tiff.c
if there is no common code in tiff.c
(filenames dont really matter, what matters is that encoder ande decoder
are seperated and that common code is shared and not duplicated)
[...]
> @@ -68,7 +72,7 @@ static const int type_sizes[6] = {
> 0, 1, 100, 2, 4, 8
> };
>
> -typedef struct TiffContext {
> +typedef struct TiffDecoderContext {
> AVCodecContext *avctx;
renaming TiffContext->TiffDecoderContext if it is still needed after
seperating the encoder into its own file, should be in a seperate patch
[...]
> @@ -518,15 +526,299 @@ static int tiff_end(AVCodecContext *avct
> return 0;
> }
>
> +static void tiff_put_short(uint8_t **p, uint16_t v)
> +{
> + *(*p)++ = v;
> + *(*p)++ = v >> 8;
> +}
> +
> +static void tiff_put_long(uint8_t **p, uint32_t v)
> +{
> + *(*p)++ = v;
> + *(*p)++ = v >> 8;
> + *(*p)++ = v >> 16;
> + *(*p)++ = v >> 24;
> +}
please use bytestream_put_le16/32()
also trailing whitespace is forbidden in svn
> +
> +static void tiff_add_ifd_entry(uint8_t **ptr, uint16_t tag, uint16_t type, uint32_t count, uint32_t value)
> +{
> + tiff_put_short(ptr, tag);
> + tiff_put_short(ptr, type);
> + tiff_put_long(ptr, count);
> + tiff_put_long(ptr, value);
> +}
> +
> +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))
please use FFMIN
> + uint8_t *dst_begin = dst,
> + *last_literal,
tabs are forbidden in svn
> + 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;\
> + }
indention of } isnt matching
> +
> + 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
i think the loop would encode 1 0 0 1 1 0 to
0 1 -1 0 -1 1 0 0
even though this can be encoded with 1 byte less:
5 1 0 0 1 1 0
either way the code above looks like its doing nearly the same of what
targa_encode_rle() does though its implementation is very different
please use targa_encode_rle()
(or change targaenc.c to use your
code after fixing your code to work optimally and after benchmarking
targaenc.c with START/STOP_TIMER() to ensure that the change doesnt
slow it down)
[...]
> + uint8_t *buf_start = buf,
> + *buf_end = buf+buf_size;
[...]
> + buf_start = buf;
> + buf_end = buf + buf_size;
redundant
> +
> + entry_point = buf;
> + buf += 4; // a place for next IFD offset [later overwritten]
> +
> + /* compute the number of strips */
> + if (avctx->width == 0 || avctx->height == 0){
> + num_of_strips = 0;
> + lines_per_strip = 0;
> + }
width or height cannot be 0 if they are thats an error
> + else{
> + lines_per_strip = (8192 + avctx->width * 3 - 1)/(avctx->width * 3); // =ceil(height/num_of_strips)
> + num_of_strips = (avctx->height + lines_per_strip - 1)/lines_per_strip;
> + }
> +
> + /* writing the image data */
> + strip_offset = (int*) av_malloc((num_of_strips + 1) * sizeof(int));
unneeded cast
[...]
> +AVCodec tiff_encoder = {
> + "tiff",
> + CODEC_TYPE_VIDEO,
> + CODEC_ID_TIFF,
> + sizeof(TiffEncoderContext),
> + tiff_encoder_init,
> + encode_frame,
> + tiff_encoder_end,
> + NULL,
> + 0,
> + NULL
> +};
.pix_fmts= (enum PixelFormat[]){PIX_FMT_RGB24, -1},
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
I have often repented speaking, but never of holding my tongue.
-- Xenocrates
-------------- 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/20070322/7800ee57/attachment.pgp>
More information about the ffmpeg-devel
mailing list