diff mbox series

[FFmpeg-devel,2/2] lavf/libsrt: deduplicate libsrt_network_wait_fd_timeout

Message ID tencent_5BA4685E00AD680E7F86AF7A38D5C2FDB70A@qq.com
State Superseded
Headers show
Series [FFmpeg-devel,1/2] lavf/network: add wait fd callback to ff_network_wait_fd_timeout | expand

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, 4:41 p.m. UTC 
---
 libavformat/libsrt.c | 45 +++++++++++++++++---------------------------
 1 file changed, 17 insertions(+), 28 deletions(-)

Comments

Marton Balint Feb. 7, 2021, 6:36 p.m. UTC | #1 
On Mon, 8 Feb 2021, Zhao Zhili wrote:

> ---
> libavformat/libsrt.c | 45 +++++++++++++++++---------------------------
> 1 file changed, 17 insertions(+), 28 deletions(-)

Hmm, it seems my latest srt patches conflics with this a bit, and you 
will have to somewhat rebase this, sorry. On the bright side, now only eid 
is passed to libsrt_network_wait_fd, so maybe you can pass eid instead of 
fd, especially since it is not always the context->eid that is used for 
wait.

Regards,
Marton

>
> diff --git a/libavformat/libsrt.c b/libavformat/libsrt.c
> index f73e7dbfa5..67e42bacc6 100644
> --- a/libavformat/libsrt.c
> +++ b/libavformat/libsrt.c
> @@ -164,12 +164,14 @@ static int libsrt_socket_nonblock(int socket, int enable)
>     return srt_setsockopt(socket, 0, SRTO_RCVSYN, &blocking, sizeof(blocking));
> }
> 
> -static int libsrt_network_wait_fd(URLContext *h, int eid, int fd, int write)
> +static int libsrt_network_wait_fd(int fd, int write, void *ctx)
> {
>     int ret, len = 1, errlen = 1;
>     int modes = SRT_EPOLL_ERR | (write ? SRT_EPOLL_OUT : SRT_EPOLL_IN);
>     SRTSOCKET ready[1];
>     SRTSOCKET error[1];
> +    URLContext *h = ctx;
> +    int eid = ((SRTContext*)h->priv_data)->eid;
>
>     if (srt_epoll_add_usock(eid, fd, &modes) < 0)
>         return libsrt_neterrno(h);
> @@ -191,29 +193,16 @@ static int libsrt_network_wait_fd(URLContext *h, int eid, int fd, int write)
>     return ret;
> }
> 
> -/* TODO de-duplicate code from ff_network_wait_fd_timeout() */
> -
> -static int libsrt_network_wait_fd_timeout(URLContext *h, int eid, int fd, int write, int64_t timeout, AVIOInterruptCB *int_cb)
> +static int libsrt_network_wait_fd_timeout(URLContext *h, int fd, int write, int64_t timeout)
> {
> -    int ret;
> -    int64_t wait_start = 0;
> -
> -    while (1) {
> -        if (ff_check_interrupt(int_cb))
> -            return AVERROR_EXIT;
> -        ret = libsrt_network_wait_fd(h, eid, 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);
> -        }
> -    }
> +    struct NetworkWaitFdCB wait_cb = {
> +        .wait_fd = libsrt_network_wait_fd,
> +        .opaque = h
> +    };
> +    return ff_network_wait_fd_timeout(fd, write, timeout, &h->interrupt_callback, &wait_cb);
> }
> 
> -static int libsrt_listen(int eid, int fd, const struct sockaddr *addr, socklen_t addrlen, URLContext *h, int64_t timeout)
> +static int libsrt_listen(int fd, const struct sockaddr *addr, socklen_t addrlen, URLContext *h, int64_t timeout)
> {
>     int ret;
>     int reuse = 1;
> @@ -228,7 +217,7 @@ static int libsrt_listen(int eid, int fd, const struct sockaddr *addr, socklen_t
>     if (ret)
>         return libsrt_neterrno(h);
> 
> -    ret = libsrt_network_wait_fd_timeout(h, eid, fd, 1, timeout, &h->interrupt_callback);
> +    ret = libsrt_network_wait_fd_timeout(h, fd, 1, timeout);
>     if (ret < 0)
>         return ret;
> 
> @@ -241,7 +230,7 @@ static int libsrt_listen(int eid, int fd, const struct sockaddr *addr, socklen_t
>     return ret;
> }
> 
> -static int libsrt_listen_connect(int eid, int fd, const struct sockaddr *addr, socklen_t addrlen, int64_t timeout, URLContext *h, int will_try_next)
> +static int libsrt_listen_connect(int fd, const struct sockaddr *addr, socklen_t addrlen, int64_t timeout, URLContext *h, int will_try_next)
> {
>     int ret;
> 
> @@ -249,7 +238,7 @@ static int libsrt_listen_connect(int eid, int fd, const struct sockaddr *addr, s
>     if (ret < 0)
>         return libsrt_neterrno(h);
> 
> -    ret = libsrt_network_wait_fd_timeout(h, eid, fd, 1, timeout, &h->interrupt_callback);
> +    ret = libsrt_network_wait_fd_timeout(h, fd, 1, timeout);
>     if (ret < 0) {
>         if (will_try_next) {
>             av_log(h, AV_LOG_WARNING,
> @@ -438,7 +427,7 @@ static int libsrt_setup(URLContext *h, const char *uri, int flags)
>
>     if (s->mode == SRT_MODE_LISTENER) {
>         // multi-client
> -        if ((ret = libsrt_listen(s->eid, fd, cur_ai->ai_addr, cur_ai->ai_addrlen, h, s->listen_timeout)) < 0)
> +        if ((ret = libsrt_listen(fd, cur_ai->ai_addr, cur_ai->ai_addrlen, h, s->listen_timeout)) < 0)
>             goto fail1;
>         listen_fd = fd;
>         fd = ret;
> @@ -449,7 +438,7 @@ static int libsrt_setup(URLContext *h, const char *uri, int flags)
>                 goto fail1;
>         }
> 
> -        if ((ret = libsrt_listen_connect(s->eid, fd, cur_ai->ai_addr, cur_ai->ai_addrlen,
> +        if ((ret = libsrt_listen_connect(fd, cur_ai->ai_addr, cur_ai->ai_addrlen,
>                                           open_timeout, h, !!cur_ai->ai_next)) < 0) {
>             if (ret == AVERROR_EXIT)
>                 goto fail1;
> @@ -652,7 +641,7 @@ static int libsrt_read(URLContext *h, uint8_t *buf, int size)
>     int ret;
>
>     if (!(h->flags & AVIO_FLAG_NONBLOCK)) {
> -        ret = libsrt_network_wait_fd_timeout(h, s->eid, s->fd, 0, h->rw_timeout, &h->interrupt_callback);
> +        ret = libsrt_network_wait_fd_timeout(h, s->fd, 0, h->rw_timeout);
>         if (ret)
>             return ret;
>     }
> @@ -671,7 +660,7 @@ static int libsrt_write(URLContext *h, const uint8_t *buf, int size)
>     int ret;
>
>     if (!(h->flags & AVIO_FLAG_NONBLOCK)) {
> -        ret = libsrt_network_wait_fd_timeout(h, s->eid, s->fd, 1, h->rw_timeout, &h->interrupt_callback);
> +        ret = libsrt_network_wait_fd_timeout(h, s->fd, 1, h->rw_timeout);
>         if (ret)
>             return ret;
>     }
> -- 
> 2.27.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. 8, 2021, 3:13 a.m. UTC | #2 
> On Feb 8, 2021, at 2:36 AM, Marton Balint <cus@passwd.hu> wrote:
> 
> 
> 
> On Mon, 8 Feb 2021, Zhao Zhili wrote:
> 
>> ---
>> libavformat/libsrt.c | 45 +++++++++++++++++---------------------------
>> 1 file changed, 17 insertions(+), 28 deletions(-)
> 
> Hmm, it seems my latest srt patches conflics with this a bit, and you will have to somewhat rebase this, sorry. On the bright side, now only eid is passed to libsrt_network_wait_fd, so maybe you can pass eid instead of fd, especially since it is not always the context->eid that is used for wait.

Thanks for review. Rebase is done:

https://patchwork.ffmpeg.org/project/ffmpeg/patch/tencent_FF259AEF9F5AAB614E6C70EF2D7A9A684008@qq.com/

libsrt_network_wait_fd_timeout is implemented on top of ff_network_wait_fd_timeout
to keep minimum code changes. libsrt_network_wait_fd_timeout can be removed if 
add a NetworkWaitFdCB field to SRTContext but more code need to be changed.
What do you think?

> 
> Regards,
> Marton
> 
>> 
>> diff --git a/libavformat/libsrt.c b/libavformat/libsrt.c
>> index f73e7dbfa5..67e42bacc6 100644
>> --- a/libavformat/libsrt.c
>> +++ b/libavformat/libsrt.c
>> @@ -164,12 +164,14 @@ static int libsrt_socket_nonblock(int socket, int enable)
>>    return srt_setsockopt(socket, 0, SRTO_RCVSYN, &blocking, sizeof(blocking));
>> }
>> -static int libsrt_network_wait_fd(URLContext *h, int eid, int fd, int write)
>> +static int libsrt_network_wait_fd(int fd, int write, void *ctx)
>> {
>>    int ret, len = 1, errlen = 1;
>>    int modes = SRT_EPOLL_ERR | (write ? SRT_EPOLL_OUT : SRT_EPOLL_IN);
>>    SRTSOCKET ready[1];
>>    SRTSOCKET error[1];
>> +    URLContext *h = ctx;
>> +    int eid = ((SRTContext*)h->priv_data)->eid;
>> 
>>    if (srt_epoll_add_usock(eid, fd, &modes) < 0)
>>        return libsrt_neterrno(h);
>> @@ -191,29 +193,16 @@ static int libsrt_network_wait_fd(URLContext *h, int eid, int fd, int write)
>>    return ret;
>> }
>> -/* TODO de-duplicate code from ff_network_wait_fd_timeout() */
>> -
>> -static int libsrt_network_wait_fd_timeout(URLContext *h, int eid, int fd, int write, int64_t timeout, AVIOInterruptCB *int_cb)
>> +static int libsrt_network_wait_fd_timeout(URLContext *h, int fd, int write, int64_t timeout)
>> {
>> -    int ret;
>> -    int64_t wait_start = 0;
>> -
>> -    while (1) {
>> -        if (ff_check_interrupt(int_cb))
>> -            return AVERROR_EXIT;
>> -        ret = libsrt_network_wait_fd(h, eid, 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);
>> -        }
>> -    }
>> +    struct NetworkWaitFdCB wait_cb = {
>> +        .wait_fd = libsrt_network_wait_fd,
>> +        .opaque = h
>> +    };
>> +    return ff_network_wait_fd_timeout(fd, write, timeout, &h->interrupt_callback, &wait_cb);
>> }
>> -static int libsrt_listen(int eid, int fd, const struct sockaddr *addr, socklen_t addrlen, URLContext *h, int64_t timeout)
>> +static int libsrt_listen(int fd, const struct sockaddr *addr, socklen_t addrlen, URLContext *h, int64_t timeout)
>> {
>>    int ret;
>>    int reuse = 1;
>> @@ -228,7 +217,7 @@ static int libsrt_listen(int eid, int fd, const struct sockaddr *addr, socklen_t
>>    if (ret)
>>        return libsrt_neterrno(h);
>> -    ret = libsrt_network_wait_fd_timeout(h, eid, fd, 1, timeout, &h->interrupt_callback);
>> +    ret = libsrt_network_wait_fd_timeout(h, fd, 1, timeout);
>>    if (ret < 0)
>>        return ret;
>> @@ -241,7 +230,7 @@ static int libsrt_listen(int eid, int fd, const struct sockaddr *addr, socklen_t
>>    return ret;
>> }
>> -static int libsrt_listen_connect(int eid, int fd, const struct sockaddr *addr, socklen_t addrlen, int64_t timeout, URLContext *h, int will_try_next)
>> +static int libsrt_listen_connect(int fd, const struct sockaddr *addr, socklen_t addrlen, int64_t timeout, URLContext *h, int will_try_next)
>> {
>>    int ret;
>> @@ -249,7 +238,7 @@ static int libsrt_listen_connect(int eid, int fd, const struct sockaddr *addr, s
>>    if (ret < 0)
>>        return libsrt_neterrno(h);
>> -    ret = libsrt_network_wait_fd_timeout(h, eid, fd, 1, timeout, &h->interrupt_callback);
>> +    ret = libsrt_network_wait_fd_timeout(h, fd, 1, timeout);
>>    if (ret < 0) {
>>        if (will_try_next) {
>>            av_log(h, AV_LOG_WARNING,
>> @@ -438,7 +427,7 @@ static int libsrt_setup(URLContext *h, const char *uri, int flags)
>> 
>>    if (s->mode == SRT_MODE_LISTENER) {
>>        // multi-client
>> -        if ((ret = libsrt_listen(s->eid, fd, cur_ai->ai_addr, cur_ai->ai_addrlen, h, s->listen_timeout)) < 0)
>> +        if ((ret = libsrt_listen(fd, cur_ai->ai_addr, cur_ai->ai_addrlen, h, s->listen_timeout)) < 0)
>>            goto fail1;
>>        listen_fd = fd;
>>        fd = ret;
>> @@ -449,7 +438,7 @@ static int libsrt_setup(URLContext *h, const char *uri, int flags)
>>                goto fail1;
>>        }
>> -        if ((ret = libsrt_listen_connect(s->eid, fd, cur_ai->ai_addr, cur_ai->ai_addrlen,
>> +        if ((ret = libsrt_listen_connect(fd, cur_ai->ai_addr, cur_ai->ai_addrlen,
>>                                          open_timeout, h, !!cur_ai->ai_next)) < 0) {
>>            if (ret == AVERROR_EXIT)
>>                goto fail1;
>> @@ -652,7 +641,7 @@ static int libsrt_read(URLContext *h, uint8_t *buf, int size)
>>    int ret;
>> 
>>    if (!(h->flags & AVIO_FLAG_NONBLOCK)) {
>> -        ret = libsrt_network_wait_fd_timeout(h, s->eid, s->fd, 0, h->rw_timeout, &h->interrupt_callback);
>> +        ret = libsrt_network_wait_fd_timeout(h, s->fd, 0, h->rw_timeout);
>>        if (ret)
>>            return ret;
>>    }
>> @@ -671,7 +660,7 @@ static int libsrt_write(URLContext *h, const uint8_t *buf, int size)
>>    int ret;
>> 
>>    if (!(h->flags & AVIO_FLAG_NONBLOCK)) {
>> -        ret = libsrt_network_wait_fd_timeout(h, s->eid, s->fd, 1, h->rw_timeout, &h->interrupt_callback);
>> +        ret = libsrt_network_wait_fd_timeout(h, s->fd, 1, h->rw_timeout);
>>        if (ret)
>>            return ret;
>>    }
>> -- 
>> 2.27.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 Feb. 8, 2021, 9:57 p.m. UTC | #3 
On Mon, 8 Feb 2021, "zhilizhao(赵志立)" wrote:

>
>
>> On Feb 8, 2021, at 2:36 AM, Marton Balint <cus@passwd.hu> wrote:
>> 
>> 
>> 
>> On Mon, 8 Feb 2021, Zhao Zhili wrote:
>> 
>>> ---
>>> libavformat/libsrt.c | 45 +++++++++++++++++---------------------------
>>> 1 file changed, 17 insertions(+), 28 deletions(-)
>> 
>> Hmm, it seems my latest srt patches conflics with this a bit, and you will have to somewhat rebase this, sorry. On the bright side, now only eid is passed to libsrt_network_wait_fd, so maybe you can pass eid instead of fd, especially since it is not always the context->eid that is used for wait.
>
> Thanks for review. Rebase is done:
>
> https://patchwork.ffmpeg.org/project/ffmpeg/patch/tencent_FF259AEF9F5AAB614E6C70EF2D7A9A684008@qq.com/
>
> libsrt_network_wait_fd_timeout is implemented on top of ff_network_wait_fd_timeout
> to keep minimum code changes. libsrt_network_wait_fd_timeout can be removed if 
> add a NetworkWaitFdCB field to SRTContext but more code need to be changed.
> What do you think?

I think it is fine as is.

Thanks,
Marton
diff mbox series

Patch

diff --git a/libavformat/libsrt.c b/libavformat/libsrt.c
index f73e7dbfa5..67e42bacc6 100644
--- a/libavformat/libsrt.c
+++ b/libavformat/libsrt.c
@@ -164,12 +164,14 @@  static int libsrt_socket_nonblock(int socket, int enable)
     return srt_setsockopt(socket, 0, SRTO_RCVSYN, &blocking, sizeof(blocking));
 }
 
-static int libsrt_network_wait_fd(URLContext *h, int eid, int fd, int write)
+static int libsrt_network_wait_fd(int fd, int write, void *ctx)
 {
     int ret, len = 1, errlen = 1;
     int modes = SRT_EPOLL_ERR | (write ? SRT_EPOLL_OUT : SRT_EPOLL_IN);
     SRTSOCKET ready[1];
     SRTSOCKET error[1];
+    URLContext *h = ctx;
+    int eid = ((SRTContext*)h->priv_data)->eid;
 
     if (srt_epoll_add_usock(eid, fd, &modes) < 0)
         return libsrt_neterrno(h);
@@ -191,29 +193,16 @@  static int libsrt_network_wait_fd(URLContext *h, int eid, int fd, int write)
     return ret;
 }
 
-/* TODO de-duplicate code from ff_network_wait_fd_timeout() */
-
-static int libsrt_network_wait_fd_timeout(URLContext *h, int eid, int fd, int write, int64_t timeout, AVIOInterruptCB *int_cb)
+static int libsrt_network_wait_fd_timeout(URLContext *h, int fd, int write, int64_t timeout)
 {
-    int ret;
-    int64_t wait_start = 0;
-
-    while (1) {
-        if (ff_check_interrupt(int_cb))
-            return AVERROR_EXIT;
-        ret = libsrt_network_wait_fd(h, eid, 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);
-        }
-    }
+    struct NetworkWaitFdCB wait_cb = {
+        .wait_fd = libsrt_network_wait_fd,
+        .opaque = h
+    };
+    return ff_network_wait_fd_timeout(fd, write, timeout, &h->interrupt_callback, &wait_cb);
 }
 
-static int libsrt_listen(int eid, int fd, const struct sockaddr *addr, socklen_t addrlen, URLContext *h, int64_t timeout)
+static int libsrt_listen(int fd, const struct sockaddr *addr, socklen_t addrlen, URLContext *h, int64_t timeout)
 {
     int ret;
     int reuse = 1;
@@ -228,7 +217,7 @@  static int libsrt_listen(int eid, int fd, const struct sockaddr *addr, socklen_t
     if (ret)
         return libsrt_neterrno(h);
 
-    ret = libsrt_network_wait_fd_timeout(h, eid, fd, 1, timeout, &h->interrupt_callback);
+    ret = libsrt_network_wait_fd_timeout(h, fd, 1, timeout);
     if (ret < 0)
         return ret;
 
@@ -241,7 +230,7 @@  static int libsrt_listen(int eid, int fd, const struct sockaddr *addr, socklen_t
     return ret;
 }
 
-static int libsrt_listen_connect(int eid, int fd, const struct sockaddr *addr, socklen_t addrlen, int64_t timeout, URLContext *h, int will_try_next)
+static int libsrt_listen_connect(int fd, const struct sockaddr *addr, socklen_t addrlen, int64_t timeout, URLContext *h, int will_try_next)
 {
     int ret;
 
@@ -249,7 +238,7 @@  static int libsrt_listen_connect(int eid, int fd, const struct sockaddr *addr, s
     if (ret < 0)
         return libsrt_neterrno(h);
 
-    ret = libsrt_network_wait_fd_timeout(h, eid, fd, 1, timeout, &h->interrupt_callback);
+    ret = libsrt_network_wait_fd_timeout(h, fd, 1, timeout);
     if (ret < 0) {
         if (will_try_next) {
             av_log(h, AV_LOG_WARNING,
@@ -438,7 +427,7 @@  static int libsrt_setup(URLContext *h, const char *uri, int flags)
 
     if (s->mode == SRT_MODE_LISTENER) {
         // multi-client
-        if ((ret = libsrt_listen(s->eid, fd, cur_ai->ai_addr, cur_ai->ai_addrlen, h, s->listen_timeout)) < 0)
+        if ((ret = libsrt_listen(fd, cur_ai->ai_addr, cur_ai->ai_addrlen, h, s->listen_timeout)) < 0)
             goto fail1;
         listen_fd = fd;
         fd = ret;
@@ -449,7 +438,7 @@  static int libsrt_setup(URLContext *h, const char *uri, int flags)
                 goto fail1;
         }
 
-        if ((ret = libsrt_listen_connect(s->eid, fd, cur_ai->ai_addr, cur_ai->ai_addrlen,
+        if ((ret = libsrt_listen_connect(fd, cur_ai->ai_addr, cur_ai->ai_addrlen,
                                           open_timeout, h, !!cur_ai->ai_next)) < 0) {
             if (ret == AVERROR_EXIT)
                 goto fail1;
@@ -652,7 +641,7 @@  static int libsrt_read(URLContext *h, uint8_t *buf, int size)
     int ret;
 
     if (!(h->flags & AVIO_FLAG_NONBLOCK)) {
-        ret = libsrt_network_wait_fd_timeout(h, s->eid, s->fd, 0, h->rw_timeout, &h->interrupt_callback);
+        ret = libsrt_network_wait_fd_timeout(h, s->fd, 0, h->rw_timeout);
         if (ret)
             return ret;
     }
@@ -671,7 +660,7 @@  static int libsrt_write(URLContext *h, const uint8_t *buf, int size)
     int ret;
 
     if (!(h->flags & AVIO_FLAG_NONBLOCK)) {
-        ret = libsrt_network_wait_fd_timeout(h, s->eid, s->fd, 1, h->rw_timeout, &h->interrupt_callback);
+        ret = libsrt_network_wait_fd_timeout(h, s->fd, 1, h->rw_timeout);
         if (ret)
             return ret;
     }