-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Autodesk: HgiPresent #3543
base: dev
Are you sure you want to change the base?
Autodesk: HgiPresent #3543
Conversation
Filed as internal issue #USD-10700 (This is an automated message. See here for more information.) |
/AzurePipelines run |
90dc01f
to
5576d15
Compare
5576d15
to
4e6ed60
Compare
/AzurePipelines run |
Azure Pipelines could not run because the pipeline triggers exclude this branch/path. |
4e6ed60
to
9ffba7c
Compare
@@ -103,11 +88,7 @@ HdxPresentTask::Execute(HdTaskContext* ctx) | |||
// framebuffer contents. | |||
// Eg. This allows us to render with HgiMetal and present the images |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interop is jargonny, since we're eliminating the use of HgInterop here, I think we should say
/// ... and keeps
/// api-interoperable presentation simple.
if (const auto windowDst = std::get_if<HgiWindowPresentParams>( | ||
&presentParams.destination)) { | ||
|
||
if (params.colorCorrectionMode.IsEmpty() || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
case HgiFormatBC6FloatVec3: | ||
case HgiFormatBC6UFloatVec3: | ||
case HgiFormatPackedInt1010102: | ||
return true; | ||
case HgiFormatInt16: | ||
case HgiFormatUInt16: | ||
case HgiFormatInt32: | ||
// While this looks like a float with packed integer format, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -73,10 +67,16 @@ class HgiInterop final | |||
void TransferToApp( | |||
Hgi *srcHgi, | |||
HgiTextureHandle const &srcColor, | |||
HgiBlendFactor colorSrcBlendFactor, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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".
@@ -42,9 +42,16 @@ class HgiInteropMetal final | |||
HGIINTEROP_API | |||
void CompositeToInterop( | |||
HgiTextureHandle const &color, | |||
HgiBlendFactor colorSrcBlendFactor, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto on retaining a simple variant
void _BlitToOpenGL(VtValue const &framebuffer, GfVec4i const& compRegion, | ||
int shaderIndex); | ||
void _BlitToOpenGL(uint32_t framebuffer, | ||
GfRect2i const& compRegion, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto on retaining a simple variant
GfVec4i const &compRegion, | ||
int shaderIndex) | ||
HgiInteropMetal::_BlitToOpenGL(uint32_t framebuffer, | ||
GfRect2i const &compRegion, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto on retaining a simple variant
inCoord.y >= outTexture.get_height()) { | ||
return; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
std::vector<CandidateFormat> candidateFormats; | ||
for (const auto formatAndColorSpace : formats) { | ||
const auto [format, colorSpace] = formatAndColorSpace; | ||
if (_VkColorSpaceEnumToName(colorSpace) != _params.surfaceColorSpace) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -47,19 +47,19 @@ HgiInterop::~HgiInterop() = default; | |||
void HgiInterop::TransferToApp( | |||
Hgi *srcHgi, | |||
HgiTextureHandle const &srcColor, | |||
HgiBlendFactor colorSrcBlendFactor, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we put all of these configuration parameters into a struct instead for better future-proofing?
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. |
USDIMAGINGGL_API | ||
void SetEnablePresentation(bool enabled); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
/// specific way. | ||
/// E.g., a uint32_t (aka GLuint) for framebuffer object for OpenGL. | ||
HDX_API | ||
void SetPresentationOutput(TfToken const &api, VtValue const &framebuffer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to comments in UsdImagingGLEngine, we don't want to delete this API immediately, and instead should mark it deprecated.
USDIMAGINGGL_API | ||
void SetEnablePresentation(bool enabled); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should also consider bumping the UsdImagingGL api version number.
/// Configure the presentation to simply do nothing. | ||
/// This is the default presentation since it doesn't | ||
/// require any external resources. | ||
struct HgiNoOpPresentParams |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
PXR_NAMESPACE_USING_DIRECTIVE | ||
|
||
std::optional<MTLPixelFormat> | ||
_FindPreferredSurfaceFormatMatch(HgiFormat format, bool needs_sRGB_Transform) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this function go in HgiMetal/conversions.mm?
|
||
class HgiVulkan; | ||
|
||
class HgiPresentVulkan final: public HgiPresentImpl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make this name (and the Metal version) more indicative of the fact that this is the Vulkan window present impl?
/// | ||
/// Forward the textures to \class HgiInterop. | ||
/// | ||
class HgiPresentInteropGL final : public HgiPresentImpl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This naming is confusing. This existing HgiInteropVulkan
means Vulkan-to-GL interop, but HgiPresentInteropGL
means interop to GL.
// Hgi::EndFrame. | ||
bool enabled; | ||
/// Parameters for the presentation destination. Forwarded to hgiPresent. | ||
HgiPresentDestinationParams destination; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personal preference: call this destinationParams
so it's clear it's more params.
if (_colorCorrectionTaskId.IsEmpty()) { | ||
return; | ||
} | ||
if (!_colorCorrectionTaskId.IsEmpty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason we got rid of the early-out? Would save us a level of indenting.
@@ -36,9 +37,16 @@ class HgiInteropVulkan final | |||
HGIINTEROP_API | |||
void CompositeToInterop( | |||
HgiTextureHandle const &color, | |||
HgiBlendFactor colorSrcBlendFactor, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment about putting configuration params into struct for here and similar functions.
TF_REGISTRY_FUNCTION(TfDebug) | ||
{ | ||
TF_DEBUG_ENVIRONMENT_SYMBOL(HGIPRESENT_DUMP_CANDIDATE_SURFACE_FORMATS, | ||
"Dump candidate VkSurfaceFormatKHR in order of match"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be updated to be non-Vulkan-specific.
|
||
#if defined(PXR_GL_SUPPORT_ENABLED) | ||
/// Interop to OpenGL | ||
struct HgiGLInteropHandle |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs different name, this looks like an HgiGL
struct. Should match whatever we come to after resolving the HgiPresentInteropGL
name.
Description of Change(s)
HgiPresent
HgiInterop
an implementation ofHgiPresent
HgiInterop
to be more consistent about its composition parametersHdxPresentTask
to useHgiPresent
instead ofHgiInterop
HdxTaskController
andUsdImagingGLEngine
to supportHgiPresent
UsdImagingGLEngine
to default toHgiInterop
presentationTested with:
Link to proposal (if applicable)
Fixes Issue(s)
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)