Add ADR for custom viewport color correction
Document the decision to replace HdxColorCorrectionTask with a custom GL post-process (linear render + sRGB/OCIO correction via the OCIO GPU API), including rationale, pipeline, consequences, and alternatives considered. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -0,0 +1,183 @@
|
||||
# ADR 0001 — Custom Viewport Color Correction (sRGB / OCIO)
|
||||
|
||||
- **Status:** Accepted
|
||||
- **Date:** 2026-06-27
|
||||
- **Component:** `src/core/UsdSceneRenderer.cpp` (`ViewportColorCorrector`, `ApplyViewportColorCorrection`)
|
||||
- **Commit:** `fb1ae9d`
|
||||
|
||||
## Context
|
||||
|
||||
The viewport renders a USD stage through `UsdImagingGLEngine` (Hydra / Storm)
|
||||
into an offscreen `GlfDrawTarget`, whose color texture ImGui samples for
|
||||
display. Color management (linear → display) was delegated to Hydra's
|
||||
`HdxColorCorrectionTask` via `UsdImagingGLEngine::SetColorCorrectionSettings()`,
|
||||
configured by the per-viewport `ColorCorrectionMode` (Disabled / sRGB /
|
||||
OpenColorIO) plus OCIO display/view/colorspace/look.
|
||||
|
||||
Two problems made the OCIO path unusable:
|
||||
|
||||
1. **OCIO output was black.** sRGB correction rendered correctly through the
|
||||
same color AOV, proving the correction *input* was fine — the hdx OCIO
|
||||
shader/LUT path itself emitted black. The exact cause stayed opaque because
|
||||
USD reports it via `TF_WARN`, which this GUI app discarded (no console).
|
||||
|
||||
2. **The failure was sticky.** OCIO resources are built lazily *inside*
|
||||
`Render()` (`HdxColorCorrectionTask::Sync`), not in `SetColorCorrectionSettings()`.
|
||||
A throw there escaped `UsdSceneRenderer::Render()` and skipped
|
||||
`m_drawTarget->Unbind()`, permanently unbalancing the `GlfDrawTarget`
|
||||
bind/restore stack — so every later frame, **including after switching back
|
||||
to sRGB**, stayed black until an app restart.
|
||||
|
||||
We do not control the bundled USD build, so fixing `HdxColorCorrectionTask`
|
||||
itself was not an option. App and USD both link OpenColorIO 2.1, and a usable
|
||||
ACES 1.2 config is bundled and pointed to via `$OCIO`.
|
||||
|
||||
## Goals / Non-Goals
|
||||
|
||||
**Goals:**
|
||||
- Correct, non-black OCIO display transforms in the viewport (ACES 1.2).
|
||||
- sRGB output visually identical to the previous hdx sRGB path.
|
||||
- A color-management failure can never permanently corrupt the viewport.
|
||||
- Overlays (grid, axis, bbox, camera/light gizmos) remain display-referred,
|
||||
drawn on top of the corrected image — unchanged from the prior behavior.
|
||||
|
||||
**Non-Goals:**
|
||||
- Replacing Hydra's *rendering* — only its color-correction stage is replaced.
|
||||
- Exposure/gamma dynamic-property UI, or interactive grading.
|
||||
- Applying an explicit OCIO look override beyond the view's own looks.
|
||||
- Color-managing the ImGui UI chrome outside the viewport image.
|
||||
|
||||
## Decision
|
||||
|
||||
### D1 — Replace `HdxColorCorrectionTask` with our own GL post-process
|
||||
|
||||
**Decision:** Hydra always renders **linear** (`colorCorrectionMode = "disabled"`).
|
||||
A custom fullscreen pass (`ViewportColorCorrector`) applies the correction
|
||||
afterwards, fully under our control.
|
||||
|
||||
**Rationale:** The hdx OCIO path is a black box we cannot patch in the bundled
|
||||
USD. Owning the post-process gives deterministic, debuggable behavior and
|
||||
removes the lazy-throw-inside-Render hazard entirely. sRGB and OCIO now share
|
||||
one code path with consistent ordering.
|
||||
|
||||
---
|
||||
|
||||
### D2 — Render linear into an `RGBA16F` color attachment
|
||||
|
||||
**Decision:** The draw-target color attachment was changed from `GL_RGBA`
|
||||
(8-bit unorm) to `GL_RGBA16F`.
|
||||
|
||||
**Rationale:** We now store the *linear* scene as the intermediate. Display
|
||||
transforms (especially ACES) need scene values outside `[0,1]`; an 8-bit unorm
|
||||
buffer would clip highlights and band the shadows before correction even runs.
|
||||
16F is ample headroom at negligible cost. ImGui and `CaptureFrame` sample/read
|
||||
it unchanged.
|
||||
|
||||
---
|
||||
|
||||
### D3 — OCIO via the OCIO GPU API (`GpuShaderDesc`), LUTs uploaded by us
|
||||
|
||||
**Decision:** For OCIO mode, build the transform directly with OCIO 2.1:
|
||||
`config->getProcessor(srcColorSpace, display, view, FORWARD)` →
|
||||
`getDefaultGPUProcessor()` → `extractGpuShaderInfo(GpuShaderDesc)` with
|
||||
`GPU_LANGUAGE_GLSL_1_3` and function name `OCIODisplay`. We compile the
|
||||
generated GLSL into our fragment shader and upload/bind every 1D/2D/3D LUT it
|
||||
requests as GL textures. The source colorspace defaults to the `scene_linear`
|
||||
role when unset.
|
||||
|
||||
**Rationale:** This is exactly what `HdxColorCorrectionTask` does internally,
|
||||
but visible and ours to debug. `GLSL_1_3` matches the project's existing
|
||||
`#version 130` shaders and emits modern `texture()` calls. The program and LUTs
|
||||
are cached and rebuilt only when display/view/colorspace/look change.
|
||||
|
||||
---
|
||||
|
||||
### D4 — Graceful degradation to sRGB; never black
|
||||
|
||||
**Decision:** Any OCIO failure — processor build, shader compile/link, or LUT
|
||||
upload — logs the reason and falls back to the sRGB encode shader for that
|
||||
frame. The failing key is remembered so we do not retry (and re-spam logs)
|
||||
until the parameters change.
|
||||
|
||||
**Rationale:** A misconfigured display/view must degrade to a usable image, not
|
||||
a black viewport. sRGB is the correct neutral fallback and matches the default.
|
||||
|
||||
---
|
||||
|
||||
### D5 — Implemented inline in `UsdSceneRenderer.cpp` (pimpl), not new TU/files
|
||||
|
||||
**Decision:** `ViewportColorCorrector` is a class defined in
|
||||
`UsdSceneRenderer.cpp`, held via `std::unique_ptr` behind a forward declaration
|
||||
in the header.
|
||||
|
||||
**Rationale:** `CORE_SOURCES` is a `file(GLOB_RECURSE ...)`; adding a new `.cpp`
|
||||
forces a CMake reconfigure, which risks re-triggering the heavy Cycles
|
||||
`ExternalProject` build. Keeping it in an existing TU avoids that while the
|
||||
pimpl keeps OCIO/GL includes out of the public header. The corrector follows
|
||||
the file's existing `Init*/Destroy*` GL-resource lifecycle conventions.
|
||||
|
||||
---
|
||||
|
||||
### D6 — Route USD diagnostics to the app log
|
||||
|
||||
**Decision:** Install a process-wide `TfDiagnosticMgr::Delegate`
|
||||
(`InstallUsdDiagnosticLogger`) that forwards USD `TF_ERROR` / `TF_WARN` /
|
||||
`TF_STATUS` to the app logger with a `[USD]` prefix.
|
||||
|
||||
**Rationale:** USD's own diagnostics were invisible in this GUI app. This made
|
||||
the original OCIO failure undiagnosable and is generally useful for any Hydra
|
||||
issue.
|
||||
|
||||
## Pipeline
|
||||
|
||||
Per frame, after Hydra renders linear into the draw target:
|
||||
|
||||
1. `m_drawTarget->Resolve()` — resolves MSAA into the color attachment (no-op
|
||||
when MSAA is off), giving a single-sample linear image.
|
||||
2. Blit that attachment into `m_ccLinearTex` — a separate copy is required
|
||||
because sampling and writing the same texture in one pass is illegal.
|
||||
3. Bind the draw-target render FBO (the MSAA FBO when multisampling) and draw
|
||||
the corrected fullscreen triangle sampling `m_ccLinearTex`.
|
||||
4. Overlays draw on top (uncorrected), then the final `Resolve()` in
|
||||
`GetColorTextureID()` produces the texture ImGui samples.
|
||||
|
||||
This keeps overlays display-referred exactly as before, and the correction is
|
||||
MSAA-correct (a flat fullscreen quad resolves to the same value per sample).
|
||||
|
||||
## Consequences
|
||||
|
||||
**Positive:**
|
||||
- OCIO display transforms work; OCIO failures degrade to sRGB, never black.
|
||||
- A color-correction failure can no longer corrupt the bind stack — the throw
|
||||
hazard is gone because Hydra no longer does OCIO.
|
||||
- The full transform is inspectable/loggable in our own code.
|
||||
- HDR-linear intermediate enables correct tone-mapped output.
|
||||
|
||||
**Negative / trade-offs:**
|
||||
- We reimplement and now maintain OCIO GPU plumbing (LUT upload, sampler
|
||||
binding) that USD would otherwise own; OCIO major-version API changes could
|
||||
require updates.
|
||||
- One extra fullscreen pass, one blit, and an `RGBA16F` buffer per viewport.
|
||||
- OCIO dynamic properties (exposure/gamma) and explicit look overrides are not
|
||||
wired up.
|
||||
- Logic lives in `UsdSceneRenderer.cpp` rather than its own module, by D5.
|
||||
|
||||
## Alternatives Considered
|
||||
|
||||
- **Keep `HdxColorCorrectionTask`, fix configuration.** Rejected: the failure
|
||||
was inside the bundled USD's GPU/LUT integration, not our parameters; we
|
||||
cannot patch it, and sRGB-correct/OCIO-black pointed at the hdx shader path.
|
||||
- **Custom sRGB only, keep hdx for OCIO.** Rejected: OCIO is the actual
|
||||
requirement; a custom sRGB-only path wouldn't address it.
|
||||
- **Bake the OCIO transform to a single 3D LUT on the CPU.** Rejected: loses
|
||||
HDR shaper handling and precision that `GpuShaderDesc` manages correctly.
|
||||
- **New `ViewportColorCorrection.{h,cpp}` module.** Rejected for now (D5) to
|
||||
avoid a CMake reconfigure / Cycles rebuild; revisit if the file grows.
|
||||
|
||||
## Verification
|
||||
|
||||
Built clean (Release) and confirmed in-app by the maintainer: OCIO produces the
|
||||
expected corrected (non-black) image and sRGB is unchanged. Headless automated
|
||||
verification is blocked by the stale test-target CMake config (missing USD
|
||||
include dirs); restoring those targets would let an OCIO-vs-sRGB center-pixel
|
||||
test assert this automatically.
|
||||
Reference in New Issue
Block a user