[FFmpeg-devel] [PATCH] V210 decoder and encoder

[Reimar Döffinger]

On Sat, May 09, 2009 at 01:30:40PM -0700, Baptiste Coudurier wrote:
> +static av_cold int decode_init(AVCodecContext *avctx)
> +{
> +    if (avctx->width & 1) {
> +        av_log(avctx, AV_LOG_ERROR, "v210 needs even width\n");
> +        return -1;
> +    }

Why, does something actually break when width is odd? Doesn't look like it to me...
On a second look, the last Y value probably wouldn't be read...

> +    int aligned_width = ((avctx->width + 47) / 48) * 48;
> +    int stride = aligned_width * 8 / 3;
> +
> +    if (avpkt->size < aligned_width * avctx->height * 8 / 3) {

Uh, why not stride * ... like almost all other raw decoders do i think?

> +        av_log(avctx, AV_LOG_ERROR, "packet too small\n");
> +        return -1;
> +    }
> +
> +    pic->reference = 0;
> +    if (avctx->get_buffer(avctx, pic) < 0)
> +        return -1;
> +
> +    ydst = pic->data[0];
> +    udst = pic->data[1];
> +    vdst = pic->data[2];
> +    yend = ydst + avctx->width;
> +    pic->pict_type = FF_I_TYPE;
> +    pic->key_frame = 1;
> +
> +    for (y = 0; y < avctx->height; y++) {
> +        const uint32_t *src = psrc;
> +        uint32_t v;
> +        for (x = 0; x < avctx->width - 5; x += 6) {
> +            v= le2me_32(*src++);
> +            *udst++ = (v & 0x3FF)      <<  6;
> +            *ydst++ = (v & 0xFFC00)    >>  4;
> +            *vdst++ = (v & 0x3FF00000) >> 14;

Both

> +            *udst++ = (v & 0x000003FF) <<  6;
> +            *ydst++ = (v & 0x000FFC00) >>  4;
> +            *vdst++ = (v & 0x3FF00000) >> 14;

and

> +            *udst++ = (v <<  6) & 0xFFC0;
> +            *ydst++ = (v >>  4) & 0xFFC0;
> +            *vdst++ = (v >> 14) & 0xFFC0;

Seem nicer to me.
I think the later one might have a speed advantage due to needing only one
constant.
Also like in the encoder you don't really need one of the ands.

> +        if (x < avctx->width - 1) {
> +            v= le2me_32(*src++);
> +            *udst++ = (v & 0x3FF)      <<  6;
> +            *ydst++ = (v & 0xFFC00)    >>  4;
> +            *vdst++ = (v & 0x3FF00000) >> 14;
> +
> +            v= le2me_32(*src++);
> +            *ydst++ = (v & 0x3FF)      <<  6;
> +        }
> +        if (x < avctx->width - 3) {
> +            *udst++ = (v & 0xFFC00)    >>  4;
> +            *ydst++ = (v & 0x3FF00000) >> 14;
> +
> +            v= le2me_32(*src++);
> +            *vdst++ = (v & 0x3FF)      <<  6;
> +            *ydst++ = (v & 0xFFC00)    >>  4;
> +        }

Hm...
I'd really love something less "ugly", but can't think of much...