[FFmpeg-devel] [PATCH] brender_pix: a new image decoder

[Carl Eugen Hoyos]

Aleksi Nurmi <aleksi.nurmi <at> gmail.com> writes:

> +    AV_CODEC_ID_BRENDER_PIX,
>      AV_CODEC_ID_Y41P       = MKBETAG('Y','4','1','P'),

Please define an id with MKBETAG for AV_CODEC_ID_BRENDER_PIX.

[...]

> + * BRender PIX (.pix) image decoder
> + * Copyright (c) 2012 Aleksi Nurmi

> + *
> + * Tested against samples from I-War / Independence War and Defiance.
> + *
> + * Note that there are PAL8 format images that do not contain a palette. In
> + * this case, the decoder sets the palette_has_changed property of the AVFrame
> + * to 0.

I would suggest to move the four lines below the license 
header in a separate comment.
(And perhaps shorten the lines a bit.)

> + *
> + * This file is part of Libav.

Please use a license header with "FFmpeg".

[...]

> +    case 7:
> +        avctx->pix_fmt = AV_PIX_FMT_0RGB;

Are you sure that there is no transparency information? 
I am especially surprised because GRAY8A is supported.
I wanted to test, but none of the samples you 
provided contains 32bit pixels.

[...]

> +    if (avctx->pix_fmt == AV_PIX_FMT_PAL8 &&
> +        (chunk_type == 0x3 || chunk_type == 0x3d))
> +    {

It's preferred to move "{" at the end of the condition.
(Feel free to ignore, same below.)

[...]

> +++ b/libavformat/img2.c
> @@ -76,6 +76,7 @@ static const IdStrMap img_tags[] = {
>      { AV_CODEC_ID_XBM       , "xbm"},
>      { AV_CODEC_ID_XFACE     , "xface"},
>      { AV_CODEC_ID_XWD       , "xwd"},
> +    { AV_CODEC_ID_BRENDER_PIX, "pix"},

This could be alphabetically ordered.

Thank you for the patch, Carl Eugen