On 4 December 2016 at 01:56, Matt Oliver <protogo...@gmail.com> 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 <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; > } > -- > 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