On 10/21/20 8:07 AM, Gabriel Ivăncescu wrote:
On 20/10/2020 22:56, Zebediah Figura wrote:
On 10/20/20 12:05 PM, Gabriel Ivăncescu wrote:
Thanks for the review.
On 20/10/2020 19:46, Zebediah Figura wrote:
On 10/19/20 11:48 AM, Gabriel Ivăncescu wrote:
We fill the video pattern with something that varies between lines and columns, to test it properly later (including the scaling algorithm).
Signed-off-by: Gabriel Ivăncescu gabrielopcode@gmail.com
dlls/qedit/tests/mediadet.c | 337 +++++++++++++++++++++++++++++++++++- 1 file changed, 333 insertions(+), 4 deletions(-)
diff --git a/dlls/qedit/tests/mediadet.c b/dlls/qedit/tests/mediadet.c index dc83bb9..010b746 100644 --- a/dlls/qedit/tests/mediadet.c +++ b/dlls/qedit/tests/mediadet.c @@ -136,6 +136,11 @@ struct testfilter struct strmbase_filter filter; struct strmbase_source source; IMediaSeeking IMediaSeeking_iface;
BOOL bitmap_grab_mode;
const GUID *time_format;
LONGLONG cur_pos;
HANDLE thread; };
static inline struct testfilter *impl_from_strmbase_filter(struct strmbase_filter *iface)
@@ -158,10 +163,103 @@ static void testfilter_destroy(struct strmbase_filter *iface) strmbase_filter_cleanup(&filter->filter); }
+static DWORD WINAPI testfilter_frame_thread(void *arg) +{
- REFERENCE_TIME start_time, end_time;
- struct testfilter *filter = arg;
- IMemAllocator *allocator;
- IMediaSample *sample;
- unsigned i;
- HRESULT hr;
- DWORD fill;
- BYTE *data;
- hr = IMemInputPin_GetAllocator(filter->source.pMemInputPin, &allocator);
- ok(hr == S_OK, "Got hr %#x.\n", hr);
- start_time = (filter->cur_pos == 0xdeadbeef) ? 0 : filter->cur_pos;
- while (hr == S_OK)
- {
hr = IMemAllocator_GetBuffer(allocator, &sample, NULL, NULL, 0);if (hr == VFW_E_NOT_COMMITTED){IMemAllocator_Commit(allocator);hr = IMemAllocator_GetBuffer(allocator, &sample, NULL, NULL, 0);}ok(hr == S_OK, "Got hr %#x.\n", hr);hr = IMediaSample_GetPointer(sample, &data);ok(hr == S_OK, "Got hr %#x.\n", hr);fill = (start_time / 10000 & 0xffffff) ^ 0xccaabb;for (i = 0; i < 640 * 480 * 3; i += 3){data[i] = fill ^ i;data[i + 1] = fill >> 8 ^ i;data[i + 2] = fill >> 16 ^ i;}hr = IMediaSample_SetActualDataLength(sample, 640 * 480 * 3);ok(hr == S_OK, "Got hr %#x.\n", hr);end_time = start_time + 400000;hr = IMediaSample_SetTime(sample, &start_time, &end_time);ok(hr == S_OK, "Got hr %#x.\n", hr);start_time = end_time;if (winetest_debug > 1) trace("%04x: Sending frame.\n", GetCurrentThreadId());hr = IMemInputPin_Receive(filter->source.pMemInputPin, sample);if (winetest_debug > 1) trace("%04x: Returned %#x.\n", GetCurrentThreadId(), hr);IMediaSample_Release(sample);- }
- IMemAllocator_Release(allocator);
- return hr;
+}
Why spawn a thread that loops like this, instead of sending individual frames? In particular, the latter obviates the need to manually flush the stream when seeking.
I can see value in trying to emulate a realistic filter, but it seems easier to me to just render a test AVI file and let the media detector insert a built-in one. A separate test that exercises the functions as a program would actually use them seems quite welcome, in fact.
Actually I uncovered several self-made bugs while implementing it (as I was not very familiar with quartz) which were apparent mostly because of implementing this like a realistic filter. So I think it's useful.
For example this implicitly tests the condition that the media detector stops the stream before seeking, else the sample grabber would still hold the previous sample and have a race condition.
That seems suspicious; I'd expect that a flush should be sufficient to achieve that.
It's possible there's a bug in the Sample Grabber, because it doesn't handle flushing at all, and I don't know if it should. But as I mentioned in the other reply to the EnterBitmapGrabMode implementation, this uncovers the bug and Windows does seem to put a Stop() when seeking (if I trace +strmbase on the test).
If Windows does fully stop the filter, then that's fine, but that should also be tested if possible, probably by checking "filter->state" when seeking.
Rendering to a test AVI file doesn't seem that ideal, unless you mean rendering it within the test itself? But I thought, from last patches I sent about qedit, that the focus was to use custom filters as much as possible to be able to test them better, and AVI files were just needed for put_Filename only.
quartz is complicated, not just because it uses a complicated system of callbacks, but also because it has a *lot* of moving parts, most of which can be system components but also can be application components. Hence one of the things I try to do, when writing quartz tests, is to try to test every single moving piece, one at a time. I try to validate every *non-obvious* implementation detail that I reasonably can, mostly with the exception of exhaustively testing error handling (e.g. in general I don't think it's worth testing whether the code checks for S_OK or SUCCEEDED(), especially if a potential failure can be flagged by an ERR/WARN message anyway) where functions are "not supposed to" fail.
When doing this kind of testing, I want to isolate the causes of a return value—sometimes if only for clarity—and often this means using our own custom filters instead of system ones. For example, it makes it clearer that the video renderer is the one responsible for holding up the graph during preroll if we don't have any *other* system filters in the graph at the time.
These tests are focused mostly on proving the implementation correct, though of course they help to catch regressions as well. At the same time, using custom filters also often means testing real-world behaviour. It's not rare for applications to insert their own filters at any point in the pipeline (source, sink, transform, parser).
On the other hand, there's also some value in testing very high-level usage of the quartz API. That's not just because we need to test high-level functions (like IGraphBuilder::RenderFile()) but also because, as you've discovered, it's easy to miss important details about how parts work together. These tests are more regression tests than conformance tests. Most of these are in rungraph() [in quartz:filtergraph] and its callers.
Back to the more concrete case: Certainly I'm not trying to advocate that we remove any of the testfilter infrastructure this patch adds. I am suggesting that we instead send frames one at a time instead of using such a loop. Although frankly, after looking at the later patches in the series I'm not even necessarily sure that's better. [I will note, though, that it would be nice to test the frames returned from GetBitmapBits() a little closer to where the test loop is introduced, if not in the same patch, so that I have that context immediately.] It may also make sense to send just one frame instead of looping, just for a little extra simplicity.
Thanks for the information :-)
I think the loop is needed to catch regressions as noticed. I should probably keep the loop in the first patch, but only send black pixels all the time, until the GetBitmapData tests, where I will convert it to what we have now with the varying pattern, so it matches the verification.
Would that be acceptable as a compromise?
My point is largely that the kind of regressions you're talking about would probably have been detectable with the second, separate test I was recommending.
Note that one problem with keeping the tests as they are now is that there's not a good way to check what happens if you call GetBitmapBits() before a sample has reached the sample grabber. This is more than a little important, as it's quite possible that either the media detector or the sample grabber is supposed to wait.
Separately, though, I think it would be a good idea to also add some tests that exercise the media detector's bitmap grab mode as an application would actually use it—e.g. call put_Filename() with a test AVI file (or whatever), and then GetBitmapBits(). Not actually checking the contents of the buffer, but making sure it succeeds, and doesn't crash or hang.
Noted.
BTW I also made a few changes to the SetPositions (with stop and stop_flags) to test them better under the specific scenarios they are supposed to be (or not be) zero.
+static HRESULT testfilter_init_stream(struct strmbase_filter *iface) +{
- struct testfilter *filter = impl_from_strmbase_filter(iface);
- HRESULT hr;
- if (!filter->bitmap_grab_mode) return S_OK;
- hr = BaseOutputPinImpl_Active(&filter->source);
- ok(hr == S_OK, "Got hr %#x.\n", hr);
- filter->thread = CreateThread(NULL, 0, testfilter_frame_thread, filter, 0, NULL);
- ok(filter->thread != NULL, "Failed to create thread: %#x.\n", GetLastError());
- return S_OK;
+}
+static HRESULT testfilter_cleanup_stream(struct strmbase_filter *iface) +{
- struct testfilter *filter = impl_from_strmbase_filter(iface);
- HRESULT hr;
- if (filter->thread)
- {
WaitForSingleObject(filter->thread, INFINITE);CloseHandle(filter->thread);filter->thread = NULL;- }
- if (!filter->bitmap_grab_mode)
return S_OK;- hr = BaseOutputPinImpl_Inactive(&filter->source);
- ok(hr == S_OK, "Got hr %#x.\n", hr);
- return S_OK;
+}
static const struct strmbase_filter_ops testfilter_ops = { .filter_get_pin = testfilter_get_pin, .filter_destroy = testfilter_destroy,
.filter_init_stream = testfilter_init_stream,
.filter_cleanup_stream = testfilter_cleanup_stream };
static inline struct testfilter *impl_from_strmbase_pin(struct strmbase_pin *iface)
@@ -175,7 +273,7 @@ static HRESULT testsource_get_media_type(struct strmbase_pin *iface, unsigned in { .bmiHeader.biSize = sizeof(BITMAPINFOHEADER), .bmiHeader.biWidth = 640,
.bmiHeader.biHeight = 480,
.bmiHeader.biHeight = -480, .bmiHeader.biPlanes = 1, .bmiHeader.biBitCount = 24, .bmiHeader.biCompression = BI_RGB,@@ -211,10 +309,37 @@ static HRESULT testsource_query_interface(struct strmbase_pin *iface, REFIID iid return S_OK; }
+static HRESULT WINAPI testsource_DecideBufferSize(struct strmbase_source *iface,
IMemAllocator *allocator, ALLOCATOR_PROPERTIES *requested)+{
- ALLOCATOR_PROPERTIES actual;
- if (!requested->cbAlign)
requested->cbAlign = 1;- if (requested->cbBuffer < 640 * 480 * 3)
requested->cbBuffer = 640 * 480 * 3;- if (!requested->cBuffers)
requested->cBuffers = 1;- return IMemAllocator_SetProperties(allocator, requested, &actual);
+}
- static HRESULT WINAPI testsource_DecideAllocator(struct strmbase_source *iface, IMemInputPin *peer, IMemAllocator **allocator) {
- return S_OK;
ALLOCATOR_PROPERTIES props = {0};
HRESULT hr;
hr = BaseOutputPinImpl_InitAllocator(iface, allocator);
ok(hr == S_OK, "Got hr %#x.\n", hr);
IMemInputPin_GetAllocatorRequirements(peer, &props);
hr = testsource_DecideBufferSize(iface, *allocator, &props);
ok(hr == S_OK, "Got hr %#x.\n", hr);
return IMemInputPin_NotifyAllocator(peer, *allocator, FALSE); }
static const struct strmbase_source_ops testsource_ops =
@@ -222,6 +347,7 @@ static const struct strmbase_source_ops testsource_ops = .base.pin_get_media_type = testsource_get_media_type, .base.pin_query_interface = testsource_query_interface, .pfnAttemptConnection = BaseOutputPinImpl_AttemptConnection,
- .pfnDecideBufferSize = testsource_DecideBufferSize, .pfnDecideAllocator = testsource_DecideAllocator, };
@@ -250,6 +376,15 @@ static ULONG WINAPI testseek_Release(IMediaSeeking *iface)
static HRESULT WINAPI testseek_GetCapabilities(IMediaSeeking *iface, DWORD *caps) {
- struct testfilter *filter = impl_from_IMediaSeeking(iface);
- if (filter->bitmap_grab_mode)
- {
if (winetest_debug > 1) trace("IMediaSeeking_GetCapabilities()\n");*caps = 0; /* Doesn't seem to have any effect, despite being called */I don't know that this comment is especially interesting; I can kind of draw that conclusion from the 0 return.
return S_OK;- }
ok(0, "Unexpected call.\n");You could, I think, simplify this (and other functions) by getting rid of the condition and using "ok(filter->bitmap_grab_mode, ...)".
return E_NOTIMPL;} @@ -262,6 +397,15 @@ static HRESULT WINAPI testseek_CheckCapabilities(IMediaSeeking *iface, DWORD *ca
static HRESULT WINAPI testseek_IsFormatSupported(IMediaSeeking *iface, const GUID *format) {
- struct testfilter *filter = impl_from_IMediaSeeking(iface);
- if (filter->bitmap_grab_mode)
- {
if (winetest_debug > 1) trace("IMediaSeeking_IsFormatSupported(%s)\n", wine_dbgstr_guid(format));ok(IsEqualGUID(format, &TIME_FORMAT_MEDIA_TIME), "Unexpected format %s.\n", wine_dbgstr_guid(format));return S_OK;- }
ok(0, "Unexpected call.\n"); return E_NOTIMPL;@@ -274,12 +418,29 @@ static HRESULT WINAPI testseek_QueryPreferredFormat(IMediaSeeking *iface, GUID *
static HRESULT WINAPI testseek_GetTimeFormat(IMediaSeeking *iface, GUID *format) {
struct testfilter *filter = impl_from_IMediaSeeking(iface);
if (filter->bitmap_grab_mode)
{
if (winetest_debug > 1) trace("IMediaSeeking_GetTimeFormat()\n");*format = *filter->time_format;return S_OK;}
ok(0, "Unexpected call.\n"); return E_NOTIMPL;}
static HRESULT WINAPI testseek_IsUsingTimeFormat(IMediaSeeking *iface, const GUID *format) {
struct testfilter *filter = impl_from_IMediaSeeking(iface);
if (filter->bitmap_grab_mode)
{
if (winetest_debug > 1) trace("IMediaSeeking_IsUsingTimeFormat(%s)\n", wine_dbgstr_guid(format));return IsEqualGUID(format, filter->time_format) ? S_OK : S_FALSE;}
ok(0, "Unexpected call.\n"); return E_NOTIMPL;}
@@ -320,8 +481,35 @@ static HRESULT WINAPI testseek_ConvertTimeFormat(IMediaSeeking *iface, LONGLONG static HRESULT WINAPI testseek_SetPositions(IMediaSeeking *iface, LONGLONG *current, DWORD current_flags, LONGLONG *stop, DWORD stop_flags) {
- ok(0, "Unexpected call.\n");
- return E_NOTIMPL;
- struct testfilter *filter = impl_from_IMediaSeeking(iface);
- if (winetest_debug > 1)
trace("IMediaSeeking_SetPositions(0x%s, 0x%08x, 0x%s, 0x%08x)\n",wine_dbgstr_longlong(*current), current_flags, wine_dbgstr_longlong(*stop), stop_flags);- if (filter->bitmap_grab_mode)
- {
ok(*stop == *current || !*stop, "Unexpected stop position: 0x%s.\n", wine_dbgstr_longlong(*stop));ok(current_flags == (AM_SEEKING_AbsolutePositioning | AM_SEEKING_ReturnTime),"Unexpected current_flags 0x%08x.\n", current_flags);ok(stop_flags == AM_SEEKING_AbsolutePositioning || !stop_flags, "Unexpected stop_flags 0x%08x.\n", stop_flags);When are "*stop" and "stop_flags" nonzero?
*stop is the same as *current when we seek with the bitmap grab mode. It's zero (same with stop_flags) when the filter is destroyed / rewound.
Also, please trace (32-bit) hexadecimal numbers with "%#x".
if (filter->thread){IPin_BeginFlush(filter->source.pin.peer);WaitForSingleObject(filter->thread, INFINITE);CloseHandle(filter->thread);filter->cur_pos = *current;IPin_EndFlush(filter->source.pin.peer);filter->thread = CreateThread(NULL, 0, testfilter_frame_thread, filter, 0, NULL);ok(filter->thread != NULL, "Failed to create thread: %#x.\n", GetLastError());}elsefilter->cur_pos = *current;}
return S_OK; }
static HRESULT WINAPI testseek_GetPositions(IMediaSeeking *iface, LONGLONG *current, LONGLONG *stop)
@@ -386,6 +574,8 @@ static void testfilter_init(struct testfilter *filter) strmbase_filter_init(&filter->filter, NULL, &clsid, &testfilter_ops); strmbase_source_init(&filter->source, &filter->filter, L"", &testsource_ops); filter->IMediaSeeking_iface.lpVtbl = &testseek_vtbl;
filter->cur_pos = 0xdeadbeef;
filter->time_format = &TIME_FORMAT_MEDIA_TIME; }
static WCHAR test_avi_filename[MAX_PATH];
@@ -1117,6 +1307,144 @@ static void test_COM_sg_enumpins(void) IBaseFilter_Release(bf); }
+static void test_bitmap_grab_mode(void) +{
- static const GUID *time_formats[] =
"static const GUID *const time_formats[]"
- {
&TIME_FORMAT_NONE,&TIME_FORMAT_FRAME,&TIME_FORMAT_SAMPLE,&TIME_FORMAT_FIELD,&TIME_FORMAT_BYTE,&TIME_FORMAT_MEDIA_TIME- };
- struct testfilter testfilter;
- IMediaDet *detector;
- AM_MEDIA_TYPE mt;
- double duration;
- IUnknown *unk;
- unsigned i;
- HRESULT hr;
- LONG count;
- ULONG ref;
- GUID guid;
- BSTR str;
- hr = CoCreateInstance(&CLSID_MediaDet, NULL, CLSCTX_INPROC_SERVER,
&IID_IMediaDet, (void **)&detector);- ok(hr == S_OK, "Got hr %#x.\n", hr);
- hr = IMediaDet_EnterBitmapGrabMode(detector, 0.0);
- todo_wine ok(hr == E_INVALIDARG, "Got hr %#x.\n", hr);
- /* EnterBitmapGrabMode only seeks once, and if SeekTime is non-negative */
- testfilter_init(&testfilter);
- testfilter.bitmap_grab_mode = TRUE;
- hr = IMediaDet_put_Filter(detector, &testfilter.filter.IUnknown_inner);
- ok(hr == S_OK, "Got hr %#x.\n", hr);
- hr = IMediaDet_EnterBitmapGrabMode(detector, -1.0);
- todo_wine ok(hr == S_OK, "Got hr %#x.\n", hr);
- ok(testfilter.cur_pos == 0xdeadbeef, "Current position was set to 0x%s.\n", wine_dbgstr_longlong(testfilter.cur_pos));
- hr = IMediaDet_EnterBitmapGrabMode(detector, 1.0);
- todo_wine ok(hr == S_OK, "Got hr %#x.\n", hr);
- ok(testfilter.cur_pos == 0xdeadbeef, "Current position was set to 0x%s.\n", wine_dbgstr_longlong(testfilter.cur_pos));
- ref = IMediaDet_Release(detector);
- ok(!ref, "Got outstanding refcount %d.\n", ref);
- ref = IBaseFilter_Release(&testfilter.filter.IBaseFilter_iface);
- ok(!ref, "Got outstanding refcount %d.\n", ref);
- /* Time formats other than TIME_FORMAT_MEDIA_TIME return E_NOTIMPL */
- for (i = 0; i < ARRAY_SIZE(time_formats); i++)
- {
hr = CoCreateInstance(&CLSID_MediaDet, NULL, CLSCTX_INPROC_SERVER,&IID_IMediaDet, (void **)&detector);ok(hr == S_OK, "Got hr %#x.\n", hr);testfilter_init(&testfilter);testfilter.bitmap_grab_mode = TRUE;testfilter.time_format = time_formats[i];hr = IMediaDet_put_Filter(detector, &testfilter.filter.IUnknown_inner);ok(hr == S_OK, "Got hr %#x.\n", hr);hr = IMediaDet_EnterBitmapGrabMode(detector, 1337.0);if (time_formats[i] == &TIME_FORMAT_MEDIA_TIME){todo_wine ok(hr == S_OK, "Got hr %#x.\n", hr);todo_wine ok(testfilter.cur_pos == 13370000000LL, "Current position was set to 0x%s.\n", wine_dbgstr_longlong(testfilter.cur_pos));hr = IMediaDet_EnterBitmapGrabMode(detector, 1.0);todo_wine ok(hr == S_OK, "Got hr %#x.\n", hr);todo_wine ok(testfilter.cur_pos == 13370000000LL, "Current position was set to 0x%s.\n", wine_dbgstr_longlong(testfilter.cur_pos));}elseok(hr == E_NOTIMPL, "Got hr %#x.\n", hr);ref = IMediaDet_Release(detector);ok(!ref, "Got outstanding refcount %d.\n", ref);ref = IBaseFilter_Release(&testfilter.filter.IBaseFilter_iface);ok(!ref, "Got outstanding refcount %d.\n", ref);- }
- hr = CoCreateInstance(&CLSID_MediaDet, NULL, CLSCTX_INPROC_SERVER,
&IID_IMediaDet, (void **)&detector);- ok(hr == S_OK, "Got hr %#x.\n", hr);
- testfilter_init(&testfilter);
- testfilter.bitmap_grab_mode = TRUE;
- hr = IMediaDet_put_Filter(detector, &testfilter.filter.IUnknown_inner);
- ok(hr == S_OK, "Got hr %#x.\n", hr);
- hr = IMediaDet_EnterBitmapGrabMode(detector, 0.0);
- todo_wine ok(hr == S_OK, "Got hr %#x.\n", hr);
- todo_wine ok(testfilter.cur_pos == 0, "Current position was set to 0x%s.\n", wine_dbgstr_longlong(testfilter.cur_pos));
- hr = IMediaDet_EnterBitmapGrabMode(detector, 1.0);
- todo_wine ok(hr == S_OK, "Got hr %#x.\n", hr);
- todo_wine ok(testfilter.cur_pos == 0, "Current position was set to 0x%s.\n", wine_dbgstr_longlong(testfilter.cur_pos));
- /* These still work */
- hr = IMediaDet_get_Filter(detector, &unk);
- ok(hr == S_OK, "Got hr %#x.\n", hr);
- IUnknown_Release(unk);
- hr = IMediaDet_get_Filename(detector, &str);
- ok(hr == S_OK, "Got hr %#x.\n", hr);
- SysFreeString(str);
- hr = IMediaDet_get_CurrentStream(detector, &count);
- ok(hr == S_OK, "Got hr %#x.\n", hr);
- ok(count == 0, "Got stream %d.\n", count);
- /* These don't work anymore */
- hr = IMediaDet_get_OutputStreams(detector, &count);
- todo_wine ok(hr == E_INVALIDARG, "Got hr %#x.\n", hr);
- hr = IMediaDet_get_FrameRate(detector, &duration);
- todo_wine ok(hr == E_INVALIDARG, "Got hr %#x.\n", hr);
- hr = IMediaDet_get_StreamLength(detector, &duration);
- todo_wine ok(hr == E_INVALIDARG, "Got hr %#x.\n", hr);
- hr = IMediaDet_get_StreamMediaType(detector, &mt);
- todo_wine ok(hr == E_INVALIDARG, "Got hr %#x.\n", hr);
- if (SUCCEEDED(hr)) FreeMediaType(&mt);
- hr = IMediaDet_get_StreamType(detector, &guid);
- todo_wine ok(hr == E_INVALIDARG, "Got hr %#x.\n", hr);
- hr = IMediaDet_get_StreamTypeB(detector, &str);
- todo_wine ok(hr == E_INVALIDARG, "Got hr %#x.\n", hr);
- if (SUCCEEDED(hr)) SysFreeString(str);
- hr = IMediaDet_put_CurrentStream(detector, 0);
- todo_wine ok(hr == E_INVALIDARG, "Got hr %#x.\n", hr);
- /* Changing filter resets bitmap grab mode */
- testfilter.bitmap_grab_mode = FALSE;
- hr = IMediaDet_put_Filter(detector, &testfilter.filter.IUnknown_inner);
- ok(hr == S_OK, "Got hr %#x.\n", hr);
- hr = IMediaDet_get_OutputStreams(detector, &count);
- ok(hr == S_OK, "Got hr %#x.\n", hr);
- ok(count == 1, "Got %d streams.\n", count);
- ref = IMediaDet_Release(detector);
- ok(!ref, "Got outstanding refcount %d.\n", ref);
- ref = IBaseFilter_Release(&testfilter.filter.IBaseFilter_iface);
- ok(!ref, "Got outstanding refcount %d.\n", ref);
+}
- START_TEST(mediadet) { IMediaDet *detector;
@@ -1145,6 +1473,7 @@ START_TEST(mediadet) test_put_filter(); test_samplegrabber(); test_COM_sg_enumpins();
test_bitmap_grab_mode();
ret = DeleteFileW(test_avi_filename); ok(ret, "Failed to delete file, error %u.\n", GetLastError());
I'll do all the other changes, but I believe the custom filter is still very useful (also to test the stretching) unless I misunderstood something with "rendering to AVI file".
Thanks, Gabriel