fix(stats): improve presentation pacing observability - #66
Conversation
|
Warning Review limit reached
Next review available in: 44 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthrough本次变更新增呈现时序诊断与滚动帧率跟踪，接入 NativeRender 的 Host-paced VSync 采样路径及 VideoDecoder 的接收、呈现帧率统计流程，并增加构建配置和行为测试。 Changes呈现可观测性与帧率统计
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 1
🧹 Nitpick comments (2)
nativelib/src/main/cpp/native_render.cpp (1)
34-53: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value注意全局变量在多实例场景下的副作用
这里使用全局原子变量和代次（generation）计数器来管理 VSync 回调状态，这是一种非常聪明且实用的做法，能够有效避免
OH_NativeVSync_Destroy后残留回调导致的悬挂this指针（dangling pointer）问题。但是，请留意如果应用在生命周期中会同时存在多个
NativeRender实例：
- 只有最新创建的实例能成功接收到 VSync 回调（因为它的代次匹配最新的全局
g_vsyncSampleGeneration）。- 当销毁任意一个实例时，
ReleaseNativeVSync会自增全局代次，从而导致所有其他仍在活跃的实例的 VSync 采样也会被永久阻断。如果该组件在业务中严格作为单例使用（或者同一时间只有一个实例存在），这种实现是完全安全且高效的。如果未来需要支持多实例同时渲染，建议考虑使用基于堆分配的控制块（结合
std::shared_ptr管理），或使用实例 ID 注册表来安全地分发回调上下文。🤖 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 `@nativelib/src/main/cpp/native_render.cpp` around lines 34 - 53, 调整 NativeRender 的 VSync 回调状态管理，避免全局 g_vsyncSampleGeneration 及相关全局采样变量让多个实例相互阻断：为每个实例使用独立且生命周期安全的回调上下文（例如堆分配控制块配合 shared_ptr），并确保 ReleaseNativeVSync 仅使对应实例失效，不影响其他活跃实例的采样。nativelib/src/main/cpp/video_decoder.cpp (1)
1611-1614: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win将计数器递增移入锁内以保证时序与数据一致性
decodedFrames_的递增操作目前在获取renderedRateMutex_锁之前执行。如果在极端并发情况下有多个解码输出回调交错，可能会导致写入renderedFrameRate_时的cumulativeFrames出现倒挂，从而触发RollingFrameRateTracker内部的防抖逻辑（清空历史记录Reset()）。建议将
fetch_add移入锁保护范围内，以确保帧数递增和写入记录操作的严格一致性。♻️ 建议的重构方案
- const uint64_t decodedFrames = - decodedFrames_.fetch_add(1, std::memory_order_relaxed) + 1; std::lock_guard<std::mutex> lock(renderedRateMutex_); + const uint64_t decodedFrames = + decodedFrames_.fetch_add(1, std::memory_order_relaxed) + 1; renderedFrameRate_.Record(currentTimeMs, decodedFrames);🤖 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 `@nativelib/src/main/cpp/video_decoder.cpp` around lines 1611 - 1614, Move the decodedFrames_ fetch_add operation inside the renderedRateMutex_ lock in the decode-output callback, keeping the increment immediately before renderedFrameRate_.Record. Ensure both the counter update and rate-record write are serialized together so cumulativeFrames remains monotonic.
🤖 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 `@nativelib/src/main/cpp/rolling_frame_rate.cpp`:
- Around line 75-84: Update the frame-rate calculation around
baseline.timestampMs, cutoffMs, and elapsedMs so the effective interval starts
at the later of baseline.timestampMs and cutoffMs. Preserve the existing
invalid-interval and counter-regression checks, and calculate elapsedMs and
frame rate using this window-bounded start time.
---
Nitpick comments:
In `@nativelib/src/main/cpp/native_render.cpp`:
- Around line 34-53: 调整 NativeRender 的 VSync 回调状态管理，避免全局 g_vsyncSampleGeneration
及相关全局采样变量让多个实例相互阻断：为每个实例使用独立且生命周期安全的回调上下文（例如堆分配控制块配合 shared_ptr），并确保
ReleaseNativeVSync 仅使对应实例失效，不影响其他活跃实例的采样。
In `@nativelib/src/main/cpp/video_decoder.cpp`:
- Around line 1611-1614: Move the decodedFrames_ fetch_add operation inside the
renderedRateMutex_ lock in the decode-output callback, keeping the increment
immediately before renderedFrameRate_.Record. Ensure both the counter update and
rate-record write are serialized together so cumulativeFrames remains monotonic.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 702ae83d-d013-44e3-8b43-0cf592c5d044
📒 Files selected for processing (10)
nativelib/src/main/cpp/CMakeLists.txtnativelib/src/main/cpp/native_render.cppnativelib/src/main/cpp/native_render.hnativelib/src/main/cpp/presentation_diagnostics.cppnativelib/src/main/cpp/presentation_diagnostics.hnativelib/src/main/cpp/rolling_frame_rate.cppnativelib/src/main/cpp/rolling_frame_rate.hnativelib/src/main/cpp/video_decoder.cppnativelib/src/main/cpp/video_decoder.hnativelib/src/test/cpp/presentation_observability_test.cpp
* fix(render): schedule frames on unique vsync slots * fix(render): preserve late-frame rebuffer threshold
1 participant
改了啥呀
为啥要改
之前那个杂鱼 1 秒窗口会把 120 FPS 的批量回调显示成偶发 115–117，害我们拿统计抖动去调未来余量。现在先把统计口径和 VSync 证据补齐，下一轮才能区分是真掉帧、同槽覆盖，还是目标时间倒退啦。
验证
c++ -std=c++17 -Wall -Wextra -Werror nativelib/src/main/cpp/rolling_frame_rate.cpp nativelib/src/main/cpp/presentation_diagnostics.cpp nativelib/src/test/cpp/presentation_observability_test.cpp -I nativelib/src/main/cpp -o /tmp/moonlight_presentation_observability_test && /tmp/moonlight_presentation_observability_testc++ -std=c++17 -Wall -Wextra -Werror nativelib/src/main/cpp/presentation_scheduler.cpp nativelib/src/test/cpp/presentation_scheduler_test.cpp -I nativelib/src/main/cpp -o /tmp/moonlight_presentation_scheduler_test && /tmp/moonlight_presentation_scheduler_testnpm run checkenv JAVA_HOME=/Applications/DevEco-Studio.app/Contents/jbr/Contents/Home OHOS_SDK_HOME=/Users/mac/ohos-sdk-cache/6.1-Release-mac/sdk-ci-shape HOS_SDK_HOME=/Users/mac/ohos-sdk-cache/6.1-Release-mac/sdk-ci-shape OHOS_BASE_SDK_HOME=/Users/mac/ohos-sdk-cache/6.1-Release-mac/sdk-ci-shape DEVECO_SDK_HOME=/Users/mac/ohos-sdk-cache/6.1-Release-mac/sdk-ci-shape node hvigorw.js assembleApp --mode project -p product=default -p buildMode=debug --no-daemonSummary by CodeRabbit