Re: [FFmpeg-devel] [PATCH 1/3] omadec: fix overflows during bit rate calculation
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 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/3] omadec: fix overflows during bit rate calculation
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 >From 6bf8af5e8db1986ec1e30143a088b91041eb9ead Mon Sep 17 00:00:00 2001 From: Andreas Cadhalpun Date: Tue, 13 Dec 2016 00:35:12 +0100 Subject: [PATCH] omadec: fix overflows during bit rate calculation Signed-off-by: Andreas Cadhalpun --- 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 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/3] omadec: fix overflows during bit rate calculation
On Wed, Dec 14, 2016 at 1:02 AM, Andreas Cadhalpun wrote: > On 13.12.2016 08:11, Paul B Mahol wrote: >> On 12/13/16, Andreas Cadhalpun wrote: >>> Signed-off-by: Andreas Cadhalpun >>> --- >>> libavformat/omadec.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/libavformat/omadec.c b/libavformat/omadec.c >>> index 6e476db..e7751d0 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 / >>> 128; >>> >>> /* 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 / 256; >>> avpriv_set_pts_info(st, 64, 1, samplerate); >>> break; >>> case OMA_CODECID_MP3: >> >> Shouldn't using 8LL or similar be more future-proof? > > Why multiply with 8 when dividing by a multiple of 8 directly afterwards? > That's just a waste of computational resources. > If sample_rate and/or framesize get larger in the future, a cast can be added. > The compiler should be able to optimize such things out and it may add clarity to what the code is doing. - Hendrik ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/3] omadec: fix overflows during bit rate calculation
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.) Moritz ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/3] omadec: fix overflows during bit rate calculation
On 12/14/16, Andreas Cadhalpun wrote: > On 13.12.2016 08:11, Paul B Mahol wrote: >> On 12/13/16, Andreas Cadhalpun wrote: >>> Signed-off-by: Andreas Cadhalpun >>> --- >>> libavformat/omadec.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/libavformat/omadec.c b/libavformat/omadec.c >>> index 6e476db..e7751d0 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 / >>> 128; >>> >>> /* 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 / 256; >>> avpriv_set_pts_info(st, 64, 1, samplerate); >>> break; >>> case OMA_CODECID_MP3: >> >> Shouldn't using 8LL or similar be more future-proof? > > Why multiply with 8 when dividing by a multiple of 8 directly afterwards? > That's just a waste of computational resources. > If sample_rate and/or framesize get larger in the future, a cast can be > added. > > Best regards, > Andreas > > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > ok ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/3] omadec: fix overflows during bit rate calculation
On 13.12.2016 08:11, Paul B Mahol wrote: > On 12/13/16, Andreas Cadhalpun wrote: >> Signed-off-by: Andreas Cadhalpun >> --- >> libavformat/omadec.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/libavformat/omadec.c b/libavformat/omadec.c >> index 6e476db..e7751d0 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 / >> 128; >> >> /* 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 / 256; >> avpriv_set_pts_info(st, 64, 1, samplerate); >> break; >> case OMA_CODECID_MP3: > > Shouldn't using 8LL or similar be more future-proof? Why multiply with 8 when dividing by a multiple of 8 directly afterwards? That's just a waste of computational resources. If sample_rate and/or framesize get larger in the future, a cast can be added. Best regards, Andreas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/3] omadec: fix overflows during bit rate calculation
On 12/13/16, Andreas Cadhalpun wrote: > Signed-off-by: Andreas Cadhalpun > --- > libavformat/omadec.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/libavformat/omadec.c b/libavformat/omadec.c > index 6e476db..e7751d0 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 / > 128; > > /* 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 / 256; > avpriv_set_pts_info(st, 64, 1, samplerate); > break; > case OMA_CODECID_MP3: Shouldn't using 8LL or similar be more future-proof? ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 1/3] omadec: fix overflows during bit rate calculation
Signed-off-by: Andreas Cadhalpun --- libavformat/omadec.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libavformat/omadec.c b/libavformat/omadec.c index 6e476db..e7751d0 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 / 128; /* 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 / 256; avpriv_set_pts_info(st, 64, 1, samplerate); break; case OMA_CODECID_MP3: -- 2.10.2 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
