From: Zebediah Figura <zfigura@codeweavers.com>
Subject: Re: [PATCH 3/3] quartz/tests: Add tests for when IMediaSeeking on a filter is queried by the filter graph.
Message-Id: <e6897cb3-d282-4c0b-bbf9-f1bff566ffd0@codeweavers.com>
Date: Mon, 6 Apr 2020 12:09:27 -0500
In-Reply-To: <a295bb2e908124ba87582cb693723c0540112ad8.1586178366.git.gabrielopcode@gmail.com>
References: <afda467ef6e7c9b18cb1bd334d8b308655b96ebd.1586178366.git.gabrielopcode@gmail.com> <a295bb2e908124ba87582cb693723c0540112ad8.1586178366.git.gabrielopcode@gmail.com>

On 4/6/20 8:07 AM, Gabriel Ivăncescu wrote:
> Signed-off-by: Gabriel Ivăncescu <gabrielopcode@gmail.com>
> ---
>  dlls/quartz/tests/filtergraph.c | 45 +++++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
> 
> diff --git a/dlls/quartz/tests/filtergraph.c b/dlls/quartz/tests/filtergraph.c
> index 9988dca..2c787c4 100644
> --- a/dlls/quartz/tests/filtergraph.c
> +++ b/dlls/quartz/tests/filtergraph.c
> @@ -26,6 +26,35 @@
>  #include "wine/heap.h"
>  #include "wine/test.h"
>  
> +#define DEFINE_EXPECT(func) \
> +    static BOOL expect_ ## func = FALSE, called_ ## func = FALSE
> +
> +#define SET_EXPECT(func) \
> +    do { called_ ## func = FALSE; expect_ ## func = TRUE; } while(0)
> +
> +#define CHECK_EXPECT2(func) \
> +    do { \
> +        ok(expect_ ##func, "unexpected call " #func "\n"); \
> +        called_ ## func = TRUE; \
> +    }while(0)
> +
> +#define CHECK_EXPECT(func) \
> +    do { \
> +        CHECK_EXPECT2(func); \
> +        expect_ ## func = FALSE; \
> +    }while(0)
> +
> +#define CHECK_CALLED(func) \
> +    do { \
> +        ok(called_ ## func, "expected " #func "\n"); \
> +        expect_ ## func = called_ ## func = FALSE; \
> +    }while(0)
> +
> +#define CLEAR_CALLED(func) \
> +    expect_ ## func = called_ ## func = FALSE
> +
> +DEFINE_EXPECT(QI_IMediaSeeking);
> +
>  static const GUID testguid = {0xabbccdde};
>  
>  typedef struct TestFilterImpl
> @@ -1273,6 +1302,7 @@ static HRESULT WINAPI testfilter_QueryInterface(IBaseFilter *iface, REFIID iid,
>      }
>      else if (IsEqualGUID(iid, &IID_IMediaSeeking) && filter->IMediaSeeking_iface.lpVtbl)
>      {
> +        CHECK_EXPECT2(QI_IMediaSeeking);
>          *out = &filter->IMediaSeeking_iface;
>      }
>      else if (IsEqualGUID(iid, &IID_IReferenceClock) && filter->IReferenceClock_iface.lpVtbl)
> @@ -3345,8 +3375,10 @@ todo_wine
>  static HRESULT check_ec_complete(IFilterGraph2 *graph, IBaseFilter *filter)
>  {
>      IMediaEventSink *eventsink;
> +    BOOL has_seeking = FALSE;
>      LONG_PTR param1, param2;
>      IMediaControl *control;
> +    IMediaSeeking *seeking;
>      IMediaEvent *eventsrc;
>      HRESULT hr, ret_hr;
>      LONG code;
> @@ -3355,7 +3387,17 @@ static HRESULT check_ec_complete(IFilterGraph2 *graph, IBaseFilter *filter)
>      IFilterGraph2_QueryInterface(graph, &IID_IMediaEvent, (void **)&eventsrc);
>      IFilterGraph2_QueryInterface(graph, &IID_IMediaEventSink, (void **)&eventsink);
>  
> +    SET_EXPECT(QI_IMediaSeeking);
> +    if (SUCCEEDED(IBaseFilter_QueryInterface(filter, &IID_IMediaSeeking, (void **)&seeking)))
> +    {
> +        IMediaSeeking_Release(seeking);
> +        has_seeking = TRUE;
> +    }
> +    CLEAR_CALLED(QI_IMediaSeeking);
> +
> +    if (has_seeking) SET_EXPECT(QI_IMediaSeeking);
>      IMediaControl_Run(control);
> +    if (has_seeking) CHECK_CALLED(QI_IMediaSeeking);
>  
>      hr = IMediaEvent_GetEvent(eventsrc, &code, &param1, &param2, 0);
>      ok(hr == E_ABORT, "Got hr %#x.\n", hr);
> @@ -3766,9 +3808,12 @@ static void test_graph_seeking(void)
>  
>      filter1.seek_caps = AM_SEEKING_CanDoSegments | AM_SEEKING_CanGetCurrentPos;
>      filter2.seek_caps = AM_SEEKING_CanDoSegments | AM_SEEKING_CanGetDuration;
> +
> +    SET_EXPECT(QI_IMediaSeeking);
>      hr = IMediaSeeking_GetCapabilities(seeking, &caps);
>      ok(hr == S_OK, "Got hr %#x.\n", hr);
>      ok(caps == AM_SEEKING_CanDoSegments, "Got caps %#x.\n", caps);
> +    CHECK_CALLED(QI_IMediaSeeking);
>  
>      caps = AM_SEEKING_CanDoSegments | AM_SEEKING_CanGetCurrentPos;
>      hr = IMediaSeeking_CheckCapabilities(seeking, &caps);
> 


Does "The Legend of Heroes" actually depend on IMediaSeeking being
queried at a certain time, or is this just a way of testing that it's
not queried every time we do a seeking call?

If the latter, I think it would be better to test the part that actually
matters, i.e. that IMediaSeeking::Release() is only called once, and
only when the graph is torn down. That way you don't need patch 2/3.

(As an aside, I've always felt sort of lukewarm about the CHECK_EXPECT
framework; I think a simple global variable such as is used in the
qasf:dmowrapper tests is more transparent and flexible.)