[PATCH 0/2] MR7417: mfplat: Modify MFCreateWaveFormatExFromMFMediaType to allow MF_MT_AM_FORMAT_TYPE to be missing or set to GUID_NULL.

[Brendan McGrath (＠redmcg)]

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

[PATCH 1/2] mfplat/tests: Add additional MFCreateWaveFormatExFromMFMediaType tests.

[Brendan McGrath]

From: Brendan McGrath

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 */

[PATCH 2/2] mfplat: Allow MF_MT_AM_FORMAT_TYPE to be missing or set to GUID_NULL.

[Brendan McGrath]

From: Brendan McGrath

---
 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);

[Nikolay Sivov (＠nsivov)]

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.

[Brendan McGrath (＠redmcg)]

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`.

[Nikolay Sivov (＠nsivov)]

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:
> !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.

[Brendan McGrath (＠redmcg)]

> 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.

[Brendan McGrath (＠redmcg)]

> 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`).