[FFmpeg-devel] [PATCH]Support 8bit BMP

[Måns Rullgård]

compn <tempn at twmi.rr.com> writes:

> On Tue, 16 Sep 2008 20:42:26 +0100, M?ns Rullg?rd wrote:
>>matthieu castet <castet.matthieu at free.fr> writes:
>>
>>> Carl Eugen Hoyos wrote:
>>>> Hi!
>>>> 
>>>> Attached patch by Kostya fixes issue 626.
>>>> 
>>>> Carl Eugen
>>>> 
>>> Mans aren't you the bmp maintainer ?
>>
>>Why am I the maintainer of so many strange things?  Does anyone want
>>this file?  I only wrote it because I needed it at the time.  I hardly
>>ever touch a BMP file nowadays.
>
> this patch was in /incoming/ without any description
> looks like bmp rle support.
>
> -compn
>
> === modified file 'libavcodec/bmp.c'
> --- libavcodec/bmp.c	2008-06-12 21:50:13 +0000
> +++ libavcodec/bmp.c	2008-09-02 14:39:32 +0000
> @@ -22,6 +22,7 @@
>  #include "avcodec.h"
>  #include "bytestream.h"
>  #include "bmp.h"
> +#include <assert.h>
>
>  static av_cold int bmp_decode_init(AVCodecContext *avctx){
>      BMPContext *s = avctx->priv_data;
> @@ -32,6 +33,129 @@
>      return 0;
>  }
>
> +static av_always_inline void bmp_rle_decode(const uint8_t *buf, uint8_t *ptr,
> +                                            int buf_size, int linesize,
> +                                            int depth){
> +    const uint8_t *src = buf;
> +    uint8_t *dst = ptr;
> +    int i;
> +
> +    assert(depth == 8 || depth == 4);

Useless.  This can never happen.

> +    for(;src < buf + buf_size;){

while()

> +        int len = 0;

Useless assignment.

> +        if(len = *src++){
> +            int k = *src++;
> +            if(depth == 8){
> +                memset(dst, k, len);
> +                dst += len;

Missing validation of len.

> +            }else if(depth == 4)
> +                for(i=0;i<len;i++)

Ditto.

> +                    *dst++ = (k >> ((~i&1)<<2)) & ((1<<4) - 1);

I don't know what this is supposed to do, but alternating between the
high and low nibble of a constant in the output doesn't make much
sense to me.  Also a very complicated way of writing 15.

> +            continue;
> +        }
> +
> +        len = *src++;
> +
> +        if(len==1)
> +            break;
> +
> +        if(!len){
> +            dst = ptr += linesize;
> +            continue;
> +        }

Good plan, leave the rest of the line uninitialised.

> +        if(len==2){
> +            dst += (int)*src++ - 128;
> +            dst += ((int)*src++ - 128) * linesize;
> +            continue;
> +        }

Go backwards?  Weird, but I suppose it could be true.  Either way, the
input values must be checked.

> +        if(depth == 8){
> +            memcpy(dst, src, len);

Again, no bounds check of len.

> +            src += len;
> +            dst += len;
> +            if(len&1 && !*src)
> +                src++;
> +        }else if(depth == 4){
> +            int x = 4;
> +            for(i=0;i<len;i++){
> +                *dst++ = (*src >> x) & ((1<<4) - 1);
> +                if (!x) ++src, x = 4;
> +                else x = 0;

Great, another obfuscated nibble unpacker.

> +            }
> +            if(!x) src++;
> +            if((len&3) && !*src)
> +                src++;
> +        }
> +    }
> +}

> [...]

I can't be bothered to look at the rest of this mess.

-- 
M?ns Rullg?rd
mans at mansr.com