Commit 9a0e18a
authored
fix(mediaplayer): detect VOD seekability under Proton/Wine (#975)
## Summary
VOD played at delivery speed for Linux users running the Windows build
under Proton, reported
after a test session as "playing at about 4x speed" through the content.
The multiplier isn't
fixed: it's download bandwidth over stream bitrate, so it varies by
connection.
The live-vs-VOD probe decides whether to pace delivery, and it
establishes seekability from
Content-Length:
```c
BOOL haveLen = WinHttpQueryHeaders(h->request,
WINHTTP_QUERY_CONTENT_LENGTH | WINHTTP_QUERY_FLAG_NUMBER64, ...);
h->seekable = (haveLen && clen > 0 && rangeable) ? 1 : 0;
```
Wine's winhttp doesn't implement `WINHTTP_QUERY_FLAG_NUMBER64`. Its
`QUERY_MODIFIER_MASK` covers
only `REQUEST_HEADERS | SYSTEMTIME | NUMBER`, so the flag isn't stripped
from the attribute index,
the header lookup misses, and the query returns FALSE. Under Proton
`haveLen` was therefore always
false, `seekable` stayed 0, `paced` and `pace_delivery` stayed 0, and
`pace_gate()` returned
immediately, leaving the demuxer to consume the file as fast as the CDN
would serve it. The comment
above the detection in `basis_media_core.c` describes that outcome
exactly.
The same failed query left `content_length` at -1, so seeking was
unavailable for those users too.
That has a knock-on in multiplayer: late joiners are synced by seeking
to the owner's playhead, so
a Proton client joining an in-progress VOD couldn't take the seek and
started from zero, while also
racing through the content.
Content-Length is now read as a string and parsed. That's the default
query path on native Windows,
and it avoids the >4GB truncation that the 32-bit
`WINHTTP_QUERY_FLAG_NUMBER` (which Wine does
implement) would bring. The two `STATUS_CODE |
WINHTTP_QUERY_FLAG_NUMBER` queries are untouched.
Parsing headers means validating them, since they're remote input:
- The whole field must be a plain in-range integer. `_wcstoi64` accepts
a numeric prefix
(`"123junk"` as 123) and saturates on overflow, either of which hands a
bogus finite length to
the seek and pacing logic.
- On a 206 the Content-Length covers the part that came back, so a
range-capping proxy could
present a truncated length as the whole file. Only Content-Range can
establish a total there.
- Content-Range must match the exact `bytes <first>-<last>/<complete>`
grammar with coherent
bounds, and its first byte must be 0. That's what the probe asks for, so
a response describing
any other start isn't the body the caller thinks it's reading.
Body length and representation total are different quantities answering
different questions, so
they're tracked separately. Finite and rangeable is enough to call a
source on-demand and pace it,
while the complete length stays unknown unless it can be proven.
Includes the rebuilt Windows x64 plugin (RIST-enabled, Unity 6000.5.4f1
PluginAPI headers). Android
is unaffected, since `basis_win_http.cpp` is Windows-only.
## 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**.
<!-- 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
Tick the platforms you actually tested on. Leave the rest unticked —
these are informational and do not block merge.
- [x] Windows
- [ ] Linux
- [ ] Android
- [ ] iOS
- [ ] macOS
Input / control mode coverage:
- [ ] Tested in VR (note headset under **Notes**)
- [ ] Tested in desktop / non-VR mode
- [ ] Tested with phone controls (mobile touch input)
- [x] 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
**Verified on Windows; the Proton path itself is not verified.** Being
straight about the split,
because it matters for how much weight to give this.
What was covered: a Windows x64 standalone build, playing HTTPS VOD
across MP4 and audio-only
lanes. Every HTTPS VOD open goes through `basis_win_http_open`, so the
new parsing ran on every
one of them. The test endpoints answer 206 with `Content-Range`, which
means the Content-Range
path was exercised against real servers and produced correct results:
duration reported correctly,
seek works in both directions, and a late joiner syncs to the owner's
playhead, which depends on
the same probe. So this is positive confirmation the new parsing
computes the right total, not
just an absence of crashes.
What was not covered: Proton itself. I don't have a Linux machine
running it, and there's no Linux
lane in the media player's TESTING.md to run against. The Wine behaviour
behind the diagnosis is
from reading Wine's `dlls/winhttp/request.c`, where
`QUERY_MODIFIER_MASK` omits
`WINHTTP_QUERY_FLAG_NUMBER64`, rather than from observing it on a
device. If anyone here runs
Basis under Proton, a single VOD playing at 1x rather than racing would
confirm it, and the seek
bar working would confirm the knock-on fix.
Android is unaffected and was not tested for this: `basis_win_http.cpp`
is Windows-only, and the
Android path uses `basis_jni_https.c`, which this PR doesn't touch.
The header parsers were exercised directly rather than by inspection,
using a harness that
`#include`s the translation unit under test so it runs the shipped
statics rather than a copy that
could drift. 30 cases pass, covering the ordinary nginx 206, capped
ranges, both `*` forms,
incoherent bounds, wrong units, whitespace inside the grammar, non-zero
start offsets, numeric
prefixes and `int64` overflow. It isn't committed here: it's
Windows-only, and the existing native
gates (`fuzz-demux`, `conformance-demux`) run on Linux, so wiring it in
belongs with the CI work
rather than bolted onto this fix. Happy to add it if you'd prefer it
landed together.
The gameplay and perf checks above are N/A: this is a native HTTP header
query plus the rebuilt
binary. No C# is touched, so nothing here goes near transforms,
Addressables, components,
`BasisEventDriver`, the camera driver, logging, scene discovery, or any
per-frame path.
One deliberate non-change, since it looks like an inconsistency on a
close read.
`Accept-Ranges: bytes` without a 206 still marks a source seekable for
*pacing* purposes, and that
is intentional. Seeking itself is gated separately and more strictly:
`basis_win_http_can_reseek`
and `basis_win_http_reseek` both additionally require `range_ok`, which
is only set on a 206. So a
server that merely advertises ranges is treated as on-demand and paced
correctly, but never has a
ranged re-request issued against it.3 files changed
Lines changed: 99 additions & 8 deletions
File tree
- Basis/Packages/com.basis.mediaplayer
- Native~/windows
- Plugins/Windows/x86_64
Lines changed: 98 additions & 8 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
6 | 6 | | |
7 | 7 | | |
8 | 8 | | |
| 9 | + | |
9 | 10 | | |
10 | 11 | | |
11 | 12 | | |
| |||
21 | 22 | | |
22 | 23 | | |
23 | 24 | | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
24 | 80 | | |
25 | 81 | | |
26 | 82 | | |
| |||
95 | 151 | | |
96 | 152 | | |
97 | 153 | | |
98 | | - | |
99 | | - | |
| 154 | + | |
| 155 | + | |
| 156 | + | |
| 157 | + | |
| 158 | + | |
| 159 | + | |
| 160 | + | |
100 | 161 | | |
101 | | - | |
102 | | - | |
103 | | - | |
104 | | - | |
| 162 | + | |
| 163 | + | |
| 164 | + | |
| 165 | + | |
| 166 | + | |
| 167 | + | |
| 168 | + | |
| 169 | + | |
| 170 | + | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
| 178 | + | |
| 179 | + | |
| 180 | + | |
| 181 | + | |
| 182 | + | |
| 183 | + | |
| 184 | + | |
| 185 | + | |
| 186 | + | |
| 187 | + | |
| 188 | + | |
| 189 | + | |
105 | 190 | | |
106 | 191 | | |
107 | 192 | | |
108 | 193 | | |
109 | | - | |
110 | | - | |
| 194 | + | |
| 195 | + | |
| 196 | + | |
| 197 | + | |
| 198 | + | |
| 199 | + | |
| 200 | + | |
111 | 201 | | |
112 | 202 | | |
113 | 203 | | |
| |||
Binary file not shown.
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
213 | 213 | | |
214 | 214 | | |
215 | 215 | | |
| 216 | + | |
216 | 217 | | |
217 | 218 | | |
218 | 219 | | |
| |||
0 commit comments