diff mbox series

[FFmpeg-devel,1/2] avformat/network: fix timeout inaccurate in wait_fd_timeout

Message ID tencent_4F202F4DBE4F85552AA75638A0960DCD5707@qq.com
State New
Headers show
Series [FFmpeg-devel,1/2] avformat/network: fix timeout inaccurate in wait_fd_timeout
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

Zhao Zhili Feb. 7, 2021, 1:18 p.m. UTC
The wait_start was about POLLING_TIME larger which leads to timeout
100ms late than the option setting.
---
 libavformat/network.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

Comments

Marton Balint Feb. 8, 2021, 10:03 p.m. UTC | #1
> The wait_start was about POLLING_TIME larger which leads to timeout
> 100ms late than the option setting.
> ---
> libavformat/network.c | 13 ++++++-------
> 1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/libavformat/network.c b/libavformat/network.c
> index 0f5a575f77..7a9a4be5bb 100644
> --- a/libavformat/network.c
> +++ b/libavformat/network.c
> @@ -78,7 +78,10 @@ int ff_network_wait_fd(int fd, int write)
> int ff_network_wait_fd_timeout(int fd, int write, int64_t timeout, AVIOInterruptCB *int_cb)
> {
>     int ret;
> -    int64_t wait_start = 0;
> +    int64_t wait_start;
> +
> +    if (timeout > 0)
> +        wait_start = av_gettime_relative();

I think we intentionally wanted to avoid calling gettime on the fast path.

>
>     while (1) {
>         if (ff_check_interrupt(int_cb))
> @@ -86,12 +89,8 @@ int ff_network_wait_fd_timeout(int fd, int write, int64_t timeout, AVIOInterrupt
>         ret = ff_network_wait_fd(fd, write);
>         if (ret != AVERROR(EAGAIN))
>             return ret;
> -        if (timeout > 0) {
> -            if (!wait_start)
> -                wait_start = av_gettime_relative();

Why not simply wait_start = av_gettime_relative() - POLLING_TIME? It seems 
to achieve the same result.

Thanks,
Marton

> -            else if (av_gettime_relative() - wait_start > timeout)
> -                return AVERROR(ETIMEDOUT);
> -        }
> +        if (timeout > 0 && (av_gettime_relative() - wait_start > timeout))
> +            return AVERROR(ETIMEDOUT);
>     }
> }
> 
> -- 
> 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".
Zhao Zhili Feb. 9, 2021, 2:36 a.m. UTC | #2
> On Feb 9, 2021, at 6:03 AM, Marton Balint <cus@passwd.hu> wrote:
> 
>> 
>> The wait_start was about POLLING_TIME larger which leads to timeout
>> 100ms late than the option setting.
>> ---
>> libavformat/network.c | 13 ++++++-------
>> 1 file changed, 6 insertions(+), 7 deletions(-)
>> 
>> diff --git a/libavformat/network.c b/libavformat/network.c
>> index 0f5a575f77..7a9a4be5bb 100644
>> --- a/libavformat/network.c
>> +++ b/libavformat/network.c
>> @@ -78,7 +78,10 @@ int ff_network_wait_fd(int fd, int write)
>> int ff_network_wait_fd_timeout(int fd, int write, int64_t timeout, AVIOInterruptCB *int_cb)
>> {
>>    int ret;
>> -    int64_t wait_start = 0;
>> +    int64_t wait_start;
>> +
>> +    if (timeout > 0)
>> +        wait_start = av_gettime_relative();
> 
> I think we intentionally wanted to avoid calling gettime on the fast path.
> 
>> 
>>    while (1) {
>>        if (ff_check_interrupt(int_cb))
>> @@ -86,12 +89,8 @@ int ff_network_wait_fd_timeout(int fd, int write, int64_t timeout, AVIOInterrupt
>>        ret = ff_network_wait_fd(fd, write);
>>        if (ret != AVERROR(EAGAIN))
>>            return ret;
>> -        if (timeout > 0) {
>> -            if (!wait_start)
>> -                wait_start = av_gettime_relative();
> 
> Why not simply wait_start = av_gettime_relative() - POLLING_TIME? It seems to achieve the same result.

Then it depends on the implementation of ff_network_wait_fd. After adding wait_fd as a callback,
there is no guarantee that it takes POLLING_TIME. It can be solved by adding a polling_time field
to NetworkWaitFdCB:

typedef struct NetworkWaitFdCB {
    int (*wait_fd)(int /*fd*/, int /*write*/, void* /*opaque*/);
    void *opaque;
    int polling_time;
} NetworkWaitFdCB;

This is trying to get rid of calling gettime once. We can go further and sacrifice the accuracy by:

int64_t loop_count = (timeout > 0) ? (timeout / polling_time) : timeout;
for (int64_t i = 0; loop_count <= 0 || i < loop_count; i++) {
    ...
}

In my opinion, either depends on gettime or loop_count, but not both.

> 
> Thanks,
> Marton
> 
>> -            else if (av_gettime_relative() - wait_start > timeout)
>> -                return AVERROR(ETIMEDOUT);
>> -        }
>> +        if (timeout > 0 && (av_gettime_relative() - wait_start > timeout))
>> +            return AVERROR(ETIMEDOUT);
>>    }
>> }
>> -- 
>> 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".
diff mbox series

Patch

diff --git a/libavformat/network.c b/libavformat/network.c
index 0f5a575f77..7a9a4be5bb 100644
--- a/libavformat/network.c
+++ b/libavformat/network.c
@@ -78,7 +78,10 @@  int ff_network_wait_fd(int fd, int write)
 int ff_network_wait_fd_timeout(int fd, int write, int64_t timeout, AVIOInterruptCB *int_cb)
 {
     int ret;
-    int64_t wait_start = 0;
+    int64_t wait_start;
+
+    if (timeout > 0)
+        wait_start = av_gettime_relative();
 
     while (1) {
         if (ff_check_interrupt(int_cb))
@@ -86,12 +89,8 @@  int ff_network_wait_fd_timeout(int fd, int write, int64_t timeout, AVIOInterrupt
         ret = ff_network_wait_fd(fd, write);
         if (ret != AVERROR(EAGAIN))
             return ret;
-        if (timeout > 0) {
-            if (!wait_start)
-                wait_start = av_gettime_relative();
-            else if (av_gettime_relative() - wait_start > timeout)
-                return AVERROR(ETIMEDOUT);
-        }
+        if (timeout > 0 && (av_gettime_relative() - wait_start > timeout))
+            return AVERROR(ETIMEDOUT);
     }
 }