Autodesk: HgiPresent #3543

erikaharrison-adsk · 2025-02-25T01:18:57Z

Description of Change(s)

Add HgiPresent
- For now only implement Vulkan presentation for window presentation
- Metal presentation will come later
Make HgiInterop an implementation of HgiPresent
Update HgiInterop to be more consistent about its composition parameters
Update HdxPresentTask to use HgiPresent instead of HgiInterop
Update HdxTaskController and UsdImagingGLEngine to support HgiPresent
Update all clients of UsdImagingGLEngine to default to HgiInterop presentation
Fix various symbol export issues I came across

Tested with:

Windows / WIN32 / NVIDIA
Windows / WIN32 / Lavapipe
Linux / X11 / Lavapipe
macOS / Metal / Lavapipe

Link to proposal (if applicable)

N/A

Fixes Issue(s)

N/A

Checklist

I have created this PR based on the dev branch
I have followed the coding conventions
I have added unit tests that exercise this functionality (Reference:
testing guidelines)
I have verified that all unit tests pass with the proposed changes
I have submitted a signed Contributor License Agreement (Reference:
Contributor License Agreement instructions)

jesschimein · 2025-02-25T19:02:46Z

Filed as internal issue #USD-10700

(This is an automated message. See here for more information.)

jesschimein · 2025-02-25T19:02:48Z

/AzurePipelines run

jesschimein · 2025-03-10T17:11:44Z

/AzurePipelines run

azure-pipelines · 2025-03-10T17:11:51Z

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

meshula · 2025-03-13T20:51:49Z

pxr/imaging/hdx/presentTask.cpp

@@ -103,11 +88,7 @@ HdxPresentTask::Execute(HdTaskContext* ctx)
        // framebuffer contents.
        // Eg. This allows us to render with HgiMetal and present the images


This comment could be improved as well, since this change is no longer focused specifically on GL interop. Something like ~

Accordingly, we may render with an Hgi backend that differs from the rendering system used by a particular application. for example, using Vulkan to render, and OpenGL to display in an application.

meshula · 2025-03-13T20:54:44Z

pxr/imaging/hdx/presentTask.h

@@ -82,6 +55,12 @@ class HdxPresentTask : public HdxTask
    HDX_API
    void Execute(HdTaskContext* ctx) override;

+    /// Returns true if the format is supported for presentation. This is useful
+    /// for upstream tasks to prepare the AOV data accordingly, and keeps the
+    /// interop step simple.


interop is jargonny, since we're eliminating the use of HgInterop here, I think we should say

/// ... and keeps /// api-interoperable presentation simple.

meshula · 2025-03-13T21:00:37Z

pxr/imaging/hdx/taskController.cpp

