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

[Reimar Döffinger]

only partial review...

On 30 Dec 2011, at 00:08, Paul B Mahol <onemda at gmail.com> wrote:
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index 690ea38..4afb8a8 100644
> --- 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,

give it some fixed value, like this it gets G2N which makes no sense.

> +    avcodec_get_frame_defaults(&c->frame);
> +    avctx->coded_frame = (AVFrame*)&c->frame;

Pointless cast, it already has that type.
But is it even necessary to do this assignment?

> +    for (i = 0; i < c->palette_size / 2; i++) {
> +        unsigned xxx = bytestream_get_be16(&c->palette);

I can't imagine it a good idea to use bytestream functions on c->palette since you end up with it no longer pointing to the palette.

> +        unsigned r   = ((xxx >> 8) & 0xF) * 17;
> +        unsigned g   = ((xxx >> 4) & 0xF) * 17;
> +        unsigned b   =  (xxx       & 0xF) * 17;

*0x11 should be more obvious. However I see no reason to do this at this point.

> +        new_palette[i] = (0xFF << 24) | (b + g * 256u + r * 256u * 256u);

You'd need a "u" for the 0xFF, but not for the 256.
Also mixing shift and multiply for doing exactly the same thing is rather confusing.
Lastly this can be done as
r = (x >> 8) & 0xf;
g = (x >> 4) & 0xf;
b = x & 0xf;
new_palette[i] = (0xf << 24) | (r << 16) | (g << 8) | b;
// expand from 4 to 8 bit
new_palette[i] *= 0x11111111;

> +    int padded_width = (avctx->width + 15) & ~15;

FFALIGN

> +    memset(c->frame.data[0], 0, c->frame.linesize[0] * avctx->height);

You should not be writing into the area between width and 
linesize, it might not be allowed to write there.
Also, linesize can be negative.

> +                c->frame.data[0][c->frame.linesize[0] * y + x] |= get_bits1(&gb) << (plane);

The () around plane is quite poinless.

> +static void cdxl_decode_ham6(CDXLVideoContext *c, AVCodecContext *avctx)
> +{
> +    int x, y, plane;
> +    uint32_t new_palette[16], r, g, b;
> +    uint8_t *ptr, *out, index, op;
> +    GetBitContext gb;
> +
> +    ptr = c->new_video;
> +    out = c->frame.data[0];
> +
> +    cdxl_import_palette(c, new_palette);
> +    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 < avctx->width; x++)
> +                c->new_video[avctx->width * y + x] |= get_bits1(&gb) << (plane);

this part is almost exactly the same 3 times, you should try to avoid duplicating it.

> +                b = index * 17;

0x11 is more obvious than 17 IMO.

> +                r = (index << 18) | (r & 196608);

That applies even more here. I doubt anyone knows what 196608 might be without doing some calculations first.

> +            AV_WL32(out + (x * 4), (0xFF << 24) | (r | g | b));

Chose a different pixfmt and use AV_WN32 (or better the variant for aligned writes).

> +    c->bpp          = AV_RB16(&buf[18]);

I don't think the decoder can handle any value for that.

> +        if (c->video_size < ((avctx->width + 15) & ~15) * avctx->height * c->bpp / 8)

FFALIGN for the width.
But sure it is "* bpp / 8" and not "* (bpp/8)" or even "* ((bpp+7)/8)"?

> +    if (p->data[0])
> +        avctx->release_buffer(avctx, p);
> +
> +    if (av_image_check_size(w, h, 0, avctx))
> +        return -1;
> +    if (w != avctx->width || h != avctx->height)
> +        avcodec_set_dimensions(avctx, w, h);
> +
> +    p->reference = 0;
> +    if (avctx->get_buffer(avctx, p) < 0) {
> +        av_log(avctx, AV_LOG_ERROR, "get_buffer() failed\n");
> +        return -1;
> +    }
> +    p->pict_type = AV_PICTURE_TYPE_I;
> +
> +    if (buf[0] != 1) {
> +        av_log(avctx, AV_LOG_ERROR, "non-standard cdxl file\n");
> +        return -1;
> +    }
> +    if (format) {
> +        av_log_ask_for_sample(avctx, "unsupported pixel format: %d\n", format);
> +        return AVERROR_PATCHWELCOME;
> +    }
> +    if (c->bpp < 1)
> +        return AVERROR_INVALIDDATA;
> +    if (c->bpp > 8) {
> +        av_log_ask_for_sample(avctx, "unsupported pixel size: %d\n", c->bpp);
> +        return AVERROR_PATCHWELCOME;
> +    }

That is really a bit late to do the checking, you already got an new buffer etc.
That does not make it wrong, but it is not good either.

> +        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 not do a malloc/free once per frame, that is needlessly slow, see e.g. av_fast_malloc.

> +    ast->codec->channels    = (AV_RB8(&header[1]) & 8) + 1;

There is no point in using AV_RB8. Also I don't think this is right, like this channels can only be either 1 or 9.


> +        if (avio_read(pb, header, CDXL_HEADER_SIZE) !=
> +                CDXL_HEADER_SIZE)
> +            return AVERROR(EIO);
> +        avio_seek(pb, -CDXL_HEADER_SIZE, SEEK_CUR);

Seeking should preferably be avoided.
There are functions to read a first part of a packet and then append the rest.

> +AVInputFormat ff_cdxl_demuxer = {
> +    .name           = "cdxl",
> +    .long_name      = NULL_IF_CONFIG_SMALL("CDXL"),
> +    .priv_data_size = sizeof(CDXLDemuxContext),
> +    .read_header    = cdxl_read_header,
> +    .read_packet    = cdxl_read_packet,
> +    .extensions     = "cdxl,xl",
> +    .priv_class     = &cdxl_demuxer_class,

generic seeking support should be added.
That requires that you set pkt->pos "correctly" for audio (since audio and video are in one block in this format audio and video packets should get the same pos values) and the state must be reset to start with reading the combined packet header, not audio.