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

[Aleksi Nurmi]

2012/11/15 mashiat.sarker at gmail.com <mashiat.sarker at gmail.com>:
>> +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.

I use two BRPixHeaders, one for the image and one for the palette.

> 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.

The brpix_read_header function is a helper function, I print a warning
and return the error code in the containing function. This could be
changed but I left as-is it for now.

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

I think it's a fine in-code lookup table :-)

>
> [...]
>
>> +
>> +    // 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?

To introduce two temporary names here. It's ugly, but better than
having to carry extra context in my head. I believe that mixing
statements and declarations is disallowed in FFmpeg although allowed
in C99.

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

Thanks!

I'll send the updated patch soon.