[FFmpeg-devel] [PATCH 4/4] avformat/udp: do not use thread cancellation when receiving data

[James Almer]

On 1/17/2020 4:51 AM, Matt Oliver wrote:
> On Fri, 17 Jan 2020 at 18:44, Hendrik Leppkes <h.leppkes at gmail.com> wrote:
> 
>> On Fri, Jan 17, 2020 at 12:30 AM Michael Niedermayer
>> <michael at niedermayer.cc> wrote:
>>>
>>> On Thu, Jan 16, 2020 at 01:20:16AM +0100, Marton Balint wrote:
>>>> It is not supported by every threading implementation, and the only
>> thing we
>>>> gain with it is an immediate shutdown of receiving packets on close and
>>>> avoiding the poll call before reading the data.
>>>>
>>>> I don't think it is a big issue if it takes 0.1 sec of delay to close
>> an udp
>>>> stream. Back when this was introduced the delay was 1 sec, which was
>> indeed
>>>> noticable.
>>>>
>>>> And anybody who needs performance sensitive UDP should not use the
>> fifo buffer
>>>> in the first place, because the kernel can buffer the data much more
>>>> effectively.
>>>>
>>>> Signed-off-by: Marton Balint <cus at passwd.hu>
>>>> ---
>>>>  libavformat/udp.c | 57
>> +++++++++++++++++++++++++------------------------------
>>>>  1 file changed, 26 insertions(+), 31 deletions(-)
>>>
>>> this breaks build on mingw64
>>>
>>> src/libavformat/udp.c: In function ‘udp_read’:
>>> src/libavformat/udp.c:980:17: error: implicit declaration of function
>> ‘pthread_cond_timedwait’ [-Werror=implicit-function-declaration]
>>>                  int err = pthread_cond_timedwait(&s->cond, &s->mutex,
>> &tv);
>>>                  ^
>>
>> We could add that to our own w32 pthread wrapper, unfortunately
>> whoever designed pthread_cond_timedwait was on drugs, the absolute
>> timespec is just crazy, instead of a delay in
>> (milli|micro|nano)seconds, so we'll practically undo the math that
>> same function already does to add its timeout to current time. But oh
>> well.
>>
>> - Hendrik
> 
> 
> I had an old patch for adding  pthread_cond_timedwait wrapper for win32.
> Haven't checked if it still applies but its attached if it is useful.

[...]

> diff --git a/compat/w32pthreads.h b/compat/w32pthreads.h
> index 4ac2a99..abd54b2 100644
> --- a/compat/w32pthreads.h
> +++ b/compat/w32pthreads.h
> @@ -38,6 +38,7 @@
>  #define WIN32_LEAN_AND_MEAN
>  #include <windows.h>
>  #include <process.h>
> +#include <time.h>
>  
>  #if _WIN32_WINNT < 0x0600 && defined(__MINGW32__)
>  #undef MemoryBarrier
> @@ -48,6 +49,7 @@
>  #include "libavutil/common.h"
>  #include "libavutil/internal.h"
>  #include "libavutil/mem.h"
> +#include "libavutil/time.h"
>  
>  typedef struct pthread_t {
>      void *handle;
> @@ -171,6 +173,17 @@ static inline int pthread_cond_wait(pthread_cond_t *cond, pthread_mutex_t *mutex
>      return 0;
>  }
>  
> +static inline int pthread_cond_timedwait(pthread_cond_t *cond, pthread_mutex_t *mutex,
> +                                         const struct timespec *abstime)
> +{
> +    int64_t abs_milli = abstime->tv_nsec * 1000LL + abstime->tv_nsec / 1.0e6;
> +    DWORD t = FFMAX(abs_milli - av_gettime(), 0LL);
> +
> +    if (SleepConditionVariableCS(cond, mutex, t) == WAIT_TIMEOUT)

Needs to use SleepConditionVariableSRW() instead, and when it fails it
needs a call to GetLastError() and convert ERROR_TIMEOUT to ETIMEDOUT,
or return EPERM or EINVAL otherwise (No point converting all error codes).

> +        return ETIMEDOUT;
> +    return 0;
> +}
> +
>  static inline int pthread_cond_signal(pthread_cond_t *cond)
>  {
>      WakeConditionVariable(cond);
> @@ -367,6 +380,44 @@ static av_unused int pthread_cond_wait(pthread_cond_t *cond, pthread_mutex_t *mu
>      return pthread_mutex_lock(mutex);
>  }
>  
> +static inline int pthread_cond_timedwait(pthread_cond_t *cond, pthread_mutex_t *mutex,
> +                                         const struct timespec *abstime)
> +{

This one isn't needed anymore. We don't support Windows <= NT5 anymore.

No comment about the os2threads implementation.