| Message ID | 20200122145210.6898-3-andreas.rheinhardt@gmail.com |
|---|---|
| State | Accepted |
| Commit | 55b46902c1f855d02ea802de1285d68577a38806 |
| Headers | show |
| Series | [FFmpeg-devel,1/4] avcodec/startcode: Use AV_RN due to UBSan warning about unaligned access | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
Andreas Rheinhardt (12020-01-22): > by using a multiplication instead. The multiplication can never overflow > an int because the sin-factor is only an int16_t. No objection. > Affected the FATE-tests filter-concat and filter-concat-vfr. I find that suspect, the change is only theoretical. What architecture are giving this? Regards,
Nicolas George (12020-01-22): > Andreas Rheinhardt (12020-01-22): > > Clang with UBSan. It of course works in practice, but we don't know whether > > future compilers might exploit undefined behaviour in an unintended way, so > > these theoretical issues should be fixed. > > This one is rather clear-cut. Although I would very much appreciate an > excerpt of the code before and after. > > But for the future, we must remember that the difference between > undefined behavior and implementation defined behavior is only relevant > to compiler developers. FFmpeg is not a compiler: all behaviors are > implementation defined. It may make a difference later for > performance-critical cases. Please remember to reply to the list. I don't know what software you are using; for good software it should be the default. Regards,
Andreas Rheinhardt: > by using a multiplication instead. The multiplication can never overflow > an int because the sin-factor is only an int16_t. > > Affected the FATE-tests filter-concat and filter-concat-vfr. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > libavfilter/asrc_sine.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavfilter/asrc_sine.c b/libavfilter/asrc_sine.c > index 3a87210b4b..61499ee2cd 100644 > --- a/libavfilter/asrc_sine.c > +++ b/libavfilter/asrc_sine.c > @@ -247,7 +247,7 @@ static int request_frame(AVFilterLink *outlink) > samples[i] = sine->sin[sine->phi >> (32 - LOG_PERIOD)]; > sine->phi += sine->dphi; > if (sine->beep_index < sine->beep_length) { > - samples[i] += sine->sin[sine->phi_beep >> (32 - LOG_PERIOD)] << 1; > + samples[i] += sine->sin[sine->phi_beep >> (32 - LOG_PERIOD)] * 2; > sine->phi_beep += sine->dphi_beep; > } > if (++sine->beep_index == sine->beep_period) > Will apply. - Andreas
Andreas Rheinhardt (12021-03-26):
> Will apply.
Sorry, missed it.
How does it impact the performance? Or even better: code? This code is
meant to be as fast as possible.
Regards,
Nicolas George: > Andreas Rheinhardt (12021-03-26): >> Will apply. > > Sorry, missed it. > You have not missed anything. > How does it impact the performance? Or even better: code? This code is > meant to be as fast as possible. > No impact whatsoever. Disassembly of request_frame is unchanged with both GCC (10) and Clang (11) on O0-O3 (I was actually surprised about O0, but not about the others.) (If it were otherwise I would have opted for a different path (like casting to unsigned).) - Andreas
Andreas Rheinhardt (12021-03-26): > No impact whatsoever. Disassembly of request_frame is unchanged with > both GCC (10) and Clang (11) on O0-O3 (I was actually surprised about > O0, but not about the others.) > (If it were otherwise I would have opted for a different path (like > casting to unsigned).) Thank you. Then ok, of course. Regards,
diff --git a/libavfilter/asrc_sine.c b/libavfilter/asrc_sine.c index 3a87210b4b..61499ee2cd 100644 --- a/libavfilter/asrc_sine.c +++ b/libavfilter/asrc_sine.c @@ -247,7 +247,7 @@ static int request_frame(AVFilterLink *outlink) samples[i] = sine->sin[sine->phi >> (32 - LOG_PERIOD)]; sine->phi += sine->dphi; if (sine->beep_index < sine->beep_length) { - samples[i] += sine->sin[sine->phi_beep >> (32 - LOG_PERIOD)] << 1; + samples[i] += sine->sin[sine->phi_beep >> (32 - LOG_PERIOD)] * 2; sine->phi_beep += sine->dphi_beep; } if (++sine->beep_index == sine->beep_period)
by using a multiplication instead. The multiplication can never overflow an int because the sin-factor is only an int16_t. Affected the FATE-tests filter-concat and filter-concat-vfr. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- libavfilter/asrc_sine.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)