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

[mashiat.sarker at gmail.com]

On 11/14/2012 06:00 PM, Aleksi Nurmi wrote:
> PIX is an image file format that was used by the BRender 3d engine.
>
> Signed-off-by: Aleksi Nurmi <aleksi.nurmi at gmail.com>
> ---
>   Changelog                |    1 +
>   doc/general.texi         |    2 +
>   libavcodec/Makefile      |    1 +
>   libavcodec/allcodecs.c   |    1 +
>   libavcodec/avcodec.h     |    1 +
>   libavcodec/brender_pix.c |  244 ++++++++++++++++++++++++++++++++++++++++++++++
>   libavcodec/codec_desc.c  |    6 ++
>   libavcodec/version.h     |    2 +-
>   libavformat/img2.c       |    1 +
>   9 files changed, 258 insertions(+), 1 deletion(-)
>   create mode 100644 libavcodec/brender_pix.c
[...]
> +typedef struct BRPixContext {
> +    AVFrame frame;
> +} BRPixContext;
> +
> +typedef struct BRPixHeader {
> +    int format;
> +    unsigned int width, height;
> +} BRPixHeader;

I don't see why these two cannot be merged.

[...]
> +    // the header is at least 11 bytes long; we read the first 7
> +    if (header_len < 11) {
> +        return 0;
> +    }

Is it an error for the header_len to be less than 11? If that's the case 
you might consider returning AVERROR_INVALIDDATA or at least printing a 
warning.

[...]
> +    switch (hdr.format) {
> +    case 3:
> +        avctx->pix_fmt = AV_PIX_FMT_PAL8;
> +        bytes_pp = 1;
> +        break;
> +    case 4:
> +        avctx->pix_fmt = AV_PIX_FMT_RGB555BE;
> +        bytes_pp = 2;
> +        break;
> +    case 5:
> +        avctx->pix_fmt = AV_PIX_FMT_RGB565BE;
> +        bytes_pp = 2;
> +        break;
> +    case 6:
> +        avctx->pix_fmt = AV_PIX_FMT_RGB24;
> +        bytes_pp = 3;
> +        break;
> +    case 7:
> +        avctx->pix_fmt = AV_PIX_FMT_0RGB;
> +        bytes_pp = 4;
> +        break;
> +    case 18:
> +        // gray + alpha
> +        avctx->pix_fmt = AV_PIX_FMT_Y400A;
> +        bytes_pp = 2;
> +        break;

Any reason why this cannot be a LUT? This looks kinda ugly to me (but 
may be that's just me).

[...]
> +
> +    // read the image data to the buffer
> +    {
> +        unsigned int bytes_per_scanline = bytes_pp * hdr.width;
> +        unsigned int bytes_left = bytestream2_get_bytes_left(&gb);
> +
> +        if (chunk_type != 0x21 || data_len != bytes_left ||
> +            bytes_left / bytes_per_scanline < hdr.height)
> +        {
> +            av_log(avctx, AV_LOG_ERROR, "Invalid image data\n");
> +            return AVERROR_INVALIDDATA;
> +        }
> +
> +        av_image_copy_plane(s->frame.data[0], s->frame.linesize[0],
> +                            avpkt->data + bytestream2_tell(&gb),
> +                            bytes_per_scanline,
> +                            bytes_per_scanline, hdr.height);
> +    }

Why do you want a new scope here?

Feel free to take to leave my suggestion. Functionally they are not 
going to improve anything.

-Shakkhar