perf: reduce artwork, subtitle and playback statistics work - #35
Conversation
Preserve shared image consumers while cancelling abandoned requests and queued decodes. Limit routine statistics projection while keeping recovery and timeline buffers current. Add focused iOS and tvOS regression coverage.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: blurbery/vivid/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (8)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughImage requests now track separate consumers and cancel shared transfers when the last consumer leaves. External subtitle cues use indexed boundaries and publish only when selections change. Playback statistics use delivered telemetry samples, update buffer measurements independently of formatting cadence, and include added tests and documentation. ChangesImage pipeline cancellation
External subtitle cue selection
Playback statistics telemetry
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Refactor Sequence Diagram(s)sequenceDiagram
participant ImageRequest
participant VividImageFlights
participant SharedTask
ImageRequest->>VividImageFlights: register or cancel waiter
VividImageFlights->>SharedTask: start or join shared transfer
VividImageFlights->>SharedTask: cancel when no waiters remain
sequenceDiagram
participant TelemetryPublisher
participant VividPlaybackController
participant PlayerViewModel
participant VividPlaybackStatsCadence
TelemetryPublisher->>VividPlaybackController: deliver telemetry sample
VividPlaybackController->>PlayerViewModel: emit telemetryChanged with sample
PlayerViewModel->>VividPlaybackStatsCadence: check refresh cadence
PlayerViewModel->>PlayerViewModel: update buffer measurements
Merge Risk: ⚪ Minimal · up to No actionable merge-blocking issue was established for the current change. It is ready to merge after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The changes remain within the app’s artwork and playback flows. The reviewed paths preserve account-scoped artwork requests and player-owned subtitle state, with no identified new security exposure. Some broader security and runtime coverage remains unverified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 3.80% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 79 functions across 9 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 review Please review the artwork cancellation and playback-statistics changes, particularly shared-consumer cancellation races, account isolation, immediate buffer updates and regression coverage. |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: blurbery/vivid/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 7a8d8021-be80-439b-b827-12988161fbc3
📒 Files selected for processing (9)
docs/apple-tv-browsing.mddocs/playback/architecture.mdiosApp/Tests/ImageDataCoalescingTests.swiftiosApp/Tests/VividPlaybackStatsProjectionTests.swiftiosApp/iosApp/Screens/Player/PlayerViewModel.swiftiosApp/iosApp/Screens/Player/VividPlaybackController.swiftiosApp/iosApp/Screens/Player/VividPlaybackStatsProjection.swiftiosApp/iosApp/Shared/VividImagePipeline.swiftiosApp/project.yml
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
blurbery
changed the title
|
@coderabbitai review |
|
|
@coderabbitai review Please review the combined changes in ef893c4, including the subtitle cursor and publication checks from #36 and the fixes for both earlier findings. The combined iOS CI suite and tvOS build are green; targeted simulator tests passed on both platforms. |
✅ Action performedReview finished.
|
1 participant
Problem and reproduction
Fast catalogue browsing could leave artwork work running after its last consumer disappeared. Playback telemetry repeatedly rebuilt statistics, and every playback tick scanned and republished external subtitle cues even when nothing changed.
This PR combines the artwork/statistics changes with the subtitle change in separate PR #36 for one GitHub release. Metadata-cache retention is outside this change.
Implementation and impact
Validation
ef893c40a231c0e95d9f4b65f412b9e4ec5514fe: 43 targeted iOS tests and 40 tvOS tests passed on OS 27.0 simulators, both with complete result bundles and successful runner exits.xcodebuild build-for-testingfollowed byxcodebuild test-without-building, with sequential platform execution and test parallelism disabled. Suites:ImageDataCoalescingTests,VividPlaybackStatsProjectionTests,VividSubtitleCueCursorTests, plus three existing iOS ASS/SRT/WebVTT loader tests.python3 scripts/tests/playback-end-recovery-tests.pypassed 178 end/recovery and 33 early-display checks;python3 scripts/tests/playback-auth-refresh-tests.pypassed 29 checks.git diff --checkpassed. The combined Player regression run passed foref893c4: 1,077 iOS tests with 2 skipped and 0 failures, plus the tvOS simulator build, focused script checks and synthetic playback preview check. The earlier perf: avoid redundant external subtitle scans and updates #36 CI run failed one exact seek-time assertion because the decoder landed at 0.125 seconds for a 0.1-second request. The test now allows a 0.05-second frame tolerance while still checking seek completion, pause intent and load identity; both simulator reruns passed.Performance
No application benchmark was run. The cadence test permits two projections from 1,000 synthetic timestamps across one second. Subtitle tests observe no empty publications with subtitles off, and one publication across 1,000 unchanged-caption clock updates.
Testing status
ef893c40a231c0e95d9f4b65f412b9e4ec5514fe.ef893c4revision, with no unresolved findings.Design impact and approval
Documentation impact
Updated the existing browsing and playback architecture guides for image cancellation, statistics cadence and subtitle cue updates.
AI disclosure
Implemented these changes with Codex as a tool.