[PATCH v5 0/3] MR1046: MfMediaEngine invalid format handling.
-- v5: mfmediaengine: Add a fallback when the set engine format cannot be used to init samplegrabber. mfmediaengine: Only accept engine formats which are render target compatible. https://gitlab.winehq.org/wine/wine/-/merge_requests/1046
From: Bernhard Kölbl <besentv(a)gmail.com> --- dlls/mfmediaengine/tests/mfmediaengine.c | 284 +++++++++++++++++++++++ 1 file changed, 284 insertions(+) diff --git a/dlls/mfmediaengine/tests/mfmediaengine.c b/dlls/mfmediaengine/tests/mfmediaengine.c index 3a5b2bf8253..ed0d1fc1c63 100644 --- a/dlls/mfmediaengine/tests/mfmediaengine.c +++ b/dlls/mfmediaengine/tests/mfmediaengine.c @@ -237,6 +237,7 @@ static ID3D11Device *create_d3d11_device(void) { static const D3D_FEATURE_LEVEL default_feature_level[] = { + D3D_FEATURE_LEVEL_11_1, D3D_FEATURE_LEVEL_11_0, D3D_FEATURE_LEVEL_10_1, D3D_FEATURE_LEVEL_10_0, @@ -1313,6 +1314,288 @@ done: CloseHandle(notify.ready_event); } +struct test_formats_notify +{ + IMFMediaEngineNotify IMFMediaEngineNotify_iface; + + DXGI_FORMAT engine_fmt, output_fmt; + IMFMediaEngineEx *media_engine; + HANDLE playing_event; + HANDLE ready_event; + HRESULT error; +}; + +static HRESULT WINAPI test_formats_notify_QueryInterface(IMFMediaEngineNotify *iface, REFIID riid, void **obj) +{ + if (IsEqualIID(riid, &IID_IUnknown) + || IsEqualIID(riid, &IID_IMFMediaEngineNotify)) + { + *obj = iface; + IMFMediaEngineNotify_AddRef(iface); + return S_OK; + } + + *obj = NULL; + return E_NOINTERFACE; +} + +static ULONG WINAPI test_formats_notify_AddRef(IMFMediaEngineNotify *iface) +{ + return 2; +} + +static ULONG WINAPI test_formats_notify_Release(IMFMediaEngineNotify *iface) +{ + return 1; +} + +static HRESULT WINAPI test_formats_notify_EventNotify(IMFMediaEngineNotify *iface, DWORD event, DWORD_PTR param1, DWORD param2) +{ + struct test_formats_notify *notify = CONTAINING_RECORD(iface, struct test_formats_notify, IMFMediaEngineNotify_iface); + IMFMediaEngineEx *media_engine = notify->media_engine; + DWORD width, height; + HRESULT hr; + BOOL ret; + + winetest_push_context("%x, %x", notify->engine_fmt, notify->output_fmt); + + switch (event) + { + case MF_MEDIA_ENGINE_EVENT_CANPLAY: + hr = IMFMediaEngineEx_Play(media_engine); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + break; + + case MF_MEDIA_ENGINE_EVENT_FORMATCHANGE: + ret = IMFMediaEngineEx_HasVideo(media_engine); + ok(ret, "Unexpected HasVideo %u.\n", ret); + hr = IMFMediaEngineEx_GetNativeVideoSize(media_engine, &width, &height); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + ok(width == 64, "Unexpected width %lu.\n", width); + ok(height == 64, "Unexpected height %lu.\n", height); + break; + + case MF_MEDIA_ENGINE_EVENT_ERROR: + //ok(param2 == MF_E_INVALIDMEDIATYPE || broken(param2 == MF_E_UNSUPPORTED_BYTESTREAM_TYPE), + // "Unexpected error %llu, error2 %#lx\n", param1, param2); + notify->error = param2; + /* fallthrough */ + case MF_MEDIA_ENGINE_EVENT_FIRSTFRAMEREADY: + case MF_MEDIA_ENGINE_EVENT_TIMEUPDATE: + case MF_MEDIA_ENGINE_EVENT_PLAYING: + SetEvent(notify->ready_event); + break; + + } + + winetest_pop_context(); + + return S_OK; +} + +static IMFMediaEngineNotifyVtbl test_formats_notify_vtbl = +{ + test_formats_notify_QueryInterface, + test_formats_notify_AddRef, + test_formats_notify_Release, + test_formats_notify_EventNotify, +}; + + +struct tested_format +{ + const DXGI_FORMAT engine_fmt; + BOOL bad_fmt; + BOOL engine_todo; + const DXGI_FORMAT output_fmt; + HRESULT exp_hr; + HRESULT broken_hr; + BOOL output_todo; +}; + +static void test_formats(void) +{ +#define NON_EXISITING_DXGI_FORMAT 0x80 + + const struct tested_format tested_formats[] = + { + /* Some supported media engine and output texture formats. */ + { + .engine_fmt = DXGI_FORMAT_R32G32B32_FLOAT, .engine_todo = TRUE, + .output_fmt = DXGI_FORMAT_B8G8R8A8_UNORM, .exp_hr = S_OK, + }, + { + .engine_fmt = DXGI_FORMAT_R16G16B16A16_FLOAT, + .output_fmt = DXGI_FORMAT_R10G10B10A2_UNORM, .exp_hr = S_OK, + }, + { + .engine_fmt = DXGI_FORMAT_R16G16B16A16_UINT, .engine_todo = TRUE, + .output_fmt = DXGI_FORMAT_B8G8R8X8_UNORM, .exp_hr = S_OK, + }, + { + .engine_fmt = DXGI_FORMAT_R8G8B8A8_UNORM, + .output_fmt = DXGI_FORMAT_R16G16B16A16_FLOAT, .exp_hr = S_OK, + }, + { + .engine_fmt = DXGI_FORMAT_R8G8B8A8_UNORM, + .output_fmt = DXGI_FORMAT_B8G8R8A8_UNORM_SRGB, .exp_hr = S_OK, .broken_hr = E_INVALIDARG, /* Broken on NVIDIA */ + }, + /* Not supported media engine formats. */ + { + .engine_fmt = DXGI_FORMAT_R32G32B32A32_FLOAT, .bad_fmt = TRUE, .engine_todo = TRUE, + .output_fmt = DXGI_FORMAT_B8G8R8A8_UNORM, .exp_hr = S_OK, + }, + { + .engine_fmt = DXGI_FORMAT_R10G10B10A2_UNORM, + .output_fmt = DXGI_FORMAT_B8G8R8A8_UNORM, .exp_hr = S_OK, + }, + /* Some exotic, ridiculous and non-exisiting media engine formats. */ + { + .engine_fmt = DXGI_FORMAT_D24_UNORM_S8_UINT, .bad_fmt = TRUE, .engine_todo = TRUE, + .output_fmt = DXGI_FORMAT_B8G8R8A8_UNORM, .exp_hr = S_OK, + }, + { + .engine_fmt = DXGI_FORMAT_BC2_UNORM_SRGB, .bad_fmt = TRUE, .engine_todo = TRUE, + .output_fmt = DXGI_FORMAT_B8G8R8A8_UNORM, .exp_hr = S_OK, + }, + { + .engine_fmt = DXGI_FORMAT_NV12, .bad_fmt = TRUE, .engine_todo = TRUE, + .output_fmt = DXGI_FORMAT_B8G8R8A8_UNORM, .exp_hr = S_OK, .output_todo = TRUE, + }, + { + .engine_fmt = NON_EXISITING_DXGI_FORMAT, .bad_fmt = TRUE, .engine_todo = TRUE, + .output_fmt = DXGI_FORMAT_B8G8R8A8_UNORM, .exp_hr = S_OK, + }, + /* Texture format working with NVIDIA and AMD driver, but broken in VMs. */ + { + .engine_fmt = DXGI_FORMAT_B5G5R5A1_UNORM, .engine_todo = TRUE, + .output_fmt = DXGI_FORMAT_R8G8B8A8_UNORM, .exp_hr = S_OK, .broken_hr = MF_E_INVALIDMEDIATYPE, + }, + /* Some unsupported output texture formats. */ + { + .engine_fmt = DXGI_FORMAT_R8G8B8A8_UNORM, + .output_fmt = DXGI_FORMAT_R16G16B16A16_UNORM, .exp_hr = MF_E_INVALIDMEDIATYPE, .output_todo = TRUE, + }, + { + .engine_fmt = DXGI_FORMAT_R8G8B8A8_UNORM, + .output_fmt = DXGI_FORMAT_R11G11B10_FLOAT, .exp_hr = MF_E_INVALIDMEDIATYPE, .output_todo = TRUE, + } + }; + +#undef NON_EXISITING_DXGI_FORMAT + + struct test_formats_notify notify = {{&test_formats_notify_vtbl}}; + WCHAR url[] = {L"i420-64x64.avi"}; + IMFMediaEngineEx *media_engine; + IMFDXGIDeviceManager *manager; + D3D11_TEXTURE2D_DESC desc; + ID3D11Texture2D *texture; + IMFByteStream *stream; + ID3D11Device *device; + RECT dst_rect; + UINT token; + HRESULT hr; + DWORD res; + UINT32 i; + + notify.ready_event = CreateEventW(NULL, FALSE, FALSE, NULL); + ok(!!notify.ready_event, "CreateEventW failed, error %lu\n", GetLastError()); + + stream = load_resource(L"i420-64x64.avi", L"video/avi"); + + if (!(device = create_d3d11_device())) + { + skip("Failed to create a D3D11 device, skipping tests.\n"); + return; + } + + hr = pMFCreateDXGIDeviceManager(&token, &manager); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + hr = IMFDXGIDeviceManager_ResetDevice(manager, (IUnknown *)device, token); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + + for (i = 0; i < ARRAY_SIZE(tested_formats); ++i) + { + notify.engine_fmt = tested_formats[i].engine_fmt; + notify.output_fmt = tested_formats[i].output_fmt; + notify.media_engine = NULL; + notify.error = S_OK; + media_engine = NULL; + + winetest_push_context("%x, %x", tested_formats[i].engine_fmt, tested_formats[i].output_fmt); + + media_engine = create_media_engine_ex(¬ify.IMFMediaEngineNotify_iface, + manager, tested_formats[i].engine_fmt); + + ok(!!media_engine, "MfMediaEngine creation failed.\n"); + + if (!(notify.media_engine = media_engine)) + goto skip_iter; + + hr = IMFMediaEngineEx_SetSourceFromByteStream(media_engine, stream, url); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + + res = WaitForSingleObject(notify.ready_event, 5000); + ok(!res, "Unexpected res %#lx.\n", res); + + if (FAILED(notify.error)) + { + todo_wine_if(tested_formats[i].engine_todo) + ok((notify.error == MF_E_INVALIDMEDIATYPE || broken(notify.error == E_INVALIDARG || notify.error == E_UNEXPECTED)) + && (tested_formats[i].bad_fmt || broken(!tested_formats[i].bad_fmt)), /* Broken: Some formats aren't working on VMs. */ + "Media engine reported unexpected error %#lx on %s format.\n", notify.error, tested_formats[i].bad_fmt ? "bad" : "good"); + + skip("Had error %#lx.\n", notify.error); + goto skip_transfer; + } + else + { + todo_wine_if(tested_formats[i].engine_todo) + ok(!tested_formats[i].bad_fmt || broken(tested_formats[i].bad_fmt), /* Broken: Some formats aren't working on VMs. */ + "Media engine reported unexpected error %#lx on %s format.\n", notify.error, tested_formats[i].bad_fmt ? "bad" : "good"); + } + + texture = NULL; + + memset(&desc, 0, sizeof(desc)); + desc.Width = 64; + desc.Height = 64; + desc.ArraySize = 1; + desc.Format = tested_formats[i].output_fmt; + desc.BindFlags = D3D11_BIND_RENDER_TARGET; + desc.SampleDesc.Count = 1; + hr = ID3D11Device_CreateTexture2D(device, &desc, NULL, &texture); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + + if (hr == E_INVALIDARG) + goto skip_transfer; /* Skip when texture can't be created. */ + + res = WaitForSingleObject(notify.ready_event, 500); + ok(!res, "Unexpected res %#lx.\n", res); + + SetRect(&dst_rect, 0, 0, desc.Width, desc.Height); + hr = IMFMediaEngineEx_TransferVideoFrame(notify.media_engine, (IUnknown *)texture, NULL, NULL, NULL); + + todo_wine_if(tested_formats[i].output_todo) + ok(hr == tested_formats[i].exp_hr || broken(hr == tested_formats[i].broken_hr || hr == E_POINTER /* w1064v1507 */ || + hr == MF_E_NO_VIDEO_SAMPLE_AVAILABLE /* Spontaneous error */), "Unexpected hr %#lx.\n", hr); + + ID3D11Texture2D_Release(texture); + +skip_transfer: + IMFMediaEngineEx_Shutdown(media_engine); + IMFMediaEngineEx_Release(media_engine); +skip_iter: + winetest_pop_context(); + } + + IMFDXGIDeviceManager_Release(manager); + + ID3D11Device_Release(device); + IMFByteStream_Release(stream); + CloseHandle(notify.ready_event); +} + START_TEST(mfmediaengine) { HRESULT hr; @@ -1344,6 +1627,7 @@ START_TEST(mfmediaengine) test_SetSourceFromByteStream(); test_audio_configuration(); test_TransferVideoFrames(); + test_formats(); IMFMediaEngineClassFactory_Release(factory); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1046
From: Bernhard Kölbl <besentv(a)gmail.com> This behavior seems to match the Windows behavior the closest. Additionally the docu mentions the output(engine) format being used as render target. --- dlls/mfmediaengine/main.c | 23 +++++++++++++++++++++++ dlls/mfmediaengine/tests/mfmediaengine.c | 8 ++++---- 2 files changed, 27 insertions(+), 4 deletions(-) diff --git a/dlls/mfmediaengine/main.c b/dlls/mfmediaengine/main.c index b2ddc376db8..68696d328e9 100644 --- a/dlls/mfmediaengine/main.c +++ b/dlls/mfmediaengine/main.c @@ -1029,9 +1029,12 @@ static HRESULT media_engine_create_audio_renderer(struct media_engine *engine, I static HRESULT media_engine_create_video_renderer(struct media_engine *engine, IMFTopologyNode **node) { + D3D11_TEXTURE2D_DESC texture_desc; DXGI_FORMAT output_format; + ID3D11Texture2D *texture; IMFMediaType *media_type; IMFActivate *activate; + ID3D11Device *device; GUID subtype; HRESULT hr; @@ -1043,6 +1046,26 @@ static HRESULT media_engine_create_video_renderer(struct media_engine *engine, I return E_FAIL; } + if (FAILED(hr = media_engine_lock_d3d_device(engine, &device))) + return hr; + + /* Check if supplied format is valid for render targets. */ + memset(&texture_desc, 0, sizeof(D3D11_TEXTURE2D_DESC)); + texture_desc.Width = 320; + texture_desc.Height = 240; + texture_desc.MipLevels = 1; + texture_desc.ArraySize = 1; + texture_desc.Format = output_format; + texture_desc.SampleDesc.Count = 1; + texture_desc.Usage = D3D11_USAGE_DEFAULT; + texture_desc.BindFlags = D3D11_BIND_RENDER_TARGET; + + if (FAILED(ID3D11Device_CreateTexture2D(device, &texture_desc, NULL, &texture))) + return MF_E_INVALIDMEDIATYPE; + + ID3D11Texture2D_Release(texture); + media_engine_unlock_d3d_device(engine, device); + memcpy(&subtype, &MFVideoFormat_Base, sizeof(subtype)); if (!(subtype.Data1 = MFMapDXGIFormatToDX9Format(output_format))) { diff --git a/dlls/mfmediaengine/tests/mfmediaengine.c b/dlls/mfmediaengine/tests/mfmediaengine.c index ed0d1fc1c63..d1a8fdb8a87 100644 --- a/dlls/mfmediaengine/tests/mfmediaengine.c +++ b/dlls/mfmediaengine/tests/mfmediaengine.c @@ -1451,19 +1451,19 @@ static void test_formats(void) }, /* Some exotic, ridiculous and non-exisiting media engine formats. */ { - .engine_fmt = DXGI_FORMAT_D24_UNORM_S8_UINT, .bad_fmt = TRUE, .engine_todo = TRUE, + .engine_fmt = DXGI_FORMAT_D24_UNORM_S8_UINT, .bad_fmt = TRUE, .output_fmt = DXGI_FORMAT_B8G8R8A8_UNORM, .exp_hr = S_OK, }, { - .engine_fmt = DXGI_FORMAT_BC2_UNORM_SRGB, .bad_fmt = TRUE, .engine_todo = TRUE, + .engine_fmt = DXGI_FORMAT_BC2_UNORM_SRGB, .bad_fmt = TRUE, .output_fmt = DXGI_FORMAT_B8G8R8A8_UNORM, .exp_hr = S_OK, }, { - .engine_fmt = DXGI_FORMAT_NV12, .bad_fmt = TRUE, .engine_todo = TRUE, + .engine_fmt = DXGI_FORMAT_NV12, .bad_fmt = TRUE, .output_fmt = DXGI_FORMAT_B8G8R8A8_UNORM, .exp_hr = S_OK, .output_todo = TRUE, }, { - .engine_fmt = NON_EXISITING_DXGI_FORMAT, .bad_fmt = TRUE, .engine_todo = TRUE, + .engine_fmt = NON_EXISITING_DXGI_FORMAT, .bad_fmt = TRUE, .output_fmt = DXGI_FORMAT_B8G8R8A8_UNORM, .exp_hr = S_OK, }, /* Texture format working with NVIDIA and AMD driver, but broken in VMs. */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1046
From: Bernhard Kölbl <besentv(a)gmail.com> Some games try to use DXGI formats for MfMediaEngine, which are not converable to DX9 (e.g. DXGI_FORMAT_R32G32B32_FLOAT). This works on Windows, so in this case use DXGI_FORMAT_B8G8R8A8_UNORM as a fallback. --- dlls/mfmediaengine/main.c | 6 +++--- dlls/mfmediaengine/tests/mfmediaengine.c | 5 ++--- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/dlls/mfmediaengine/main.c b/dlls/mfmediaengine/main.c index 68696d328e9..4833a3ea2e5 100644 --- a/dlls/mfmediaengine/main.c +++ b/dlls/mfmediaengine/main.c @@ -1067,10 +1067,10 @@ static HRESULT media_engine_create_video_renderer(struct media_engine *engine, I media_engine_unlock_d3d_device(engine, device); memcpy(&subtype, &MFVideoFormat_Base, sizeof(subtype)); - if (!(subtype.Data1 = MFMapDXGIFormatToDX9Format(output_format))) + while (!(subtype.Data1 = MFMapDXGIFormatToDX9Format(output_format))) { - WARN("Unrecognized output format %#x.\n", output_format); - return E_FAIL; + FIXME("Output format %#x cannot be converted to DX9, using DXGI_FORMAT_B8G8R8A8_UNORM as fallback.\n", output_format); + output_format = DXGI_FORMAT_B8G8R8A8_UNORM; } if (FAILED(hr = MFCreateMediaType(&media_type))) diff --git a/dlls/mfmediaengine/tests/mfmediaengine.c b/dlls/mfmediaengine/tests/mfmediaengine.c index d1a8fdb8a87..792db04a4c3 100644 --- a/dlls/mfmediaengine/tests/mfmediaengine.c +++ b/dlls/mfmediaengine/tests/mfmediaengine.c @@ -226,7 +226,6 @@ static struct media_engine_notify *create_callback(void) struct media_engine_notify *object; object = calloc(1, sizeof(*object)); - object->IMFMediaEngineNotify_iface.lpVtbl = &media_engine_notify_vtbl; object->refcount = 1; @@ -1421,7 +1420,7 @@ static void test_formats(void) { /* Some supported media engine and output texture formats. */ { - .engine_fmt = DXGI_FORMAT_R32G32B32_FLOAT, .engine_todo = TRUE, + .engine_fmt = DXGI_FORMAT_R32G32B32_FLOAT, .output_fmt = DXGI_FORMAT_B8G8R8A8_UNORM, .exp_hr = S_OK, }, { @@ -1429,7 +1428,7 @@ static void test_formats(void) .output_fmt = DXGI_FORMAT_R10G10B10A2_UNORM, .exp_hr = S_OK, }, { - .engine_fmt = DXGI_FORMAT_R16G16B16A16_UINT, .engine_todo = TRUE, + .engine_fmt = DXGI_FORMAT_R16G16B16A16_UINT, .output_fmt = DXGI_FORMAT_B8G8R8X8_UNORM, .exp_hr = S_OK, }, { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1046
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=126560 Your paranoid android. === debian11 (32 bit de report) === mfmediaengine: mfmediaengine: Timeout
v3: - Removed failing `todo_wine`s. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1046#note_16701
Nikolay Sivov (@nsivov) commented about dlls/mfmediaengine/main.c:
+ /* Check if supplied format is valid for render targets. */ + memset(&texture_desc, 0, sizeof(D3D11_TEXTURE2D_DESC)); + texture_desc.Width = 320; + texture_desc.Height = 240; + texture_desc.MipLevels = 1; + texture_desc.ArraySize = 1; + texture_desc.Format = output_format; + texture_desc.SampleDesc.Count = 1; + texture_desc.Usage = D3D11_USAGE_DEFAULT; + texture_desc.BindFlags = D3D11_BIND_RENDER_TARGET; + + if (FAILED(ID3D11Device_CreateTexture2D(device, &texture_desc, NULL, &texture))) + return MF_E_INVALIDMEDIATYPE; + + ID3D11Texture2D_Release(texture); + media_engine_unlock_d3d_device(engine, device); Is this really different from D3D11_FORMAT_SUPPORT_RENDER_TARGET? More importantly, what difference does this output format attribute really make? We are not using internally created texture as a render target, so why check for such support?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1046#note_16740
Nikolay Sivov (@nsivov) commented about dlls/mfmediaengine/main.c:
media_engine_unlock_d3d_device(engine, device);
memcpy(&subtype, &MFVideoFormat_Base, sizeof(subtype)); - if (!(subtype.Data1 = MFMapDXGIFormatToDX9Format(output_format))) + while (!(subtype.Data1 = MFMapDXGIFormatToDX9Format(output_format))) { - WARN("Unrecognized output format %#x.\n", output_format); - return E_FAIL; + FIXME("Output format %#x cannot be converted to DX9, using DXGI_FORMAT_B8G8R8A8_UNORM as fallback.\n", output_format); + output_format = DXGI_FORMAT_B8G8R8A8_UNORM; }
Does this work if device was created without D3D11_CREATE_DEVICE_BGRA_SUPPORT? And then again, why bother with specified format at all, if we can always use BGRA. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1046#note_16741
On Mon Nov 21 17:12:12 2022 +0000, Nikolay Sivov wrote:
Is this really different from D3D11_FORMAT_SUPPORT_RENDER_TARGET? More importantly, what difference does this output format attribute really make? We are not using internally created texture as a render target, so why check for such support? Yes it is and it's closest matching to working DXGI formats on windows. The only other solution I'm seeing is making a lookup table.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1046#note_16751
On Mon Nov 21 17:13:55 2022 +0000, Nikolay Sivov wrote:
Does this work if device was created without D3D11_CREATE_DEVICE_BGRA_SUPPORT? And then again, why bother with specified format at all, if we can always use BGRA. Hm. I just thought mimicking windows behavior is the best solution. Sure we can just always fallback to this format not sure if it makes sense tho. It should probably work as much without BRGA support as it did before.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1046#note_16754
On Mon Nov 21 17:42:06 2022 +0000, Bernhard Kölbl wrote:
Hm. I just thought mimicking windows behavior is the best solution. Sure we can just always fallback to this format not sure if it makes sense tho. It should probably work as much without BRGA support as it did before. How can you tell what is the windows behavior here?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1046#note_16763
On Mon Nov 21 17:36:20 2022 +0000, Bernhard Kölbl wrote:
Yes it is and it's closest matching to working DXGI formats on windows. The only other solution I'm seeing is making a lookup table. It would be best if such check was also meaningful for our implementation.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1046#note_16764
On Mon Nov 21 18:27:27 2022 +0000, Nikolay Sivov wrote:
How can you tell what is the windows behavior here? Well the tests show that media engine accepts only specific formats, or I'm misunderstanding your question.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1046#note_16766
On Mon Nov 21 18:28:05 2022 +0000, Nikolay Sivov wrote:
It would be best if such check was also meaningful for our implementation. What check? Also wdym with meaningful?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1046#note_16767
On Mon Nov 21 18:41:51 2022 +0000, Bernhard Kölbl wrote:
What check? Also wdym with meaningful? I mean quoted part. It's testing if a texture with this format could be created with BIND_RENDER_TARGET, but then we don't use this functionality, so why check for it?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1046#note_16770
On Mon Nov 21 18:44:13 2022 +0000, Nikolay Sivov wrote:
I mean quoted part. It's testing if a texture with this format could be created with BIND_RENDER_TARGET, but then we don't use this functionality, so why check for it? Yeah creating a texture for this is suboptimal, but I don't know of a simpler/other way that matches the same error returns.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1046#note_16771
On Mon Nov 21 18:58:51 2022 +0000, Bernhard Kölbl wrote:
Yeah creating a texture for this is suboptimal, but I don't know of a simpler/other way that matches the same error returns. So, I generally check for this because of the matching error pattern.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1046#note_16772
On Mon Nov 21 18:38:46 2022 +0000, Bernhard Kölbl wrote:
Well the tests show that media engine accepts only specific formats, or I'm misunderstanding your question. On the Wine side we don't care much, since the implemention is most likely different.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1046#note_16788
Since the solution with creating a render target texture for just pattern matching is not ideal, there are two options: - Find out why CheckFormatSupport yields different results for render targets. - Use a lookup table instead. What's preferred? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1046#note_17008
Rémi Bernon (@rbernon) commented about dlls/mfmediaengine/tests/mfmediaengine.c:
+ ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + break; + + case MF_MEDIA_ENGINE_EVENT_FORMATCHANGE: + ret = IMFMediaEngineEx_HasVideo(media_engine); + ok(ret, "Unexpected HasVideo %u.\n", ret); + hr = IMFMediaEngineEx_GetNativeVideoSize(media_engine, &width, &height); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + ok(width == 64, "Unexpected width %lu.\n", width); + ok(height == 64, "Unexpected height %lu.\n", height); + break; + + case MF_MEDIA_ENGINE_EVENT_ERROR: + //ok(param2 == MF_E_INVALIDMEDIATYPE || broken(param2 == MF_E_UNSUPPORTED_BYTESTREAM_TYPE), + // "Unexpected error %llu, error2 %#lx\n", param1, param2); + notify->error = param2; Leftovers.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1046#note_17226
Rémi Bernon (@rbernon) commented about dlls/mfmediaengine/tests/mfmediaengine.c:
+ { + notify.engine_fmt = tested_formats[i].engine_fmt; + notify.output_fmt = tested_formats[i].output_fmt; + notify.media_engine = NULL; + notify.error = S_OK; + media_engine = NULL; + + winetest_push_context("%x, %x", tested_formats[i].engine_fmt, tested_formats[i].output_fmt); + + media_engine = create_media_engine_ex(¬ify.IMFMediaEngineNotify_iface, + manager, tested_formats[i].engine_fmt); + + ok(!!media_engine, "MfMediaEngine creation failed.\n"); + + if (!(notify.media_engine = media_engine)) + goto skip_iter; I don't think you need to check that again or even skip tests if it's not supposed to happen.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1046#note_17227
Rémi Bernon (@rbernon) commented about dlls/mfmediaengine/tests/mfmediaengine.c:
+ + if (!(notify.media_engine = media_engine)) + goto skip_iter; + + hr = IMFMediaEngineEx_SetSourceFromByteStream(media_engine, stream, url); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + + res = WaitForSingleObject(notify.ready_event, 5000); + ok(!res, "Unexpected res %#lx.\n", res); + + if (FAILED(notify.error)) + { + todo_wine_if(tested_formats[i].engine_todo) + ok((notify.error == MF_E_INVALIDMEDIATYPE || broken(notify.error == E_INVALIDARG || notify.error == E_UNEXPECTED)) + && (tested_formats[i].bad_fmt || broken(!tested_formats[i].bad_fmt)), /* Broken: Some formats aren't working on VMs. */ + "Media engine reported unexpected error %#lx on %s format.\n", notify.error, tested_formats[i].bad_fmt ? "bad" : "good"); It's a bit hard to parse the condition, maybe a `if (tested_formats[i].bad_fmt)` with the corresponding expectations?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1046#note_17228
Rémi Bernon (@rbernon) commented about dlls/mfmediaengine/tests/mfmediaengine.c:
+ if (FAILED(notify.error)) + { + todo_wine_if(tested_formats[i].engine_todo) + ok((notify.error == MF_E_INVALIDMEDIATYPE || broken(notify.error == E_INVALIDARG || notify.error == E_UNEXPECTED)) + && (tested_formats[i].bad_fmt || broken(!tested_formats[i].bad_fmt)), /* Broken: Some formats aren't working on VMs. */ + "Media engine reported unexpected error %#lx on %s format.\n", notify.error, tested_formats[i].bad_fmt ? "bad" : "good"); + + skip("Had error %#lx.\n", notify.error); + goto skip_transfer; + } + else + { + todo_wine_if(tested_formats[i].engine_todo) + ok(!tested_formats[i].bad_fmt || broken(tested_formats[i].bad_fmt), /* Broken: Some formats aren't working on VMs. */ + "Media engine reported unexpected error %#lx on %s format.\n", notify.error, tested_formats[i].bad_fmt ? "bad" : "good"); + } I think that this should perhaps instead be a condition on whether you expect an error or not (`bad_fmt`), then, once you checked that. Then you can have an extra check whether to skip the rest of the test depending on if wine returned an error when it shouldn't have.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1046#note_17229
Rémi Bernon (@rbernon) commented about dlls/mfmediaengine/tests/mfmediaengine.c:
+ } + + texture = NULL; + + memset(&desc, 0, sizeof(desc)); + desc.Width = 64; + desc.Height = 64; + desc.ArraySize = 1; + desc.Format = tested_formats[i].output_fmt; + desc.BindFlags = D3D11_BIND_RENDER_TARGET; + desc.SampleDesc.Count = 1; + hr = ID3D11Device_CreateTexture2D(device, &desc, NULL, &texture); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + + if (hr == E_INVALIDARG) + goto skip_transfer; /* Skip when texture can't be created. */ This is tested above and shouldn't happen, you don't need to include code paths that aren't supposed to be taken.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1046#note_17230
Rémi Bernon (@rbernon) commented about dlls/mfmediaengine/tests/mfmediaengine.c:
+ desc.SampleDesc.Count = 1; + hr = ID3D11Device_CreateTexture2D(device, &desc, NULL, &texture); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + + if (hr == E_INVALIDARG) + goto skip_transfer; /* Skip when texture can't be created. */ + + res = WaitForSingleObject(notify.ready_event, 500); + ok(!res, "Unexpected res %#lx.\n", res); + + SetRect(&dst_rect, 0, 0, desc.Width, desc.Height); + hr = IMFMediaEngineEx_TransferVideoFrame(notify.media_engine, (IUnknown *)texture, NULL, NULL, NULL); + + todo_wine_if(tested_formats[i].output_todo) + ok(hr == tested_formats[i].exp_hr || broken(hr == tested_formats[i].broken_hr || hr == E_POINTER /* w1064v1507 */ || + hr == MF_E_NO_VIDEO_SAMPLE_AVAILABLE /* Spontaneous error */), "Unexpected hr %#lx.\n", hr); There too it's a bit hard to parse what is expected and if this will actually be tested. There's a lot of possible broken results. Could it be reduced somehow? At least the `broken_hr` seems a bit pointless if it doesn't cover all the broken hrs, maybe it should just be a boolean to accept failures (though it makes the test unlikely to catch anything).
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1046#note_17231
Rémi Bernon (@rbernon) commented about dlls/mfmediaengine/main.c:
media_engine_unlock_d3d_device(engine, device);
memcpy(&subtype, &MFVideoFormat_Base, sizeof(subtype)); - if (!(subtype.Data1 = MFMapDXGIFormatToDX9Format(output_format))) + while (!(subtype.Data1 = MFMapDXGIFormatToDX9Format(output_format))) { - WARN("Unrecognized output format %#x.\n", output_format); - return E_FAIL; + FIXME("Output format %#x cannot be converted to DX9, using DXGI_FORMAT_B8G8R8A8_UNORM as fallback.\n", output_format); + output_format = DXGI_FORMAT_B8G8R8A8_UNORM;
The while feels a bit off. It would be better to just call MFMapDXGIFormatToDX9Format again or hardcode the Data1 to the corresponding format. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1046#note_17232
Rémi Bernon (@rbernon) commented about dlls/mfmediaengine/tests/mfmediaengine.c:
struct media_engine_notify *object;
object = calloc(1, sizeof(*object)); -
Unrelated. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1046#note_17233
On Wed Nov 23 21:28:14 2022 +0000, Rémi Bernon wrote:
I think that this should perhaps instead be a condition on whether you expect an error or not (`bad_fmt`), then, once you checked that. Then you can have an extra check whether to skip the rest of the test depending on if wine returned an error when it shouldn't have. Seems like I just marked some formats wrong so I can just eliminate the broken here, which should make it more simple.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1046#note_17641
On Wed Nov 23 21:28:16 2022 +0000, Rémi Bernon wrote:
The while feels a bit off. It would be better to just call MFMapDXGIFormatToDX9Format again or hardcode the Data1 to the corresponding format. I hope a second call to the function is also okay. I'd prefer to do that as it's probably more easy to follow, than some D3D9 fmt being assigned.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1046#note_17648
On Mon Nov 21 19:00:21 2022 +0000, Bernhard Kölbl wrote:
So, I generally check for this because of the matching error pattern. I'll remove it.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1046#note_17660
participants (4)
-
Bernhard Kölbl -
Marvin -
Nikolay Sivov (＠nsivov)
-
Rémi Bernon