[PATCH 0/4] MR5717: Improve dmo_wrapper_sink_Receive.
This reduce the chance of buffer allocating issue for Kirikiri2 games(eg. Super Naughty Maid) when the game is trying to play videos. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5717
From: Ziqing Hui <zhui(a)codeweavers.com> --- dlls/qasf/dmowrapper.c | 61 ++++++++++++++++++++++++------------ dlls/qasf/tests/dmowrapper.c | 3 -- 2 files changed, 41 insertions(+), 23 deletions(-) diff --git a/dlls/qasf/dmowrapper.c b/dlls/qasf/dmowrapper.c index 95e12e860ae..97cf43cbeff 100644 --- a/dlls/qasf/dmowrapper.c +++ b/dlls/qasf/dmowrapper.c @@ -202,12 +202,24 @@ static void dmo_wrapper_sink_disconnect(struct strmbase_sink *iface) IMediaObject_Release(dmo); } -static HRESULT process_output(struct dmo_wrapper *filter, IMediaObject *dmo) +static void release_output_samples(struct dmo_wrapper *filter) +{ + DWORD i; + + for (i = 0; i < filter->source_count; ++i) + { + if (filter->sources[i].buffer.sample) + { + IMediaSample_Release(filter->sources[i].buffer.sample); + filter->sources[i].buffer.sample = NULL; + } + } +} + +static HRESULT get_output_samples(struct dmo_wrapper *filter) { - DMO_OUTPUT_DATA_BUFFER *buffers = filter->buffers; - DWORD status, i; - BOOL more_data; HRESULT hr; + DWORD i; for (i = 0; i < filter->source_count; ++i) { @@ -216,16 +228,27 @@ static HRESULT process_output(struct dmo_wrapper *filter, IMediaObject *dmo) if (FAILED(hr = IMemAllocator_GetBuffer(filter->sources[i].pin.pAllocator, &filter->sources[i].buffer.sample, NULL, NULL, 0))) { - ERR("Failed to get sample, hr %#lx.\n", hr); - goto out; + ERR("Failed to get sample for source %lu, hr %#lx.\n", i, hr); + release_output_samples(filter); + return hr; } - buffers[i].pBuffer = &filter->sources[i].buffer.IMediaBuffer_iface; + filter->buffers[i].pBuffer = &filter->sources[i].buffer.IMediaBuffer_iface; IMediaSample_SetActualDataLength(filter->sources[i].buffer.sample, 0); } else - buffers[i].pBuffer = NULL; + filter->buffers[i].pBuffer = NULL; } + return S_OK; +} + +static HRESULT process_output(struct dmo_wrapper *filter, IMediaObject *dmo) +{ + DMO_OUTPUT_DATA_BUFFER *buffers = filter->buffers; + DWORD status, i; + BOOL more_data; + HRESULT hr; + do { more_data = FALSE; @@ -264,7 +287,8 @@ static HRESULT process_output(struct dmo_wrapper *filter, IMediaObject *dmo) if (FAILED(hr = IMemInputPin_Receive(filter->sources[i].pin.pMemInputPin, sample))) { WARN("Downstream sink returned %#lx.\n", hr); - goto out; + release_output_samples(filter); + return hr; } IMediaSample_SetActualDataLength(sample, 0); } @@ -272,16 +296,7 @@ static HRESULT process_output(struct dmo_wrapper *filter, IMediaObject *dmo) } } while (more_data); -out: - for (i = 0; i < filter->source_count; ++i) - { - if (filter->sources[i].buffer.sample) - { - IMediaSample_Release(filter->sources[i].buffer.sample); - filter->sources[i].buffer.sample = NULL; - } - } - + release_output_samples(filter); return hr; } @@ -307,9 +322,14 @@ static HRESULT WINAPI dmo_wrapper_sink_Receive(struct strmbase_sink *iface, IMed /* Calling Discontinuity() might change the DMO's mind about whether it * has more data to process. The DirectX documentation explicitly * states that we should call ProcessOutput() again in this case. */ + if (FAILED(hr = get_output_samples(filter))) + goto out; process_output(filter, dmo); } + if (FAILED(hr = get_output_samples(filter))) + goto out; + if (IMediaSample_IsSyncPoint(sample) == S_OK) flags |= DMO_INPUT_DATA_BUFFERF_SYNCPOINT; @@ -348,7 +368,8 @@ static HRESULT dmo_wrapper_sink_eos(struct strmbase_sink *iface) if (FAILED(hr = IMediaObject_Discontinuity(dmo, index))) ERR("Discontinuity() failed, hr %#lx.\n", hr); - process_output(filter, dmo); + if (SUCCEEDED(get_output_samples(filter))) + process_output(filter, dmo); if (FAILED(hr = IMediaObject_Flush(dmo))) ERR("Flush() failed, hr %#lx.\n", hr); diff --git a/dlls/qasf/tests/dmowrapper.c b/dlls/qasf/tests/dmowrapper.c index 632b7e2e558..ada45c447b1 100644 --- a/dlls/qasf/tests/dmowrapper.c +++ b/dlls/qasf/tests/dmowrapper.c @@ -1703,9 +1703,7 @@ static void test_sample_processing(IMediaControl *control, IMemInputPin *input, hr = IMemAllocator_Decommit(testsink->sink.pAllocator); ok(hr == S_OK, "Decommit returned %#lx.\n", hr); hr = IMemInputPin_Receive(input, sample); - todo_wine ok(hr == VFW_E_NOT_COMMITTED, "Receive returned %#lx.\n", hr); - todo_wine ok(got_ProcessInput == 0, "Got %u calls to ProcessInput().\n", got_ProcessInput); ok(got_ProcessOutput == 0, "Got %u calls to ProcessOutput().\n", got_ProcessOutput); ok(got_Receive == 0, "Got %u calls to Receive().\n", got_Receive); @@ -1720,7 +1718,6 @@ static void test_sample_processing(IMediaControl *control, IMemInputPin *input, hr = IMemInputPin_Receive(input, sample); todo_wine ok(hr == VFW_E_WRONG_STATE, "Receive returned %#lx.\n", hr); - todo_wine ok(got_ProcessInput == 0, "Got %u calls to ProcessInput().\n", got_ProcessInput); ok(got_ProcessOutput == 0, "Got %u calls to ProcessOutput().\n", got_ProcessOutput); ok(got_Receive == 0, "Got %u calls to Receive().\n", got_Receive); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5717
From: Ziqing Hui <zhui(a)codeweavers.com> --- dlls/qasf/tests/dmowrapper.c | 101 ++++++++++++++++++++++++++++++++++- 1 file changed, 99 insertions(+), 2 deletions(-) diff --git a/dlls/qasf/tests/dmowrapper.c b/dlls/qasf/tests/dmowrapper.c index 9fade2f7f1f..632b7e2e558 100644 --- a/dlls/qasf/tests/dmowrapper.c +++ b/dlls/qasf/tests/dmowrapper.c @@ -1122,6 +1122,7 @@ struct testfilter struct strmbase_sink sink; const AM_MEDIA_TYPE *sink_mt; unsigned int got_new_segment, got_eos, got_begin_flush, got_end_flush; + HANDLE event; }; static inline struct testfilter *impl_from_strmbase_filter(struct strmbase_filter *iface) @@ -1145,6 +1146,7 @@ static void testfilter_destroy(struct strmbase_filter *iface) strmbase_source_cleanup(&filter->source); strmbase_sink_cleanup(&filter->sink); strmbase_filter_cleanup(&filter->filter); + CloseHandle(filter->event); } static const struct strmbase_filter_ops testfilter_ops = @@ -1199,6 +1201,7 @@ static HRESULT testsink_get_media_type(struct strmbase_pin *iface, unsigned int static HRESULT WINAPI testsink_Receive(struct strmbase_sink *iface, IMediaSample *sample) { + struct testfilter *filter = impl_from_strmbase_filter(iface->pin.filter); REFERENCE_TIME start, stop; LONG len, i; HRESULT hr; @@ -1257,6 +1260,9 @@ static HRESULT WINAPI testsink_Receive(struct strmbase_sink *iface, IMediaSample if (testmode == 10) testmode = 11; + if (testmode == 13 && got_Receive > 1) + WaitForSingleObject(filter->event, INFINITE); + return S_OK; } @@ -1311,6 +1317,7 @@ static void testfilter_init(struct testfilter *filter) strmbase_filter_init(&filter->filter, NULL, &clsid, &testfilter_ops); strmbase_source_init(&filter->source, &filter->filter, L"source", &testsource_ops); strmbase_sink_init(&filter->sink, &filter->filter, L"sink", &testsink_ops, NULL); + filter->event = CreateEventA(NULL, TRUE, FALSE, NULL); } static void test_sink_allocator(IMemInputPin *input) @@ -1518,15 +1525,46 @@ static void test_filter_state(IMediaControl *control) ok(state == State_Stopped, "Got state %lu.\n", state); } +struct receive_proc_arg +{ + IMemInputPin *input_pin; + IMediaSample *sample; +}; + +static DWORD WINAPI receive_proc(void *arg) +{ + struct receive_proc_arg *proc_arg = arg; + HRESULT hr; + + hr = IMemInputPin_Receive(proc_arg->input_pin, proc_arg->sample); + ok(hr == S_OK, "Receive returned %#lx.\n", hr); + + return 0; +} + +static DWORD WINAPI stop_filter_proc(void *arg) +{ + IBaseFilter *filter = arg; + HRESULT hr; + + hr = IBaseFilter_Stop(filter); + ok(hr == S_OK, "Stop returned %#lx.\n", hr); + + return 0; +} + static void test_sample_processing(IMediaControl *control, IMemInputPin *input, - struct testfilter *testsink) + struct testfilter *testsink, IBaseFilter *dmo_filter) { + struct receive_proc_arg receive_proc_arg; + HANDLE stop_thread, receive_thread; REFERENCE_TIME start, stop; IMemAllocator *allocator; IMediaSample *sample; LONG size, i; HRESULT hr; BYTE *data; + DWORD ret; hr = IMemInputPin_ReceiveCanBlock(input); ok(hr == S_OK, "Got hr %#lx.\n", hr); @@ -1661,6 +1699,65 @@ static void test_sample_processing(IMediaControl *control, IMemInputPin *input, ok(got_Discontinuity == 1, "Got %u calls to Discontinuity().\n", got_Discontinuity); got_ProcessInput = got_ProcessOutput = got_Receive = got_Discontinuity = 0; + /* Test receive with downstream input allocator decommitted. */ + hr = IMemAllocator_Decommit(testsink->sink.pAllocator); + ok(hr == S_OK, "Decommit returned %#lx.\n", hr); + hr = IMemInputPin_Receive(input, sample); + todo_wine + ok(hr == VFW_E_NOT_COMMITTED, "Receive returned %#lx.\n", hr); + todo_wine + ok(got_ProcessInput == 0, "Got %u calls to ProcessInput().\n", got_ProcessInput); + ok(got_ProcessOutput == 0, "Got %u calls to ProcessOutput().\n", got_ProcessOutput); + ok(got_Receive == 0, "Got %u calls to Receive().\n", got_Receive); + ok(got_Discontinuity == 1, "Got %u calls to Discontinuity().\n", got_Discontinuity); + hr = IMemAllocator_Commit(testsink->sink.pAllocator); + ok(hr == S_OK, "Commit returned %#lx.\n", hr); + got_ProcessInput = got_ProcessOutput = got_Receive = got_Discontinuity = 0; + + /* Test receive with filter stopped. */ + hr = IBaseFilter_Stop(dmo_filter); + ok(hr == S_OK, "Stop returned %#lx.\n", hr); + hr = IMemInputPin_Receive(input, sample); + todo_wine + ok(hr == VFW_E_WRONG_STATE, "Receive returned %#lx.\n", hr); + todo_wine + ok(got_ProcessInput == 0, "Got %u calls to ProcessInput().\n", got_ProcessInput); + ok(got_ProcessOutput == 0, "Got %u calls to ProcessOutput().\n", got_ProcessOutput); + ok(got_Receive == 0, "Got %u calls to Receive().\n", got_Receive); + todo_wine + ok(got_Discontinuity == 0, "Got %u calls to Discontinuity().\n", got_Discontinuity); + hr = IMediaControl_Run(control); + ok(hr == S_OK, "Run returned %#lx.\n", hr); + got_ProcessInput = got_ProcessOutput = got_Receive = got_Discontinuity = 0; + + /* Test stop filter during receiving. */ + testmode = 13; + /* Call Receive() in new thread, it will be blocked + * by event waiting in testsink_Receive(). */ + receive_proc_arg.input_pin = input; + receive_proc_arg.sample = sample; + receive_thread = CreateThread(NULL, 0, receive_proc, &receive_proc_arg, 0, NULL); + ok(!!receive_thread, "CreateThread returned NULL thread.\n"); + ret = WaitForSingleObject(receive_thread, 200); + ok(ret == WAIT_TIMEOUT, "WaitForSingleObject returned %#lx.\n", ret); + /* Call Stop() in new thread, it will be blocked + * because Receive() is still processing. */ + stop_thread = CreateThread(NULL, 0, stop_filter_proc, dmo_filter, 0, NULL); + ok(!!stop_thread, "CreateThread returned NULL thread.\n"); + ret = WaitForSingleObject(stop_thread, 200); + todo_wine + ok(ret == WAIT_TIMEOUT, "WaitForSingleObject returned %#lx.\n", ret); + /* Signal event to end Receive(). */ + SetEvent(testsink->event); + ret = WaitForSingleObject(receive_thread, 200); + ok(ret == WAIT_OBJECT_0, "WaitForSingleObject returned %#lx.\n", ret); + ret = WaitForSingleObject(stop_thread, 200); + ok(ret == WAIT_OBJECT_0, "WaitForSingleObject returned %#lx.\n", ret); + got_ProcessInput = got_ProcessOutput = got_Receive = got_Discontinuity = 0; + + testmode = 0; + CloseHandle(stop_thread); + CloseHandle(receive_thread); hr = IMediaControl_Stop(control); ok(hr == S_OK, "Got hr %#lx.\n", hr); IMediaSample_Release(sample); @@ -1882,7 +1979,7 @@ static void test_connect_pin(void) ok(hr == S_OK, "Got hr %#lx.\n", hr); test_filter_state(control); - test_sample_processing(control, meminput, &testsink); + test_sample_processing(control, meminput, &testsink, filter); /* Streaming event tests are more interesting if multiple source pins are * connected. */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5717
From: Ziqing Hui <zhui(a)codeweavers.com> --- dlls/qasf/dmowrapper.c | 2 ++ dlls/qasf/tests/dmowrapper.c | 1 - 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/dlls/qasf/dmowrapper.c b/dlls/qasf/dmowrapper.c index 515e1464c5c..48294ca4823 100644 --- a/dlls/qasf/dmowrapper.c +++ b/dlls/qasf/dmowrapper.c @@ -702,6 +702,7 @@ static HRESULT dmo_wrapper_cleanup_stream(struct strmbase_filter *iface) IUnknown_QueryInterface(filter->dmo, &IID_IMediaObject, (void **)&dmo); + EnterCriticalSection(&filter->filter.stream_cs); for (i = 0; i < filter->source_count; ++i) { if (filter->sources[i].pin.pin.peer) @@ -711,6 +712,7 @@ static HRESULT dmo_wrapper_cleanup_stream(struct strmbase_filter *iface) IMediaObject_Flush(dmo); IMediaObject_Release(dmo); + LeaveCriticalSection(&filter->filter.stream_cs); return S_OK; } diff --git a/dlls/qasf/tests/dmowrapper.c b/dlls/qasf/tests/dmowrapper.c index 70dcc0c453e..388004de783 100644 --- a/dlls/qasf/tests/dmowrapper.c +++ b/dlls/qasf/tests/dmowrapper.c @@ -1740,7 +1740,6 @@ static void test_sample_processing(IMediaControl *control, IMemInputPin *input, stop_thread = CreateThread(NULL, 0, stop_filter_proc, dmo_filter, 0, NULL); ok(!!stop_thread, "CreateThread returned NULL thread.\n"); ret = WaitForSingleObject(stop_thread, 200); - todo_wine ok(ret == WAIT_TIMEOUT, "WaitForSingleObject returned %#lx.\n", ret); /* Signal event to end Receive(). */ SetEvent(testsink->event); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5717
From: Ziqing Hui <zhui(a)codeweavers.com> --- dlls/qasf/dmowrapper.c | 3 +++ dlls/qasf/tests/dmowrapper.c | 2 -- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/dlls/qasf/dmowrapper.c b/dlls/qasf/dmowrapper.c index 97cf43cbeff..515e1464c5c 100644 --- a/dlls/qasf/dmowrapper.c +++ b/dlls/qasf/dmowrapper.c @@ -309,6 +309,9 @@ static HRESULT WINAPI dmo_wrapper_sink_Receive(struct strmbase_sink *iface, IMed DWORD flags = 0; HRESULT hr; + if (filter->filter.state == State_Stopped) + return VFW_E_WRONG_STATE; + IUnknown_QueryInterface(filter->dmo, &IID_IMediaObject, (void **)&dmo); if (IMediaSample_IsDiscontinuity(sample) == S_OK) diff --git a/dlls/qasf/tests/dmowrapper.c b/dlls/qasf/tests/dmowrapper.c index ada45c447b1..70dcc0c453e 100644 --- a/dlls/qasf/tests/dmowrapper.c +++ b/dlls/qasf/tests/dmowrapper.c @@ -1716,12 +1716,10 @@ static void test_sample_processing(IMediaControl *control, IMemInputPin *input, hr = IBaseFilter_Stop(dmo_filter); ok(hr == S_OK, "Stop returned %#lx.\n", hr); hr = IMemInputPin_Receive(input, sample); - todo_wine ok(hr == VFW_E_WRONG_STATE, "Receive returned %#lx.\n", hr); ok(got_ProcessInput == 0, "Got %u calls to ProcessInput().\n", got_ProcessInput); ok(got_ProcessOutput == 0, "Got %u calls to ProcessOutput().\n", got_ProcessOutput); ok(got_Receive == 0, "Got %u calls to Receive().\n", got_Receive); - todo_wine ok(got_Discontinuity == 0, "Got %u calls to Discontinuity().\n", got_Discontinuity); hr = IMediaControl_Run(control); ok(hr == S_OK, "Run returned %#lx.\n", hr); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5717
2/4 is moving the code around at the same time as making a functional change, and I can't tell why it's moving the code around. Why does dmo_wrapper_sink_Receive() now call get_output_samples() an extra time? I know 4/4 is backed by tests, but what race or deadlock is it trying to fix? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5717#note_72153
(Sorry for the late review, also, I did not notice this patch due to GitLab problems...) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5717#note_72154
2/4 is moving the code around at the same time as making a functional change, and I can't tell why it's moving the code around.
We split get_output_samples() from process_output() in 2/4. The reason of the splitting is that, according to our tests, ProcessInput() is called between get_output_samples() and process_output(). The correct implementation should be: get_output_samples() -> ProcessInput() -> process_output(). So can't make get_output_samples() inside process_output().
Why does dmo_wrapper_sink_Receive() now call get_output_samples() an extra time?
We have 2 process_output() calls in dmo_wrapper_sink_Receive(), so we call it 2 times, which is the same as what we do before. (The second process_output() doesn't show in the commit diff.) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5717#note_72156
I know 4/4 is backed by tests, but what race or deadlock is it trying to fix?
The situation is, we have a filter graph like this: [source(reader)] -> [dmo decoder] -> [renderer]. We have a worker thead in source filter, the thread will get samples from the allocator of decoder's input pin, fill the samples, then push samples downsteam. So it will call dmo_wrapper_sink_Receive() on dmo decoder filter. And our main thread receive user inputs, it will skip the video if it got mouse click. So it will call Stop() on dmo decoder filter. Receive() should wait for Stop() and fail eventually, the Receive() failure will then break the loop in worker thread, and make the thread terminate normally. But if we don't sync Stop() with Receive(), the loop in worker thread will fail on sample allcating, because the allocator is decommited when calling Stop(), sample allcating failure will raise a error in the game. So the point is: for kirikiri2 games, it expect a Receive() failure, but don't expect a sample allocating failure. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5717#note_72164
2/4 is moving the code around at the same time as making a functional change, and I can't tell why it's moving the code around.
We split get_output_samples() from process_output() in 2/4. The reason of the splitting is that, according to our tests, ProcessInput() is called between get_output_samples() and process_output(). The correct implementation should be: get_output_samples() -\> ProcessInput() -\> process_output(). So can't make get_output_samples() inside process_output().
Okay, that's a separate functional change from what the subject describes and implements, so it should certainly be a separate patch. And is it actually necessary?
Why does dmo_wrapper_sink_Receive() now call get_output_samples() an extra time?
We have 2 process_output() calls in dmo_wrapper_sink_Receive(), so we call it 2 times, which is the same as what we do before. (The second process_output() doesn't show in the commit diff.)
Oops, yes, I should have read more carefully.
I know 4/4 is backed by tests, but what race or deadlock is it trying to fix?
The situation is, we have a filter graph like this: \[source(reader)\] -\> \[dmo decoder\] -\> \[renderer\].
We have a worker thead in source filter, the thread will get samples from the allocator of decoder's input pin, fill the samples, then push samples downsteam. So it will call dmo_wrapper_sink_Receive() on dmo decoder filter.
And our main thread receive user inputs, it will skip the video if it got mouse click. So it will call Stop() on dmo decoder filter.
Receive() should wait for Stop() and fail eventually, the Receive() failure will then break the loop in worker thread, and make the thread terminate normally.
But if we don't sync Stop() with Receive(), the loop in worker thread will fail on sample allcating, because the allocator is decommited when calling Stop(), sample allcating failure will raise a error in the game.
So the source filter is an application component, and it's fine with VFW_E_WRONG_STATE from Receive(), but doesn't like VFW_E_NOT_COMMITTED? That sounds like a good reason then. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5717#note_72241
Okay, that's a separate functional change from what the subject describes and implements, so it should certainly be a separate patch. And is it actually necessary?
Yeah, at least the game works better after this patch. I'll send a V2 version which split 2/4.
So the source filter is an application component, and it's fine with VFW_E_WRONG_STATE from Receive(), but doesn't like VFW_E_NOT_COMMITTED? That sounds like a good reason then.
True. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5717#note_72270
participants (3)
-
Elizabeth Figura (＠zfigura) -
Ziqing Hui
-
Ziqing Hui (＠zhui)