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

mypopy at gmail.com mypopy at gmail.com
Tue Nov 13 02:24:18 EET 2018


On Mon, Nov 12, 2018 at 12:26 AM Mark Thompson <sw at 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 at 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 at intel.com>
> > Signed-off-by: Jun Zhao <jun.zhao at 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


More information about the ffmpeg-devel mailing list