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

Matt Oliver protogonoi at gmail.com
Wed Dec 7 08:03:12 EET 2016


On 4 December 2016 at 01:56, Matt Oliver <protogonoi at 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


More information about the ffmpeg-devel mailing list