Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Missing definition for ICompositionTexture #1760

Closed
MarijnS95 opened this issue Dec 12, 2023 · 31 comments · Fixed by #1763 or #1853
Closed

Missing definition for ICompositionTexture #1760

MarijnS95 opened this issue Dec 12, 2023 · 31 comments · Fixed by #1763 or #1853
Labels
blocking A projection cannot use the latest version

Comments

@MarijnS95
Copy link
Contributor

MarijnS95 commented Dec 12, 2023

In c41ef74 a new ICompositorInterop2 interface is added to generation/WinSDK/RecompiledIdlHeaders/winrt/windows.ui.composition.interop.h which makes use of ICompositionTexture that is only defined in generation/WinSDK/RecompiledIdlHeaders/winrt/windows.ui.composition.h. This file is not listed under --traverse in generation\WinSDK\Partitions\WinRT.Composition\settings.rsp, causing the winmd to not have a definition for ICompositionTexture (only a type reference). windows-rs requires a type definition to understand how it behaves, and will otherwise crash (with a still rather-obscure error):

microsoft/windows-rs#2539 (comment)

Simply adding <IncludeRoot>/winrt/windows.ui.composition.h to --traverse to scrape the whole file results in many errors that I cannot immediately resolve (duplicates in namespace Apis, many missing references to types from Windows.Foundation.Numerics).

Is it okay to open a PR that adds ICompositorInterop2 to --exclude for now, until someone figures out how to successfully scrape windows.ui.composition.h?

@riverar
Copy link
Collaborator

riverar commented Dec 14, 2023

I don't see ICompositorTexture defined in windows.ui.composition.h or anywhere else. Am I missing something?

@riverar
Copy link
Collaborator

riverar commented Dec 14, 2023

@mikebattista Think we're missing a header or idl here?

@robmikh
Copy link
Member

robmikh commented Dec 14, 2023

Composition textures were added in 22621, I'm able to see it in the WinRT metadata from the SDK.

I'd only expect the interop interfaces to be included here.

@riverar
Copy link
Collaborator

riverar commented Dec 14, 2023

@robmikh I'm looking at 22621 and don't see ICompositorTexture in there. Am I blind? 😂

@robmikh
Copy link
Member

robmikh commented Dec 14, 2023

Before I comment on your eyesight, where are you pulling the metadata? I'm looking at Windows Kits\10\UnionMetadata\10.0.22621.0\Windows.winmd:

image

EDIT: Ah, the name is ICompositionTexture not ICompositorTexture. That may be the issue.

@MarijnS95
Copy link
Contributor Author

Looks like I misspelled it in the issue description, while writing it the correct way in the title.

https://github.com/microsoft/win32metadata/blob/3d65d3752a1a957de011a11c8b9f9745df2ad106/generation/WinSDK/RecompiledIdlHeaders/winrt/windows.ui.composition.h#L10500

@riverar
Copy link
Collaborator

riverar commented Dec 14, 2023

Got it, thanks!

// \winrt\windows.ui.composition.interop.h

#if (NTDDI_VERSION >= NTDDI_WIN10_NI)

#undef INTERFACE
#define INTERFACE ICompositorInterop2
DECLARE_INTERFACE_IID_(ICompositorInterop2, IUnknown, "D3EEF34C-0667-4AFC-8D13-867607B0FE91")
{
    IFACEMETHOD(CheckCompositionTextureSupport)(
        _In_ IUnknown * renderingDevice,
        _Out_ BOOL * supportsCompositionTextures) PURE;

    IFACEMETHOD(CreateCompositionTexture)(
        _In_ IUnknown * d3dTexture,
-       _Outptr_ ICompositionTexture ** compositionTexture) PURE;
};

...

// \10.0.22621.0\winrt\windows.ui.composition.idl
[contract(Windows.Foundation.UniversalApiContract, 15.0)]
[exclusiveto(Windows.UI.Composition.CompositionTexture)]
[uuid(347D03A0-1C0A-4C0B-B232-8570B2B1A4EA)]
-interface ICompositionTexture : IInspectable
{
	[propget] HRESULT SourceRect([out] [retval] Windows.Graphics.RectInt32* value);
	[propput] HRESULT SourceRect([in] Windows.Graphics.RectInt32 value);
	[propget] HRESULT AlphaMode([out] [retval] Windows.Graphics.DirectX.DirectXAlphaMode* value);
	[propput] HRESULT AlphaMode([in] Windows.Graphics.DirectX.DirectXAlphaMode value);
	[propget] HRESULT ColorSpace([out] [retval] Windows.Graphics.DirectX.DirectXColorSpace* value);
	[propput] HRESULT ColorSpace([in] Windows.Graphics.DirectX.DirectXColorSpace value);
}

