Re: [FFmpeg-devel] [PATCH 2/2] avformat/udp: Replace use of pthread_cancel.
> > > Ive reupped the current versions of each seperated patch for comment > ___ > > Hi, could this patch fix issue #5783? (https://trac.ffmpeg.org/ticket/5783) Thanks, MB ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/2] avformat/udp: Replace use of pthread_cancel.
On 4 December 2016 at 01:56, Matt Oliver wrote: > Indeed, in theory that would work. I always forget about these options. >> In my experience they do not work reliably, and I would argue against >> their use in portable code. For example, starting there: >> http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/ >> sys_socket.h.html >> can you tell me the type of the arguments to SO_RCVTIMEO? I know Linux >> man page tells us it is struct timeval, but that is not a portable >> standard. >> >> Also, these options are just a band aid to implement polling. > > > the same function on windows would take a int64 so the function would have > to be wrapped internally to deal with platform differences. But your right > in that polling is not the ideal answer. Using Hendriks solution would be > the preferred option on Windows. > > >> Directly defining pthread_cancel to this seems risky, as someone might >> introduce such a call at another place at some point without >> realizing. >> I would probably feel better to directly see that code where its meant to >> go. >> > > Fair enough > > In general I agree with Nicolas, split the ifdef renames and the win32 >> compat into two patches, that way its much clearer whats actually >> going on in the patch. >> > > the ifdef changes arent necessary and it doesnt bother me if they were > there or not. I just put them in incase someone in the future got confused > as to how pthread_cancel was enabled on win32. So to be honest im happy > leaving things without any ifdef changes unless someone else requests it. > > I hope the final version would get some cosmetic cleanup. >> > > What would you like to see? > > Also, in this version you are closing the socket twice, once here in >> pthread_cancel() and once in the code after the call to >> pthread_cancel(). On Unix, that would be a big no. On windows I do not >> know. I would prefer if the normal code path still calls close() only >> after joining the thread, since concurrent uses of the same socket are >> not well specified. >> > > I was trying to find a suitable way to only call closesocket inplace of > pthread_cancel on windows as for all other systems the closesocket still > should be after the pthread_join, and to do that with minimal changes. This > seems as clean and minimal as i can get it at the moment: > > --- > libavformat/udp.c | 31 +++ > 1 file changed, 21 insertions(+), 6 deletions(-) > > diff --git a/libavformat/udp.c b/libavformat/udp.c > index 3835f98..0e4766f 100644 > --- a/libavformat/udp.c > +++ b/libavformat/udp.c > @@ -60,14 +60,22 @@ > #define IPPROTO_UDPLITE 136 > #endif > > -#if HAVE_PTHREAD_CANCEL > -#include > -#endif > - > #ifndef HAVE_PTHREAD_CANCEL > #define HAVE_PTHREAD_CANCEL 0 > #endif > > +#if !HAVE_PTHREAD_CANCEL && (HAVE_THREADS && HAVE_WINSOCK2_H) > +/* Winsock2 recv function can be unblocked by shutting down and closing > the socket */ > +#define pthread_setcancelstate(x, y) > +#define pthread_cancel > +#undef HAVE_PTHREAD_CANCEL > +#define HAVE_PTHREAD_CANCEL 1 > +#endif > + > +#if HAVE_PTHREAD_CANCEL > +#include "libavutil/thread.h" > +#endif > + > #ifndef IPV6_ADD_MEMBERSHIP > #define IPV6_ADD_MEMBERSHIP IPV6_JOIN_GROUP > #define IPV6_DROP_MEMBERSHIP IPV6_LEAVE_GROUP > @@ -1144,8 +1152,14 @@ 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) { > +# if !HAVE_PTHREAD_CANCEL && (HAVE_THREADS && HAVE_WINSOCK2_H) > +closesocket(s->udp_fd); > +s->udp_fd = INVALID_SOCKET; > +# 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)); > @@ -1153,7 +1167,8 @@ static int udp_close(URLContext *h) > pthread_cond_destroy(&s->cond); > } > #endif > -closesocket(s->udp_fd); > +if(s->udp_fd != INVALID_SOCKET) > +closesocket(s->udp_fd); > av_fifo_freep(&s->fifo); > return 0; > } > -- > Ive reupped the current versions of each seperated patch for comment ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/2] avformat/udp: Replace use of pthread_cancel.
> > Indeed, in theory that would work. I always forget about these options. > In my experience they do not work reliably, and I would argue against > their use in portable code. For example, starting there: > http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_socket.h.html > can you tell me the type of the arguments to SO_RCVTIMEO? I know Linux > man page tells us it is struct timeval, but that is not a portable > standard. > > Also, these options are just a band aid to implement polling. the same function on windows would take a int64 so the function would have to be wrapped internally to deal with platform differences. But your right in that polling is not the ideal answer. Using Hendriks solution would be the preferred option on Windows. > Directly defining pthread_cancel to this seems risky, as someone might > introduce such a call at another place at some point without > realizing. > I would probably feel better to directly see that code where its meant to > go. > Fair enough In general I agree with Nicolas, split the ifdef renames and the win32 > compat into two patches, that way its much clearer whats actually > going on in the patch. > the ifdef changes arent necessary and it doesnt bother me if they were there or not. I just put them in incase someone in the future got confused as to how pthread_cancel was enabled on win32. So to be honest im happy leaving things without any ifdef changes unless someone else requests it. I hope the final version would get some cosmetic cleanup. > What would you like to see? Also, in this version you are closing the socket twice, once here in > pthread_cancel() and once in the code after the call to > pthread_cancel(). On Unix, that would be a big no. On windows I do not > know. I would prefer if the normal code path still calls close() only > after joining the thread, since concurrent uses of the same socket are > not well specified. > I was trying to find a suitable way to only call closesocket inplace of pthread_cancel on windows as for all other systems the closesocket still should be after the pthread_join, and to do that with minimal changes. This seems as clean and minimal as i can get it at the moment: --- libavformat/udp.c | 31 +++ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/libavformat/udp.c b/libavformat/udp.c index 3835f98..0e4766f 100644 --- a/libavformat/udp.c +++ b/libavformat/udp.c @@ -60,14 +60,22 @@ #define IPPROTO_UDPLITE 136 #endif -#if HAVE_PTHREAD_CANCEL -#include -#endif - #ifndef HAVE_PTHREAD_CANCEL #define HAVE_PTHREAD_CANCEL 0 #endif +#if !HAVE_PTHREAD_CANCEL && (HAVE_THREADS && HAVE_WINSOCK2_H) +/* Winsock2 recv function can be unblocked by shutting down and closing the socket */ +#define pthread_setcancelstate(x, y) +#define pthread_cancel +#undef HAVE_PTHREAD_CANCEL +#define HAVE_PTHREAD_CANCEL 1 +#endif + +#if HAVE_PTHREAD_CANCEL +#include "libavutil/thread.h" +#endif + #ifndef IPV6_ADD_MEMBERSHIP #define IPV6_ADD_MEMBERSHIP IPV6_JOIN_GROUP #define IPV6_DROP_MEMBERSHIP IPV6_LEAVE_GROUP @@ -1144,8 +1152,14 @@ 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) { +# if !HAVE_PTHREAD_CANCEL && (HAVE_THREADS && HAVE_WINSOCK2_H) +closesocket(s->udp_fd); +s->udp_fd = INVALID_SOCKET; +# 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)); @@ -1153,7 +1167,8 @@ static int udp_close(URLContext *h) pthread_cond_destroy(&s->cond); } #endif -closesocket(s->udp_fd); +if(s->udp_fd != INVALID_SOCKET) +closesocket(s->udp_fd); av_fifo_freep(&s->fifo); return 0; } -- ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/2] avformat/udp: Replace use of pthread_cancel.
On Sat, Dec 3, 2016 at 2:13 PM, Matt Oliver wrote: > On 3 December 2016 at 23:40, Hendrik Leppkes wrote: > >> On Sat, Dec 3, 2016 at 1:33 PM, Matt Oliver wrote: >> > >> > I havent fully tested Hendriks idea as the msdn docs dont recommend >> calling >> > multiple winsock functions at the same time from different threads so i >> > wasnt sure about how well received a patch that relies on closesocket to >> > unblock the recv function would be received (although i have seen it >> > extensively used without issuers). If Hendrik has tested this though with >> > his local patch without issues then I would accept that as a solution to >> > fixing my issue. ping Hendrik! >> >> I don't really use UDP streaming on a regular basis, but I did test >> this approach when I build it, and it works just fine. >> What I did was basically just define pthread_cancel/setcancelstate to >> empty in udp.c (and define HAVE_PTHREAD_CANCEL in that file to enable >> the code) and re-shuffle the udp_close code to close the socket to >> unblock the thread before waiting for it to exit. >> >> I don't have a clean and finished patch to go, because I had no real >> interest in it working on other platforms, so its rather ugly. But the >> approach is rather simple. > > > Would something like the following work for you: > > --- > libavformat/udp.c | 46 +- > 1 file changed, 29 insertions(+), 17 deletions(-) > > diff --git a/libavformat/udp.c b/libavformat/udp.c > index 3835f98..a30918b 100644 > --- a/libavformat/udp.c > +++ b/libavformat/udp.c > @@ -60,14 +60,26 @@ > #define IPPROTO_UDPLITE 136 > #endif > > -#if HAVE_PTHREAD_CANCEL > -#include > -#endif > - > #ifndef HAVE_PTHREAD_CANCEL > #define HAVE_PTHREAD_CANCEL 0 > #endif > > +#if HAVE_PTHREAD_CANCEL || (HAVE_THREADS && HAVE_WINSOCK2_H) > +#define THREAD_RXRW 1 > +#else > +#define THREAD_RXRW 0 > +#endif > + > +#if !HAVE_PTHREAD_CANCEL && (HAVE_THREADS && HAVE_WINSOCK2_H) > +/* Winsock2 recv function can be unblocked by shutting down and closing > the socket */ > +#define pthread_setcancelstate(x, y) > +#define pthread_cancel {shutdown(s->udp_fd, SD_BOTH); > closesocket(s->udp_fd);} > +#endif > + Directly defining pthread_cancel to this seems risky, as someone might introduce such a call at another place at some point without realizing. I would probably feel better to directly see that code where its meant to go. AFAIK you should only call closesocket without shutdown, though. > +#if THREAD_RXRW > +#include "libavutil/thread.h" > +#endif > + > #ifndef IPV6_ADD_MEMBERSHIP > #define IPV6_ADD_MEMBERSHIP IPV6_JOIN_GROUP > #define IPV6_DROP_MEMBERSHIP IPV6_LEAVE_GROUP > @@ -100,7 +112,7 @@ typedef struct UDPContext { > int64_t bitrate; /* number of bits to send per second */ > int64_t burst_bits; > int close_req; > -#if HAVE_PTHREAD_CANCEL > +#if THREAD_RXRW > pthread_t circular_buffer_thread; > pthread_mutex_t mutex; > pthread_cond_t cond; > @@ -495,7 +507,7 @@ static int udp_get_file_handle(URLContext *h) > return s->udp_fd; > } > > -#if HAVE_PTHREAD_CANCEL > +#if THREAD_RXRW > static void *circular_buffer_task_rx( void *_URLContext) > { > URLContext *h = _URLContext; > @@ -730,7 +742,7 @@ static int udp_open(URLContext *h, const char *uri, int > flags) > /* assume if no digits were found it is a request to enable it > */ > if (buf == endptr) > s->overrun_nonfatal = 1; > -if (!HAVE_PTHREAD_CANCEL) > +if (!THREAD_RXRW) > av_log(h, AV_LOG_WARNING, > "'overrun_nonfatal' option was set but it is not > supported " > "on this build (pthread support is required)\n"); > @@ -758,14 +770,14 @@ static int udp_open(URLContext *h, const char *uri, > int flags) > } > if (av_find_info_tag(buf, sizeof(buf), "fifo_size", p)) { > s->circular_buffer_size = strtol(buf, NULL, 10); > -if (!HAVE_PTHREAD_CANCEL) > +if (!THREAD_RXRW) > av_log(h, AV_LOG_WARNING, > "'circular_buffer_size' option was set but it is > not supported " > "on this build (pthread support is required)\n"); > } > if (av_find_info_tag(buf, sizeof(buf), "bitrate", p)) { > s->bitrate = strtoll(buf, NULL, 10); > -if (!HAVE_PTHREAD_CANCEL) > +if (!THREAD_RXRW) > av_log(h, AV_LOG_WARNING, > "'bitrate' option was set but it is not supported " > "on this build (pthread support is required)\n"); > @@ -951,7 +963,7 @@ static int udp_open(URLContext *h, const char *uri, int > flags) > > s->udp_fd = udp_fd; > > -#if HAVE_PTHREAD_CANCEL > +#if THREAD_RXRW > /* >Create thread in case of: >1. Input and circular_buffer_size is set >
Re: [FFmpeg-devel] [PATCH 2/2] avformat/udp: Replace use of pthread_cancel.
Replying for the completeness of the discussion. Le tridi 13 frimaire, an CCXXV, Matt Oliver a écrit : > Thats the trick, not to large that it results noticeable shutdown delays to > the user but not to quick that it polls to often, on a modern machine id > think something like 0.01s would be ok. I would agree. Unfortunately, even not speaking of the deep-sleep states I evoked earlier, FFmpeg also supports much smaller hardware than what is required to run anything related to MSVC. On some supported hardware, a 100 Hz poll would be too much. > I was thinking of just using setsockopt to set a value for SO_RCVTIMEO only > in circular_buffer_task_rx. That way recv will timeout at the specified > interval which will then use the existing code that checks the recv return. > In case of timeout that check will continue back to top of loop that will > have the s->close_req check to terminate the thread if needed. > something like: Indeed, in theory that would work. I always forget about these options. In my experience they do not work reliably, and I would argue against their use in portable code. For example, starting there: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_socket.h.html can you tell me the type of the arguments to SO_RCVTIMEO? I know Linux man page tells us it is struct timeval, but that is not a portable standard. Also, these options are just a band aid to implement polling. > Is anyone working on that as your right i definitely think an event system > for all protocols is the best option. Its just that has a larger scope than > i can handle so i was trying to get a simple shorter term fix for udp. Working on it really, no, and not in the foreseeable future. But I have some detailed ideas about how to proceed for when I will get to it. I can share them if someone wants to do it. Regards, -- Nicolas George signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/2] avformat/udp: Replace use of pthread_cancel.
Le quartidi 14 frimaire, an CCXXV, Matt Oliver a écrit : > Would something like the following work for you: I would not oppose on principle, just a few details. > > --- > libavformat/udp.c | 46 +- > 1 file changed, 29 insertions(+), 17 deletions(-) > > diff --git a/libavformat/udp.c b/libavformat/udp.c > index 3835f98..a30918b 100644 > --- a/libavformat/udp.c > +++ b/libavformat/udp.c > @@ -60,14 +60,26 @@ > #define IPPROTO_UDPLITE 136 > #endif > > -#if HAVE_PTHREAD_CANCEL > -#include > -#endif > - > #ifndef HAVE_PTHREAD_CANCEL > #define HAVE_PTHREAD_CANCEL 0 > #endif > > +#if HAVE_PTHREAD_CANCEL || (HAVE_THREADS && HAVE_WINSOCK2_H) > +#define THREAD_RXRW 1 > +#else > +#define THREAD_RXRW 0 > +#endif > + > +#if !HAVE_PTHREAD_CANCEL && (HAVE_THREADS && HAVE_WINSOCK2_H) > +/* Winsock2 recv function can be unblocked by shutting down and closing > the socket */ > +#define pthread_setcancelstate(x, y) > +#define pthread_cancel {shutdown(s->udp_fd, SD_BOTH); > closesocket(s->udp_fd);} > +#endif I hope the final version would get some cosmetic cleanup. Also, in this version you are closing the socket twice, once here in pthread_cancel() and once in the code after the call to pthread_cancel(). On Unix, that would be a big no. On windows I do not know. I would prefer if the normal code path still calls close() only after joining the thread, since concurrent uses of the same socket are not well specified. If just shutdown() is enough, then I suggest using it only. Otherwise, if it is not necessary then it would probably better to remove it. > + > +#if THREAD_RXRW > +#include "libavutil/thread.h" > +#endif > + > #ifndef IPV6_ADD_MEMBERSHIP > #define IPV6_ADD_MEMBERSHIP IPV6_JOIN_GROUP > #define IPV6_DROP_MEMBERSHIP IPV6_LEAVE_GROUP > @@ -100,7 +112,7 @@ typedef struct UDPContext { > int64_t bitrate; /* number of bits to send per second */ > int64_t burst_bits; > int close_req; > -#if HAVE_PTHREAD_CANCEL > +#if THREAD_RXRW > pthread_t circular_buffer_thread; > pthread_mutex_t mutex; > pthread_cond_t cond; > @@ -495,7 +507,7 @@ static int udp_get_file_handle(URLContext *h) > return s->udp_fd; > } > > -#if HAVE_PTHREAD_CANCEL > +#if THREAD_RXRW > static void *circular_buffer_task_rx( void *_URLContext) > { > URLContext *h = _URLContext; > @@ -730,7 +742,7 @@ static int udp_open(URLContext *h, const char *uri, int > flags) > /* assume if no digits were found it is a request to enable it > */ > if (buf == endptr) > s->overrun_nonfatal = 1; > -if (!HAVE_PTHREAD_CANCEL) > +if (!THREAD_RXRW) > av_log(h, AV_LOG_WARNING, > "'overrun_nonfatal' option was set but it is not > supported " > "on this build (pthread support is required)\n"); > @@ -758,14 +770,14 @@ static int udp_open(URLContext *h, const char *uri, > int flags) > } > if (av_find_info_tag(buf, sizeof(buf), "fifo_size", p)) { > s->circular_buffer_size = strtol(buf, NULL, 10); > -if (!HAVE_PTHREAD_CANCEL) > +if (!THREAD_RXRW) > av_log(h, AV_LOG_WARNING, > "'circular_buffer_size' option was set but it is > not supported " > "on this build (pthread support is required)\n"); > } > if (av_find_info_tag(buf, sizeof(buf), "bitrate", p)) { > s->bitrate = strtoll(buf, NULL, 10); > -if (!HAVE_PTHREAD_CANCEL) > +if (!THREAD_RXRW) > av_log(h, AV_LOG_WARNING, > "'bitrate' option was set but it is not supported " > "on this build (pthread support is required)\n"); > @@ -951,7 +963,7 @@ static int udp_open(URLContext *h, const char *uri, int > flags) > > s->udp_fd = udp_fd; > > -#if HAVE_PTHREAD_CANCEL > +#if THREAD_RXRW > /* >Create thread in case of: >1. Input and circular_buffer_size is set > @@ -988,7 +1000,7 @@ static int udp_open(URLContext *h, const char *uri, > int flags) > #endif > > return 0; > -#if HAVE_PTHREAD_CANCEL > +#if THREAD_RXRW > thread_fail: > pthread_cond_destroy(&s->cond); > cond_fail: > @@ -1019,7 +1031,7 @@ static int udp_read(URLContext *h, uint8_t *buf, int > size) > { > UDPContext *s = h->priv_data; > int ret; > -#if HAVE_PTHREAD_CANCEL > +#if THREAD_RXRW > int avail, nonblock = h->flags & AVIO_FLAG_NONBLOCK; > > if (s->fifo) { > @@ -1054,9 +1066,9 @@ static int udp_read(URLContext *h, uint8_t *buf, int > size) > int64_t t = av_gettime() + 10; > struct timespec tv = { .tv_sec = t / 100, > .tv_nsec = (t % 100) * 1000 }; > -if (pthread_cond_timedwait(&s->cond, &s->mutex, &tv) < 0) { >
Re: [FFmpeg-devel] [PATCH 2/2] avformat/udp: Replace use of pthread_cancel.
On 3 December 2016 at 23:40, Hendrik Leppkes wrote: > On Sat, Dec 3, 2016 at 1:33 PM, Matt Oliver wrote: > > > > I havent fully tested Hendriks idea as the msdn docs dont recommend > calling > > multiple winsock functions at the same time from different threads so i > > wasnt sure about how well received a patch that relies on closesocket to > > unblock the recv function would be received (although i have seen it > > extensively used without issuers). If Hendrik has tested this though with > > his local patch without issues then I would accept that as a solution to > > fixing my issue. ping Hendrik! > > I don't really use UDP streaming on a regular basis, but I did test > this approach when I build it, and it works just fine. > What I did was basically just define pthread_cancel/setcancelstate to > empty in udp.c (and define HAVE_PTHREAD_CANCEL in that file to enable > the code) and re-shuffle the udp_close code to close the socket to > unblock the thread before waiting for it to exit. > > I don't have a clean and finished patch to go, because I had no real > interest in it working on other platforms, so its rather ugly. But the > approach is rather simple. Would something like the following work for you: --- libavformat/udp.c | 46 +- 1 file changed, 29 insertions(+), 17 deletions(-) diff --git a/libavformat/udp.c b/libavformat/udp.c index 3835f98..a30918b 100644 --- a/libavformat/udp.c +++ b/libavformat/udp.c @@ -60,14 +60,26 @@ #define IPPROTO_UDPLITE 136 #endif -#if HAVE_PTHREAD_CANCEL -#include -#endif - #ifndef HAVE_PTHREAD_CANCEL #define HAVE_PTHREAD_CANCEL 0 #endif +#if HAVE_PTHREAD_CANCEL || (HAVE_THREADS && HAVE_WINSOCK2_H) +#define THREAD_RXRW 1 +#else +#define THREAD_RXRW 0 +#endif + +#if !HAVE_PTHREAD_CANCEL && (HAVE_THREADS && HAVE_WINSOCK2_H) +/* Winsock2 recv function can be unblocked by shutting down and closing the socket */ +#define pthread_setcancelstate(x, y) +#define pthread_cancel {shutdown(s->udp_fd, SD_BOTH); closesocket(s->udp_fd);} +#endif + +#if THREAD_RXRW +#include "libavutil/thread.h" +#endif + #ifndef IPV6_ADD_MEMBERSHIP #define IPV6_ADD_MEMBERSHIP IPV6_JOIN_GROUP #define IPV6_DROP_MEMBERSHIP IPV6_LEAVE_GROUP @@ -100,7 +112,7 @@ typedef struct UDPContext { int64_t bitrate; /* number of bits to send per second */ int64_t burst_bits; int close_req; -#if HAVE_PTHREAD_CANCEL +#if THREAD_RXRW pthread_t circular_buffer_thread; pthread_mutex_t mutex; pthread_cond_t cond; @@ -495,7 +507,7 @@ static int udp_get_file_handle(URLContext *h) return s->udp_fd; } -#if HAVE_PTHREAD_CANCEL +#if THREAD_RXRW static void *circular_buffer_task_rx( void *_URLContext) { URLContext *h = _URLContext; @@ -730,7 +742,7 @@ static int udp_open(URLContext *h, const char *uri, int flags) /* assume if no digits were found it is a request to enable it */ if (buf == endptr) s->overrun_nonfatal = 1; -if (!HAVE_PTHREAD_CANCEL) +if (!THREAD_RXRW) av_log(h, AV_LOG_WARNING, "'overrun_nonfatal' option was set but it is not supported " "on this build (pthread support is required)\n"); @@ -758,14 +770,14 @@ static int udp_open(URLContext *h, const char *uri, int flags) } if (av_find_info_tag(buf, sizeof(buf), "fifo_size", p)) { s->circular_buffer_size = strtol(buf, NULL, 10); -if (!HAVE_PTHREAD_CANCEL) +if (!THREAD_RXRW) av_log(h, AV_LOG_WARNING, "'circular_buffer_size' option was set but it is not supported " "on this build (pthread support is required)\n"); } if (av_find_info_tag(buf, sizeof(buf), "bitrate", p)) { s->bitrate = strtoll(buf, NULL, 10); -if (!HAVE_PTHREAD_CANCEL) +if (!THREAD_RXRW) av_log(h, AV_LOG_WARNING, "'bitrate' option was set but it is not supported " "on this build (pthread support is required)\n"); @@ -951,7 +963,7 @@ static int udp_open(URLContext *h, const char *uri, int flags) s->udp_fd = udp_fd; -#if HAVE_PTHREAD_CANCEL +#if THREAD_RXRW /* Create thread in case of: 1. Input and circular_buffer_size is set @@ -988,7 +1000,7 @@ static int udp_open(URLContext *h, const char *uri, int flags) #endif return 0; -#if HAVE_PTHREAD_CANCEL +#if THREAD_RXRW thread_fail: pthread_cond_destroy(&s->cond); cond_fail: @@ -1019,7 +1031,7 @@ static int udp_read(URLContext *h, uint8_t *buf, int size) { UDPContext *s = h->priv_data; int ret; -#if HAVE_PTHREAD_CANCEL +#if THREAD_RXRW int avail, nonblock = h->flags & AVIO_FLAG_NONBLOCK; if (s->fifo) { @@ -1054,9 +1066,9 @@ static int udp_read(URLContext *h, uint8_t *buf, int size)
Re: [FFmpeg-devel] [PATCH 2/2] avformat/udp: Replace use of pthread_cancel.
On Sat, Dec 3, 2016 at 1:33 PM, Matt Oliver wrote: > > I havent fully tested Hendriks idea as the msdn docs dont recommend calling > multiple winsock functions at the same time from different threads so i > wasnt sure about how well received a patch that relies on closesocket to > unblock the recv function would be received (although i have seen it > extensively used without issuers). If Hendrik has tested this though with > his local patch without issues then I would accept that as a solution to > fixing my issue. ping Hendrik! I don't really use UDP streaming on a regular basis, but I did test this approach when I build it, and it works just fine. What I did was basically just define pthread_cancel/setcancelstate to empty in udp.c (and define HAVE_PTHREAD_CANCEL in that file to enable the code) and re-shuffle the udp_close code to close the socket to unblock the thread before waiting for it to exit. I don't have a clean and finished patch to go, because I had no real interest in it working on other platforms, so its rather ugly. But the approach is rather simple. - Hendrik ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/2] avformat/udp: Replace use of pthread_cancel.
On 3 December 2016 at 22:33, Nicolas George wrote: > Le tridi 13 frimaire, an CCXXV, Matt Oliver a écrit : > > I was just writing an email to rephrase but you beet me to it (sorry i > > wasnt faster) > > No problem. > > > I should rephrase this to "Why is polling with an acceptable > timeout/sleep > > not acceptable". Obviously a event based callback would be more efficient > > but wouldnt setting an appropriate recv timeout then checking for thread > > exit before looping the recv call be an acceptable compromise. The thread > > exit check is minimal and if not set it would just go back into the > > blocking recv until data is received or the timeout triggers again. Long > > term callbacks would be better but would this be acceptable short term. > > What would you consider an appropriate timeout, more precisely? > Thats the trick, not to large that it results noticeable shutdown delays to the user but not to quick that it polls to often, on a modern machine id think something like 0.01s would be ok. > First, I would like to remind you that you cannot block with a timeout, > you would have to rework the code to use the ill-named poll(), which > would bring it back to its state before the use of pthread_cancel(). > I was thinking of just using setsockopt to set a value for SO_RCVTIMEO only in circular_buffer_task_rx. That way recv will timeout at the specified interval which will then use the existing code that checks the recv return. In case of timeout that check will continue back to top of loop that will have the s->close_req check to terminate the thread if needed. something like: + setsockopt(s->udp_fd, SOL_SOCKET, SO_RCVTIMEO, &timeout, sizeof(timeout)); while(1) { int len; + if (s->close_req) + goto end; pthread_mutex_unlock(&s->mutex); len = recv(s->udp_fd, s->tmp+4, sizeof(s->tmp)-4, 0); pthread_mutex_lock(&s->mutex); if (len < 0) { if (ff_neterrno() != AVERROR(EAGAIN) && ff_neterrno() != AVERROR(EINTR)) { s->circular_buffer_error = ff_neterrno(); goto end; } continue; } > > > You are right that it would take care of the latency problem of the > network side, and allow a long timeout. But the latency is a problem too > for the cancellation. We had a 3 seconds timeout, users did get > impatient and interrupted it forcefully. > > Furthermore, the core of your argument is that "the thread exit check is > minimal", but as I explained, it is not on embedded devices: anything > except "block until the kernel wakes up" is too much because it prevents > deep sleep states and drains the battery. > So perhaps keep the existing pthread_cancel functionality and add in the changes only when pthread_cancel is not available. The user can always choose not to use the thread stuff if they dont like it by not setting a fifo_size option etc. Modern operating systems allow multi-peer applications to work in a > completely poll-free completely notification-based way. FFmpeg, based on > very old code without global design, is quite far from achieving that. > But moving away from that is definitely moving in the wrong direction. > Is anyone working on that as your right i definitely think an event system for all protocols is the best option. Its just that has a larger scope than i can handle so i was trying to get a simple shorter term fix for udp. I assume you are not working just on a general distaste about > pthread_cancel(): you have in mind a specific platform in mind where it > is not available and you need the UDP thread, have you not? > Yep, although im not a huge fan of pthread_cancel, im mainly just trying to improve functionality with msvc. Then I suggest working for that platform: find a way of implementing a > poll-free delay-free interruption mechanism. Maybe using non-portable > properties of the platform (like closing on another thread like Hendrik > evoked) or an entirely different API. > I havent fully tested Hendriks idea as the msdn docs dont recommend calling multiple winsock functions at the same time from different threads so i wasnt sure about how well received a patch that relies on closesocket to unblock the recv function would be received (although i have seen it extensively used without issuers). If Hendrik has tested this though with his local patch without issues then I would accept that as a solution to fixing my issue. ping Hendrik! ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/2] avformat/udp: Replace use of pthread_cancel.
Le tridi 13 frimaire, an CCXXV, Matt Oliver a écrit : > I was just writing an email to rephrase but you beet me to it (sorry i > wasnt faster) No problem. > I should rephrase this to "Why is polling with an acceptable timeout/sleep > not acceptable". Obviously a event based callback would be more efficient > but wouldnt setting an appropriate recv timeout then checking for thread > exit before looping the recv call be an acceptable compromise. The thread > exit check is minimal and if not set it would just go back into the > blocking recv until data is received or the timeout triggers again. Long > term callbacks would be better but would this be acceptable short term. What would you consider an appropriate timeout, more precisely? First, I would like to remind you that you cannot block with a timeout, you would have to rework the code to use the ill-named poll(), which would bring it back to its state before the use of pthread_cancel(). You are right that it would take care of the latency problem of the network side, and allow a long timeout. But the latency is a problem too for the cancellation. We had a 3 seconds timeout, users did get impatient and interrupted it forcefully. Furthermore, the core of your argument is that "the thread exit check is minimal", but as I explained, it is not on embedded devices: anything except "block until the kernel wakes up" is too much because it prevents deep sleep states and drains the battery. Modern operating systems allow multi-peer applications to work in a completely poll-free completely notification-based way. FFmpeg, based on very old code without global design, is quite far from achieving that. But moving away from that is definitely moving in the wrong direction. I assume you are not working just on a general distaste about pthread_cancel(): you have in mind a specific platform in mind where it is not available and you need the UDP thread, have you not? Then I suggest working for that platform: find a way of implementing a poll-free delay-free interruption mechanism. Maybe using non-portable properties of the platform (like closing on another thread like Hendrik evoked) or an entirely different API. Regards, -- Nicolas George signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/2] avformat/udp: Replace use of pthread_cancel.
On 3 December 2016 at 22:05, Nicolas George wrote: > Le tridi 13 frimaire, an CCXXV, Matt Oliver a écrit : > > That was because the pthread wrappers dont set errno, so its a required > > change to remove dependency on pthread. > > The pthread wrappers are supposed to match the pthread API, and it does > not use errno: this change is needed by itself. > > > Your right i misread the use of ff_socket_nonblock so an extra change > would > > need to be added to set that to nonblocking instead. Theres already code > > after the recv call that checks if any data was read back and just loops > if > > there wasnt any available which implements polling (should it be in > > nonblocking mode). So thats why i assumed nonblocking was acceptable > > Looping is necessary in case of interrupted system calls; the code is > overly careful by testing EAGAIN too. > > > Just out of curiosity why is polling considered unacceptable? > > It is awfully inefficient. > > If the polling interval is large, it adds latency to the processing. If > it is short, it becomes a resource drain. Multimedia application often > require low latency. > > You may think that a 100 Hz polling will have low latency enough for > most uses and still be a negligible power drain, and thinking about PC > applications you would probably be right. But think of a player on an > embedded device reading a network stream that got interrupted. Since it > is no longer playing, you do not pay attention to it, and you do not > realize the player is still there, polling the network to see if it > comes back. As a consequence, the CPU cannot enter deep-sleep state, and > your battery in drained in a few hours. > > And of course, if someone were to just turn the socket non-blocking, > since the loop does not have a timer in it, it becomes the ultimate form > of polling evil: busy wait, and this is a terrible power drain even on > huge PCs. > > FFmpeg uses polling all over the place, but fortunately, with just the > UDP streams it can be entirely avoided. Eliminating all polling is > amongst my long-term projects. > I was just writing an email to rephrase but you beet me to it (sorry i wasnt faster) I should rephrase this to "Why is polling with an acceptable timeout/sleep not acceptable". Obviously a event based callback would be more efficient but wouldnt setting an appropriate recv timeout then checking for thread exit before looping the recv call be an acceptable compromise. The thread exit check is minimal and if not set it would just go back into the blocking recv until data is received or the timeout triggers again. Long term callbacks would be better but would this be acceptable short term. > > > As i understand it pthread_cancel implements polling internally anyway > as > > any blocking function performed in the rx thread has to periodically > check > > the status of pthread_cancel to see if it has been set from another > thread. > > I believe you are wrong. The core property of pthread_cancel(), which > you can not emulate portably with other functions, is that it interrupts > blocking system calls like recv(). It is implemented internally with > non-portable notification techniques: a specific signal on Linux if I > read correctly. Again i was about to correct myself but you beet me to it. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/2] avformat/udp: Replace use of pthread_cancel.
Le tridi 13 frimaire, an CCXXV, Matt Oliver a écrit : > That was because the pthread wrappers dont set errno, so its a required > change to remove dependency on pthread. The pthread wrappers are supposed to match the pthread API, and it does not use errno: this change is needed by itself. > Your right i misread the use of ff_socket_nonblock so an extra change would > need to be added to set that to nonblocking instead. Theres already code > after the recv call that checks if any data was read back and just loops if > there wasnt any available which implements polling (should it be in > nonblocking mode). So thats why i assumed nonblocking was acceptable Looping is necessary in case of interrupted system calls; the code is overly careful by testing EAGAIN too. > Just out of curiosity why is polling considered unacceptable? It is awfully inefficient. If the polling interval is large, it adds latency to the processing. If it is short, it becomes a resource drain. Multimedia application often require low latency. You may think that a 100 Hz polling will have low latency enough for most uses and still be a negligible power drain, and thinking about PC applications you would probably be right. But think of a player on an embedded device reading a network stream that got interrupted. Since it is no longer playing, you do not pay attention to it, and you do not realize the player is still there, polling the network to see if it comes back. As a consequence, the CPU cannot enter deep-sleep state, and your battery in drained in a few hours. And of course, if someone were to just turn the socket non-blocking, since the loop does not have a timer in it, it becomes the ultimate form of polling evil: busy wait, and this is a terrible power drain even on huge PCs. FFmpeg uses polling all over the place, but fortunately, with just the UDP streams it can be entirely avoided. Eliminating all polling is amongst my long-term projects. > As i understand it pthread_cancel implements polling internally anyway as > any blocking function performed in the rx thread has to periodically check > the status of pthread_cancel to see if it has been set from another thread. I believe you are wrong. The core property of pthread_cancel(), which you can not emulate portably with other functions, is that it interrupts blocking system calls like recv(). It is implemented internally with non-portable notification techniques: a specific signal on Linux if I read correctly. Regards, -- Nicolas George signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/2] avformat/udp: Replace use of pthread_cancel.
On 3 December 2016 at 05:17, Nicolas George wrote: > Le duodi 12 frimaire, an CCXXV, Matt Oliver a écrit : > > Which changes would those be? > > The fix of errno / return value for pthread_cond_timedwait() for > example. > That was because the pthread wrappers dont set errno, so its a required change to remove dependency on pthread. > > > This function is already set as nonblocking > > I think you read this specific part of the code wrong. Unfortunately, > since this is a pivotal part of your reasoning, it ruins everything > else. > Your right i misread the use of ff_socket_nonblock so an extra change would need to be added to set that to nonblocking instead. Theres already code after the recv call that checks if any data was read back and just loops if there wasnt any available which implements polling (should it be in nonblocking mode). So thats why i assumed nonblocking was acceptable > > recv() can not be in non-blocking mode here because it would cause the > whole thread to behave in busy-wait, which is completely unacceptable. > Just out of curiosity why is polling considered unacceptable? > > Remember that pthread_cancel() was introduced here to eliminate polling. > Please do not bring it back. The way forward is to implement an event > loop for all protocols. As i understand it pthread_cancel implements polling internally anyway as any blocking function performed in the rx thread has to periodically check the status of pthread_cancel to see if it has been set from another thread. So isnt this just removing the polling from ffmpeg and adding it back in somewhere else anyway. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/2] avformat/udp: Replace use of pthread_cancel.
Le duodi 12 frimaire, an CCXXV, Matt Oliver a écrit : > Which changes would those be? The fix of errno / return value for pthread_cond_timedwait() for example. > This function is already set as nonblocking I think you read this specific part of the code wrong. Unfortunately, since this is a pivotal part of your reasoning, it ruins everything else. recv() can not be in non-blocking mode here because it would cause the whole thread to behave in busy-wait, which is completely unacceptable. Remember that pthread_cancel() was introduced here to eliminate polling. Please do not bring it back. The way forward is to implement an event loop for all protocols. Regards, -- Nicolas George signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/2] avformat/udp: Replace use of pthread_cancel.
On 2 December 2016 at 22:16, Nicolas George wrote: > Le duodi 12 frimaire, an CCXXV, Matt Oliver a écrit : > > --- > > configure | 6 -- > > libavformat/udp.c | 48 +++- > > 2 files changed, 19 insertions(+), 35 deletions(-) > > It looks like there are unrelated changes in this patch. > Which changes would those be? all of them are just what is required to enable threading on non posix systems with pthread_cancel > Can you explain how it achieves the same result as pthread_cancel()? It > looks to me it does not. > For one currently pthread_cancel is only called for the read thread which uses the circular_buffer_task_rx function. pthread_cancel is never called on the transmit thread so the pthread_setcancelstate calls in circular_buffer_task_tx dont actually do anything. In fact the pthread cancel state is only enabled in the rx function on either side of the recv function. This function is already set as nonblocking and so it wont hold up the thread which can then continue back to the s->close_req check and terminate cleanly. So functionally I dont think the the pthread_cancel functionality is even needed as I dont think there is any advantage to having it and removing it allows the functionality to be enabled on more systems. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/2] avformat/udp: Replace use of pthread_cancel.
Le duodi 12 frimaire, an CCXXV, Matt Oliver a écrit : > --- > configure | 6 -- > libavformat/udp.c | 48 +++- > 2 files changed, 19 insertions(+), 35 deletions(-) It looks like there are unrelated changes in this patch. Can you explain how it achieves the same result as pthread_cancel()? It looks to me it does not. Regards, -- Nicolas George ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/2] avformat/udp: Replace use of pthread_cancel.
On Fri, Dec 2, 2016 at 9:22 AM, Matt Oliver wrote: > --- > configure | 6 -- > libavformat/udp.c | 48 +++- > 2 files changed, 19 insertions(+), 35 deletions(-) > > diff --git a/configure b/configure > index b5bfad6..cec94c4 100755 > --- a/configure > +++ b/configure > @@ -1934,7 +1934,6 @@ SYSTEM_FUNCS=" > nanosleep > PeekNamedPipe > posix_memalign > -pthread_cancel > sched_getaffinity > SetConsoleTextAttribute > SetConsoleCtrlHandler > @@ -5623,11 +5622,6 @@ if ! disabled pthreads && ! enabled w32threads && ! > enabled os2threads; then > check_code cc "pthread.h" "static pthread_mutex_t atomic_lock = > PTHREAD_MUTEX_INITIALIZER" || disable pthreads > fi > > - > -if enabled pthreads; then > - check_func pthread_cancel > -fi > - > enabled pthreads && > check_builtin sem_timedwait semaphore.h "sem_t *s; sem_init(s,0,0); > sem_timedwait(s,0); sem_destroy(s)" > > diff --git a/libavformat/udp.c b/libavformat/udp.c > index 3835f98..857a979 100644 > --- a/libavformat/udp.c > +++ b/libavformat/udp.c > @@ -60,12 +60,8 @@ > #define IPPROTO_UDPLITE 136 > #endif > > -#if HAVE_PTHREAD_CANCEL > -#include > -#endif > - > -#ifndef HAVE_PTHREAD_CANCEL > -#define HAVE_PTHREAD_CANCEL 0 > +#if HAVE_THREADS > +#include "libavutil/thread.h" > #endif > > #ifndef IPV6_ADD_MEMBERSHIP > @@ -100,7 +96,7 @@ typedef struct UDPContext { > int64_t bitrate; /* number of bits to send per second */ > int64_t burst_bits; > int close_req; > -#if HAVE_PTHREAD_CANCEL > +#if HAVE_THREADS > pthread_t circular_buffer_thread; > pthread_mutex_t mutex; > pthread_cond_t cond; > @@ -495,14 +491,13 @@ static int udp_get_file_handle(URLContext *h) > return s->udp_fd; > } > > -#if HAVE_PTHREAD_CANCEL > +#if HAVE_THREADS > static void *circular_buffer_task_rx( void *_URLContext) > { > URLContext *h = _URLContext; > UDPContext *s = h->priv_data; > int old_cancelstate; > > -pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &old_cancelstate); > pthread_mutex_lock(&s->mutex); > if (ff_socket_nonblock(s->udp_fd, 0) < 0) { > av_log(h, AV_LOG_ERROR, "Failed to set blocking mode"); > @@ -511,14 +506,11 @@ static void *circular_buffer_task_rx( void > *_URLContext) > } > while(1) { > int len; > +if (s->close_req) > +goto end; > > pthread_mutex_unlock(&s->mutex); > -/* Blocking operations are always cancellation points; > - see "General Information" / "Thread Cancelation Overview" > - in Single Unix. */ > -pthread_setcancelstate(PTHREAD_CANCEL_ENABLE, &old_cancelstate); > len = recv(s->udp_fd, s->tmp+4, sizeof(s->tmp)-4, 0); > -pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &old_cancelstate); pthread_cancel can unblock operations like these on Linux/Unix, so I don't think some manual logic is going to solve the same problem. On Windows this happens to work because closing the socket on another thread unblocks any calls to it. I have had a local patch for years that just defines pthread_cancel/setcancelstate to do nothing there. - Hendrik ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 2/2] avformat/udp: Replace use of pthread_cancel.
--- configure | 6 -- libavformat/udp.c | 48 +++- 2 files changed, 19 insertions(+), 35 deletions(-) diff --git a/configure b/configure index b5bfad6..cec94c4 100755 --- a/configure +++ b/configure @@ -1934,7 +1934,6 @@ SYSTEM_FUNCS=" nanosleep PeekNamedPipe posix_memalign -pthread_cancel sched_getaffinity SetConsoleTextAttribute SetConsoleCtrlHandler @@ -5623,11 +5622,6 @@ if ! disabled pthreads && ! enabled w32threads && ! enabled os2threads; then check_code cc "pthread.h" "static pthread_mutex_t atomic_lock = PTHREAD_MUTEX_INITIALIZER" || disable pthreads fi - -if enabled pthreads; then - check_func pthread_cancel -fi - enabled pthreads && check_builtin sem_timedwait semaphore.h "sem_t *s; sem_init(s,0,0); sem_timedwait(s,0); sem_destroy(s)" diff --git a/libavformat/udp.c b/libavformat/udp.c index 3835f98..857a979 100644 --- a/libavformat/udp.c +++ b/libavformat/udp.c @@ -60,12 +60,8 @@ #define IPPROTO_UDPLITE 136 #endif -#if HAVE_PTHREAD_CANCEL -#include -#endif - -#ifndef HAVE_PTHREAD_CANCEL -#define HAVE_PTHREAD_CANCEL 0 +#if HAVE_THREADS +#include "libavutil/thread.h" #endif #ifndef IPV6_ADD_MEMBERSHIP @@ -100,7 +96,7 @@ typedef struct UDPContext { int64_t bitrate; /* number of bits to send per second */ int64_t burst_bits; int close_req; -#if HAVE_PTHREAD_CANCEL +#if HAVE_THREADS pthread_t circular_buffer_thread; pthread_mutex_t mutex; pthread_cond_t cond; @@ -495,14 +491,13 @@ static int udp_get_file_handle(URLContext *h) return s->udp_fd; } -#if HAVE_PTHREAD_CANCEL +#if HAVE_THREADS static void *circular_buffer_task_rx( void *_URLContext) { URLContext *h = _URLContext; UDPContext *s = h->priv_data; int old_cancelstate; -pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &old_cancelstate); pthread_mutex_lock(&s->mutex); if (ff_socket_nonblock(s->udp_fd, 0) < 0) { av_log(h, AV_LOG_ERROR, "Failed to set blocking mode"); @@ -511,14 +506,11 @@ static void *circular_buffer_task_rx( void *_URLContext) } while(1) { int len; +if (s->close_req) +goto end; pthread_mutex_unlock(&s->mutex); -/* Blocking operations are always cancellation points; - see "General Information" / "Thread Cancelation Overview" - in Single Unix. */ -pthread_setcancelstate(PTHREAD_CANCEL_ENABLE, &old_cancelstate); len = recv(s->udp_fd, s->tmp+4, sizeof(s->tmp)-4, 0); -pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &old_cancelstate); pthread_mutex_lock(&s->mutex); if (len < 0) { if (ff_neterrno() != AVERROR(EAGAIN) && ff_neterrno() != AVERROR(EINTR)) { @@ -564,7 +556,6 @@ static void *circular_buffer_task_tx( void *_URLContext) int64_t burst_interval = s->bitrate ? (s->burst_bits * 100 / s->bitrate) : 0; int64_t max_delay = s->bitrate ? ((int64_t)h->max_packet_size * 8 * 100 / s->bitrate + 1) : 0; -pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &old_cancelstate); pthread_mutex_lock(&s->mutex); if (ff_socket_nonblock(s->udp_fd, 0) < 0) { @@ -599,7 +590,6 @@ static void *circular_buffer_task_tx( void *_URLContext) av_fifo_generic_read(s->fifo, s->tmp, len, NULL); pthread_mutex_unlock(&s->mutex); -pthread_setcancelstate(PTHREAD_CANCEL_ENABLE, &old_cancelstate); if (s->bitrate) { timestamp = av_gettime_relative(); @@ -645,7 +635,6 @@ static void *circular_buffer_task_tx( void *_URLContext) } } -pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &old_cancelstate); pthread_mutex_lock(&s->mutex); } @@ -730,7 +719,7 @@ static int udp_open(URLContext *h, const char *uri, int flags) /* assume if no digits were found it is a request to enable it */ if (buf == endptr) s->overrun_nonfatal = 1; -if (!HAVE_PTHREAD_CANCEL) +if (!HAVE_THREADS) av_log(h, AV_LOG_WARNING, "'overrun_nonfatal' option was set but it is not supported " "on this build (pthread support is required)\n"); @@ -758,14 +747,14 @@ static int udp_open(URLContext *h, const char *uri, int flags) } if (av_find_info_tag(buf, sizeof(buf), "fifo_size", p)) { s->circular_buffer_size = strtol(buf, NULL, 10); -if (!HAVE_PTHREAD_CANCEL) +if (!HAVE_THREADS) av_log(h, AV_LOG_WARNING, "'circular_buffer_size' option was set but it is not supported " "on this build (pthread support is required)\n"); } if (av_find_info_tag(buf, sizeof(buf), "bitrate", p)) { s->bitrate = strtoll(buf, NULL, 10); -if (!HAVE_PTHREAD_CANCEL) +if