| 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. | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
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,
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?
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.
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.
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,
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?
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.
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?
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.
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.
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 --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);
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(-)