+            if (const auto windowDst = std::get_if<HgiWindowPresentParams>(
+                &presentParams.destination)) {
+
+                if (params.colorCorrectionMode.IsEmpty() ||


It would be preferable that we track the Hgi target render color space, and the HgiWindow color space at a higher level, rather than hard coding the values here, but I realize that such a change should be out of scope since there is more work to be done to ensure that Hgi works in the right color space throughout the rest of the code.

It's not so much as hardcoded as these are the only cases Storm supports right now. Rendering is in linear extended sRGB, and optional colour correction converts it to non-linear (extended?) sRGB or some arbitrary user defined colour space.
I don't think there's anything wrong with using extended linear sRGB for rendering, since it simplifies the rendering code around one standard space, and doesn't loose colour information. But I think that the output colour space could be a property of the render index, like it is for the pixel format. I'm not sure if the window needs a separate colour space, it should really make a best effort at preserving the colour information according to the system constraints, which might mean doing extra transformations. To make things easier, the render index needs to have not just some colour space name tokens, but the full transformation data to make it simple to implement the necessary conversions.
Anyway I think this is out of scope of this PR. It does the best it can with the current situation.

meshula · 2025-03-13T21:15:33Z

pxr/imaging/hgi/types.cpp

    case HgiFormatBC6FloatVec3:
    case HgiFormatBC6UFloatVec3:
    case HgiFormatPackedInt1010102:
        return true;
    case HgiFormatInt16:
    case HgiFormatUInt16:
    case HgiFormatInt32:
+    // While this looks like a float with packed integer format,


I wonder if this is an opportune moment to name this what it is, which is HgiFormatDepth32UInt8. The mapping from Hgi to graphics APIs would be far less ambiguous.

I could, but it's 19 usages in 10 new files not in the PR. Also I would recommend naming it HgiFormatDepthFloat32Stencil8 since some APIs like Metal and Vulkan distinguish between colour and depth floats (VK_FORMAT_D32_SFLOAT vs VK_FORMAT_R32_SFLOAT, MTLPixelFormatDepth32Float vs MTLPixelFormatR32Float), and stencil is a bit mask not an integer.

meshula · 2025-03-13T21:29:14Z

pxr/imaging/hgiInterop/hgiInterop.h

@@ -73,10 +67,16 @@ class HgiInterop final
    void TransferToApp(
        Hgi *srcHgi,
        HgiTextureHandle const &srcColor,
+        HgiBlendFactor colorSrcBlendFactor,


I'm very curious about why GL style fixed function parameters are passed here? In general wouldn't you just want a blit to a target? Fancy compositing really seems like something in the app domain and something an app could take care of in window/widget composition. ie, the viewport widget could just draw a textured quad, as opposed to trying to set up Hydra to do something a bit fancy? Minimally could we at least also have a not-fancy plain old blit? This set up seems like something I wouldn't want to have to deal with in my own apps, for example.

These parameters were there before, just hardcoded (except for the depth function), so I exposed them and unified the interop backend capabilities. It makes sense that interop would let you configure composition since it might be drawing in a framebuffer that already has data in it, or will be drawn into again (so you might want depth values for example).
And while we could change this to assume the framebuffer is "cleared", just do a plain blit, and ask that the application composite it itself after that; it would add an extra framebuffer copy and break backwards compabitlity for those who rely on the existing composition behaviour.

Also worth noting that these operators are still part of modern APIs, for example VkPipelineColorBlendAttachmentState and VkPipelineDepthStencilStateCreateInfo, since that part of the fragment pipeline, while more customizable, is still pretty much "fixed function".

meshula · 2025-03-13T21:30:33Z

pxr/imaging/hgiInterop/metal.h

@@ -42,9 +42,16 @@ class HgiInteropMetal final
    HGIINTEROP_API
    void CompositeToInterop(
        HgiTextureHandle const &color,
+        HgiBlendFactor colorSrcBlendFactor,


ditto on retaining a simple variant

meshula · 2025-03-13T21:30:53Z

pxr/imaging/hgiInterop/metal.h

-    void _BlitToOpenGL(VtValue const &framebuffer, GfVec4i const& compRegion,
-                       int shaderIndex);
+    void _BlitToOpenGL(uint32_t framebuffer,
+        GfRect2i const& compRegion,


ditto on retaining a simple variant

meshula · 2025-03-13T21:31:22Z

pxr/imaging/hgiInterop/metal.mm

-                               GfVec4i const &compRegion,
-                               int shaderIndex)
+HgiInteropMetal::_BlitToOpenGL(uint32_t framebuffer,
+    GfRect2i const &compRegion,


ditto on retaining a simple variant

meshula · 2025-03-13T21:42:03Z

pxr/imaging/hgiPresent/metal.mm

+        inCoord.y >= outTexture.get_height()) {
+        return;
+    }
+


was the intent to insert a matrix here to convert the color space to the presentation color space? GfColorSpace will give you the proper matrix, and it would be very inexpensive to insert it here. GfColorSpace will also give you parameters for the EOTF in the case of SRGB conversion from linear.

No, this shader is needed for Metal because it doesn't support blitting to a different format or coordinate systems like Vulkan does. So we need to read the AOV to floats, flip vertically, then write back to the surface format.
But if we do decide that the presentation layer should do a best effort conversion to a supported window colour space, then yes it would go here.

meshula · 2025-03-13T21:54:49Z

pxr/imaging/hgiPresent/vulkan.cpp

+    case VK_COLOR_SPACE_DISPLAY_P3_LINEAR_EXT:
+        return GfColorSpaceNames->LinearDisplayP3;
+    case VK_COLOR_SPACE_BT709_NONLINEAR_EXT:
+        // Neither a gamma of 1.8 nor 2.2 are good approximations


Since this refers to BT709 nonlinear, I cannot see a reason to deviate from SRGBRec709 here. The only difference I can think of between SRGB and BT709 transfer functions is in the number of digits of precision each specification records. I think we should just use SRGBRec709, and there is no need for a comment.

Wikipedia and the Vulkan documentation say the OETF are different.
BT.709:

sRGB:

sRGB was created after the early development of Rec.709. The creators of sRGB chose to use the same primaries and white point as Rec.709, but changed the tone response curve (sometimes referred to as gamma) to better suit the intended use in offices and brighter conditions than television viewing in a dark living room.

meshula · 2025-03-13T22:00:28Z

pxr/imaging/hgiPresent/vulkan.cpp

+    std::vector<CandidateFormat> candidateFormats;
+    for (const auto formatAndColorSpace : formats) {
+        const auto [format, colorSpace] = formatAndColorSpace;
+        if (_VkColorSpaceEnumToName(colorSpace) != _params.surfaceColorSpace) {


similarly to the metal case, although the hardware can help us with some conversions, it can't help with all. So I think there should also be a use of the GfColorSpace matrices and coefficients in a shader to help with those cases. VK doesn't know about ACES for example.

How would we know the output of the colour correction task is, for example, ACES? As far as I can tell, only linear extended sRGB, non linear (extended?) sRGB, and some user defined OCIO spaces are supported. The user defined colour space could be ACES, but the strings used by OCIO aren't standardized; they're only IDs to the actual transformation in a config file. At least that's what I've been told.

clach · 2025-03-14T21:33:11Z

pxr/imaging/hgiInterop/hgiInterop.cpp

@@ -47,19 +47,19 @@ HgiInterop::~HgiInterop() = default;
 void HgiInterop::TransferToApp(
    Hgi *srcHgi,
    HgiTextureHandle const &srcColor,
+    HgiBlendFactor colorSrcBlendFactor,


Can we put all of these configuration parameters into a struct instead for better future-proofing?

clach · 2025-03-14T21:35:18Z

We talked about this during the meeting as a potential followup, but it'd be great to add a test executing the window present codepath.

clach · 2025-03-14T21:40:41Z

pxr/usdImaging/usdImagingGL/engine.h

    USDIMAGINGGL_API
-    void SetEnablePresentation(bool enabled);


We don't want to delete this API immediately, and instead should mark it deprecated for at least 1 or 2 releases before deletion. If possible, we could implement the existing functions using the new API.

clach · 2025-03-14T21:42:29Z

pxr/imaging/hdx/taskController.h

-    /// specific way.
-    /// E.g., a uint32_t (aka GLuint) for framebuffer object for OpenGL.
-    HDX_API
-    void SetPresentationOutput(TfToken const &api, VtValue const &framebuffer);


Similar to comments in UsdImagingGLEngine, we don't want to delete this API immediately, and instead should mark it deprecated.

clach · 2025-03-14T21:43:04Z

pxr/usdImaging/usdImagingGL/engine.h

    USDIMAGINGGL_API
-    void SetEnablePresentation(bool enabled);


Should also consider bumping the UsdImagingGL api version number.

clach · 2025-03-14T21:51:07Z

pxr/imaging/hgiPresent/present.h

+/// Configure the presentation to simply do nothing.
+/// This is the default presentation since it doesn't
+/// require any external resources.
+struct HgiNoOpPresentParams


I believe our coding style is everything in a library should be prefaced by the library name. So this struct name would be "HgiPresentNoOpPresentParams," and the same pattern for everything else in HgiPresent.

clach · 2025-03-14T21:53:20Z

pxr/imaging/hgiPresent/metal.mm

+PXR_NAMESPACE_USING_DIRECTIVE
+
+std::optional<MTLPixelFormat>
+_FindPreferredSurfaceFormatMatch(HgiFormat format, bool needs_sRGB_Transform)


Could this function go in HgiMetal/conversions.mm?

clach · 2025-03-14T21:56:28Z

pxr/imaging/hgiPresent/vulkan.h

+
+class HgiVulkan;
+
+class HgiPresentVulkan final: public HgiPresentImpl


Can we make this name (and the Metal version) more indicative of the fact that this is the Vulkan window present impl?

clach · 2025-03-14T21:59:27Z

pxr/imaging/hgiPresent/interopGL.h

+///
+/// Forward the textures to \class HgiInterop.
+///
+class HgiPresentInteropGL final : public HgiPresentImpl


This naming is confusing. This existing HgiInteropVulkan means Vulkan-to-GL interop, but HgiPresentInteropGL means interop to GL.

clach · 2025-03-14T22:04:40Z

pxr/imaging/hdx/presentTask.h

-    // Hgi::EndFrame.
-    bool enabled;
+    /// Parameters for the presentation destination. Forwarded to hgiPresent.
+    HgiPresentDestinationParams destination;


Personal preference: call this destinationParams so it's clear it's more params.

clach · 2025-03-14T22:08:57Z

pxr/imaging/hdx/taskController.cpp

-    if (_colorCorrectionTaskId.IsEmpty()) {
-        return;
-    }
+    if (!_colorCorrectionTaskId.IsEmpty()) {


Any reason we got rid of the early-out? Would save us a level of indenting.

clach · 2025-03-14T22:13:57Z

pxr/imaging/hgiInterop/vulkan.h

@@ -36,9 +37,16 @@ class HgiInteropVulkan final
    HGIINTEROP_API
    void CompositeToInterop(
        HgiTextureHandle const &color,
+        HgiBlendFactor colorSrcBlendFactor,


Same comment about putting configuration params into struct for here and similar functions.

clach · 2025-03-14T22:15:03Z

pxr/imaging/hgiPresent/debugCodes.cpp

+TF_REGISTRY_FUNCTION(TfDebug)
+{
+     TF_DEBUG_ENVIRONMENT_SYMBOL(HGIPRESENT_DUMP_CANDIDATE_SURFACE_FORMATS,
+        "Dump candidate VkSurfaceFormatKHR in order of match");


This needs to be updated to be non-Vulkan-specific.

clach · 2025-03-14T22:17:11Z

pxr/imaging/hgiPresent/interopHandle.h

+
+#if defined(PXR_GL_SUPPORT_ENABLED)
+/// Interop to OpenGL
+struct HgiGLInteropHandle


Needs different name, this looks like an HgiGL struct. Should match whatever we come to after resolving the HgiPresentInteropGL name.

DDoS force-pushed the adsk/feature/hgipresent branch from 90dc01f to 5576d15 Compare February 28, 2025 20:30

DDoS force-pushed the adsk/feature/hgipresent branch from 5576d15 to 4e6ed60 Compare March 10, 2025 16:07

DDoS added 2 commits March 13, 2025 12:58

HgiPresent

86c173e

HgiPresent Metal

9ffba7c

DDoS force-pushed the adsk/feature/hgipresent branch from 4e6ed60 to 9ffba7c Compare March 13, 2025 17:03

meshula reviewed Mar 13, 2025

View reviewed changes

clach reviewed Mar 14, 2025

View reviewed changes

		@@ -103,11 +88,7 @@ HdxPresentTask::Execute(HdTaskContext* ctx)
		// framebuffer contents.
		// Eg. This allows us to render with HgiMetal and present the images


		class HgiVulkan;

		class HgiPresentVulkan final: public HgiPresentImpl

Autodesk: HgiPresent #3543

Are you sure you want to change the base?

Autodesk: HgiPresent #3543

Conversation

erikaharrison-adsk commented Feb 25, 2025 • edited Loading

Description of Change(s)

Link to proposal (if applicable)

Fixes Issue(s)

Checklist

jesschimein commented Feb 25, 2025

jesschimein commented Feb 25, 2025

jesschimein commented Mar 10, 2025

azure-pipelines bot commented Mar 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clach commented Mar 14, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clach Mar 14, 2025 • edited Loading

Choose a reason for hiding this comment

erikaharrison-adsk commented Feb 25, 2025 •

edited

Loading

clach Mar 14, 2025 •

edited

Loading