| Message ID | CAHVN4mgYCeGZ1X=_vWnGOB+ixSCC=4nqF830Xv07rSfmpSgLzA@mail.gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On Fri, Dec 2, 2016 at 9:22 AM, Matt Oliver <protogonoi@gmail.com> 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 <pthread.h> > -#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
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,
On 2 December 2016 at 22:16, Nicolas George <george@nsup.org> 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.
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,
On 3 December 2016 at 05:17, Nicolas George <george@nsup.org> 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.
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,
On 3 December 2016 at 22:05, Nicolas George <george@nsup.org> 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.
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,
On 3 December 2016 at 22:33, Nicolas George <george@nsup.org> 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!
On Sat, Dec 3, 2016 at 1:33 PM, Matt Oliver <protogonoi@gmail.com> 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
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,
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 <pthread.h> -#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) {
--- configure | 6 ------ libavformat/udp.c | 48 +++++++++++++++++++----------------------------- 2 files changed, 19 insertions(+), 35 deletions(-) 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 * 1000000 / s->bitrate) : 0; int64_t max_delay = s->bitrate ? ((int64_t)h->max_packet_size * 8 * 1000000 / 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 (!HAVE_THREADS) 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 +940,7 @@ static int udp_open(URLContext *h, const char *uri, int flags) s->udp_fd = udp_fd; -#if HAVE_PTHREAD_CANCEL +#if HAVE_THREADS /* Create thread in case of: 1. Input and circular_buffer_size is set @@ -988,7 +977,7 @@ static int udp_open(URLContext *h, const char *uri, int flags) #endif return 0; -#if HAVE_PTHREAD_CANCEL +#if HAVE_THREADS thread_fail: pthread_cond_destroy(&s->cond); cond_fail: @@ -1019,7 +1008,7 @@ static int udp_read(URLContext *h, uint8_t *buf, int size) { UDPContext *s = h->priv_data; int ret; -#if HAVE_PTHREAD_CANCEL +#if HAVE_THREADS int avail, nonblock = h->flags & AVIO_FLAG_NONBLOCK; if (s->fifo) { @@ -1051,12 +1040,13 @@ static int udp_read(URLContext *h, uint8_t *buf, int size) else { /* FIXME: using the monotonic clock would be better, but it does not exist on all supported platforms. */ + int ret; int64_t t = av_gettime() + 100000; struct timespec tv = { .tv_sec = t / 1000000, .tv_nsec = (t % 1000000) * 1000 }; - if (pthread_cond_timedwait(&s->cond, &s->mutex, &tv) < 0) { + if (ret = pthread_cond_timedwait(&s->cond, &s->mutex, &tv) < 0) { pthread_mutex_unlock(&s->mutex); - return AVERROR(errno == ETIMEDOUT ? EAGAIN : errno); + return AVERROR(ret == ETIMEDOUT ? EAGAIN : ret); } nonblock = 1; } @@ -1079,7 +1069,7 @@ static int udp_write(URLContext *h, const uint8_t *buf, int size) UDPContext *s = h->priv_data; int ret; -#if HAVE_PTHREAD_CANCEL +#if HAVE_THREADS if (s->fifo) { uint8_t tmp[4]; @@ -1128,7 +1118,7 @@ static int udp_close(URLContext *h) { UDPContext *s = h->priv_data; -#if HAVE_PTHREAD_CANCEL +#if HAVE_THREADS // Request close once writing is finished if (s->thread_started && !(h->flags & AVIO_FLAG_READ)) { pthread_mutex_lock(&s->mutex); @@ -1140,12 +1130,12 @@ static int udp_close(URLContext *h) if (s->is_multicast && (h->flags & AVIO_FLAG_READ)) udp_leave_multicast_group(s->udp_fd, (struct sockaddr *)&s->dest_addr,(struct sockaddr *)&s->local_addr_storage); -#if HAVE_PTHREAD_CANCEL +#if HAVE_THREADS 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) - pthread_cancel(s->circular_buffer_thread); + s->close_req = 1; ret = pthread_join(s->circular_buffer_thread, NULL); if (ret != 0) av_log(h, AV_LOG_ERROR, "pthread_join(): %s\n", strerror(ret));