diff mbox series

[FFmpeg-devel] avformat/utils: fix seek failed

Message ID tencent_E39F23AB404E2370BB2D5D9A3940FE9A2408@qq.com
State New
Headers show
Series [FFmpeg-devel] avformat/utils: fix seek failed
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

zhilizhao(赵志立) Dec. 31, 2020, 8:15 a.m. UTC
Rounding min_ts towards +infinity and max_ts towards -infinity can
make ts out of the [min_ts, max_ts] range, and then leads to seek
failure. For example,

max_ts = ts = -25057
time_base = (num = 1, den = 14112000)

After rescale, ts = -353604, and max_ts = -353605.
---
 libavformat/utils.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Marton Balint Dec. 31, 2020, 10:51 a.m. UTC | #1
On Thu, 31 Dec 2020, Zhao Zhili wrote:

> Rounding min_ts towards +infinity and max_ts towards -infinity can
> make ts out of the [min_ts, max_ts] range, and then leads to seek
> failure. For example,

I think this is intentional to adhere to the docs of the function:

"Seeking will be done so that the point from which all active streams
can be presented successfully will be closest to ts and within 
min/max_ts."

If you round down min_ts, you might return a packet with a timestamp less 
than the original min_ts, which contradicts the documentation.

Regards,
Marton


>
> max_ts = ts = -25057
> time_base = (num = 1, den = 14112000)
>
> After rescale, ts = -353604, and max_ts = -353605.
> ---
> libavformat/utils.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/libavformat/utils.c b/libavformat/utils.c
> index 503e583ad0..69a0f901b2 100644
> --- a/libavformat/utils.c
> +++ b/libavformat/utils.c
> @@ -2500,10 +2500,10 @@ int avformat_seek_file(AVFormatContext *s, int stream_index, int64_t min_ts,
>             ts = av_rescale_q(ts, AV_TIME_BASE_Q, time_base);
>             min_ts = av_rescale_rnd(min_ts, time_base.den,
>                                     time_base.num * (int64_t)AV_TIME_BASE,
> -                                    AV_ROUND_UP   | AV_ROUND_PASS_MINMAX);
> +                                    AV_ROUND_DOWN | AV_ROUND_PASS_MINMAX);
>             max_ts = av_rescale_rnd(max_ts, time_base.den,
>                                     time_base.num * (int64_t)AV_TIME_BASE,
> -                                    AV_ROUND_DOWN | AV_ROUND_PASS_MINMAX);
> +                                    AV_ROUND_UP | AV_ROUND_PASS_MINMAX);
>             stream_index = 0;
>         }
> 
> -- 
> 2.28.0
>
> _______________________________________________
> 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".
zhilizhao(赵志立) Dec. 31, 2020, 11:49 a.m. UTC | #2
> On Dec 31, 2020, at 6:51 PM, Marton Balint <cus@passwd.hu> wrote:
> 
> 
> 
> On Thu, 31 Dec 2020, Zhao Zhili wrote:
> 
>> Rounding min_ts towards +infinity and max_ts towards -infinity can
>> make ts out of the [min_ts, max_ts] range, and then leads to seek
>> failure. For example,
> 
> I think this is intentional to adhere to the docs of the function:
> 
> "Seeking will be done so that the point from which all active streams
> can be presented successfully will be closest to ts and within min/max_ts."
> 
> If you round down min_ts, you might return a packet with a timestamp less than the original min_ts, which contradicts the documentation.
> 

Yes, it’s not comply with the documentation strictly. On the other side, does
the documentation have strong guarantee about the rounding error? Rounding
error is less of a surprise than seek failure in my opinion.

ffmpeg.c seek_to_start() call avformat_seek_file() with max_ts equal to ts.
If avformat_seek_file() should be kept as it is, then the caller should be fixed.

> Regards,
> Marton
> 
> 
>> 
>> max_ts = ts = -25057
>> time_base = (num = 1, den = 14112000)
>> 
>> After rescale, ts = -353604, and max_ts = -353605.
>> ---
>> libavformat/utils.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/libavformat/utils.c b/libavformat/utils.c
>> index 503e583ad0..69a0f901b2 100644
>> --- a/libavformat/utils.c
>> +++ b/libavformat/utils.c
>> @@ -2500,10 +2500,10 @@ int avformat_seek_file(AVFormatContext *s, int stream_index, int64_t min_ts,
>>            ts = av_rescale_q(ts, AV_TIME_BASE_Q, time_base);
>>            min_ts = av_rescale_rnd(min_ts, time_base.den,
>>                                    time_base.num * (int64_t)AV_TIME_BASE,
>> -                                    AV_ROUND_UP   | AV_ROUND_PASS_MINMAX);
>> +                                    AV_ROUND_DOWN | AV_ROUND_PASS_MINMAX);
>>            max_ts = av_rescale_rnd(max_ts, time_base.den,
>>                                    time_base.num * (int64_t)AV_TIME_BASE,
>> -                                    AV_ROUND_DOWN | AV_ROUND_PASS_MINMAX);
>> +                                    AV_ROUND_UP | AV_ROUND_PASS_MINMAX);
>>            stream_index = 0;
>>        }
>> -- 
>> 2.28.0
>> 
>> _______________________________________________
>> 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".
> _______________________________________________
> 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".
Marton Balint Jan. 2, 2021, 12:54 p.m. UTC | #3
On Thu, 31 Dec 2020, "zhilizhao(赵志立)" wrote:

