[PATCH 0/3] MR5281: Preserve topology id of the original topology in the resolved topology when calling IMFTopoLoader::Load
This merge request is related to MR !4443 -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5281
From: Santino Mazza <smazza(a)codeweavers.com> --- dlls/mf/tests/mf.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/dlls/mf/tests/mf.c b/dlls/mf/tests/mf.c index c05482778df..58688c0cfba 100644 --- a/dlls/mf/tests/mf.c +++ b/dlls/mf/tests/mf.c @@ -3951,7 +3951,7 @@ static void test_topology_loader(void) IMFTopoLoader *loader; IUnknown *node_object; WORD node_count; - TOPOID node_id; + TOPOID node_id, oldtopoid, newtopoid; DWORD index; HRESULT hr; BOOL ret; @@ -4177,6 +4177,11 @@ static void test_topology_loader(void) } else if (test->expected_result == S_OK) { + IMFTopology_GetTopologyID(topology, &oldtopoid); + IMFTopology_GetTopologyID(full_topology, &newtopoid); + todo_wine ok(oldtopoid == newtopoid, "Expected the same topology id. %llu == %llu\n", oldtopoid, newtopoid); + ok(topology != full_topology, "Expected a different object for the resolved topology.\n"); + hr = IMFTopology_GetCount(full_topology, &count); ok(hr == S_OK, "Failed to get attribute count, hr %#lx.\n", hr); todo_wine @@ -4324,6 +4329,9 @@ todo_wine { hr = IMFTopoLoader_Load(loader, full_topology, &topology2, NULL); ok(hr == S_OK, "Failed to resolve topology, hr %#lx.\n", hr); ok(full_topology != topology2, "Unexpected instance.\n"); + IMFTopology_GetTopologyID(topology2, &oldtopoid); + IMFTopology_GetTopologyID(full_topology, &newtopoid); + todo_wine ok(oldtopoid == newtopoid, "Expected the same topology id. %llu == %llu\n", oldtopoid, newtopoid); hr = IMFTopology_GetUINT32(topology2, &IID_IMFTopology, &value); ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5281
From: Santino Mazza <smazza(a)codeweavers.com> --- dlls/mf/tests/mf.c | 152 ++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 150 insertions(+), 2 deletions(-) diff --git a/dlls/mf/tests/mf.c b/dlls/mf/tests/mf.c index 58688c0cfba..14a66bc9f57 100644 --- a/dlls/mf/tests/mf.c +++ b/dlls/mf/tests/mf.c @@ -2006,9 +2006,10 @@ static HRESULT wait_media_event_(int line, IMFMediaSession *session, IMFAsyncCal return status; } -#define wait_media_event_until_blocking(a, b, c, d, e) wait_media_event_until_blocking_(__LINE__, a, b, c, d, e) +#define get_media_event(a, b, c, d, e, f) wait_media_event_until_blocking_(__LINE__, a, b, c, d, e, f) +#define wait_media_event_until_blocking(a, b, c, d, e) wait_media_event_until_blocking_(__LINE__, a, b, c, d, e, NULL) static HRESULT wait_media_event_until_blocking_(int line, IMFMediaSession *session, IMFAsyncCallback *callback, - MediaEventType expect_type, DWORD timeout, PROPVARIANT *value) + MediaEventType expect_type, DWORD timeout, PROPVARIANT *value, IMFMediaEvent **media_event) { struct test_callback *impl = impl_from_IMFAsyncCallback(callback); MediaEventType type; @@ -2016,6 +2017,9 @@ static HRESULT wait_media_event_until_blocking_(int line, IMFMediaSession *sessi DWORD ret; GUID guid; + if (media_event) + *media_event = NULL; + do { hr = IMFMediaSession_BeginGetEvent(session, &impl->IMFAsyncCallback_iface, (IUnknown *)session); @@ -2038,6 +2042,12 @@ static HRESULT wait_media_event_until_blocking_(int line, IMFMediaSession *sessi hr = IMFMediaEvent_GetStatus(impl->media_event, &status); ok_(__FILE__, line)(hr == S_OK, "Unexpected hr %#lx.\n", hr); + if (media_event) + { + IMFMediaEvent_AddRef(impl->media_event); + *media_event = impl->media_event; + } + return status; } @@ -3452,6 +3462,143 @@ static void test_media_session_rate_control(void) ok(hr == S_OK, "Shutdown failure, hr %#lx.\n", hr); } +#define check_topologyid(a, b) check_topologyid_(__LINE__, a, b) +static void check_topologyid_(unsigned int line, IMFMediaEvent *event, TOPOID expected_topoid) +{ + PROPVARIANT pv; + TOPOID topoid; + IMFTopology *topology; + + IMFMediaEvent_GetValue(event, &pv); + IUnknown_QueryInterface(pv.punkVal, &IID_IMFTopology, (void**)&topology); + IMFTopology_GetTopologyID(topology, &topoid); + ok_(__FILE__, line)(topoid == expected_topoid, "Expected topoid %I64u, got %I64u\n", expected_topoid, topoid); + IMFTopology_Release(topology); +} + +#define check_topostatus(a, b) check_topostatus_(__LINE__, a, b) +static void check_topostatus_(unsigned int line, IMFMediaEvent *event, UINT32 expected_status) +{ + UINT32 topostatus; + IMFMediaEvent_GetUINT32(event, &MF_EVENT_TOPOLOGY_STATUS, &topostatus); + ok_(__FILE__, line)(topostatus == expected_status, "Expected MF_TOPOSTATUS_READY got %d\n", topostatus); +} + +static IMFTopology *create_topology_from_file(LPCWSTR file, LPCWSTR mime) +{ + IMFTopologyNode *src_node, *sink_node; + IMFPresentationDescriptor *pd; + IMFActivate *sar_activate; + IMFStreamDescriptor *sd; + IMFMediaSource *source; + IMFTopology *topology; + BOOL selected; + HRESULT hr; + + if(!(source = create_media_source(file, mime))) + return NULL; + + hr = MFCreateTopology(&topology); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + + hr = MFCreateTopologyNode(MF_TOPOLOGY_SOURCESTREAM_NODE, &src_node); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + + IMFMediaSource_CreatePresentationDescriptor(source, &pd); + hr = IMFPresentationDescriptor_GetStreamDescriptorByIndex(pd, 0, &selected, &sd); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + init_source_node(source, -1, src_node, pd, sd); + + hr = MFCreateTopologyNode(MF_TOPOLOGY_OUTPUT_NODE, &sink_node); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + + hr = MFCreateAudioRendererActivate(&sar_activate); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + + IMFTopologyNode_SetObject(sink_node, (IUnknown *)sar_activate); + + IMFTopology_AddNode(topology, src_node); + IMFTopology_AddNode(topology, sink_node); + IMFTopologyNode_ConnectOutput(src_node, 0, sink_node, 0); + + IMFTopologyNode_Release(src_node); + IMFTopologyNode_Release(sink_node); + IMFActivate_Release(sar_activate); + IMFPresentationDescriptor_Release(pd); + IMFStreamDescriptor_Release(sd); + + return topology; +} + +static void test_media_session_topologies(void) +{ + IMFTopology *topology; + IMFAsyncCallback *callback; + IMFMediaSession *session; + IMFMediaEvent *event; + TOPOID topoid; + PROPVARIANT pv; + HRESULT hr; + + hr = MFStartup(MF_VERSION, MFSTARTUP_FULL); + ok(hr == S_OK, "Startup failure, hr %#lx.\n", hr); + + if (!(topology = create_topology_from_file(L"mp3encdata.bin", L"audio/mpeg"))) + { + win_skip("MP3 not supported.\n"); + MFShutdown(); + return; + } + + IMFTopology_GetTopologyID(topology, &topoid); + + callback = create_test_callback(TRUE); + hr = MFCreateMediaSession(NULL, &session); + ok(hr == S_OK, "Failed to create media session, hr %#lx.\n", hr); + + hr = IMFMediaSession_SetTopology(session, 0, topology); + ok(hr == S_OK, "Unexpected hr %#lx\n", hr); + hr = wait_media_event(session, callback, MESessionTopologySet, 1000, &pv); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + ok(pv.vt == VT_UNKNOWN, "got vt %u\n", pv.vt); + ok(pv.punkVal != (IUnknown *)topology, "got punkVal %p\n", pv.punkVal); + PropVariantClear(&pv); + + hr = get_media_event(session, callback, MESessionTopologyStatus, 1000, &pv, &event); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + todo_wine check_topologyid(event, topoid); + check_topostatus(event, MF_TOPOSTATUS_READY); + IMFMediaEvent_Release(event); + + pv.vt = VT_EMPTY; + hr = IMFMediaSession_Start(session, &GUID_NULL, &pv); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + hr = wait_media_event(session, callback, MESessionStarted, 1000, &pv); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + ok(pv.vt == VT_EMPTY, "got vt %u\n", pv.vt); + ok(pv.punkVal != (IUnknown *)topology, "got punkVal %p\n", pv.punkVal); + PropVariantClear(&pv); + + hr = get_media_event(session, callback, MESessionTopologyStatus, 1000, &pv, &event); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + todo_wine check_topologyid(event, topoid); + check_topostatus(event, MF_TOPOSTATUS_ENDED); + IMFMediaEvent_Release(event); + + hr = wait_media_event(session, callback, MESessionEnded, 1000, &pv); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + ok(pv.vt == VT_EMPTY, "got vt %u\n", pv.vt); + ok(pv.punkVal != (IUnknown *)topology, "got punkVal %p\n", pv.punkVal); + PropVariantClear(&pv); + + IMFMediaSession_Shutdown(session); + IMFMediaSession_Release(session); + IMFAsyncCallback_Release(callback); + IMFTopology_Release(topology); + + MFShutdown(); +} + struct test_grabber_callback { IMFSampleGrabberSinkCallback IMFSampleGrabberSinkCallback_iface; @@ -8259,6 +8406,7 @@ START_TEST(mf) test_media_session(); test_media_session_events(); test_media_session_rate_control(); + test_media_session_topologies(); test_MFShutdownObject(); test_presentation_clock(); test_sample_grabber(); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5281
From: Santino Mazza <smazza(a)codeweavers.com> --- dlls/mf/mf_private.h | 1 + dlls/mf/tests/mf.c | 8 ++++---- dlls/mf/topology.c | 21 ++++++++++++++------- dlls/mf/topology_loader.c | 4 +++- 4 files changed, 22 insertions(+), 12 deletions(-) diff --git a/dlls/mf/mf_private.h b/dlls/mf/mf_private.h index bbfadaee5d8..2790de5d64b 100644 --- a/dlls/mf/mf_private.h +++ b/dlls/mf/mf_private.h @@ -118,6 +118,7 @@ extern HRESULT urlmon_scheme_handler_construct(REFIID riid, void **obj); extern BOOL mf_is_sample_copier_transform(IMFTransform *transform); extern BOOL mf_is_sar_sink(IMFMediaSink *sink); +extern HRESULT create_topology_with_id(TOPOID id, IMFTopology **topology); extern HRESULT topology_node_get_object(IMFTopologyNode *node, REFIID riid, void **obj); extern HRESULT topology_node_get_type_handler(IMFTopologyNode *node, DWORD stream, BOOL output, IMFMediaTypeHandler **handler); extern HRESULT topology_node_init_media_type(IMFTopologyNode *node, DWORD stream, BOOL output, IMFMediaType **type); diff --git a/dlls/mf/tests/mf.c b/dlls/mf/tests/mf.c index 14a66bc9f57..bb5a0b1f0b3 100644 --- a/dlls/mf/tests/mf.c +++ b/dlls/mf/tests/mf.c @@ -3566,7 +3566,7 @@ static void test_media_session_topologies(void) hr = get_media_event(session, callback, MESessionTopologyStatus, 1000, &pv, &event); ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); - todo_wine check_topologyid(event, topoid); + check_topologyid(event, topoid); check_topostatus(event, MF_TOPOSTATUS_READY); IMFMediaEvent_Release(event); @@ -3581,7 +3581,7 @@ static void test_media_session_topologies(void) hr = get_media_event(session, callback, MESessionTopologyStatus, 1000, &pv, &event); ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); - todo_wine check_topologyid(event, topoid); + check_topologyid(event, topoid); check_topostatus(event, MF_TOPOSTATUS_ENDED); IMFMediaEvent_Release(event); @@ -4326,7 +4326,7 @@ static void test_topology_loader(void) { IMFTopology_GetTopologyID(topology, &oldtopoid); IMFTopology_GetTopologyID(full_topology, &newtopoid); - todo_wine ok(oldtopoid == newtopoid, "Expected the same topology id. %llu == %llu\n", oldtopoid, newtopoid); + ok(oldtopoid == newtopoid, "Expected the same topology id. %llu == %llu\n", oldtopoid, newtopoid); ok(topology != full_topology, "Expected a different object for the resolved topology.\n"); hr = IMFTopology_GetCount(full_topology, &count); @@ -4478,7 +4478,7 @@ todo_wine { ok(full_topology != topology2, "Unexpected instance.\n"); IMFTopology_GetTopologyID(topology2, &oldtopoid); IMFTopology_GetTopologyID(full_topology, &newtopoid); - todo_wine ok(oldtopoid == newtopoid, "Expected the same topology id. %llu == %llu\n", oldtopoid, newtopoid); + ok(oldtopoid == newtopoid, "Expected the same topology id. %llu == %llu\n", oldtopoid, newtopoid); hr = IMFTopology_GetUINT32(topology2, &IID_IMFTopology, &value); ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); diff --git a/dlls/mf/topology.c b/dlls/mf/topology.c index f09a9c87a46..56ce6a8f642 100644 --- a/dlls/mf/topology.c +++ b/dlls/mf/topology.c @@ -866,16 +866,12 @@ static TOPOID topology_generate_id(void) return next_topology_id; } -/*********************************************************************** - * MFCreateTopology (mf.@) - */ -HRESULT WINAPI MFCreateTopology(IMFTopology **topology) +/* If no topoid is provided a new one will be generated. */ +HRESULT create_topology_with_id(TOPOID id, IMFTopology **topology) { struct topology *object; HRESULT hr; - TRACE("%p.\n", topology); - if (!topology) return E_POINTER; @@ -892,13 +888,24 @@ HRESULT WINAPI MFCreateTopology(IMFTopology **topology) return hr; } - object->id = topology_generate_id(); + if (!id) id = topology_generate_id(); + object->id = id; *topology = &object->IMFTopology_iface; return S_OK; } +/*********************************************************************** + * MFCreateTopology (mf.@) + */ +HRESULT WINAPI MFCreateTopology(IMFTopology **topology) +{ + TRACE("%p.\n", topology); + + return create_topology_with_id(0, topology); +} + static HRESULT WINAPI topology_node_QueryInterface(IMFTopologyNode *iface, REFIID riid, void **out) { struct topology_node *node = impl_from_IMFTopologyNode(iface); diff --git a/dlls/mf/topology_loader.c b/dlls/mf/topology_loader.c index 11d0162d94f..3a4cb7caa52 100644 --- a/dlls/mf/topology_loader.c +++ b/dlls/mf/topology_loader.c @@ -721,6 +721,7 @@ static HRESULT WINAPI topology_loader_Load(IMFTopoLoader *iface, IMFTopology *in unsigned short i = 0; IMFStreamSink *sink; IUnknown *object; + TOPOID topoid; HRESULT hr = E_FAIL; FIXME("iface %p, input_topology %p, ret_topology %p, current_topology %p stub!\n", @@ -763,7 +764,8 @@ static HRESULT WINAPI topology_loader_Load(IMFTopoLoader *iface, IMFTopology *in return hr; } - if (FAILED(hr = MFCreateTopology(&output_topology))) + IMFTopology_GetTopologyID(input_topology, &topoid); + if (FAILED(hr = create_topology_with_id(topoid, &output_topology))) return hr; IMFTopology_CopyAllItems(input_topology, (IMFAttributes *)output_topology); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5281
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=143964 Your paranoid android. === w11pro64_amd (64 bit report) === mf: mf.c:3562: Test failed: Unexpected hr 0xc00d36fa. mf.c:3564: Test failed: got punkVal 00000000010C2EE0 mf.c:3568: Test failed: Unexpected hr 0x102. === debian11b (64 bit WoW report) === secur32: schannel.c:538: Test failed: cert_cnt = 2
Nikolay Sivov (@nsivov) commented about dlls/mf/topology.c:
+ object->id = id;
*topology = &object->IMFTopology_iface;
return S_OK; }
+/*********************************************************************** + * MFCreateTopology (mf.@) + */ +HRESULT WINAPI MFCreateTopology(IMFTopology **topology) +{ + TRACE("%p.\n", topology); + + return create_topology_with_id(0, topology); +} Just call it with a new topoid value, no need to complicate this.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5281#note_64783
Nikolay Sivov (@nsivov) commented about dlls/mf/tests/mf.c:
test_media_session(); test_media_session_events(); test_media_session_rate_control(); + test_media_session_topologies();
I don't think we need this test. It's enough to see what Load() does. For example, if using custom topology loader that does not preserve topoid results in a new id in SetTopology(), then we don't learn much from this test. I suggest to drop it. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5281#note_64784
On Thu Mar 14 19:35:23 2024 +0000, Nikolay Sivov wrote:
Just call it with a new topoid value, no need to complicate this. I have a problem with this, if I generate a new topoid, and create_topology_with_id fails, the next_topology_id global variable will stay incremented causing tests to fail. Another solution might be to return a `struct topology` pointer instead of `IMFTopology` so the caller sets the topoid directly.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5281#note_64925
On Fri Mar 15 20:16:23 2024 +0000, Santino Mazza wrote:
I have a problem with this, if I generate a new topoid, and create_topology_with_id fails, the next_topology_id global variable will stay incremented causing tests to fail. Another solution might be to return a `struct topology` pointer instead of `IMFTopology` so the caller sets the topoid directly. If you mean allocation failure, it's not a problem, because this could fail at any point after topology is created, and id will be incremented next time. Regarding obvious failure on null argument, simply move the check to MFCreateTopology(). Also I'd rename create_topology_with_id() to create_topology().
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5281#note_64927
participants (4)
-
Marvin -
Nikolay Sivov (＠nsivov) -
Santino Mazza
-
Santino Mazza (＠tati1454)