[FFmpeg-devel] mjpegdec: Fill raw huffman tables with default values too

These may be used by hwaccel decoders when the standard tables are not
otherwise available.  At the same time, clean up that code into an array
so it's a little less repetitive.
---
On 29/10/18 10:26, Jun Zhao wrote:
> From: Jun Zhao <jun.zhao@intel.com>
> 
> Now VA-API HWAccel MJPEG decoding uninitialized huffman table, it's
> will lead to the decoding error like"Failed to sync surface 0x5: 23
> (internal decoding error)." in iHD open source driver.
> 
> Signed-off-by: dlin2 <decai.lin@intel.com>
> Signed-off-by: Jun Zhao <jun.zhao@intel.com>
> ---
>  libavcodec/mjpegdec.c |   19 +++++++++++++++++++
>  1 files changed, 19 insertions(+), 0 deletions(-)
> 
> diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c
> index b0cb3ff..89effb6 100644
> --- a/libavcodec/mjpegdec.c
> +++ b/libavcodec/mjpegdec.c
> @@ -75,6 +75,25 @@ static int build_vlc(VLC *vlc, const uint8_t *bits_table,
>  static int build_basic_mjpeg_vlc(MJpegDecodeContext *s)
>  {
>      int ret;
> +    int i;
> +
> +    /* initialize default huffman tables */
> +    for (i = 0; i < 16; i++)
> +        s->raw_huffman_lengths[0][0][i] = avpriv_mjpeg_bits_dc_luminance[i + 1];
> +    for (i = 0; i < 12; i++)
> +        s->raw_huffman_values[0][0][i] = avpriv_mjpeg_val_dc[i];
> +    for (i = 0; i < 16; i++)
> +        s->raw_huffman_lengths[0][1][i] = avpriv_mjpeg_bits_dc_chrominance[i + 1];
> +    for (i = 0; i < 12; i++)
> +        s->raw_huffman_values[0][1][i] = avpriv_mjpeg_val_dc[i];
> +    for (i = 0; i < 16; i++)
> +        s->raw_huffman_lengths[1][0][i] = avpriv_mjpeg_bits_ac_luminance[i + 1];
> +    for (i = 0; i < 162; i++)
> +        s->raw_huffman_values[1][0][i] = avpriv_mjpeg_val_ac_luminance[i];
> +    for (i = 0; i < 16; i++)
> +        s->raw_huffman_lengths[1][1][i] = avpriv_mjpeg_bits_ac_chrominance[i + 1];
> +    for (i = 0; i < 162; i++)
> +        s->raw_huffman_values[1][1][i] = avpriv_mjpeg_val_ac_chrominance[i];
>  
>      if ((ret = build_vlc(&s->vlcs[0][0], avpriv_mjpeg_bits_dc_luminance,
>                           avpriv_mjpeg_val_dc, 12, 0, 0)) < 0)
> 

Seems reasonable, but perhaps the enclosed patch instead makes it clearer what is going on and easier to verify correctness?

(Even better: the builtin tables would be in DHT form and we would just call decode_dht() on them and avoid messing around with separate code, but since they are used in multiple places that looks more inconvenient to arrange.)

- Mark


 libavcodec/mjpegdec.c | 67 +++++++++++++++++++++++++------------------
 1 file changed, 39 insertions(+), 28 deletions(-)

On Mon, Nov 12, 2018 at 12:26 AM Mark Thompson <sw@jkqxz.net> wrote:
>
> These may be used by hwaccel decoders when the standard tables are not
> otherwise available.  At the same time, clean up that code into an array
> so it's a little less repetitive.
> ---
> On 29/10/18 10:26, Jun Zhao wrote:
> > From: Jun Zhao <jun.zhao@intel.com>
> >
> > Now VA-API HWAccel MJPEG decoding uninitialized huffman table, it's
> > will lead to the decoding error like"Failed to sync surface 0x5: 23
> > (internal decoding error)." in iHD open source driver.
> >
> > Signed-off-by: dlin2 <decai.lin@intel.com>
> > Signed-off-by: Jun Zhao <jun.zhao@intel.com>
> > ---
> >  libavcodec/mjpegdec.c |   19 +++++++++++++++++++
> >  1 files changed, 19 insertions(+), 0 deletions(-)
> >
> > diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c
> > index b0cb3ff..89effb6 100644
> > --- a/libavcodec/mjpegdec.c
> > +++ b/libavcodec/mjpegdec.c
> > @@ -75,6 +75,25 @@ static int build_vlc(VLC *vlc, const uint8_t *bits_table,
> >  static int build_basic_mjpeg_vlc(MJpegDecodeContext *s)
> >  {
> >      int ret;
> > +    int i;
> > +
> > +    /* initialize default huffman tables */
> > +    for (i = 0; i < 16; i++)
> > +        s->raw_huffman_lengths[0][0][i] = avpriv_mjpeg_bits_dc_luminance[i + 1];
> > +    for (i = 0; i < 12; i++)
> > +        s->raw_huffman_values[0][0][i] = avpriv_mjpeg_val_dc[i];
> > +    for (i = 0; i < 16; i++)
> > +        s->raw_huffman_lengths[0][1][i] = avpriv_mjpeg_bits_dc_chrominance[i + 1];
> > +    for (i = 0; i < 12; i++)
> > +        s->raw_huffman_values[0][1][i] = avpriv_mjpeg_val_dc[i];
> > +    for (i = 0; i < 16; i++)
> > +        s->raw_huffman_lengths[1][0][i] = avpriv_mjpeg_bits_ac_luminance[i + 1];
> > +    for (i = 0; i < 162; i++)
> > +        s->raw_huffman_values[1][0][i] = avpriv_mjpeg_val_ac_luminance[i];
> > +    for (i = 0; i < 16; i++)
> > +        s->raw_huffman_lengths[1][1][i] = avpriv_mjpeg_bits_ac_chrominance[i + 1];
> > +    for (i = 0; i < 162; i++)
> > +        s->raw_huffman_values[1][1][i] = avpriv_mjpeg_val_ac_chrominance[i];
> >
> >      if ((ret = build_vlc(&s->vlcs[0][0], avpriv_mjpeg_bits_dc_luminance,
> >                           avpriv_mjpeg_val_dc, 12, 0, 0)) < 0)
> >
>
> Seems reasonable, but perhaps the enclosed patch instead makes it clearer what is going on and easier to verify correctness?
>
> (Even better: the builtin tables would be in DHT form and we would just call decode_dht() on them and avoid messing around with separate code, but since they are used in multiple places that looks more inconvenient to arrange.)
>
> - Mark
>
>
>  libavcodec/mjpegdec.c | 67 +++++++++++++++++++++++++------------------
>  1 file changed, 39 insertions(+), 28 deletions(-)
>
> diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c
> index 96c425515a..2f1635838a 100644
> --- a/libavcodec/mjpegdec.c
> +++ b/libavcodec/mjpegdec.c
> @@ -73,34 +73,45 @@ static int build_vlc(VLC *vlc, const uint8_t *bits_table,
>                                huff_code, 2, 2, huff_sym, 2, 2, use_static);
>  }
>
> -static int build_basic_mjpeg_vlc(MJpegDecodeContext *s)
> +static int init_default_huffman_tables(MJpegDecodeContext *s)
>  {
> -    int ret;
> -
> -    if ((ret = build_vlc(&s->vlcs[0][0], avpriv_mjpeg_bits_dc_luminance,
> -                         avpriv_mjpeg_val_dc, 12, 0, 0)) < 0)
> -        return ret;
> -
> -    if ((ret = build_vlc(&s->vlcs[0][1], avpriv_mjpeg_bits_dc_chrominance,
> -                         avpriv_mjpeg_val_dc, 12, 0, 0)) < 0)
> -        return ret;
> -
> -    if ((ret = build_vlc(&s->vlcs[1][0], avpriv_mjpeg_bits_ac_luminance,
> -                         avpriv_mjpeg_val_ac_luminance, 251, 0, 1)) < 0)
> -        return ret;
> -
> -    if ((ret = build_vlc(&s->vlcs[1][1], avpriv_mjpeg_bits_ac_chrominance,
> -                         avpriv_mjpeg_val_ac_chrominance, 251, 0, 1)) < 0)
> -        return ret;
> -
> -    if ((ret = build_vlc(&s->vlcs[2][0], avpriv_mjpeg_bits_ac_luminance,
> -                         avpriv_mjpeg_val_ac_luminance, 251, 0, 0)) < 0)
> -        return ret;
> -
> -    if ((ret = build_vlc(&s->vlcs[2][1], avpriv_mjpeg_bits_ac_chrominance,
> -                         avpriv_mjpeg_val_ac_chrominance, 251, 0, 0)) < 0)
> -        return ret;
> +    static const struct {
> +        int class;
> +        int index;
> +        const uint8_t *bits;
> +        const uint8_t *values;
> +        int codes;
> +        int length;
> +    } ht[] = {
> +        { 0, 0, avpriv_mjpeg_bits_dc_luminance,
> +                avpriv_mjpeg_val_dc, 12, 12 },
> +        { 0, 1, avpriv_mjpeg_bits_dc_chrominance,
> +                avpriv_mjpeg_val_dc, 12, 12 },
> +        { 1, 0, avpriv_mjpeg_bits_ac_luminance,
> +                avpriv_mjpeg_val_ac_luminance,   251, 162 },
> +        { 1, 1, avpriv_mjpeg_bits_ac_chrominance,
> +                avpriv_mjpeg_val_ac_chrominance, 251, 162 },
> +        { 2, 0, avpriv_mjpeg_bits_ac_luminance,
> +                avpriv_mjpeg_val_ac_luminance,   251, 162 },
> +        { 2, 1, avpriv_mjpeg_bits_ac_chrominance,
> +                avpriv_mjpeg_val_ac_chrominance, 251, 162 },
> +    };
> +    int i, ret;
> +
> +    for (i = 0; i < FF_ARRAY_ELEMS(ht); i++) {
> +        ret = build_vlc(&s->vlcs[ht[i].class][ht[i].index],
> +                        ht[i].bits, ht[i].values, ht[i].codes,
> +                        0, ht[i].class == 1);
> +        if (ret < 0)
> +            return ret;
>
> +        if (ht[i].class < 2) {
> +            memcpy(s->raw_huffman_lengths[ht[i].class][ht[i].index],
> +                   ht[i].bits + 1, 16);
> +            memcpy(s->raw_huffman_values[ht[i].class][ht[i].index],
> +                   ht[i].values, ht[i].length);
> +        }
> +    }
>
>      return 0;
>  }
> @@ -151,7 +162,7 @@ av_cold int ff_mjpeg_decode_init(AVCodecContext *avctx)
>      avctx->colorspace = AVCOL_SPC_BT470BG;
>      s->hwaccel_pix_fmt = s->hwaccel_sw_pix_fmt = AV_PIX_FMT_NONE;
>
> -    if ((ret = build_basic_mjpeg_vlc(s)) < 0)
> +    if ((ret = init_default_huffman_tables(s)) < 0)
>          return ret;
>
>      if (s->extern_huff) {
> @@ -161,7 +172,7 @@ av_cold int ff_mjpeg_decode_init(AVCodecContext *avctx)
>          if (ff_mjpeg_decode_dht(s)) {
>              av_log(avctx, AV_LOG_ERROR,
>                     "error using external huffman table, switching back to internal\n");
> -            build_basic_mjpeg_vlc(s);
> +            init_default_huffman_tables(s);
>          }
>      }
>      if (avctx->field_order == AV_FIELD_BB) { /* quicktime icefloe 019 */
> --
> 2.19.1
LGTM

On 13/11/18 00:24, mypopy@gmail.com wrote:
> On Mon, Nov 12, 2018 at 12:26 AM Mark Thompson <sw@jkqxz.net> wrote:
>>
>> These may be used by hwaccel decoders when the standard tables are not
>> otherwise available.  At the same time, clean up that code into an array
>> so it's a little less repetitive.
>> ---
>>  libavcodec/mjpegdec.c | 67 +++++++++++++++++++++++++------------------
>>  1 file changed, 39 insertions(+), 28 deletions(-)
>>
>> diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c
>> index 96c425515a..2f1635838a 100644
>> --- a/libavcodec/mjpegdec.c
>> +++ b/libavcodec/mjpegdec.c
>> @@ -73,34 +73,45 @@ static int build_vlc(VLC *vlc, const uint8_t *bits_table,
>>                                huff_code, 2, 2, huff_sym, 2, 2, use_static);
>>  }
>>
>> -static int build_basic_mjpeg_vlc(MJpegDecodeContext *s)
>> +static int init_default_huffman_tables(MJpegDecodeContext *s)
>>  {
>> -    int ret;
>> -
>> -    if ((ret = build_vlc(&s->vlcs[0][0], avpriv_mjpeg_bits_dc_luminance,
>> -                         avpriv_mjpeg_val_dc, 12, 0, 0)) < 0)
>> -        return ret;
>> -
>> -    if ((ret = build_vlc(&s->vlcs[0][1], avpriv_mjpeg_bits_dc_chrominance,
>> -                         avpriv_mjpeg_val_dc, 12, 0, 0)) < 0)
>> -        return ret;
>> -
>> -    if ((ret = build_vlc(&s->vlcs[1][0], avpriv_mjpeg_bits_ac_luminance,
>> -                         avpriv_mjpeg_val_ac_luminance, 251, 0, 1)) < 0)
>> -        return ret;
>> -
>> -    if ((ret = build_vlc(&s->vlcs[1][1], avpriv_mjpeg_bits_ac_chrominance,
>> -                         avpriv_mjpeg_val_ac_chrominance, 251, 0, 1)) < 0)
>> -        return ret;
>> -
>> -    if ((ret = build_vlc(&s->vlcs[2][0], avpriv_mjpeg_bits_ac_luminance,
>> -                         avpriv_mjpeg_val_ac_luminance, 251, 0, 0)) < 0)
>> -        return ret;
>> -
>> -    if ((ret = build_vlc(&s->vlcs[2][1], avpriv_mjpeg_bits_ac_chrominance,
>> -                         avpriv_mjpeg_val_ac_chrominance, 251, 0, 0)) < 0)
>> -        return ret;
>> +    static const struct {
>> +        int class;
>> +        int index;
>> +        const uint8_t *bits;
>> +        const uint8_t *values;
>> +        int codes;
>> +        int length;
>> +    } ht[] = {
>> +        { 0, 0, avpriv_mjpeg_bits_dc_luminance,
>> +                avpriv_mjpeg_val_dc, 12, 12 },
>> +        { 0, 1, avpriv_mjpeg_bits_dc_chrominance,
>> +                avpriv_mjpeg_val_dc, 12, 12 },
>> +        { 1, 0, avpriv_mjpeg_bits_ac_luminance,
>> +                avpriv_mjpeg_val_ac_luminance,   251, 162 },
>> +        { 1, 1, avpriv_mjpeg_bits_ac_chrominance,
>> +                avpriv_mjpeg_val_ac_chrominance, 251, 162 },
>> +        { 2, 0, avpriv_mjpeg_bits_ac_luminance,
>> +                avpriv_mjpeg_val_ac_luminance,   251, 162 },
>> +        { 2, 1, avpriv_mjpeg_bits_ac_chrominance,
>> +                avpriv_mjpeg_val_ac_chrominance, 251, 162 },
>> +    };
>> +    int i, ret;
>> +
>> +    for (i = 0; i < FF_ARRAY_ELEMS(ht); i++) {
>> +        ret = build_vlc(&s->vlcs[ht[i].class][ht[i].index],
>> +                        ht[i].bits, ht[i].values, ht[i].codes,
>> +                        0, ht[i].class == 1);
>> +        if (ret < 0)
>> +            return ret;
>>
>> +        if (ht[i].class < 2) {
>> +            memcpy(s->raw_huffman_lengths[ht[i].class][ht[i].index],
>> +                   ht[i].bits + 1, 16);
>> +            memcpy(s->raw_huffman_values[ht[i].class][ht[i].index],
>> +                   ht[i].values, ht[i].length);
>> +        }
>> +    }
>>
>>      return 0;
>>  }
>> @@ -151,7 +162,7 @@ av_cold int ff_mjpeg_decode_init(AVCodecContext *avctx)
>>      avctx->colorspace = AVCOL_SPC_BT470BG;
>>      s->hwaccel_pix_fmt = s->hwaccel_sw_pix_fmt = AV_PIX_FMT_NONE;
>>
>> -    if ((ret = build_basic_mjpeg_vlc(s)) < 0)
>> +    if ((ret = init_default_huffman_tables(s)) < 0)
>>          return ret;
>>
>>      if (s->extern_huff) {
>> @@ -161,7 +172,7 @@ av_cold int ff_mjpeg_decode_init(AVCodecContext *avctx)
>>          if (ff_mjpeg_decode_dht(s)) {
>>              av_log(avctx, AV_LOG_ERROR,
>>                     "error using external huffman table, switching back to internal\n");
>> -            build_basic_mjpeg_vlc(s);
>> +            init_default_huffman_tables(s);
>>          }
>>      }
>>      if (avctx->field_order == AV_FIELD_BB) { /* quicktime icefloe 019 */
>> --
>> 2.19.1
> LGTM

Applied.

Thanks,

- Mark