[Ffmpeg-devel] tiff encoder (qualification task for GSoC)
Michael Niedermayer
michaelni
Sat Mar 31 23:10:00 CEST 2007
Hi
On Sat, Mar 31, 2007 at 09:00:01PM +0200, Bartlomiej Wolowiec 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)
>
> > > > [...]
> > > >
> > > > > + if (buf_size <
> > > > > + (((s->height * s->width * s->bpp >> 3) * 129) >> 7) + 128 +
> > > > > + TIFF_MAX_ENTRY * 12 + strips * 8) {
> > > > > + av_log(s->avctx, AV_LOG_ERROR, "Buffer is too small\n");
> > > > > + return -1;
> > > > > + }
> > > >
> > > > this check is buggy
> > >
> > > Ok, I added check_size to my code.
> >
> > which is more complex and too looks buggy though i didnt check it
> > carefull so i could be wrong ...
> >
>
> I think that it works correctly, I've made tests checking how much memory is
> allocated in comparison with how much is used. For RAW the value was exactly
> the same, of course for packbits and deflate there was diferrence.
hmm, ok, i accept the check_size() if it works, though alternatively you
could do the original check with uint64_t so the multiplications dont
have issues with overflows ..
[...]
> > > + }
> > > +}
> > > +
> > > +/**
> > > + * Write yuv line in tiffs order.
> > > + *
> > > + * @param s Tiff context
> > > + * @param line Line buffer
> > > + * @param y Line position
> > > + * @pram yuv_y Number of bytes of luma corresponding to chrominance
> > > + */
> > > +
> > > +static void make_yuv_line(TiffEncoderContext * s, uint8_t * line, int y,
> > > + int yuv_y)
> > > +{
> > > + uint8_t *line_ptr = line;
> > > + AVFrame *const p = (AVFrame *) & s->picture;
> > > + int j, k;
> > > +
> > > + for (j = 0; j < s->width / yuv_y; j++) {
> > > + memcpy(line_ptr, p->data[0] + y * p->linesize[0] + j * yuv_y,
> > > + yuv_y);
> > > + line_ptr += yuv_y;
> > > + *line_ptr++ = p->data[1][y * p->linesize[1] + j];
> > > + *line_ptr++ = p->data[2][y * p->linesize[2] + j];
> > > +
> > > + }
> > > +
> > > + // write end bytes
> > > + for (k = 0; k < yuv_y; k++) {
> > > + if (j * yuv_y + k < s->width)
> > > + *line_ptr++ = p->data[0][y * p->linesize[0] + j * yuv_y +
> > > k]; + else
> > > + *line_ptr++ = 0;
> > > + }
> > > + *line_ptr++ = p->data[1][y * p->linesize[1] + j - 1];
> > > + *line_ptr = p->data[2][y * p->linesize[2] + j - 1];
> >
> > thinking about this a little, this looks wrong, its converting planar
> > yuv to packed, you rather should support the packed format used in tiff
> > directly and not support the planar ones, and if ffmpeg happens not to
> > support the exact packed format (i didnt check) then the correct action
> > would be either add support in a seperate patch or drop yuv in tiff
> > support for now
> >
>
> tiff uses Y0 Y1 Cb Cr format, the information from libavutil/avutil.h shows
> that ffmpeg doesn't support that YUV format. Currently, I'm deleting support
> for YUV from my implementation of TIFF. In the future I'm willing to write
> the proper patch.
ok
[...]
> Index: libavcodec/rle.c
> ===================================================================
> --- libavcodec/rle.c (wersja 8567)
> +++ libavcodec/rle.c (kopia robocza)
> @@ -1,6 +1,5 @@
> /*
> - * Targa (.tga) image encoder
> - * Copyright (c) 2007 Bobby Bingham
> + * RLE encoder
feel free to add your name if you think you are a main author of the file
but dont remove the existing names / copyright
> *
> * 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 ...
[...]
> Index: libavcodec/rle.h
> ===================================================================
> --- libavcodec/rle.h (wersja 8567)
> +++ libavcodec/rle.h (kopia robocza)
> @@ -1,6 +1,5 @@
> /*
> - * Targa (.tga) image encoder
> - * Copyright (c) 2007 Bobby Bingham
> + * RLE encoder
> *
> * This file is part of FFmpeg.
> *
> @@ -19,181 +18,10 @@
> * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> *
> */
> -#include "avcodec.h"
>
[-]
> +#ifndef RLE_H
> +#define RLE_H
>
[-]
> +int rle_encode(uint8_t *outbuf, int out_size, uint8_t *inbuf, int bpp, int w, int targa_style);
this needs a ff_ prefix to avoid name clashes
>
[-]
> +#endif /* RLE_H */
hmm does targa.h have anything in common with the new rle.h except the
license header? if no theres no sense in diffing rle.h against targa.h
[...]
> + 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?
[...]
> + uint16_t bpp_tab[] = { 8, 8, 8, 8 };
const static
[...]
> + start_strip = ptr;
> +
> +
> + for (i = 0; i < s->height; i++) {
> + if (strip_sizes[i / s->rps] == 0) {
> + strip_offsets[i / s->rps] = ptr - buf;
> + start_strip = ptr;
> + }
> +
> + if ((n = encode_strip(s, p->data[0] + i * p->linesize[0]
> + , ptr, bytes_per_row, s->compr)) < 0) {
> + av_log(s->avctx, AV_LOG_ERROR, "Encode strip failed\n");
> + goto fail;
> + }
> + ptr += n;
> + strip_sizes[i / s->rps] = ptr - start_strip;
+= n and start_strip becomes unneeded
> + }
> + }
> +
> + s->num_entries = 0;
> +
> + add_entry(s, TIFF_SUBFILE, TIFF_LONG, 1, (uint32_t[]) {0});
> + add_entry(s, TIFF_WIDTH, TIFF_LONG, 1, (uint32_t[]) {s->width});
> + add_entry(s, TIFF_HEIGHT, TIFF_LONG, 1, (uint32_t[]) {s->height});
you could align these like:
add_entry(s, TIFF_SUBFILE, TIFF_LONG, 1, (uint32_t[]) {0 });
add_entry(s, TIFF_WIDTH , TIFF_LONG, 1, (uint32_t[]) {s->width });
add_entry(s, TIFF_HEIGHT , TIFF_LONG, 1, (uint32_t[]) {s->height});
makes them slightly more readable
[...]
> + pal[i] = ((rgb >> 16) & 0xff) * 257;
> + pal[i + 256] = ((rgb >> 8) & 0xff) * 257;
> + pal[i + 512] = (rgb & 0xff) * 257;
this too could be aligned vertically
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
If you really think that XML is the answer, then you definitly missunderstood
the question -- Attila Kinali
-------------- 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/20070331/f29453c8/attachment.pgp>
More information about the ffmpeg-devel
mailing list