[FFmpeg-devel,2/2] wmavoice: don't error out if we're skipping more bits than available.

Submitted by Ronald S. Bultje on Dec. 16, 2016, 1:19 p.m.

Details

Message ID 1481894385-64104-2-git-send-email-rsbultje@gmail.com
State New
Headers show

Commit Message

Ronald S. Bultje Dec. 16, 2016, 1:19 p.m.
This reverts 2a4700a4f03280fa8ba4fc0f8a9987bb550f0d1e and implements it
correctly so streams actually decode the way the encoder intended them
to.
---
 libavcodec/wmavoice.c | 20 ++++----------------
 1 file changed, 4 insertions(+), 16 deletions(-)

Comments

Andreas Cadhalpun Dec. 19, 2016, 10:45 p.m.
On 16.12.2016 14:19, Ronald S. Bultje wrote:
> This reverts 2a4700a4f03280fa8ba4fc0f8a9987bb550f0d1e and implements it
> correctly so streams actually decode the way the encoder intended them
> to.

Why is it correct?
Is this behavior defined in a specification or reference decoder?

> ---
>  libavcodec/wmavoice.c | 20 ++++----------------
>  1 file changed, 4 insertions(+), 16 deletions(-)
> 
> diff --git a/libavcodec/wmavoice.c b/libavcodec/wmavoice.c
> index 0f29bdd..f1b5369 100644
> --- a/libavcodec/wmavoice.c
> +++ b/libavcodec/wmavoice.c
> @@ -1900,16 +1900,10 @@ static int wmavoice_decode_packet(AVCodecContext *ctx, void *data,
>                      cnt += s->spillover_nbits;
>                      s->skip_bits_next = cnt & 7;
>                      res = cnt >> 3;
> -                    if (res > avpkt->size) {
> -                        av_log(ctx, AV_LOG_ERROR,
> -                               "Trying to skip %d bytes in packet of size %d\n",
> -                               res, avpkt->size);
> -                        return AVERROR_INVALIDDATA;
> -                    }
> -                    return res;
> +                    return FFMIN(avpkt->size, res);
>                  } else
> -                    skip_bits_long (gb, s->spillover_nbits - cnt +
> -                                    get_bits_count(gb)); // resync
> +                    skip_bits_long(gb, s->spillover_nbits - cnt +
> +                                   get_bits_count(gb)); // resync

Please do cosmetic changes in a separate commit.

Best regards,
Andreas
Carl Eugen Hoyos Dec. 19, 2016, 11:59 p.m.
2016-12-19 23:45 GMT+01:00 Andreas Cadhalpun <andreas.cadhalpun@googlemail.com>:
> On 16.12.2016 14:19, Ronald S. Bultje wrote:
>> This reverts 2a4700a4f03280fa8ba4fc0f8a9987bb550f0d1e and implements it
>> correctly so streams actually decode the way the encoder intended them
>> to.
>
> Why is it correct?

Because the patch introduced an (imo horrible) regression, files
that before reported why they couldn't be played suddenly gave
no explanation, just a useless error message.

Carl Eugen
Andreas Cadhalpun Dec. 20, 2016, 12:10 a.m.
On 20.12.2016 00:59, Carl Eugen Hoyos wrote:
> 2016-12-19 23:45 GMT+01:00 Andreas Cadhalpun <andreas.cadhalpun@googlemail.com>:
>> On 16.12.2016 14:19, Ronald S. Bultje wrote:
>>> This reverts 2a4700a4f03280fa8ba4fc0f8a9987bb550f0d1e and implements it
>>> correctly so streams actually decode the way the encoder intended them
>>> to.
>>
>> Why is it correct?
> 
> Because the patch introduced an (imo horrible) regression,

I don't think that not triggering an av_assert0 is a regression.

> files that before reported why they couldn't be played suddenly
> gave no explanation, just a useless error message.

An error message is not no explanation.

Best regards,
Andreas
Carl Eugen Hoyos Dec. 20, 2016, 1 a.m.
2016-12-20 1:10 GMT+01:00 Andreas Cadhalpun <andreas.cadhalpun@googlemail.com>:
> On 20.12.2016 00:59, Carl Eugen Hoyos wrote:
>> 2016-12-19 23:45 GMT+01:00 Andreas Cadhalpun <andreas.cadhalpun@googlemail.com>:
>>> On 16.12.2016 14:19, Ronald S. Bultje wrote:
>>>> This reverts 2a4700a4f03280fa8ba4fc0f8a9987bb550f0d1e and implements it
>>>> correctly so streams actually decode the way the encoder intended them
>>>> to.
>>>
>>> Why is it correct?
>>
>> Because the patch introduced an (imo horrible) regression,
>
> I don't think that not triggering an av_assert0 is a regression.
>
>> files that before reported why they couldn't be played suddenly
>> gave no explanation, just a useless error message.
>
> An error message is not no explanation.

Sorry!

I was seriously expecting Ronald to fix 04e9853a
- an untested broken change - but I was wrong.

Carl Eugen

Patch hide | download patch | download mbox

diff --git a/libavcodec/wmavoice.c b/libavcodec/wmavoice.c
index 0f29bdd..f1b5369 100644
--- a/libavcodec/wmavoice.c
+++ b/libavcodec/wmavoice.c
@@ -1900,16 +1900,10 @@  static int wmavoice_decode_packet(AVCodecContext *ctx, void *data,
                     cnt += s->spillover_nbits;
                     s->skip_bits_next = cnt & 7;
                     res = cnt >> 3;
-                    if (res > avpkt->size) {
-                        av_log(ctx, AV_LOG_ERROR,
-                               "Trying to skip %d bytes in packet of size %d\n",
-                               res, avpkt->size);
-                        return AVERROR_INVALIDDATA;
-                    }
-                    return res;
+                    return FFMIN(avpkt->size, res);
                 } else
-                    skip_bits_long (gb, s->spillover_nbits - cnt +
-                                    get_bits_count(gb)); // resync
+                    skip_bits_long(gb, s->spillover_nbits - cnt +
+                                   get_bits_count(gb)); // resync
             } else
                 skip_bits_long(gb, s->spillover_nbits);  // resync
         }
@@ -1926,13 +1920,7 @@  static int wmavoice_decode_packet(AVCodecContext *ctx, void *data,
         int cnt = get_bits_count(gb);
         s->skip_bits_next = cnt & 7;
         res = cnt >> 3;
-        if (res > avpkt->size) {
-            av_log(ctx, AV_LOG_ERROR,
-                   "Trying to skip %d bytes in packet of size %d\n",
-                   res, avpkt->size);
-            return AVERROR_INVALIDDATA;
-        }
-        return res;
+        return FFMIN(res, avpkt->size);
     } else if ((s->sframe_cache_size = pos) > 0) {
         /* rewind bit reader to start of last (incomplete) superframe... */
         init_get_bits(gb, avpkt->data, size << 3);