>
>
>> On Dec 31, 2020, at 6:51 PM, Marton Balint <cus@passwd.hu> wrote:
>> 
>> 
>> 
>> On Thu, 31 Dec 2020, Zhao Zhili wrote:
>> 
>>> Rounding min_ts towards +infinity and max_ts towards -infinity can
>>> make ts out of the [min_ts, max_ts] range, and then leads to seek
>>> failure. For example,
>> 
>> I think this is intentional to adhere to the docs of the function:
>> 
>> "Seeking will be done so that the point from which all active streams
>> can be presented successfully will be closest to ts and within min/max_ts."
>> 
>> If you round down min_ts, you might return a packet with a timestamp less than the original min_ts, which contradicts the documentation.
>> 
>
> Yes, it’s not comply with the documentation strictly. On the other side, does
> the documentation have strong guarantee about the rounding error? Rounding
> error is less of a surprise than seek failure in my opinion.

If the question is what the user would expect, then the answer is probably 
simple rounding for both min_ts and max_ts. That should also solve your 
error of ts becoming out of the [min_ts,max_ts] interval.

However in your example the time base of the stream is more precise than 
AV_TIME_BASE_Q, and that can cause unexpected behaviour regardless of the 
rouding you use. If start_time is not represented accurately, a 
seek attempt to start_time may well cause a seek after, or before the 
actual start of the stream, because the error is at most AV_TIME_BASE_Q, 
not AVStream->time_base.

Regards,
Marton

> ffmpeg.c seek_to_start() call avformat_seek_file() with max_ts equal to ts.
> If avformat_seek_file() should be kept as it is, then the caller should be fixed.
>
>> Regards,
>> Marton
>> 
>> 
>>> 
>>> max_ts = ts = -25057
>>> time_base = (num = 1, den = 14112000)
>>> 
>>> After rescale, ts = -353604, and max_ts = -353605.
>>> ---
>>> libavformat/utils.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/libavformat/utils.c b/libavformat/utils.c
>>> index 503e583ad0..69a0f901b2 100644
>>> --- a/libavformat/utils.c
>>> +++ b/libavformat/utils.c
>>> @@ -2500,10 +2500,10 @@ int avformat_seek_file(AVFormatContext *s, int stream_index, int64_t min_ts,
>>>            ts = av_rescale_q(ts, AV_TIME_BASE_Q, time_base);
>>>            min_ts = av_rescale_rnd(min_ts, time_base.den,
>>>                                    time_base.num * (int64_t)AV_TIME_BASE,
>>> -                                    AV_ROUND_UP   | AV_ROUND_PASS_MINMAX);
>>> +                                    AV_ROUND_DOWN | AV_ROUND_PASS_MINMAX);
>>>            max_ts = av_rescale_rnd(max_ts, time_base.den,
>>>                                    time_base.num * (int64_t)AV_TIME_BASE,
>>> -                                    AV_ROUND_DOWN | AV_ROUND_PASS_MINMAX);
>>> +                                    AV_ROUND_UP | AV_ROUND_PASS_MINMAX);
>>>            stream_index = 0;
>>>        }
>>> -- 
>>> 2.28.0
>>> 
>>> _______________________________________________
>>> 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".
>> _______________________________________________
>> 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".
>
> _______________________________________________
> 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/libavformat/utils.c b/libavformat/utils.c
index 503e583ad0..69a0f901b2 100644
--- a/libavformat/utils.c
+++ b/libavformat/utils.c
@@ -2500,10 +2500,10 @@  int avformat_seek_file(AVFormatContext *s, int stream_index, int64_t min_ts,
             ts = av_rescale_q(ts, AV_TIME_BASE_Q, time_base);
             min_ts = av_rescale_rnd(min_ts, time_base.den,
                                     time_base.num * (int64_t)AV_TIME_BASE,
-                                    AV_ROUND_UP   | AV_ROUND_PASS_MINMAX);
+                                    AV_ROUND_DOWN | AV_ROUND_PASS_MINMAX);
             max_ts = av_rescale_rnd(max_ts, time_base.den,
                                     time_base.num * (int64_t)AV_TIME_BASE,
-                                    AV_ROUND_DOWN | AV_ROUND_PASS_MINMAX);
+                                    AV_ROUND_UP | AV_ROUND_PASS_MINMAX);
             stream_index = 0;
         }