diff mbox

[FFmpeg-devel] avformat/unix: properly handling timeout at reading

Message ID 251ebccc-cf32-3981-a43b-63e276f8ef85@vivanet.hu
State New
Headers show

Commit Message

Bodecs Bela March 20, 2018, 7:42 p.m. UTC 
Dear All,

avio rw_timeout handling in retry_transfer_wrapper() is based on returning
EAGAIN from protocols' read function. unix_read function returns 0 in
case of no data was read. It happens even if timeout it set for a valid
value and thus rw_timeout handling can not work and wait for ever. This
patch fixes the return value to AVERROR(EAGAIN) when recv()
reads/returns zero value/data and timeout is set.

please review this patch!

thank you in advance,

Bela Bodecs
From f2fc0773500dd617a481ad67a195a811577f578a Mon Sep 17 00:00:00 2001
From: Bela Bodecs <bodecsb@vivanet.hu>
Date: Tue, 20 Mar 2018 20:38:22 +0100
Subject: [PATCH] avformat/unix: properly handling timeout at reading

avio rw_timeout handling in retry_transfer_wrapper is based on returning
EAGAIN from protocols' read function. unix_read function returns 0 in
case of no data was read. It happens even if timeout it set for a valid
value and thus rw_timeout handling can not work and wait for ever. This
patch fixes the return value to AVERROR(EAGAIN) when recv()
reads/returns zero value/data and timeout is set.

Signed-off-by: Bela Bodecs <bodecsb@vivanet.hu>
---
 libavformat/unix.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Nicolas George March 20, 2018, 7:50 p.m. UTC | #1 
Bodecs Bela (2018-03-20):
> avio rw_timeout handling in retry_transfer_wrapper() is based on returning
> EAGAIN from protocols' read function. unix_read function returns 0 in
> case of no data was read. It happens even if timeout it set for a valid
> value and thus rw_timeout handling can not work and wait for ever. This
> patch fixes the return value to AVERROR(EAGAIN) when recv()
> reads/returns zero value/data and timeout is set.

I am pretty sure this is not correct. Can you tell how you did observe
that?

Regards,
Bodecs Bela March 20, 2018, 7:56 p.m. UTC | #2 
2018.03.20. 20:50 keltezéssel, Nicolas George írta:
> Bodecs Bela (2018-03-20):
>> avio rw_timeout handling in retry_transfer_wrapper() is based on returning
>> EAGAIN from protocols' read function. unix_read function returns 0 in
>> case of no data was read. It happens even if timeout it set for a valid
>> value and thus rw_timeout handling can not work and wait for ever. This
>> patch fixes the return value to AVERROR(EAGAIN) when recv()
>> reads/returns zero value/data and timeout is set.
> I am pretty sure this is not correct. Can you tell how you did observe
> that?
>
> Regards,
see the retry_transfer_wrapper() in avio.c If

ret = transfer_func(h, buf + len, size - len);

is always zero, so it gets into infinite loop.

transfer_func is unix_read in case of unix protocol.

bb
>
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Nicolas George March 20, 2018, 7:58 p.m. UTC | #3 
Bodecs Bela (2018-03-20):
> see the retry_transfer_wrapper() in avio.c If
> 
> ret = transfer_func(h, buf + len, size - len);
> 
> is always zero, so it gets into infinite loop.
> 
> transfer_func is unix_read in case of unix protocol.

Let me be more accurate: I am pretty sure you analysis is wrong as
recv() does not return 0 on timeout. Please tell how you came to that
conclusion.

Regards,
Bodecs Bela March 20, 2018, 8 p.m. UTC | #4 
2018.03.20. 20:56 keltezéssel, Bodecs Bela írta:
>
>
> 2018.03.20. 20:50 keltezéssel, Nicolas George írta:
>> Bodecs Bela (2018-03-20):
>>> avio rw_timeout handling in retry_transfer_wrapper() is based on 
>>> returning
>>> EAGAIN from protocols' read function. unix_read function returns 0 in
>>> case of no data was read. It happens even if timeout it set for a valid
>>> value and thus rw_timeout handling can not work and wait for ever. This
>>> patch fixes the return value to AVERROR(EAGAIN) when recv()
>>> reads/returns zero value/data and timeout is set.
>> I am pretty sure this is not correct. Can you tell how you did observe
>> that?
>>
>> Regards,
> see the retry_transfer_wrapper() in avio.c If
>
> ret = transfer_func(h, buf + len, size - len);
>
> is always zero, so it gets into infinite loop.
>
> transfer_func is unix_read in case of unix protocol.
>
originally I thought to fix the retry_transfer_wrapper() function to 
handle zero as EAGAIN but on devel IRC it was suggsted to fix unix proto.

bb

> bb
>>
>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Bodecs Bela March 20, 2018, 8:04 p.m. UTC | #5 
2018.03.20. 20:58 keltezéssel, Nicolas George írta:
> Bodecs Bela (2018-03-20):
>> see the retry_transfer_wrapper() in avio.c If
>>
>> ret = transfer_func(h, buf + len, size - len);
>>
>> is always zero, so it gets into infinite loop.
>>
>> transfer_func is unix_read in case of unix protocol.
> Let me be more accurate: I am pretty sure you analysis is wrong as
> recv() does not return 0 on timeout. Please tell how you came to that
> conclusion.
I am sorry, but you misunderstood me. When recv() return 0 it means that 
no data was read.

