[Ffmpeg-devel] tiff encoder (qualification task for GSoC)
Bartlomiej Wolowiec
b.wolowiec
Sun Apr 1 13:08:52 CEST 2007
On Saturday 31 March 2007 23:10, Michael Niedermayer wrote:
> > > > > > + case TIFF_PACKBITS:
> > > > > > +
> > > > > > + from = src;
> > > > > > + val = *(src++);
> > > > > > + while (src < end) {
> > > > > > + // max 128 bytes in direct copy
> > > > > > + if (src - from >= 127) {
> > > > > > + *(dst++) = src - from - 1;
> > > > > > + memcpy(dst, from, src - from);
> > > > > > + dst += src - from;
> > > > > > + from = src;
> > > > > > + }
> > > > > > + if (val == *src) {
> > > > > > + // how long is run ?
> > > > > > + run_end = src;
> > > > > > + while (run_end < end && *run_end == val
> > > > > > + && run_end - from < 128)
> > > > > > + 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;
> > > > > > + }
> > > > > > + }
> > > > >
> > > > > this is duplicate relative to targaenc.c
> > > >
> > > > But targa_encode_rle use a little different coding. It takes the bpp
> > > > argument and my tests showed that it is slower for bpp=1...
> > >
> > > well, merge both codes into a generic rle encoder in rle.c/h and use
> > > that there are many codecs which use rle duplicating it for each is not
> > > ok and dont hesitate to optimize rle.c/h if you want though iam not
> > > insisting on this, what i insist on though is that code duplication is
> > > not ok
> >
> > Ok, I added rle.c/h files, modified targaenc.c and my code (I know that
> > it should be in different patch, but as a qualification task I send it in
> > one).
>
> ok, though i would prefer splited patches
> for the actual SOC having well seperated and reviewable changes is also
> important, otherwise i and the mentors would have alot of problems
> following development (that was one of many problems with last years SOC)
Ok, I divided it into two patches.
> > *
> > * This file is part of FFmpeg.
> > *
> > @@ -20,6 +19,7 @@
> > *
> > */
> > #include "avcodec.h"
> > +#include "rle.h"
> >
> > /**
> > * Count up to 127 consecutive pixels which are either all the same or
> > @@ -62,138 +62,37 @@
> > * @param bpp Bytes per pixel
> > * @param w Image width
> > * @param h Image height
> > + * @param targa_style 1 - targa style ( repeat: 0x80 | (count - 1) ), 0
> > - tiff style ( repeat: -(count - 1) ) * @return Size of output in bytes,
> > or -1 if larger than out_size */
> > -static int targa_encode_rle(uint8_t *outbuf, int out_size, AVFrame *pic,
> > - int bpp, int w, int h)
> > +int rle_encode(uint8_t *outbuf, int out_size, uint8_t *inbuf, int bpp,
> > int w, int targa_style) {
> > - int count, x, y;
> > - uint8_t *ptr, *line, *out;
> > + int count, x;
> > + uint8_t *out;
> >
> > out = outbuf;
> > - line = pic->data[0];
> >
> > - for(y = 0; y < h; y ++) {
> > - ptr = line;
> > + for(x = 0; x < w; x += count) {
> > + /* see if we can encode the next set of pixels with RLE
> > */ + if((count = count_pixels(inbuf, w-x, bpp, 1)) > 1) {
> > + if(out + bpp + 1 > outbuf + out_size) return -1;
> > + if(targa_style)
> > + *out++ = 0x80 | (count - 1);
> > + else
> > + *out++ = -(count - 1);
> > + memcpy(out, inbuf, bpp);
> > + out += bpp;
> > + } else {
> > + /* fall back on uncompressed */
> > + count = count_pixels(inbuf, w-x, bpp, 0);
> > + *out++ = count - 1;
> >
> > - 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(out + bpp + 1 > outbuf + out_size) return -1;
>
> please dont reindent code, also please use 4 space indention like in the
> rest of libav*
> ... how should i know what you changed if diff cant match the lines up
> due to indention changes ...
The main problem is, that the oryginal code processed all lines, but the new
function is processing single line, so almost all indentions moved 4 space
back.
>
> > + for (i = 0; i < (count * ts) / 2; i++) {
> > + FFSWAP(uint8_t, *(ptr + i), *(ptr + count * ts - i - 1));
> > + }
> > + tnput(&entries_ptr, count, ptr_val, type, ts - 1);
>
> are you sure this is correct? i cant help but it looks quite odd to me
> also i cant find anything in the spec which would require that
> it rather looks a simple
> tnput(&entries_ptr, count, ptr_val, type, 0);
> might do the correct thing?
There, you're right. When I had problems with code, the answer from tiffdump
from libtiff (http://www.remotesensing.org/libtiff/) suggested me the
solution, but unfortunately there was a bug and the values were showed
reverse.
>
>
> [...]
>
> > + uint16_t bpp_tab[] = { 8, 8, 8, 8 };
>
> const static
>
Corrected. Earlier add_entry function couldn't have const argument.
Thank you for a great patience during correction of my bugs :)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: rle.patch
Type: text/x-diff
Size: 10423 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070401/1da7ef58/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: tiff.patch
Type: text/x-diff
Size: 30147 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070401/1da7ef58/attachment-0001.patch>
More information about the ffmpeg-devel
mailing list