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

[Zhao Zhili <quinkblack@foxmail.com>]

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