From patchwork Sat Dec 3 13:13:56 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Matt Oliver X-Patchwork-Id: 1668 Delivered-To: ffmpegpatchwork@gmail.com Received: by 10.103.65.86 with SMTP id o83csp784290vsa; Sat, 3 Dec 2016 05:14:07 -0800 (PST) X-Received: by 10.28.131.1 with SMTP id f1mr1972742wmd.43.1480770847225; Sat, 03 Dec 2016 05:14:07 -0800 (PST) Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org. [79.124.17.100]) by mx.google.com with ESMTP id c135si6639589wmh.118.2016.12.03.05.14.06; Sat, 03 Dec 2016 05:14:07 -0800 (PST) Received-SPF: pass (google.com: domain of ffmpeg-devel-bounces@ffmpeg.org designates 79.124.17.100 as permitted sender) client-ip=79.124.17.100; Authentication-Results: mx.google.com; dkim=neutral (body hash did not verify) header.i=@gmail.com; spf=pass (google.com: domain of ffmpeg-devel-bounces@ffmpeg.org designates 79.124.17.100 as permitted sender) smtp.mailfrom=ffmpeg-devel-bounces@ffmpeg.org; dmarc=fail (p=NONE dis=NONE) header.from=gmail.com Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id D440F689FEC; Sat, 3 Dec 2016 15:13:55 +0200 (EET) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-io0-f194.google.com (mail-io0-f194.google.com [209.85.223.194]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 63808689FB7 for ; Sat, 3 Dec 2016 15:13:49 +0200 (EET) Received: by mail-io0-f194.google.com with SMTP id s82so13237317ioi.3 for ; Sat, 03 Dec 2016 05:13:58 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:from:date:message-id:subject:to; bh=0EBLN8AA/HJKh0pEQvvQzg1DLUU3mUa4aKx27MfAeXI=; b=kbRwOqV/KGD+sUrqsYbFRCVcAk1REgM0qeaQVLMpqeZ+9Z2Hj7r/JnIrv1+d55A2+c f1dMSvIZlGSI21oqe5grArGsfqHJEYJTA8g12fMedLILqFq/euGLeXbhBhxJx3X2tCQ0 EiKBZZgsEXtodGI/77UfdOJ5qoVNyGCXomxbSaMrkR4yZwByOEonxvAoprymgkctCZBq F9uX+Qh5nn7BISWXOuJhGSBlDOkntC/8Jz76bZC7p/qBzIbrkpvPIBW5RDZVDja0xDES L77F+yt5mkGQbyJf+ZbaxPkcJIvfaAtSd9EIA9RtOi9ViiGdcT4bNYIiDTooCBzM79cN csPg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to; bh=0EBLN8AA/HJKh0pEQvvQzg1DLUU3mUa4aKx27MfAeXI=; b=N7eAsmlKiRjrhGjOPI5sPg1ecHq/HEd3/1iJZMtgoL441xRGISC2QWk4dvfOxtHQbB 2h+IlQ3nF7Oq6yHTbWcWzLV8CcOwpFRkksqoSRe9hNt8BcRZ9IjBfkKozrENYpgsW4Hr 9JrGo1CNRLrcQz4GhKoHystLlHyAP3upmpUdlmI6/vbZ/GiIZTt3/wufxhtcw8ubk5dp 5Ym13q9InPhTWfbZtzQVeAwRJh1ObprGgC6v4v1i0P1TiH1jAYJ8auKMlUrcnu5KBAfb BEc/tBHyg7gK2wmB7Uf5BibDg+SAV8BZXEySuMMeL7e9ppXH0t9P+vLfBbsawMJCIPcI gazQ== X-Gm-Message-State: AKaTC01WYoLH58U07AOdnYTd6M/vofAQwEv+WgKCh1/4vsYGP0MdRDU1eTPlO7D8f4qUVU5Xj+gFGTSEWk9HfA== X-Received: by 10.36.204.9 with SMTP id x9mr1546496itf.123.1480770836823; Sat, 03 Dec 2016 05:13:56 -0800 (PST) MIME-Version: 1.0 Received: by 10.36.70.138 with HTTP; Sat, 3 Dec 2016 05:13:56 -0800 (PST) In-Reply-To: References: <20161202111645.GA403566@phare.normalesup.org> <20161202181708.GA640178@phare.normalesup.org> <20161203110539.GA999526@phare.normalesup.org> <20161203113334.GA1013667@phare.normalesup.org> From: Matt Oliver Date: Sun, 4 Dec 2016 00:13:56 +1100 Message-ID: To: FFmpeg development discussions and patches X-Content-Filtered-By: Mailman/MimeDel 2.1.20 Subject: Re: [FFmpeg-devel] [PATCH 2/2] avformat/udp: Replace use of pthread_cancel. X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" 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(-) #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() + 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 +1091,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 THREAD_RXRW if (s->fifo) { uint8_t tmp[4]; @@ -1128,7 +1140,7 @@ static int udp_close(URLContext *h) { UDPContext *s = h->priv_data; -#if HAVE_PTHREAD_CANCEL +#if THREAD_RXRW // Request close once writing is finished if (s->thread_started && !(h->flags & AVIO_FLAG_READ)) { pthread_mutex_lock(&s->mutex); @@ -1140,7 +1152,7 @@ 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 THREAD_RXRW if (s->thread_started) { int ret; // Cancel only read, as write has been signaled as success to the user -- Ill split the patch so that pthread_cond_timedwait is separate and name it properly if everyone is happy with it first. 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