[PATCH v2 1/4] winegstreamer: Advertise support for fast forward and slow motion.
Signed-off-by: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- dlls/winegstreamer/media_source.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/dlls/winegstreamer/media_source.c b/dlls/winegstreamer/media_source.c index 90efc9aaed2..3b7233ac2b5 100644 --- a/dlls/winegstreamer/media_source.c +++ b/dlls/winegstreamer/media_source.c @@ -933,7 +933,7 @@ static HRESULT WINAPI media_source_rate_support_GetSlowestRate(IMFRateSupport *i { TRACE("%p, %d, %d, %p.\n", iface, direction, thin, rate); - *rate = direction == MFRATE_FORWARD ? 1.0f : -1.0f; + *rate = 0.0f; return S_OK; } @@ -942,14 +942,14 @@ static HRESULT WINAPI media_source_rate_support_GetFastestRate(IMFRateSupport *i { TRACE("%p, %d, %d, %p.\n", iface, direction, thin, rate); - *rate = direction == MFRATE_FORWARD ? 1.0f : -1.0f; + *rate = direction == MFRATE_FORWARD ? 1e6f : -1e6f; return S_OK; } static HRESULT WINAPI media_source_rate_support_IsRateSupported(IMFRateSupport *iface, BOOL thin, float rate, float *nearest_support_rate) { - const float supported_rate = rate >= 0.0f ? 1.0f : -1.0f; + const float supported_rate = max(min(rate, 1e6f), -1e6f); TRACE("%p, %d, %f, %p.\n", iface, thin, rate, nearest_support_rate); -- 2.32.0.rc0
Signed-off-by: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- dlls/mf/tests/mf.c | 53 +++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 50 insertions(+), 3 deletions(-) diff --git a/dlls/mf/tests/mf.c b/dlls/mf/tests/mf.c index c8c7e0569cc..da7e40c67fa 100644 --- a/dlls/mf/tests/mf.c +++ b/dlls/mf/tests/mf.c @@ -2725,17 +2725,17 @@ static ULONG WINAPI grabber_callback_Release(IMFSampleGrabberSinkCallback *iface static HRESULT WINAPI grabber_callback_OnClockStart(IMFSampleGrabberSinkCallback *iface, MFTIME time, LONGLONG offset) { - return E_NOTIMPL; + return S_OK; } static HRESULT WINAPI grabber_callback_OnClockStop(IMFSampleGrabberSinkCallback *iface, MFTIME time) { - return E_NOTIMPL; + return S_OK; } static HRESULT WINAPI grabber_callback_OnClockPause(IMFSampleGrabberSinkCallback *iface, MFTIME time) { - return E_NOTIMPL; + return S_OK; } static HRESULT WINAPI grabber_callback_OnClockRestart(IMFSampleGrabberSinkCallback *iface, MFTIME time) @@ -2782,6 +2782,28 @@ static const IMFSampleGrabberSinkCallbackVtbl grabber_callback_vtbl = static IMFSampleGrabberSinkCallback grabber_callback = { &grabber_callback_vtbl }; +#define expect_event(s, m) expect_event_(__LINE__, s, m) +static void expect_event_(int line, IMFStreamSink *stream, MediaEventType met) +{ + HRESULT hr, hr2; + IMFMediaEvent *event; + MediaEventType got_met; + + hr = IMFStreamSink_GetEvent(stream, 0, &event); + ok_(__FILE__, line)(hr == S_OK, "Cannot get event, hr %#x.\n", hr); + ok_(__FILE__, line)(event != NULL, "Got NULL event.\n"); + + hr = IMFMediaEvent_GetStatus(event, &hr2); + ok_(__FILE__, line)(hr == S_OK, "Cannot get status, hr %#x.\n", hr); + ok_(__FILE__, line)(hr2 == S_OK, "Unexpected status, hr %#x.\n", hr2); + + hr = IMFMediaEvent_GetType(event, &got_met); + ok_(__FILE__, line)(hr == S_OK, "Cannot get type, hr %#x.\n", hr); + ok_(__FILE__, line)(got_met == met, "Unexpected event type %d instead of %d.\n", got_met, met); + + IMFMediaEvent_Release(event); +} + static void test_sample_grabber(void) { IMFMediaType *media_type, *media_type2, *media_type3; @@ -2791,6 +2813,7 @@ static void test_sample_grabber(void) IMFStreamSink *stream, *stream2; IMFRateSupport *rate_support; IMFMediaEventGenerator *eg; + IMFClockStateSink *css; IMFMediaSink *sink, *sink2; DWORD flags, count, id; IMFActivate *activate; @@ -2800,6 +2823,7 @@ static void test_sample_grabber(void) float rate; HRESULT hr; GUID guid; + int i; hr = MFStartup(MF_VERSION, MFSTARTUP_FULL); ok(hr == S_OK, "Failed to start up, hr %#x.\n", hr); @@ -3080,6 +3104,29 @@ static void test_sample_grabber(void) hr = IMFStreamSink_GetEvent(stream, MF_EVENT_FLAG_NO_WAIT, &event); ok(hr == MF_E_NO_EVENTS_AVAILABLE, "Unexpected hr %#x.\n", hr); + hr = IMFMediaSink_QueryInterface(sink, &IID_IMFClockStateSink, (void **)&css); + ok(hr == S_OK, "Failed to get interface, hr %#x.\n", hr); + + hr = IMFClockStateSink_OnClockStart(css, MFGetSystemTime(), 0); + ok(hr == S_OK, "Failed to start clock, hr %#x.\n", hr); + + for (i = 0; i < 4; i++) + expect_event(stream, MEStreamSinkRequestSample); + expect_event(stream, MEStreamSinkStarted); + + hr = IMFClockStateSink_OnClockPause(css, MFGetSystemTime()); + ok(hr == S_OK, "Failed to pause clock, hr %#x.\n", hr); + if (strcmp(winetest_platform, "wine") != 0) + expect_event(stream, MEStreamSinkPaused); + else + todo_wine ok(0, "MEStreamSinkPaused event was not sent.\n"); + + hr = IMFClockStateSink_OnClockStop(css, MFGetSystemTime()); + ok(hr == S_OK, "Failed to stop clock, hr %#x.\n", hr); + expect_event(stream, MEStreamSinkStopped); + + IMFClockStateSink_Release(css); + EXPECT_REF(clock, 3); hr = IMFMediaSink_Shutdown(sink); ok(hr == S_OK, "Failed to shut down, hr %#x.\n", hr); -- 2.32.0.rc0
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=91381 Your paranoid android. === w7u_adm (32 bit report) === mf: 0a2c:mf: unhandled exception c0000005 at 0066A0E8 === w7u_el (32 bit report) === mf: 0a58:mf: unhandled exception c0000005 at 005D44E0
On 5/28/21 10:50 AM, Giovanni Mascellani wrote:
+#define expect_event(s, m) expect_event_(__LINE__, s, m) +static void expect_event_(int line, IMFStreamSink *stream, MediaEventType met) +{ + HRESULT hr, hr2; + IMFMediaEvent *event; + MediaEventType got_met; + + hr = IMFStreamSink_GetEvent(stream, 0, &event); + ok_(__FILE__, line)(hr == S_OK, "Cannot get event, hr %#x.\n", hr); + ok_(__FILE__, line)(event != NULL, "Got NULL event.\n"); + + hr = IMFMediaEvent_GetStatus(event, &hr2); + ok_(__FILE__, line)(hr == S_OK, "Cannot get status, hr %#x.\n", hr); + ok_(__FILE__, line)(hr2 == S_OK, "Unexpected status, hr %#x.\n", hr2); + + hr = IMFMediaEvent_GetType(event, &got_met); + ok_(__FILE__, line)(hr == S_OK, "Cannot get type, hr %#x.\n", hr); + ok_(__FILE__, line)(got_met == met, "Unexpected event type %d instead of %d.\n", got_met, met); + + IMFMediaEvent_Release(event); +} I don't think we want that, it's blocking, so can potentially hang the test program, and you won't know where that happened.
+ for (i = 0; i < 4; i++) + expect_event(stream, MEStreamSinkRequestSample); + expect_event(stream, MEStreamSinkStarted); Same here, the test only cares for state changes, and checking for everything exposes this improvised on-start prerolling.
Hi, Il 28/05/21 10:32, Nikolay Sivov ha scritto:
I don't think we want that, it's blocking, so can potentially hang the test program, and you won't know where that happened.
So what should the test do? Implement a timeout? We cannot just assume that the event will be delivered immediately.
+ for (i = 0; i < 4; i++) + expect_event(stream, MEStreamSinkRequestSample); + expect_event(stream, MEStreamSinkStarted); Same here, the test only cares for state changes, and checking for everything exposes this improvised on-start prerolling.
That's curious, because on Windows the test passes on basically all versions (except some segmentation faults with Win 7, but I think that is a separate issue, given that I was hitting it also with another patch set), so I thought that four was considered a consistent constant of Windows and that we wanted to emulate it. What would you expect here? I just check that at least on MEStreamSinkRequestSample event is generated? Or I just ignore any generated MEStreamSinkReuqestSample event until I get the MEStreamSinkStarted? Thanks, Giovanni.
On 5/28/21 12:09 PM, Giovanni Mascellani wrote:
Hi,
Il 28/05/21 10:32, Nikolay Sivov ha scritto:
I don't think we want that, it's blocking, so can potentially hang the test program, and you won't know where that happened.
So what should the test do? Implement a timeout? We cannot just assume that the event will be delivered immediately.
For GetEvent() you'll need to call it with no-wait flag several times to implement such timeout. Async event delivery is causing troubles too, as mfplat/tests demonstrate (at least on Win7 that we still use).
+ for (i = 0; i < 4; i++) + expect_event(stream, MEStreamSinkRequestSample); + expect_event(stream, MEStreamSinkStarted); Same here, the test only cares for state changes, and checking for everything exposes this improvised on-start prerolling.
That's curious, because on Windows the test passes on basically all versions (except some segmentation faults with Win 7, but I think that is a separate issue, given that I was hitting it also with another patch set), so I thought that four was considered a consistent constant of Windows and that we wanted to emulate it.
What would you expect here? I just check that at least on MEStreamSinkRequestSample event is generated? Or I just ignore any generated MEStreamSinkReuqestSample event until I get the MEStreamSinkStarted?
I don't think we need event tests at all for this case, it's already clear where state change happens, and that pause is not triggered.
Thanks, Giovanni.
Signed-off-by: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- dlls/mf/samplegrabber.c | 2 ++ dlls/mf/tests/mf.c | 5 +---- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/dlls/mf/samplegrabber.c b/dlls/mf/samplegrabber.c index f60ce2a8433..815e3f60feb 100644 --- a/dlls/mf/samplegrabber.c +++ b/dlls/mf/samplegrabber.c @@ -1171,6 +1171,8 @@ static HRESULT WINAPI sample_grabber_clock_sink_OnClockPause(IMFClockStateSink * TRACE("%p, %s.\n", iface, debugstr_time(systime)); + IMFStreamSink_QueueEvent(&grabber->IMFStreamSink_iface, MEStreamSinkPaused, &GUID_NULL, S_OK, NULL); + return IMFSampleGrabberSinkCallback_OnClockPause(sample_grabber_get_callback(grabber), systime); } diff --git a/dlls/mf/tests/mf.c b/dlls/mf/tests/mf.c index da7e40c67fa..3dd3a36626f 100644 --- a/dlls/mf/tests/mf.c +++ b/dlls/mf/tests/mf.c @@ -3116,10 +3116,7 @@ static void test_sample_grabber(void) hr = IMFClockStateSink_OnClockPause(css, MFGetSystemTime()); ok(hr == S_OK, "Failed to pause clock, hr %#x.\n", hr); - if (strcmp(winetest_platform, "wine") != 0) - expect_event(stream, MEStreamSinkPaused); - else - todo_wine ok(0, "MEStreamSinkPaused event was not sent.\n"); + expect_event(stream, MEStreamSinkPaused); hr = IMFClockStateSink_OnClockStop(css, MFGetSystemTime()); ok(hr == S_OK, "Failed to stop clock, hr %#x.\n", hr); -- 2.32.0.rc0
On 5/28/21 10:50 AM, Giovanni Mascellani wrote:
diff --git a/dlls/mf/samplegrabber.c b/dlls/mf/samplegrabber.c index f60ce2a8433..815e3f60feb 100644 --- a/dlls/mf/samplegrabber.c +++ b/dlls/mf/samplegrabber.c @@ -1171,6 +1171,8 @@ static HRESULT WINAPI sample_grabber_clock_sink_OnClockPause(IMFClockStateSink *
TRACE("%p, %s.\n", iface, debugstr_time(systime));
+ IMFStreamSink_QueueEvent(&grabber->IMFStreamSink_iface, MEStreamSinkPaused, &GUID_NULL, S_OK, NULL); + return IMFSampleGrabberSinkCallback_OnClockPause(sample_grabber_get_callback(grabber), systime); } This is incomplete, for several reasons. Pause when stopped is an invalid transition; pause when already paused should not generate an event.
Il 28/05/21 10:29, Nikolay Sivov ha scritto:
This is incomplete, for several reasons. Pause when stopped is an invalid transition; pause when already paused should not generate an event.
Ok. Should I write additional tests for these cases? Last time you suggested that I was writing too many tests, so I would like to understand what is the right amount. Thanks again, Giovanni.
On 5/28/21 12:13 PM, Giovanni Mascellani wrote:
Il 28/05/21 10:29, Nikolay Sivov ha scritto:
This is incomplete, for several reasons. Pause when stopped is an invalid transition; pause when already paused should not generate an event.
Ok. Should I write additional tests for these cases? Last time you suggested that I was writing too many tests, so I would like to understand what is the right amount.
That's easily observable with tests from 2/4 and a couple more calls. We do need more tests in general, but not potentially fragile ones, and event collection appears to be that for media foundation.
Thanks again, Giovanni.
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=91382 Your paranoid android. === w7u_adm (32 bit report) === mf: 0a38:mf: unhandled exception c0000005 at 0059A0E8 === w7u_el (32 bit report) === mf: 0a4c:mf: unhandled exception c0000005 at 006544E0
Signed-off-by: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- dlls/mf/session.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/dlls/mf/session.c b/dlls/mf/session.c index 925a8c93d20..9aa886adee9 100644 --- a/dlls/mf/session.c +++ b/dlls/mf/session.c @@ -18,6 +18,7 @@ #include <stdarg.h> #include <math.h> +#include <float.h> #define COBJMACROS @@ -3512,12 +3513,14 @@ static HRESULT session_presentation_object_get_rate(IUnknown *object, MFRATE_DIR if (fastest) { if (SUCCEEDED(hr = IMFRateSupport_GetFastestRate(rate_support, direction, thin, &rate))) - *result = min(fabsf(rate), *result); + if (fabsf(rate) < fabsf(*result)) + *result = rate; } else { if (SUCCEEDED(hr = IMFRateSupport_GetSlowestRate(rate_support, direction, thin, &rate))) - *result = max(fabsf(rate), *result); + if (fabsf(rate) > fabsf(*result)) + *result = rate; } IMFRateSupport_Release(rate_support); @@ -3532,7 +3535,7 @@ static HRESULT session_get_presentation_rate(struct media_session *session, MFRA struct media_sink *sink; HRESULT hr = E_POINTER; - *result = 0.0f; + *result = fastest ? (direction == MFRATE_FORWARD ? FLT_MAX : -FLT_MAX) : 0.0f; EnterCriticalSection(&session->cs); -- 2.32.0.rc0
On 5/28/21 10:50 AM, Giovanni Mascellani wrote:
if (fastest) { if (SUCCEEDED(hr = IMFRateSupport_GetFastestRate(rate_support, direction, thin, &rate))) - *result = min(fabsf(rate), *result); + if (fabsf(rate) < fabsf(*result)) + *result = rate; } else { if (SUCCEEDED(hr = IMFRateSupport_GetSlowestRate(rate_support, direction, thin, &rate))) - *result = max(fabsf(rate), *result); + if (fabsf(rate) > fabsf(*result)) + *result = rate; }
IMFRateSupport_Release(rate_support); @@ -3532,7 +3535,7 @@ static HRESULT session_get_presentation_rate(struct media_session *session, MFRA struct media_sink *sink; HRESULT hr = E_POINTER;
- *result = 0.0f; + *result = fastest ? (direction == MFRATE_FORWARD ? FLT_MAX : -FLT_MAX) : 0.0f;
EnterCriticalSection(&session->cs);
These two changes (or three) changes are separate. Second changes default even when none of components support rate change. How did you test this?
Hi, Il 28/05/21 10:37, Nikolay Sivov ha scritto:
These two changes (or three) changes are separate. Second changes default even when none of components support rate change. How did you test this?
I don't understand why they should be separate. This piece of code is basically computing the minimum or the maximum between a bunch of values. There were two bugs: first one is that 0.0 is a valid initialization value when you're computing a maximum, but not when you are computing a minimum (if you iteratively compute min() starting from 0.0, the result will always be 0.0). Second bug is that you have compute the maximum and minimum in absolute value. For example, if a session has a source with fastest reverse rate -100.0 and another with -20.0, the global fastest should be the minimum in absolute value, which is -20.0, and not -100.0. My patch fixes two bugs, but they are in the same algorithm, so it felt right to have them together. As for the tests, again, it's never clear to me when it's appropriate to have tests and when not. I would be inclined to always add tests, the more the better, but given other reviews I received, this is not necessarily true. I can add tests, but they will require a few mock classes in order not to be trivial. Let me know if this is welcome or not. Lacking tests, I fixed this algorithm based on what it could logically do. Since the point is finding the set of rates that are acceptable for all the components in the session, and that reverse rates are represented with negative numbers, it felt right to implement the algorithm that I described above (compute the maximum absolute value when dealing with slowest rates and the minimum absolute value when dealing with fastest rates). Also, this new algorithm makes Deep Rock Galactic working, which it didn't before. Giovanni.
On 5/28/21 12:28 PM, Giovanni Mascellani wrote:
Hi,
Il 28/05/21 10:37, Nikolay Sivov ha scritto:
These two changes (or three) changes are separate. Second changes default even when none of components support rate change. How did you test this?
I don't understand why they should be separate. This piece of code is basically computing the minimum or the maximum between a bunch of values.
There were two bugs: first one is that 0.0 is a valid initialization value when you're computing a maximum, but not when you are computing a minimum (if you iteratively compute min() starting from 0.0, the result will always be 0.0).
Second bug is that you have compute the maximum and minimum in absolute value. For example, if a session has a source with fastest reverse rate -100.0 and another with -20.0, the global fastest should be the minimum in absolute value, which is -20.0, and not -100.0.
My patch fixes two bugs, but they are in the same algorithm, so it felt right to have them together.
As for the tests, again, it's never clear to me when it's appropriate to have tests and when not. I would be inclined to always add tests, the more the better, but given other reviews I received, this is not necessarily true. I can add tests, but they will require a few mock classes in order not to be trivial. Let me know if this is welcome or not.
I don't necessarily mean tests integrated in wine tests. Just something that I can run to see what happens.
Lacking tests, I fixed this algorithm based on what it could logically do. Since the point is finding the set of rates that are acceptable for all the components in the session, and that reverse rates are represented with negative numbers, it felt right to implement the algorithm that I described above (compute the maximum absolute value when dealing with slowest rates and the minimum absolute value when dealing with fastest rates). Also, this new algorithm makes Deep Rock Galactic working, which it didn't before.
Giovanni.
participants (3)
-
Giovanni Mascellani -
Marvin -
Nikolay Sivov