this 0 value is returned to retry_transfer_wrapper(), then it call the 
unix_read again.

it is an infinite wait if data was never read again. But if user sets a 
timeout value,
it should quit the waiting loop after the preset time period elapsed.

this wait time checking is handled in retry_transfer_wrapper() but only 
if unix_read return EAGAIN

bb


> Regards,
>
>
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Nicolas George March 20, 2018, 8:23 p.m. UTC | #6 
Bodecs Bela (2018-03-20):
> I am sorry, but you misunderstood me. When recv() return 0 it means that no
> data was read.

And for the third time I am asking how you observe that. What is your
testing procedure?

Regards,
Bodecs Bela March 20, 2018, 9:20 p.m. UTC | #7 
2018.03.20. 21:23 keltezéssel, Nicolas George írta:
> Bodecs Bela (2018-03-20):
>> I am sorry, but you misunderstood me. When recv() return 0 it means that no
>> data was read.
> And for the third time I am asking how you observe that. What is your
> testing procedure?
>
> Regards,
>
>
ah, sorry. Let's see the following two command lines:

a.
ffmpeg -y -re -f lavfi -i 
"aevalsrc=exprs=0:nb_samples=1024:sample_rate=44100:channel_layout=stereo, 
aformat=sample_fmts=s16" -re -f lavfi -i 
"smptehdbars=size=640x360:rate=25, format=pix_fmts=yuv420p" -c:v 
rawvideo -c:a pcm_s16le -map 0 -map 1 -disposition:0 default -t 
00:00:10.0 -f nut -listen 1 unix:doc/examples/pause.unix

b.
ffmpeg -y -f nut -rw_timeout 10000000 unix:doc/examples/pause,unix -c 
copy -f framemd5 -

start a. and in a separate terminal window start b.  After 10 seconds a. 
terminates but b. remains running on infinite time. But I expected b. to 
terminate after 1 sec (1000000 microsec) when no data received because 
a. terminated.

If you run b in debugger, you wil see that ffmpeg running in infinite 
loop in retry_transfer_wrapper() / unix_read()
With my patch b. will terminates after 1 sec as I expected.

bb


>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Nicolas George March 20, 2018, 9:41 p.m. UTC | #8 
Bodecs Bela (2018-03-20):
> start a. and in a separate terminal window start b.  After 10 seconds a.
> terminates but b. remains running on infinite time.

Ok, got it.

>						      But I expected b. to
> terminate after 1 sec (1000000 microsec) when no data received because a.
> terminated.

Your expectation is wrong: it should terminate immediately.

The correct fix is to return AVERROR_EOF, but only if the socket type is
SOCK_STREAM.

Regards,
Bodecs Bela March 20, 2018, 10:22 p.m. UTC | #9 
2018.03.20. 22:41 keltezéssel, Nicolas George írta:
> Bodecs Bela (2018-03-20):
>> start a. and in a separate terminal window start b.  After 10 seconds a.
>> terminates but b. remains running on infinite time.
> Ok, got it.
>
>> 						      But I expected b. to
>> terminate after 1 sec (1000000 microsec) when no data received because a.
>> terminated.
> Your expectation is wrong: it should terminate immediately.
>
> The correct fix is to return AVERROR_EOF, but only if the socket type is
> SOCK_STREAM.
>
> Regards,
thank you for your help! I have sent the new pacth in a different 
thread, not to confuse others by the new patch title

bb
>
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Bodecs Bela March 21, 2018, 3:53 p.m. UTC | #10 
2018.03.20. 22:41 keltezéssel, Nicolas George írta:
> Bodecs Bela (2018-03-20):
>> start a. and in a separate terminal window start b.  After 10 seconds a.
>> terminates but b. remains running on infinite time.
> Ok, got it.
>
>> 						      But I expected b. to
>> terminate after 1 sec (1000000 microsec) when no data received because a.
>> terminated.
> Your expectation is wrong: it should terminate immediately.
>
> The correct fix is to return AVERROR_EOF, but only if the socket type is
> SOCK_STREAM.
>
> Regards,
Should not the unix_write behaves the  same? so, when

ret = send(s->fd, buf, size, MSG_NOSIGNAL);

return 0 in case of SOCK_STREAM type, then the unix_write() should 
return AVERROR_EOF instead of 0?

best regards,

bb

>
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Nicolas George March 21, 2018, 4:37 p.m. UTC | #11 
Bodecs Bela (2018-03-21):
> Should not the unix_write behaves the  same? so, when
> 
> ret = send(s->fd, buf, size, MSG_NOSIGNAL);
> 
> return 0 in case of SOCK_STREAM type, then the unix_write() should return
> AVERROR_EOF instead of 0?

No: there is no EOF reporting on writing, since the application is
deciding the contents.

Regards,
diff mbox

Patch

diff --git a/libavformat/unix.c b/libavformat/unix.c
index 4f01d14..1c1eec4 100644
--- a/libavformat/unix.c
+++ b/libavformat/unix.c
@@ -111,6 +111,8 @@  static int unix_read(URLContext *h, uint8_t *buf, int size)
             return ret;
     }
     ret = recv(s->fd, buf, size, 0);
+    if (!ret && s->timeout > -1)
+        return AVERROR(EAGAIN);
     return ret < 0 ? ff_neterrno() : ret;
 }