[FFmpeg-devel] [PATCH]Basic XSUB encoder

[Reimar Döffinger]

On Sun, Feb 01, 2009 at 11:39:46PM +0100, Bj?rn Axelsson wrote:
> +// Adapted from dvdsubenc.c
> +// ncnt is the nibble counter
> +#define PUTNIBBLE(val)\
> +do {\
> +    if (ncnt++ & 1)\
> +        *q++ = bitbuf | ((val) & 0x0f);\
> +    else\
> +        bitbuf = (val) << 4;\
> +} while(0)

Almost none of the data has nibble size, IMO this code does not speed
things up but makes the code an ugly mess.
Unless benchmarks show a significant speed difference IMHO please use
put_bits.

> +            color = bitmap[x1++] & 0x03;

Is it actually necessary to mask the upper bits away?
If anything I think you should return an error if any of the upper bits
are set, not quietly ignore them.

> +            while (x1 < w && (bitmap[x1] & 0x03) == color)
> +                x1++;
> +            len = x1 - x;
> +
> +            // Run can't be longer than 255, unless it is the rest of a row
> +            if(x1 < w && len > 255) {
> +                int diff = len - 255;
> +                len -= diff;
> +                x1  -= diff;
> +            }

x1 isn't used anymore, which simplifies this to
if (x1 < w)
    len = FFMIN(len, 255);

> +            if (len <=3) {
> +                // 2-bit len, 2-bit color
> +                PUTNIBBLE((len << 2) | color);
> +            } else if (len <= 15) {
> +                // 2-bit zero, 4-bit len, 2-bit color
> +                PUTNIBBLE( (len & 0x0c) >> 2);
> +                PUTNIBBLE(((len & 0x03) << 2) | color);
> +            } else if (len <= 63) {
> +                // 4-bit zero, 6-bit len, 2-bit color
> +                PUTNIBBLE(0);
> +                PUTNIBBLE( (len & 0x3c) >> 2);
> +                PUTNIBBLE(((len & 0x03) << 2) | color);
> +            } else if (len <= 255) {
> +                // 6-bit zero, 8-bit len, 2-bit color
> +                PUTNIBBLE(0);
> +                PUTNIBBLE( (len & 0xc0) >> 6);
> +                PUTNIBBLE( (len & 0x3c) >> 2);
> +                PUTNIBBLE(((len & 0x03) << 2) | color);

going by the decoder, it should be possible to simplify once you use
put_bits, I think it should not need any branches, only the
full-line-length does.

> +    // Width and height must be powers of 2

This is neither checked nor enforced, what is the meaning of this?

> +    // Bitmap
> +    resized_bitmap = av_malloc(width * height);
> +    memset(resized_bitmap, 0, width * height);
> +    for (i = 0; i < h->rects[0]->h; i++)
> +        memcpy(resized_bitmap + (width * i + 2), h->rects[0]->pict.data[0] + (h->rects[0]->w * i), h->rects[0]->w);

I really don't think this should be too hard to avoid...

Greetings,
Reimar D?ffinger