[PATCH v2 12/13] qedit/tests: Add tests for get_Filter and put_Filter with a custom filter.
Gabriel Ivăncescu
gabrielopcode at gmail.com
Fri Apr 17 07:26:27 CDT 2020
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.
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)
>> +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?
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%).
>> +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.
>> +
>> + 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.
>> +
>> + 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);
>>
>>
>
More information about the wine-devel
mailing list