[FFmpeg-devel] [PATCH] CDXL demuxer and decoder

[Derek Buitenhuis]

> ---
>  Changelog                |    1 +
>  doc/general.texi         |    4 +
>  libavcodec/Makefile      |    1 +
>  libavcodec/allcodecs.c   |    1 +
>  libavcodec/avcodec.h     |    1 +
>  libavcodec/cdxl.c        |  270 ++++++++++++++++++++++++++++++++++++++++++++++
>  libavcodec/version.h     |    2 +-
>  libavformat/Makefile     |    1 +
>  libavformat/allformats.c |    1 +
>  libavformat/cdxl.c       |  131 ++++++++++++++++++++++
>  libavformat/version.h    |    2 +-
>  11 files changed, 413 insertions(+), 2 deletions(-)
>  create mode 100644 libavcodec/cdxl.c
>  create mode 100644 libavformat/cdxl.c

[...]

> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -259,6 +259,7 @@ enum CodecID {
>      CODEC_ID_ESCAPE130  = MKBETAG('E','1','3','0'),
>
>      CODEC_ID_G2M        = MKBETAG( 0 ,'G','2','M'),
> +    CODEC_ID_CDXL,

This should actually go under CODEC_ID_V410.

[...]

> +    for (i = 0; i < c->palette_size / 2; i++) {
> +        unsigned xxx = bytestream_get_be16(&c->palette);
> +        unsigned r   = ((xxx >> 8) & 0xF) * 17;
> +        unsigned g   = ((xxx >> 4) & 0xF) * 17;
> +        unsigned b   =  (xxx       & 0xF) * 17;
> +        new_palette[i] = (0xFF << 24) | (b + g * 256u + r * 256u * 256u);
> +    }

Nit: 256u * 256u could be merged. I doubt this makes any difference
to the compiler though.

> +    init_get_bits(&gb, c->video, c->video_size * 8);
> +    for (plane = 0; plane < c->bpp; plane++)
> +        for (y = 0; y < avctx->height; y++)
> +            for (x = 0; x < padded_width; x++)
> +                c->frame.data[0][c->frame.linesize[0] * y + x] |= get_bits1(&gb) << (plane);

This chunk of code seems to be used in many areas. Perhaps
put it in its own inline function? Also, it could be wrapped
to fit nicely in 80 cols.

> +    if (buf_size < 32)
> +        return AVERROR_INVALIDDATA;

Nit: Could do with a line break here.

> +    w               = AV_RB16(&buf[14]);
> +    h               = AV_RB16(&buf[16]);
> +    c->bpp          = AV_RB16(&buf[18]);
> +    c->palette_size = AV_RB16(&buf[20]);
> +    c->palette      = buf + 32;

Any reason why you switch notations here?

> +    c->video        = c->palette + c->palette_size;
> +    c->video_size   = buf_size - c->palette_size - 32;

Perhaps some sanity checks? Dunno how useful it is though.

> +    if (encoding == 0) {
> +        if (c->video_size < ((avctx->width + 15) & ~15) * avctx->height * c->bpp / 8)
> +            return AVERROR(EINVAL);
> +        avctx->pix_fmt = PIX_FMT_PAL8;
> +    } else if (encoding == 1) {
> +        if (c->video_size < avctx->width * avctx->height * c->bpp / 8)
> +            return AVERROR(EINVAL);
> +        avctx->pix_fmt = PIX_FMT_RGB32;

Shouldn't these also be AVERROR_INVALIDDATA? I could be wrong.

> +    } else {
> +        av_log_ask_for_sample(avctx, "unsupported video format: %d\n", encoding);
> +        return AVERROR_PATCHWELCOME;
> +    }

:D

> +    if (av_image_check_size(w, h, 0, avctx))
> +        return -1;

Should return a descriptive error code.

> +    if (w != avctx->width || h != avctx->height)
> +        avcodec_set_dimensions(avctx, w, h);

Can libavcodec actually handle changes in width or height? :/

> +    if (avctx->get_buffer(avctx, p) < 0) {
> +        av_log(avctx, AV_LOG_ERROR, "get_buffer() failed\n");
> +        return -1;
> +    }

Should be an error code. Something like AVERROR(ENOMEM)?

> +    p->pict_type = AV_PICTURE_TYPE_I;
> +
> +    if (buf[0] != 1) {
> +        av_log(avctx, AV_LOG_ERROR, "non-standard cdxl file\n");
> +        return -1;
> +    }

Should be AVERROR_INVALIDDATA, I think.

> +    if (encoding == 0) {
> +        cdxl_decode_rgb(c, avctx);
> +    } else if (encoding == 1) {
> +        c->new_video = av_mallocz(avctx->height * avctx->width);
> +        if (c->new_video == NULL)
> +            return AVERROR(ENOMEM);
> +        if (c->bpp > 6)
> +            cdxl_decode_ham8(c, avctx);
> +        else
> +            cdxl_decode_ham6(c, avctx);
> +        av_freep(&c->new_video);
> +    }

You should either add a final else statement here, or change the else
if to an else.

> +    *data_size = sizeof(AVFrame);
> +    *(AVFrame*)data = c->frame;
> +    return buf_size;
> +}

