fix(mediaplayer): (Fixes broken video!) present D3D12 frames via a typeless shared texture#907
Merged
Conversation
6a6509c to
8753a66
Compare
On D3D12 the present path opened the decoded BGRA ring buffer (a typed B8G8R8A8_UNORM resource) and handed it straight to Unity. Unity wraps it with an sRGB shader resource view (B8G8R8A8_UNORM_SRGB) for a linear colour space, which D3D12 refuses to create over a typed UNORM resource: Creating a default shader resource view with non compatible format (dxgi-fmt=91 for a resource that uses dxgi-fmt=87) The view then fails and the texture binds as NULL every frame. The D3D11 path already avoids this by copying the due frame into a separate TYPELESS texture; the D3D12 path did not. Mirror that for D3D12: allocate a TYPELESS shared texture on the decode device and copy the due ring slot into it each present, then hand that to Unity, which can cast the UNORM or sRGB SRV its colour space needs. This also corrects the D3D12 path pinning a single ring slot for the lifetime of the stream instead of presenting the newest due frame. The decode-device copy has no implicit handoff sync with Unity's D3D12 device, so the render thread polls an event query until the copy retires, releases the surface's keyed mutex only then, and publishes only once Unity holds the external texture, so Unity never samples a half-written or absent frame. The shared-texture size is cached only after the full output path succeeds, so a transient create/share failure retries instead of leaving no usable output. D3D11 behaviour is unchanged; all new work is gated on the D3D12 graphics API.
8753a66 to
6c8a92d
Compare
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
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
On D3D12, video playback through the media player showed a black screen with a repeating D3D12 error:
followed by
Attempting to bind a NULL or empty textureevery frame.Root cause: the D3D12 present path handed Unity the decoded BGRA frame as a typed
B8G8R8A8_UNORMresource. Unity wraps it withCreateExternalTextureand, in a linear colour space, builds aB8G8R8A8_UNORM_SRGBshader resource view — which D3D12 refuses to create over a typed UNORM resource. The view fails and the texture binds as NULL. D3D11 tolerated this cast, so it only surfaced after the move to D3D12. The D3D11 path already sidesteps it by copying each frame into a separateTYPELESStexture; the D3D12 path didn't.Fix: mirror the D3D11 approach on D3D12 — allocate a
TYPELESSshared texture on the decode device, copy the due ring slot into it each present, and hand that to Unity, which can then cast whichever SRV its colour space needs. This also fixes a second issue where the D3D12 path pinned a single ring slot for the lifetime of the stream instead of presenting the newest due frame.Because the decode-device copy has no implicit sync with Unity's D3D12 device (which samples the surface directly), the render thread polls an event query until the copy retires, releases the surface's keyed mutex only then, and publishes the frame only once Unity holds the external texture — so Unity never samples a half-written or not-yet-available copy. A device-loss or multi-second GPU stall is surfaced as an engine error rather than exposing an in-flight surface. The shared-texture size is cached only after the full setup succeeds, so a transient allocation/share failure retries rather than dead-ending.
D3D11 is unaffected — all new work is gated on the D3D12 graphics API.
Verified: in-editor on Windows/D3D12, playing an HTTPS MPEG-TS stream — correct video, orientation, colour, and audio, with no per-frame stutter from the completion wait.
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
This is a native plugin change only —
basis_media_native(C++/Direct3D),Native~/windows/basis_win_decode.cpp. There is no C# gameplay code in this PR, so the C#/Unity-specific required checks (transform access, Addressables,GetComponent,BasisEventDriver, jobification, properties, camera driver,BasisDebug, scene discovery, hot-path allocations/collections) are N/A and ticked on that basis. Native logging continues to use the engine's existingbasis_engine_set_errorpath.Verified in-editor on Windows with the D3D12 backend, playing an HTTPS MPEG-TS stream (correct video, orientation, colour, audio; no stutter from the per-present completion wait). Not separately tested in a VR headset or across a runtime desktop↔VR hot-switch — but the D3D11 path is byte-for-byte unchanged (all new work is gated on the D3D12 graphics API), and the D3D12 path rebuilds its shared textures on any graphics-device or video-size change.
The per-present completion wait polls an
ID3D11Query(event query) until the decode-device copy retires — sub-millisecond in practice, since the render thread is blocked inside the plugin event so the copy can't be lapped. The surface is single-buffered and Unity samples it directly, so the wait is what guarantees a complete frame; the keyed mutex is released only on confirmed completion. A device-loss or multi-second stall is treated as a wedged GPU and surfaced as an engine error rather than exposing an in-flight write.