[FFmpeg-devel] [PATCH]v308 and yuv4 encoders and decoders

[Derek Buitenhuis]

> Hi!
>
> Attached implements v308 as defined in QuickTime and yuv4 which only exists in
> libquicktime afaict, see ticket #470 for samples.
>
> To be committed in two parts.
>
> Please comment, Carl Eugen
>

Given I reviewed part of this in private, there shouldn't be
too much in that part to review :P.

>  @item v210 QuickTime uncompressed 4:2:2 10-bit     @tab  X  @tab  X
> + at item v308 QuickTime uncompressed 4:4:4  @tab  X  @tab  X
>  @item v410 QuickTime uncompressed 4:4:4 10-bit     @tab  X  @tab  X

Perhaps Align the @tab and friends? I have no idea if this makes any
difference to texi2html.

> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index 690ea38..d2915ad 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -259,6 +259,8 @@ enum CodecID {
>      CODEC_ID_ESCAPE130  = MKBETAG('E','1','3','0'),
>
>      CODEC_ID_G2M        = MKBETAG( 0 ,'G','2','M'),
> +    CODEC_ID_V308       = MKBETAG('V','3','0','8'),
> +    CODEC_ID_YUV4       = MKBETAG('Y','U','V','4'),

This should be added as:

CODEC_ID_V308,
CODEC_ID_YUV4,

directly above:

CODEC_ID_UTVIDEO = 0x800,

> +static av_cold int v308_decode_init(AVCodecContext *avctx)
> +{
> +    avctx->pix_fmt = PIX_FMT_YUV444P;
> +
> +    avctx->coded_frame = avcodec_alloc_frame();
> +
> +    if (!avctx->coded_frame) {
> +        av_log(avctx, AV_LOG_ERROR, "Could not allocate frame.\n");
> +        return AVERROR(ENOMEM);
> +    }
> +
> +    return 0;
> +}

The spec requires it to have an even width, so I think at
the very least, you should add a warning if it doesn't
meet this.

[...]

> +    if (avpkt->size < 3 * (avctx->width + 1 >> 1) * (avctx->height + 1 >> 1)) {
> +        av_log(avctx, AV_LOG_ERROR, "Insufficient input data.\n");
> +        return AVERROR(EINVAL);
> +    }

Given that 4:2:0 must always have an even width, I'm not sure what the
+ 1 accomplishes?

> +    for (i = 0; i < (avctx->height + 1) >> 1; i++) {
> +        for (j = 0; j < (avctx->width + 1) >> 1; j++) {

Ditto.

> +            u[j] = *src++ ^ 0x80;
> +            v[j] = *src++ ^ 0x80;

Where does the xor with 0x80 come from? Something to do with
signed-ness?

[...]

> --- a/libavformat/riff.c
> +++ b/libavformat/riff.c
> @@ -290,6 +290,8 @@ const AVCodecTag ff_codec_bmp_tags[] = {
>      { CODEC_ID_VBLE,         MKTAG('V', 'B', 'L', 'E') },
>      { CODEC_ID_ESCAPE130,    MKTAG('E', '1', '3', '0') },
>      { CODEC_ID_DXTORY,       MKTAG('x', 't', 'o', 'r') },
> +    { CODEC_ID_V308,         MKTAG('v', '3', '0', '8') },
> +    { CODEC_ID_YUV4,         MKTAG('y', 'u', 'v', '4') },
>      { CODEC_ID_NONE,         0 }
>  };

It should be possible to add these up near where v210 and v410 are,
without breaking ABI, no?

Overall looks good. :) Are you interested in adding tests to FATE
after?

- Derek