[FFmpeg-devel] avcodec/wma: handle run_level_decode error

Consider data as invalid if ff_wma_run_level_decode
gets out with an error.

It avoids an unpleasant sound distorsion.

See http://trac.ffmpeg.org/ticket/9358
---
 libavcodec/wmadec.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Stéphane Cerveau:
> Consider data as invalid if ff_wma_run_level_decode
> gets out with an error.
> 
> It avoids an unpleasant sound distorsion.
> 
> See http://trac.ffmpeg.org/ticket/9358
> ---
>  libavcodec/wmadec.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/wmadec.c b/libavcodec/wmadec.c
> index d627bbe50e..74ea8cea77 100644
> --- a/libavcodec/wmadec.c
> +++ b/libavcodec/wmadec.c
> @@ -606,10 +606,11 @@ static int wma_decode_block(WMACodecContext *s)
>               * there is potentially less energy there */
>              tindex = (ch == 1 && s->ms_stereo);
>              memset(ptr, 0, s->block_len * sizeof(WMACoef));
> -            ff_wma_run_level_decode(s->avctx, &s->gb, &s->coef_vlc[tindex],
> +            if (ff_wma_run_level_decode(s->avctx, &s->gb, &s->coef_vlc[tindex],
>                                      s->level_table[tindex], s->run_table[tindex],
>                                      0, ptr, 0, nb_coefs[ch],
> -                                    s->block_len, s->frame_len_bits, coef_nb_bits);
> +                                    s->block_len, s->frame_len_bits, coef_nb_bits))
> +              return AVERROR_INVALIDDATA;
>          }
>          if (s->version == 1 && s->avctx->channels >= 2)
>              align_get_bits(&s->gb);
> 
Generally, one should forward error codes and not make up some on the
fly; in this case, the callee does not return proper error codes, so
this should be fixed, too (but not in the same commit and not
necessarily by you). Even more importantly, the callee emits an error
message, stating that an error will be ignored, which will no longer
true with this patch.

- Andreas

Hi Andreas,

> Generally, one should forward error codes and not make up some on the
> fly; in this case, the callee does not return proper error codes, so
> this should be fixed, too (but not in the same commit and not
> necessarily by you). Even more importantly, the callee emits an error
> message, stating that an error will be ignored, which will no longer
> true with this patch.

If I understand correctly, the "ignoring" refers to the block being
ignored because of errors, not to the error being ignored.

Is there any chance to get this into 4.4.1 ?

Olivier

On Wed, 2021-08-11 at 16:54 +0200, Stéphane Cerveau wrote:
> Consider data as invalid if ff_wma_run_level_decode
> gets out with an error.
> 
> It avoids an unpleasant sound distorsion.
> 
> See http://trac.ffmpeg.org/ticket/9358
> ---
>  libavcodec/wmadec.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/wmadec.c b/libavcodec/wmadec.c
> index d627bbe50e..74ea8cea77 100644
> --- a/libavcodec/wmadec.c
> +++ b/libavcodec/wmadec.c
> @@ -606,10 +606,11 @@ static int wma_decode_block(WMACodecContext *s)
>               * there is potentially less energy there */
>              tindex = (ch == 1 && s->ms_stereo);
>              memset(ptr, 0, s->block_len * sizeof(WMACoef));
> -            ff_wma_run_level_decode(s->avctx, &s->gb, &s->coef_vlc[tindex],
> +            if (ff_wma_run_level_decode(s->avctx, &s->gb, &s->coef_vlc[tindex],
>                                      s->level_table[tindex], s->run_table[tindex],
>                                      0, ptr, 0, nb_coefs[ch],
> -                                    s->block_len, s->frame_len_bits, coef_nb_bits);
> +                                    s->block_len, s->frame_len_bits, coef_nb_bits))
> +              return AVERROR_INVALIDDATA;
>          }
>          if (s->version == 1 && s->avctx->channels >= 2)
>              align_get_bits(&s->gb);

Hi,

On Tue, Aug 24, 2021 at 9:07 AM Olivier Crête <olivier.crete@collabora.com>
wrote:

> Hi Andreas,
>
> > Generally, one should forward error codes and not make up some on the
> > fly; in this case, the callee does not return proper error codes, so
> > this should be fixed, too (but not in the same commit and not
> > necessarily by you). Even more importantly, the callee emits an error
> > message, stating that an error will be ignored, which will no longer
> > true with this patch.
>
> If I understand correctly, the "ignoring" refers to the block being
> ignored because of errors, not to the error being ignored.
>

No, the opposite. He suggests to try:

ret = try_decode_wma();
if (ret < 0) return ret;

instead of:

if (ret < 0) return AVERROR_INVALIDDATA;

Ronald