Message ID | 0bc8f48c-db5a-0730-2bd4-c90a92855b5f@jkqxz.net |
---|---|
State | Accepted |
Commit | 7ba63695b78c13bb6e6a734c55ad9e829c738b42 |
Headers | show |
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
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 */