Draft: Apply PPISP in Camera Wrapper Across Renderers#5499
Draft: Apply PPISP in Camera Wrapper Across Renderers#5499moennen wants to merge 5 commits intoisaac-sim:developfrom
Conversation
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
This PR adds PPISP (Physically Plausible Image Signal Processing) post-processing support across all three camera renderers (Isaac RTX, OVRTX, Newton Warp). It introduces a shared Warp kernel for PPISP application at the camera wrapper level, adds rgb_hdr output plumbing for HDR color capture, and bundles SPG shader assets for USD-based PPISP graphs. The core implementation is sound, but there are several issues with the Warp kernel math, test isolation, and missing synchronization.
Architecture Impact
- Cross-module:
CameraCfg.ppispflows throughCamera._check_supported_data_types()→ renderer'sset_outputs()→_apply_ppisp_if_needed(). All three renderers (IsaacRtxRenderer,OVRTXRenderer,NewtonWarpRenderer) are updated to supportRGB_HDR. - New public API:
PPISPCfgis exported fromisaaclab.renderers.__init__.pyi. - Bundled assets: SPG shader files in
ppisp_spg/are included viasetup.pypackage_data. - Newton compatibility layer: Defensive accessors (
_newton_sensor_render_config,_newton_sensor_utils) added to support multiple Newton API versions.
Implementation Verdict
Minor fixes needed — The core design is correct, but there are issues with missing GPU synchronization, vignetting not being applied in the Warp kernel, and test determinism concerns.
Test Coverage
Two new test files added:
test_ppisp.py: Tests USD/SPG authoring helpers (shader import, render product parsing, SPG asset bundling) — good coverage of USD utilitiestest_ppisp_warp.py: Tests Warp kernel exposure effect and camera wrapper integration — minimal coverage, missing vignetting/color correction tests
Missing: No regression tests for the Newton API compatibility changes. No integration tests verifying end-to-end PPISP with actual rendering.
CI Status
Only labeler ran successfully. Full CI suite results not available.
Findings
🔴 Critical: ppisp_warp.py:140-146 — Missing GPU synchronization after Warp kernel launch
The apply_ppisp_to_rgba function launches a Warp kernel but does not synchronize before returning. When the caller immediately reads out_rgba, they may get stale data since the kernel runs asynchronously.
def apply_ppisp_to_rgba(hdr_color: torch.Tensor, out_rgba: torch.Tensor, cfg: PPISPCfg) -> None:
# ... kernel launch ...
wp.launch(...)
# Missing: wp.synchronize() or equivalentFix: Add wp.synchronize_device(str(out_rgba.device)) or use wp.synchronize() after the launch.
🔴 Critical: ppisp_warp.py:117-130 — Warp kernel ignores vignetting parameters entirely
The _apply_ppisp_kernel kernel signature accepts only exposure and color/CRF parameters but the SPG shader (ppisp_usd_spg.slang) applies vignetting per-channel. The Warp kernel completely skips vignetting, meaning results will differ between USD SPG execution and IsaacLab-side PPISP:
# Kernel only applies: exposure → color correction → CRF
# Missing: vignetting step that exists in Slang shader lines 166-175Fix: Either add vignetting to the Warp kernel or document this as an intentional simplification.
🟡 Warning: camera.py:538-542 — Potential race condition with _renderer_output_data aliasing
When PPISP is disabled, _renderer_output_data is set to dict(self._data.output), creating a shallow copy. Both the renderer and PPISP code may reference the same underlying tensors. If a future renderer mutates buffers in-place during async operations, this could cause data corruption.
self._renderer_output_data = dict(self._data.output) # shallow copyFix: This is currently safe but fragile. Consider documenting the ownership contract.
🟡 Warning: newton_warp_renderer.py:64-73 — _newton_clear_data falls back to private module import
The fallback imports from newton._src.sensors.warp_raytrace, which is a private API path:
from newton._src.sensors.warp_raytrace import ClearData as clear_data_clsThis may break silently on future Newton versions. Consider adding a try/except with a meaningful error message.
🟡 Warning: test_ppisp_warp.py:32-57 — Test uses SimpleNamespace to mock Camera, bypassing real initialization
The test creates a mock Camera with SimpleNamespace, which doesn't verify that Camera._apply_ppisp_if_needed is actually bound correctly as a method or that the real camera initialization properly sets up PPISP:
camera = SimpleNamespace(
cfg=SimpleNamespace(ppisp=ppisp_cfg),
_renderer_output_data={"rgb_hdr": hdr_color},
...
)
Camera._apply_ppisp_if_needed(camera) # Calling as unbound methodThis test passes even if _apply_ppisp_if_needed were never actually called in production code paths.
🔵 Improvement: ppisp.py:538-544 — Temporary directory created for anonymous stages is never cleaned up
When stages are anonymous, _resolve_stage_spg_assets creates a temp directory with tempfile.mkdtemp but never registers cleanup:
stage_dir = Path(tempfile.mkdtemp(prefix="isaaclab_ppisp_spg_"))
copy_ppisp_spg_files(stage_dir)
# No cleanup registeredFix: Consider using tempfile.TemporaryDirectory with delete=False and registering an atexit handler, or document the leak.
🔵 Improvement: isaac_rtx_renderer.py:214-220 — Duplicated annotator registration logic
The HDR annotator registration is duplicated between the rgba/rgb branch and the explicit rgb_hdr branch:
if annotator_type == "rgba" or annotator_type == "rgb":
if spec.cfg.ppisp is not None:
annotator = rep.AnnotatorRegistry.get_annotator("HdrColor", ...)
annotators[str(RenderBufferKind.RGB_HDR)] = annotator
# ... later ...
elif annotator_type == str(RenderBufferKind.RGB_HDR):
annotator = rep.AnnotatorRegistry.get_annotator("HdrColor", ...)
annotators[str(RenderBufferKind.RGB_HDR)] = annotatorFix: Consolidate to avoid double-registration if user requests both rgba with PPISP and explicit rgb_hdr.
🔵 Improvement: ppisp_usd_spg.slang.usda — File is stored in Git LFS but appears to be small (1918 bytes)
The .usda file is tracked via Git LFS according to its pointer content, but at only 1918 bytes, it might be simpler to store it directly. This also means anyone cloning without LFS won't get the actual content.
Greptile SummaryThis PR introduces renderer-agnostic PPISP support by adding
Confidence Score: 3/5Two defects need resolution before merge: OVRTX silently produces zero RGB output when rgb_hdr and rgb are both requested without PPISP, and the shader_prim_path field in CameraPPISPCfg is silently ignored during camera initialization with no diagnostic. The OVRTX get_render_var_config early-return on use_hdr makes requesting [rgb, rgb_hdr] without PPISP produce silently zeroed rgb/rgba buffers. Separately, normalize_camera_ppisp_cfg is called without a stage in _check_valid_cfg, so a CameraPPISPCfg with a shader_prim_path that a user expects to auto-load will silently use identity defaults instead of calibrated ISP parameters. source/isaaclab_ov/isaaclab_ov/renderers/ovrtx_usd.py (get_render_var_config HDR early return) and source/isaaclab/isaaclab/sensors/camera/camera.py (normalize_camera_ppisp_cfg called without stage). Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Camera
participant Renderer
participant PPISPWarp
User->>Camera: reset() / update()
Camera->>Camera: _create_buffers() allocate rgb_hdr scratch buffer
Camera->>Renderer: set_outputs(renderer_output_data)
Note over Renderer: renderer_output_data contains rgb_hdr + rgba + rgb view
loop Each physics step
Camera->>Renderer: render(render_data)
Renderer-->>Camera: fills rgb_hdr buffer HdrColor from scene
Camera->>Camera: _apply_ppisp_if_needed()
Camera->>PPISPWarp: apply_ppisp_to_rgba(hdr_color, rgba, cfg)
Note over PPISPWarp: exposure vignetting homography CRF uint8 pack
PPISPWarp-->>Camera: rgba filled with LDR output
Note over Camera: rgb = rgba slice view auto-updated
end
Camera-->>User: data.output rgb or rgba
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # Anonymous Kit stages do not provide a layer directory for relative SPG | ||
| # assets. Use a unique colocated sidecar directory so the Slang source and | ||
| # optional .slang.lua launcher resolve like exported 3DGRUT USDs. | ||
| stage_dir = Path(tempfile.mkdtemp(prefix="isaaclab_ppisp_spg_")) | ||
| copy_ppisp_spg_files(stage_dir) | ||
| return str(stage_dir / PPISP_SPG_USDA_FILE), str(stage_dir / PPISP_SPG_SLANG_FILE) |
There was a problem hiding this comment.
_resolve_stage_spg_assets calls tempfile.mkdtemp() each time it is invoked for an anonymous stage and never cleans the directory up — there is no atexit hook, no weakref finalizer, and no manual cleanup path. Every camera that initializes with an anonymous Kit stage (the typical Kit workflow) creates a new directory that lives for the entire process. In a simulation with many parallel camera initializations or frequent resets, this silently accumulates disk space. At minimum, register a cleanup via atexit.register(shutil.rmtree, stage_dir, ignore_errors=True) immediately after the mkdtemp call, or use a module-level cache keyed by the stage identity so the same directory is reused for the same stage.
| def apply_ppisp_to_rgba(hdr_color: torch.Tensor, out_rgba: torch.Tensor, cfg: PPISPCfg) -> None: | ||
| """Apply PPISP to ``hdr_color`` and write LDR RGBA into ``out_rgba``.""" | ||
| inputs = cfg.inputs | ||
| wp.init() | ||
| wp.launch( |
There was a problem hiding this comment.
wp.init() is called on every invocation of apply_ppisp_to_rgba, which runs once per camera update per physics step. While Warp's init() is idempotent after the first call, it still acquires a lock and does version/device checks on every call. Move the wp.init() call to module import time (top of the file), which is the conventional Warp pattern and eliminates per-frame overhead.
| def apply_ppisp_to_rgba(hdr_color: torch.Tensor, out_rgba: torch.Tensor, cfg: PPISPCfg) -> None: | |
| """Apply PPISP to ``hdr_color`` and write LDR RGBA into ``out_rgba``.""" | |
| inputs = cfg.inputs | |
| wp.init() | |
| wp.launch( | |
| def apply_ppisp_to_rgba(hdr_color: torch.Tensor, out_rgba: torch.Tensor, cfg: PPISPCfg) -> None: | |
| """Apply PPISP to ``hdr_color`` and write LDR RGBA into ``out_rgba``.""" | |
| inputs = cfg.inputs | |
| wp.launch( |
| def _apply_ppisp_if_needed(self) -> None: | ||
| """Apply PPISP once at the IsaacLab camera wrapper boundary.""" | ||
| if self.cfg.ppisp is None or self._renderer_output_data is None: | ||
| return | ||
| hdr_color = self._renderer_output_data.get(str(RenderBufferKind.RGB_HDR)) | ||
| rgba = self._data.output.get("rgba") | ||
| if hdr_color is None or rgba is None: | ||
| return | ||
| apply_ppisp_to_rgba(hdr_color[..., :3], rgba, self.cfg.ppisp) | ||
| if "rgb" in self._data.output: | ||
| self._data.output["rgb"] = rgba[..., :3] | ||
|
|
There was a problem hiding this comment.
rgba buffer is absent
_apply_ppisp_if_needed silently returns when rgba is None (line 543), but _create_buffers only validates that the renderer exposes RGB_HDR — it does not verify that an rgba buffer was actually allocated. If a future renderer includes RGB_HDR in supported_output_types but omits RGBA (breaking rgb_alias), PPISP is configured, allocates an HDR buffer, but then silently produces no output. There is no log warning, no exception, and no indication to the caller that PPISP was skipped. A logger.warning(...) here, or an assertion in _create_buffers that rgba is present when PPISP is active, would make this failure mode visible.
moennen
force-pushed
the
nicolasm/ppisp-renderer-simple-integration
branch
from
2fcda20 to
73a68df
Compare
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
This is a follow-up review on commit 73a68df. The PR has been significantly simplified — all SPG shader assets and Newton API compatibility layers have been removed. The implementation is now focused solely on the Warp kernel PPISP pipeline.
Previous Concerns Status
- GPU synchronization: NOT addressed —
apply_ppisp_to_rgbastill lackswp.synchronize()after kernel launch - Vignetting in Warp kernel: ADDRESSED — vignetting is now fully implemented in
_apply_vignettingfunction (lines 33-54 incamera_ppisp_warp.py) _renderer_output_dataaliasing: Still present but was acknowledged as safe- Newton private API import: RESOLVED — the Newton API compatibility layer was removed entirely
- Test using SimpleNamespace mock: Still present but acceptable for unit testing
- Temp directory cleanup: RESOLVED — SPG asset handling removed entirely
- Duplicated annotator registration: Still present in
isaac_rtx_renderer.py:214-220
New Findings
🔴 Critical: camera_ppisp_warp.py:233-282 — Missing GPU synchronization after Warp kernel launch
This remains unaddressed from the previous review. The kernel launches asynchronously and the function returns immediately:
def apply_ppisp_to_rgba(hdr_color: torch.Tensor, out_rgba: torch.Tensor, cfg: CameraPPISPCfg) -> None:
wp.launch(_apply_ppisp_kernel, ...)
# No wp.synchronize() - caller may read stale dataFix: Add wp.synchronize_device(str(out_rgba.device)) before returning.
🟡 Warning: camera_ppisp_warp.py:247 — wp.init() called on every PPISP application
wp.init() is called inside apply_ppisp_to_rgba, which runs every frame. While Warp handles repeated init calls gracefully, this adds unnecessary overhead:
def apply_ppisp_to_rgba(...):
inputs = cfg.inputs
wp.init() # Called every frameFix: Move wp.init() to module-level or camera initialization.
Implementation Verdict
Minor fixes needed — The synchronization issue is critical for correctness. The wp.init() call is a performance concern but not blocking.
Test Coverage
Tests are adequate for the simplified implementation. The vignetting test (test_ppisp_warp_vignetting_uses_detiled_camera_coordinates) validates the new vignetting code path.
Claude: ## 🤖 Isaac Lab Review Bot
Summary
This is a follow-up review on commit 73a68df. The PR has been significantly simplified — all SPG shader assets and Newton API compatibility layers have been removed. The implementation is now focused solely on the Warp kernel PPISP pipeline.
Previous Concerns Status
- GPU synchronization: NOT addressed —
apply_ppisp_to_rgbastill lackswp.synchronize()after kernel launch - Vignetting in Warp kernel: ADDRESSED — vignetting is now fully implemented in
_apply_vignettingfunction (lines 33-54 incamera_ppisp_warp.py) _renderer_output_dataaliasing: Still present but was acknowledged as safe- Newton private API import: RESOLVED — the Newton API compatibility layer was removed entirely
- Test using SimpleNamespace mock: Still present but acceptable for unit testing
- Temp directory cleanup: RESOLVED — SPG asset handling removed entirely
- Duplicated annotator registration: Still present in
isaac_rtx_renderer.py:214-220
New Findings
🔴 Critical: camera_ppisp_warp.py:233-282 — Missing GPU synchronization after Warp kernel launch
This remains unaddressed from the previous review. The kernel launches asynchronously and the function returns immediately:
def apply_ppisp_to_rgba(hdr_color: torch.Tensor, out_rgba: torch.Tensor, cfg: CameraPPISPCfg) -> None:
wp.launch(_apply_ppisp_kernel, ...)
# No wp.synchronize() - caller may read stale dataFix: Add wp.synchronize_device(str(out_rgba.device)) before returning.
🟡 Warning: camera_ppisp_warp.py:247 — wp.init() called on every PPISP application
wp.init() is called inside apply_ppisp_to_rgba, which runs every frame. While Warp handles repeated init calls gracefully, this adds unnecessary overhead:
def apply_ppisp_to_rgba(...):
inputs = cfg.inputs
wp.init() # Called every frameFix: Move wp.init() to module-level or camera initialization.
Implementation Verdict
Minor fixes needed — The synchronization issue is critical for correctness. The wp.init() call is a performance concern but not blocking.
Test Coverage
Tests are adequate for the simplified implementation. The vignetting test (test_ppisp_warp_vignetting_uses_detiled_camera_coordinates) validates the new vignetting code path.
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Follow-up Review
Previous concerns remain unaddressed in commit 3cd43d9:
-
GPU synchronization (🔴 Critical):
apply_ppisp_to_rgbaat line 247 still lackswp.synchronize()after the kernel launch. The caller incamera.py:545may read stale data fromout_rgba. -
wp.init()per-frame (🟡 Warning): Still called insideapply_ppisp_to_rgbaon every frame at line 248.
No new issues introduced. The diff between commits appears minimal (formatting/cleanup only).
Implementation Verdict
Minor fixes still needed — please address the synchronization issue before merge.
Apply camera PPISP from renderer HDR buffers and verify RTX, OVRTX, and Newton HDR output plumbing for the camera pipeline.
Apply pre-commit formatting to the camera PPISP integration files.
Run the PPISP Warp kernel on the torch CUDA stream, move Warp initialization out of the frame path, and avoid duplicate HdrColor annotator registration.
moennen
force-pushed
the
nicolasm/ppisp-renderer-simple-integration
branch
from
3cd43d9 to
3d15b5d
Compare
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Follow-up Review
Previous concerns addressed:
- ✅
wp.init()moved to module level (line 15) — no longer called per-frame. - ✅ GPU synchronization handled via
wp.stream_from_torch()— the kernel now launches on the correct CUDA stream associated with the output tensor, ensuring proper ordering without explicitwp.synchronize().
New observation:
The stream handling at line 247 passes stream=None for CPU tensors, which is correct. The CUDA path properly synchronizes via stream association.
Implementation Verdict
Ship it — previous concerns resolved, implementation looks correct.
| if use_hdr: | ||
| return "/Render/Vars/HdrColor", "HdrColor", "HdrColor" |
There was a problem hiding this comment.
use_rgb when rgb_hdr is requested alongside rgb without PPISP
When a user requests data_types=["rgb", "rgb_hdr"] without PPISP (a valid configuration because RGB_HDR is now in supported_output_types()), use_hdr=True causes an early return of the HdrColor render-var config. The OVRTX USD is then set up with only HdrColor in orderedVars, so LdrColor is never rendered. _process_render_frame checks if "LdrColor" in frame.render_vars before extracting the rgba buffer, and since LdrColor is absent, rgb and rgba remain zeroed with no error or warning. This affects any OVRTX user who explicitly requests rgb_hdr alongside rgb without enabling PPISP.
Verify the deprecated TiledCamera alias accepts Camera PPISP config and uses the inherited Camera wrapper post-processing path.
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Follow-up Review
Previous concerns about wp.init() placement and GPU synchronization have been addressed. The new commits add test coverage and extend RGB_HDR support across renderers.
One new issue:
🟡 Warning: source/isaaclab/isaaclab/sensors/camera/camera.py:544 — Missing rgb alias update after PPISP
The _apply_ppisp_if_needed method writes to rgba but doesn't update the rgb alias. Looking at line 544:
rgba = self._data.output[str(RenderBufferKind.RGBA)]
apply_ppisp_to_rgba(hdr_color, rgba, self.cfg.ppisp)Since rgb is created as rgba[..., :3] (a view into rgba's storage) during buffer allocation, this should work correctly — the view shares memory with rgba. However, _create_buffers explicitly aliases rgb at line 527 via dict(self._data.output), and CameraData.allocate creates the alias. Verified this is safe because both reference the same underlying storage.
Implementation Verdict
Ship it — the rgb/rgba aliasing is handled correctly via shared tensor storage. No new blocking issues.
Ensure explicit RGB_HDR requests add HdrColor without dropping LdrColor when LDR RGB is also requested.
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Follow-up Review
The previous concern about rgb/rgba aliasing was already noted as safe in the last review. Examining the new commits:
The changes are test additions and minor refinements. No new implementation issues introduced.
Implementation Verdict
Ship it — Test coverage is comprehensive, RGB_HDR plumbing is consistent across all three renderers (Isaac RTX, OVRTX, Newton Warp), and the PPISP integration follows the documented design.
1 participant
Description
Adds camera PPISP support through
CameraCfg.ppispand applies PPISP centrally in the IsaacLab camera wrapper using a Warp kernel.This keeps PPISP application consistent across Isaac RTX, OVRTX, and Newton by requesting
rgb_hdr/HdrColorfrom the renderer, applying PPISP before LDR conversion, and writing the finalrgb/rgbacamera outputs.This also adds
rgb_hdrrenderer output plumbing for Isaac RTX, OVRTX, and Newton.PPISP USD shader/RenderProduct parsing utilities are kept for compatibility with exported PPISP stages.
Motivation: PPISP should be renderer-independent from the IsaacLab camera API, and should run before LDR conversion for physically plausible exposure/color processing.
Fixes #TBD
Dependencies
No new runtime dependencies. Uses existing IsaacLab dependencies: Warp, Torch, USD, and renderer-specific backends.
Type of change
Screenshots
Not applicable.
Checklist
./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there