[PATCH v2 12/13] qedit/tests: Add tests for get_Filter and put_Filter with a custom filter.

[Zebediah Figura]

On 4/17/20 7:26 AM, Gabriel Ivăncescu wrote:
> On 16/04/2020 19:44, Zebediah Figura wrote:
>> On 4/16/20 10:25 AM, Gabriel Ivăncescu wrote:
>>> Signed-off-by: Gabriel Ivăncescu <gabrielopcode at gmail.com>
>>> ---
>>>   dlls/qedit/tests/mediadet.c | 965 ++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 965 insertions(+)
>>>
>>
>> It might also be nice to convert the rest of test_mediadet() to use a
>> custom filter, instead of relying on behaviour of the native AVI
>> demuxer. I won't call that necessary for this series, although I don't
>> particularly like that test_mediadet() is now a mixture of two different
>> source filters; it'd be preferable to me to at least add these tests in
>> a separate function.
>>
>> Also, can this patch be moved earlier in the series? It's nice to have
>> tests before fixes, so we can see more clearly what's fixed by each
>> patch. That might require temporarily guarding some code by e.g. "if (hr
>> == S_OK)". If it can't be done reasonably, though, that's fine.
>>
> 
> Uhm, I think the current tests layout is fine. I'm not particularly 
> thrilled with splitting it into two functions: half of the reason I put 
> it here was to test the relationship between put_Filter, put_Filename 
> and get_Filter (there's the reason I placed them at varying parts of the 
> function). There's no much point splitting it since we'd end up still 
> having to do those tests, anyway, which would just duplicate them.
> Also, the put_Filename always uses the native AVI demuxer in this case, 
> so obviously it will still have to remain to be able to test that 
> properly. It shouldn't be removed.
> 

Hmm, right, I forgot that put_Filename() does autoplugging.

I don't particularly like how big test_mediadet() is, but I guess I can 
deal with that.

> I'll try to see if I can move it earlier, though.
> 
>>> diff --git a/dlls/qedit/tests/mediadet.c b/dlls/qedit/tests/mediadet.c
>>> index 596171b..10127cf 100644
>>> --- a/dlls/qedit/tests/mediadet.c
>>> +++ b/dlls/qedit/tests/mediadet.c
>>> @@ -24,6 +24,7 @@
>>>   #include "ole2.h"
>>>   #include "vfwmsgs.h"
>>>   #include "uuids.h"
>>> +#include "dshow.h"
>>>   #include "wine/test.h"
>>>   #include "qedit.h"
>>>   #include "control.h"
>>> @@ -71,6 +72,867 @@ static const IUnknownVtbl outer_vtbl =
>>>   static IUnknown test_outer = {&outer_vtbl};
>>> +struct testfilter
>>> +{
>>> +    IBaseFilter IBaseFilter_iface;
>>> +    IMediaSeeking IMediaSeeking_iface;
>>> +    IMediaPosition IMediaPosition_iface;
>>> +    LONG ref;
>>> +    WCHAR *name;
>>> +    IFilterGraph *graph;
>>> +    FILTER_STATE state;
>>> +    double rate;
>>> +
>>> +    IEnumPins IEnumPins_iface;
>>> +    UINT enum_idx;
>>> +
>>> +    IPin IPin_iface;
>>> +    IPin *peer;
>>> +
>>> +    IEnumMediaTypes IEnumMediaTypes_iface;
>>> +    UINT mt_enum_idx;
>>> +};
>>> +
>>
>> Could we use strmbase here instead?
>>
>> ...
>>
> 
> I wasn't aware that we can use it in tests since they could potentially 
> be compiled on Windows. I'm not familiar with strmbase much, though, so 
> I'll have to look into it. (I mostly just used the layout of the test 
> code from quartz's filtergraph tests)

Yes, we can; we do compile half of DirectShow as PE after all. 
quartz:filtergraph uses its own filters, partially because it preceded 
the ability to use strmbase in tests, but at this point also because it 
does some unusual things that aren't worth hacking into strmbase (in 
particular see e.g. test_connect_direct_vtbl).

