[FFmpeg-devel] [PATCH 3/4] zmbvenc: Prevent memory/math overflows in block_cmp()

[Tomas Härdin]

ons 2018-12-19 klockan 22:00 +0000 skrev matthew.w.fearnley at gmail.com:
> > From: Matthew Fearnley <matthew.w.fearnley at gmail.com>
> 
> score_tab[] was only declared/initialised for elements 0..255, but with
> block sizes set to 16*16, it was possible to reach 256.
> 
> This limit could also be overflowed in the histogram, because it was
> declared with a uint8_t type.
> 
> This can be fixed, and also allow different ZMBV_BLOCK sizes, by making
> score_tab[] with (ZMBV_BLOCK*ZMBV_BLOCK+1) elements, and declaring
> histogram[] to use a uint16_t type.
> 
> Note: the maximum block size possible for PAL8 is 255*255 bytes, which is close
> to the uint16_t limit.  To support full-colour pixel formats, a uint32_t could
> potentially be required.

So a potential future encoder improvement would be to try different
block sizes? Could be a fun project for someone, like a GSoC
qualification task

> @@ -69,7 +69,7 @@ static inline int block_cmp(ZmbvEncContext *c, uint8_t *src, int stride,
>  {
>      int sum = 0;
>      int i, j;
> -    uint8_t histogram[256] = {0};
> +    uint16_t histogram[256] = {0};
>  
>      /* build frequency histogram of byte values for src[] ^ src2[] */
>      *xored = 0;
> @@ -285,7 +285,9 @@ static av_cold int encode_init(AVCodecContext *avctx)
>      int i;
>      int lvl = 9;
>  
> -    for(i=1; i<256; i++)
> +    /* entropy score table for block_cmp() */
> +    c->score_tab[0] = 0;
> +    for(i = 1; i <= ZMBV_BLOCK * ZMBV_BLOCK; i++)
>          c->score_tab[i] = -i * log2(i / (double)(ZMBV_BLOCK * ZMBV_BLOCK)) * 256;

It strikes me that due to the uint8_t overflow the old code actually
worked correctly, but only incidentally..

Looks good!

/Tomas