[vlc-devel] [PATCH] win32: use the ThreadPoolTimer API to handle timers
Rémi Denis-Courmont
remi at remlab.net
Fri Jun 12 09:06:14 CEST 2020
Memory fault should abort or reliably crash the process ASAP. Nothing new or VLC-specific here.
I'm not saying that we should proactively check for those errors since they are by definition UB and not supposed to happen. But to the extent that we detect them anyway, we must abort.
Le 12 juin 2020 08:31:30 GMT+03:00, Steve Lhomme <robux4 at ycbcr.xyz> a écrit :
>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
>>
>_______________________________________________
>vlc-devel mailing list
>To unsubscribe or modify your subscription options:
>https://mailman.videolan.org/listinfo/vlc-devel
--
Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser ma brièveté.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20200612/107e96cb/attachment.html>
More information about the vlc-devel
mailing list