Implement Phase 1 performance optimizations - #86
Conversation
|
Warning Review limit reached
Next review available in: 43 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughChangesThe PR adds performance metrics, revision-aware timeline caching, guarded video decoding, synchronized ECS rendering, configurable preview and encoder settings, a QML splash view, and render-synchronized export capture. Performance and rendering pipeline
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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: 9
🧹 Nitpick comments (4)
core/include/ffmpeg_video_buffer.hpp (1)
27-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe subsampled plane-height table exists in two places. Both sites list
Format_YUV420P,Format_YV12,Format_NV12,Format_NV21,Format_P010, andFormat_YUV420P10, and both apply the same(height + 1) / 2rule for planes after plane 0. The buffer mapping and the cache-cost accounting must agree on this table. If a format is added to one site only, the mapped plane size and the recorded frame cost diverge silently.
core/include/ffmpeg_video_buffer.hpp#L27-L44: replace the inlineswitchinmap()with a call to one shared plane-height helper.core/src/video_decoder.cpp#L70-L84: move thestatic int planeHeight(...)helper into a shared header soframeStorageCost()andFFmpegVideoBuffer::map()use the same definition, then remove the local copy.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/include/ffmpeg_video_buffer.hpp` around lines 27 - 44, Centralize the subsampled plane-height logic by moving the static planeHeight(...) helper from frameStorageCost() in core/src/video_decoder.cpp:70-84 into a shared header, then remove the local copy. Update FFmpegVideoBuffer::map() in core/include/ffmpeg_video_buffer.hpp:27-44 to call that helper instead of its inline switch, preserving the shared format table and (height + 1) / 2 behavior for planes after plane 0; both sites must use the same definition.core/src/video_decoder.cpp (1)
693-697: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe Qt rotation API gate is correct.
Qt 6.5 provides
setRotationAngle(QVideoFrame::RotationAngle). Qt 6.7 providessetRotation(QtVideo::Rotation)and deprecates the older API. The project does not declare a minimum Qt version. Declare the supported minimum Qt version and compile against it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/src/video_decoder.cpp` around lines 693 - 697, Declare the project’s supported minimum Qt version as Qt 6.5, matching the existing setRotationAngle compatibility branch in the video frame rotation code. Update the project’s Qt version requirement/configuration rather than changing the version-gated APIs in the video decoder.Source: Linters/SAST tools
core/src/compute_render_node.cpp (1)
219-230: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider reusing one binding-construction path.
The full rebuild block builds the compute bindings inline from
m_extraTextures[i]->rhiTexture()at Lines 254-259.rebuildMainSrb()builds the same bindings fromm_extraRhiTexturesat Lines 358-363. Two sources of truth can diverge if the binding layout changes later.Extract the binding list construction into one helper and call it from both paths.
Also applies to: 240-259
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/src/compute_render_node.cpp` around lines 219 - 230, The full rebuild path and rebuildMainSrb() duplicate compute-binding construction using different texture sources. Extract the shared binding-list construction into a helper, make both the m_bufferLayoutDirty path and rebuildMainSrb() call it, and ensure the helper uses the same authoritative texture collection so future layout changes remain synchronized.tests/test_qml_composite_capture.cpp (1)
185-194: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider asserting the resulting
SceneEnvironmentstate.The test verifies
previewMsaaSamplesand theView3Dgeometry. It does not verify thatantialiasingModeandantialiasingQualityfollow the sample count. The mapping atui/qml/CompositeView.qmllines 677-680 is the part that changes rendering behavior. Readingview3D->property("environment")and checking both enum values would cover that mapping.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_qml_composite_capture.cpp` around lines 185 - 194, Extend the test around the previewMsaaSamples updates in the composite capture test to inspect view3D’s environment property and assert that the resulting SceneEnvironment antialiasingMode and antialiasingQuality match the configured sample count, covering the mapping in CompositeView.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/src/compute_render_node.cpp`:
- Around line 344-379: ensureBuffers() loses dirty-state recovery and leaves
stale RHI references when incremental resource creation fails. In
core/src/compute_render_node.cpp lines 344-379, update rebuildMainSrb() failure
handling to destroy m_pipeline and mark m_shaderDirty; lines 283-303 must mark
m_bufferLayoutDirty and destroy m_outputTextureB on both vertex-buffer and
uniform-buffer failures; lines 381-419 must destroy m_renderSrb and
m_renderPipeline and clear m_renderTexture immediately after the output-texture
rebuild failure; lines 458-471 must set m_passSrbDirty in the failure branch.
Preserve successful rebuild behavior while ensuring a later prepare() can
recreate all dependent resources.
- Around line 458-471: Update the create() failure branch in the multi-pass SRB
build loop to set m_passSrbDirty = true before breaking, while preserving the
existing resource cleanup and error assignment. This ensures prepare() retries
building the remaining pass SRBs after a failed creation.
In `@core/src/video_decoder.cpp`:
- Around line 405-412: In the request-update sequence, move the
m_lastRequestedFrame.store(frame, ...) operation before
m_requestGeneration.fetch_add(...). Preserve the existing memory orders and
subsequent compare_exchange logic so the worker’s generation and target-frame
reads remain consistent.
In `@tests/test_video_decoder.cpp`:
- Around line 245-247: Update the burst-frame assertions around burstFrameSpy to
remove the fixed 100 ms negative wait and replace the final last-frame check
with QTRY_VERIFY, requiring that burstFrameSpy.last().first().toInt() equals 36
while preserving the existing frame-36 delivery assertion.
In `@ui/include/bridge/ecs_render_bridge.hpp`:
- Around line 3-6: Add the missing <QVector> header include in
ecs_render_bridge.hpp before its QVector member declarations, preserving the
existing includes and declarations.
In `@ui/qml/CompositeView.qml`:
- Around line 25-31: Update the previewRenderScale property to detect when
Number(value) produces NaN and fall back to 1.0 before applying the existing
0.25–1.0 clamp, matching the strict-NaN handling used by previewMsaaSamples and
ensuring downstream View3D sizing receives a valid scale.
In `@ui/qml/settings/PerformanceSettingsPage.qml`:
- Around line 68-105: The labels and strategy values added to the settings pages
are not localized. Update ui/qml/settings/PerformanceSettingsPage.qml lines
68-105 and 126-153 to provide Japanese translations for the preview-rendering
and timeline-baking strings, including strategy values; update
ui/qml/settings/ExportSettingsPage.qml lines 158-160 to localize the encoder
queue memory label. Preserve the existing settings behavior and use the pages’
established qsTr translation approach.
In `@ui/src/bridge/ecs_render_bridge.cpp`:
- Around line 169-198: Latch the change result from syncSnapshot() in
ECSRenderBridge instead of discarding it in renderStates(), renderStateMap(),
getRenderState(), and getEffectParams(). Add a pending-change member in the
class, set it whenever an accessor or notifyFrameReady() observes a changed
snapshot, and have notifyFrameReady() increment m_renderRevision and emit
renderStatesChanged() when the latched flag is set, then clear it after
notification.
In `@ui/src/timeline/timeline_export_manager.cpp`:
- Around line 171-179: The export flow around waitForRenderFrame and grabToImage
must support minimized, hidden, or otherwise unexposed preview windows by
keeping the window exposed during export or using an offscreen rendering path;
do not rely on grabToImage as a fallback when exposure is unavailable. Apply
this to both video and image-sequence exports, and use separate error messages
for render-wait timeouts versus frame-grab failures.
---
Nitpick comments:
In `@core/include/ffmpeg_video_buffer.hpp`:
- Around line 27-44: Centralize the subsampled plane-height logic by moving the
static planeHeight(...) helper from frameStorageCost() in
core/src/video_decoder.cpp:70-84 into a shared header, then remove the local
copy. Update FFmpegVideoBuffer::map() in
core/include/ffmpeg_video_buffer.hpp:27-44 to call that helper instead of its
inline switch, preserving the shared format table and (height + 1) / 2 behavior
for planes after plane 0; both sites must use the same definition.
In `@core/src/compute_render_node.cpp`:
- Around line 219-230: The full rebuild path and rebuildMainSrb() duplicate
compute-binding construction using different texture sources. Extract the shared
binding-list construction into a helper, make both the m_bufferLayoutDirty path
and rebuildMainSrb() call it, and ensure the helper uses the same authoritative
texture collection so future layout changes remain synchronized.
In `@core/src/video_decoder.cpp`:
- Around line 693-697: Declare the project’s supported minimum Qt version as Qt
6.5, matching the existing setRotationAngle compatibility branch in the video
frame rotation code. Update the project’s Qt version requirement/configuration
rather than changing the version-gated APIs in the video decoder.
In `@tests/test_qml_composite_capture.cpp`:
- Around line 185-194: Extend the test around the previewMsaaSamples updates in
the composite capture test to inspect view3D’s environment property and assert
that the resulting SceneEnvironment antialiasingMode and antialiasingQuality
match the configured sample count, covering the mapping in CompositeView.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9fd20e48-6b07-4492-ad51-e7bdde490743
📒 Files selected for processing (35)
CMakeLists.txtcore/include/compute_render_node.hppcore/include/document_model.hppcore/include/ffmpeg_video_buffer.hppcore/include/performance_metrics.hppcore/include/video_decoder.hppcore/include/video_encoder.hppcore/src/compute_render_node.cppcore/src/document_model.cppcore/src/main.cppcore/src/performance_metrics.cppcore/src/settings_manager.cppcore/src/video_decoder.cppcore/src/video_encoder.cppengine/timeline/bake_controller.cppengine/timeline/bake_controller.hpptests/CMakeLists.txttests/test_bake_controller.cpptests/test_document_model.cpptests/test_ecs_render.cpptests/test_export_workflow.cpptests/test_performance_metrics.cpptests/test_phase1_performance.cpptests/test_qml_composite_capture.cpptests/test_settings_manager.cpptests/test_video_decoder.cppui/include/bridge/ecs_render_bridge.hppui/include/timeline_export_manager.hppui/qml/CompositeView.qmlui/qml/SceneRenderer.qmlui/qml/SplashView.qmlui/qml/settings/ExportSettingsPage.qmlui/qml/settings/PerformanceSettingsPage.qmlui/src/bridge/ecs_render_bridge.cppui/src/timeline/timeline_export_manager.cpp
| GroupBox { | ||
| title: qsTr("Preview rendering") | ||
| Layout.fillWidth: true | ||
|
|
||
| GridLayout { | ||
| columns: 2 | ||
| columnSpacing: 12 | ||
| rowSpacing: 8 | ||
| anchors.fill: parent | ||
|
|
||
| Label { | ||
| text: qsTr("Render scale") | ||
| } | ||
|
|
||
| ComboBox { | ||
| textRole: "text" | ||
| valueRole: "value" | ||
| model: [ | ||
| { "text": "100%", "value": 1.0 }, | ||
| { "text": "75%", "value": 0.75 }, | ||
| { "text": "50%", "value": 0.5 }, | ||
| { "text": "25%", "value": 0.25 } | ||
| ] | ||
| currentIndex: { | ||
| const value = Number(root.valueOr("previewRenderScale", 1.0)); | ||
| if (value <= 0.25) | ||
| return 3; | ||
| if (value <= 0.5) | ||
| return 2; | ||
| if (value <= 0.75) | ||
| return 1; | ||
| return 0; | ||
| } | ||
| onActivated: root.setValue("previewRenderScale", currentValue) | ||
| } | ||
|
|
||
| Label { | ||
| text: qsTr("Preview antialiasing") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use translated labels in the settings pages.
The new labels display in English in UI sections that otherwise display Japanese. Add Japanese source strings or provide translations for these qsTr() messages.
ui/qml/settings/PerformanceSettingsPage.qml#L68-L105: Localize the preview rendering labels.ui/qml/settings/PerformanceSettingsPage.qml#L126-L153: Localize the timeline baking labels and strategy values.ui/qml/settings/ExportSettingsPage.qml#L158-L160: Localize the encoder queue memory label.
📍 Affects 2 files
ui/qml/settings/PerformanceSettingsPage.qml#L68-L105(this comment)ui/qml/settings/PerformanceSettingsPage.qml#L126-L153ui/qml/settings/ExportSettingsPage.qml#L158-L160
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ui/qml/settings/PerformanceSettingsPage.qml` around lines 68 - 105, The
labels and strategy values added to the settings pages are not localized. Update
ui/qml/settings/PerformanceSettingsPage.qml lines 68-105 and 126-153 to provide
Japanese translations for the preview-rendering and timeline-baking strings,
including strategy values; update ui/qml/settings/ExportSettingsPage.qml lines
158-160 to localize the encoder queue memory label. Preserve the existing
settings behavior and use the pages’ established qsTr translation approach.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
i18n/AviQtl_zh_CN.ts (1)
3480-3497: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winLocale files are not synchronized with new Performance/Export settings strings.
i18n/AviQtl_ja_JP.tsadds translations for nine newPerformanceSettingsPagestrings (Preview rendering, Render scale, Preview antialiasing, Disabled, Timeline baking, Strategy, On demand, Bake all clips, Prefetch frames) and one newExportSettingsPagestring (Encoder queue memory (MB)).zh_CN.tsanden_US.tswere not regenerated/updated with the same entries, sotr()will fall back to the source text for these labels in both locales.
i18n/AviQtl_zh_CN.ts#L3480-L3497: add the nine newPerformanceSettingsPagemessage entries with Chinese translations, matching the set added inja_JP.tslines 3502-3546.i18n/AviQtl_zh_CN.ts#L2731-L2788: add the newExportSettingsPage"Encoder queue memory (MB)" message entry with a Chinese translation, matchingja_JP.tslines 2788-2792.i18n/AviQtl_en_US.ts#L3481-L3497: add the same ninePerformanceSettingsPagemessage entries (translation can equal the English source text).i18n/AviQtl_en_US.ts#L2732-L2788: add the same "Encoder queue memory (MB)" message entry (translation can equal the English source text).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@i18n/AviQtl_zh_CN.ts` around lines 3480 - 3497, Synchronize the locale files with the new settings messages: in i18n/AviQtl_zh_CN.ts lines 3480-3497 add the nine PerformanceSettingsPage entries with Chinese translations, and in lines 2731-2788 add ExportSettingsPage’s “Encoder queue memory (MB)” entry with a Chinese translation; in i18n/AviQtl_en_US.ts lines 3481-3497 add the same nine PerformanceSettingsPage entries and in lines 2732-2788 add the encoder queue entry, using the English source text as each translation.
🧹 Nitpick comments (1)
tests/test_export_workflow.cpp (1)
333-364: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
kTestSequencePaddinginstead of a hardcoded file name for consistency.
hiddenWindowExportsAreRestoredhardcodes"frame_0000.png"at line 352. Other tests in this file (for exampleimageSequenceCaptureFailureRemovesPartialOutput) build the expected file name fromkTestSequencePaddingviaarg(0, kTestSequencePadding, 10, QLatin1Char('0')). Build the expected path the same way here, so the test does not silently break ifkTestSequencePaddingchanges.♻️ Proposed fix
- QVERIFY(QFileInfo::exists(QDir(sequenceDir).filePath(QStringLiteral("frame_0000.png")))); + QVERIFY(QFileInfo::exists(QDir(sequenceDir).filePath(QStringLiteral("frame_%1.png").arg(0, kTestSequencePadding, 10, QLatin1Char('0')))));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_export_workflow.cpp` around lines 333 - 364, Update hiddenWindowExportsAreRestored to construct the expected frame filename using kTestSequencePadding and the existing arg-based zero-padding pattern, then use that generated path in the QFileInfo::exists assertion instead of the hardcoded frame_0000.png name.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@i18n/AviQtl_zh_CN.ts`:
- Around line 3480-3497: Synchronize the locale files with the new settings
messages: in i18n/AviQtl_zh_CN.ts lines 3480-3497 add the nine
PerformanceSettingsPage entries with Chinese translations, and in lines
2731-2788 add ExportSettingsPage’s “Encoder queue memory (MB)” entry with a
Chinese translation; in i18n/AviQtl_en_US.ts lines 3481-3497 add the same nine
PerformanceSettingsPage entries and in lines 2732-2788 add the encoder queue
entry, using the English source text as each translation.
---
Nitpick comments:
In `@tests/test_export_workflow.cpp`:
- Around line 333-364: Update hiddenWindowExportsAreRestored to construct the
expected frame filename using kTestSequencePadding and the existing arg-based
zero-padding pattern, then use that generated path in the QFileInfo::exists
assertion instead of the hardcoded frame_0000.png name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1d58dfe4-8bf2-4a97-a998-a203a6866782
📒 Files selected for processing (15)
CMakeLists.txtcore/include/ffmpeg_video_buffer.hppcore/src/compute_render_node.cppcore/src/video_decoder.cppi18n/AviQtl_en_US.tsi18n/AviQtl_ja_JP.tsi18n/AviQtl_zh_CN.tstests/test_ecs_render.cpptests/test_export_workflow.cpptests/test_qml_composite_capture.cpptests/test_video_decoder.cppui/include/bridge/ecs_render_bridge.hppui/qml/CompositeView.qmlui/src/bridge/ecs_render_bridge.cppui/src/timeline/timeline_export_manager.cpp
🚧 Files skipped from review as they are similar to previous changes (10)
- tests/test_ecs_render.cpp
- ui/src/bridge/ecs_render_bridge.cpp
- ui/qml/CompositeView.qml
- tests/test_qml_composite_capture.cpp
- CMakeLists.txt
- core/include/ffmpeg_video_buffer.hpp
- ui/include/bridge/ecs_render_bridge.hpp
- core/src/video_decoder.cpp
- tests/test_video_decoder.cpp
- core/src/compute_render_node.cpp
1 participant
Summary
Performance workload
The Phase 1 workload scrubs 121 frames across a 3,000-object timeline. It visits 8,405 clip candidates instead of 363,000 candidates for a full scan on every bake.
Testing
cmake --build .build_tmp/phase1/Release --clean-first -j 8ctest --test-dir .build_tmp/phase1/Release --output-on-failure -j 4Summary by CodeRabbit
New Features
Performance Improvements
Bug Fixes