[FFmpeg-devel] [PATCH]Basic XSUB encoder

[Björn Axelsson]

On Mon, 2 Feb 2009, Reimar D?ffinger wrote:

> 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.

New patch uses put_bits and ff_log2_tab. Thanks for the pointer!

> > +            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.

I don't agree. A messed up subtitle is better than no subtitle in case of
an error in the incoming data. Perhaps a warning would be the best
solution?

[...]

> > +    // Width and height must be powers of 2
>
> This is neither checked nor enforced, what is the meaning of this?

Brain malfunction. I meant multiple of 2.

> > +    // 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...

Ok. The new patch does the padding without a temporary buffer.

-- 
Bj?rn Axelsson