If you want examples, there are plenty in other quartz tests, see e.g. 
qasf:dmowrapper, quartz:videorenderer, qcap:capturegraph...

>>> +static inline struct testfilter 
>>> *impl_from_IMediaSeeking(IMediaSeeking *iface)
>>> +{
>>> +    return CONTAINING_RECORD(iface, struct testfilter, 
>>> IMediaSeeking_iface);
>>> +}
>>> +
>>> +static HRESULT WINAPI testseek_QueryInterface(IMediaSeeking *iface, 
>>> REFIID iid, void **out)
>>> +{
>>> +    struct testfilter *filter = impl_from_IMediaSeeking(iface);
>>> +    return IBaseFilter_QueryInterface(&filter->IBaseFilter_iface, 
>>> iid, out);
>>> +}
>>> +
>>> +static ULONG WINAPI testseek_AddRef(IMediaSeeking *iface)
>>> +{
>>> +    struct testfilter *filter = impl_from_IMediaSeeking(iface);
>>> +    return IBaseFilter_AddRef(&filter->IBaseFilter_iface);
>>> +}
>>> +
>>> +static ULONG WINAPI testseek_Release(IMediaSeeking *iface)
>>> +{
>>> +    struct testfilter *filter = impl_from_IMediaSeeking(iface);
>>> +    return IBaseFilter_Release(&filter->IBaseFilter_iface);
>>> +}
>>> +
>>> +static HRESULT WINAPI testseek_GetCapabilities(IMediaSeeking *iface, 
>>> DWORD *caps)
>>> +{
>>> +    *caps = AM_SEEKING_CanGetDuration;
>>> +    return S_OK;
>>> +}
>>
>> Can you please add trace messages to locally defined methods, along the
>> lines of "if (winetest_debug > 1) trace("GetCapabilities()\n");"
>>
>>> +
>>> +static HRESULT WINAPI testseek_CheckCapabilities(IMediaSeeking 
>>> *iface, DWORD *caps)
>>> +{
>>> +    ok(0, "Unexpected call.\n");
>>> +    return E_NOTIMPL;
>>> +}
>>> +
>>> +static HRESULT WINAPI testseek_IsFormatSupported(IMediaSeeking 
>>> *iface, const GUID *format)
>>> +{
>>> +    return IsEqualGUID(format, &TIME_FORMAT_MEDIA_TIME) ? S_OK : 
>>> S_FALSE;
>>> +}
>>> +
>>> +static HRESULT WINAPI testseek_QueryPreferredFormat(IMediaSeeking 
>>> *iface, GUID *format)
>>> +{
>>> +    *format = TIME_FORMAT_MEDIA_TIME;
>>> +    return S_OK;
>>> +}
>>> +
>>> +static HRESULT WINAPI testseek_GetTimeFormat(IMediaSeeking *iface, 
>>> GUID *format)
>>> +{
>>> +    *format = TIME_FORMAT_MEDIA_TIME;
>>> +    return S_OK;
>>> +}
>>> +
>>> +static HRESULT WINAPI testseek_IsUsingTimeFormat(IMediaSeeking 
>>> *iface, const GUID *format)
>>> +{
>>> +    return IsEqualGUID(format, &TIME_FORMAT_MEDIA_TIME) ? S_OK : 
>>> S_FALSE;
>>> +}
>>> +
>>> +static HRESULT WINAPI testseek_SetTimeFormat(IMediaSeeking *iface, 
>>> const GUID *format)
>>> +{
>>> +    return IsEqualGUID(format, &TIME_FORMAT_MEDIA_TIME) ? S_OK : 
>>> E_INVALIDARG;
>>> +}
>>> +
>>> +static HRESULT WINAPI testseek_GetDuration(IMediaSeeking *iface, 
>>> LONGLONG *duration)
>>> +{
>>> +    *duration = 42000000;
>>> +    return S_OK;
>>> +}
>>> +
>>> +static HRESULT WINAPI testseek_GetStopPosition(IMediaSeeking *iface, 
>>> LONGLONG *stop)
>>> +{
>>> +    *stop = 42000000;
>>> +    return S_OK;
>>> +}
>>> +
>>> +static HRESULT WINAPI testseek_GetCurrentPosition(IMediaSeeking 
>>> *iface, LONGLONG *current)
>>> +{
>>> +    *current = 0;
>>> +    return S_OK;
>>> +}
>>> +
>>> +static HRESULT WINAPI testseek_ConvertTimeFormat(IMediaSeeking 
>>> *iface, LONGLONG *target,
>>> +    const GUID *target_format, LONGLONG source, const GUID 
>>> *source_format)
>>> +{
>>> +    ok(0, "Unexpected call.\n");
>>> +    return E_NOTIMPL;
>>> +}
>>> +
>>> +static HRESULT WINAPI testseek_SetPositions(IMediaSeeking *iface, 
>>> LONGLONG *current,
>>> +    DWORD current_flags, LONGLONG *stop, DWORD stop_flags)
>>> +{
>>> +    *current = 0;
>>> +    *stop = 42000000;
>>> +    return S_OK;
>>> +}
>>> +
>>> +static HRESULT WINAPI testseek_GetPositions(IMediaSeeking *iface, 
>>> LONGLONG *current, LONGLONG *stop)
>>> +{
>>> +    *current = 0;
>>> +    *stop = 42000000;
>>> +    return S_OK;
>>> +}
>>> +
>>> +static HRESULT WINAPI testseek_GetAvailable(IMediaSeeking *iface, 
>>> LONGLONG *earliest, LONGLONG *latest)
>>> +{
>>> +    *earliest = 0;
>>> +    *latest = 42000000;
>>> +    return S_OK;
>>> +}
>>> +
>>> +static HRESULT WINAPI testseek_SetRate(IMediaSeeking *iface, double 
>>> rate)
>>> +{
>>> +    struct testfilter *filter = impl_from_IMediaSeeking(iface);
>>> +
>>> +    filter->rate = rate;
>>> +    return S_OK;
>>> +}
>>> +
>>> +static HRESULT WINAPI testseek_GetRate(IMediaSeeking *iface, double 
>>> *rate)
>>> +{
>>> +    struct testfilter *filter = impl_from_IMediaSeeking(iface);
>>> +
>>> +    *rate = filter->rate;
>>> +    return S_OK;
>>> +}
>>> +
>>> +static HRESULT WINAPI testseek_GetPreroll(IMediaSeeking *iface, 
>>> LONGLONG *preroll)
>>> +{
>>> +    *preroll = 0;
>>> +    return S_OK;
>>> +}
>>
>> Do you need to implement all of these methods? Does the implementation
>> need to return S_OK, or can it return E_NOTIMPL? Same for IMediaPosition
>> methods below.
>>
>> ...
>>
> 
> I don't know, I considered it an implementation detail. I thought it 
> trivial enough to implement fake values here. Should I really make it 
> return E_NOTIMPL?

