ffmpeg_opt.c correct_input_start_times(void) calculates wrong ts_offset

[Jens Viebig]

Summary of the bug:
The abs_start_seek calculation in correct_input_start_times(void) is not done correctly since the sum on the left is evaluated before the shorthand if, giving unexecpected results

abs_start_seek = is->start_time + (ifile->start_time != AV_NOPTS_VALUE) ? ifile->start_time : 0;

This expression almost always evaluates to "ifile->start_time" but is expected to be evaluated to: is->start_time + ifile->start_time

As an example:

is->start_time = 10
ifile->start_time = 20

abs_start_seek = 10 + (20 != AV_NOPTS_VALUE) ? 20 : 0;
evaluates to
abs_start_seek = 10 + (true) ? 20 : 0;
then
abs_start_seek = 11 ? 20 : 0;
then
abs_start_seek = 20

We already submitted a patch for this issue, but it seems it was misunderstood by the maintainers and is still not merged to the current codebase
​https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2023-May/309555.html

With the patch the evaluation is corrected to:
abs_start_seek = is->start_time + ((ifile->start_time != AV_NOPTS_VALUE) ? ifile->start_time : 0);

With the example, this evaluates to:

is->start_time = 10
ifile->start_time = 20

abs_start_seek = 10 + ((20 != AV_NOPTS_VALUE) ? 20 : 0);
evaluates to
abs_start_seek = 10 + ((true) ? 20 : 0);
then
abs_start_seek = 10 + 20;
then
abs_start_seek = 30;

[Balling]

​https://patchwork.ffmpeg.org/project/ffmpeg/patch/0eabd803-a3d7-3577-52f6-dfa5822c16b3@gmx.de/

Although the parentheses around (ifile->start_tile !=
AV_NOPTS_VALUE) can be removed as well.

Also change the name of the patch fftools: correct expression of ternary operation

[Jens Viebig]

We actually added parentheses to make it work.

Why should we remove the other existing parentheses ?
Feels unsafe and a new potential issue for wrong evaluation of the expression

[Jens Viebig]

This is much more clear to read:
abs_start_seek = is->start_time + ((ifile->start_time != AV_NOPTS_VALUE) ? ifile->start_time : 0);

if we remove the parenthesis like this:
abs_start_seek = is->start_time + (ifile->start_time != AV_NOPTS_VALUE ? ifile->start_time : 0);

is it guaranteed to evaluate "ifile->start_time != AV_NOPTS_VALUE" before the shorthand if ?
Or is there a risk that it will be evaluated as "AV_NOPTS_VALUE ? ifile->start_time : 0" ?

We think with both parenthesis in place is the most safe and readable option

[Balling]

Why should we remove the other existing parentheses?

Code style... Some other projects literally use clang format to remove all unneeded parentheses. This is high level code base, we do not care that people do not know C rules.

[Balling]

Yes, != has higher priority than ternary operation. Also (not that it matters here) everyone that is coding C needs to know that middle of the conditional operator (between ? and :) is parsed as if parenthesized: its precedence relative to ?: is ignored.

[Jens Viebig]

Understood, we will change the patch and resubmit