diff mbox series

[FFmpeg-devel,EXTREMELY,IMPORTANT,PLEASE,DO,NOT,IGNORE] avfilter/framesync: do not pick AV_TIME_BASE for time base when not needed.

Message ID 20200201200549.4776-1-onemda@gmail.com
State New
Headers show
Series [FFmpeg-devel,EXTREMELY,IMPORTANT,PLEASE,DO,NOT,IGNORE] avfilter/framesync: do not pick AV_TIME_BASE for time base when not needed.
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

Commit Message

Paul B Mahol Feb. 1, 2020, 8:05 p.m. UTC
Fixes picking time base when all input time bases are same and for example
20833/500000.

Signed-off-by: Paul B Mahol <onemda@gmail.com>
---
 libavfilter/framesync.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Nicolas George Feb. 2, 2020, 10:41 a.m. UTC | #1
Paul B Mahol (12020-02-01):
> Fixes picking time base when all input time bases are same and for example
> 20833/500000.
> 
> Signed-off-by: Paul B Mahol <onemda@gmail.com>
> ---
>  libavfilter/framesync.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavfilter/framesync.c b/libavfilter/framesync.c
> index bc95f7d904..94abb469e6 100644
> --- a/libavfilter/framesync.c
> +++ b/libavfilter/framesync.c
> @@ -144,7 +144,7 @@ int ff_framesync_configure(FFFrameSync *fs)
>                  if (fs->time_base.num) {
>                      gcd = av_gcd(fs->time_base.den, fs->in[i].time_base.den);
>                      lcm = (fs->time_base.den / gcd) * fs->in[i].time_base.den;
> -                    if (lcm < AV_TIME_BASE / 2) {
> +                    if (lcm <= AV_TIME_BASE / 2) {
>                          fs->time_base.den = lcm;
>                          fs->time_base.num = av_gcd(fs->time_base.num,
>                                                     fs->in[i].time_base.num);

Picking AV_TIME_BASE for a decimal approximation of 1/24 seems like the
right thing to do. Also, since this change only affects the case with
equality, there is no loss of precision. I'll keep the code as is,
unless there's a better reason.

Regards,
Paul B Mahol Feb. 2, 2020, 10:45 a.m. UTC | #2
On 2/2/20, Nicolas George <george@nsup.org> wrote:
> Paul B Mahol (12020-02-01):
>> Fixes picking time base when all input time bases are same and for
>> example
>> 20833/500000.
>>
>> Signed-off-by: Paul B Mahol <onemda@gmail.com>
>> ---
>>  libavfilter/framesync.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libavfilter/framesync.c b/libavfilter/framesync.c
>> index bc95f7d904..94abb469e6 100644
>> --- a/libavfilter/framesync.c
>> +++ b/libavfilter/framesync.c
>> @@ -144,7 +144,7 @@ int ff_framesync_configure(FFFrameSync *fs)
>>                  if (fs->time_base.num) {
>>                      gcd = av_gcd(fs->time_base.den,
>> fs->in[i].time_base.den);
>>                      lcm = (fs->time_base.den / gcd) *
>> fs->in[i].time_base.den;
>> -                    if (lcm < AV_TIME_BASE / 2) {
>> +                    if (lcm <= AV_TIME_BASE / 2) {
>>                          fs->time_base.den = lcm;
>>                          fs->time_base.num = av_gcd(fs->time_base.num,
>>
>> fs->in[i].time_base.num);
>
> Picking AV_TIME_BASE for a decimal approximation of 1/24 seems like the
> right thing to do. Also, since this change only affects the case with
> equality, there is no loss of precision. I'll keep the code as is,
> unless there's a better reason.

Changing time base when all inputs have same time base seems strange
and excessive to me.
Is there something wrong with my patch or you just prefer status quo?
Nicolas George Feb. 2, 2020, 10:49 a.m. UTC | #3
Paul B Mahol (12020-02-02):
> Changing time base when all inputs have same time base seems strange
> and excessive to me.

This is true when the denominator is 500000, when it's 499999 and when
it's 500001. Expending efforts just for 500000 is not efficient,
especially since this one does not lead to a loss of precision.

> Is there something wrong with my patch or you just prefer status quo?

The current code seems more logical to me. Large round denominators are
a sign that the time base was not really rational in the first place.
Paul B Mahol Feb. 2, 2020, 12:09 p.m. UTC | #4
On 2/2/20, Nicolas George <george@nsup.org> wrote:
> Paul B Mahol (12020-02-02):
>> Changing time base when all inputs have same time base seems strange
>> and excessive to me.
>
> This is true when the denominator is 500000, when it's 499999 and when
> it's 500001. Expending efforts just for 500000 is not efficient,
> especially since this one does not lead to a loss of precision.
>
>> Is there something wrong with my patch or you just prefer status quo?
>
> The current code seems more logical to me. Large round denominators are
> a sign that the time base was not really rational in the first place.

What about special code that checks all time-bases are same and keeping it?

Changing time-base is far from ideal solution in such scenario for any usecase.
Nicolas George Feb. 2, 2020, 7:04 p.m. UTC | #5
Paul B Mahol (12020-02-02):
> What about special code that checks all time-bases are same and
> keeping it?
> 
> Changing time-base is far from ideal solution in such scenario for any
> usecase.

I consider any code or effort for time bases larger than, say, 1E5, to
be a complete waste.

Regards,
Paul B Mahol Feb. 2, 2020, 7:12 p.m. UTC | #6
On 2/2/20, Nicolas George <george@nsup.org> wrote:
> Paul B Mahol (12020-02-02):
>> What about special code that checks all time-bases are same and
>> keeping it?
>>
>> Changing time-base is far from ideal solution in such scenario for any
>> usecase.
>
> I consider any code or effort for time bases larger than, say, 1E5, to
> be a complete waste.
>

I will repeat my last question once again, is it ok for you to check that all
input time-bases are same and that not AV_TIME_BASE is used in such case?
Nicolas George Feb. 2, 2020, 7:16 p.m. UTC | #7
Paul B Mahol (12020-02-02):
> I will repeat my last question once again, is it ok for you to check that all
> input time-bases are same and that not AV_TIME_BASE is used in such case?

Already told you: waste of time.
Paul B Mahol Feb. 2, 2020, 7:58 p.m. UTC | #8
On 2/2/20, Nicolas George <george@nsup.org> wrote:
> Paul B Mahol (12020-02-02):
>> I will repeat my last question once again, is it ok for you to check that
>> all
>> input time-bases are same and that not AV_TIME_BASE is used in such case?
>
> Already told you: waste of time.

How so?
Paul B Mahol Feb. 5, 2020, 9:15 a.m. UTC | #9
On 2/2/20, Paul B Mahol <onemda@gmail.com> wrote:
> On 2/2/20, Nicolas George <george@nsup.org> wrote:
>> Paul B Mahol (12020-02-02):
>>> I will repeat my last question once again, is it ok for you to check
>>> that
>>> all
>>> input time-bases are same and that not AV_TIME_BASE is used in such
>>> case?
>>
>> Already told you: waste of time.
>
> How so?

I will write patch that fixes this. As unnecessary changing time-base
is wrong way around.
Nicolas George Feb. 6, 2020, 7:28 a.m. UTC | #10
Paul B Mahol (12020-02-05):
> I will write patch that fixes this. As unnecessary changing time-base
> is wrong way around.

I do not want several code paths, requiring extra testing in case of
change, unless it is necessary. There is no mystique about the time
base: the current code finds one that works, it is enough.
Paul B Mahol Feb. 6, 2020, 8:34 a.m. UTC | #11
On 2/6/20, Nicolas George <george@nsup.org> wrote:
> Paul B Mahol (12020-02-05):
>> I will write patch that fixes this. As unnecessary changing time-base
>> is wrong way around.
>
> I do not want several code paths, requiring extra testing in case of
> change, unless it is necessary. There is no mystique about the time
> base: the current code finds one that works, it is enough.
>

Who tells you that your personal opinion is higher valued than mine?
Current code does not work at all in such scenario and it utterly
fails to deliver
same presentation timestamps to output.

> --
>   Nicolas George
> _______________________________________________
> 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/framesync.c b/libavfilter/framesync.c
index bc95f7d904..94abb469e6 100644
--- a/libavfilter/framesync.c
+++ b/libavfilter/framesync.c
@@ -144,7 +144,7 @@  int ff_framesync_configure(FFFrameSync *fs)
                 if (fs->time_base.num) {
                     gcd = av_gcd(fs->time_base.den, fs->in[i].time_base.den);
                     lcm = (fs->time_base.den / gcd) * fs->in[i].time_base.den;
-                    if (lcm < AV_TIME_BASE / 2) {
+                    if (lcm <= AV_TIME_BASE / 2) {
                         fs->time_base.den = lcm;
                         fs->time_base.num = av_gcd(fs->time_base.num,
                                                    fs->in[i].time_base.num);