> On Feb 1, 2020, at 8:00 PM, Michael Niedermayer <mich...@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 <quinkbl...@foxmail.com> wrote:
>>>
>>>
>>>
>>>> On Jan 27, 2020, at 12:59 AM, Carl Eugen Hoyos <ceffm...@gmail.com
>>>> <mailto:ceffm...@gmail.com>> wrote:
>>>>
>>>> Am So., 26. Jan. 2020 um 17:13 Uhr schrieb Zhao Zhili
>>>> <quinkbl...@foxmail.com <mailto:quinkbl...@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-requ...@ffmpeg.org with subject "unsubscribe".
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
