microsoft / win32metadata

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

Annotate `IDCompositionDesktopDevice/IDCompositionDevice::CreateSurfaceFromHandle()` to return `IDCompositionSurface`? #1968

Closed MarijnS95 closed 3 months ago

MarijnS95 commented 3 months ago

[!TIP] IDCompositionDesktopDevice::CreateSurfaceFromHandle() and IDCompositionDevice::CreateSurfaceFromHandle() may not return a known typed handle at all. All you need this "opaque surface proxy" IUnknown object for is to pass it into IDCompositionVisual::SetContent(here) to make it visible on a Visual somewhere in your composition tree. (Perhaps it may be QueryInterfaced into something?)

Currently IDCompositionDesktopDevice::CreateSurfaceFromHandle() and IDCompositionDevice::CreateSurfaceFromHandle() return IUknown (and it seems the same is true for their CreateSurfaceFromHwnd() counterparts). Unfortunately, calling QueryInterface on the returned COM interface returns None for IDCompositionSurface.

Digging in a debugger, the symbols for the returned pointer are from an internal class DirectComposition::CCompositionSurfaceProxy. And further stumbling upon this official examle from Microsoft, it simply turns out that we have to reinterpret the pointer to pretend it's an IDCompositionSurface.

I'm not sure what we should do here, but I assume QueryInterface is always supposed to be functional? Either:

  1. QueryInterface fixed to be queriable for its own type (anticipating extensions later on), or;
  2. A version of this function that takes an IID so that we can call it for the expected COM object type (similar to DCompositionCreateDevice()).

In the title I suggested to use the return type of IDCompositionSurface but that should trickle down into the internal bindings and doesn't allow other interfaces to be returned further down the road unless they derive from IDCompositionSurface (though that afaik relies on the caller passing in an IID?).

Thanks!

kennykerr commented 3 months ago

@robmikh may have some thoughts...

robmikh commented 3 months ago

Hmmm, that's certainly strange. I'll investigate why we're failing the QI. Thanks for flagging this!

riverar commented 3 months ago

I don't believe it would be appropriate for metadata or windows-rs to make one-off changes in this situation. It might be more effective to address this with a warning in the composition documentation if needed.

MarijnS95 commented 3 months ago

@riverar yes, I had hoped alarm bells would go off when a reinterpret_cast() is required to get a certain interface in/out of an API in an official sample. A warning would be nice, especially if QueryInterface is fixed in a future release (and developers see it working on their machine, and expect it to be working everywhere else).

MarijnS95 commented 3 months ago

And not to forget, the IDCompositionDevice::CreateSurfaceFromHandle() only vaguely mentions that this parameter will contain The new composition surface object and not at all explicitly that this will be IDCompositionSurface with an intra-documentation link. That should be clarified I think.

riverar commented 3 months ago

I suspect the sample is wrong and that function is correctly returning an opaque object for you to pass into subsequent methods. The sample doesn't call any methods hanging off surface so the bug escapes detection.

MarijnS95 commented 3 months ago

@riverar you're on to something. I may have spent too much attempting to understand what kind of The new composition surface object to cast the IUnknown into - and was happy to immediately find that example - without considering for a second that my "use case" only involves passing it into IDCompositionVisual::SetContent() (to make an "external" swapchain visible in a hierarchy) which takes an IUnknown anyway.