fix(sensor): repair latent bugs in GPU pixel readback path#9744
fix(sensor): repair latent bugs in GPU pixel readback path#9744youtalk wants to merge 5 commits into
Conversation
`FPixelReader::SendPixelsInRenderThread` bailed out with
if (!Sensor.HasActorBegunPlay() || IsValidChecked(&Sensor))
return;
`IsValidChecked` returns true for a *live* sensor, so the guard returned
early for every valid, begun-play sensor and the capture/stream path
never ran. The intent is to skip only when the sensor has not begun play
or is no longer valid.
Negate the validity check so a valid, begun-play sensor proceeds.
This path has had no callers on ue5-dev, so the inverted guard sat
dormant; the wide-angle (fisheye) sensors are the first code to exercise
it.
Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
`WritePixelsToBuffer` spawned its readback-completion task with `AsyncTask(ENamedThreads::HighTaskPriority, ...)`. That priority can be scheduled onto the RHI thread, where the task then busy-waits on `FRHIGPUTextureReadback::IsReady()`. The readback only becomes ready once the RHI thread itself submits and signals the GPU copy, so the RHI thread ends up waiting on work only it can perform: the render thread blocks on `FRHICommandListExecutor::Submit`, the game thread blocks on `FlushRenderingCommands`, and the whole pipeline deadlocks. Schedule the task on `AnyBackgroundThreadNormalTask` so it runs on a generic worker pool thread, leaving the RHI thread free to drive GPU completion. Latent on ue5-dev (no callers); first exercised by the wide-angle fisheye sensors. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
…yload `WritePixelsToBuffer` called `Readback->Lock(Size)` with `Size` holding the full payload length. `FRHIGPUTextureReadback` exposes two overloads: the deprecated `Lock(uint32 NumBytes)` and the current `Lock(int32& OutRowPitchInPixels, ...)`. Because `Size` is an `int32` lvalue, overload resolution binds the reference overload exactly, so `Lock` overwrites `Size` with the row stride in pixels (e.g. 1280). The subsequent `FuncForSending(LockedData, Size, ...)` then streamed only one row's worth of pixels, and the client failed to reshape the buffer. Pass a dedicated `RowPitchInPixels` out-parameter and leave `Size` intact. Latent on ue5-dev (no callers); first exercised by the wide-angle fisheye sensors. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
|
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would update our CHANGELOG.md based on your changes. |
There was a problem hiding this comment.
Pull request overview
Fixes latent issues in CARLA’s Unreal GPU pixel readback path (FPixelReader::SendPixelsInRenderThread / WritePixelsToBuffer) that became visible once new sensor code began exercising this (previously unused) path.
Changes:
- Corrects an inverted early-return condition in
SendPixelsInRenderThread. - Moves the GPU-readback wait off render/RHI named threads to avoid a potential render-pipeline deadlock.
- Fixes a
FRHIGPUTextureReadback::Lock(...)misuse that previously truncated the streamed payload.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| Unreal/CarlaUnreal/Plugins/Carla/Source/Carla/Sensor/PixelReader.h | Fixes the early-out condition that prevented valid sensors from sending pixels. |
| Unreal/CarlaUnreal/Plugins/Carla/Source/Carla/Sensor/PixelReader.cpp | Adjusts async scheduling and readback locking to avoid deadlock and truncation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
It looks like your settings are out of sync with the last configuration. You can run |
|
Thanks for the suggestion, but the stale-config path doesn't apply here.
The original CarlaUE4 code read: if (!Sensor.HasActorBegunPlay() || Sensor.IsPendingKill())
return;
if (!Sensor.HasActorBegunPlay() || IsValidChecked(&Sensor)) // polarity flipped
return;So every valid, playing sensor now returns before capturing anything, producing only black frames. Deleting the config file would not change this. The fix in this PR ( |
The AsyncTask lambda captured FuncForSending by copy via [=], copying the (potentially large) std::function payload on every frame. It is not used after the task is dispatched, so capture it with an init-capture that moves it instead. Addresses a Copilot review comment on PR carla-simulator#9744. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
Addresses the update-docs bot reminder on PR carla-simulator#9744. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
|
You're right. I meant that the rendering quality in your screenshot looked like it was affected by an older configuration. My suggestion is not related to this issue; I forgot to add that disclaimer. |
|
Oh, I'm sorry about my misunderstand. I've tried the Before: After: |
|
Now it looks good; in the first screenshot, the car's rendering quality looked poorer. Probably did you run it in Low mode? I want to ensure there are no unexpected issues with the recent changes to improve GPU performance. I'm checking that it doesn't introduce unexpected issues. It looks all right. Sorry for those off-topic comments. Let's get back to your original concerns. |
| // thread. The task busy-waits on FRHIGPUTextureReadback::IsReady(), whose | ||
| // completion the RHI thread itself has to drive; scheduling it on the RHI | ||
| // (or render) thread deadlocks the whole pipeline. | ||
| AsyncTask(ENamedThreads::AnyBackgroundThreadNormalTask, |
There was a problem hiding this comment.
The original intent was high-priority scheduling.
Since you're fixing the dispatch anyway, consider using AnyBackgroundHiPriTask instead of AnyBackgroundThreadNormalTask to preserve that intent
Description
FPixelReader::SendPixelsInRenderThread/WritePixelsToBuffercarried three latent bugs. The path had no callers onue5-dev(dead code since theCarlaUE4→CarlaUnrealrename), so the bugs were dormant.The wide-angle fisheye sensors (separate PR) are the first code to exercise it, which surfaced all three. Fixing them here keeps the fisheye PR scoped to fisheye work.
Inverted validity guard (
PixelReader.h) — the early-out usedIsValidChecked(&Sensor)without negation, so every valid sensor returned before capturing. The sensor produced only black frames.Render-pipeline deadlock (
PixelReader.cpp) — the readback-wait task was posted withAsyncTask(ENamedThreads::HighTaskPriority, ...), which can land on the RHI thread. There it busy-waits onFRHIGPUTextureReadback::IsReady(), whose completion the RHI thread must itself drive — deadlocking game + render + RHI threads. Moved toAnyBackgroundThreadNormalTask.Truncated payload (
PixelReader.cpp) —Readback->Lock(Size)binds the currentLock(int32& OutRowPitchInPixels)overload, which overwritesSizewith the row stride in pixels. Only one row was streamed. Now uses a dedicatedRowPitchInPixelsout-parameter.Where has this been tested?
Verified by building the packaged Shipping server and running the wide-angle camera sensors end-to-end: capture, GPU readback, and client-side image delivery all succeed.
Before:
After:
Possible Drawbacks
None expected. The code path is currently unreachable on
ue5-dev, sothese changes only correct behaviour for future callers; no existing
sensor path changes.
This change is