diff mbox series

[FFmpeg-devel] af_volume: fix integer clip

Message ID 20200126161246.45779-1-quinkblack@foxmail.com
State New
Headers show
Series [FFmpeg-devel] af_volume: fix integer clip
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

zhilizhao Jan. 26, 2020, 4:12 p.m. UTC
---
Or specify an upper limit on volume. What do you think?

 libavfilter/af_volume.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Carl Eugen Hoyos Jan. 26, 2020, 4:59 p.m. UTC | #1
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
zhilizhao Jan. 27, 2020, 10:28 a.m. UTC | #2
> 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".
zhilizhao Feb. 1, 2020, 10:06 a.m. UTC | #3
> 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".
Michael Niedermayer Feb. 1, 2020, noon UTC | #4
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

[...]
zhilizhao Feb. 1, 2020, 12:45 p.m. UTC | #5
> 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 mbox series

Patch

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,