microsoft / win32metadata

Tooling to generate metadata for Win32 APIs in the Windows SDK.
Other
1.34k stars 116 forks source link

Possible misbinding #1957

Open marci1175 opened 2 months ago

marci1175 commented 2 months ago

Summary

I belive I have found a misbinding in the Win32_Media_MediaFoundation feature. The IMFSourceReader::SetCurrentMediaType(arg , . . .) expects arg to be of type u32. The documentation provided my microsoft says there are 3 variants of arg: 0–0xFFFFFFFB | The zero-based index of a stream. MF_SOURCE_READER_FIRST_VIDEO_STREAM 0xFFFFFFFC | The first video stream. MF_SOURCE_READER_FIRST_AUDIO_STREAM 0xFFFFFFFD | The first audio stream.

But in the create all of them have an i32 type. Which can still be casted to a u32, but it was confusing for me at first and I dont think this is intended behavior.

Crate manifest

[dependencies]
anyhow = "1.0.86"

[dependencies.windows]
version = "0.58.0"
features = [
    "Data_Xml_Dom",
    "Win32_Foundation",
    "Win32_Security",
    "Win32_System_Threading",
    "Win32_UI_WindowsAndMessaging",
    "Win32_Media_MediaFoundation",
]

Crate code

use windows::{core::w, Win32::Media::MediaFoundation::{MFCreateMediaType, MFCreateSourceReaderFromURL, MFMediaType_Video, MFStartup, MFVideoFormat_RGB32, MFSTARTUP_FULL, MF_MT_MAJOR_TYPE, MF_MT_SUBTYPE, MF_SOURCE_READER_FIRST_VIDEO_STREAM, MF_VERSION}};

pub unsafe fn mf_startup() -> anyhow::Result<()> {
    Ok(MFStartup(MF_VERSION, MFSTARTUP_FULL)?)
}

fn main() -> anyhow::Result<()> {
    unsafe {
        mf_startup()?;

        let source_reader = MFCreateSourceReaderFromURL(w!("video://0"), None)?;

        let media_type = MFCreateMediaType()?;

        media_type.SetGUID(&MF_MT_MAJOR_TYPE, &MFMediaType_Video)?;

        media_type.SetGUID(&MF_MT_SUBTYPE, &MFVideoFormat_RGB32)?;

        source_reader.SetCurrentMediaType(MF_SOURCE_READER_FIRST_VIDEO_STREAM, None, &media_type)?; //Compiler error

    }

    Ok(())
}
kennykerr commented 2 months ago

Looks like the Win32 metadata invents the MF_SOURCE_READER_CONSTANTS enum. I'm not sure where that comes from. The type of the constants like MF_SOURCE_READER_FIRST_VIDEO_STREAM, which should just be loose constants, is u32 in the original API definitions but ends up i32 in metadata. I'll transfer to the Win32 metadata repo for consideration.

tim-weis commented 2 months ago

The enum comes from mfreadwrite.idl that's compiled to mfreadwrite.h. If this was an integer literal 0xFFFFFFFD (for example) should be of type unsigned int (as I understand the rules), though I don't know whether the enum changes things.

riverar commented 2 months ago
using T = std::underlying_type<__MIDL___MIDL_itf_mfreadwrite_0000_0001_0001>::type;
std::cout << typeid(T).name() << std::endl;
> int

In C and, the underlying type is always int. MSVC C++ also assumes the underlying type is int, see also C4471.

kennykerr commented 2 months ago

The issue is more about what the API expects than the limitations of the C and C++ languages. The API expects unsigned arguments and C/C++ compilers were happy to smooth things over. Same with #define constants like ERROR_ACCESS_DENIED that are used with GetLastError and SetLastError and expect unsigned values.

mikebattista commented 2 months ago

This enum is not an invented enum. It's scraped from the headers so the type is whatever ClangSharp detects. We just give it a friendly name based on the recommendation from ClangSharp.

https://github.com/microsoft/win32metadata/blob/a133cf43996797cc03c87bca70842bbaa3fcd84f/generation/WinSDK/RecompiledIdlHeaders/um/mfreadwrite.h#L324-L332

We've had feedback in the past that we shouldn't deviate from the original types as scraped by ClangSharp. If we add [AssociatedEnum] on the parameter where these constants are used, would the projections handle things appropriately?