[PATCH 1/2] qedit/tests: Check the Sample Grabber's buffer during and after flushing.
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/qedit/tests/samplegrabber.c | 234 +++++++++++++++++++++++++++++++ 1 file changed, 234 insertions(+) diff --git a/dlls/qedit/tests/samplegrabber.c b/dlls/qedit/tests/samplegrabber.c index 8e68e3e..e66180f 100644 --- a/dlls/qedit/tests/samplegrabber.c +++ b/dlls/qedit/tests/samplegrabber.c @@ -1036,6 +1036,239 @@ static void test_connect_pin(void) ok(!ref, "Got outstanding refcount %d.\n", ref); } +struct frame_thread_params +{ + IMemInputPin *sink; + IMediaSample *sample; + DWORD delay; +}; + +static DWORD WINAPI frame_thread(void *arg) +{ + struct frame_thread_params *params = arg; + HRESULT hr; + + if (params->delay) + { + if (winetest_debug > 1) trace("%04x: Sleeping %u ms.\n", GetCurrentThreadId(), params->delay); + Sleep(params->delay); + } + if (winetest_debug > 1) trace("%04x: Sending frame.\n", GetCurrentThreadId()); + hr = IMemInputPin_Receive(params->sink, params->sample); + if (winetest_debug > 1) trace("%04x: Returned %#x.\n", GetCurrentThreadId(), hr); + IMediaSample_Release(params->sample); + free(params); + return hr; +} + +static HANDLE send_frame_time(IMemInputPin *sink, REFERENCE_TIME start_time, unsigned char color, DWORD delay) +{ + struct frame_thread_params *params = malloc(sizeof(*params)); + IMemAllocator *allocator; + REFERENCE_TIME end_time; + IMediaSample *sample; + HANDLE thread; + HRESULT hr; + BYTE *data; + + hr = IMemInputPin_GetAllocator(sink, &allocator); + ok(hr == S_OK, "Got hr %#x.\n", hr); + + 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); + memset(data, 0x55, 32 * 16 * 2); + + hr = IMediaSample_SetActualDataLength(sample, 32 * 16 * 2); + ok(hr == S_OK, "Got hr %#x.\n", hr); + + start_time *= 10000000; + end_time = start_time + 10000000; + hr = IMediaSample_SetTime(sample, &start_time, &end_time); + ok(hr == S_OK, "Got hr %#x.\n", hr); + + params->sink = sink; + params->sample = sample; + params->delay = delay; + thread = CreateThread(NULL, 0, frame_thread, params, 0, NULL); + + IMemAllocator_Release(allocator); + return thread; +} + +static HANDLE send_frame(IMemInputPin *sink, DWORD delay) +{ + return send_frame_time(sink, 0, 0x55, delay); /* purple */ +} + +static HRESULT join_thread_(int line, HANDLE thread) +{ + DWORD ret; + ok_(__FILE__, line)(!WaitForSingleObject(thread, 1000), "Wait failed.\n"); + GetExitCodeThread(thread, &ret); + CloseHandle(thread); + return ret; +} +#define join_thread(a) join_thread_(__LINE__, a) + +static void test_buffer_flush(void) +{ + AM_MEDIA_TYPE req_mt = + { + .majortype = MEDIATYPE_Video, + .subtype = MEDIASUBTYPE_RGB565, + .formattype = FORMAT_VideoInfo, + .bTemporalCompression = TRUE, + }; + LONG buf[(32 * 16 * 2) / sizeof(LONG)], size = sizeof(buf); + IPin *sink, *source, *renderer_pin; + struct testfilter testsource; + ISampleGrabber *grabber; + IMediaControl *control; + IFilterGraph2 *graph; + IMemInputPin *input; + IBaseFilter *filter; + OAFilterState state; + IMediaFilter *mf; + HANDLE thread; + DWORD ticks; + unsigned i; + HRESULT hr; + ULONG ref; + + testfilter_init(&testsource); + CoCreateInstance(&CLSID_FilterGraph, NULL, CLSCTX_INPROC_SERVER, + &IID_IFilterGraph2, (void **)&graph); + CoCreateInstance(&CLSID_NullRenderer, NULL, CLSCTX_INPROC_SERVER, + &IID_IBaseFilter, (void **)&filter); + IFilterGraph2_AddFilter(graph, filter, L"sink"); + IBaseFilter_FindPin(filter, L"In", &renderer_pin); + IBaseFilter_Release(filter); + + filter = create_sample_grabber(); + IFilterGraph2_AddFilter(graph, &testsource.filter.IBaseFilter_iface, L"source"); + IFilterGraph2_AddFilter(graph, filter, L"sample grabber"); + IBaseFilter_FindPin(filter, L"In", &sink); + IBaseFilter_FindPin(filter, L"Out", &source); + IBaseFilter_QueryInterface(filter, &IID_ISampleGrabber, (void **)&grabber); + IFilterGraph2_QueryInterface(graph, &IID_IMediaControl, (void **)&control); + IPin_QueryInterface(sink, &IID_IMemInputPin, (void **)&input); + + ISampleGrabber_SetMediaType(grabber, &req_mt); + ISampleGrabber_SetBufferSamples(grabber, TRUE); + + testsource.sink_mt = &req_mt; + hr = IFilterGraph2_ConnectDirect(graph, &testsource.source.pin.IPin_iface, sink, &req_mt); + ok(hr == S_OK, "Got hr %#x.\n", hr); + hr = IFilterGraph2_ConnectDirect(graph, source, renderer_pin, &req_mt); + ok(hr == S_OK, "Got hr %#x.\n", hr); + IPin_Release(renderer_pin); + + IFilterGraph2_QueryInterface(graph, &IID_IMediaFilter, (void **)&mf); + IMediaFilter_SetSyncSource(mf, NULL); + IMediaFilter_Release(mf); + + hr = IMemInputPin_ReceiveCanBlock(input); + ok(hr == S_OK, "Got hr %#x.\n", hr); + + hr = ISampleGrabber_GetCurrentBuffer(grabber, &size, buf); + ok(hr == VFW_E_WRONG_STATE, "Got hr %#x.\n", hr); + + hr = IMediaControl_Pause(control); + ok(hr == S_FALSE, "Got hr %#x.\n", hr); + hr = IMediaControl_GetState(control, 0, &state); + ok(hr == VFW_S_STATE_INTERMEDIATE, "Got hr %#x.\n", hr); + + thread = send_frame(input, 0); + hr = IMediaControl_GetState(control, 1000, &state); + ok(hr == S_OK, "Got hr %#x.\n", hr); + ok(WaitForSingleObject(thread, 100) == WAIT_TIMEOUT, "Thread should block in Receive().\n"); + + for (i = 0; i < 3; i++) + { + ticks = GetTickCount(); + hr = ISampleGrabber_GetCurrentBuffer(grabber, &size, buf); + ticks = GetTickCount() - ticks; + ok(hr == S_OK, "Got hr %#x.\n", hr); + ok(ticks < 100, "Got %u ticks.\n", ticks); + ok(size == sizeof(buf), "Got size %d.\n", size); + } + + hr = IMediaControl_Stop(control); + ok(hr == S_OK, "Got hr %#x.\n", hr); + hr = join_thread(thread); + ok(hr == S_OK, "Got hr %#x.\n", hr); + hr = IMediaControl_GetState(control, 1000, &state); + ok(hr == S_OK, "Got hr %#x.\n", hr); + ok(state == State_Stopped, "Got state %d.\n", state); + hr = ISampleGrabber_GetCurrentBuffer(grabber, &size, buf); + ok(hr == S_OK, "Got hr %#x.\n", hr); + + hr = IMediaControl_Pause(control); + ok(hr == S_FALSE, "Got hr %#x.\n", hr); + hr = IMediaControl_GetState(control, 0, &state); + ok(hr == VFW_S_STATE_INTERMEDIATE, "Got hr %#x.\n", hr); + + thread = send_frame(input, 0); + hr = IMediaControl_GetState(control, 1000, &state); + ok(hr == S_OK, "Got hr %#x.\n", hr); + ok(WaitForSingleObject(thread, 100) == WAIT_TIMEOUT, "Thread should block in Receive().\n"); + + for (i = 0; i < 3; i++) + { + ticks = GetTickCount(); + hr = ISampleGrabber_GetCurrentBuffer(grabber, &size, buf); + ticks = GetTickCount() - ticks; + ok(hr == S_OK, "Got hr %#x.\n", hr); + ok(ticks < 100, "Got %u ticks.\n", ticks); + ok(size == sizeof(buf), "Got size %d.\n", size); + } + + IPin_BeginFlush(sink); + hr = join_thread(thread); + ok(hr == S_OK, "Got hr %#x.\n", hr); + hr = ISampleGrabber_GetCurrentBuffer(grabber, &size, buf); + ok(hr == S_OK, "Got hr %#x.\n", hr); + IPin_EndFlush(sink); + + hr = IMediaControl_GetState(control, 0, &state); + todo_wine ok(hr == VFW_S_STATE_INTERMEDIATE, "Got hr %#x.\n", hr); + + ticks = GetTickCount(); + thread = send_frame(input, 150); + hr = ISampleGrabber_GetCurrentBuffer(grabber, &size, buf); + ticks = GetTickCount() - ticks; + ok(hr == S_OK, "Got hr %#x.\n", hr); + ok(ticks < 100, "Got %u ticks.\n", ticks); + ok(size == sizeof(buf), "Got size %d.\n", size); + + hr = IMediaControl_GetState(control, 1000, &state); + ok(hr == S_OK, "Got hr %#x.\n", hr); + hr = IMediaControl_Stop(control); + ok(hr == S_OK, "Got hr %#x.\n", hr); + hr = join_thread(thread); + todo_wine ok(hr == S_OK, "Got hr %#x.\n", hr); + + IPin_Release(sink); + IPin_Release(source); + IMemInputPin_Release(input); + IMediaControl_Release(control); + ref = IFilterGraph2_Release(graph); + ok(!ref, "Got outstanding refcount %d.\n", ref); + ISampleGrabber_Release(grabber); + ref = IBaseFilter_Release(filter); + ok(!ref, "Got outstanding refcount %d.\n", ref); + ref = IBaseFilter_Release(&testsource.filter.IBaseFilter_iface); + ok(!ref, "Got outstanding refcount %d.\n", ref); +} + START_TEST(samplegrabber) { IBaseFilter *filter; @@ -1059,6 +1292,7 @@ START_TEST(samplegrabber) test_aggregation(); test_media_types(); test_connect_pin(); + test_buffer_flush(); CoUninitialize(); } -- 2.21.0
GetState must return VFW_S_STATE_INTERMEDIATE after flushing, until a sample is received. Note the re-checks for 'flushing' after the critical section is acquired, to prevent a race condition in which BeginFlush() is called after Receive() is already in middle of operation, but resets the state_event before the Receive() renderer sets it, which would defeat the whole purpose. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/qedit/tests/nullrenderer.c | 2 +- dlls/qedit/tests/samplegrabber.c | 4 ++-- dlls/quartz/tests/videorenderer.c | 2 +- dlls/quartz/tests/vmr7.c | 2 +- dlls/quartz/tests/vmr9.c | 2 +- dlls/strmbase/renderer.c | 17 +++++++++++++++++ 6 files changed, 23 insertions(+), 6 deletions(-) diff --git a/dlls/qedit/tests/nullrenderer.c b/dlls/qedit/tests/nullrenderer.c index 754228d..fe22d2f 100644 --- a/dlls/qedit/tests/nullrenderer.c +++ b/dlls/qedit/tests/nullrenderer.c @@ -763,7 +763,7 @@ static void test_flushing(IPin *pin, IMemInputPin *input, IFilterGraph2 *graph) /* We dropped the sample we were holding, so now we need a new one... */ hr = IMediaControl_GetState(control, 0, &state); - todo_wine ok(hr == VFW_S_STATE_INTERMEDIATE, "Got hr %#x.\n", hr); + ok(hr == VFW_S_STATE_INTERMEDIATE, "Got hr %#x.\n", hr); thread = send_frame(input); ok(WaitForSingleObject(thread, 100) == WAIT_TIMEOUT, "Thread should block in Receive().\n"); diff --git a/dlls/qedit/tests/samplegrabber.c b/dlls/qedit/tests/samplegrabber.c index e66180f..8f318ab 100644 --- a/dlls/qedit/tests/samplegrabber.c +++ b/dlls/qedit/tests/samplegrabber.c @@ -1239,7 +1239,7 @@ static void test_buffer_flush(void) IPin_EndFlush(sink); hr = IMediaControl_GetState(control, 0, &state); - todo_wine ok(hr == VFW_S_STATE_INTERMEDIATE, "Got hr %#x.\n", hr); + ok(hr == VFW_S_STATE_INTERMEDIATE, "Got hr %#x.\n", hr); ticks = GetTickCount(); thread = send_frame(input, 150); @@ -1254,7 +1254,7 @@ static void test_buffer_flush(void) hr = IMediaControl_Stop(control); ok(hr == S_OK, "Got hr %#x.\n", hr); hr = join_thread(thread); - todo_wine ok(hr == S_OK, "Got hr %#x.\n", hr); + ok(hr == S_OK, "Got hr %#x.\n", hr); IPin_Release(sink); IPin_Release(source); diff --git a/dlls/quartz/tests/videorenderer.c b/dlls/quartz/tests/videorenderer.c index 448efe5..4dd7155 100644 --- a/dlls/quartz/tests/videorenderer.c +++ b/dlls/quartz/tests/videorenderer.c @@ -998,7 +998,7 @@ static void test_flushing(IPin *pin, IMemInputPin *input, IFilterGraph2 *graph) /* We dropped the sample we were holding, so now we need a new one... */ hr = IMediaControl_GetState(control, 0, &state); - todo_wine ok(hr == VFW_S_STATE_INTERMEDIATE, "Got hr %#x.\n", hr); + ok(hr == VFW_S_STATE_INTERMEDIATE, "Got hr %#x.\n", hr); thread = send_frame(input); ok(WaitForSingleObject(thread, 100) == WAIT_TIMEOUT, "Thread should block in Receive().\n"); diff --git a/dlls/quartz/tests/vmr7.c b/dlls/quartz/tests/vmr7.c index 39592af..fd101d7 100644 --- a/dlls/quartz/tests/vmr7.c +++ b/dlls/quartz/tests/vmr7.c @@ -1207,7 +1207,7 @@ static void test_flushing(IPin *pin, IMemInputPin *input, IFilterGraph2 *graph) /* We dropped the sample we were holding, so now we need a new one... */ hr = IMediaControl_GetState(control, 0, &state); - todo_wine ok(hr == VFW_S_STATE_INTERMEDIATE, "Got hr %#x.\n", hr); + ok(hr == VFW_S_STATE_INTERMEDIATE, "Got hr %#x.\n", hr); thread = send_frame(input); ok(WaitForSingleObject(thread, 100) == WAIT_TIMEOUT, "Thread should block in Receive().\n"); diff --git a/dlls/quartz/tests/vmr9.c b/dlls/quartz/tests/vmr9.c index df18bd5..ed7ea9e 100644 --- a/dlls/quartz/tests/vmr9.c +++ b/dlls/quartz/tests/vmr9.c @@ -1219,7 +1219,7 @@ static void test_flushing(IPin *pin, IMemInputPin *input, IMediaControl *control /* We dropped the sample we were holding, so now we need a new one... */ hr = IMediaControl_GetState(control, 0, &state); - todo_wine ok(hr == VFW_S_STATE_INTERMEDIATE, "Got hr %#x.\n", hr); + ok(hr == VFW_S_STATE_INTERMEDIATE, "Got hr %#x.\n", hr); thread = send_frame(input); ok(WaitForSingleObject(thread, 100) == WAIT_TIMEOUT, "Thread should block in Receive().\n"); diff --git a/dlls/strmbase/renderer.c b/dlls/strmbase/renderer.c index dde56b7..c497552 100644 --- a/dlls/strmbase/renderer.c +++ b/dlls/strmbase/renderer.c @@ -194,6 +194,12 @@ static HRESULT WINAPI BaseRenderer_Receive(struct strmbase_sink *pin, IMediaSamp EnterCriticalSection(&filter->csRenderLock); + if (filter->sink.flushing) + { + LeaveCriticalSection(&filter->csRenderLock); + return S_OK; + } + if (filter->filter.clock && SUCCEEDED(IMediaSample_GetTime(sample, &start, &stop))) { strmbase_passthrough_update_time(&filter->passthrough, start); @@ -236,6 +242,12 @@ static HRESULT WINAPI BaseRenderer_Receive(struct strmbase_sink *pin, IMediaSamp } EnterCriticalSection(&filter->csRenderLock); + + if (filter->sink.flushing) + { + LeaveCriticalSection(&filter->csRenderLock); + return S_OK; + } } } @@ -298,8 +310,13 @@ static HRESULT sink_begin_flush(struct strmbase_sink *iface) { struct strmbase_renderer *filter = impl_from_IPin(&iface->pin.IPin_iface); + EnterCriticalSection(&filter->csRenderLock); + + ResetEvent(filter->state_event); SetEvent(filter->flush_event); + LeaveCriticalSection(&filter->csRenderLock); + return S_OK; } -- 2.21.0
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=81070 Your paranoid android. === w10pro64_2scr (32 bit report) === quartz: videorenderer.c:2144: Test failed: Got unexpected status 0x480048. === w10pro64_ar (32 bit report) === quartz: videorenderer.c:1098: Test failed: Got hr 0x80004005. === w10pro64_ar (64 bit report) === quartz: videorenderer.c:1098: Test failed: Got hr 0x80004005. === w10pro64_ja (64 bit report) === quartz: videorenderer.c:1088: Test failed: Thread should block in Receive(). videorenderer.c:1108: Test failed: Got hr 0x80004005. === w10pro64_ar (32 bit report) === quartz: vmr7.c:1216: Test failed: Got hr 0x1. === w10pro64_he (32 bit report) === quartz: vmr7.c:2117: Test failed: Got unexpected status 0. vmr7.c:2120: Test failed: Wait timed out. vmr7.c:2128: Test failed: Got unexpected status 0x400040. === w10pro64 (32 bit report) === quartz: vmr9.c:1446: Test failed: Got hr 0x1. === w10pro64_ja (32 bit report) === quartz: vmr9.c:1339: Test failed: Wait failed. vmr9.c:1340: Test failed: Got hr 0x103. === w10pro64_zh_CN (32 bit report) === quartz: vmr9.c:1446: Test failed: Got hr 0x1. === w10pro64 (64 bit report) === quartz: vmr9.c:2345: Test failed: Got unexpected status 0. vmr9.c:2348: Test failed: Wait timed out. vmr9.c:2356: Test failed: Got unexpected status 0x400040.
This patch isn't wrong per se, but it's less pretty than it could be, and exposes some of the still-present problems with renderer locking. Sorry, I'll need more than a little time to review this.
On 02/11/2020 20:11, Zebediah Figura wrote:
This patch isn't wrong per se, but it's less pretty than it could be, and exposes some of the still-present problems with renderer locking. Sorry, I'll need more than a little time to review this.
Understood. Only thing is that I won't be able to send a new version of the media detector patches until then, though, because it depends on it. (that is why I haven't)
On 11/3/20 6:43 AM, Gabriel Ivăncescu wrote:
On 02/11/2020 20:11, Zebediah Figura wrote:
This patch isn't wrong per se, but it's less pretty than it could be, and exposes some of the still-present problems with renderer locking. Sorry, I'll need more than a little time to review this.
Understood. Only thing is that I won't be able to send a new version of the media detector patches until then, though, because it depends on it. (that is why I haven't)
For the sake of transparency, the basic problem is that this patch treats "filter->sink.flushing" as protected by "csRenderLock", when in fact it's not. The lock taken in sink_begin_flush() *does* prevent the race you describe, but it does so in a very non-obvious way. I'm currently working on restructuring some things so that "flushing" is protected by a lock which we can safely take from the streaming thread, but it may take a while (and may not make it in by code freeze).
On 16/11/2020 23:07, Zebediah Figura (she/her) wrote:
On 11/3/20 6:43 AM, Gabriel Ivăncescu wrote:
On 02/11/2020 20:11, Zebediah Figura wrote:
This patch isn't wrong per se, but it's less pretty than it could be, and exposes some of the still-present problems with renderer locking. Sorry, I'll need more than a little time to review this.
Understood. Only thing is that I won't be able to send a new version of the media detector patches until then, though, because it depends on it. (that is why I haven't)
For the sake of transparency, the basic problem is that this patch treats "filter->sink.flushing" as protected by "csRenderLock", when in fact it's not. The lock taken in sink_begin_flush() *does* prevent the race you describe, but it does so in a very non-obvious way. I'm currently working on restructuring some things so that "flushing" is protected by a lock which we can safely take from the streaming thread, but it may take a while (and may not make it in by code freeze).
Hi Zeb, Sorry if this is already done, but I was going through some of my stuff, it seems I still need this patch to have those tests work (and subsequently the other things in qedit). I've no idea how hard this would be to restructure properly, so maybe it's already in progress or done in some way, but if not it's probably just a reminder. Or if there's a better way now to fix it then please let me know.
On 9/26/22 08:06, Gabriel Ivăncescu wrote:
On 16/11/2020 23:07, Zebediah Figura (she/her) wrote:
On 11/3/20 6:43 AM, Gabriel Ivăncescu wrote:
On 02/11/2020 20:11, Zebediah Figura wrote:
This patch isn't wrong per se, but it's less pretty than it could be, and exposes some of the still-present problems with renderer locking. Sorry, I'll need more than a little time to review this.
Understood. Only thing is that I won't be able to send a new version of the media detector patches until then, though, because it depends on it. (that is why I haven't)
For the sake of transparency, the basic problem is that this patch treats "filter->sink.flushing" as protected by "csRenderLock", when in fact it's not. The lock taken in sink_begin_flush() *does* prevent the race you describe, but it does so in a very non-obvious way. I'm currently working on restructuring some things so that "flushing" is protected by a lock which we can safely take from the streaming thread, but it may take a while (and may not make it in by code freeze).
Hi Zeb,
Sorry if this is already done, but I was going through some of my stuff, it seems I still need this patch to have those tests work (and subsequently the other things in qedit). I've no idea how hard this would be to restructure properly, so maybe it's already in progress or done in some way, but if not it's probably just a reminder.
Or if there's a better way now to fix it then please let me know.
Hi Gabriel, Sorry for the lack of response. I haven't had time to actually work on this in a long time. I'd like to get back to it but I don't know when that will happen. I don't remember if I even have in-progress patches, but I think most of the difficulty has been in the design. ἔρρωσο, Zeb
participants (5)
-
Gabriel Ivăncescu -
Marvin -
Zebediah Figura -
Zebediah Figura -
Zebediah Figura (she/her)