A line break would be nice here. :)

[...]

> +#define CDXL_HEADER_SIZE 32

Perhaps the decoder should have this too?

> +typedef struct CDXLDemuxContext {
> +    AVClass *class;
> +    int raw_sound_size;
> +    int sample_rate;
> +    char *framerate;
> +} CDXLDemuxContext;

Can you align the the var declarations as you did in the decoder?

> +    ast = avformat_new_stream(s, NULL);
> +    if (!ast)
> +        return AVERROR(ENOMEM);
> +    ast->codec->bits_per_coded_sample = 8;
> +    ast->codec->sample_rate = c->sample_rate;

Could use some line breaks. Also, the rest of the vars could be
aligned with the = in the first line... but that may make it ugly,
so take it with a grain of salt.

> +    vst = avformat_new_stream(s, NULL);
> +    if (!vst)
> +        return AVERROR(ENOMEM);
> +    vst->codec->codec_type = AVMEDIA_TYPE_VIDEO;

Ditto.

> +    ast->codec->codec_tag  = 0;
> +    vst->codec->codec_id   = CODEC_ID_CDXL;
> +    vst->codec->width      = AV_RB16(&header[14]);
> +    vst->codec->height     = AV_RB16(&header[16]);

Same question as above. Just a matter of consistency in
notation.

> +    avio_seek(pb, 0, SEEK_SET);
> +    return 0;
> +}

Line break? :) You needn't take all my line break comments too
seriously. It's all personal taste.

> +        if (avio_read(pb, header, CDXL_HEADER_SIZE) !=
> +                CDXL_HEADER_SIZE)

Should be indented like:

if (avio_read(pb, header, CDXL_HEADER_SIZE) !=
     CDXL_HEADER_SIZE)

> +#define OFFSET(x) offsetof(CDXLDemuxContext, x)

Is something like this really needed? It seems to merely add an extra
layer of abstraction. I apologize for bikeshedding.

> +static const AVOption cdxl_options[] = {
> +    { "sample_rate", "", OFFSET(sample_rate), AV_OPT_TYPE_INT, {.dbl = 11025}, 1, INT_MAX, AV_OPT_FLAG_DECODING_PARAM },
> +    { "framerate", "", OFFSET(framerate), AV_OPT_TYPE_STRING, {.str = "50"}, 0, 0, AV_OPT_FLAG_DECODING_PARAM },
> +    { NULL },
> +};

Is it possible to align these?

Sorry if I've missed some technical things, or been too cosmetics-
focused. Overall it looks good! :)

- Derek