Will try getting this added in tomorrow.

@MarijnS95
Copy link
Contributor Author

@riverar the diff context is somewhat malformed without a leading space; what are we looking at? Is your idea to remove this type / argument from CreateCompositionTexture? I don't think you should remove the interface declaration from windows.ui.composition.idl while leaving the annotations and block with function declarations in place?

@riverar
Copy link
Collaborator

riverar commented Dec 14, 2023

Was just talking notes and using the diff format to gain a highlighter. Not a real diff, apologies for any confusion ha!

@MarijnS95
Copy link
Contributor Author

@riverar don't forget to look at the linked main...MarijnS95:win32metadata:winrt-composition-exclude-interop2 then! I've already done the work necessary to exclude it without botching/modifying the existing headers.

@MarijnS95
Copy link
Contributor Author

Note that I proposed to just PR that, in case you're thinking of doing the exact same.

@riverar
Copy link
Collaborator

riverar commented Dec 14, 2023

I was going to take a stab at including it, don't think we want to exclude it from metadata.

@MarijnS95
Copy link
Contributor Author

That might be even nicer, but probably a lot of work. I recall reading somewhere that "WinRT bindings are provided in a different file/way" (Microsoft.Windows.SDK.Contracts) but can't find the mention anymore. These are WinRT headers if the file is to be believed, and I shortly thought that the interop header was specifically for "Win32 bindings" into it?

@riverar
Copy link
Collaborator

riverar commented Dec 14, 2023

Upon closer inspection, doesn't look like there's any work to be done here.

Metadata is emitting the correct definition and pointing to the correct external source (Windows.Foundation.UniversalApiContract).

.method public hidebysig newslot abstract virtual 
instance valuetype [Windows.Win32.winmd]Windows.Win32.Foundation.HRESULT CreateCompositionTexture (
	[in] class [Windows.Win32.winmd]Windows.Win32.System.Com.IUnknown d3dTexture,
+	[out] class [Windows.Foundation.UniversalApiContract]Windows.UI.Composition.ICompositionTexture* compositionTexture
) cil managed 
{
} // end of method ICompositorInterop2::CreateCompositionTexture

@riverar
Copy link
Collaborator

riverar commented Dec 14, 2023

Closing for now but feel free to holler or hit me if we're missing something here.

@riverar riverar closed this as not planned Won't fix, can't repro, duplicate, stale Dec 14, 2023
@MarijnS95
Copy link
Contributor Author

Is this possibly the only non-Win32 reference in Win32 metadata? It's the only (external) TypeRef that windows-rs' riddle is failing on.

@robmikh
Copy link
Member

robmikh commented Dec 15, 2023

ICompositionSurface should be in the same boat, and that's usable today from windows-rs. Maybe the WinRT metadata is out of date?

@robmikh
Copy link
Member

robmikh commented Dec 15, 2023

Ah, maybe you're right. ICompositionTexture has a typedef in the interop header, which is unusual. That may be at play here when parsed.

@riverar
Copy link
Collaborator

riverar commented Dec 15, 2023

It's not the only reference tied to UniversalApiContract.

Metadata has references to Windows.Foundation.UniversalApiContract 10.0 and 14.0, but ICompositionTexture requires contract 15.0. Will re-open this to track fixing that.

@riverar riverar reopened this Dec 15, 2023
@riverar
Copy link
Collaborator

riverar commented Dec 15, 2023

Complicating matters is that it appears ICompositionTexture was added to 22621.2428's variant of contract 15.0, whilst not in 22621.755's contract 15.0. Or in other words, Microsoft forgot to increment the contract version again.

Note to self: I'll need to update the Rust side too.

@riverar
Copy link
Collaborator

riverar commented Dec 15, 2023

