[PATCH v3 0/2] MR574: mf: Improve session pause / restart handling.
Patch 1. The tests on Windows show that when the timer callback is queued behind the clock time it is queued and executed quite soon with 0-15ms delay which corresponds to default timer granularity. The timeout tolerance in the test is quite bigger to avoid a flaky test. Wine currently may execute the callback right away or delay it indefinitely depending on the time difference and current system time. Patch 2. It turns out when the session is paused and then restarted and all the samples were delivered to sample grabber before restart, nothing is going to request the samples again when the session is restarted. I am attaching a test in a patch which hopefully confirms how that should work. I am not including that in the patches as the test involves delays (to make sure all the async events settle at certain points) and inherently flaky. Also, _ IMFStreamSink_GetEvent(stream, ...) in the test crashes on Win7 for some unknown reason. [0001-mf-tests-Add-test-for-pausing-and-restarting-sample-.patch](/uploads/58c1738d48b3392e7ec430a0757210d0/0001-mf-tests-Add-test-for-pausing-and-restarting-sample-.patch) -- v3: mf/samplegrabber: Request a sample when going from paused to running state. mf: Handle timer time behind clock time in present_clock_schedule_timer(). https://gitlab.winehq.org/wine/wine/-/merge_requests/574
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/mf/clock.c | 5 ++++- dlls/mf/tests/mf.c | 42 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 1 deletion(-) diff --git a/dlls/mf/clock.c b/dlls/mf/clock.c index e9877533a5a..aef4366c6d9 100644 --- a/dlls/mf/clock.c +++ b/dlls/mf/clock.c @@ -819,7 +819,10 @@ static HRESULT present_clock_schedule_timer(struct presentation_clock *clock, DW WARN("Failed to get clock time, hr %#lx.\n", hr); return hr; } - time -= clocktime; + if (time > clocktime) + time -= clocktime; + else + time = 0; } frequency = clock->frequency / 1000; diff --git a/dlls/mf/tests/mf.c b/dlls/mf/tests/mf.c index 6621a0f0674..62658a32500 100644 --- a/dlls/mf/tests/mf.c +++ b/dlls/mf/tests/mf.c @@ -1428,8 +1428,14 @@ static const struct test_stream_sink test_stream_sink = {.IMFStreamSink_iface.lp struct test_callback { IMFAsyncCallback IMFAsyncCallback_iface; + HANDLE event; }; +static struct test_callback *test_callback_from_IMFAsyncCallback(IMFAsyncCallback *iface) +{ + return CONTAINING_RECORD(iface, struct test_callback, IMFAsyncCallback_iface); +} + static HRESULT WINAPI testcallback_QueryInterface(IMFAsyncCallback *iface, REFIID riid, void **obj) { if (IsEqualIID(riid, &IID_IMFAsyncCallback) || @@ -1462,8 +1468,12 @@ static HRESULT WINAPI testcallback_GetParameters(IMFAsyncCallback *iface, DWORD static HRESULT WINAPI testcallback_Invoke(IMFAsyncCallback *iface, IMFAsyncResult *result) { + struct test_callback *test = test_callback_from_IMFAsyncCallback(iface); ok(result != NULL, "Unexpected result object.\n"); + if (test->event) + SetEvent(test->event); + return E_NOTIMPL; } @@ -1479,6 +1489,7 @@ static const IMFAsyncCallbackVtbl testcallbackvtbl = static void init_test_callback(struct test_callback *callback) { callback->IMFAsyncCallback_iface.lpVtbl = &testcallbackvtbl; + callback->event = NULL; } static void test_session_events(IMFMediaSession *session) @@ -2865,15 +2876,19 @@ static void test_presentation_clock(void) }; IMFClockStateSink test_sink = { &test_clock_sink_vtbl }; IMFPresentationTimeSource *time_source; + struct test_callback timer_callback; MFCLOCK_PROPERTIES props, props2; IMFRateControl *rate_control; IMFPresentationClock *clock; + IUnknown *timer_cancel_key; MFSHUTDOWN_STATUS status; IMFShutdown *shutdown; MFTIME systime, time; LONGLONG clock_time; MFCLOCK_STATE state; + IMFTimer *timer; unsigned int i; + DWORD t1, t2; DWORD value; float rate; HRESULT hr; @@ -3097,6 +3112,33 @@ static void test_presentation_clock(void) IMFRateControl_Release(rate_control); + + hr = IMFPresentationClock_QueryInterface(clock, &IID_IMFTimer, (void **)&timer); + ok(hr == S_OK, "got hr %#lx.\n", hr); + + hr = IMFPresentationClock_Start(clock, 200000); + ok(hr == S_OK, "got hr %#lx.\n", hr); + + hr = IMFPresentationClock_GetCorrelatedTime(clock, 0, &time, &systime); + ok(hr == S_OK, "got hr %#lx.\n", hr); + + init_test_callback(&timer_callback); + timer_callback.event = CreateEventW(NULL, FALSE, FALSE, NULL); + hr = IMFTimer_SetTimer(timer, 0, 100000, + &timer_callback.IMFAsyncCallback_iface, NULL, &timer_cancel_key); + ok(hr == S_OK, "got hr %#lx.\n", hr); + + t1 = GetTickCount(); + ok(WaitForSingleObject(timer_callback.event, 4000) == WAIT_OBJECT_0, "WaitForSingleObject failed.\n"); + t2 = GetTickCount(); + + ok(t2 - t1 < 200, "unexpected time difference %lu.\n", t2 - t1); + + CloseHandle(timer_callback.event); + IUnknown_Release(timer_cancel_key); + IMFTimer_Release(timer); + + hr = IMFPresentationClock_QueryInterface(clock, &IID_IMFShutdown, (void **)&shutdown); ok(hr == S_OK, "Failed to get shutdown interface, hr %#lx.\n", hr); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/574
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/mf/samplegrabber.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/dlls/mf/samplegrabber.c b/dlls/mf/samplegrabber.c index 2d9f8109aca..5808669f729 100644 --- a/dlls/mf/samplegrabber.c +++ b/dlls/mf/samplegrabber.c @@ -85,6 +85,7 @@ struct sample_grabber UINT64 sample_time_offset; enum sink_state state; CRITICAL_SECTION cs; + UINT32 sample_request_on_start_count; }; static IMFSampleGrabberSinkCallback *sample_grabber_get_callback(const struct sample_grabber *sink) @@ -442,6 +443,11 @@ static HRESULT WINAPI sample_grabber_stream_ProcessSample(IMFStreamSink *iface, hr = stream_queue_sample(grabber, sample); } } + else if (grabber->state == SINK_STATE_PAUSED) + { + if (grabber->sample_request_on_start_count < 4) + ++grabber->sample_request_on_start_count; + } LeaveCriticalSection(&grabber->cs); @@ -1128,14 +1134,19 @@ static HRESULT sample_grabber_set_state(struct sample_grabber *grabber, enum sin else { if (state == SINK_STATE_STOPPED) + { sample_grabber_cancel_timer(grabber); + grabber->sample_request_on_start_count = 4; + } - if (state == SINK_STATE_RUNNING && grabber->state == SINK_STATE_STOPPED) + if (state == SINK_STATE_RUNNING && grabber->state != SINK_STATE_RUNNING) { /* Every transition to running state sends a bunch requests to build up initial queue. */ - for (i = 0; i < 4; ++i) + for (i = 0; i < grabber->sample_request_on_start_count; ++i) sample_grabber_stream_request_sample(grabber); + grabber->sample_request_on_start_count = 0; } + do_callback = state != grabber->state || state != SINK_STATE_PAUSED; if (do_callback) IMFStreamSink_QueueEvent(&grabber->IMFStreamSink_iface, events[state], &GUID_NULL, S_OK, NULL); @@ -1419,6 +1430,7 @@ static HRESULT sample_grabber_create_object(IMFAttributes *attributes, void *use object->IMFStreamSink_iface.lpVtbl = &sample_grabber_stream_vtbl; object->IMFMediaTypeHandler_iface.lpVtbl = &sample_grabber_stream_type_handler_vtbl; object->timer_callback.lpVtbl = &sample_grabber_stream_timer_callback_vtbl; + object->sample_request_on_start_count = 4; object->refcount = 1; if (FAILED(IMFSampleGrabberSinkCallback_QueryInterface(context->callback, &IID_IMFSampleGrabberSinkCallback2, (void **)&object->callback2))) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/574
On Mon Aug 8 16:57:05 2022 +0000, **** wrote:
Marvin replied on the mailing list: ``` Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=120690 Your paranoid android. === w7u_el (32 bit report) === mf: mf: Timeout ``` I changed infinite timeout to a limited one in my newly added test but then the test ran ok without failures (https://testbot.winehq.org/JobDetails.pl?Key=120692&f204=exe32.report#k204), while it would produce an error if timeout was hit, so looks like the the issue on w7u_el wasn't reproduced.
I've updated the patches and v3 has the timeout, so if it reproduces once again it will be clear if that it is my added test is failing or if that is unrelated. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/574#note_5836
We probably should be treating RUNNING and PAUSED state the same in ProcessSample(). For example as far as I can tell in MF_SAMPLEGRABBERSINK_IGNORE_CLOCK mode ProcessSample() calls OnProcessSample() right away, as we do in RUNNING state currently. That's a trivial change of course, what needs to be tested is what happens with queued samples when restart time is ahead of sample timestamps. In attached test they seem to be dropped, but a test that will tell more would do ProcessSample(timestamp=1), ProcessSample(timestamp=10), ProcessSample(timestamp=20), then starting at clock position 5 or 15. Regarding outstanding requests count, I think the logic is that it's always limited to the maximum of 4, so that when PAUSED any number of samples could be queued, but on restart 4 requests at max will be issued. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/574#note_6210
participants (3)
-
Nikolay Sivov (＠nsivov) -
Paul Gofman
-
Paul Gofman (＠gofman)