[vlc-devel] [PATCH] win32: use the ThreadPoolTimer API to handle timers
Steve Lhomme
robux4 at ycbcr.xyz
Fri Jun 12 09:11:02 CEST 2020
On 2020-06-12 9:06, Rémi Denis-Courmont wrote:
> 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.
I think you misunderstood the patch. There is no memory error. I picked
EFAULT because the name looked nice. I don't care about the error value.
I don't even know why we carry "system" error code in a VLC internal API.
> 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é.
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
>
More information about the vlc-devel
mailing list