Re: [FFmpeg-devel] [PATCH 3/3] avformat/udp: Enable FIFO when using windows sockets.
On 15 December 2016 at 21:42, Nicolas Georgewrote: > Le duodi 22 frimaire, an CCXXV, Matt Oliver a écrit : > > --- > > libavformat/udp.c | 17 - > > 1 file changed, 16 insertions(+), 1 deletion(-) > > > > diff --git a/libavformat/udp.c b/libavformat/udp.c > > index f8c861d..3cafb32 100644 > > --- a/libavformat/udp.c > > +++ b/libavformat/udp.c > > @@ -64,6 +64,14 @@ > > #define HAVE_PTHREAD_CANCEL 0 > > #endif > > > > +#if HAVE_THREADS && HAVE_WINSOCK2_H > > +/* Winsock2 recv function can be unblocked by shutting down 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 > > @@ -526,6 +534,8 @@ static void *circular_buffer_task_rx( void > *_URLContext) > > goto end; > > } > > continue; > > > +} else if (len == 0) { > > +goto end; > > Unfortunately, UDP packets of size 0 exist and are returned to the > application. If len == 0 is the only criterion to detect a read > interrupted by shutdown(), it will not be usable for UDP. > > You can still combine with your original idea of an atomic flag, though. On further reading it turns out the zero length check is not required (as its not needed for connectionless sockets such as udp). So it turns out it works without the addition of that check as instead it just returns an error of WSAESHUTDOW. So ive updated the patch locally to remove then len==0 check. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 3/3] avformat/udp: Enable FIFO when using windows sockets.
Le duodi 22 frimaire, an CCXXV, Matt Oliver a écrit : > --- > libavformat/udp.c | 17 - > 1 file changed, 16 insertions(+), 1 deletion(-) > > diff --git a/libavformat/udp.c b/libavformat/udp.c > index f8c861d..3cafb32 100644 > --- a/libavformat/udp.c > +++ b/libavformat/udp.c > @@ -64,6 +64,14 @@ > #define HAVE_PTHREAD_CANCEL 0 > #endif > > +#if HAVE_THREADS && HAVE_WINSOCK2_H > +/* Winsock2 recv function can be unblocked by shutting down 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 > @@ -526,6 +534,8 @@ static void *circular_buffer_task_rx( void *_URLContext) > goto end; > } > continue; > +} else if (len == 0) { > +goto end; Unfortunately, UDP packets of size 0 exist and are returned to the application. If len == 0 is the only criterion to detect a read interrupted by shutdown(), it will not be usable for UDP. You can still combine with your original idea of an atomic flag, though. > } > AV_WL32(s->tmp, len); > > @@ -1144,8 +1154,13 @@ 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_THREADS && HAVE_WINSOCK2_H > +shutdown(s->udp_fd, SD_BOTH); > +# 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)); Regards, -- Nicolas George signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 3/3] avformat/udp: Enable FIFO when using windows sockets.
--- libavformat/udp.c | 17 - 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/libavformat/udp.c b/libavformat/udp.c index f8c861d..3cafb32 100644 --- a/libavformat/udp.c +++ b/libavformat/udp.c @@ -64,6 +64,14 @@ #define HAVE_PTHREAD_CANCEL 0 #endif +#if HAVE_THREADS && HAVE_WINSOCK2_H +/* Winsock2 recv function can be unblocked by shutting down 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 @@ -526,6 +534,8 @@ static void *circular_buffer_task_rx( void *_URLContext) goto end; } continue; +} else if (len == 0) { +goto end; } AV_WL32(s->tmp, len); @@ -1144,8 +1154,13 @@ 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_THREADS && HAVE_WINSOCK2_H +shutdown(s->udp_fd, SD_BOTH); +# 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)); -- 0003-avformat-udp-Enable-FIFO-when-using-windows-sockets.patch Description: Binary data ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 3/3] avformat/udp: Enable FIFO when using windows sockets.
On 8 December 2016 at 04:49, Nicolas Georgewrote: > Le septidi 17 frimaire, an CCXXV, Hendrik Leppkes a écrit : > > I'm not sure you can safely avoid that in this design when dealing > > with a fully generic library and no information or control whats going > > on in other threads. > > You are right, I had not realized there was a race condition here. > > What happens if shutdown() is performed but not close()? According to msdn using shutdown instead should work, so ill up a new patch later today. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 3/3] avformat/udp: Enable FIFO when using windows sockets.
Le septidi 17 frimaire, an CCXXV, Hendrik Leppkes a écrit : > I'm not sure you can safely avoid that in this design when dealing > with a fully generic library and no information or control whats going > on in other threads. You are right, I had not realized there was a race condition here. What happens if shutdown() is performed but not close()? > The "proper" win32 way of doing non-polling IO is using the Overlapped > IO functions, which runs async and signals event objects when > something happens - but that would practically require re-implementing > the entire worker thread for win32. The receiving worker thread is less than 60 lines, the sending one is 100 lines because of the rate control code that can certainly be factored. All in all, that would not be that terrible, and if done cleanly can be reused later for more generic code. signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 3/3] avformat/udp: Enable FIFO when using windows sockets.
On Wed, Dec 7, 2016 at 12:25 PM, Matt Oliverwrote: > On 7 December 2016 at 21:19, Mark Thompson wrote: > >> On 07/12/16 06:05, Matt Oliver wrote: >> > Signed-off-by: Matt Oliver >> > --- >> > libavformat/udp.c | 19 +-- >> > 1 file changed, 17 insertions(+), 2 deletions(-) >> > >> > diff --git a/libavformat/udp.c b/libavformat/udp.c >> > index f8c861d..0e4766f 100644 >> > --- a/libavformat/udp.c >> > +++ b/libavformat/udp.c >> > @@ -64,6 +64,14 @@ >> > #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 */ >> >> This seems dubious to me. Can you explain how this can work reliably on >> Windows? >> >> To offer some context, the reason that POSIX states that close() at the >> same time as any other operation is undefined is because it is impossible >> to avoid the following race: >> >> Thread 1: >> Load the file descriptor >> Enter the recv() call in the standard library >> Get preempted just before entering the system call >> Thread 2: >> Call close() on the file descriptor >> Finish closing, the file descriptor is now invalid and can be reused >> Thread 3 (could be thread 2 again, or a hidden part of the implementation): >> Make a new file (with open() or similar) >> Get given the same file descriptor, reused >> Thread 1: >> Start running again >> Actually make the recv() system call, which now refers to thread 3's file >> Data loss/crash/other badness >> >> Since there is no way to determine that a thread is actually inside the >> system call rather than preempted immediately before it there is no way in >> POSIX to avoid the race. An alternative implementation seeking to avoid >> this issue could perhaps supply such a function to determine the blocked >> state of another thread, or maybe file descriptors could be unique forever, >> but neither of these seems to apply in this case. >> > > Well to be honest this patch is actually based on a suggestion from Hendrik > in another thread so he maybe the better person to ask about that. I can > however state that it is a commonly used Windows winsock programming > technique as closesocket behaves differently on windows than it does on > linux (the above is a very bad idea on linux). Calling closesocket on > windows causes any existing recv calls in other threads to instantly return > with an error code. But im not sure how it would handle the specific case > set above. I'm not sure you can safely avoid that in this design when dealing with a fully generic library and no information or control whats going on in other threads. The "proper" win32 way of doing non-polling IO is using the Overlapped IO functions, which runs async and signals event objects when something happens - but that would practically require re-implementing the entire worker thread for win32. > An alternative would be to then combine it with win32s ability to terminate > threads. Terminating threads is never a good idea. - Hendrik ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 3/3] avformat/udp: Enable FIFO when using windows sockets.
On 7 December 2016 at 21:19, Mark Thompsonwrote: > On 07/12/16 06:05, Matt Oliver wrote: > > Signed-off-by: Matt Oliver > > --- > > libavformat/udp.c | 19 +-- > > 1 file changed, 17 insertions(+), 2 deletions(-) > > > > diff --git a/libavformat/udp.c b/libavformat/udp.c > > index f8c861d..0e4766f 100644 > > --- a/libavformat/udp.c > > +++ b/libavformat/udp.c > > @@ -64,6 +64,14 @@ > > #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 */ > > This seems dubious to me. Can you explain how this can work reliably on > Windows? > > To offer some context, the reason that POSIX states that close() at the > same time as any other operation is undefined is because it is impossible > to avoid the following race: > > Thread 1: > Load the file descriptor > Enter the recv() call in the standard library > Get preempted just before entering the system call > Thread 2: > Call close() on the file descriptor > Finish closing, the file descriptor is now invalid and can be reused > Thread 3 (could be thread 2 again, or a hidden part of the implementation): > Make a new file (with open() or similar) > Get given the same file descriptor, reused > Thread 1: > Start running again > Actually make the recv() system call, which now refers to thread 3's file > Data loss/crash/other badness > > Since there is no way to determine that a thread is actually inside the > system call rather than preempted immediately before it there is no way in > POSIX to avoid the race. An alternative implementation seeking to avoid > this issue could perhaps supply such a function to determine the blocked > state of another thread, or maybe file descriptors could be unique forever, > but neither of these seems to apply in this case. > Well to be honest this patch is actually based on a suggestion from Hendrik in another thread so he maybe the better person to ask about that. I can however state that it is a commonly used Windows winsock programming technique as closesocket behaves differently on windows than it does on linux (the above is a very bad idea on linux). Calling closesocket on windows causes any existing recv calls in other threads to instantly return with an error code. But im not sure how it would handle the specific case set above. An alternative would be to then combine it with win32s ability to terminate threads. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 3/3] avformat/udp: Enable FIFO when using windows sockets.
On 07/12/16 06:05, Matt Oliver wrote: > Signed-off-by: Matt Oliver> --- > libavformat/udp.c | 19 +-- > 1 file changed, 17 insertions(+), 2 deletions(-) > > diff --git a/libavformat/udp.c b/libavformat/udp.c > index f8c861d..0e4766f 100644 > --- a/libavformat/udp.c > +++ b/libavformat/udp.c > @@ -64,6 +64,14 @@ > #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 */ This seems dubious to me. Can you explain how this can work reliably on Windows? To offer some context, the reason that POSIX states that close() at the same time as any other operation is undefined is because it is impossible to avoid the following race: Thread 1: Load the file descriptor Enter the recv() call in the standard library Get preempted just before entering the system call Thread 2: Call close() on the file descriptor Finish closing, the file descriptor is now invalid and can be reused Thread 3 (could be thread 2 again, or a hidden part of the implementation): Make a new file (with open() or similar) Get given the same file descriptor, reused Thread 1: Start running again Actually make the recv() system call, which now refers to thread 3's file Data loss/crash/other badness Since there is no way to determine that a thread is actually inside the system call rather than preempted immediately before it there is no way in POSIX to avoid the race. An alternative implementation seeking to avoid this issue could perhaps supply such a function to determine the blocked state of another thread, or maybe file descriptors could be unique forever, but neither of these seems to apply in this case. Thanks, - Mark ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 3/3] avformat/udp: Enable FIFO when using windows sockets.
Signed-off-by: Matt Oliver--- libavformat/udp.c | 19 +-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/libavformat/udp.c b/libavformat/udp.c index f8c861d..0e4766f 100644 --- a/libavformat/udp.c +++ b/libavformat/udp.c @@ -64,6 +64,14 @@ #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 @@ -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(>cond); } #endif -closesocket(s->udp_fd); +if(s->udp_fd != INVALID_SOCKET) +closesocket(s->udp_fd); av_fifo_freep(>fifo); return 0; } -- 2.10.2.windows.1 0003-avformat-udp-Enable-FIFO-when-using-windows-sockets.patch Description: Binary data ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel