[PATCH 0/2] MR7417: mfplat: Modify MFCreateWaveFormatExFromMFMediaType to allow MF_MT_AM_FORMAT_TYPE to be missing or set to GUID_NULL.
This MR: - adds tests for when MF_MT_AM_FORMAT_TYPE is missing or set to GUID_NULL; and - modifies MFCreateWaveFormatExFromMFMediaType to pass these tests -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7417
From: Brendan McGrath <bmcgrath(a)codeweavers.com> Test when MF_MT_AM_FORMAT_TYPE is missing or set to GUID_NULL. --- dlls/mfplat/tests/mfplat.c | 42 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/dlls/mfplat/tests/mfplat.c b/dlls/mfplat/tests/mfplat.c index 6bd6bb74d68..db85577fb2c 100644 --- a/dlls/mfplat/tests/mfplat.c +++ b/dlls/mfplat/tests/mfplat.c @@ -5954,6 +5954,36 @@ static void test_MFCreateWaveFormatExFromMFMediaType(void) hr = MFCreateWaveFormatExFromMFMediaType(mediatype, &format, &size, MFWaveFormatExConvertFlag_Normal); ok(hr == MF_E_ATTRIBUTENOTFOUND, "Unexpected hr %#lx.\n", hr); + hr = IMFMediaType_SetGUID(mediatype, &MF_MT_MAJOR_TYPE, &MFMediaType_Audio); + ok(hr == S_OK, "Failed to set attribute, hr %#lx.\n", hr); + + hr = IMFMediaType_SetGUID(mediatype, &MF_MT_SUBTYPE, &MEDIASUBTYPE_MP3); + ok(hr == S_OK, "Failed to set attribute, hr %#lx.\n", hr); + + hr = MFCreateWaveFormatExFromMFMediaType(mediatype, &format, &size, MFWaveFormatExConvertFlag_Normal); + todo_wine + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + CoTaskMemFree(format); + + hr = IMFMediaType_SetGUID(mediatype, &MF_MT_SUBTYPE, &DUMMY_CLSID); + ok(hr == S_OK, "Failed to set attribute, hr %#lx.\n", hr); + + hr = MFCreateWaveFormatExFromMFMediaType(mediatype, &format, &size, MFWaveFormatExConvertFlag_Normal); + todo_wine + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + CoTaskMemFree(format); + + hr = IMFMediaType_SetGUID(mediatype, &MF_MT_AM_FORMAT_TYPE, &GUID_NULL); + ok(hr == S_OK, "Failed to set attribute, hr %#lx.\n", hr); + + hr = MFCreateWaveFormatExFromMFMediaType(mediatype, &format, &size, MFWaveFormatExConvertFlag_Normal); + todo_wine + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + CoTaskMemFree(format); + + hr = IMFMediaType_DeleteAllItems(mediatype); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + hr = IMFMediaType_SetGUID(mediatype, &MF_MT_SUBTYPE, &MFMediaType_Video); ok(hr == S_OK, "Failed to set attribute, hr %#lx.\n", hr); @@ -8485,6 +8515,18 @@ static void test_MFInitAMMediaTypeFromMFMediaType(void) ok(IsEqualGUID(&am_type.formattype, &FORMAT_MPEG2Video), "got %s.\n", debugstr_guid(&am_type.formattype)); ok(am_type.cbFormat == sizeof(MPEG2VIDEOINFO), "got %lu\n", am_type.cbFormat); CoTaskMemFree(am_type.pbFormat); + IMFMediaType_DeleteAllItems(media_type); + + /* test audio with NULL mapping */ + hr = IMFMediaType_SetGUID(media_type, &MF_MT_MAJOR_TYPE, &MFMediaType_Audio); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + hr = IMFMediaType_SetGUID(media_type, &MF_MT_SUBTYPE, &MFAudioFormat_MP3); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + hr = IMFMediaType_SetUINT32(media_type, &MF_MT_AUDIO_NUM_CHANNELS, 2); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + hr = MFInitAMMediaTypeFromMFMediaType(media_type, GUID_NULL, &am_type); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + IMFMediaType_DeleteAllItems(media_type); /* test WAVEFORMATEX mapping */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7417
From: Brendan McGrath <bmcgrath(a)codeweavers.com> --- dlls/mfplat/mediatype.c | 6 ++++-- dlls/mfplat/tests/mfplat.c | 3 --- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/dlls/mfplat/mediatype.c b/dlls/mfplat/mediatype.c index feab99e3d8f..0ee29b7a5e3 100644 --- a/dlls/mfplat/mediatype.c +++ b/dlls/mfplat/mediatype.c @@ -2989,7 +2989,7 @@ HRESULT WINAPI MFCreateWaveFormatExFromMFMediaType(IMFMediaType *mediatype, WAVE { UINT32 extra_size = 0, user_size; WAVEFORMATEX *format; - GUID major, subtype, basetype = MFAudioFormat_Base; + GUID major, subtype, basetype = MFAudioFormat_Base, formattype; void *user_data; HRESULT hr; @@ -3006,7 +3006,9 @@ HRESULT WINAPI MFCreateWaveFormatExFromMFMediaType(IMFMediaType *mediatype, WAVE if (FAILED(hr = IMFMediaType_GetBlobSize(mediatype, &MF_MT_USER_DATA, &user_size))) { - if (!IsEqualGUID(&subtype, &MFAudioFormat_PCM) && !IsEqualGUID(&subtype, &MFAudioFormat_Float)) + if (SUCCEEDED(IMFMediaType_GetGUID(mediatype, &MF_MT_AM_FORMAT_TYPE, &formattype)) && + !IsEqualGUID(&formattype, &GUID_NULL) && + !IsEqualGUID(&subtype, &MFAudioFormat_PCM) && !IsEqualGUID(&subtype, &MFAudioFormat_Float)) return hr; user_size = 0; } diff --git a/dlls/mfplat/tests/mfplat.c b/dlls/mfplat/tests/mfplat.c index db85577fb2c..1dbaddbfdcd 100644 --- a/dlls/mfplat/tests/mfplat.c +++ b/dlls/mfplat/tests/mfplat.c @@ -5961,7 +5961,6 @@ static void test_MFCreateWaveFormatExFromMFMediaType(void) ok(hr == S_OK, "Failed to set attribute, hr %#lx.\n", hr); hr = MFCreateWaveFormatExFromMFMediaType(mediatype, &format, &size, MFWaveFormatExConvertFlag_Normal); - todo_wine ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); CoTaskMemFree(format); @@ -5969,7 +5968,6 @@ static void test_MFCreateWaveFormatExFromMFMediaType(void) ok(hr == S_OK, "Failed to set attribute, hr %#lx.\n", hr); hr = MFCreateWaveFormatExFromMFMediaType(mediatype, &format, &size, MFWaveFormatExConvertFlag_Normal); - todo_wine ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); CoTaskMemFree(format); @@ -5977,7 +5975,6 @@ static void test_MFCreateWaveFormatExFromMFMediaType(void) ok(hr == S_OK, "Failed to set attribute, hr %#lx.\n", hr); hr = MFCreateWaveFormatExFromMFMediaType(mediatype, &format, &size, MFWaveFormatExConvertFlag_Normal); - todo_wine ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); CoTaskMemFree(format); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7417
Nikolay Sivov (@nsivov) commented about dlls/mfplat/mediatype.c:
if (FAILED(hr = IMFMediaType_GetBlobSize(mediatype, &MF_MT_USER_DATA, &user_size))) { - if (!IsEqualGUID(&subtype, &MFAudioFormat_PCM) && !IsEqualGUID(&subtype, &MFAudioFormat_Float)) + if (SUCCEEDED(IMFMediaType_GetGUID(mediatype, &MF_MT_AM_FORMAT_TYPE, &formattype)) && + !IsEqualGUID(&formattype, &GUID_NULL) && + !IsEqualGUID(&subtype, &MFAudioFormat_PCM) && !IsEqualGUID(&subtype, &MFAudioFormat_Float)) return hr;
Either commit message is confusing, or there is something else missing. The message says "allow attribute to be missing", but it was never checked before and its value is not used in your change. Is the the idea that we either have MT_USER_DATA or MT_AM_FORMAT_TYPE? Docs seem to imply that a little bit. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7417#note_95853
On Tue Feb 25 09:31:08 2025 +0000, Nikolay Sivov wrote:
Either commit message is confusing, or there is something else missing. The message says "allow attribute to be missing", but it was never checked before and its value is not used in your change. Is the the idea that we either have MT_USER_DATA or MT_AM_FORMAT_TYPE? Docs seem to imply that a little bit. The function already has everything needed to support a `MT_AM_FORMAT_TYPE` value of `GUID_NULL`, but it is not being accepted. As you can see in the tests, it should be returning `S_OK` when `MT_AM_FORMAT_TYPE` is `GUID_NULL` (or missing, as `GUID_NULL` is the implied default), but instead it is returning an error (hence the `todo_wine` prior to the fix).
The issue is with the following `if` statement: ```c !IsEqualGUID(&subtype, &MFAudioFormat_PCM) && !IsEqualGUID(&subtype, &MFAudioFormat_Float) ``` it will return an error when subtype is neither PCM or Float. But it should also consider the value of `MT_AM_FORMAT_TYPE` and allow `GUID_NULL` (whether explicitly set or implied by not being set at all). So the I've modified the if statement to allow `GUID_NULL` by saying in addition to subtype being neither PCM or Float, `MT_AM_FORMAT_TYPE` must be explicitly set and must be a value other than `GUID_NULL`. Otherwise we'll allow it. Hence I've modified `MFCreateWaveFormatExFromMFMediaType` to allow `MF_MT_AM_FORMAT_TYPE` to be missing or set to `GUID_NULL`. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7417#note_95927
On Tue Feb 25 20:12:23 2025 +0000, Brendan McGrath wrote:
The function already has everything needed to support a `MT_AM_FORMAT_TYPE` value of `GUID_NULL`, but it is not being accepted. As you can see in the tests, it should be returning `S_OK` when `MT_AM_FORMAT_TYPE` is `GUID_NULL` (or missing, as `GUID_NULL` is the implied default), but instead it is returning an error (hence the `todo_wine` prior to the fix). The issue is with the following `if` statement: ```c !IsEqualGUID(&subtype, &MFAudioFormat_PCM) && !IsEqualGUID(&subtype, &MFAudioFormat_Float) ``` it will return an error when subtype is neither PCM or Float. But it should also consider the value of `MT_AM_FORMAT_TYPE` and allow `GUID_NULL` (whether explicitly set or implied by not being set at all). So the I've modified the if statement to allow `GUID_NULL` by saying in addition to subtype being neither PCM or Float, `MT_AM_FORMAT_TYPE` must be explicitly set and must be a value other than `GUID_NULL`. Otherwise we'll allow it. Hence I've modified `MFCreateWaveFormatExFromMFMediaType` to allow `MF_MT_AM_FORMAT_TYPE` to be missing or set to `GUID_NULL`. I see. So basically this means type could provide MT_USER_DATA or MT_AM_FORMAT_TYPE. MT_USER_DATA prioritized and MT_AM_FORMAT_TYPE(GUID_NULL) is a no-op. In addition to that MT_USER_DATA or MF_AM_FORMAT_TYPE are ignored for MFAudioFormat_PCM and MFAudioFormat_Float (so for any raw format?). Do we know what should happen if MT_AM_FORMAT_TYPE is used with non-zero GUID value? This nesting check looks too complicated and something to be changed completely once we need to handle non-zero guids.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7417#note_95937
Do we know what should happen if MT_AM_FORMAT_TYPE is used with non-zero GUID value?
My understanding is that the content of `MT_USER_DATA` is either: - `WAVEFORMATEX` / `WAVEFORMATEXTENSIBLE` (for raw audio); or - arbitrary binary data where the expected format of that data is defined by `MT_AM_FORMAT_TYPE` (`GUID_NULL` meaning we can ignore it) https://learn.microsoft.com/en-us/windows/win32/medfound/mf-mt-am-format-typ... https://learn.microsoft.com/en-us/windows/win32/medfound/mf-mt-user-data-att...
This nesting check looks too complicated and something to be changed completely once we need to handle non-zero guids.
Agreed. I'll to look rewrite that and push a change. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7417#note_95938
Do we know what should happen if MT_AM_FORMAT_TYPE is used with non-zero GUID value?
Sorry, I realise my previous answer doesn't really answer the question. If there's not already a test I'll add one. But I suspect it should fail (as the function returns a `WAVEFORMATEX`, which doesn't support anything but `GUID_NULL` or `WAVEFORMATEX`). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7417#note_95939
participants (3)
-
Brendan McGrath -
Brendan McGrath (＠redmcg)
-
Nikolay Sivov (＠nsivov)