fix(mediaplayer): crop the coded macroblock pad on Windows so 1080p has no top strip#948
Merged
dooly123 merged 2 commits intoJul 13, 2026
Merged
Conversation
3d15288 to
54c2dac
Compare
…as no top strip The Windows decoder sized its output from the coded NV12 surface. H.264/H.265 round coded height up to a multiple of 16 (1080 -> 1088), and the VideoProcessorBlt copied the whole coded picture with no source rect, so the ~8 pad rows carried through; the decode-time vertical mirror then moved them from the bottom of the coded picture to the top of the displayed frame: a thin black strip on every protocol at any non-16-aligned height (1080p being the common case; 720p is clean). Read MF_MT_MINIMUM_DISPLAY_APERTURE from the MFT output type (at configure and on the first-frame stream change, where most decoders first populate it) and crop the video processor to it: size the shared/output texture to the visible region and set the stream source rect to the aperture. The pad never reaches Unity, and Unity receives the true visible dimensions. Android is unaffected: it decodes through an AImageReader Surface with render=true, and MediaCodec applies the crop when rendering to a Surface, so the delivered frame is already the display size. Rebuilds the RIST-enabled Windows x64 plugin DLL.
54c2dac to
767f7df
Compare
…r-video-geometry # Conflicts: # Basis/Packages/com.basis.mediaplayer/Plugins/Windows/x86_64/basis_media_native.dll
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
Fixes a thin black strip along the top edge of the video on Windows at non-16-aligned heights (1080p being the common case).
The Windows decoder sized its output from the coded NV12 surface. H.264/H.265 round coded height up to a multiple of 16 (1080 to 1088), and the
VideoProcessorBltcopied the whole coded picture with no source rect, so the ~8 pad rows carried through. The decode-time vertical mirror then moved them from the bottom of the coded picture to the top of the displayed frame.The backend now reads
MF_MT_MINIMUM_DISPLAY_APERTUREfrom the MFT output type (at configure and on the first-frame stream change, where most decoders first populate it) and crops the video processor to it: the shared/output texture is sized to the visible region and the stream source rect is set to the aperture. The pad never reaches Unity, and Unity receives the true visible dimensions. 720p and other 16-aligned heights are unaffected.Android is unaffected: it decodes through an
AImageReaderSurface withrender=true, and MediaCodec applies the crop on Surface render, so the delivered frame is already display-sized.Rebuilds the RIST-enabled Windows x64 plugin DLL.
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
MF_MT_MINIMUM_DISPLAY_APERTURE= offset (0,0), 1920×1080 (the 8-row bottom pad), that the aperture read returns exactly the crop rect the fix applies, and that the aperture is absent before the first frame and only appears after the stream change — which is why the fix reads it in both places. 720p and other 16-aligned heights don't exercise the pad.basis_engine_set_error.