diff mbox series

[FFmpeg-devel,2/2] avformat/udp: cancel pending IO on win32 manually

Message ID 20200126212235.29546-2-cus@passwd.hu
State Accepted
Headers show
Series [FFmpeg-devel,1/2] avformat/udp: remove setting cancel state from the TX thread
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Marton Balint Jan. 26, 2020, 9:22 p.m. UTC
recvfrom() is not a cancellation point in pthreads-win32, see
https://sourceware.org/pthreads-win32/manual/pthread_cancel.html

In order to be able to cancel the reader thread on Win32 properly we first
shutdown the socket then call CancelIoEx to abort pending IO. Subsequent
recvfrom() calls will fail with WSAESHUTDOWN causing the thread to exit.

Fixes ticket #5717.

Signed-off-by: Marton Balint <cus@passwd.hu>
---
 libavformat/udp.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

Comments

Mark Himsley Feb. 3, 2020, 10:42 a.m. UTC | #1
On Sun, 26 Jan 2020 at 21:22, Marton Balint <cus@passwd.hu> wrote:
>
> recvfrom() is not a cancellation point in pthreads-win32, see
> https://sourceware.org/pthreads-win32/manual/pthread_cancel.html
>
> In order to be able to cancel the reader thread on Win32 properly we first
> shutdown the socket then call CancelIoEx to abort pending IO. Subsequent
> recvfrom() calls will fail with WSAESHUTDOWN causing the thread to exit.
>
> Fixes ticket #5717.
>
> Signed-off-by: Marton Balint <cus@passwd.hu>
> ---
>  libavformat/udp.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/libavformat/udp.c b/libavformat/udp.c
> index 85c9e3a900..23c3773c64 100644
> --- a/libavformat/udp.c
> +++ b/libavformat/udp.c
> @@ -1069,8 +1069,17 @@ static int udp_close(URLContext *h)
>      if (s->thread_started) {
>          int ret;
>          // Cancel only read, as write has been signaled as success to the user
> -        if (h->flags & AVIO_FLAG_READ)
> +        if (h->flags & AVIO_FLAG_READ) {
> +#ifdef _WIN32
> +            /* recvfrom() is not a cancellation point for win32, so we shutdown
> +             * the socket and abort pending IO, subsequent recvfrom() calls
> +             * will fail with WSAESHUTDOWN causing the thread to exit. */
> +            shutdown(s->udp_fd, SD_RECEIVE);
> +            CancelIoEx((HANDLE)(SOCKET)s->udp_fd, NULL);
> +#else
>              pthread_cancel(s->circular_buffer_thread);
> +#endif
> +        }
>          ret = pthread_join(s->circular_buffer_thread, NULL);
>          if (ret != 0)
>              av_log(h, AV_LOG_ERROR, "pthread_join(): %s\n", strerror(ret));
> --
> 2.16.4

This was applied as 53aa76686e7ff4f1f6625502503d7923cec8c10e, but that
commit fails to compile in my mingw cross-compile build.


x86_64-w64-mingw32-gcc -I. -I./ --static -DPTW32_STATIC_LIB
-D_WIN32_WINNT=0x0501 -I/opt/ffbuild/include -D_ISOC99_SOURCE
-D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -U__STRICT_ANSI__
-D__USE_MINGW_ANSI_STDIO=1 -D__printf__=__gnu_printf__
-D_WIN32_WINNT=0x0600 -D_POSIX_C_SOURCE=200112 -D_XOPEN_SOURCE=600
-DPIC -DOPJ_STATIC -DZLIB_CONST -DHAVE_AV_CONFIG_H -DBUILDING_avformat
--static -DPTW32_STATIC_LIB -D_WIN32_WINNT=0x0501
-I/opt/ffbuild/include  --static -DPTW32_STATIC_LIB
-D_WIN32_WINNT=0x0501 -std=c11 -fomit-frame-pointer -pthread
-I/opt/ffbuild/include -I/opt/ffbuild/include -I/opt/ffbuild/include
-I/opt/ffbuild/include/openjpeg-2.3 -I/opt/ffbuild/include/opus
-I/opt/ffbuild/include/opus -I/opt/ffbuild/include
-I/opt/ffbuild/include -I/opt/ffbuild/include -I/opt/ffbuild/include
-I/opt/ffbuild/include -I/opt/ffbuild/include -I/opt/ffbuild/include
-I/opt/ffbuild/include -I/opt/ffbuild/include -DLIBXML_STATIC
-I/opt/ffbuild/include/libxml2 -I/opt/ffbuild/include -g
-Wdeclaration-after-statement -Wall -Wdisabled-optimization
-Wpointer-arith -Wredundant-decls -Wwrite-strings -Wtype-limits
-Wundef -Wmissing-prototypes -Wno-pointer-to-int-cast
-Wstrict-prototypes -Wempty-body -Wno-parentheses -Wno-switch
-Wno-format-zero-length -Wno-pointer-sign -Wno-char-subscripts -O3
-fno-math-errno -fno-signed-zeros -fno-tree-vectorize
-Werror=format-security -Werror=implicit-function-declaration
-Werror=missing-prototypes -Werror=return-type -Werror=vla -Wformat
-fdiagnostics-color=auto -Wno-maybe-uninitialized  -MMD -MF
libavformat/udp.d -MT libavformat/udp.o -c -o libavformat/udp.o
libavformat/udp.c
In file included from libavformat/udp.c:65:0:
/opt/ffbuild/include/pthread.h:108:0: warning: "PTW32_LEVEL" redefined
 #define PTW32_LEVEL PTW32_LEVEL_MAX
 ^
/opt/ffbuild/include/pthread.h:95:0: note: this is the location of the
previous definition
 #define PTW32_LEVEL 1
 ^
In file included from /opt/ffbuild/include/pthread.h:299:0,
                 from libavformat/udp.c:65:
/opt/ffbuild/include/sched.h:64:0: warning: "PTW32_SCHED_LEVEL" redefined
 #define PTW32_SCHED_LEVEL PTW32_SCHED_LEVEL_MAX
 ^
/opt/ffbuild/include/sched.h:51:0: note: this is the location of the
previous definition
 #define PTW32_SCHED_LEVEL 1
 ^
libavformat/udp.c: In function 'udp_close':
libavformat/udp.c:1078:13: error: implicit declaration of function
'CancelIoEx' [-Werror=implicit-function-declaration]
             CancelIoEx((HANDLE)(SOCKET)s->udp_fd, NULL);
             ^
cc1: some warnings being treated as errors
ffbuild/common.mak:59: recipe for target 'libavformat/udp.o' failed
make: *** [libavformat/udp.o] Error 1




> _______________________________________________
> 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. 3, 2020, 5:18 p.m. UTC | #2
On Mon, 3 Feb 2020, Mark Himsley wrote:

> On Sun, 26 Jan 2020 at 21:22, Marton Balint <cus@passwd.hu> wrote:
>>
>> recvfrom() is not a cancellation point in pthreads-win32, see
>> https://sourceware.org/pthreads-win32/manual/pthread_cancel.html
>>
>> In order to be able to cancel the reader thread on Win32 properly we first
>> shutdown the socket then call CancelIoEx to abort pending IO. Subsequent
>> recvfrom() calls will fail with WSAESHUTDOWN causing the thread to exit.
>>
>> Fixes ticket #5717.
>>
>> Signed-off-by: Marton Balint <cus@passwd.hu>
>> ---
>>  libavformat/udp.c | 11 ++++++++++-
>>  1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/libavformat/udp.c b/libavformat/udp.c
>> index 85c9e3a900..23c3773c64 100644
>> --- a/libavformat/udp.c
>> +++ b/libavformat/udp.c
>> @@ -1069,8 +1069,17 @@ static int udp_close(URLContext *h)
>>      if (s->thread_started) {
>>          int ret;
>>          // Cancel only read, as write has been signaled as success to the user
>> -        if (h->flags & AVIO_FLAG_READ)
>> +        if (h->flags & AVIO_FLAG_READ) {
>> +#ifdef _WIN32
>> +            /* recvfrom() is not a cancellation point for win32, so we shutdown
>> +             * the socket and abort pending IO, subsequent recvfrom() calls
>> +             * will fail with WSAESHUTDOWN causing the thread to exit. */
>> +            shutdown(s->udp_fd, SD_RECEIVE);
>> +            CancelIoEx((HANDLE)(SOCKET)s->udp_fd, NULL);
>> +#else
>>              pthread_cancel(s->circular_buffer_thread);
>> +#endif
>> +        }
>>          ret = pthread_join(s->circular_buffer_thread, NULL);
>>          if (ret != 0)
>>              av_log(h, AV_LOG_ERROR, "pthread_join(): %s\n", strerror(ret));
>> --
>> 2.16.4
>
> This was applied as 53aa76686e7ff4f1f6625502503d7923cec8c10e, but that
> commit fails to compile in my mingw cross-compile build.
>
>
> x86_64-w64-mingw32-gcc -I. -I./ --static -DPTW32_STATIC_LIB
> -D_WIN32_WINNT=0x0501 -I/opt/ffbuild/include -D_ISOC99_SOURCE

It looks like you are still building for Windows XP, but that is no longer 
supported. _WIN32_WINNT should be at least 0x0600.

Regards,
Marton


> -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -U__STRICT_ANSI__
> -D__USE_MINGW_ANSI_STDIO=1 -D__printf__=__gnu_printf__
> -D_WIN32_WINNT=0x0600 -D_POSIX_C_SOURCE=200112 -D_XOPEN_SOURCE=600
> -DPIC -DOPJ_STATIC -DZLIB_CONST -DHAVE_AV_CONFIG_H -DBUILDING_avformat
> --static -DPTW32_STATIC_LIB -D_WIN32_WINNT=0x0501
> -I/opt/ffbuild/include  --static -DPTW32_STATIC_LIB
> -D_WIN32_WINNT=0x0501 -std=c11 -fomit-frame-pointer -pthread
> -I/opt/ffbuild/include -I/opt/ffbuild/include -I/opt/ffbuild/include
> -I/opt/ffbuild/include/openjpeg-2.3 -I/opt/ffbuild/include/opus
> -I/opt/ffbuild/include/opus -I/opt/ffbuild/include
> -I/opt/ffbuild/include -I/opt/ffbuild/include -I/opt/ffbuild/include
> -I/opt/ffbuild/include -I/opt/ffbuild/include -I/opt/ffbuild/include
> -I/opt/ffbuild/include -I/opt/ffbuild/include -DLIBXML_STATIC
> -I/opt/ffbuild/include/libxml2 -I/opt/ffbuild/include -g
> -Wdeclaration-after-statement -Wall -Wdisabled-optimization
> -Wpointer-arith -Wredundant-decls -Wwrite-strings -Wtype-limits
> -Wundef -Wmissing-prototypes -Wno-pointer-to-int-cast
> -Wstrict-prototypes -Wempty-body -Wno-parentheses -Wno-switch
> -Wno-format-zero-length -Wno-pointer-sign -Wno-char-subscripts -O3
> -fno-math-errno -fno-signed-zeros -fno-tree-vectorize
> -Werror=format-security -Werror=implicit-function-declaration
> -Werror=missing-prototypes -Werror=return-type -Werror=vla -Wformat
> -fdiagnostics-color=auto -Wno-maybe-uninitialized  -MMD -MF
> libavformat/udp.d -MT libavformat/udp.o -c -o libavformat/udp.o
> libavformat/udp.c
> In file included from libavformat/udp.c:65:0:
> /opt/ffbuild/include/pthread.h:108:0: warning: "PTW32_LEVEL" redefined
> #define PTW32_LEVEL PTW32_LEVEL_MAX
> ^
> /opt/ffbuild/include/pthread.h:95:0: note: this is the location of the
> previous definition
> #define PTW32_LEVEL 1
> ^
> In file included from /opt/ffbuild/include/pthread.h:299:0,
>                 from libavformat/udp.c:65:
> /opt/ffbuild/include/sched.h:64:0: warning: "PTW32_SCHED_LEVEL" redefined
> #define PTW32_SCHED_LEVEL PTW32_SCHED_LEVEL_MAX
> ^
> /opt/ffbuild/include/sched.h:51:0: note: this is the location of the
> previous definition
> #define PTW32_SCHED_LEVEL 1
> ^
> libavformat/udp.c: In function 'udp_close':
> libavformat/udp.c:1078:13: error: implicit declaration of function
> 'CancelIoEx' [-Werror=implicit-function-declaration]
>             CancelIoEx((HANDLE)(SOCKET)s->udp_fd, NULL);
>             ^
> cc1: some warnings being treated as errors
> ffbuild/common.mak:59: recipe for target 'libavformat/udp.o' failed
> make: *** [libavformat/udp.o] Error 1
>
>
>
>
>> _______________________________________________
>> 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".
>
>
>
> -- 
> Mark Himsley
> _______________________________________________
> 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".
Mark Himsley Feb. 3, 2020, 7:40 p.m. UTC | #3
On Mon, 3 Feb 2020 at 17:19, Marton Balint <cus@passwd.hu> wrote:
>
>
>
> On Mon, 3 Feb 2020, Mark Himsley wrote:
>
> > On Sun, 26 Jan 2020 at 21:22, Marton Balint <cus@passwd.hu> wrote:
> >>
> >> recvfrom() is not a cancellation point in pthreads-win32, see
> >> https://sourceware.org/pthreads-win32/manual/pthread_cancel.html
> >>
> >> In order to be able to cancel the reader thread on Win32 properly we first
> >> shutdown the socket then call CancelIoEx to abort pending IO. Subsequent
> >> recvfrom() calls will fail with WSAESHUTDOWN causing the thread to exit.
> >>
> >> Fixes ticket #5717.
> >>
> >> Signed-off-by: Marton Balint <cus@passwd.hu>
> >> ---
> >>  libavformat/udp.c | 11 ++++++++++-
> >>  1 file changed, 10 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/libavformat/udp.c b/libavformat/udp.c
> >> index 85c9e3a900..23c3773c64 100644
> >> --- a/libavformat/udp.c
> >> +++ b/libavformat/udp.c
> >> @@ -1069,8 +1069,17 @@ static int udp_close(URLContext *h)
> >>      if (s->thread_started) {
> >>          int ret;
> >>          // Cancel only read, as write has been signaled as success to the user
> >> -        if (h->flags & AVIO_FLAG_READ)
> >> +        if (h->flags & AVIO_FLAG_READ) {
> >> +#ifdef _WIN32
> >> +            /* recvfrom() is not a cancellation point for win32, so we shutdown
> >> +             * the socket and abort pending IO, subsequent recvfrom() calls
> >> +             * will fail with WSAESHUTDOWN causing the thread to exit. */
> >> +            shutdown(s->udp_fd, SD_RECEIVE);
> >> +            CancelIoEx((HANDLE)(SOCKET)s->udp_fd, NULL);
> >> +#else
> >>              pthread_cancel(s->circular_buffer_thread);
> >> +#endif
> >> +        }
> >>          ret = pthread_join(s->circular_buffer_thread, NULL);
> >>          if (ret != 0)
> >>              av_log(h, AV_LOG_ERROR, "pthread_join(): %s\n", strerror(ret));
> >> --
> >> 2.16.4
> >
> > This was applied as 53aa76686e7ff4f1f6625502503d7923cec8c10e, but that
> > commit fails to compile in my mingw cross-compile build.
> >
> >
> > x86_64-w64-mingw32-gcc -I. -I./ --static -DPTW32_STATIC_LIB
> > -D_WIN32_WINNT=0x0501 -I/opt/ffbuild/include -D_ISOC99_SOURCE
>
> It looks like you are still building for Windows XP, but that is no longer
> supported. _WIN32_WINNT should be at least 0x0600.

Thanks Marton.
I'll remember, Windows Vista and above only.
Thanks - I'll update my build script.


> Regards,
> Marton
>
>
> > -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -U__STRICT_ANSI__
> > -D__USE_MINGW_ANSI_STDIO=1 -D__printf__=__gnu_printf__
> > -D_WIN32_WINNT=0x0600 -D_POSIX_C_SOURCE=200112 -D_XOPEN_SOURCE=600
> > -DPIC -DOPJ_STATIC -DZLIB_CONST -DHAVE_AV_CONFIG_H -DBUILDING_avformat
> > --static -DPTW32_STATIC_LIB -D_WIN32_WINNT=0x0501
> > -I/opt/ffbuild/include  --static -DPTW32_STATIC_LIB
> > -D_WIN32_WINNT=0x0501 -std=c11 -fomit-frame-pointer -pthread
> > -I/opt/ffbuild/include -I/opt/ffbuild/include -I/opt/ffbuild/include
> > -I/opt/ffbuild/include/openjpeg-2.3 -I/opt/ffbuild/include/opus
> > -I/opt/ffbuild/include/opus -I/opt/ffbuild/include
> > -I/opt/ffbuild/include -I/opt/ffbuild/include -I/opt/ffbuild/include
> > -I/opt/ffbuild/include -I/opt/ffbuild/include -I/opt/ffbuild/include
> > -I/opt/ffbuild/include -I/opt/ffbuild/include -DLIBXML_STATIC
> > -I/opt/ffbuild/include/libxml2 -I/opt/ffbuild/include -g
> > -Wdeclaration-after-statement -Wall -Wdisabled-optimization
> > -Wpointer-arith -Wredundant-decls -Wwrite-strings -Wtype-limits
> > -Wundef -Wmissing-prototypes -Wno-pointer-to-int-cast
> > -Wstrict-prototypes -Wempty-body -Wno-parentheses -Wno-switch
> > -Wno-format-zero-length -Wno-pointer-sign -Wno-char-subscripts -O3
> > -fno-math-errno -fno-signed-zeros -fno-tree-vectorize
> > -Werror=format-security -Werror=implicit-function-declaration
> > -Werror=missing-prototypes -Werror=return-type -Werror=vla -Wformat
> > -fdiagnostics-color=auto -Wno-maybe-uninitialized  -MMD -MF
> > libavformat/udp.d -MT libavformat/udp.o -c -o libavformat/udp.o
> > libavformat/udp.c
> > In file included from libavformat/udp.c:65:0:
> > /opt/ffbuild/include/pthread.h:108:0: warning: "PTW32_LEVEL" redefined
> > #define PTW32_LEVEL PTW32_LEVEL_MAX
> > ^
> > /opt/ffbuild/include/pthread.h:95:0: note: this is the location of the
> > previous definition
> > #define PTW32_LEVEL 1
> > ^
> > In file included from /opt/ffbuild/include/pthread.h:299:0,
> >                 from libavformat/udp.c:65:
> > /opt/ffbuild/include/sched.h:64:0: warning: "PTW32_SCHED_LEVEL" redefined
> > #define PTW32_SCHED_LEVEL PTW32_SCHED_LEVEL_MAX
> > ^
> > /opt/ffbuild/include/sched.h:51:0: note: this is the location of the
> > previous definition
> > #define PTW32_SCHED_LEVEL 1
> > ^
> > libavformat/udp.c: In function 'udp_close':
> > libavformat/udp.c:1078:13: error: implicit declaration of function
> > 'CancelIoEx' [-Werror=implicit-function-declaration]
> >             CancelIoEx((HANDLE)(SOCKET)s->udp_fd, NULL);
> >             ^
> > cc1: some warnings being treated as errors
> > ffbuild/common.mak:59: recipe for target 'libavformat/udp.o' failed
> > make: *** [libavformat/udp.o] Error 1
> >
> >
> >
> > --
> > Mark Himsley
diff mbox series

Patch

diff --git a/libavformat/udp.c b/libavformat/udp.c
index 85c9e3a900..23c3773c64 100644
--- a/libavformat/udp.c
+++ b/libavformat/udp.c
@@ -1069,8 +1069,17 @@  static int udp_close(URLContext *h)
     if (s->thread_started) {
         int ret;
         // Cancel only read, as write has been signaled as success to the user
-        if (h->flags & AVIO_FLAG_READ)
+        if (h->flags & AVIO_FLAG_READ) {
+#ifdef _WIN32
+            /* recvfrom() is not a cancellation point for win32, so we shutdown
+             * the socket and abort pending IO, subsequent recvfrom() calls
+             * will fail with WSAESHUTDOWN causing the thread to exit. */
+            shutdown(s->udp_fd, SD_RECEIVE);
+            CancelIoEx((HANDLE)(SOCKET)s->udp_fd, NULL);
+#else
             pthread_cancel(s->circular_buffer_thread);
+#endif
+        }
         ret = pthread_join(s->circular_buffer_thread, NULL);
         if (ret != 0)
             av_log(h, AV_LOG_ERROR, "pthread_join(): %s\n", strerror(ret));