[vlc-devel] [PATCH] win32: use the ThreadPoolTimer API to handle timers

[Steve Lhomme]

On 2020-06-11 17:25, Rémi Denis-Courmont wrote:
> Le keskiviikkona 10. kesäkuuta 2020, 15.00.57 EEST Steve Lhomme a écrit :
>> This is available since Windows 7 and is allowed in Winstore apps. So we
>> only maintain one code for all Windows versions.
>>
>> It should handle timers with an absolute time better as they had to be
>> turned into relative times but the system sleep was delaying the timer. [1]
>>
>> [1]
>> https://docs.microsoft.com/en-us/windows/win32/api/threadpoollegacyapiset/n
>> f-threadpoollegacyapiset-createtimerqueuetimer "The time that the system
>> spends in sleep or hibernation does not count toward the expiration of the
>> timer."
>> ---
>>   src/Makefile.am   |  5 +++--
>>   src/win32/timer.c | 46 ++++++++++++++++++++++++----------------------
>>   2 files changed, 27 insertions(+), 24 deletions(-)
>>
>> diff --git a/src/Makefile.am b/src/Makefile.am
>> index 9e088c1fd9e6..dc75776d7af7 100644
>> --- a/src/Makefile.am
>> +++ b/src/Makefile.am
>> @@ -406,11 +406,12 @@ libvlccore_la_SOURCES += \
>>   	win32/rand.c \
>>   	win32/specific.c \
>>   	win32/thread.c \
>> +	win32/timer.c \
>>   	win32/winsock.c
>>   if HAVE_WINSTORE
>> -libvlccore_la_SOURCES += posix/timer.c win32/dirs-uap.c
>> +libvlccore_la_SOURCES += win32/dirs-uap.c
>>   else
>> -libvlccore_la_SOURCES += win32/timer.c win32/dirs.c
>> +libvlccore_la_SOURCES += win32/dirs.c
>>   endif
>>   endif
>>
>> diff --git a/src/win32/timer.c b/src/win32/timer.c
>> index 4fe3d2b067cb..4a2d68f996fb 100644
>> --- a/src/win32/timer.c
>> +++ b/src/win32/timer.c
>> @@ -29,16 +29,16 @@
>>
>>   struct vlc_timer
>>   {
>> -    HANDLE handle;
>> +    PTP_TIMER ptimer;
>>       void (*func) (void *);
>>       void *data;
>>   };
>>
>> -static void CALLBACK vlc_timer_do (void *val, BOOLEAN timeout)
>> +static void CALLBACK vlc_timer_do(PTP_CALLBACK_INSTANCE instance, void
>> *val, PTP_TIMER ptimer) {
>> +    (void) instance; (void) ptimer;
>>       struct vlc_timer *timer = val;
>>
>> -    assert (timeout);
>>       timer->func (timer->data);
>>   }
>>
>> @@ -50,50 +50,52 @@ int vlc_timer_create (vlc_timer_t *id, void (*func)
>> (void *), void *data) return ENOMEM;
>>       timer->func = func;
>>       timer->data = data;
>> -    timer->handle = INVALID_HANDLE_VALUE;
>> +    timer->ptimer = CreateThreadpoolTimer(vlc_timer_do, timer, NULL);
>> +    if (unlikely(timer->ptimer == NULL))
>> +    {
>> +        free(timer);
>> +        return EFAULT;
> 
> Is EFAULT really the intended error scenario? IMO, it's better to abort than
> return EFAULT, because it can only mean the calling code is buggy.

I would never add an abort in the core (or in modules for that matter). 
If you mean asserting, maybe. But do have a return value here that 
callers should handle (and they do) so I don't see why we would want to 
add extra checks.

I don't see how this is a bug in our code if the OS cannot create a 
timer pool at that moment. The documentation doesn't say in which case 
it can happen, but it can.

>> +    }
>>       *id = timer;
>>       return 0;
>>   }
>>
>>   void vlc_timer_destroy (vlc_timer_t timer)
>>   {
>> -    if (timer->handle != INVALID_HANDLE_VALUE)
>> -        DeleteTimerQueueTimer (NULL, timer->handle, INVALID_HANDLE_VALUE);
>> +    SetThreadpoolTimer(timer->ptimer, NULL, 0, 0);
>> +    WaitForThreadpoolTimerCallbacks(timer->ptimer, TRUE);
>> +    CloseThreadpoolTimer(timer->ptimer);
>>       free (timer);
>>   }
>>
>>   void vlc_timer_schedule (vlc_timer_t timer, bool absolute,
>>                            vlc_tick_t value, vlc_tick_t interval)
>>   {
>> -    if (timer->handle != INVALID_HANDLE_VALUE)
>> -    {
>> -        DeleteTimerQueueTimer (NULL, timer->handle, INVALID_HANDLE_VALUE);
>> -        timer->handle = INVALID_HANDLE_VALUE;
>> -    }
>> +    SetThreadpoolTimer(timer->ptimer, NULL, 0, 0);
>> +    WaitForThreadpoolTimerCallbacks(timer->ptimer, TRUE);
>>       if (value == VLC_TIMER_DISARM)
>>           return; /* Disarm */
>>
>> +    ULARGE_INTEGER s;
>>       if (absolute)
>> -    {
>> -        value -= vlc_tick_now ();
>> -        if (value < 0)
>> -            value = 0;
>> -    }
>> +        s.QuadPart = MSFTIME_FROM_VLC_TICK(value);
>> +    else
>> +        s.QuadPart = -MSFTIME_FROM_VLC_TICK(value);
>> +    FILETIME ts;
>> +    ts.dwLowDateTime  = s.LowPart;
>> +    ts.dwHighDateTime = s.HighPart;
>>
>> -    DWORD val    = MS_FROM_VLC_TICK(value);
>>       DWORD interv = MS_FROM_VLC_TICK(interval);
>> -    if (val == 0 && value != 0)
>> -        val = 1; /* rounding error */
>>       if (interv == 0 && interval != 0)
>>           interv = 1; /* rounding error */
>>
>> -    if (!CreateTimerQueueTimer(&timer->handle, NULL, vlc_timer_do, timer,
>> -                               val, interv, WT_EXECUTEDEFAULT))
>> -        abort ();
>> +    WaitForThreadpoolTimerCallbacks(timer->ptimer, FALSE);
>> +    SetThreadpoolTimer(timer->ptimer, &ts, interv, 0 );
>>   }
>>
>>   unsigned vlc_timer_getoverrun (vlc_timer_t timer)
>>   {
>> +    // TODO return the amount of missed timers if any
>>       (void)timer;
>>       return 0;
>>   }
> 
> 
> -- 
> Rémi Denis-Courmont
> http://www.remlab.net/
> 
> 
> 
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
>