@MarijnS95 Corrected that contract (#1763) and kicked off a metadata update @ microsoft/windows-rs#2745. Want to give those a whirl?

@MarijnS95
Copy link
Contributor Author

@riverar thanks, yeah, the Windows SDK update in microsoft/windows-rs#2745 makes me able to pull in a new Win32 winmd - without the workaround to disable ICompositorInterop2 proposed above - and it generates fine now without the error observed in microsoft/windows-rs#2742.

Good to know that there are multiple reasons for a TypeRef, this makes a lot of sense but the diagnostics are just missing.

@AArnott
Copy link
Member

AArnott commented Feb 22, 2024

I'm having a similar problem in CsWin32 whilst trying to update the metadata it consumes. It's failing to generate ICompositorInterop2 because of its reference to ICompositionTexture:

.method public hidebysig newslot abstract virtual 
		instance valuetype [Windows.Win32.winmd]Windows.Win32.Foundation.HRESULT CreateCompositionTexture (
			[in] class [Windows.Win32.winmd]Windows.Win32.System.Com.IUnknown d3dTexture,
			[out] class [Windows.Foundation.UniversalApiContract]Windows.UI.Composition.ICompositionTexture* compositionTexture
		) cil managed 

But here's the rub: as defined in the Windows.Foundation.UniversalApiContract dll, the ICompositionTexture interface is internal!
image

So while CsWin32 can generate ICompositorInterop2, the code fails to compile in the user's project with this error:

error CS0122: 'ICompositionTexture' is inaccessible due to its protection level

Generally speaking, C# doesn't let a public API such as ICompositorInterop2 reference an internal API in another assembly.

How can we reconcile this?

@AArnott AArnott reopened this Feb 22, 2024
@AArnott AArnott added the blocking A projection cannot use the latest version label Feb 22, 2024
@riverar
Copy link
Collaborator

riverar commented Feb 22, 2024

I don't think the access modifiers for the types in Windows.Foundation.UniversalApiContract.winmd are meant to be used that way. Note that most of the interface types in that file are internal, which is probably incorrect.

@kennykerr
Copy link
Contributor

The metadata should never refer to exclusive interfaces. They should be replaced by the class that implements the exclusive interface.

@AArnott
Copy link
Member

AArnott commented Feb 23, 2024

I don't think the access modifiers for the types in Windows.Foundation.UniversalApiContract.winmd are meant to be used that way.

It isn't up to me. This is passed to the C# compiler as a reference assembly, and C# is going to honor them.

@riverar
Copy link
Collaborator

riverar commented Feb 23, 2024

Looks like we'll need to fix up these references again (emitter.settings.rsp):

# Windows 10 (19041.1)
IPropertyValue(interface)=<Windows.Foundation.FoundationContract, Version=4.0.0.0, Culture=neutral, PublicKeyToken=null>Windows.Foundation.IPropertyValue
IPropertySet(interface)=<Windows.Foundation.FoundationContract, Version=4.0.0.0, Culture=neutral, PublicKeyToken=null>Windows.Foundation.Collections.IPropertySet
ICompositionSurface(interface)=<Windows.Foundation.UniversalApiContract, Version=10.0.0.0, Culture=neutral, PublicKeyToken=null>Windows.UI.Composition.ICompositionSurface
ICompositionCapabilities(interface)=<Windows.Foundation.UniversalApiContract, Version=10.0.0.0, Culture=neutral, PublicKeyToken=null>Windows.UI.Composition.CompositionCapabilities
ICompositionGraphicsDevice(interface)=<Windows.Foundation.UniversalApiContract, Version=10.0.0.0, Culture=neutral, PublicKeyToken=null>Windows.UI.Composition.CompositionGraphicsDevice
ICompositor(interface)=<Windows.Foundation.UniversalApiContract, Version=10.0.0.0, Culture=neutral, PublicKeyToken=null>Windows.UI.Composition.Compositor
IGraphicsEffectSource(interface)=<Windows.Foundation.UniversalApiContract, Version=10.0.0.0, Culture=neutral, PublicKeyToken=null>Windows.Graphics.Effects.IGraphicsEffectSource
IDesktopWindowTarget(interface)=<Windows.Foundation.UniversalApiContract, Version=10.0.0.0, Culture=neutral, PublicKeyToken=null>Windows.UI.Composition.Desktop.DesktopWindowTarget
IDispatcherQueueController(interface)=<Windows.Foundation.UniversalApiContract, Version=10.0.0.0, Culture=neutral, PublicKeyToken=null>Windows.System.DispatcherQueueController
# Windows 11 (22000.194)
IPropertySetSerializer(interface)=<Windows.Foundation.UniversalApiContract, Version=14.0.0.0, Culture=neutral, PublicKeyToken=null>Windows.Storage.Streams.IPropertySetSerializer
# Windows 11 (22621.2428)
ICompositionTexture(interface)=<Windows.Foundation.UniversalApiContract, Version=15.0.0.0, Culture=neutral, PublicKeyToken=null>Windows.UI.Composition.ICompositionTexture

@riverar
Copy link
Collaborator

riverar commented Feb 23, 2024

Actually think most of them are okay, just not the one I added back in 2023 🥲

@riverar
Copy link
Collaborator

riverar commented Feb 23, 2024

Fix in the pipe, have to figure out what's going on with failing tests though.

@AArnott
Copy link
Member

AArnott commented Mar 20, 2024

@mikebattista any idea when the next metadata will be published to nuget.org with the fix for this so I can get a CsWin32 update out?

@MarijnS95
Copy link
Contributor Author

Just saying, I'd love for the next release to include #1757 in addition to the Agility SDK upgrade recently, if we can wait for that 🤞

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocking A projection cannot use the latest version
Projects
None yet
5 participants