[FFmpeg-devel] [PATCH] avformat/rtmpproto: Pass rw_timeout to underlying transport protocol

[FFmpeg-devel] [PATCH] avformat/rtmpproto: Pass rw_timeout to underlying transport protocol

[Zhao Zhili]

From: Zhao Zhili <zhilizhao@tencent.com>

Signed-off-by: Zhao Zhili <zhilizhao@tencent.com>
---
 libavformat/rtmpproto.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/libavformat/rtmpproto.c b/libavformat/rtmpproto.c
index 98718bc6da..a0c6195eb2 100644
--- a/libavformat/rtmpproto.c
+++ b/libavformat/rtmpproto.c
@@ -2635,6 +2635,9 @@ static int rtmp_open(URLContext *s, const char *uri, int flags, AVDictionary **o
 
     if (rt->listen_timeout > 0)
         rt->listen = 1;
+    /* Pass rw_timeout to underlying transport protocol */
+    if (s->rw_timeout > 0)
+        av_dict_set_int(opts, "rw_timeout", s->rw_timeout, 0);
 
     rt->is_input = !(flags & AVIO_FLAG_WRITE);
 
-- 
2.42.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".

Re: [FFmpeg-devel] [PATCH] avformat/rtmpproto: Pass rw_timeout to underlying transport protocol

[Zhao Zhili]

> On Nov 15, 2023, at 21:24, Zhao Zhili <quinkblack@foxmail.com> wrote:
> 
> From: Zhao Zhili <zhilizhao@tencent.com>
> 
> Signed-off-by: Zhao Zhili <zhilizhao@tencent.com>
> ---
> libavformat/rtmpproto.c | 3 +++
> 1 file changed, 3 insertions(+)
> 
> diff --git a/libavformat/rtmpproto.c b/libavformat/rtmpproto.c
> index 98718bc6da..a0c6195eb2 100644
> --- a/libavformat/rtmpproto.c
> +++ b/libavformat/rtmpproto.c
> @@ -2635,6 +2635,9 @@ static int rtmp_open(URLContext *s, const char *uri, int flags, AVDictionary **o
> 
>     if (rt->listen_timeout > 0)
>         rt->listen = 1;
> +    /* Pass rw_timeout to underlying transport protocol */
> +    if (s->rw_timeout > 0)
> +        av_dict_set_int(opts, "rw_timeout", s->rw_timeout, 0);

OK, I made a mistake. Since ffurl_open_whitelist copy from parent automatically,
this is a NOP. Will revert this commit.

> 
>     rt->is_input = !(flags & AVIO_FLAG_WRITE);
> 
> -- 
> 2.42.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".

Re: [FFmpeg-devel] [PATCH] avformat/rtmpproto: Pass rw_timeout to underlying transport protocol

[Martin Storsjö]

On Wed, 22 Nov 2023, Zhao Zhili wrote:

>
>
>> On Nov 15, 2023, at 21:24, Zhao Zhili <quinkblack@foxmail.com> wrote:
>>
>> From: Zhao Zhili <zhilizhao@tencent.com>
>>
>> Signed-off-by: Zhao Zhili <zhilizhao@tencent.com>
>> ---
>> libavformat/rtmpproto.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/libavformat/rtmpproto.c b/libavformat/rtmpproto.c
>> index 98718bc6da..a0c6195eb2 100644
>> --- a/libavformat/rtmpproto.c
>> +++ b/libavformat/rtmpproto.c
>> @@ -2635,6 +2635,9 @@ static int rtmp_open(URLContext *s, const char *uri, int flags, AVDictionary **o
>>
>>     if (rt->listen_timeout > 0)
>>         rt->listen = 1;
>> +    /* Pass rw_timeout to underlying transport protocol */
>> +    if (s->rw_timeout > 0)
>> +        av_dict_set_int(opts, "rw_timeout", s->rw_timeout, 0);
>
> OK, I made a mistake. Since ffurl_open_whitelist copy from parent automatically,
> this is a NOP. Will revert this commit.

Thanks! I was just starting to look into this, to confirm this as well, 
but it's good if you've come to the same conclusion already.

It's interesting how we end up with similar changes for this multiple 
times, see https://ffmpeg.org/pipermail/ffmpeg-devel/2023-July/312362.html 
for another similar patch a few months ago, when it should be working 
already.

The fact that so many protocols have similar but vaguely different timeout 
options, each defined as a per-protocol private option, is a bit of a 
mess; that's why this approach, of actually sharing the same variable 
through the URLContext hierarchy, tries to avoid that to some extent.

// Martin

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avformat/rtmpproto: Pass rw_timeout to underlying transport protocol

[Zhao Zhili]

> On Nov 22, 2023, at 17:33, Martin Storsjö <martin@martin.st> wrote:
> 
> On Wed, 22 Nov 2023, Zhao Zhili wrote:
> 
>> 
>> 
>>> On Nov 15, 2023, at 21:24, Zhao Zhili <quinkblack@foxmail.com> wrote:
>>> 
>>> From: Zhao Zhili <zhilizhao@tencent.com>
>>> 
>>> Signed-off-by: Zhao Zhili <zhilizhao@tencent.com>
>>> ---
>>> libavformat/rtmpproto.c | 3 +++
>>> 1 file changed, 3 insertions(+)
>>> 
>>> diff --git a/libavformat/rtmpproto.c b/libavformat/rtmpproto.c
>>> index 98718bc6da..a0c6195eb2 100644
>>> --- a/libavformat/rtmpproto.c
>>> +++ b/libavformat/rtmpproto.c
>>> @@ -2635,6 +2635,9 @@ static int rtmp_open(URLContext *s, const char *uri, int flags, AVDictionary **o
>>> 
>>>    if (rt->listen_timeout > 0)
>>>        rt->listen = 1;
>>> +    /* Pass rw_timeout to underlying transport protocol */
>>> +    if (s->rw_timeout > 0)
>>> +        av_dict_set_int(opts, "rw_timeout", s->rw_timeout, 0);
>> 
>> OK, I made a mistake. Since ffurl_open_whitelist copy from parent automatically,
>> this is a NOP. Will revert this commit.
> 
> Thanks! I was just starting to look into this, to confirm this as well, but it's good if you've come to the same conclusion already.
> 
> It's interesting how we end up with similar changes for this multiple times, see https://ffmpeg.org/pipermail/ffmpeg-devel/2023-July/312362.html for another similar patch a few months ago, when it should be working already.
> 
> The fact that so many protocols have similar but vaguely different timeout options, each defined as a per-protocol private option, is a bit of a mess; that's why this approach, of actually sharing the same variable through the URLContext hierarchy, tries to avoid that to some extent.

I have taken code from ftp.c as reference:
```
        if (s->rw_timeout != -1) {
            av_dict_set_int(&opts, "timeout", s->rw_timeout, 0);
        } /* if option is not given, don't pass it and let tcp use its own default */
```

Now it’s obvious that code comes before 
```
commit fab8156b2f30666adabe227b3d7712fd193873b1
Author: Martin Storsjö
Date:   Sat Feb 28 02:00:50 2015 +0200

    avio: Copy URLContext generic options into child URLContexts
```

Should we remove that part from ftp.c?

It worth to note that there is a small difference between rw_timeout and timeout
option for TCP:

1. timeout apply to accept, read and write
2. rw_timeout apply to read and write, but not accept.

What do you think?

> 
> // Martin
> 
> _______________________________________________
> 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".

Re: [FFmpeg-devel] [PATCH] avformat/rtmpproto: Pass rw_timeout to underlying transport protocol

[Zhao Zhili]

> On Nov 22, 2023, at 17:40, Zhao Zhili <quinkblack@foxmail.com> wrote:
> 
> 
> 
>> On Nov 22, 2023, at 17:33, Martin Storsjö <martin@martin.st> wrote:
>> 
>> On Wed, 22 Nov 2023, Zhao Zhili wrote:
>> 
>>> 
>>> 
>>>> On Nov 15, 2023, at 21:24, Zhao Zhili <quinkblack@foxmail.com> wrote:
>>>> 
>>>> From: Zhao Zhili <zhilizhao@tencent.com>
>>>> 
>>>> Signed-off-by: Zhao Zhili <zhilizhao@tencent.com>
>>>> ---
>>>> libavformat/rtmpproto.c | 3 +++
>>>> 1 file changed, 3 insertions(+)
>>>> 
>>>> diff --git a/libavformat/rtmpproto.c b/libavformat/rtmpproto.c
>>>> index 98718bc6da..a0c6195eb2 100644
>>>> --- a/libavformat/rtmpproto.c
>>>> +++ b/libavformat/rtmpproto.c
>>>> @@ -2635,6 +2635,9 @@ static int rtmp_open(URLContext *s, const char *uri, int flags, AVDictionary **o
>>>> 
>>>>   if (rt->listen_timeout > 0)
>>>>       rt->listen = 1;
>>>> +    /* Pass rw_timeout to underlying transport protocol */
>>>> +    if (s->rw_timeout > 0)
>>>> +        av_dict_set_int(opts, "rw_timeout", s->rw_timeout, 0);
>>> 
>>> OK, I made a mistake. Since ffurl_open_whitelist copy from parent automatically,
>>> this is a NOP. Will revert this commit.
>> 
>> Thanks! I was just starting to look into this, to confirm this as well, but it's good if you've come to the same conclusion already.
>> 
>> It's interesting how we end up with similar changes for this multiple times, see https://ffmpeg.org/pipermail/ffmpeg-devel/2023-July/312362.html for another similar patch a few months ago, when it should be working already.
>> 
>> The fact that so many protocols have similar but vaguely different timeout options, each defined as a per-protocol private option, is a bit of a mess; that's why this approach, of actually sharing the same variable through the URLContext hierarchy, tries to avoid that to some extent.
> 
> I have taken code from ftp.c as reference:
> ```
>        if (s->rw_timeout != -1) {
>            av_dict_set_int(&opts, "timeout", s->rw_timeout, 0);
>        } /* if option is not given, don't pass it and let tcp use its own default */
> ```
> 
> Now it’s obvious that code comes before 
> ```
> commit fab8156b2f30666adabe227b3d7712fd193873b1
> Author: Martin Storsjö
> Date:   Sat Feb 28 02:00:50 2015 +0200
> 
>    avio: Copy URLContext generic options into child URLContexts
> ```
> 
> Should we remove that part from ftp.c?
> 
> It worth to note that there is a small difference between rw_timeout and timeout
> option for TCP:
> 
> 1. timeout apply to accept, read and write
> 2. rw_timeout apply to read and write, but not accept.

I mean connect.

> 
> What do you think?
> 
>> 
>> // Martin
>> 
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org <mailto:ffmpeg-devel@ffmpeg.org>
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>> 
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request@ffmpeg.org <mailto:ffmpeg-devel-request@ffmpeg.org> with subject "unsubscribe".
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org <mailto:ffmpeg-devel@ffmpeg.org>
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org <mailto: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".

Re: [FFmpeg-devel] [PATCH] avformat/rtmpproto: Pass rw_timeout to underlying transport protocol

[Martin Storsjö]

On Wed, 22 Nov 2023, Zhao Zhili wrote:

> I have taken code from ftp.c as reference:
> ```
>        if (s->rw_timeout != -1) {
>            av_dict_set_int(&opts, "timeout", s->rw_timeout, 0);
>        } /* if option is not given, don't pass it and let tcp use its own default */
> ```

Ah, I see!

> Now it’s obvious that code comes before 
> ```
> commit fab8156b2f30666adabe227b3d7712fd193873b1
> Author: Martin Storsjö
> Date:   Sat Feb 28 02:00:50 2015 +0200
>
>    avio: Copy URLContext generic options into child URLContexts
> ```
>
> Should we remove that part from ftp.c?
>
> It worth to note that there is a small difference between rw_timeout and timeout
> option for TCP:
>
> 1. timeout apply to accept, read and write
> 2. rw_timeout apply to read and write, but not accept.
>
> What do you think?

Hmm, I'm not entirely sure.

I wonder if it would be best to change the tcp protocol to apply the 
rw_timeout option for connect timeout as well (open_timeout). With so many 
different options on different levels, that all do almost the same but 
with subtle differences (as for which cases it applies to), it's really 
hard to use them properly.

It's kinda problematic if a user searches for a way to make sure that the 
protocol always would abort if things stall, at any point, for more than X 
seconds, the user finds and picks one option, and it only applies in some 
case but not others.

But changing existing behaviours can of course always break someone as 
well... Although I haven't looked through all the protocols and their 
individual uses of options for this, so I'm not sure how much potential 
breakage there would be if we'd try to unify it.

Then again, I also see that it's quite possible for someone to want to set 
one kind of timeout for open but a different one for stalling once the 
connection has been opened.


So overall, I don't have a firm settled opinion on this matter right now. 
But anything to unify the code, reducing the amount of per-protocol 
setting of almost similar but slightly different options, would be good. 
And ideally using less per-protocol code and more of the common shared 
framework within e.g. URLContext.

// Martin
_______________________________________________
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".