feat(audio): report playback position in AudioEvent while playing - #10123
LautaroPetaccio wants to merge 9 commits into
Conversation
AudioEventsSystem only wrote an AudioEvent when the media state changed, and PBAudioSource.CurrentTime is a write-only seek, so a scene could never learn where the audio it hears actually is. Rhythm and video-synced scenes therefore drift by the renderer's playback start latency (100 to 250 ms measured) with no way to correct it. While a source plays, append a report every PLAYBACK_REPORT_INTERVAL_TICKS (15 ticks, about twice a second) carrying the scene tick, the clip position and the clip length, in addition to the existing state-change events. Streams keep state-only events through the same writer. Requires the protocol change that adds tick_number, current_offset and clip_length to PBAudioEvent; regenerate the C# bindings with make regenerate-protocol once that version is published.
🚦 CI StatusBuild cancelled — push a new commit or re-run the workflow to refresh this section. No C# files changed — lint ratchet skipped.
Tests time sums the test cases; Job time is the job's wall clock including checkout, licensing and asset import. Full report: run summary · results + editor logs: playmode Bare-metal benchmarks run automatically after each successful build; results land in this section. Add the On demand — comment |
decentraland-bot
added
the
ext-contribution
|
Slack notification sent to #explorer-ext-contributions for external review. |
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
PR Review — #10123 feat(audio): report playback position in AudioEvent while playing
STEP 1 — Context & scope
Files changed (3):
Explorer/Assets/DCL/SDKComponents/AudioSources/Components/AudioSourceComponent.cs(+6)Explorer/Assets/DCL/SDKComponents/AudioSources/Systems/AudioEventsSystem.cs(+50 −10)Explorer/Assets/DCL/SDKComponents/AudioSources/Tests/AudioEventsSystemShould.cs(+48 −7)
Surrounding files reviewed: AudioSourceComponent.cs (full), AudioEventsSystem.cs (full), AudioEventsSystemShould.cs (full), VideoEventsSystem.cs (pattern comparison), AudioEvent.gen.cs (protocol state).
The lint pre-flight (scripts/lint/custom-rules.sh) could not be run — the shallow checkout does not have the working tree populated. Machine-checkable rules are verified by hand below.
STEP 2 — Root-cause check: ✅ PASS
Problem: PBAudioSource.CurrentTime is write-only (seek), and AudioEvent only fired on state transitions. Scenes had no way to learn the actual playback position of the audio they hear, making rhythm-game and video-sync scenes unable to correct for the 100–250 ms renderer start latency.
Fix: While a clip plays, AudioEventsSystem now appends a position report every 15 scene ticks (~2× per second at 30 Hz), carrying tick number, clip offset, and clip length. This gives the scene the signal it needs at the source — not a symptom-level workaround.
STEP 3 — Design & integration: ✅ PASS
Owner search: AudioEventsSystem already owns all audio-event propagation — state-change events, natural-finish detection, and the CRDT AppendMessage writes for PBAudioEvent. It has direct access to sceneStateProvider.TickNumber, IECSToCRDTWriter, and the [Query] over AudioSourceComponent. Periodic position reporting is a natural extension of this existing responsibility.
Files checked: AudioEventsSystem.cs, UpdateAudioSourceSystem.cs, StartAudioSourceLoadingSystem.cs, CleanUpAudioSourceSystem.cs (directory listing), VideoEventsSystem.cs (pattern sibling). No other system owns audio-event propagation.
No new lifecycle introduced. The diff adds a plain uint cadence field (LastReportedTick) to the existing AudioSourceComponent struct — no new system, no new subscription, no persistent collection. The cadence check runs inside the existing [Query] method.
Teardown trace: LastReportedTick is a plain value-type field — nothing to unsubscribe or dispose. AudioEventReport is a temporary readonly struct passed by value — no lifecycle. No new subscriptions, callbacks, or resources are opened.
STEP 4 — Member audit
| Member | Consumers | Verdict |
|---|---|---|
AudioEventReport struct |
PropagateAudioSourceEvents (with position), PropagateAudioStreamEvents (without position) |
2 consumers — justified |
LastReportedTick field |
Cadence check in PropagateAudioSourceEvents |
Tracking field on component — appropriate |
PLAYBACK_REPORT_INTERVAL_TICKS |
Cadence check + test | Named constant with XML doc — correct |
PropagateAudioEvent method |
Replaces former PropagateStateInAudioEvent — called from both query methods |
2 consumers — justified |
No single-use derived predicates. No absent-≠-false conflation (HasPosition: false on streams means "position not applicable" — modelled correctly via the flag rather than returning 0 and pretending it's a real offset).
STEP 5 — Line-level review
R1 (alloc-free): ✅ AudioEventReport is a readonly struct (no heap alloc). The prepareMessage delegate is static (no closure). No new objects, arrays, or collections in the query path.
R2 (no LINQ): ✅ No LINQ used.
R3 (no class refs in structs): ✅ AudioEventReport fields are MediaState (enum), uint, bool, float, float — all value types.
R4 (ECS discipline): ✅ No new system fields (the system remains stateless). Cadence state lives on the component. Query uses [None(typeof(DeleteEntityIntention))].
R5 (entity by-ref): ✅ in CRDTEntity, ref PBAudioSource, ref AudioSourceComponent. No structural changes after ref acquisition.
R6 (teardown): ✅ No new subscriptions, events, or resources.
R7 (nullability): One P2 finding — see inline comment.
R8 (root cause): ✅ Covered in Step 2.
R9 (logging): ✅ No new logging. Debug logging remains behind #if AUDIO_EVENTS_DEBUG.
R11 (async): N/A — no async changes.
R12 (no single-consumer abstraction): ✅ AudioEventReport has two consumers.
R13 (reuse/centralize): ✅ Reuses the existing PropagateAudioEvent pathway and AppendMessage pattern. The TickNumber+Timestamp dual-write mirrors VideoEventsSystem.
R14 (contract honesty): ✅ HasPosition flag models optionality correctly.
R15 (dead weight): ✅ No dead code.
R16/R17 (naming): ✅ AudioEventReport, LastReportedTick, PLAYBACK_REPORT_INTERVAL_TICKS — names state responsibility clearly.
R18 (magic numbers): ✅ 15 is named as PLAYBACK_REPORT_INTERVAL_TICKS with XML summary.
R19 (Unity file hygiene): ✅ Files in correct locations.
R20 (idiom cluster): ✅ readonly struct, readonly fields, const, internal visibility.
R22 (tests): ✅ New test ReportPlaybackPositionPeriodicallyWhilePlaying covers: too-early tick (no report), at-cadence tick (one report), too-early-again tick (no additional report). Validates State, Tick, HasPosition, ClipLength, CurrentOffset. Existing tests adapted to AudioEventReport type.
R23 (comments): ✅ No AI-style, narrator, or restating comments. XML summaries on public members are appropriate.
R24 (scope): ✅ All 3 files directly serve the feature. No unrelated changes.
STEP 6 — Complexity: SIMPLE
3 files, ~100 lines of meaningful changes. Extends an existing ECS system's event propagation with a cadence check. Does not modify system registration, async flows, plugin wiring, or cross-world access.
STEP 7 — QA: YES
Changes runtime audio behavior — the AudioEventsSystem query now writes additional CRDT messages while clips play. User-audible behavior is affected (scene can now react to playback position), and the change ships in the Unity player.
STEP 8 — Warnings
pbAudioEvent.TickNumber, pbAudioEvent.CurrentOffset, and pbAudioEvent.ClipLength, but the current AudioEvent.gen.cs only has State and Timestamp. The PR correctly notes it depends on protocol#488 — run make regenerate-protocol once that version is published. CI builds are skipping accordingly.
Merge gates
- R25 (open threads): 0 prior review threads. 0 unresolved bot findings.
- R26 (QA): Awaiting QA. No
no QA neededlabel. Test instructions in PR description are concrete and followable (edit-mode test name given, manual steps with observable outcome).
REVIEW_RESULT: PASS ✅
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: Extends existing AudioEventsSystem with a periodic cadence check — 3 files, no new systems, no cross-world or async changes.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by unknown (<@unknown>) via Slack
| AudioSource? audioSource = audioSourceComponent.AudioSource; | ||
| AudioClip? clip = audioSource != null ? audioSource.clip : null; | ||
| PropagateAudioEvent(in sdkEntity, new AudioEventReport(state, tick, | ||
| hasPosition: clip != null, currentOffset: clip != null ? audioSource!.time : 0f, clipLength: clip != null ? clip.length : 0f)); |
There was a problem hiding this comment.
[P2 · R7] The null-forgiving audioSource! is correct — clip != null implies audioSource != null since clip was derived from audioSource.clip — but R7 requires a justifying comment when ! is used.
| AudioSource? audioSource = audioSourceComponent.AudioSource; | |
| AudioClip? clip = audioSource != null ? audioSource.clip : null; | |
| PropagateAudioEvent(in sdkEntity, new AudioEventReport(state, tick, | |
| hasPosition: clip != null, currentOffset: clip != null ? audioSource!.time : 0f, clipLength: clip != null ? clip.length : 0f)); | |
| AudioSource? audioSource = audioSourceComponent.AudioSource; | |
| AudioClip? clip = audioSource != null ? audioSource.clip : null; | |
| // audioSource! safe: clip non-null implies audioSource non-null (clip derived from audioSource.clip) | |
| PropagateAudioEvent(in sdkEntity, new AudioEventReport(state, tick, | |
| hasPosition: clip != null, currentOffset: clip != null ? audioSource!.time : 0f, clipLength: clip != null ? clip.length : 0f)); |
There was a problem hiding this comment.
Added the justifying comment in 9f98d35.
|
I need to merge the protocol changes first. I'm reverting it to draft until then. |
…ystem Drop the 15-tick cadence. A report now goes out whenever the state changes or the clip position differs from the last propagated one, which is what VideoEventsSystem does for video. A playing clip therefore reports every frame and converges immediately after a seek; a paused or stopped clip emits nothing until something changes.
LastPropagatedOffset started at NaN to mean 'nothing reported yet', but the comparison against it is !offset.Equals(last), and 0f.Equals(float.NaN) is false. Every source therefore looked like it had moved the first time the system examined it, and NotEmitEventsWhenStateIsUnchanged went red. A fresh source sits at the start of its clip, so zero is the honest initial value and is what MediaPlayerComponent uses for video. Extract the emit rule as ShouldReport, alongside IsNaturalFinish. The rule is the part worth testing and it is pure, so it can be covered without a GameObject, an AudioSource or an audio device. Replace the playback-position test accordingly. It asked a real AudioSource to play and guarded on isPlaying with Assume.That, which in batch-mode CI has no audio device: the guard reports Inconclusive, and Inconclusive does not fail a run, so the behaviour went unverified. It also wrote LastPropagatedOffset by hand before each update, so it never observed a playhead moving on its own.
…ition Adds TickNumber, CurrentOffset and ClipLength to PBAudioEvent, with the Has/Clear accessors proto3 optional fields generate. Without them the system in this branch does not compile. Pins @dcl/protocol to the build of decentraland/protocol#488. The pin goes back to a released version once that merges.
Adds PBAudioSource.report_playback_position (field 8, optional bool), the opt-in that gates the playback position reports, and picks up the PBAudioEvent doc change that describes when a position is written. Moves the @dcl/protocol pin to the newer build of decentraland/protocol#488. The pin goes back to a released version once that merges.
…sition ShouldReport takes a reportsPosition flag that gates only the moved-playhead half of the rule, and the AudioEventReport now carries CurrentOffset and ClipLength only for a source that asked for them. A source that leaves the flag unset emits no position at all. Media state changes stay unconditional. They are what scenes have always listened for, and gating them would break every scene that reacts to a clip starting, finishing or failing to load. The position reports are the expensive half: they go out whenever the playhead moves, which is every frame a clip plays, while state changes are rare. A scene can hold far more audio sources than videos, since video playback is capped by the prioritisation system and audio is not, and AudioEvent is a grow-only set capped at 100 entries per entity, so a single source reporting every frame fills its own buffer in under two seconds. Making the reports opt-in keeps that cost with the scenes that want the data. The natural-finish writeback now copies the flag too, so a source that opted in does not silently lose the opt-in when the renderer PUTs playing: false back.
This branch has not been deployed
3 participants
Pull Request Description
What does this PR change?
AudioEventsSystemonly wrote anAudioEventwhen the media state changed, andPBAudioSource.CurrentTimeis a write-only seek, so a scene could never learn where the audio it hears actually is. Playback starts 100 to 250 ms after the component is applied (measured with a rhythm-game scene, varying per start), which such scenes cannot correct without a signal.The system now appends a report whenever the media state changed or the clip position moved since the last report, carrying the scene tick, the clip position and the clip length. For a playing clip that is every frame; a paused or stopped clip emits nothing until something changes. This is the rule
VideoEventsSystemalready applies to video, which writes when the state or the current time differs from the last propagated value; there is no fixed cadence. State-change events written while a clip is attached carry the same fields. Streams keep state-only events through the same writer. The natural-finish write-back is unchanged.The emit rule is extracted as
ShouldReport, next to the existingIsNaturalFinish, so it can be covered directly.Two things worth a reviewer's attention
The last propagated offset starts at zero. NaN looks like the natural way to say "nothing reported yet", and it is a trap: the comparison is
!offset.Equals(last)and0f.Equals(float.NaN)is false, so every source would look like it had moved the first time the system examined it and emit a spurious position report. A fresh source sits at the start of its clip, so zero is the honest initial value, and it matchesMediaPlayerComponent.LastPropagatedVideoTimefor video.A finished clip reports a zero offset, not the clip length. Unity rewinds the playhead on
Stop()and when a non-looping clip reaches its end, so the last report of a finished clip carries zero. A scene tracking completion has to read the state transition out ofMsPlaying, not wait for the offset to approach the length. This is documented in ADR-318.Opting in
Position reports are gated behind
PBAudioSource.report_playback_position, new in decentraland/protocol#488. A source that did not set it gets exactly the events it gets today: media state changes are reported unconditionally, and only the position half of the emit rule is gated.The rule lives in
ShouldReport, so the gate is one extra disjunct there plus thehasPositionargument on the report. The query already receivedPBAudioSource, so nothing about its signature changed.One thing worth a reviewer's eye:
WriteBackNaturalFinishPUTs a rentedPBAudioSourceback to the scene when a clip ends by itself, and that lambda has to copy every field or the pooled message drops it. It now copies the new flag too. Without that, a source would silently lose its opt-in the first time a clip finished.Protocol
Depends on decentraland/protocol#488, which adds the optional
tick_number,current_offsetandclip_lengthfields toPBAudioEvent.AudioEvent.gen.csis regenerated against that PR's published build, so this branch compiles as it stands.scripts/package.jsontherefore holds a branch pin, which is what thenew-dependencyclassification is reacting to; before merging, runmake upgrade-protocolto move back to a released version once the protocol change lands.Design: ADR-318 decentraland/adr#324. SDK counterpart: decentraland/js-sdk-toolchain#1624.
Test Instructions
Edit mode:
AudioEventsSystemShould.ShouldReportis covered directly across the four cases that matter: state changed, playhead moved with the state unchanged, neither changed, and a source with no clip loaded. A fifth pins the regression above, that a fresh source sitting at offset zero stays quiet.CarryTheTickThePositionAndTheClipLengthInAPositionReportchecks the payload, andStoreThePropagatedOffsetSoAnUnchangedPlayheadStaysQuietchecks the state written back.AudioSourceand guarded onisPlayingwithAssume.Thatwould report Inconclusive there, and Inconclusive does not fail a run: the behaviour would go unverified without anyone noticing.Manual: run a scene with a playing
AudioSourceand readAudioEventvalues in the scene. While a clip plays, new values carryingcurrentOffsetshould arrive every frame with the offset advancing; pausing should stop them; seeking should converge on the next frame. The scene at decentraland/sdk7-test-scenes#99 shows this against the SDK branch build.