Add pipeline_deffered_rd.h for the motion blur PR#1202
Add pipeline_deffered_rd.h for the motion blur PR#1202AR-DEV-1 wants to merge 1 commit intoRedot-Engine:masterfrom
pipeline_deffered_rd.h for the motion blur PR#1202Conversation
WalkthroughA new helper class Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant PipelineDeferredRD
participant WorkerThreadPool
participant RenderingDevice
Client->>PipelineDeferredRD: create_render_pipeline(...)
PipelineDeferredRD->>PipelineDeferredRD: Store CreationParameters
PipelineDeferredRD->>PipelineDeferredRD: _start(params)
PipelineDeferredRD->>WorkerThreadPool: Queue background task
WorkerThreadPool->>PipelineDeferredRD: _create(params)
PipelineDeferredRD->>RenderingDevice: Compile pipeline (render or compute)
RenderingDevice-->>PipelineDeferredRD: Pipeline RID
PipelineDeferredRD->>PipelineDeferredRD: Store pipeline RID
Client->>PipelineDeferredRD: get_rid()
PipelineDeferredRD->>PipelineDeferredRD: _wait()
PipelineDeferredRD-->>Client: Return pipeline RID
Client->>PipelineDeferredRD: free()
PipelineDeferredRD->>PipelineDeferredRD: _wait()
PipelineDeferredRD->>RenderingDevice: Release pipeline
PipelineDeferredRD->>PipelineDeferredRD: Clear state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
servers/rendering/renderer_rd/pipeline_deferred_rd.h (1)
82-84: Nit: remove vacuous constructor comment.
// Default constructor.adds no information over the empty body itself.✏️ Proposed fix
PipelineDeferredRD() { - // Default constructor. }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@servers/rendering/renderer_rd/pipeline_deferred_rd.h` around lines 82 - 84, Remove the vacuous inline comment inside the default constructor for PipelineDeferredRD; edit the PipelineDeferredRD() { /* ... */ } definition to have an empty body with no "// Default constructor." comment so the constructor remains concise and self-explanatory.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@servers/rendering/renderer_rd/pipeline_deferred_rd.h`:
- Around line 132-133: The file ends with the closing token "};" but is missing
a trailing newline; to fix the CI failure update the file so that after the
final "};" there is a single newline character at EOF (ensure the file
terminates with a newline), e.g., modify the end of pipeline_deferred_rd.h to
add a trailing newline after the closing "};".
- Line 127: There's a typo in the error message string inside the
ERR_FAIL_COND_MSG call that checks
RD::get_singleton()->render_pipeline_is_valid(pipeline) ||
RD::get_singleton()->compute_pipeline_is_valid(pipeline); change the message
from "`free()` must be called manually before the dependent shader is freed."
to use a single space after "called" (e.g. "`free()` must be called manually
before the dependent shader is freed.") so the message reads correctly.
- Around line 86-90: The destructor of PipelineDeferredRD must unconditionally
wait for any in-flight worker task before performing validation or returning:
call _wait() at the very start of ~PipelineDeferredRD() to ensure background
tasks (from create_render_pipeline()/create_compute_pipeline()) complete and do
not write into freed memory, then keep the DEV_ENABLED validation
(pipeline.is_valid()) after the wait; also ensure the task handle is
cleared/collected after waiting so it is not leaked. Ensure references to
pipeline and the worker task are only accessed after the _wait() call.
---
Nitpick comments:
In `@servers/rendering/renderer_rd/pipeline_deferred_rd.h`:
- Around line 82-84: Remove the vacuous inline comment inside the default
constructor for PipelineDeferredRD; edit the PipelineDeferredRD() { /* ... */ }
definition to have an empty body with no "// Default constructor." comment so
the constructor remains concise and self-explanatory.
| ~PipelineDeferredRD() { | ||
| #ifdef DEV_ENABLED | ||
| ERR_FAIL_COND_MSG(pipeline.is_valid(), "'free()' must be called manually before deconstruction and before the corresponding shader is freed."); | ||
| #endif | ||
| } |
There was a problem hiding this comment.
Critical: use-after-free when object is destroyed while a task is still in-flight.
The destructor's DEV_ENABLED guard only checks pipeline.is_valid(). However, if create_render_pipeline() (or create_compute_pipeline()) was called and the background task hasn't run yet, pipeline is still an invalid RID() — the guard silently passes, the destructor returns, the object's memory is freed, and the worker thread then writes into the freed pipeline field in _create.
Even when the guard does fire, ERR_FAIL_COND_MSG merely logs and returns — it does not abort, so the in-flight task still corrupts freed memory.
Additionally, per the WorkerThreadPool documentation, "every task must be waited for completion using wait_for_task_completion... so that any allocated resources inside the task can be cleaned up." Skipping _wait() in the destructor also leaks the task handle.
_wait() must be called unconditionally in the destructor before any validation logic.
🔒 Proposed fix
~PipelineDeferredRD() {
+ _wait(); // Prevent use-after-free: ensure background task has finished before memory is released.
`#ifdef` DEV_ENABLED
ERR_FAIL_COND_MSG(pipeline.is_valid(), "'free()' must be called manually before deconstruction and before the corresponding shader is freed.");
`#endif`
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@servers/rendering/renderer_rd/pipeline_deferred_rd.h` around lines 86 - 90,
The destructor of PipelineDeferredRD must unconditionally wait for any in-flight
worker task before performing validation or returning: call _wait() at the very
start of ~PipelineDeferredRD() to ensure background tasks (from
create_render_pipeline()/create_compute_pipeline()) complete and do not write
into freed memory, then keep the DEV_ENABLED validation (pipeline.is_valid())
after the wait; also ensure the task handle is cleared/collected after waiting
so it is not leaked. Ensure references to pipeline and the worker task are only
accessed after the _wait() call.
There was a problem hiding this comment.
Coderabbit, is there really anything to leak here? I see two protected class members -- pipeline and task. But I dont really see any place that these get passed outside of this class, and are only used inside of this class's methods, which should become inaccessible once this object is destroyed. So I am not seeing a use-after-free error here.
Looking at this PR, it seems that this is just a helper class to reduce the boiler plate to kick off the task. Am I missing something?
There was a problem hiding this comment.
@Arctis-Fireblight, great question! You're right that the members don't escape the class—but the use-after-free happens inside the class when the worker thread accesses this after the object is destroyed.
The use-after-free scenario:
// 1. User creates object and kicks off background compilation
PipelineDeferredRD pipeline_obj;
pipeline_obj.create_render_pipeline(...);
// This calls _start(), which queues a task on the worker thread.
// The task captures 'this' (a pointer to pipeline_obj).
// 2. Object goes out of scope BEFORE the task finishes
// Destructor runs, checks pipeline.is_valid() (false, so no error)
// Object memory is freed
// 3. Worker thread is STILL running and executes _create():
void _create(const CreationParameters &c) {
// 'this' now points to freed memory!
this->pipeline = RD::get_singleton()->compute_pipeline_create(...); // ❌ Writing to freed memory
}The worker thread holds a raw this pointer captured by add_template_task(). When it finally executes _create(), it writes to this->pipeline—but if the object was already destroyed, that's a use-after-free write.
The task leak:
Per WorkerThreadPool documentation, every task must be waited on with wait_for_task_completion so resources can be cleaned up. Without _wait() in the destructor, the task handle may leak internal WorkerThreadPool resources.
The fix:
Call _wait() unconditionally at the start of the destructor to ensure the background task finishes before the object's memory is freed:
~PipelineDeferredRD() {
_wait(); // Block until worker thread completes
`#ifdef` DEV_ENABLED
ERR_FAIL_COND_MSG(pipeline.is_valid(), "'free()' must be called manually...");
`#endif`
}Does that clarify the issue? 🐰
There was a problem hiding this comment.
Ok, yeah, I missed the this pointer in _start(). This is 100% a valid issue.
There was a problem hiding this comment.
@Arctis-Fireblight, thanks for confirming! Yes, the this pointer capture in add_template_task() is the culprit—easy to miss at first glance.
Let me know if you'd like any help with the fix or if you have questions about the implementation. 🐰
|
|
||
| if (pipeline.is_valid()) { | ||
| #ifdef DEV_ENABLED | ||
| ERR_FAIL_COND_MSG(!(RD::get_singleton()->render_pipeline_is_valid(pipeline) || RD::get_singleton()->compute_pipeline_is_valid(pipeline)), "`free()` must be called manually before the dependent shader is freed."); |
There was a problem hiding this comment.
Typo: double space in error message.
"must be called manually" has two consecutive spaces.
✏️ Proposed fix
- ERR_FAIL_COND_MSG(!(RD::get_singleton()->render_pipeline_is_valid(pipeline) || RD::get_singleton()->compute_pipeline_is_valid(pipeline)), "`free()` must be called manually before the dependent shader is freed.");
+ ERR_FAIL_COND_MSG(!(RD::get_singleton()->render_pipeline_is_valid(pipeline) || RD::get_singleton()->compute_pipeline_is_valid(pipeline)), "`free()` must be called manually before the dependent shader is freed.");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ERR_FAIL_COND_MSG(!(RD::get_singleton()->render_pipeline_is_valid(pipeline) || RD::get_singleton()->compute_pipeline_is_valid(pipeline)), "`free()` must be called manually before the dependent shader is freed."); | |
| ERR_FAIL_COND_MSG(!(RD::get_singleton()->render_pipeline_is_valid(pipeline) || RD::get_singleton()->compute_pipeline_is_valid(pipeline)), "`free()` must be called manually before the dependent shader is freed."); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@servers/rendering/renderer_rd/pipeline_deferred_rd.h` at line 127, There's a
typo in the error message string inside the ERR_FAIL_COND_MSG call that checks
RD::get_singleton()->render_pipeline_is_valid(pipeline) ||
RD::get_singleton()->compute_pipeline_is_valid(pipeline); change the message
from "`free()` must be called manually before the dependent shader is freed."
to use a single space after "called" (e.g. "`free()` must be called manually
before the dependent shader is freed.") so the message reads correctly.
| } | ||
| }; No newline at end of file |
There was a problem hiding this comment.
CI failure: missing newline at end of file.
The pre-commit file-format hook failed because the file has no trailing newline. Add a newline after the closing }; to fix this.
✏️ Proposed fix
};
+📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } | |
| }; | |
| } | |
| }; | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@servers/rendering/renderer_rd/pipeline_deferred_rd.h` around lines 132 - 133,
The file ends with the closing token "};" but is missing a trailing newline; to
fix the CI failure update the file so that after the final "};" there is a
single newline character at EOF (ensure the file terminates with a newline),
e.g., modify the end of pipeline_deferred_rd.h to add a trailing newline after
the closing "};".
|
@AR-DEV-1 do you mind running |
2 participants
TL;DR
pipeline_deffered_rd.hfor the motion blur PR (Add motion blur support #1201)Note
Contributed by 2LazyDevs.
Summary by CodeRabbit