Commit fe94a04
authored
fix(mediaplayer): re-anchor the present clock on seek (+ Ycbcr filter gating, HLS leak fix) (#971)
## Summary
Fixes a batch of media-player seek and playback regressions found in a
full pass across the Unity Editor, Windows standalone, and Quest Pro.
The main one: a seek repositioned the demuxer but never told the
decoder, so the presentation clock stayed clamped to the stale decode
edge and the picture froze until a post-seek frame happened to arrive
(up to ~18s on a cold forward seek). This adds a decoder-side seek —
`basis_decoder_seek` flushes the decoder and the PCM ring and re-anchors
the present clock to the target. The render leg no longer clears the
frame ring itself; it waits on a small generation ack the demux thread
(which owns the ring) publishes after it has flushed, so the two threads
don't both reset the ring and the render leg can never erase a post-seek
frame the demux just wrote. Verified on Quest and in the Windows Editor:
video re-anchors straight to the target, no freeze, no stale pre-seek
frames, backward and forward.
Also in here:
- **Android Vulkan Y'CbCr sampler** — gate the chroma and sampler
filters on the format's advertised feature bits instead of forcing
`VK_FILTER_LINEAR` unconditionally. Formats that only advertise nearest
(some UBWC external formats on Adreno) were getting linear forced on
them, which is undefined.
- **HLS** — free the VOD playlist metadata (`vod_uri` / `vod_dur_ms`) on
the two open-failure paths; they were leaked.
- **Review round** — flush the video MFT on the video-submit thread (its
reorder buffer could otherwise re-emit pre-seek frames), reset the Opus
decoder state on seek, split the chroma vs sampler filter capability
checks so linear luma isn't dropped when the format supports it, flush
queued PCM at the seek call so the audio callback stops serving pre-seek
samples immediately, and keep the frame-ring reset on the owning thread.
Windows x64 and Android arm64 native binaries are rebuilt from this
source and included.
### Known follow-ups (tracked, not in this PR)
The clock fix is correct, but re-anchoring to the true target surfaced a
separate weakness the old frozen clock had been masking. Flagging these
so reviewers know the shape of what's left:
- **Post-seek audio recovery lags.** Video re-anchors immediately; audio
catches up a few seconds later on a large backward seek (the shared
delivery-pacing anchor holds audio whose timestamp sits ahead of the
video-set base). Same on both backends; being handled as a focused
follow-up.
- **HLS VOD seek into the final segment snaps forward.** Once the
producer has fetched every segment it rejects further seeks;
pre-existing. The proper fix is to let the producer idle and revive on a
seek rather than exit.
- **Quest HE-AAC startup pop.** An implicit-SBR stream brings the
AudioTrack up at the 24 kHz core rate before it corrects to 48 kHz,
audible on an RTSP live-join. Separate change.
## Required checks
This is a native C/C++ change to the media-player plugin (decode / demux
/ present); it touches no C# and none of the Unity per-frame, component,
Addressables, camera, logging, or event-driver surfaces the checks below
cover. Ticked as N/A per the template, with the substantive one (Tested)
genuinely done.
<!-- required-checks-start -->
<!-- Tick the boxes in place — do not edit the line text. The
pr-checklist workflow parses this block; per-PR context goes under
Notes. -->
- [x] **Tested** — I built and ran this locally. The change works in the
editor and (where relevant) in a built player.
- [x] **Transform access is combined and limited** — In hot paths,
transform reads/writes go through `TransformAccessArray` or are
otherwise batched. I have not added per-frame `transform.position` /
`transform.rotation` / `transform.localPosition` calls inside loops.
Whenever I need both position and rotation, I use the combined APIs —
`SetPositionAndRotation` / `SetLocalPositionAndRotation` for writes,
`GetPositionAndRotation` / `GetLocalPositionAndRotation` for reads —
instead of two separate property accesses; the combined call does one
local-to-world matrix traversal instead of two.
- [x] **Addressables used for asset/memory loading** — Any new asset
loads go through Addressables. No new `Resources.Load`, no direct asset
references that pull large content into memory on scene load.
- [x] **No new `GetComponent` / `AddComponent` where avoidable** — Where
unavoidable, the result is cached on a field, and any `GetComponent<T>`
is replaced with `TryGetComponent<T>(out var x)` — bare `GetComponent`
will be denied. `TryGetComponent` is the modern API (Unity 2019.2+) and
skips the Editor-only GC allocation `GetComponent` causes when a
component is missing: Unity wraps the `null` return in a managed "fake
null" object so its overloaded `==` operator can still detect destroyed
C++ objects, and constructing that wrapper allocates; `TryGetComponent`
returns a `bool` plus `out` parameter and never builds the wrapper. None
of these calls run inside `Update`, `LateUpdate`, `FixedUpdate`, jobs,
or other per-frame code paths.
- [x] **Per-frame work is scheduled through `BasisEventDriver`** — Any
new per-frame work hooks into `BasisEventDriver` rather than adding
standalone `Update` / `LateUpdate` / `FixedUpdate` callbacks on a
MonoBehaviour.
- [x] **Anything added to `BasisEventDriver` is bulletproof, or guarded
by `try`/`catch`** — `BasisEventDriver` runs the single per-frame tick
that drives the whole framework (network apply, local player sim,
blendshapes, JigglePhysics, nameplates, and more) as one sequential
chain. An unhandled exception anywhere in that chain aborts the rest of
the tick, so every step after the throwing one is silently skipped for
that frame. New work added to the driver must either be guaranteed not
to throw, or be wrapped in a `try`/`catch` that contains the failure and
surfaces it through `BasisDebug` — logged once / rate-limited, never
every frame (see the existing `HVRBasisBuiltInAddresses.Simulate()`
guard for the pattern). Expect this to be scrutinized closely in review.
- [x] **Considered jobification** — I asked whether this work can be
moved to a Unity Job (Burst-compiled where possible). If it can, it is.
If it cannot, the reason is in **Notes**.
- [x] **No needless `{ get; set; }` properties or access lockdowns** —
Public fields are fine; Basis is meant to be read and modified freely,
so don't wall things off `private`/`internal` without a real reason.
Don't wrap a field in `{ get; set; }` when the accessors do nothing —
property accessors have a real performance cost vs direct field access,
and the lead maintainer prefers plain fields (or a method / setter-only
property when only the setter needs logic) over a noop-getter pair. For
`.Instance` singletons, callers reassigning `Type.Instance` is allowed;
if that would break your code, log a warning or throw — don't block the
assignment. Locking down access is not your call.
- [x] **Camera access goes through `BasisLocalCameraDriver`** — Code
that needs the local camera (transform, projection, rig data, etc.)
pulls it from `BasisLocalCameraDriver` rather than looking one up
itself. Don't roll a separate camera discovery path.
- [x] **Logging uses `BasisDebug`** — All new logging calls go through
`BasisDebug.Log` / `BasisDebug.LogWarning` / `BasisDebug.LogError` (with
an appropriate `LogTag`) instead of `UnityEngine.Debug.Log` /
`Debug.LogWarning` / `Debug.LogError`. `BasisDebug` routes through
Basis's tagged, color-coded logger and respects the project-wide
`LoggingDisabled` toggle so logging can be killed at runtime; bare
`Debug.Log` calls bypass that and will be denied.
- [x] **No scene-wide discovery for dependencies** — New code is
architected so it does not need `FindObjectOfType` / `FindObjectsOfType`
/ `GameObject.Find` / `FindGameObjectsWithTag` to locate what it depends
on. References are wired in — registered through an existing
manager/driver, injected at init, or passed in by the caller — rather
than discovered by scanning the scene at runtime. If a scene scan is
genuinely unavoidable, justify it under **Notes**.
- [x] **No allocations in hot paths** — Per-frame code (Update /
LateUpdate / FixedUpdate, simulation loops, jobs, anything called once
per frame or more) does not allocate. No `new` on reference types, no
LINQ, no `string` concatenation/interpolation, no boxing, no `foreach`
over interface-typed collections. Allocate once at init and reuse the
buffer.
- [x] **No debugging in hot paths** — No log calls of any kind on
per-frame paths, including `BasisDebug`. Hot-path logging floods the
console and incurs cost on every frame regardless of whether the message
is filtered out downstream. If a hot-path log is needed while iterating,
gate it behind `#if UNITY_EDITOR` and remove (or leave gated) before
merge.
- [x] **Hot-path collection access is optimized** — Cache `.Count`
(lists) / `.Length` (arrays) into a local `int` before the loop instead
of re-reading the property each iteration. Prefer `T[]` (with a separate
length int when the array is over-sized) over `List<T>` where the data
is hot — Unity's mono BCL doesn't expose
`CollectionsMarshal.AsSpan(List<T>)`, so a list can't be fed into
`Span<T>` / unsafe paths cleanly. Where the perf justifies it, drop into
`Span<T>` / `ref` locals / `Unsafe.As` / `unsafe` pointer code to skip
bounds checks and copies, and call out the invariants you're relying on
under **Notes** so reviewers can sanity-check them.
<!-- required-checks-end -->
## Testing details
- [x] Windows
- [ ] Linux
- [x] Android
- [ ] iOS
- [ ] macOS
Input / control mode coverage:
- [x] Tested in VR (note headset under **Notes**)
- [x] Tested in desktop / non-VR mode
- [ ] Tested with phone controls (mobile touch input)
- [ ] N/A — change does not touch player/XR/input code
Where applicable, confirm these flows still work after your changes:
- [ ] Hot-switching (desktop ↔ VR mode swap at runtime)
- [ ] Avatar swapping
- [ ] Server swapping (joining / leaving / changing servers)
- [x] N/A — change does not touch any of the above
## Notes
Native plugin change only — no C# — so the Unity-pattern required checks
above are N/A and ticked as such per the template.
Tested on **Quest Pro** (Android arm64, VR) and the **Windows Editor**
(Direct3D). Seek was exercised both directions on progressive MP4,
integrated fMP4 (sidx), and TS-segmented HLS; playback verified on
VP9/WebM, Opus, MP3, and AAC 5.1. The seek clock fix was corroborated
against the diagnostics CSV on both platforms (video position re-anchors
to the target immediately). The audio-recovery follow-up above is the
same CSV showing audio catching up a few seconds behind on a large
backward seek.8 files changed
Lines changed: 240 additions & 5 deletions
File tree
- Basis/Packages/com.basis.mediaplayer
- Native~
- android
- protocol
- windows
- Plugins
- Android/arm64-v8a
- Windows/x86_64
Lines changed: 90 additions & 1 deletion
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
186 | 186 | | |
187 | 187 | | |
188 | 188 | | |
| 189 | + | |
| 190 | + | |
| 191 | + | |
| 192 | + | |
| 193 | + | |
| 194 | + | |
| 195 | + | |
| 196 | + | |
| 197 | + | |
| 198 | + | |
| 199 | + | |
189 | 200 | | |
190 | 201 | | |
191 | 202 | | |
| |||
259 | 270 | | |
260 | 271 | | |
261 | 272 | | |
| 273 | + | |
| 274 | + | |
| 275 | + | |
| 276 | + | |
| 277 | + | |
| 278 | + | |
| 279 | + | |
| 280 | + | |
| 281 | + | |
| 282 | + | |
| 283 | + | |
| 284 | + | |
| 285 | + | |
| 286 | + | |
| 287 | + | |
| 288 | + | |
262 | 289 | | |
263 | 290 | | |
264 | 291 | | |
| |||
860 | 887 | | |
861 | 888 | | |
862 | 889 | | |
| 890 | + | |
| 891 | + | |
| 892 | + | |
| 893 | + | |
| 894 | + | |
| 895 | + | |
| 896 | + | |
| 897 | + | |
| 898 | + | |
| 899 | + | |
| 900 | + | |
| 901 | + | |
| 902 | + | |
| 903 | + | |
| 904 | + | |
| 905 | + | |
863 | 906 | | |
864 | 907 | | |
865 | 908 | | |
| |||
933 | 976 | | |
934 | 977 | | |
935 | 978 | | |
| 979 | + | |
| 980 | + | |
| 981 | + | |
| 982 | + | |
| 983 | + | |
| 984 | + | |
| 985 | + | |
| 986 | + | |
| 987 | + | |
936 | 988 | | |
937 | 989 | | |
938 | 990 | | |
| |||
1143 | 1195 | | |
1144 | 1196 | | |
1145 | 1197 | | |
1146 | | - | |
| 1198 | + | |
| 1199 | + | |
| 1200 | + | |
| 1201 | + | |
| 1202 | + | |
| 1203 | + | |
| 1204 | + | |
| 1205 | + | |
| 1206 | + | |
| 1207 | + | |
| 1208 | + | |
| 1209 | + | |
| 1210 | + | |
| 1211 | + | |
| 1212 | + | |
| 1213 | + | |
| 1214 | + | |
| 1215 | + | |
| 1216 | + | |
| 1217 | + | |
| 1218 | + | |
| 1219 | + | |
| 1220 | + | |
1147 | 1221 | | |
1148 | 1222 | | |
1149 | 1223 | | |
| |||
1170 | 1244 | | |
1171 | 1245 | | |
1172 | 1246 | | |
| 1247 | + | |
| 1248 | + | |
| 1249 | + | |
| 1250 | + | |
| 1251 | + | |
| 1252 | + | |
| 1253 | + | |
| 1254 | + | |
| 1255 | + | |
| 1256 | + | |
| 1257 | + | |
| 1258 | + | |
| 1259 | + | |
| 1260 | + | |
| 1261 | + | |
1173 | 1262 | | |
1174 | 1263 | | |
1175 | 1264 | | |
| |||
Lines changed: 21 additions & 2 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
269 | 269 | | |
270 | 270 | | |
271 | 271 | | |
| 272 | + | |
| 273 | + | |
| 274 | + | |
| 275 | + | |
| 276 | + | |
| 277 | + | |
| 278 | + | |
| 279 | + | |
| 280 | + | |
| 281 | + | |
| 282 | + | |
| 283 | + | |
| 284 | + | |
| 285 | + | |
| 286 | + | |
| 287 | + | |
| 288 | + | |
| 289 | + | |
| 290 | + | |
272 | 291 | | |
273 | 292 | | |
274 | 293 | | |
| |||
277 | 296 | | |
278 | 297 | | |
279 | 298 | | |
280 | | - | |
| 299 | + | |
281 | 300 | | |
282 | 301 | | |
283 | 302 | | |
284 | 303 | | |
285 | 304 | | |
286 | 305 | | |
287 | 306 | | |
288 | | - | |
| 307 | + | |
289 | 308 | | |
290 | 309 | | |
291 | 310 | | |
| |||
Lines changed: 5 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1359 | 1359 | | |
1360 | 1360 | | |
1361 | 1361 | | |
| 1362 | + | |
| 1363 | + | |
| 1364 | + | |
| 1365 | + | |
| 1366 | + | |
1362 | 1367 | | |
1363 | 1368 | | |
1364 | 1369 | | |
| |||
Lines changed: 6 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
165 | 165 | | |
166 | 166 | | |
167 | 167 | | |
| 168 | + | |
| 169 | + | |
| 170 | + | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
168 | 174 | | |
169 | 175 | | |
170 | 176 | | |
| |||
Lines changed: 8 additions & 2 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
56 | 56 | | |
57 | 57 | | |
58 | 58 | | |
59 | | - | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
60 | 64 | | |
61 | 65 | | |
62 | 66 | | |
| |||
785 | 789 | | |
786 | 790 | | |
787 | 791 | | |
788 | | - | |
| 792 | + | |
789 | 793 | | |
790 | 794 | | |
791 | 795 | | |
792 | 796 | | |
| 797 | + | |
| 798 | + | |
793 | 799 | | |
794 | 800 | | |
795 | 801 | | |
| |||
0 commit comments