[FFmpeg-devel] [PATCH 2/2] avformat/udp: Replace use of pthread_cancel.

Matt Oliver protogonoi at gmail.com
Sat Dec 3 16:56:06 EET 2016


>
> 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 <pthread.h>
-#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;
 }
--


More information about the ffmpeg-devel mailing list