From: "Gabriel Ivăncescu" Subject: Re: [PATCH 3/3] quartz/tests: Add tests for when IMediaSeeking on a filter is queried by the filter graph. Message-Id: <27ed60c4-8427-e238-0532-5500645629cf@gmail.com> Date: Tue, 7 Apr 2020 14:52:11 +0300 In-Reply-To: References: On 06/04/2020 20:09, Zebediah Figura wrote: > On 4/6/20 8:07 AM, Gabriel Ivăncescu wrote: >> Signed-off-by: Gabriel Ivăncescu >> --- >> 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, ¶m1, ¶m2, 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.) > Hi Zeb, You're right, it's only on the Release, I just wanted to make it closer to Windows, but it seems it might be too much of an implementation detail. For the tests, I don't think calling Release itself multiple times is the real problem, but only when the ref count becomes 0, so I'll need to add a separate ref count just for testing the IMediaSeeking. Thanks, Gabriel