[vlc-devel] [PATCH 2/2] aout: wasapi: use a vlc_timer for the deferred start

[Thomas Guillem]

On Wed, Jun 10, 2020, at 09:25, Steve Lhomme wrote:
> On 2020-06-10 8:56, Thomas Guillem wrote:
> > 
> > On Tue, Jun 9, 2020, at 17:33, Rémi Denis-Courmont wrote:
> >> Le tiistaina 9. kesäkuuta 2020, 17.48.48 EEST Steve Lhomme a écrit :
> >>> The timer API is not supported in Winstore builds so switch to something
> >>> that works for all.
> >>> ---
> >>>   modules/audio_output/wasapi.c | 41 ++++++++++++++++-------------------
> >>>   1 file changed, 19 insertions(+), 22 deletions(-)
> >>>
> >>> diff --git a/modules/audio_output/wasapi.c b/modules/audio_output/wasapi.c
> >>> index ecfcc99ae1dd..3644d7cea5ae 100644
> >>> --- a/modules/audio_output/wasapi.c
> >>> +++ b/modules/audio_output/wasapi.c
> >>> @@ -91,7 +91,8 @@ static msftime_t GetQPC(void)
> >>>   typedef struct aout_stream_sys
> >>>   {
> >>>       IAudioClient *client;
> >>> -    HANDLE hTimer;
> >>> +    vlc_timer_t timer;
> >>> +    atomic_bool has_timer;
> >>>
> >>>   #define STARTED_STATE_INIT 0
> >>>   #define STARTED_STATE_OK 1
> >>> @@ -112,11 +113,8 @@ typedef struct aout_stream_sys
> >>>   static void ResetTimer(aout_stream_t *s)
> >>>   {
> >>>       aout_stream_sys_t *sys = s->sys;
> >>> -    if (sys->hTimer != NULL)
> >>> -    {
> >>> -        DeleteTimerQueueTimer(NULL, sys->hTimer, INVALID_HANDLE_VALUE);
> >>> -        sys->hTimer = NULL;
> >>> -    }
> >>> +    if (atomic_exchange(&sys->has_timer,false))
> >>> +        vlc_timer_destroy(sys->timer);
> >>>   }
> >>>
> >>>   /*** VLC audio output callbacks ***/
> >>> @@ -160,7 +158,7 @@ static HRESULT TimeGet(aout_stream_t *s, vlc_tick_t
> >>> *restrict delay) return hr;
> >>>   }
> >>>
> >>> -static void DoStartDeferred(void *val)
> >>> +static void StartDeferredCallback(void *val)
> >>>   {
> >>>       aout_stream_t *s = val;
> >>>       aout_stream_sys_t *sys = s->sys;
> >>> @@ -169,25 +167,24 @@ static void DoStartDeferred(void *val)
> >>>       atomic_store(&sys->started_state,
> >>>                    SUCCEEDED(hr) ? STARTED_STATE_OK : STARTED_STATE_ERROR);
> >>>   }
> >>> -static void CALLBACK StartDeferredCallback(void *val, BOOLEAN timeout)
> >>> -{
> >>> -    DoStartDeferred(val);
> >>> -    (void)timeout;
> >>> -}
> >>>
> >>>   static bool StartTimer(aout_stream_t *s, vlc_tick_t start_delay)
> >>>   {
> >>>       aout_stream_sys_t *sys = s->sys;
> >>>       bool timer_updated = false;
> >>> -    DWORD start_delay_ms = MS_FROM_VLC_TICK(start_delay);
> >>> -    if (sys->hTimer == NULL)
> >>> -        timer_updated =
> >>> -            CreateTimerQueueTimer(&sys->hTimer, NULL,
> >>> StartDeferredCallback, -                                    s,
> >>> start_delay_ms, 0,
> >>> -                                    WT_EXECUTEDEFAULT |
> >>> WT_EXECUTEONLYONCE); -    else
> >>> -        timer_updated =
> >>> -            ChangeTimerQueueTimer(NULL, sys->hTimer, start_delay_ms, 0);
> >>> +    if (!atomic_load(&sys->has_timer))
> >>
> >> I don't follow how the atomic works here. It looks like it's either
> >> unnecessary or insufficient, as it would race with vlc_timer_create().
> > 
> > Indeed, note that every audio callbacks are called from the same thread.
> 
> I saw started_state, an atomic_char, is used more or less in the same 
> places, so went for an atomic. I will remove the atomic. But it seems 
> that variable doesn't one either. It's only accessed via callbacks as 
> well (TimeGet, Play, Pause, Flush, Stop).
> 

started_date is used from 2 threads. Audio callback thread and StartDeferredCallback().

> >>> +    {
> >>> +        if (vlc_timer_create( &sys->timer, StartDeferredCallback, s ) == 0)
> >>> +        {
> >>> +            msg_Warn(s, "start THREAD!");
> >>> +            atomic_store(&sys->has_timer, true);
> >>> +        }
> >>> +    }
> >>> +    if (atomic_load(&sys->has_timer))
> >>> +    {
> >>> +        vlc_timer_schedule( sys->timer, false, start_delay, 0);
> >>> +        timer_updated = true;
> >>> +    }
> >>>       return timer_updated;
> >>>   }
> >>>
> >>> @@ -759,7 +756,7 @@ static HRESULT Start(aout_stream_t *s,
> >>> audio_sample_format_t *restrict pfmt, if (unlikely(sys == NULL))
> >>>           return E_OUTOFMEMORY;
> >>>       sys->client = NULL;
> >>> -    sys->hTimer = NULL;
> >>> +    atomic_init(&sys->has_timer, false);
> >>>       atomic_init(&sys->started_state, STARTED_STATE_INIT);
> >>>
> >>>       /* Configure audio stream */
> >>
> >>
> >> -- 
> >> レミ・デニ-クールモン
> >> 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
> > 
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel