| Message ID | 20200126161246.45779-1-quinkblack@foxmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] af_volume: fix integer clip | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
Am So., 26. Jan. 2020 um 17:13 Uhr schrieb Zhao Zhili <quinkblack@foxmail.com>: > > --- > Or specify an upper limit on volume. What do you think? > > libavfilter/af_volume.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavfilter/af_volume.c b/libavfilter/af_volume.c > index 213c57195a..029925cbfb 100644 > --- a/libavfilter/af_volume.c > +++ b/libavfilter/af_volume.c > @@ -200,7 +200,7 @@ static inline void scale_samples_s16(uint8_t *dst, const uint8_t *src, > int16_t *smp_dst = (int16_t *)dst; > const int16_t *smp_src = (const int16_t *)src; > for (i = 0; i < nb_samples; i++) > - smp_dst[i] = av_clip_int16(((int64_t)smp_src[i] * volume + 128) >> 8); > + smp_dst[i] = (int16_t)av_clip64(((int64_t)smp_src[i] * volume + 128) >> 8, INT16_MIN, INT16_MAX); The cast looks unnecessary and confusing but if a limit works, it is likely simpler imo. Carl Eugen
> On Jan 27, 2020, at 12:59 AM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > > Am So., 26. Jan. 2020 um 17:13 Uhr schrieb Zhao Zhili <quinkblack@foxmail.com <mailto:quinkblack@foxmail.com>>: >> >> --- >> Or specify an upper limit on volume. What do you think? >> >> libavfilter/af_volume.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/libavfilter/af_volume.c b/libavfilter/af_volume.c >> index 213c57195a..029925cbfb 100644 >> --- a/libavfilter/af_volume.c >> +++ b/libavfilter/af_volume.c >> @@ -200,7 +200,7 @@ static inline void scale_samples_s16(uint8_t *dst, const uint8_t *src, >> int16_t *smp_dst = (int16_t *)dst; >> const int16_t *smp_src = (const int16_t *)src; >> for (i = 0; i < nb_samples; i++) >> - smp_dst[i] = av_clip_int16(((int64_t)smp_src[i] * volume + 128) >> 8); >> + smp_dst[i] = (int16_t)av_clip64(((int64_t)smp_src[i] * volume + 128) >> 8, INT16_MIN, INT16_MAX); > > The cast looks unnecessary and confusing but if a limit works, it is likely > simpler imo. The function is supposed to handle smp_src[i] * volume > INT32_MAX, so the cast is necessary. case AV_SAMPLE_FMT_S16: if (vol->volume_i < 0x10000) vol->scale_samples = scale_samples_s16_small; else vol->scale_samples = scale_samples_s16; break; I'm not sure about the use case. Does the function suppose to handle (((int64_t)smp_src[i] * volume + 128) >> 8) > INT32_MAX? By the way, there is another overflow in set_volume(): if (vol->precision == PRECISION_FIXED) { vol->volume_i = (int)(vol->volume * 256 + 0.5); vol->volume = vol->volume_i / 256.0; av_log(ctx, AV_LOG_VERBOSE, "volume_i:%d/255 ", vol->volume_i); } > > Carl Eugen > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org <mailto:ffmpeg-devel@ffmpeg.org> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel <https://ffmpeg.org/mailman/listinfo/ffmpeg-devel> > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org <mailto:ffmpeg-devel-request@ffmpeg.org> with subject "unsubscribe".
> On Jan 27, 2020, at 6:28 PM, Zhao Zhili <quinkblack@foxmail.com> wrote: > > > >> On Jan 27, 2020, at 12:59 AM, Carl Eugen Hoyos <ceffmpeg@gmail.com <mailto:ceffmpeg@gmail.com>> wrote: >> >> Am So., 26. Jan. 2020 um 17:13 Uhr schrieb Zhao Zhili <quinkblack@foxmail.com <mailto:quinkblack@foxmail.com>>: >>> >>> --- >>> Or specify an upper limit on volume. What do you think? >>> >>> libavfilter/af_volume.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/libavfilter/af_volume.c b/libavfilter/af_volume.c >>> index 213c57195a..029925cbfb 100644 >>> --- a/libavfilter/af_volume.c >>> +++ b/libavfilter/af_volume.c >>> @@ -200,7 +200,7 @@ static inline void scale_samples_s16(uint8_t *dst, const uint8_t *src, >>> int16_t *smp_dst = (int16_t *)dst; >>> const int16_t *smp_src = (const int16_t *)src; >>> for (i = 0; i < nb_samples; i++) >>> - smp_dst[i] = av_clip_int16(((int64_t)smp_src[i] * volume + 128) >> 8); >>> + smp_dst[i] = (int16_t)av_clip64(((int64_t)smp_src[i] * volume + 128) >> 8, INT16_MIN, INT16_MAX); >> >> The cast looks unnecessary and confusing but if a limit works, it is likely >> simpler imo. > > The function is supposed to handle smp_src[i] * volume > INT32_MAX, so the cast is necessary. > > case AV_SAMPLE_FMT_S16: > if (vol->volume_i < 0x10000) > vol->scale_samples = scale_samples_s16_small; > else > vol->scale_samples = scale_samples_s16; > break; > > I'm not sure about the use case. Does the function suppose to handle > (((int64_t)smp_src[i] * volume + 128) >> 8) > INT32_MAX? > > By the way, there is another overflow in set_volume(): > > if (vol->precision == PRECISION_FIXED) { > vol->volume_i = (int)(vol->volume * 256 + 0.5); > vol->volume = vol->volume_i / 256.0; > av_log(ctx, AV_LOG_VERBOSE, "volume_i:%d/255 ", vol->volume_i); > } Ping. Any suggestions? > >> >> Carl Eugen >> _______________________________________________ >> ffmpeg-devel mailing list >> ffmpeg-devel@ffmpeg.org <mailto:ffmpeg-devel@ffmpeg.org> >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel <https://ffmpeg.org/mailman/listinfo/ffmpeg-devel> >> >> To unsubscribe, visit link above, or email >> ffmpeg-devel-request@ffmpeg.org <mailto:ffmpeg-devel-request@ffmpeg.org> with subject "unsubscribe".
On Sat, Feb 01, 2020 at 06:06:39PM +0800, zhilizhao wrote: > > > > On Jan 27, 2020, at 6:28 PM, Zhao Zhili <quinkblack@foxmail.com> wrote: > > > > > > > >> On Jan 27, 2020, at 12:59 AM, Carl Eugen Hoyos <ceffmpeg@gmail.com <mailto:ceffmpeg@gmail.com>> wrote: > >> > >> Am So., 26. Jan. 2020 um 17:13 Uhr schrieb Zhao Zhili <quinkblack@foxmail.com <mailto:quinkblack@foxmail.com>>: > >>> > >>> --- > >>> Or specify an upper limit on volume. What do you think? > >>> > >>> libavfilter/af_volume.c | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/libavfilter/af_volume.c b/libavfilter/af_volume.c > >>> index 213c57195a..029925cbfb 100644 > >>> --- a/libavfilter/af_volume.c > >>> +++ b/libavfilter/af_volume.c > >>> @@ -200,7 +200,7 @@ static inline void scale_samples_s16(uint8_t *dst, const uint8_t *src, > >>> int16_t *smp_dst = (int16_t *)dst; > >>> const int16_t *smp_src = (const int16_t *)src; > >>> for (i = 0; i < nb_samples; i++) > >>> - smp_dst[i] = av_clip_int16(((int64_t)smp_src[i] * volume + 128) >> 8); > >>> + smp_dst[i] = (int16_t)av_clip64(((int64_t)smp_src[i] * volume + 128) >> 8, INT16_MIN, INT16_MAX); > >> > >> The cast looks unnecessary and confusing but if a limit works, it is likely > >> simpler imo. > > > > The function is supposed to handle smp_src[i] * volume > INT32_MAX, so the cast is necessary. > > > > case AV_SAMPLE_FMT_S16: > > if (vol->volume_i < 0x10000) > > vol->scale_samples = scale_samples_s16_small; > > else > > vol->scale_samples = scale_samples_s16; > > break; > > > > I'm not sure about the use case. Does the function suppose to handle > > (((int64_t)smp_src[i] * volume + 128) >> 8) > INT32_MAX? > > > > By the way, there is another overflow in set_volume(): > > > > if (vol->precision == PRECISION_FIXED) { > > vol->volume_i = (int)(vol->volume * 256 + 0.5); > > vol->volume = vol->volume_i / 256.0; > > av_log(ctx, AV_LOG_VERBOSE, "volume_i:%d/255 ", vol->volume_i); > > } > > Ping. Any suggestions? no but a question for the 64bit clip to make a difference the volume needs to be increased by like a factor of 65536. so everything will clip. Does this have a usecase ? or maybe iam missing something ? Thanks [...]
> On Feb 1, 2020, at 8:00 PM, Michael Niedermayer <michael@niedermayer.cc> wrote: > > On Sat, Feb 01, 2020 at 06:06:39PM +0800, zhilizhao wrote: >> >> >>> On Jan 27, 2020, at 6:28 PM, Zhao Zhili <quinkblack@foxmail.com> wrote: >>> >>> >>> >>>> On Jan 27, 2020, at 12:59 AM, Carl Eugen Hoyos <ceffmpeg@gmail.com <mailto:ceffmpeg@gmail.com>> wrote: >>>> >>>> Am So., 26. Jan. 2020 um 17:13 Uhr schrieb Zhao Zhili <quinkblack@foxmail.com <mailto:quinkblack@foxmail.com>>: >>>>> >>>>> --- >>>>> Or specify an upper limit on volume. What do you think? >>>>> >>>>> libavfilter/af_volume.c | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/libavfilter/af_volume.c b/libavfilter/af_volume.c >>>>> index 213c57195a..029925cbfb 100644 >>>>> --- a/libavfilter/af_volume.c >>>>> +++ b/libavfilter/af_volume.c >>>>> @@ -200,7 +200,7 @@ static inline void scale_samples_s16(uint8_t *dst, const uint8_t *src, >>>>> int16_t *smp_dst = (int16_t *)dst; >>>>> const int16_t *smp_src = (const int16_t *)src; >>>>> for (i = 0; i < nb_samples; i++) >>>>> - smp_dst[i] = av_clip_int16(((int64_t)smp_src[i] * volume + 128) >> 8); >>>>> + smp_dst[i] = (int16_t)av_clip64(((int64_t)smp_src[i] * volume + 128) >> 8, INT16_MIN, INT16_MAX); >>>> >>>> The cast looks unnecessary and confusing but if a limit works, it is likely >>>> simpler imo. >>> >>> The function is supposed to handle smp_src[i] * volume > INT32_MAX, so the cast is necessary. >>> >>> case AV_SAMPLE_FMT_S16: >>> if (vol->volume_i < 0x10000) >>> vol->scale_samples = scale_samples_s16_small; >>> else >>> vol->scale_samples = scale_samples_s16; >>> break; >>> >>> I'm not sure about the use case. Does the function suppose to handle >>> (((int64_t)smp_src[i] * volume + 128) >> 8) > INT32_MAX? >>> >>> By the way, there is another overflow in set_volume(): >>> >>> if (vol->precision == PRECISION_FIXED) { >>> vol->volume_i = (int)(vol->volume * 256 + 0.5); >>> vol->volume = vol->volume_i / 256.0; >>> av_log(ctx, AV_LOG_VERBOSE, "volume_i:%d/255 ", vol->volume_i); >>> } >> >> Ping. Any suggestions? > > no but a question > for the 64bit clip to make a difference the volume needs to be increased by > like a factor of 65536. so everything will clip. Does this have a usecase ? > or maybe iam missing something ? I’m wondering the usecase too. There are codes check volume_i against (65536 * 256) explicitly (merged as b38c79bf232). Does support change volume on the fly is the answer? It’s really a corner case. I’m trying to figure it out before implement AArch64 SIMD. case AV_SAMPLE_FMT_U8: if (vol->volume_i < 0x1000000) vol->scale_samples = scale_samples_u8_small; else vol->scale_samples = scale_samples_u8; break; > > Thanks > > [...] > > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > Does the universe only have a finite lifespan? No, its going to go on > forever, its just that you wont like living in it. -- Hiranya Peiri > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
diff --git a/libavfilter/af_volume.c b/libavfilter/af_volume.c index 213c57195a..029925cbfb 100644 --- a/libavfilter/af_volume.c +++ b/libavfilter/af_volume.c @@ -200,7 +200,7 @@ static inline void scale_samples_s16(uint8_t *dst, const uint8_t *src, int16_t *smp_dst = (int16_t *)dst; const int16_t *smp_src = (const int16_t *)src; for (i = 0; i < nb_samples; i++) - smp_dst[i] = av_clip_int16(((int64_t)smp_src[i] * volume + 128) >> 8); + smp_dst[i] = (int16_t)av_clip64(((int64_t)smp_src[i] * volume + 128) >> 8, INT16_MIN, INT16_MAX); } static inline void scale_samples_s16_small(uint8_t *dst, const uint8_t *src,