From patchwork Sat Dec 3 14:56:06 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Matt Oliver X-Patchwork-Id: 1669 Delivered-To: ffmpegpatchwork@gmail.com Received: by 10.103.65.86 with SMTP id o83csp821637vsa; Sat, 3 Dec 2016 06:56:18 -0800 (PST) X-Received: by 10.194.188.9 with SMTP id fw9mr42817992wjc.213.1480776977967; Sat, 03 Dec 2016 06:56:17 -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 k24si6840738wmi.156.2016.12.03.06.56.17; Sat, 03 Dec 2016 06:56:17 -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 A3C4D689FBF; Sat, 3 Dec 2016 16:56:06 +0200 (EET) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-io0-f196.google.com (mail-io0-f196.google.com [209.85.223.196]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 4A76468981A for ; Sat, 3 Dec 2016 16:56:00 +0200 (EET) Received: by mail-io0-f196.google.com with SMTP id j92so13696392ioi.0 for ; Sat, 03 Dec 2016 06:56:09 -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=3KvVC6DGI5xZOQaiNeNMu+aPg7qUvm8p4c1C7M99MyA=; b=OLzjuuHFkJD/MiGUAccig8tmLsMDeuGjHQ4xFrQjGTwEuDQZupjOc3WjuGANlx7nXl CRM4JYLpnGGjEuqIZWwyfpWZAoQRM7MsMQCnMkAt2DNMLIlTT9qqh3Xn5X9M58ZqPCtQ mN11lHyCzZlw9omjnWNl36PKo3ykFT0Rmx939SpukxCgT0Y9MyGy1hDO/9SjhyY6+D+A gJzw0CkuFFRJ7eKAhQvH9MBmiJBLu3G2K6LUhAxAirHVFcB1p9lM0ZB61db658Mm5c4O Sw5zAVQ7Nhwbk+CamG91Vl0IE4+Garz2ZdEkxsNunJ1Wkg8wK+1epftLakQhSaD5IRMu jnwQ== 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=3KvVC6DGI5xZOQaiNeNMu+aPg7qUvm8p4c1C7M99MyA=; b=DC5cQ/qX5wNV4sGxBVUotsOAGKPXsp9+7CsbesgGMqx1iBODMuHTVr+9j1p+p60+ab 5mTRLLo0KBkl6nKzU5rsmHzsd4+Anac832h6qZ41rbtDtkgMUT0NjaP8YmxH1osah81K UwjGikam26aZTb3KIXO2PQTiLMGSIjABqSmdcio0L986cZM2odYCyJYAe2K4cBE+gVy3 c1HS2e5lzS+gdD4G3OqQpSbHukXQEO/haEPBCrno/lcJsUZf14+hruHg9w+/66cclkII wcNkNUDOgseI6H3nT02f6veybanbpVNJGFhkrf4L1kthhSrr3JzFUW+P15h5GfKSWOuo 57bw== X-Gm-Message-State: AKaTC03dalAgPmsiQVQ9rOpbbCsiYFD6bpT0/AlRZjGDS3l+9mUkbiKjCKR3IQgVcsGAZgwDDBHm3uU7+SH1cA== X-Received: by 10.36.7.85 with SMTP id f82mr1941586itf.10.1480776967500; Sat, 03 Dec 2016 06:56:07 -0800 (PST) MIME-Version: 1.0 Received: by 10.36.70.138 with HTTP; Sat, 3 Dec 2016 06:56:06 -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 01:56:06 +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" > > 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(-) #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; } -- 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