Re: [FFmpeg-devel] [PATCH] avcodec/exr: simplify piz decompression

2021-02-23 Thread Paul B Mahol
will apply soon
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] avcodec/exr: simplify piz decompression

2021-02-20 Thread Paul B Mahol
On Sat, Feb 20, 2021 at 11:11 PM Andreas Rheinhardt <
andreas.rheinha...@gmail.com> wrote:

> Paul B Mahol:
> > Signed-off-by: Paul B Mahol 
> > ---
> >  libavcodec/exr.c | 212 +++
> >  1 file changed, 65 insertions(+), 147 deletions(-)
> >
> > diff --git a/libavcodec/exr.c b/libavcodec/exr.c
> > index cacdff5774..625ee4680c 100644
> > --- a/libavcodec/exr.c
> > +++ b/libavcodec/exr.c
> > @@ -91,6 +91,12 @@ enum ExrTileLevelRound {
> >  EXR_TILE_ROUND_UNKNOWN,
> >  };
> >
> > +typedef struct HuffEntry {
> > +uint8_t  len;
> > +uint16_t sym;
> > +uint16_t code;
>
> The old code allowed codes with a length of <= 58. This is more than our
> VLC-API allows and even more than fits into a 16-bit code. You seem to
> believe that all codes have a length <= 16 just because HUF_ENCBITS is
> 16. But this is wrong: It just means that there are at most 1<<16
> ordinary symbols and one special symbol for runs. It also means that we
> can't even distinguish all possible symbols because VLC_TYPE is 16 bits.
>

Fixed to use 32bits code, also added messages to ask for sample if more is
needed.


> > +} HuffEntry;
> > +
> >  typedef struct EXRChannel {
> >  int xsub, ysub;
> >  enum ExrPixelType pixel_type;
> > @@ -116,6 +122,11 @@ typedef struct EXRThreadData {
> >  int ysize, xsize;
> >
> >  int channel_line_size;
> > +
> > +uint16_t run_sym;
> > +HuffEntry *he;
> > +uint64_t *freq;
> > +VLC vlc;
> >  } EXRThreadData;
> >
> >  typedef struct EXRContext {
> > @@ -319,11 +330,8 @@ static void apply_lut(const uint16_t *lut, uint16_t
> *dst, int dsize)
> >  }
> >
> >  #define HUF_ENCBITS 16  // literal (value) bit length
> > -#define HUF_DECBITS 14  // decoding bit size (>= 8)
> >
> >  #define HUF_ENCSIZE ((1 << HUF_ENCBITS) + 1)  // encoding table size
> > -#define HUF_DECSIZE (1 << HUF_DECBITS)// decoding table size
> > -#define HUF_DECMASK (HUF_DECSIZE - 1)
> >
> >  typedef struct HufDec {
> >  int len;
> > @@ -336,7 +344,7 @@ static void huf_canonical_code_table(uint64_t *hcode)
> >  uint64_t c, n[59] = { 0 };
> >  int i;
> >
> > -for (i = 0; i < HUF_ENCSIZE; ++i)
> > +for (i = 0; i < HUF_ENCSIZE; i++)
>
> Spurious change.
>
> >  n[hcode[i]] += 1;
> >
> >  c = 0;
> > @@ -399,149 +407,63 @@ static int huf_unpack_enc_table(GetByteContext
> *gb,
> >  return 0;
> >  }
> >
> > -static int huf_build_dec_table(const uint64_t *hcode, int im,
> > -   int iM, HufDec *hdecod)
> > +static int huf_build_dec_table(EXRThreadData *td, int im, int iM)
> >  {
> > -for (; im <= iM; im++) {
> > -uint64_t c = hcode[im] >> 6;
> > -int i, l = hcode[im] & 63;
> > -
> > -if (c >> l)
> > -return AVERROR_INVALIDDATA;
> > -
> > -if (l > HUF_DECBITS) {
> > -HufDec *pl = hdecod + (c >> (l - HUF_DECBITS));
> > -if (pl->len)
> > -return AVERROR_INVALIDDATA;
> > -
> > -pl->lit++;
> > -
> > -pl->p = av_realloc(pl->p, pl->lit * sizeof(int));
> > -if (!pl->p)
> > -return AVERROR(ENOMEM);
> > -
> > -pl->p[pl->lit - 1] = im;
> > -} else if (l) {
> > -HufDec *pl = hdecod + (c << (HUF_DECBITS - l));
> > -
> > -for (i = 1 << (HUF_DECBITS - l); i > 0; i--, pl++) {
> > -if (pl->len || pl->p)
> > -return AVERROR_INVALIDDATA;
> > -pl->len = l;
> > -pl->lit = im;
> > -}
> > -}
> > +int j = 0;
> > +
> > +for (int i = im; i < iM; i++) {
> > +td->he[j].sym = i;
> > +td->he[j].len = td->freq[i] & 63;
> > +td->he[j].code = td->freq[i] >> 6;> +if (td->he[j].len
> > 0)
> > +j++;
> > +else
> > +td->run_sym = i;
> >  }
> >
> > -return 0;
> > -}
> > -
> > -#define get_char(c, lc, gb)
>\
> > -{
>\
> > -c   = (c << 8) | bytestream2_get_byte(gb);
>   \
> > -lc += 8;
>   \
> > -}
> > +td->he[j].sym = td->run_sym;
> > +td->he[j].len = td->freq[iM] & 63;
> > +td->he[j].code = td->freq[iM] >> 6;
> > +j++;
> >
> > -#define get_code(po, rlc, c, lc, gb, out, oe, outb)
>\
> > -{
>\
> > -if (po == rlc) {
>   \
> > -if (lc < 8)
>\
> > -get_char(c, lc, gb);
>   \
> > -lc -= 8;
>   \
> > -
>   \
> > -cs = c >> lc;
>\
> > -
>   \
> > -if (out + cs > oe || out == outb)
>\
> > -return AVERROR_INVALIDDATA;
>\
> > -
>   \
> > -s = out[-1];
>   \
> > -
>   \
> > -while (cs-- > 0)
>   \
> > -*out++ = s;
>\
> > -} else if (out < oe) {
>   \
> > -*out++ = po;
>   \
> > -} else {
>   \

Re: [FFmpeg-devel] [PATCH] avcodec/exr: simplify piz decompression

2021-02-20 Thread Andreas Rheinhardt
Paul B Mahol:
> Signed-off-by: Paul B Mahol 
> ---
>  libavcodec/exr.c | 212 +++
>  1 file changed, 65 insertions(+), 147 deletions(-)
> 
> diff --git a/libavcodec/exr.c b/libavcodec/exr.c
> index cacdff5774..625ee4680c 100644
> --- a/libavcodec/exr.c
> +++ b/libavcodec/exr.c
> @@ -91,6 +91,12 @@ enum ExrTileLevelRound {
>  EXR_TILE_ROUND_UNKNOWN,
>  };
>  
> +typedef struct HuffEntry {
> +uint8_t  len;
> +uint16_t sym;
> +uint16_t code;

The old code allowed codes with a length of <= 58. This is more than our
VLC-API allows and even more than fits into a 16-bit code. You seem to
believe that all codes have a length <= 16 just because HUF_ENCBITS is
16. But this is wrong: It just means that there are at most 1<<16
ordinary symbols and one special symbol for runs. It also means that we
can't even distinguish all possible symbols because VLC_TYPE is 16 bits.

> +} HuffEntry;
> +
>  typedef struct EXRChannel {
>  int xsub, ysub;
>  enum ExrPixelType pixel_type;
> @@ -116,6 +122,11 @@ typedef struct EXRThreadData {
>  int ysize, xsize;
>  
>  int channel_line_size;
> +
> +uint16_t run_sym;
> +HuffEntry *he;
> +uint64_t *freq;
> +VLC vlc;
>  } EXRThreadData;
>  
>  typedef struct EXRContext {
> @@ -319,11 +330,8 @@ static void apply_lut(const uint16_t *lut, uint16_t 
> *dst, int dsize)
>  }
>  
>  #define HUF_ENCBITS 16  // literal (value) bit length
> -#define HUF_DECBITS 14  // decoding bit size (>= 8)
>  
>  #define HUF_ENCSIZE ((1 << HUF_ENCBITS) + 1)  // encoding table size
> -#define HUF_DECSIZE (1 << HUF_DECBITS)// decoding table size
> -#define HUF_DECMASK (HUF_DECSIZE - 1)
>  
>  typedef struct HufDec {
>  int len;
> @@ -336,7 +344,7 @@ static void huf_canonical_code_table(uint64_t *hcode)
>  uint64_t c, n[59] = { 0 };
>  int i;
>  
> -for (i = 0; i < HUF_ENCSIZE; ++i)
> +for (i = 0; i < HUF_ENCSIZE; i++)

Spurious change.

>  n[hcode[i]] += 1;
>  
>  c = 0;
> @@ -399,149 +407,63 @@ static int huf_unpack_enc_table(GetByteContext *gb,
>  return 0;
>  }
>  
> -static int huf_build_dec_table(const uint64_t *hcode, int im,
> -   int iM, HufDec *hdecod)
> +static int huf_build_dec_table(EXRThreadData *td, int im, int iM)
>  {
> -for (; im <= iM; im++) {
> -uint64_t c = hcode[im] >> 6;
> -int i, l = hcode[im] & 63;
> -
> -if (c >> l)
> -return AVERROR_INVALIDDATA;
> -
> -if (l > HUF_DECBITS) {
> -HufDec *pl = hdecod + (c >> (l - HUF_DECBITS));
> -if (pl->len)
> -return AVERROR_INVALIDDATA;
> -
> -pl->lit++;
> -
> -pl->p = av_realloc(pl->p, pl->lit * sizeof(int));
> -if (!pl->p)
> -return AVERROR(ENOMEM);
> -
> -pl->p[pl->lit - 1] = im;
> -} else if (l) {
> -HufDec *pl = hdecod + (c << (HUF_DECBITS - l));
> -
> -for (i = 1 << (HUF_DECBITS - l); i > 0; i--, pl++) {
> -if (pl->len || pl->p)
> -return AVERROR_INVALIDDATA;
> -pl->len = l;
> -pl->lit = im;
> -}
> -}
> +int j = 0;
> +
> +for (int i = im; i < iM; i++) {
> +td->he[j].sym = i;
> +td->he[j].len = td->freq[i] & 63;
> +td->he[j].code = td->freq[i] >> 6;> +if (td->he[j].len > 0)
> +j++;
> +else
> +td->run_sym = i;
>  }
>  
> -return 0;
> -}
> -
> -#define get_char(c, lc, gb)  
>  \
> -{
>  \
> -c   = (c << 8) | bytestream2_get_byte(gb);   
>  \
> -lc += 8; 
>  \
> -}
> +td->he[j].sym = td->run_sym;
> +td->he[j].len = td->freq[iM] & 63;
> +td->he[j].code = td->freq[iM] >> 6;
> +j++;
>  
> -#define get_code(po, rlc, c, lc, gb, out, oe, outb)  
>  \
> -{
>  \
> -if (po == rlc) { 
>  \
> -if (lc < 8)  
>  \
> -get_char(c, lc, gb); 
>  \
> -lc -= 8; 
>  \
> - 
>  \
> -cs = c >> lc;
>  \
> - 
>  \
> -if (out + cs > oe || out == outb)
>  \
> -return AVERROR_INVALIDDATA;  
>  \