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

[Paul B Mahol]

On 12/30/11, Reimar Doeffinger <Reimar.Doeffinger at gmx.de> wrote:
> 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).

I'm little lost here, wouldn't AV_WN32() be enough?

What pixfmt was on your mind?

[...]