It doesn't matter a lot, but returning E_NOTIMPL is generally "easier". 
For example, then you don't have to bother tracking playback rate.

Note also that SetPositions() isn't really correct, it should only 
modify its arguments when passed AM_SEEKING_ReturnTime. (Yes, I know 
that quartz:filtergraph doesn't check for that flag; it probably should.)

> One example of such thing is: IMediaSeeking on the filter (not the pin) 
> is *not* queried by Windows, but we do when we add the filter (because 
> of the patch that caches it which doesn't match Windows 100%).

In my opinion, deciding whether an implementation detail is worth 
replicating or testing depends largely on whether it's easier not to, 
and how likely it is that an application will rely on it later.

For example, my armchair analysis of 0320f165 is that applications 
aren't particularly likely to care when an interface is queried, and 
it's easier for us to just do it once.

By contrast, IMediaDet::get_StreamLength() could potentially work in 
several different ways, which are a roughly equal amount of work to 
implement: does it return the stream length from 
IMediaSeeking::GetDuration(), or GetAvailable(), or maybe even 
GetStopPosition()? Does it actually bother converting to 
TIME_FORMAT_MEDIA_TIME? In most cases these will end up being identical 
in practice, but we don't really gain anything by just assuming one is 
correct, and it's not hard to check.

>>> +static IBaseFilter *create_testfilter(void)
>>> +{
>>> +    struct testfilter *filter = HeapAlloc(GetProcessHeap(), 
>>> HEAP_ZERO_MEMORY, sizeof(*filter));
>>> +
>>> +    if (!filter) return NULL;
>>
>> I don't think we need to allocate this from heap. Note though that if
>> you do, we can use malloc() instead, and checking for failure in tests
>> is not really necessary.
>>
>>> +
>>> +    filter->IBaseFilter_iface.lpVtbl = &testfilter_vtbl;
>>> +    filter->IMediaSeeking_iface.lpVtbl = &testseek_vtbl;
>>> +    filter->IMediaPosition_iface.lpVtbl = &testpos_vtbl;
>>> +    filter->IEnumPins_iface.lpVtbl = &testenumpins_vtbl;
>>> +    filter->IPin_iface.lpVtbl = &testpin_vtbl;
>>> +    filter->IEnumMediaTypes_iface.lpVtbl = &testenummt_vtbl;
>>> +    filter->ref = 1;
>>> +    filter->state = State_Stopped;
>>> +    filter->rate = 1.0;
>>> +
>>> +    return &filter->IBaseFilter_iface;
>>> +}
>>> +
>>>   static void test_aggregation(void)
>>>   {
>>>       IMediaDet *detector, *detector2;
>>> @@ -188,13 +1050,17 @@ static BOOL init_tests(void)
>>>   static void test_mediadet(void)
>>>   {
>>>       HRESULT hr;
>>> +    IBaseFilter *filter, *filter2;
>>> +    FILTER_INFO filter_info;
>>>       IMediaDet *pM = NULL;
>>>       BSTR filename = NULL;
>>> +    IFilterGraph *graph;
>>>       LONG nstrms = 0;
>>>       LONG strm;
>>>       GUID guid;
>>>       BSTR bstr;
>>>       AM_MEDIA_TYPE mt;
>>> +    IUnknown *unk;
>>>       double fps;
>>>       int flags;
>>>       int i;
>>> @@ -256,6 +1122,61 @@ static void test_mediadet(void)
>>>       hr = IMediaDet_get_StreamTypeB(pM, NULL);
>>>       ok(hr == E_INVALIDARG, "IMediaDet_get_StreamTypeB failed: 
>>> %08x\n", hr);
>>> +    hr = IMediaDet_get_Filter(pM, NULL);
>>> +    ok(hr == E_POINTER, "IMediaDet_get_Filter failed: %08x\n", hr);
>>> +
>>> +    unk = (IUnknown*)0xdeadbeef;
>>> +    hr = IMediaDet_get_Filter(pM, &unk);
>>> +    ok(hr == S_FALSE, "IMediaDet_get_Filter failed: %08x\n", hr);
>>> +    ok(unk == NULL, "Wrong filter %p\n", unk);
>>> +
>>> +    hr = IMediaDet_put_Filter(pM, NULL);
>>> +    ok(hr == E_POINTER, "IMediaDet_put_Filter failed: %08x\n", hr);
>>> +
>>> +    filter = create_testfilter();
>>> +    hr = IMediaDet_put_Filter(pM, (IUnknown*)filter);
>>> +    ok(hr == S_OK, "IMediaDet_put_Filter failed: %08x\n", hr);
>>> +
>>> +    hr = IMediaDet_get_Filter(pM, &unk);
>>> +    ok(hr == S_OK, "IMediaDet_get_Filter failed: %08x\n", hr);
>>> +    ok(unk != NULL, "NULL filter\n");
>>> +    hr = IUnknown_QueryInterface(unk, &IID_IBaseFilter, 
>>> (void**)&filter2);
>>> +    IUnknown_Release(unk);
>>> +    ok(hr == S_OK, "Could not get IBaseFilter interface: %08x\n", hr);
>>> +    ok(filter == filter2, "Wrong filter\n");
>>> +    IBaseFilter_Release(filter2);
>>
>> Couldn't you compare it directly to filter->IBaseFiler_iface instead?
>>
> 
> That would defeat the purpose of testing get_Filter, wouldn't it? Since 
> we'd assume it is our filter in the first place (and if it's not, we 
> would end up using random data from the object, which might succeed 
> randomly and IMO is not a "correct" solution by any means).
> 
> What I'm trying to say is that there's no guarantee that get_Filter 
> returns a "struct testfilter", hence the test. We can assume it is, but 
> we shouldn't rely on the exact struct layout while *testing* that since 
> it can access invalid data or memory otherwise. I'm really not 
> comfortable with that.

Eh, sorry, I think I misread the query here.

>>> +
>>> +    hr = IBaseFilter_QueryFilterInfo(filter, &filter_info);
>>> +    ok(hr == S_OK, "IBaseFilter_QueryFilterInfo failed: %08x\n", hr);
>>> +    ok(!wcscmp(filter_info.achName, L"Source"), "Wrong filter name: 
>>> %s\n", wine_dbgstr_w(filter_info.achName));
>>> +    IBaseFilter_Release(filter);
>>> +    graph = filter_info.pGraph;
>>
>> Similarly, you could just access filter->name directly instead of
>> calling IBaseFilter::QueryFilterInfo().
>>
> 
> Same as above.

You're querying "filter", which is our own filter. Yes, "filter" is an 
IBaseFilter pointer rather than a "struct testfilter" pointer, but it 
doesn't really need to be.

>>> +
>>> +    filter = create_testfilter();
>>> +    hr = IMediaDet_put_Filter(pM, (IUnknown*)filter);
>>> +    ok(hr == S_OK, "IMediaDet_put_Filter failed: %08x\n", hr);
>>> +    IBaseFilter_Release(filter);
>>> +
>>> +    hr = IMediaDet_get_Filter(pM, &unk);
>>> +    ok(hr == S_OK, "IMediaDet_get_Filter failed: %08x\n", hr);
>>> +    ok(unk != NULL, "NULL filter\n");
>>> +    hr = IUnknown_QueryInterface(unk, &IID_IBaseFilter, 
>>> (void**)&filter);
>>> +    IUnknown_Release(unk);
>>> +    ok(hr == S_OK, "Could not get IBaseFilter interface: %08x\n", hr);
>>> +
>>> +    hr = IBaseFilter_QueryFilterInfo(filter, &filter_info);
>>> +    ok(hr == S_OK, "IBaseFilter_QueryFilterInfo failed: %08x\n", hr);
>>> +    ok(!wcscmp(filter_info.achName, L"Source"), "Wrong filter name: 
>>> %s\n", wine_dbgstr_w(filter_info.achName));
>>> +    ok(graph != filter_info.pGraph, "Same filter graph was used\n");
>>> +    IFilterGraph_Release(filter_info.pGraph);
>>> +    IFilterGraph_Release(graph);
>>> +    IBaseFilter_Release(filter);
>>> +
>>> +    strm = -1;
>>> +    hr = IMediaDet_get_CurrentStream(pM, &strm);
>>> +    ok(hr == S_OK, "IMediaDet_get_CurrentStream failed: %08x\n", hr);
>>> +    ok(strm == 0, "IMediaDet_get_CurrentStream: strm is %i\n", strm);
>>> +
>>>       filename = SysAllocString(test_avi_filename);
>>>       hr = IMediaDet_put_Filename(pM, filename);
>>>       ok(hr == S_OK, "IMediaDet_put_Filename failed: %08x\n", hr);
>>> @@ -427,6 +1348,50 @@ static void test_mediadet(void)
>>>       ok(hr == S_OK, "IMediaDet_get_CurrentStream failed: %08x\n", hr);
>>>       ok(strm == 1, "IMediaDet_get_CurrentStream: strm is %i\n", strm);
>>> +    hr = IMediaDet_get_Filter(pM, &unk);
>>> +    ok(hr == S_OK, "IMediaDet_get_Filter failed: %08x\n", hr);
>>> +    ok(unk != NULL, "NULL filter\n");
>>> +    hr = IUnknown_QueryInterface(unk, &IID_IBaseFilter, 
>>> (void**)&filter2);
>>> +    IUnknown_Release(unk);
>>> +    ok(hr == S_OK, "Could not get IBaseFilter interface: %08x\n", hr);
>>> +
>>> +    hr = IBaseFilter_QueryFilterInfo(filter2, &filter_info);
>>> +    ok(hr == S_OK, "IBaseFilter_QueryFilterInfo failed: %08x\n", hr);
>>> +    ok(!wcscmp(filter_info.achName, L"Source"), "Wrong filter name: 
>>> %s\n", wine_dbgstr_w(filter_info.achName));
>>> +    graph = filter_info.pGraph;
>>> +
>>> +    filter = create_testfilter();
>>> +    hr = IMediaDet_put_Filter(pM, (IUnknown*)filter);
>>> +    ok(hr == S_OK, "IMediaDet_put_Filter failed: %08x\n", hr);
>>> +    IBaseFilter_Release(filter);
>>> +
>>> +    hr = IMediaDet_get_Filter(pM, &unk);
>>> +    ok(hr == S_OK, "IMediaDet_get_Filter failed: %08x\n", hr);
>>> +    ok(unk != NULL, "NULL filter\n");
>>> +    hr = IUnknown_QueryInterface(unk, &IID_IBaseFilter, 
>>> (void**)&filter);
>>> +    IUnknown_Release(unk);
>>> +    ok(hr == S_OK, "Could not get IBaseFilter interface: %08x\n", hr);
>>> +    ok(filter != filter2, "Same filter\n");
>>> +    IBaseFilter_Release(filter2);
>>> +
>>> +    hr = IBaseFilter_QueryFilterInfo(filter, &filter_info);
>>> +    ok(hr == S_OK, "IBaseFilter_QueryFilterInfo failed: %08x\n", hr);
>>> +    ok(!wcscmp(filter_info.achName, L"Source"), "Wrong filter name: 
>>> %s\n", wine_dbgstr_w(filter_info.achName));
>>> +    ok(graph != filter_info.pGraph, "Same filter graph was used\n");
>>> +    IFilterGraph_Release(filter_info.pGraph);
>>> +    IFilterGraph_Release(graph);
>>> +    IBaseFilter_Release(filter);
>>> +
>>> +    filename = NULL;
>>> +    hr = IMediaDet_get_Filename(pM, &filename);
>>> +    ok(hr == S_OK, "IMediaDet_get_Filename failed: %08x\n", hr);
>>> +    ok(!filename, "Expected NULL filename, got %s.\n", 
>>> debugstr_w(filename));
>>> +    SysFreeString(filename);
>>> +
>>> +    hr = IMediaDet_get_OutputStreams(pM, &nstrms);
>>> +    ok(hr == S_OK, "IMediaDet_get_OutputStreams failed: %08x\n", hr);
>>> +    ok(nstrms == 1, "IMediaDet_get_OutputStreams: nstrms is %i\n", 
>>> nstrms);
>>> +
>>>       hr = IMediaDet_Release(pM);
>>>       ok(hr == 0, "IMediaDet_Release returned: %x\n", hr);
>>>
>>
>