diff mbox

[FFmpeg-devel,1/3] omadec: fix overflows during bit rate calculation

Message ID da218c83-a165-375f-82b3-6b39e1ac7e10@googlemail.com
State Accepted
Headers show

Commit Message

Andreas Cadhalpun Dec. 15, 2016, 12:53 a.m. UTC 
On 14.12.2016 11:16, Moritz Barsnick wrote:
> On Wed, Dec 14, 2016 at 01:02:41 +0100, Andreas Cadhalpun wrote:
>> On 13.12.2016 08:11, Paul B Mahol wrote:
>>>> -        st->codecpar->bit_rate    = samplerate * framesize * 8 / 2048;
>>>> +        st->codecpar->bit_rate    = samplerate * framesize / 256;
>>
>> Why multiply with 8 when dividing by a multiple of 8 directly afterwards?
>> That's just a waste of computational resources.
> 
> I can only explain the term with "readability" (e.g. "number of bytes
> times 8 is number of bits, divided by 2048 is the rate"). If you
> bracket the (8 / 2048), it would avoid the overflow, and the compiler
> should evaluate the term to that constant 256 anyway, right? (Just if
> anyone cares about the presumed readability.)

Well, (8 / 2048) = 0, but one can do "/ (2048 / 8)".
Attached is a version doing it that way. Do you think that's better?

Best regards,
Andreas

Comments

Andreas Cadhalpun Jan. 6, 2017, 7:01 p.m. UTC | #1 
On 15.12.2016 01:53, Andreas Cadhalpun wrote:
> On 14.12.2016 11:16, Moritz Barsnick wrote:
>> On Wed, Dec 14, 2016 at 01:02:41 +0100, Andreas Cadhalpun wrote:
>>> On 13.12.2016 08:11, Paul B Mahol wrote:
>>>>> -        st->codecpar->bit_rate    = samplerate * framesize * 8 / 2048;
>>>>> +        st->codecpar->bit_rate    = samplerate * framesize / 256;
>>>
>>> Why multiply with 8 when dividing by a multiple of 8 directly afterwards?
>>> That's just a waste of computational resources.
>>
>> I can only explain the term with "readability" (e.g. "number of bytes
>> times 8 is number of bits, divided by 2048 is the rate"). If you
>> bracket the (8 / 2048), it would avoid the overflow, and the compiler
>> should evaluate the term to that constant 256 anyway, right? (Just if
>> anyone cares about the presumed readability.)
> 
> Well, (8 / 2048) = 0, but one can do "/ (2048 / 8)".
> Attached is a version doing it that way. Do you think that's better?

I've pushed this variant now.

Best regards,
Andreas
diff mbox

Patch

From 6bf8af5e8db1986ec1e30143a088b91041eb9ead Mon Sep 17 00:00:00 2001
From: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
Date: Tue, 13 Dec 2016 00:35:12 +0100
Subject: [PATCH] omadec: fix overflows during bit rate calculation

Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
---
 libavformat/omadec.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libavformat/omadec.c b/libavformat/omadec.c
index 6e476db..757ae53 100644
--- a/libavformat/omadec.c
+++ b/libavformat/omadec.c
@@ -365,7 +365,7 @@  static int oma_read_header(AVFormatContext *s)
         st->codecpar->channels    = 2;
         st->codecpar->channel_layout = AV_CH_LAYOUT_STEREO;
         st->codecpar->sample_rate = samplerate;
-        st->codecpar->bit_rate    = st->codecpar->sample_rate * framesize * 8 / 1024;
+        st->codecpar->bit_rate    = st->codecpar->sample_rate * framesize / (1024 / 8);
 
         /* fake the ATRAC3 extradata
          * (wav format, makes stream copy to wav work) */
@@ -398,7 +398,7 @@  static int oma_read_header(AVFormatContext *s)
             return AVERROR_INVALIDDATA;
         }
         st->codecpar->sample_rate = samplerate;
-        st->codecpar->bit_rate    = samplerate * framesize * 8 / 2048;
+        st->codecpar->bit_rate    = samplerate * framesize / (2048 / 8);
         avpriv_set_pts_info(st, 64, 1, samplerate);
         break;
     case OMA_CODECID_MP3:
-- 
2.10.2