[PATCH v2 0/3] MR2675: mfreadwrite: Fix an address of operator typo.
-- v2: mfreadwrite: Introduce create_sink_writer_from_factory. mfreadwrite: Fix an address of operator typo. mfreadwrite/tests: Add more tests for MFCreateSinkWriterFromURL. https://gitlab.winehq.org/wine/wine/-/merge_requests/2675
From: Ziqing Hui <zhui(a)codeweavers.com> The tests shows that, for sink writer created by MFCreateSinkWriterFromURL, we are not able to grab the media sink object immediately after the creation, unless the media sink object can be created without setting media type (MP3, ASF). --- dlls/mfreadwrite/tests/mfplat.c | 116 +++++++++++++++++++++++++++++--- 1 file changed, 108 insertions(+), 8 deletions(-) diff --git a/dlls/mfreadwrite/tests/mfplat.c b/dlls/mfreadwrite/tests/mfplat.c index 41b2a033a59..27a503a8025 100644 --- a/dlls/mfreadwrite/tests/mfplat.c +++ b/dlls/mfreadwrite/tests/mfplat.c @@ -1415,19 +1415,113 @@ done: DestroyWindow(window); } -static void test_sink_writer(void) +static void test_sink_writer_create(const char *extension, BOOL todo) { + const GUID *container_type = &GUID_NULL; + WCHAR extension_w[MAX_PATH]; + WCHAR tmp_file[MAX_PATH]; IMFSinkWriter *writer; + IMFByteStream *stream; + IMFAttributes *attr; + IMFMediaSink *sink; + unsigned int i; HRESULT hr; - hr = MFCreateSinkWriterFromURL(NULL, NULL, NULL, NULL); - ok(hr == E_INVALIDARG, "Unexpected hr %#lx.\n", hr); + static const struct extension_map + { + const char *extension; + const GUID *container_type; + } ext_map[] = + { + {"mp4", &MFTranscodeContainerType_MPEG4}, + {"mp3", &MFTranscodeContainerType_MP3}, + {"wav", &MFTranscodeContainerType_WAVE}, + {"avi", &MFTranscodeContainerType_AVI}, + {"asf", &MFTranscodeContainerType_ASF}, + }; + + struct create_sink_writer_from_url_test + { + const WCHAR *url; + IMFByteStream **stream; + IMFAttributes **attr; + IMFSinkWriter **writer; + HRESULT hr; + } create_sink_writer_from_url_tests[] = + { + {NULL, NULL, NULL, NULL, E_INVALIDARG}, + {NULL, NULL, NULL, &writer, E_INVALIDARG}, + {NULL, NULL, &attr, &writer, E_INVALIDARG}, + {NULL, &stream, NULL, &writer, E_INVALIDARG}, + {NULL, &stream, &attr, &writer, S_OK}, + {tmp_file, NULL, NULL, &writer, S_OK}, + {tmp_file, NULL, &attr, &writer, S_OK}, + {tmp_file, &stream, NULL, &writer, S_OK}, + {tmp_file, &stream, &attr, &writer, S_OK}, + }; + + winetest_push_context("%s", extension); + + /* Get temp file name. */ + GetTempPathW(ARRAY_SIZE(tmp_file), tmp_file); + MultiByteToWideChar(CP_ACP, 0, extension, -1, extension_w, ARRAY_SIZE(extension_w)); + wcscat(tmp_file, L"tmp."); + wcscat(tmp_file, extension_w); + + /* Create attribute and byte stream. */ + for (i = 0; i < ARRAY_SIZE(ext_map); ++i) + { + if (!strcmp(extension, ext_map[i].extension)) + { + container_type = ext_map[i].container_type; + break; + } + } + hr = MFCreateAttributes(&attr, 1); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + hr = IMFAttributes_SetGUID(attr, &MF_TRANSCODE_CONTAINERTYPE, container_type); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + hr = MFCreateTempFile(MF_ACCESSMODE_READWRITE, MF_OPENMODE_DELETE_IF_EXIST, MF_FILEFLAGS_NONE, &stream); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); - writer = (void *)0xdeadbeef; - hr = MFCreateSinkWriterFromURL(NULL, NULL, NULL, &writer); - ok(hr == E_INVALIDARG, "Unexpected hr %#lx.\n", hr); - ok(!writer, "Unexpected pointer %p.\n", writer); + /* Test MFCreateSinkWriterFromURL. */ + for (i = 0; i < ARRAY_SIZE(create_sink_writer_from_url_tests); ++i) + { + struct create_sink_writer_from_url_test *test = &create_sink_writer_from_url_tests[i]; + IMFByteStream *test_stream = test->stream ? *test->stream : NULL; + IMFAttributes *test_attr = test->attr ? *test->attr : NULL; + winetest_push_context("Test %u", i); + + writer = (void *)0xdeadbeef; + hr = MFCreateSinkWriterFromURL(test->url, test_stream, test_attr, test->writer); + todo_wine_if(test->hr == S_OK && todo) + { + ok(hr == test->hr, "Unexpected hr %#lx, expected %#lx.\n", hr, test->hr); + if (test->writer) + ok((test->hr == S_OK && writer) || (test->hr != S_OK && !writer), "Unexpected writer pointer %p.\n", writer); + } + if (hr == S_OK) + { + hr = IMFSinkWriter_GetServiceForStream(writer, MF_SINK_WRITER_MEDIASINK, + &GUID_NULL, &IID_IMFMediaSink, (void **)&sink); + if (IsEqualGUID(container_type, &MFTranscodeContainerType_ASF) + || IsEqualGUID(container_type, &MFTranscodeContainerType_MP3)) + { + ok(hr == S_OK, "Unexpected hr %#lx,\n", hr); + IMFMediaSink_Release(sink); + } + else + { + ok(hr == MF_E_UNSUPPORTED_SERVICE, "Unexpected hr %#lx.\n", hr); + } + IMFSinkWriter_Release(writer); + } + + winetest_pop_context(); + } + + /* Test MFCreateSinkWriterFromMediaSink. */ hr = MFCreateSinkWriterFromMediaSink(NULL, NULL, NULL); ok(hr == E_INVALIDARG, "Unexpected hr %#lx.\n", hr); @@ -1435,6 +1529,8 @@ static void test_sink_writer(void) hr = MFCreateSinkWriterFromMediaSink(NULL, NULL, &writer); ok(hr == E_INVALIDARG, "Unexpected hr %#lx.\n", hr); ok(!writer, "Unexpected pointer %p.\n", writer); + + winetest_pop_context(); } START_TEST(mfplat) @@ -1451,7 +1547,11 @@ START_TEST(mfplat) test_source_reader("test.mp4", true); test_source_reader_from_media_source(); test_reader_d3d9(); - test_sink_writer(); + + test_sink_writer_create("wav", TRUE); + test_sink_writer_create("mp3", TRUE); + test_sink_writer_create("mp4", TRUE); + test_sink_writer_create("asf", TRUE); hr = MFShutdown(); ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2675
From: Ziqing Hui <zhui(a)codeweavers.com> --- dlls/mfreadwrite/writer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/mfreadwrite/writer.c b/dlls/mfreadwrite/writer.c index aaf972734c4..83b0442b20e 100644 --- a/dlls/mfreadwrite/writer.c +++ b/dlls/mfreadwrite/writer.c @@ -1070,7 +1070,7 @@ static HRESULT sink_writer_get_sink_factory_class(const WCHAR *url, IMFAttribute for (i = 0; i < ARRAY_SIZE(class_map); ++i) { - if (IsEqualGUID(&container, &class_map[i].container)) + if (IsEqualGUID(&container, class_map[i].container)) { *clsid = *class_map[i].clsid; return S_OK; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2675
From: Ziqing Hui <zhui(a)codeweavers.com> For media sink which can not be created without setting media types, instead of creating the media sink during sink writer creation, we store the factory and bytestream objects and create the media sink later. --- dlls/mfreadwrite/tests/mfplat.c | 2 +- dlls/mfreadwrite/writer.c | 94 +++++++++++++++++++++++---------- 2 files changed, 67 insertions(+), 29 deletions(-) diff --git a/dlls/mfreadwrite/tests/mfplat.c b/dlls/mfreadwrite/tests/mfplat.c index 27a503a8025..da9966e78fc 100644 --- a/dlls/mfreadwrite/tests/mfplat.c +++ b/dlls/mfreadwrite/tests/mfplat.c @@ -1548,7 +1548,7 @@ START_TEST(mfplat) test_source_reader_from_media_source(); test_reader_d3d9(); - test_sink_writer_create("wav", TRUE); + test_sink_writer_create("wav", FALSE); test_sink_writer_create("mp3", TRUE); test_sink_writer_create("mp4", TRUE); test_sink_writer_create("asf", TRUE); diff --git a/dlls/mfreadwrite/writer.c b/dlls/mfreadwrite/writer.c index 83b0442b20e..a3d105c0994 100644 --- a/dlls/mfreadwrite/writer.c +++ b/dlls/mfreadwrite/writer.c @@ -71,6 +71,9 @@ struct sink_writer IMFAsyncCallback events_callback; LONG refcount; + IMFSinkClassFactory *factory; + IMFByteStream *bytestream; + struct { struct stream *items; @@ -223,6 +226,10 @@ static ULONG WINAPI sink_writer_Release(IMFSinkWriter *iface) if (!refcount) { + if (writer->factory) + IMFSinkClassFactory_Release(writer->factory); + if (writer->bytestream) + IMFByteStream_Release(writer->bytestream); if (writer->clock) IMFPresentationClock_Release(writer->clock); if (writer->sink) @@ -942,6 +949,8 @@ static HRESULT sink_writer_initialize_existing_streams(struct sink_writer *write DWORD count = 0, i, index; HRESULT hr; + IMFMediaSink_AddRef(writer->sink = sink); + if (FAILED(hr = IMFMediaSink_GetStreamSinkCount(sink, &count))) return hr; if (!count) return S_OK; @@ -956,10 +965,33 @@ static HRESULT sink_writer_initialize_existing_streams(struct sink_writer *write return hr; } +static HRESULT create_sink_writer(IMFAttributes *attributes, struct sink_writer **out) +{ + struct sink_writer *writer; + + if (!(writer = calloc(1, sizeof(*writer)))) + return E_OUTOFMEMORY; + + writer->IMFSinkWriter_iface.lpVtbl = &sink_writer_vtbl; + writer->events_callback.lpVtbl = &sink_writer_events_callback_vtbl; + writer->refcount = 1; + writer->stats.cb = sizeof(writer->stats); + InitializeCriticalSection(&writer->cs); + + if (attributes) + { + IMFAttributes_GetUnknown(attributes, &MF_SINK_WRITER_ASYNC_CALLBACK, + &IID_IMFSinkWriterCallback, (void **)&writer->callback); + } + + *out = writer; + return S_OK; +} + HRESULT create_sink_writer_from_sink(IMFMediaSink *sink, IMFAttributes *attributes, REFIID riid, void **out) { - struct sink_writer *object; + struct sink_writer *writer; HRESULT hr; *out = NULL; @@ -967,31 +999,36 @@ HRESULT create_sink_writer_from_sink(IMFMediaSink *sink, IMFAttributes *attribut if (!sink) return E_INVALIDARG; - if (!(object = calloc(1, sizeof(*object)))) - return E_OUTOFMEMORY; - - object->IMFSinkWriter_iface.lpVtbl = &sink_writer_vtbl; - object->events_callback.lpVtbl = &sink_writer_events_callback_vtbl; - object->refcount = 1; - object->sink = sink; - IMFMediaSink_AddRef(sink); - object->stats.cb = sizeof(object->stats); - InitializeCriticalSection(&object->cs); + if (FAILED(hr = create_sink_writer(attributes, &writer))) + return hr; - if (attributes) + if (FAILED(hr = sink_writer_initialize_existing_streams(writer, sink))) { - IMFAttributes_GetUnknown(attributes, &MF_SINK_WRITER_ASYNC_CALLBACK, - &IID_IMFSinkWriterCallback, (void **)&object->callback); + IMFSinkWriter_Release(&writer->IMFSinkWriter_iface); + return hr; } - if (FAILED(hr = sink_writer_initialize_existing_streams(object, sink))) - { - IMFSinkWriter_Release(&object->IMFSinkWriter_iface); + hr = IMFSinkWriter_QueryInterface(&writer->IMFSinkWriter_iface, riid, out); + IMFSinkWriter_Release(&writer->IMFSinkWriter_iface); + return hr; +} + +HRESULT create_sink_writer_from_factory(IMFSinkClassFactory *factory, IMFByteStream *bytestream, + IMFAttributes *attributes, REFIID riid, void **out) +{ + struct sink_writer *writer; + HRESULT hr; + + *out = NULL; + + if (FAILED(hr = create_sink_writer(attributes, &writer))) return hr; - } - hr = IMFSinkWriter_QueryInterface(&object->IMFSinkWriter_iface, riid, out); - IMFSinkWriter_Release(&object->IMFSinkWriter_iface); + IMFSinkClassFactory_AddRef(writer->factory = factory); + IMFByteStream_AddRef(writer->bytestream = bytestream); + + hr = IMFSinkWriter_QueryInterface(&writer->IMFSinkWriter_iface, riid, out); + IMFSinkWriter_Release(&writer->IMFSinkWriter_iface); return hr; } @@ -1112,16 +1149,17 @@ HRESULT create_sink_writer_from_url(const WCHAR *url, IMFByteStream *bytestream, } hr = IMFSinkClassFactory_CreateMediaSink(factory, bytestream, NULL, NULL, &sink); - IMFSinkClassFactory_Release(factory); - IMFByteStream_Release(bytestream); - if (FAILED(hr)) + if (SUCCEEDED(hr)) { - WARN("Failed to create a sink, hr %#lx.\n", hr); - return hr; + hr = create_sink_writer_from_sink(sink, attributes, riid, out); + IMFMediaSink_Release(sink); } - - hr = create_sink_writer_from_sink(sink, attributes, riid, out); - IMFMediaSink_Release(sink); + else + { + hr = create_sink_writer_from_factory(factory, bytestream, attributes, riid, out); + } + IMFSinkClassFactory_Release(factory); + IMFByteStream_Release(bytestream); return hr; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2675
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=132095 Your paranoid android. === w7u_2qxl (32 bit report) === mfreadwrite: mfplat.c:1499: Test failed: wav: Test 4: Unexpected hr 0xc00d36fa, expected 0. mfplat.c:1501: Test failed: wav: Test 4: Unexpected writer pointer 00000000. mfplat.c:1499: Test failed: wav: Test 5: Unexpected hr 0xc00d36d5, expected 0. mfplat.c:1501: Test failed: wav: Test 5: Unexpected writer pointer 00000000. mfplat.c:1499: Test failed: wav: Test 6: Unexpected hr 0xc00d36fa, expected 0. mfplat.c:1501: Test failed: wav: Test 6: Unexpected writer pointer 00000000. mfplat.c:1499: Test failed: wav: Test 7: Unexpected hr 0xc00d36d5, expected 0. mfplat.c:1501: Test failed: wav: Test 7: Unexpected writer pointer 00000000. mfplat.c:1499: Test failed: wav: Test 8: Unexpected hr 0xc00d36fa, expected 0. mfplat.c:1501: Test failed: wav: Test 8: Unexpected writer pointer 00000000. === w7u_adm (32 bit report) === mfreadwrite: mfplat.c:1499: Test failed: wav: Test 4: Unexpected hr 0xc00d36fa, expected 0. mfplat.c:1501: Test failed: wav: Test 4: Unexpected writer pointer 00000000. mfplat.c:1499: Test failed: wav: Test 5: Unexpected hr 0xc00d36d5, expected 0. mfplat.c:1501: Test failed: wav: Test 5: Unexpected writer pointer 00000000. mfplat.c:1499: Test failed: wav: Test 6: Unexpected hr 0xc00d36fa, expected 0. mfplat.c:1501: Test failed: wav: Test 6: Unexpected writer pointer 00000000. mfplat.c:1499: Test failed: wav: Test 7: Unexpected hr 0xc00d36d5, expected 0. mfplat.c:1501: Test failed: wav: Test 7: Unexpected writer pointer 00000000. mfplat.c:1499: Test failed: wav: Test 8: Unexpected hr 0xc00d36fa, expected 0. mfplat.c:1501: Test failed: wav: Test 8: Unexpected writer pointer 00000000. === w7u_el (32 bit report) === mfreadwrite: mfplat.c:1499: Test failed: wav: Test 4: Unexpected hr 0xc00d36fa, expected 0. mfplat.c:1501: Test failed: wav: Test 4: Unexpected writer pointer 00000000. mfplat.c:1499: Test failed: wav: Test 5: Unexpected hr 0xc00d36d5, expected 0. mfplat.c:1501: Test failed: wav: Test 5: Unexpected writer pointer 00000000. mfplat.c:1499: Test failed: wav: Test 6: Unexpected hr 0xc00d36fa, expected 0. mfplat.c:1501: Test failed: wav: Test 6: Unexpected writer pointer 00000000. mfplat.c:1499: Test failed: wav: Test 7: Unexpected hr 0xc00d36d5, expected 0. mfplat.c:1501: Test failed: wav: Test 7: Unexpected writer pointer 00000000. mfplat.c:1499: Test failed: wav: Test 8: Unexpected hr 0xc00d36fa, expected 0. mfplat.c:1501: Test failed: wav: Test 8: Unexpected writer pointer 00000000. === w7pro64 (64 bit report) === mfreadwrite: mfplat.c:1499: Test failed: wav: Test 4: Unexpected hr 0xc00d36fa, expected 0. mfplat.c:1501: Test failed: wav: Test 4: Unexpected writer pointer 0000000000000000. mfplat.c:1499: Test failed: wav: Test 5: Unexpected hr 0xc00d36d5, expected 0. mfplat.c:1501: Test failed: wav: Test 5: Unexpected writer pointer 0000000000000000. mfplat.c:1499: Test failed: wav: Test 6: Unexpected hr 0xc00d36fa, expected 0. mfplat.c:1501: Test failed: wav: Test 6: Unexpected writer pointer 0000000000000000. mfplat.c:1499: Test failed: wav: Test 7: Unexpected hr 0xc00d36d5, expected 0. mfplat.c:1501: Test failed: wav: Test 7: Unexpected writer pointer 0000000000000000. mfplat.c:1499: Test failed: wav: Test 8: Unexpected hr 0xc00d36fa, expected 0. mfplat.c:1501: Test failed: wav: Test 8: Unexpected writer pointer 0000000000000000. === debian11 (32 bit report) === user32: win.c:10983: Test failed: GetForegroundWindow() = 00210148
This is going to another direction, unrelated to initial fix. I'm aware that for selected sinks classes are not created right away and logic seems to be hardcoded. But we don't really need to replicate that right now. For the test, I meant to have a call with ".wav" url to validate that CLSID is picked up. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2675#note_30813
On Fri Apr 21 06:45:26 2023 +0000, Nikolay Sivov wrote:
This is going to another direction, unrelated to initial fix. I'm aware that for selected sinks classes are not created right away and logic seems to be hardcoded. But we don't really need to replicate that right now. For the test, I meant to have a call with ".wav" url to validate that CLSID is picked up. I'll remove patch 3 from this MR for now.
Do we keep the test patch(patch 1) in this MR which is much more complicated than you expected? I think we'll need these tests sooner or later. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2675#note_30819
On Fri Apr 21 06:45:26 2023 +0000, Ziqing Hui wrote:
I'll remove patch 3 from this MR for now. Do we keep the test patch(patch 1) in this MR which is much more complicated than you expected? I think we'll need these tests sooner or later. Yes, it's too complicated in my opinion. We only need to implement this delayed object creation behavior if we want our mfreadwrite to be a closer drop-in replacement on Windows, for whatever reason.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2675#note_30820
On Fri Apr 21 06:47:26 2023 +0000, Nikolay Sivov wrote:
Yes, it's too complicated in my opinion. We only need to implement this delayed object creation behavior if we want our mfreadwrite to be a closer drop-in replacement on Windows, for whatever reason. There is a problem: if we don't delay create sink object, I don't know how to create it during writer initialization.
Take WAVE as example: we use IMFSinkClassFactory_CreateMediaSink() to create sink object, and IMFSinkClassFactory_CreateMediaSink() calls MFCreateWAVEMediaSink(). However, we must pass a valid media type argument to MFCreateWAVEMediaSink(), or this function fails. See tests here: https://gitlab.winehq.org/wine/wine/-/blob/master/dlls/mfsrcsnk/tests/mfsrcs... What I mean is, after patch 2, we are still not able to successfully create WAV writter sink. Because for now, we pass a NULL media type to MFCreateWAVEMediaSink(). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2675#note_30822
On Fri Apr 21 06:57:52 2023 +0000, Ziqing Hui wrote:
There is a problem: if we don't delay create sink object, I don't know how to create it during writer initialization. Take WAVE as example: we use IMFSinkClassFactory_CreateMediaSink() to create sink object, and IMFSinkClassFactory_CreateMediaSink() calls MFCreateWAVEMediaSink(). However, we must pass a valid media type argument to MFCreateWAVEMediaSink(), or this function fails. See tests here: https://gitlab.winehq.org/wine/wine/-/blob/master/dlls/mfsrcsnk/tests/mfsrcs... What I mean is, after patch 2, we are still not able to successfully create WAV writter sink. Because for now, we pass a NULL media type to MFCreateWAVEMediaSink(). I see. Let's drop the tests entirely then, and have the fix as is. Delayed creation logic we can have later as a separate MR.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2675#note_30823
participants (4)
-
Marvin -
Nikolay Sivov (＠nsivov) -
Ziqing Hui
-
Ziqing Hui (＠zhui)