Repository navigation
ADR-318: Audio playback position reports - #324
LautaroPetaccio wants to merge 10 commits into
Conversation
Deploying adr with
|
| Latest commit: |
8e98c96
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d92bb443.adr-cvq.pages.dev |
| Branch Preview URL: | https://feat-adr-318-audio-playback.adr-cvq.pages.dev |
2f78c61 to
7282ff5
Compare
|
The only potential problem with that is that both the scene and the engine could modify the property ( What we can do is follow the same approach we do for videos, constantly updating their videoEvent with the current time in that other component, WDYT? |
|
You're right on both counts, and that is how it works now: the position goes in The abstract names the |
| ### Renderer behaviour | ||
|
|
||
| - Renderers keep appending an `AudioEvent` on every media state change, as today. | ||
| - While an `AudioSource` is in `MS_PLAYING`, renderers also append a report at least every 15 scene ticks (about twice a second at the reference tick rate), carrying `tick_number`, `current_offset` and `clip_length`. This is the same mechanism the Unity explorer uses for `PBVideoEvent` in [`VideoEventsSystem`](https://github.com/decentraland/unity-explorer/blob/fe6974465b0d2e3a70eeb1ba2da3cb87df27e654/Explorer/Assets/DCL/SDKComponents/MediaStream/Systems/VideoEventsSystem.cs#L63), applied to audio sources. |
There was a problem hiding this comment.
renderers also append a report at least every 15 scene ticks (about twice a second at the reference tick rate)
It'd be a very strange approach leading to heavy desync.
VideoEventsSystem doesn't do it - it writes the component off every frame if the video has progressed
There was a problem hiding this comment.
You're right. There is no cadence now: a report goes out whenever the state or the clip position differs from the last propagated value, exactly as VideoEventsSystem does. A playing clip reports every frame, and a paused or stopped one emits nothing.
Address review: reports go out whenever the clip position changes, as VideoEventsSystem does, not on a fixed cadence; the scene-clock history that resolves a report's tick is part of the SDK, shown in full, and no round-trip estimation is involved because renderer and scene key on the same tick.
The text claimed the transport delay cancels out by construction and that the remaining error is one tick. The first half is what the tick stamp does; the second was wrong. Two terms survive it: the sampling granularity of the playhead, and the renderer's output latency, which is the gap between the decoder position a report carries and the moment a sample leaves the speaker. The second is tens of milliseconds and is precisely what a scene aligning to audible sound wants removed, so it is named rather than implied away. Also correct the SDK bullets: delivery is once per scene frame with the newest report, not one callback per appended report.
Players rewind the playhead on stop and on reaching the end, so the last report of a finished clip carries zero, not the clip length. A scene driving a progress bar from current_offset would see it snap back instead of complete.
|
Status across the five pull requests. Protocol (decentraland/protocol#488): three optional fields on Renderer (decentraland/unity-explorer#10123): a report goes out whenever the media state changed or the clip position moved, which is the rule SDK (decentraland/js-sdk-toolchain#1624): Test scene (decentraland/sdk7-test-scenes#99): three beat cubes contrasting the scene's own clock, the resolved reading, and the same reading timed on arrival, against a clip that beeps on every whole second. Documentation and skills: decentraland/documentation#609 and decentraland/sdk-skills#97. One behaviour worth flagging: a finished clip reports a zero offset rather than The SDK and renderer pull requests both hold a branch pin on |
The proposal listed a raw-report callback beside a resolved one. They were the same delivery; the resolved payload carries the raw report as a field, so the raw entry point added no reachable information and was the shorter name a scene author would pick, leading back to per-scene histories of clock snapshots. Record the choice and why the raw form was dropped: reports carrying no position are media-state changes, which registerAudioEventsEntity already delivers.
Adds PBAudioSource.report_playback_position. A position report is written whenever the playhead moves, far more often than a media state changes, and a scene can hold many more audio sources than video players, which are capped by a prioritisation mechanism. AudioEvent is also a grow-only set bounded per entity, so an unconditional reporter evicts its own state history within seconds. State changes are reported whatever the flag says. State a reporting rate rather than leaving it to each renderer: at most one report per tick, since the tick is the resolution of the stamp, and at least one per second of playback so a scene converges promptly. Existing video implementations already differ from one another inside that band, which is the reason to bound it here. Drop the SDK implementation and the scene usage example. What the SDK does with these fields is a client-library concern, and the code duplicated what the js-sdk-toolchain pull request already carries. What survives is the requirement: the tick-to-clock correlation belongs in the library, not in every scene, and a report must not be dropped when its tick is missing from the history. That requirement is also the protocol rationale for stamping a tick rather than a wall-clock time, so it belongs here. Move the accuracy limits out of the SDK section. Sampling granularity and output latency bound what any renderer can deliver through these fields, so they are protocol-level, not a property of one client library.
|
You're right, and the spec is protocol-only now. The SDK implementation and the scene usage example are gone. What I kept is the requirement rather than the code: the tick-to-clock correlation belongs in the client library instead of each scene, and a report must not be dropped when its tick is missing from the history. That one is really protocol rationale, since it is why a report carries a tick instead of a wall-clock timestamp, so removing it would leave a future reader asking why the field is shaped that way. The implementation itself is in decentraland/js-sdk-toolchain#1624. I also moved the accuracy limits out of that section. Sampling granularity and output latency bound what any renderer can deliver through these fields, so they belong next to the protocol rather than under one client library. |
This branch has not been deployed
3 participants
Summary
Proposes extending
PBAudioEventso renderers report the clip playback position while anAudioSourceplays, in addition to the media state changes they already report. A scene asks for the reports with a newPBAudioSource.report_playback_positionflag; state changes are reported whatever the flag says.Why
Scenes that align gameplay or visuals with sound have no way to learn where a clip's playhead is:
PBAudioSource.current_timeis a write-only seek, andPBAudioEventcarries only a media state and a counter. Measured on the Unity explorer, audible playback starts 100 to 250 ms after the scene asks for it, with a different value on each start, which is as wide as a rhythm game's hit window.PBVideoEventalready reportscurrent_offsetandtick_numberfor video.Contents
current_time, higher tick rates, onset detection), compatibilityNotes on two decisions
The reporting rate is bounded here rather than left to each renderer. A survey of the three explorers found they already disagree on video: one reports on every frame the position changes, one at most once per second of playback, one once the position has moved half a second. Scenes cannot write against that. The document now states at most one report per tick, since the tick is the resolution of the stamp and finer carries nothing a scene can place in time, and at least one per second of playback so a scene converges promptly. All three existing implementations sit inside those bounds.
The specification stays protocol-only. An earlier revision showed the SDK implementation and a scene usage example. Both are gone; what an SDK does with these fields is a client-library concern and the code lives in the js-sdk-toolchain pull request. What survives is the requirement that the tick-to-clock correlation belongs in the library rather than in each scene, and that a report must not be dropped when its tick is missing from the history. That requirement is also the protocol rationale for stamping a tick rather than a wall-clock time.
Status: Draft. Numbering:
mainreaches 316 and open PRs use 317, so 318 is the next free number; a few open PRs sit out of sequence at 323, 400 and 401.Related
Related pull requests