Re: [PATCH 4/5] amstream: Implement MediaStreamFilter::WaitUntil.

[Anton Baskanov]

On Friday, 23 October 2020 23:30:55 +07 you wrote:
> On 10/22/20 2:06 PM, Anton Baskanov wrote:
> > Signed-off-by: Anton Baskanov
> >
> > dlls/amstream/filter.c         |  95 +++++++++++++++++-
> >  dlls/amstream/tests/amstream.c | 177 ++++++++++++++++++++++++++++++++-
> >  2 files changed, 266 insertions(+), 6 deletions(-)
> >
> > diff --git a/dlls/amstream/filter.c b/dlls/amstream/filter.c
> > index daeb1fafe71..763ecd434aa 100644
> > --- a/dlls/amstream/filter.c
> > +++ b/dlls/amstream/filter.c
> > @@ -21,6 +21,7 @@
> >
> > #define COBJMACROS
> >  #include "amstream_private.h"
> >  #include "wine/debug.h"
> >
> > +#include "wine/list.h"
> >
> > WINE_DEFAULT_DEBUG_CHANNEL(amstream);
> >
> > @@ -175,6 +176,16 @@ struct filter
> >  IAMMediaStream *seekable_stream;
> >  FILTER_STATE state;
> >  REFERENCE_TIME start_time;
> >
> > - struct list free_events;
> > - struct list used_events;
> >
> > +};
> > -
> >
> > +struct event
> > +{
> > - struct list entry;
> > - HANDLE event;
> > - DWORD_PTR cookie;
> > - BOOL interrupted;
> >
> > };
> >
> > static inline struct filter
> >  *impl_from_IMediaStreamFilter(IMediaStreamFilter *iface)> 
> > @@ -225,6 +236,15 @@ static ULONG WINAPI filter_Release(IMediaStreamFilter
> > *iface)> 
> >      if (!refcount)
> >      {
> > -
> >    struct list *entry;
> > -
> > -
> >    while ((entry = list_head(&filter->free_events)))
> > -
> >    {
> > -
> >        struct event *event = LIST_ENTRY(entry, struct event, entry);
> > -
> >        list_remove(entry);
> > -
> >        CloseHandle(event->event);
> > -
> >        free(event);
> > -
> >    }
> >
> >    for (i = 0; i < filter->nb_streams; ++i)
> >    {
> >
> >        IAMMediaStream_JoinFilter(filter->streams[i], NULL);
> >
> > @@ -261,6 +281,7 @@ static void set_state(struct filter *filter,
> > FILTER_STATE state)> 
> >  static HRESULT WINAPI filter_Stop(IMediaStreamFilter *iface)
> >  {
> >  struct filter *filter = impl_from_IMediaStreamFilter(iface);
> >
> > -
> >
> > struct list *entry;
> >
> > TRACE("iface %p.\n", iface);
> >
> > @@ -268,6 +289,17 @@ static HRESULT WINAPI filter_Stop(IMediaStreamFilter
> > *iface)> 
> >      set_state(filter, State_Stopped);
> > - while ((entry = list_head(&filter->used_events)))
> > - {
> > -
> >    struct event *event = LIST_ENTRY(entry, struct event, entry);
> > -
> > -
> >    list_remove(entry);
>
> It strikes me as a little bit simpler to get rid of this list_remove(),
> use a simple LIST_FOR_EACH_ENTRY in this function, and then always
> list_remove in WaitUntil().

Done.
> > -
> > -
> >    event->interrupted = TRUE;
> > -
> >    IReferenceClock_Unadvise(filter->clock, event->cookie);
> > -
> >    SetEvent(event->event);
> > -
> >
> > }
> > -
> >
> > LeaveCriticalSection(&filter->cs);
> >
> > return S_OK;
> >
> > @@ -664,11 +696,66 @@ static HRESULT WINAPI
> > filter_GetCurrentStreamTime(IMediaStreamFilter *iface, REF> 
> >      return S_OK;
> >
> > }
> >
> > -static HRESULT WINAPI filter_WaitUntil(IMediaStreamFilter *iface,
> > REFERENCE_TIME WaitStreamTime) +static HRESULT WINAPI
> > filter_WaitUntil(IMediaStreamFilter *iface, REFERENCE_TIME time)> 
> >  {
> > - FIXME("(%p)->(%s): Stub!\n", iface,
> >
> > wine_dbgstr_longlong(WaitStreamTime)); +    struct filter *filter =
> > impl_from_IMediaStreamFilter(iface);
> > - struct event *event;
> > - struct list *entry;
> > - HRESULT hr;
> > - return E_NOTIMPL;
> > - TRACE("filter %p, time %s.\n", iface, wine_dbgstr_longlong(time));
> > -
> > - EnterCriticalSection(&filter->cs);
> > -
> > - if (!filter->clock)
> > - {
> > -
> >    LeaveCriticalSection(&filter->cs);
> > -
> >    return E_FAIL;
> > - }
> > -
> > - if ((entry = list_head(&filter->free_events)))
> > - {
> > -
> >    list_remove(entry);
> > -
> >    event = LIST_ENTRY(entry, struct event, entry);
> > - }
> > - else
> > - {
> > -
> >    event = calloc(1, sizeof(struct event));
> > -
> >    event->event = CreateEventW(NULL, FALSE, FALSE, NULL);
> > -
> > -
> >    entry = &event->entry;
> > - }
> > -
> > - hr = IReferenceClock_AdviseTime(filter->clock, time,
> >
> > filter->start_time, (HEVENT)event->event, &event->cookie); +    if
> > (FAILED(hr))
> > - {
> > -
> >    list_add_tail(&filter->free_events, entry);
> > -
> >    LeaveCriticalSection(&filter->cs);
> > -
> >    return hr;
> > - }
> > -
> > - event->interrupted = FALSE;
> > - list_add_tail(&filter->used_events, entry);
> > -
> > - LeaveCriticalSection(&filter->cs);
> > - WaitForSingleObject(event->event, INFINITE);
> > - EnterCriticalSection(&filter->cs);
> > -
> > - if (event->interrupted)
> > - {
> > -
> >    hr = S_FALSE;
> > - }
> > - else
> > - {
> > -
> >    list_remove(entry);
> > -
> >    hr = S_OK;
> > - }
> > -
> > - list_add_tail(&filter->free_events, entry);
> > -
> > - LeaveCriticalSection(&filter->cs);
> > -
> > - return hr;
> >
> > }
> >
> > static HRESULT WINAPI filter_Flush(IMediaStreamFilter *iface, BOOL
> >  bCancelEOS)> 
> > @@ -948,6 +1035,8 @@ HRESULT filter_create(IUnknown *outer, void **out)
> >  object->IMediaStreamFilter_iface.lpVtbl = &filter_vtbl;
> >  object->IMediaSeeking_iface.lpVtbl = &filter_seeking_vtbl;
> >  object->refcount = 1;
> >
> > -
> >
> > list_init(&object->free_events);
> > -
> >
> > list_init(&object->used_events);
> >
> > InitializeCriticalSection(&object->cs);
> > object->cs.DebugInfo->Spare[0] = (DWORD_PTR)(__FILE__ ":
> > MediaStreamFilter.cs");>
> >
> > diff --git a/dlls/amstream/tests/amstream.c
> > b/dlls/amstream/tests/amstream.c index dbde4f6e20e..d89f1ecb109 100644
> > --- a/dlls/amstream/tests/amstream.c
> > +++ b/dlls/amstream/tests/amstream.c
> >
> > @@ -3052,11 +3052,21 @@ out_unknown:
> >      IUnknown_Release(unknown);
> >
> > }
> >
> > +struct advise_time_cookie
> > +{
> > - LONGLONG base;
> > - LONGLONG offset;
> > - HANDLE event;
> > - HANDLE advise_time_called_event;
> > - BOOL unadvise_called;
> >
> > +};
> > -
> >
> > struct testclock
> >  {
> >  IReferenceClock IReferenceClock_iface;
> >  LONG refcount;
> >  LONGLONG time;
> >
> > -
> >
> > struct advise_time_cookie *advise_time_cookie;
> >
> > HRESULT get_time_hr;
> >
> > };
> >
> > @@ -3100,7 +3110,19 @@ static HRESULT WINAPI
> > testclock_GetTime(IReferenceClock *iface, REFERENCE_TIME *> 
> >  static HRESULT WINAPI testclock_AdviseTime(IReferenceClock *iface,
> >  REFERENCE_TIME base, REFERENCE_TIME offset, HEVENT event, DWORD_PTR
> >  *cookie) {
> > - SetEvent((HANDLE)event);
> > -
> >
> > struct testclock *clock = impl_from_IReferenceClock(iface);
> > -
> >
> > if (clock->advise_time_cookie)
> > -
> >
> > {
> > -
> >    clock->advise_time_cookie->base = base;
> > -
> >    clock->advise_time_cookie->offset = offset;
> > -
> >    clock->advise_time_cookie->event = (HANDLE)event;
> > -
> >    SetEvent(clock->advise_time_cookie->advise_time_called_event);
> > -
> >
> > }
> > -
> >
> > else
> > -
> >
> > {
> > -
> >    SetEvent((HANDLE)event);
> > -
> >
> > }
> > -
> >
> > *cookie = (DWORD_PTR)clock->advise_time_cookie;
> >
> > return S_OK;
> >
> > }
> >
> > @@ -3112,8 +3134,9 @@ static HRESULT WINAPI
> > testclock_AdvisePeriodic(IReferenceClock *iface, REFERENCE> 
> >  static HRESULT WINAPI testclock_Unadvise(IReferenceClock *iface,
> >  DWORD_PTR cookie) {
> > - ok(0, "Unexpected call.\n");
> > - return E_NOTIMPL;
> > - if (cookie)
> > -
> >    ((struct advise_time_cookie *)cookie)->unadvise_called = TRUE;
> > - return S_OK;
> >
> > }
> >
> > static IReferenceClockVtbl testclock_vtbl =
> >
> > @@ -6673,6 +6696,153 @@ static void
> > test_mediastreamfilter_reference_time_to_stream_time(void)> 
> >      ok(!ref, "Got outstanding refcount %d.\n", ref);
> >
> > }
> >
> > +struct mediastreamfilter_wait_until_params
> > +{
> > - IMediaStreamFilter *filter;
> > - REFERENCE_TIME time;
> > - HRESULT expected_hr;
> >
> > +};
> > -
> >
> > +static DWORD CALLBACK mediastreamfilter_wait_until(void *p)
> > +{
> > - struct mediastreamfilter_wait_until_params *params = (struct
> >
> > mediastreamfilter_wait_until_params *)p; +    HRESULT hr;
> > -
> > - hr = IMediaStreamFilter_WaitUntil(params->filter, params->time);
> > - ok(hr == params->expected_hr, "Got hr %#x.\n", hr);
> > -
> > - return 0;
> >
> > +}
> > -
> >
> > +static void test_mediastreamfilter_wait_until(void)
> > +{
> > - struct mediastreamfilter_wait_until_params params1;
> > - struct mediastreamfilter_wait_until_params params2;
> > - struct advise_time_cookie cookie1 = { 0 };
> > - struct advise_time_cookie cookie2 = { 0 };
> > - IMediaStreamFilter *filter;
> > - struct testclock clock;
> > - HANDLE thread1;
> > - HANDLE thread2;
> > - HRESULT hr;
> > - ULONG ref;
> > -
> > - hr = CoCreateInstance(&CLSID_MediaStreamFilter, NULL,
> >
> > CLSCTX_INPROC_SERVER, +            &IID_IMediaStreamFilter, (void
> > **)&filter);
> > - ok(hr == S_OK, "Got hr %#x.\n", hr);
> > - testclock_init(&clock);
> > - cookie1.advise_time_called_event = CreateEventW(NULL, FALSE, FALSE,
> >
> > NULL); +    cookie2.advise_time_called_event = CreateEventW(NULL, FALSE,
> > FALSE, NULL); +
> > - hr = IMediaStreamFilter_Run(filter, 12345678);
> > - ok(hr == S_OK, "Got hr %#x.\n", hr);
> > -
> > - hr = IMediaStreamFilter_WaitUntil(filter, 23456789);
> > - ok(hr == E_FAIL, "Got hr %#x.\n", hr);
> > -
> > - hr = IMediaStreamFilter_Stop(filter);
> > - ok(hr == S_OK, "Got hr %#x.\n", hr);
> > -
> > - hr = IMediaStreamFilter_SetSyncSource(filter,
> >
> > &clock.IReferenceClock_iface); +    ok(hr == S_OK, "Got hr %#x.\n", hr);
> > -
> > - clock.advise_time_cookie = &cookie1;
> > -
> > - params1.filter = filter;
> > - params1.time = 23456789;
> > - params1.expected_hr = S_OK;
> > - thread1 = CreateThread(NULL, 0, mediastreamfilter_wait_until,
> >
> > &params1, 0, NULL); +   
> > ok(!WaitForSingleObject(cookie1.advise_time_called_event, 2000),
> > "Expected AdviseTime to be called.\n"); +   
> > ok(WaitForSingleObject(thread1, 100) == WAIT_TIMEOUT, "WaitUntil returned
> > prematurely.\n"); +
> > - ok(cookie1.base == 23456789, "Got base %s.\n",
> >
> > wine_dbgstr_longlong(cookie1.base)); +    ok(cookie1.offset == 12345678,
> > "Got offset %s.\n", wine_dbgstr_longlong(cookie1.offset)); +   
> > ok(!!cookie1.event, "Expected non-NULL event.\n");
> > -
> > - SetEvent(cookie1.event);
> > -
> > - ok(!WaitForSingleObject(thread1, 2000), "Wait timed out.\n");
> > - CloseHandle(thread1);
> > -
> > - ok(!cookie1.unadvise_called, "Unexpected Unadvise call.\n");
> > -
> > - hr = IMediaStreamFilter_Run(filter, 12345678);
> > - ok(hr == S_OK, "Got hr %#x.\n", hr);
> > -
> > - clock.advise_time_cookie = &cookie1;
> > -
> > - params1.filter = filter;
> > - params1.time = 23456789;
> > - params1.expected_hr = S_OK;
> > - thread1 = CreateThread(NULL, 0, mediastreamfilter_wait_until,
> >
> > &params1, 0, NULL); +   
> > ok(!WaitForSingleObject(cookie1.advise_time_called_event, 2000),
> > "Expected AdviseTime to be called.\n"); +   
> > ok(WaitForSingleObject(thread1, 100) == WAIT_TIMEOUT, "WaitUntil returned
> > prematurely.\n"); +
> > - ok(cookie1.base == 23456789, "Got base %s.\n",
> >
> > wine_dbgstr_longlong(cookie1.base)); +    ok(cookie1.offset == 12345678,
> > "Got offset %s.\n", wine_dbgstr_longlong(cookie1.offset)); +   
> > ok(!!cookie1.event, "Expected non-NULL event.\n");
> > -
> > - clock.advise_time_cookie = &cookie2;
> > -
> > - params2.filter = filter;
> > - params2.time = 11111111;
> > - params2.expected_hr = S_OK;
> > - thread2 = CreateThread(NULL, 0, mediastreamfilter_wait_until,
> >
> > &params2, 0, NULL); +   
> > ok(!WaitForSingleObject(cookie2.advise_time_called_event, 2000),
> > "Expected AdviseTime to be called.\n"); +   
> > ok(WaitForSingleObject(thread2, 100) == WAIT_TIMEOUT, "WaitUntil returned
> > prematurely.\n"); +
> > - ok(cookie2.base == 11111111, "Got base %s.\n",
> >
> > wine_dbgstr_longlong(cookie2.base)); +    ok(cookie2.offset == 12345678,
> > "Got offset %s.\n", wine_dbgstr_longlong(cookie2.offset)); +   
> > ok(!!cookie2.event, "Expected non-NULL event.\n");
> > -
> > - SetEvent(cookie1.event);
> > -
> > - ok(!WaitForSingleObject(thread1, 2000), "Wait timed out.\n");
> > - CloseHandle(thread1);
> > -
> > - ok(WaitForSingleObject(thread2, 100) == WAIT_TIMEOUT, "WaitUntil
> >
> > returned prematurely.\n"); +
> > - SetEvent(cookie2.event);
> > -
> > - ok(!WaitForSingleObject(thread2, 2000), "Wait timed out.\n");
> > - CloseHandle(thread2);
> > -
> > - clock.advise_time_cookie = &cookie1;
> > -
> > - params1.filter = filter;
> > - params1.time = 23456789;
> > - params1.expected_hr = S_FALSE;
> > - thread1 = CreateThread(NULL, 0, mediastreamfilter_wait_until,
> >
> > &params1, 0, NULL); +   
> > ok(!WaitForSingleObject(cookie1.advise_time_called_event, 2000),
> > "Expected AdviseTime to be called.\n"); +   
> > ok(WaitForSingleObject(thread1, 100) == WAIT_TIMEOUT, "WaitUntil returned
> > prematurely.\n"); +
> > - clock.advise_time_cookie = &cookie2;
> > -
> > - params2.filter = filter;
> > - params2.time = 23456789;
> > - params2.expected_hr = S_FALSE;
> > - thread2 = CreateThread(NULL, 0, mediastreamfilter_wait_until,
> >
> > &params2, 0, NULL); +   
> > ok(!WaitForSingleObject(cookie2.advise_time_called_event, 2000),
> > "Expected AdviseTime to be called.\n"); +   
> > ok(WaitForSingleObject(thread2, 100) == WAIT_TIMEOUT, "WaitUntil returned
> > prematurely.\n"); +
> > - hr = IMediaStreamFilter_Stop(filter);
> > - ok(hr == S_OK, "Got hr %#x.\n", hr);
> > -
> > - ok(cookie1.unadvise_called, "Expected Unadvise to be called.\n");
> > - ok(cookie2.unadvise_called, "Expected Unadvise to be called.\n");
> > -
> > - ok(!WaitForSingleObject(thread1, 2000), "Wait timed out.\n");
> > - CloseHandle(thread1);
> > - ok(!WaitForSingleObject(thread2, 2000), "Wait timed out.\n");
> > - CloseHandle(thread2);
>
> Can you please also check whether flushing unblocks waits? The
> documentation implies it does, but it's also been wrong before...

MediaStreamFilter::Flush was unimplemented, so I added another patch to the 
series that implements it and adds tests.
> > -
> > - CloseHandle(cookie1.advise_time_called_event);
> > - CloseHandle(cookie2.advise_time_called_event);
> > -
> > - ref = IMediaStreamFilter_Release(filter);
> > - ok(!ref, "Got outstanding refcount %d.\n", ref);
> >
> > +}
> > -
> >
> > static void test_ddrawstream_getsetdirectdraw(void)
> >  {
> >  IAMMultiMediaStream *mmstream = create_ammultimediastream();
> >
> > @@ -8318,6 +8488,7 @@ START_TEST(amstream)
> >  test_mediastreamfilter_get_stop_position();
> >  test_mediastreamfilter_get_current_stream_time();
> >  test_mediastreamfilter_reference_time_to_stream_time();
> >
> > -
> >
> > test_mediastreamfilter_wait_until();
> >
> > CoUninitialize();
> >
> > }