[PATCH v6 0/3] 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) -- v6: mf/samplegrabber: Handle samples receieved in paused state. mf/samplegrabber: Process samples in paused state in ignore_clock mode. 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 | 61 ++++++++++++++++++++++++++++++++++++---------- 2 files changed, 52 insertions(+), 14 deletions(-) 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 c9f8fe96f8b..891c82673ca 100644 --- a/dlls/mf/tests/mf.c +++ b/dlls/mf/tests/mf.c @@ -1775,6 +1775,7 @@ struct test_callback HANDLE event; IMFMediaEvent *media_event; + BOOL check_media_event; }; static struct test_callback *impl_from_IMFAsyncCallback(IMFAsyncCallback *iface) @@ -1835,15 +1836,18 @@ static HRESULT WINAPI testcallback_Invoke(IMFAsyncCallback *iface, IMFAsyncResul if (callback->media_event) IMFMediaEvent_Release(callback->media_event); - hr = IMFAsyncResult_GetObject(result, &object); - ok(hr == E_POINTER, "Unexpected hr %#lx.\n", hr); + if (callback->check_media_event) + { + hr = IMFAsyncResult_GetObject(result, &object); + ok(hr == E_POINTER, "Unexpected hr %#lx.\n", hr); - hr = IMFAsyncResult_GetState(result, &object); - ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); - hr = IMFMediaEventGenerator_EndGetEvent((IMFMediaEventGenerator *)object, - result, &callback->media_event); - ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); - IUnknown_Release(object); + hr = IMFAsyncResult_GetState(result, &object); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + hr = IMFMediaEventGenerator_EndGetEvent((IMFMediaEventGenerator *)object, + result, &callback->media_event); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + IUnknown_Release(object); + } SetEvent(callback->event); @@ -1859,7 +1863,7 @@ static const IMFAsyncCallbackVtbl testcallbackvtbl = testcallback_Invoke, }; -static IMFAsyncCallback *create_test_callback(void) +static IMFAsyncCallback *create_test_callback(BOOL check_media_event) { struct test_callback *callback; @@ -1867,6 +1871,7 @@ static IMFAsyncCallback *create_test_callback(void) return NULL; callback->refcount = 1; + callback->check_media_event = check_media_event; callback->IMFAsyncCallback_iface.lpVtbl = &testcallbackvtbl; callback->event = CreateEventW(NULL, FALSE, FALSE, NULL); ok(!!callback->event, "CreateEventW failed, error %lu\n", GetLastError()); @@ -2000,8 +2005,8 @@ static void test_media_session_events(void) hr = MFStartup(MF_VERSION, MFSTARTUP_FULL); ok(hr == S_OK, "Startup failure, hr %#lx.\n", hr); - callback = create_test_callback(); - callback2 = create_test_callback(); + callback = create_test_callback(TRUE); + callback2 = create_test_callback(TRUE); hr = MFCreateMediaSession(NULL, &session); ok(hr == S_OK, "Failed to create media session, hr %#lx.\n", hr); @@ -2056,7 +2061,7 @@ static void test_media_session_events(void) IMFAsyncCallback_Release(callback2); - callback = create_test_callback(); + callback = create_test_callback(TRUE); hr = MFCreateMediaSession(NULL, &session); ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); @@ -3799,15 +3804,20 @@ 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; + IMFAsyncCallback *callback; + 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; @@ -4031,6 +4041,31 @@ 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); + + callback = create_test_callback(FALSE); + timer_callback = impl_from_IMFAsyncCallback(callback); + hr = IMFTimer_SetTimer(timer, 0, 100000, callback, 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); + + IUnknown_Release(timer_cancel_key); + IMFTimer_Release(timer); + IMFAsyncCallback_Release(callback); + hr = IMFPresentationClock_QueryInterface(clock, &IID_IMFShutdown, (void **)&shutdown); ok(hr == S_OK, "Failed to get shutdown interface, hr %#lx.\n", hr); @@ -4682,7 +4717,7 @@ static void test_sample_grabber_orientation(GUID subtype) return; } - callback = create_test_callback(); + callback = create_test_callback(TRUE); grabber_callback = impl_from_IMFSampleGrabberSinkCallback(create_test_grabber_callback()); grabber_callback->ready_event = CreateEventW(NULL, FALSE, FALSE, NULL); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/574
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/mf/samplegrabber.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/mf/samplegrabber.c b/dlls/mf/samplegrabber.c index e41e74d35ac..c0c92143b6d 100644 --- a/dlls/mf/samplegrabber.c +++ b/dlls/mf/samplegrabber.c @@ -425,7 +425,7 @@ static HRESULT WINAPI sample_grabber_stream_ProcessSample(IMFStreamSink *iface, if (grabber->is_shut_down) hr = MF_E_STREAMSINK_REMOVED; - else if (grabber->state == SINK_STATE_RUNNING) + else if (grabber->state == SINK_STATE_RUNNING || (grabber->state == SINK_STATE_PAUSED && grabber->ignore_clock)) { hr = IMFSample_GetSampleTime(sample, &sampletime); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/574
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/mf/samplegrabber.c | 52 ++++++++++++++++++++++++++++++++++++++--- 1 file changed, 49 insertions(+), 3 deletions(-) diff --git a/dlls/mf/samplegrabber.c b/dlls/mf/samplegrabber.c index c0c92143b6d..deb1ce09a55 100644 --- a/dlls/mf/samplegrabber.c +++ b/dlls/mf/samplegrabber.c @@ -19,6 +19,7 @@ #define COBJMACROS #include <float.h> +#include <assert.h> #include "mfidl.h" #include "mf_private.h" @@ -58,6 +59,8 @@ struct scheduled_item } u; }; +#define MAX_SAMPLE_QUEUE_LENGTH 4 + struct sample_grabber { IMFMediaSink IMFMediaSink_iface; @@ -86,6 +89,8 @@ struct sample_grabber float rate; enum sink_state state; CRITICAL_SECTION cs; + UINT32 sample_count; + IMFSample *samples[MAX_SAMPLE_QUEUE_LENGTH]; }; static IMFSampleGrabberSinkCallback *sample_grabber_get_callback(const struct sample_grabber *sink) @@ -443,6 +448,14 @@ 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_count < MAX_SAMPLE_QUEUE_LENGTH) + { + IMFSample_AddRef(sample); + grabber->samples[grabber->sample_count++] = sample; + } + } LeaveCriticalSection(&grabber->cs); @@ -827,6 +840,20 @@ static void sample_grabber_release_pending_items(struct sample_grabber *grabber) } } +static void release_samples(struct sample_grabber *grabber) +{ + unsigned int i; + + for (i = 0; i < MAX_SAMPLE_QUEUE_LENGTH; ++i) + { + if (grabber->samples[i]) + { + IMFSample_Release(grabber->samples[i]); + grabber->samples[i] = NULL; + } + } +} + static ULONG WINAPI sample_grabber_sink_Release(IMFMediaSink *iface) { struct sample_grabber *grabber = impl_from_IMFMediaSink(iface); @@ -858,6 +885,7 @@ static ULONG WINAPI sample_grabber_sink_Release(IMFMediaSink *iface) if (grabber->sample_attributes) IMFAttributes_Release(grabber->sample_attributes); sample_grabber_release_pending_items(grabber); + release_samples(grabber); DeleteCriticalSection(&grabber->cs); free(grabber); } @@ -1129,14 +1157,31 @@ static HRESULT sample_grabber_set_state(struct sample_grabber *grabber, enum sin else { if (state == SINK_STATE_STOPPED) + { sample_grabber_cancel_timer(grabber); + release_samples(grabber); + grabber->sample_count = MAX_SAMPLE_QUEUE_LENGTH; + } - 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) - sample_grabber_stream_request_sample(grabber); + for (i = 0; i < grabber->sample_count; ++i) + { + if (grabber->state == SINK_STATE_PAUSED && offset == PRESENTATION_CURRENT_POSITION) + { + assert(grabber->samples[i]); + stream_queue_sample(grabber, grabber->samples[i]); + } + else + { + sample_grabber_stream_request_sample(grabber); + } + } + release_samples(grabber); + grabber->sample_count = 0; } + do_callback = state != grabber->state || state != SINK_STATE_PAUSED; if (do_callback) { @@ -1442,6 +1487,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_count = MAX_SAMPLE_QUEUE_LENGTH; object->refcount = 1; object->rate = 1.0f; if (FAILED(IMFSampleGrabberSinkCallback_QueryInterface(context->callback, &IID_IMFSampleGrabberSinkCallback2, -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/574
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 tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=125852 Your paranoid android. === debian11 (32 bit report) === shell32: shelllink.c:847: Test failed: SHCreateStreamOnFileW failed 80070020 shelllink.c:849: Test failed: IPersistFile_Save failed (0x00000000) shelllink.c:851: Test failed: IPersistFile_Load failed (0x00000000) wmvcore: wmvcore.c:2997: Test failed: Wait timed out.
v6: - Define MAX_SAMPLE_QUEUE_LENGTH instead of hardcoding 4; - rename `sample_request_on_start_count` / `samples_queued_while_paused` to sample_count / samples; - rebase test in the first patch. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/574#note_13425
This merge request was approved by Nikolay Sivov. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/574
participants (4)
-
Marvin -
Nikolay Sivov (＠nsivov) -
Paul Gofman
-
Paul Gofman (＠gofman)