feat(cdg): overhaul CD+G playback for correctness and robustness#113
feat(cdg): overhaul CD+G playback for correctness and robustness#113thedavidweng wants to merge 13 commits into
Conversation
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #113 +/- ##
=======================================
+ Coverage 77.3% 79.0% +1.6%
=======================================
Files 126 127 +1
Lines 6266 6374 +108
Branches 1984 2044 +60
=======================================
+ Hits 4846 5037 +191
+ Misses 1376 1293 -83
Partials 44 44
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Greptile SummaryThis PR overhauls CD+G playback across local, fullscreen, and AirPlay paths. The main changes are:
Confidence Score: 5/5Safe to merge based on the reviewed CDG playback changes. The backend lifecycle, frontend protocol handling, status reporting, and prior reported CDG issues are consistent on HEAD. No files require special attention.
 T-Rex LogsWhat T-Rex did
Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant UI as React CDG sync
participant IPC as Tauri CDG IPC
participant Coord as PlaybackCoordinator
participant Slot as CdgPlaybackSlot
participant Local as Local timeline
participant AirPlay as AirPlay timeline
Coord->>Slot: mark loading / attach packets / mark error
UI->>IPC: get_cdg_frame(songId, generation, positionMs, lastFrameVersion)
IPC->>Slot: validate songId + transportGeneration
alt active ready CDG
IPC->>Local: advance_cdg_timeline(positionMs)
Local-->>IPC: header-only or RGBA frame
IPC-->>UI: binary envelope
else no CDG / stale / error
IPC-->>UI: 0-byte response
UI->>IPC: get_cdg_status(songId, generation)
IPC-->>UI: availability + errorCode
end
AirPlay->>Slot: native phase-gated runtime tick
Slot->>AirPlay: advance separate AirPlay timeline
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant UI as React CDG sync
participant IPC as Tauri CDG IPC
participant Coord as PlaybackCoordinator
participant Slot as CdgPlaybackSlot
participant Local as Local timeline
participant AirPlay as AirPlay timeline
Coord->>Slot: mark loading / attach packets / mark error
UI->>IPC: get_cdg_frame(songId, generation, positionMs, lastFrameVersion)
IPC->>Slot: validate songId + transportGeneration
alt active ready CDG
IPC->>Local: advance_cdg_timeline(positionMs)
Local-->>IPC: header-only or RGBA frame
IPC-->>UI: binary envelope
else no CDG / stale / error
IPC-->>UI: 0-byte response
UI->>IPC: get_cdg_status(songId, generation)
IPC-->>UI: availability + errorCode
end
AirPlay->>Slot: native phase-gated runtime tick
Slot->>AirPlay: advance separate AirPlay timeline
Reviews (5): Last reviewed commit: "fix(cdg): report backend error states in..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Maintainer acceptance — isolated high-risk slot 13BLOCKED — changes required. Blocking CDG request overlapThe song-probe effect sends Use one request coordinator/sequence for probe and hot paths, and add a deterministic delayed-probe + position-tick test proving one request at a time and that the latest generation/version wins. This PR also still needs the substantial local/fullscreen/AirPlay manual matrix required by #99. Keep it isolated from routine merges after the automated fix and fresh CI. |
Re-review: blocking CD+G request-coordination fix and manual timeline matrix requiredThe song-probe effect and the hot playback loop currently issue Implement one explicit request coordinator shared by all CD+G frame callers:
Add deferred-promise regression tests that:
Then perform the required packaged manual matrix for local, fullscreen, and AirPlay timelines (start, pause/resume, seek, song change, handoff/return). Attach concise evidence and exact test results. This remains a high-risk late merge candidate even after CI is green. |
71cebc9 to
dc26653
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
b010246 to
b917527
Compare
Re-review — blocked: the CDG coordinator invalidates every slow in-flight frame requestReviewed at At the intended 33ms cadence, any Separate lifecycle invalidation from request coalescing:
Add a deferred-promise regression with position ticks arriving faster than IPC completion: enqueue A, enqueue B/C while A is in flight, resolve A, assert A is applied and only the latest pending request is issued, then resolve it and assert it is applied. Keep the existing invalidation test proving a song/generation change drops the old response. This is a real playback-display liveness failure; no LGTM until it is fixed and covered. |
Re-review — liveness fix verified; one exported contract correction remainsReviewed at Please still correct the public |
The JSDoc claimed request() 'returns false if dropped as identical/ noop', but the method returns void. Updated the doc to accurately describe the coalescing behavior and direct callers to isInFlight() for test assertions.
|
@greptile review |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…eck (#113) The frontend songHasCdgMedia() early-return skipped the getCdgFrame probe whenever a song lacked an explicit cdg_path or Media+G container, but the backend still loads implicit sidecars via audio_path.with_extension("cdg") when song.cdg_path is absent. Songs with a colocated .cdg sidecar were wrongly cleared and reported hasCdg=false even though load_cdg_packets_from_sidecar would make them playable. Remove the early-return so the probe fires for every song and the backend's 0-byte response determines CDG availability. Add a hasCdg field to the coordinator's onProbeResolved callback to distinguish "no active CDG slot" (null envelope, 0 bytes) from "CDG active but frame unchanged" (header-only response). The hook's onProbeResolved now clears the display and reports hasCdg=false only when the backend confirms no CDG is active.
|
@greptile review |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@greptile review |
1 similar comment
|
@greptile review |
|
Re-review of head The slow-IPC production fix is correct: One documentation correction is still needed in this concurrency-critical code before code-review sign-off: the top coordinator JSDoc currently says “A monotonic serial invalidates late results after song/generation change,” and
Do not change the fixed request/coalescing behavior merely to make the old comment true. After this docs-only correction and green CI, the remaining gate is the declared manual CDG/local-window/AirPlay acceptance. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Split CDG state into per-timeline (Local/AirPlay) mutable state with shared immutable packet storage so AirPlay cannot rewind or corrupt local playback. Replace raw RGBA IPC with a binary frame protocol (32-byte header + optional RGBA payload) that carries transport generation and frame version, letting the frontend skip redundant redraws and discard stale frames. Add get_cdg_status IPC for availability/error reporting. Make transport lifecycle helpers authoritative through PlaybackCoordinator. Gate AirPlay CDG decoding by native output phase. Add CdgRendererSnapshot for checkpoint save/restore with bounded policy. Implement decoder correctness fixes: instruction 28 (transparent color), scroll offset clamping, scroll-copy wrap, and change-only process_packet return. Add golden fixture tests and parser diagnostics. Closes #99.
Rust 1.97 clippy promotes these lints under -D warnings. Replace manual Default impls on CdgAvailability, CdgStatus, and CdgPlaybackSlot with derive(Default) (using #[default] on the None variant). Collapse the match-with-wildcard in the CDG timeline benchmark into an if let pattern.
Four review comments addressed: 1. Right/down scroll in preset mode wraps pixels instead of filling with preset color (renderer.rs): The modular arithmetic (x + FULL_WIDTH - 6) % FULL_WIDTH always produced in-range indices, so the copy/color branch was never reached for right/down scrolls. Rewrote scroll_horizontal and scroll_vertical to explicitly check whether the source index is in range, and only wrap in copy mode. In preset mode, vacated columns/rows are filled with the preset color. 2. Stale callers lose frames (commands/cdg.rs): When advance_cdg_timeline returns NoChange but the caller's last_frame_version differs, the response was header-only without RGBA data. Now the cached RGBA frame is included so remounted receivers or dropped IPC responses can recover without waiting for the position to advance. 3. Load failures hide status (services/playback.rs + coordinator.rs): ReadFailed and ZipFailed were collapsed to None by load_cdg_packets_as_arc, causing the coordinator to clear the CDG slot instead of marking the error. Added cdg_error field to ReadyTrack and changed load_cdg_packets_as_arc to return the error code, so the coordinator calls mark_cdg_error for load failures.
Probe and position ticks share createCdgFrameCoordinator so only one getCdgFrame is in flight; pending requests coalesce and a serial drops stale results after song/generation invalidate.
parseCdgFrameResponse returns Uint8Array; narrowing onFrame avoids Uint8ClampedArray union failures under tsc for drawFrame/emitCdgFrame.
createCdgFrameCoordinator incremented its serial on every request() call, so when a newer frame request was enqueued while an older one was still in flight (common under slow IPC), the older response was always dropped because its serial no longer matched. Under sustained slow IPC this froze CDG playback entirely — every response was superseded before it could be drawn. The serial is now only incremented by invalidate() (song change / unmount), not by request(), so in-flight responses for the current song are still processed while newer requests coalesce in the pending slot.
The JSDoc claimed request() 'returns false if dropped as identical/ noop', but the method returns void. Updated the doc to accurately describe the coalescing behavior and direct callers to isInFlight() for test assertions.
…eck (#113) The frontend songHasCdgMedia() early-return skipped the getCdgFrame probe whenever a song lacked an explicit cdg_path or Media+G container, but the backend still loads implicit sidecars via audio_path.with_extension("cdg") when song.cdg_path is absent. Songs with a colocated .cdg sidecar were wrongly cleared and reported hasCdg=false even though load_cdg_packets_from_sidecar would make them playable. Remove the early-return so the probe fires for every song and the backend's 0-byte response determines CDG availability. Add a hasCdg field to the coordinator's onProbeResolved callback to distinguish "no active CDG slot" (null envelope, 0 bytes) from "CDG active but frame unchanged" (header-only response). The hook's onProbeResolved now clears the display and reports hasCdg=false only when the backend confirms no CDG is active.
481d2a4 to
315551d
Compare
…es (#113) Finding 1 — Status errors are hidden: The 0-byte frame response also represents backend CDG error states (invalid, empty, unreadable, or broken ZIP CDG). The coordinator already consults getCdgStatus on 0-byte responses and forwards the backend availability/errorCode to onProbeResolved, which calls setStatus so the UI can surface error states instead of silently hiding them as audio-only. Finding 2 — Graphics stay blank for songs still loading when first probed: The onProbeResolved handler treated every 0-byte probe response identically — calling setSong(sid, false) and clearing the display. When a song's CDG stream was still decoding (e.g., Media+G ZIPs and remote tracks), the first probe returned 0-byte with availability="loading", which permanently marked hasCdg=false. The hot loop's !hasCdg guard then blocked all further probes, so graphics never appeared for the rest of the song. Fix: when availability is "loading", keep hasCdg optimistic (set by the song-detection effect) and only update the status to "loading". This allows the hot loop to re-probe on subsequent position ticks; once decoding completes, the backend returns frames and availability transitions to "ready". Add coordinator test for 0-byte + loading availability forwarding, and a render test verifying hasCdg stays true during loading and the hot loop eventually draws frames once the backend finishes decoding.
The top coordinator JSDoc and invalidate() doc previously said "song change / unmount", but non-null song/generation changes are rejected by isCurrent(), not invalidate(). invalidate() is only called for unmount cleanup and empty-song (null songId) reset. Update the comments to state the exact split required by #113 re-review.
Re-review — code fixes present, but current branch conflicts and manual CDG acceptance remainsReviewed head The slow-IPC coordinator fix, loading-state re-probe, sidecar probing, and status propagation are present, and CI is green on this old base. The PR is now conflicting with current Rebase onto the repaired current mainline, keep the diff CDG-only, and rerun full CI/Packaging. Final LGTM also remains gated on packaged/manual validation with representative local and Media+G/sidecar content across:
Request re-review on the rebased head with that evidence. |
1 participant
Summary
Comprehensive overhaul of CD+G playback to improve correctness, efficiency, and robustness across local, fullscreen, and AirPlay timelines.
get_cdg_statusIPC for availability/error reportingCdgRendererSnapshotfor checkpoint save/restore with bounded policyprocess_packetreturnCloses #99.
Test plan
tsc --noEmit)cargo test)