fix(mediaplayer): re-anchor HLS-TS VOD seek pacing (+ Android AAC-config / crop-rect hardening)#956
Merged
dooly123 merged 14 commits intoJul 14, 2026
Conversation
…ndary An HLS-TS VOD seek repositioned the segment source but left delivery pacing anchored to the pre-seek timeline, so a forward seek froze for the jump distance and a backward seek flooded unpaced back to the pre-seek position. The pace re-anchor (pace_started = 0) was set on the main thread in basis_media_seek_us while the ring flush ran on the producer thread and AUs were produced by the demux thread. With no synchronisation, a stale pre-seek AU (still buffered in the TS demuxer, or read from the ring before the flush) won the re-anchor and pinned the pace base to the old PTS: post-seek AUs then read as far-future (forward: pace_gate sleeps for the jump) or as late (backward: pace_gate never sleeps, flood). The byte-source (MP4) seek avoids this by doing the reposition, sample drop and re-anchor atomically on the demux thread; HLS had no equivalent. Make HLS match: signal the reposition through the read path so the flush and the re-anchor happen atomically at the exact byte boundary, on the demux thread. - basis_hls: a seek generation pair (seek_gen bumped on request, flush_gen on the producer after it clears the ring and requeues at the target). While they differ basis_hls_read withholds the pre-seek ring (and ring_write stops filling so the producer can't wedge against a withholding consumer); once they match it returns BASIS_READ_REPOSITION once at the boundary. - basis_ts_run: on BASIS_READ_REPOSITION, drop the partial-packet buffer and both PES accumulations and take the seek, re-anchoring the pace clock on the demux thread before the target segment's bytes flow. - basis_media_seek_us: publish seek_seq before arming the producer and drop the racy main-thread re-anchor. TS-HLS VOD seek shipped with this defect (never a working case), so this is a correctness fix, not a regression. Shared clock and demux path, so it applies to Windows and Android alike. TESTING.md gains an HLS-TS VOD seek row.
The seek-generation handshake between the seek caller, the segment producer
and the byte reader used only volatile fields, which orders nothing across
threads under the C11 memory model. Publish {seek_target, generation, pending}
under the ring lock in basis_hls_request_seek, snapshot them under the same
lock in the producer's flush, and read the generations under the lock in
basis_hls_read alongside the ring state they gate. The byte path was already
lock-guarded; this closes the visibility gap on the generation counters, which
matters on weakly-ordered targets (Quest arm64).
The MediaFormat display-crop rect (fcValid/fcL/fcT/fcR/fcB) is written on the decode thread in drain_video_output and read every frame on the AImageReader listener thread in on_image, as plain ints with no synchronisation — a data race, undefined behaviour on arm64, and a torn/stale crop rect at worst. The neighbouring dispW/dispH already use atomics; extend the same pattern to the crop rect: store the coordinates then publish fcValid with release, and load fcValid with acquire before snapshotting the coordinates into locals so the reader sees one consistent rect.
The seek-withhold branch in basis_hls_read spun forever if a seek was armed in the narrow window after the producer's top-of-loop seek check but before it broke out on endlist: seek_gen advanced, the producer set producer_done without ever flushing, and the withhold branch (seek_gen != flush_gen) never reached the producer_done check below it. Seeking near the end of a VOD hung the demux thread. Two-part fix: the producer loops back to honour a pending seek before breaking on endlist (so a late seek still repositions rather than being dropped), and the withhold branch returns 0 when producer_done is set as a backstop, ending cleanly instead of spinning.
README lists finished TS-segment HLS VOD playlists among the seekable sources alongside progressive MP4 and WAV. TESTING.md's on-demand multiplayer note no longer claims the backend "exposes no absolute seek" (it does): on-demand clients drift-correct by seeking to the owner's playhead when the source is seekable, matching the README's networked-sync section.
…gths The esds parser read each descriptor's declared length but discarded it, so the DecoderSpecificInfo (0x05) was bounded only by the whole esds payload — a malformed file could place a stray 0x05 after the real DecoderConfig and have it accepted as the AudioSpecificConfig. esds_desc_len now reports validity (rejecting truncated and unterminated 4-byte lengths), and the walk bounds the ES and DecoderConfig children, optional fields, fixed header and ASC copy against their owning descriptor's end. The ASC field parse also handles the escape forms via a bounded bit reader: audioObjectType 31 (AOT = 32 + 6 bits) and samplingFrequencyIndex 15 (24-bit explicit rate) shift the channelConfiguration that follows, which the old fixed two-byte offsets misread. No memory-safety change (the ASC copy was already bounds-checked); this rejects malformed structure and reads exotic AAC configs correctly.
…endpoints Two crop-rect refinements on the Android decode path: - The per-field release/acquire scheme was safe only for the first publication. On a later format change fcValid stays 1, so the listener could combine old and newly written coordinates, and a format that drops DISPLAY_CROP left the previous rectangle active. Publish and snapshot the whole rect under d->vm and clear validity on every format change, so the rect is always consistent and never stale. (d->vm is free on both the decode and listener paths.) - The bounds check validated crop dimensions but not endpoints: a non-zero offset could pass while left+width or top+height ran past the buffer. Require the full rectangle inside the buffer on both the MediaFormat and AImage paths.
Bring every side of the seek/terminal-state protocol under the ring lock so volatile is no longer relied on for cross-thread ordering. seek_pending is now read under the lock in ring_write and taken as a locked snapshot at the top of the producer loop; producer_done is written under the lock on every exit; and basis_hls_request_seek accepts a request under the same lock that the endlist path uses to set producer_done. That arbitration closes the lost-seek window: a request landing just as the producer exits on end-of-stream is either honoured or cleanly rejected, never left pending while the reader withholds forever.
…trim - Coded dims (vw/vh) were read on the listener thread but written on the decode thread without synchronisation; snapshot and publish them under d->vm with the crop rect. - The display size was two separate relaxed atomics, so a reader could latch a mixed (new width, old height) pair across a format change and size the Unity render texture wrong; publish it as one packed atomic. - The inert-SBR trim kept (p+7)/8 bytes, which for a set dependsOnCoreCoder (p=30) retained two bits of the sync extension in a malformed csd-0; only trim the byte-aligned two-byte AAC-LC core.
t->asc_len was committed before the ASC bit parse was checked, so a truncated escaped config (AOT 31 / explicit rate) was still handed to the decoder despite field decoding rejecting it. Publish asc_len only inside the bit>=0 guard.
- BASIS_READ_REPOSITION: state that this exact value is the reposition signal and any other negative return is an error (it is itself < 0). - README: a reported duration is necessary but not on its own a guarantee of seekability. - TESTING: live/unindexed clients converge independently to the live edge rather than staying start-together, matching the README's networked-sync section.
Rebuilt off developer with the full HLS seek handshake synchronisation, the Android video-dims/crop synchronisation, the AAC-LC core-trim fix, and the fully-parsed-AAC-config guard, on top of the seek re-anchor, EOS-hang fix, crop-rect race/endpoint fixes and esds hardening. The Windows DLL also carries BasisVR#953's cross-platform esds multichannel-AAC parse.
27 tasks
Both streaming prefabs still referenced the script GUID that now belongs to the deprecated BasisObjectSyncNetworking compatibility shim, left over from when the shim took over the legacy GUID so old content bundles keep loading. The serialized fields already match BasisPickupSyncNetworking, so only the m_Script reference changes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A batch of mediaplayer native fixes on top of #953, one commit per concern. The headline is the HLS-TS VOD seek fix; the rest are Android AAC-config and crop-rect hardening that came up alongside it.
HLS-TS VOD seek re-anchor (the main fix). On the HLS-TS VOD path, seeking the on-demand slider misbehaved: a forward seek froze for roughly the jump distance before resuming, and a backward seek fast-forwarded unpaced through the intervening segments back to the pre-seek position instead of honouring the seek. Root cause is the delivery pace clock: it has to re-anchor at the flushed boundary on a seek, and while that re-anchor existed it lost a race — a stale pre-seek access unit could win it and pin the pace anchor to the old timestamp. The fix routes the HLS seek through the same flush-and-re-anchor handshake the MP4 byte-source path already uses:
basis_hls_readraises aBASIS_READ_REPOSITIONsentinel at the flushed boundary (gated on the seek generation),basis_ts_rundrops the ring and PES accumulators and takes the seek to re-anchor on the demux thread, the seek publishes its sequence before arming the producer, and the whole handshake is serialised through the ring lock. A seek that races end-of-stream on a VOD no longer hangs.Android AAC-config hardening (follow-up to #953). Only publish a fully-parsed AudioSpecificConfig rather than announcing a partial one, bound the esds descriptor walk against its declared lengths, and fix the AAC-LC core trim while synchronising the Android video dimensions.
Android crop-rect hardening (follow-up to #953). Read and apply the display-crop rect as a unit with endpoint validation, and close a data race on the crop rect between the decode and present paths.
Binaries. Win x64 and Android arm64 native plugins rebuilt (RIST-enabled) to carry all of the above.
Docs: README refreshed, and TESTING.md gained an HLS-TS VOD seek row (seek both directions on the Mux master, confirm playback resumes paced at 1x from the target rather than freezing forward or flooding backward).
Verified on Windows (Unity editor, D3D11) and Android (Quest Pro) — full test matrix under Notes.
Required checks
All boxes below must be ticked before this PR can merge. If a check is genuinely N/A, tick it anyway and explain under Notes.
TransformAccessArrayor are otherwise batched. I have not added per-frametransform.position/transform.rotation/transform.localPositioncalls inside loops. Whenever I need both position and rotation, I use the combined APIs —SetPositionAndRotation/SetLocalPositionAndRotationfor writes,GetPositionAndRotation/GetLocalPositionAndRotationfor reads — instead of two separate property accesses; the combined call does one local-to-world matrix traversal instead of two.Resources.Load, no direct asset references that pull large content into memory on scene load.GetComponent/AddComponentwhere avoidable — Where unavoidable, the result is cached on a field, and anyGetComponent<T>is replaced withTryGetComponent<T>(out var x)— bareGetComponentwill be denied.TryGetComponentis the modern API (Unity 2019.2+) and skips the Editor-only GC allocationGetComponentcauses when a component is missing: Unity wraps thenullreturn in a managed "fake null" object so its overloaded==operator can still detect destroyed C++ objects, and constructing that wrapper allocates;TryGetComponentreturns aboolplusoutparameter and never builds the wrapper. None of these calls run insideUpdate,LateUpdate,FixedUpdate, jobs, or other per-frame code paths.BasisEventDriver— Any new per-frame work hooks intoBasisEventDriverrather than adding standaloneUpdate/LateUpdate/FixedUpdatecallbacks on a MonoBehaviour.BasisEventDriveris bulletproof, or guarded bytry/catch—BasisEventDriverruns 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 atry/catchthat contains the failure and surfaces it throughBasisDebug— logged once / rate-limited, never every frame (see the existingHVRBasisBuiltInAddresses.Simulate()guard for the pattern). Expect this to be scrutinized closely in review.{ get; set; }properties or access lockdowns — Public fields are fine; Basis is meant to be read and modified freely, so don't wall things offprivate/internalwithout 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.Instancesingletons, callers reassigningType.Instanceis allowed; if that would break your code, log a warning or throw — don't block the assignment. Locking down access is not your call.BasisLocalCameraDriver— Code that needs the local camera (transform, projection, rig data, etc.) pulls it fromBasisLocalCameraDriverrather than looking one up itself. Don't roll a separate camera discovery path.BasisDebug— All new logging calls go throughBasisDebug.Log/BasisDebug.LogWarning/BasisDebug.LogError(with an appropriateLogTag) instead ofUnityEngine.Debug.Log/Debug.LogWarning/Debug.LogError.BasisDebugroutes through Basis's tagged, color-coded logger and respects the project-wideLoggingDisabledtoggle so logging can be killed at runtime; bareDebug.Logcalls bypass that and will be denied.FindObjectOfType/FindObjectsOfType/GameObject.Find/FindGameObjectsWithTagto 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.newon reference types, no LINQ, nostringconcatenation/interpolation, no boxing, noforeachover interface-typed collections. Allocate once at init and reuse the buffer.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_EDITORand remove (or leave gated) before merge..Count(lists) /.Length(arrays) into a localintbefore the loop instead of re-reading the property each iteration. PreferT[](with a separate length int when the array is over-sized) overList<T>where the data is hot — Unity's mono BCL doesn't exposeCollectionsMarshal.AsSpan(List<T>), so a list can't be fed intoSpan<T>/ unsafe paths cleanly. Where the perf justifies it, drop intoSpan<T>/reflocals /Unsafe.As/unsafepointer code to skip bounds checks and copies, and call out the invariants you're relying on under Notes so reviewers can sanity-check them.Testing details
Tick the platforms you actually tested on. Leave the rest unticked — these are informational and do not block merge.
Input / control mode coverage:
Where applicable, confirm these flows still work after your changes:
Notes
file://security gates.com.basis.mediaplayer/Native~plus the prebuilt Win x64 / Android arm64 binaries, and README/TESTING docs. No C#, Unity gameplay, scene, transform, camera, per-frame, or input code, so the Unity/C#-specific required checks are ticked N/A on that basis.developer; no mediaplayer source has changed ondevelopersince fix(mediaplayer): multichannel AAC in progressive MP4 + coded-pad crop (Android) #953, so it rebases clean and the committed Win x64 / Android arm64 binaries are current.