fix H264 decoder to handle dropped frames and avoid blocking render q… - #2074
Open
RonShani wants to merge 1 commit into
Open
fix H264 decoder to handle dropped frames and avoid blocking render q…#2074RonShani wants to merge 1 commit into
RonShani wants to merge 1 commit into
Conversation
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.
Brief
Fixes a bug where noVNC's H.264 decoder would permanently and silently freeze the entire VNC session the first time anything went slightly wrong with decoding (a dropped frame, a timestamp mismatch, or a WebCodecs error). It was caused due to H264Context throwing instead of resolving its pending-frame promise on any hiccup, which deadlocked the render queue that gates all further server reads
Also - a self/this typo and a synchronous-throw gap in decode() caused the same freeze via different paths. Separately, hardware-accelerated VideoDecoder sessions on some browser/OS combos accept input but silently never call back at all and this is what we've experienced on our PLC's (at Unitronics) where the server delivered frames correctly but the browser swallowed them. It was fixed by forcing hardwareAcceleration: 'prefer-software'. All changes are confined to H264Context in h264.js and the H.264-only 'frame' case in display.js, so every other VNC encoding (Tight/ZRLE/Raw/etc.) is completely untouched. It passed our QA tests and gave excellent results of continuous H.264 streaming with no stalls over enduring run, where before it froze after one frame.
Just to make the motivation clear - H264 encodings (and H265 in more recent PLC's) is the only one with hardware-backed acceleration thats supported by all the PLC's we use (NXP and ST) both ends - encoding and decoding so we had to lay the entire frame-pipe and noVNC is used by our clients.
A bit more in-details
Root cause is a combination of four separate bugs, all in the H.264-only code path:
_handleFrame() / _handleError() threw on any mismatch or decode error instead of resolving the frame's pending promise. display.js's render queue blocks on that exact promise before letting rfb.js read more data from the socket, so an uncaught throw inside an async WebCodecs callback (invisible to any normal try/catch) leaves the render queue and therefore the whole connection's FramebufferUpdateRequest loop stuck forever, with no visible error in the console.
A self/this typo in decode() wrote parsed SPS fields (_profileIdc etc.) onto the global window object instead of the per-connection H264Context instance. Harmless-looking (no crash -self aliases window in a browser tab) but silently wrong state that breaks multi-connection or multi-region use.
A synchronous-throw gap: if VideoDecoder.decode() itself threw synchronously (chunk rejected before being queued), the pending frame just created was never cleaned up same permanent-freeze outcome as (1), via a different path.
Hardware decode sessions on some browser/OS/driver combinations accept configure() and decode() calls but then never invoke output() or error() at all - confirmed against a real Unitronics imx6 PLC (ImxVncServer/neatvnc, hardware H.264 over /dev/mxc_vpu): server-side logging proved the server correctly encoded and delivered two H.264 payloads (keyframe + delta) to the browser client, while the browser's VideoDecoder silently swallowed the first one isConfigSupported() reported hardware decode as supported, but no callback ever fired.
Why it's safe to merge
Fully scoped to the H.264 path. H264Context is only instantiated for the encodingH264 RFB encoding, and display.js's 'frame' case is exclusively fed by that same path - every other encoding (Raw, Tight, ZRLE, Hextile, RRE, CopyRect, JPEG) resolves through the separate 'blit'/'copy'/'fill'/'img' cases, untouched by this diff. No shared decoder, encoding-negotiation code, or other core/decoders/*.js file is modified.
Strictly safer behavior, not new risk. Before: one decode hiccup → permanent silent freeze. After: that same event is logged and the stream recovers. No new failure mode is introduced.
hardwareAcceleration: 'prefer-software' only affects the browser's local decode of an already-encoded stream - so no effect on the server, wire protocol, or the PLC's hardware VPU encode pipeline. Software H.264 decode at 1024×600 @ ~15 fps is trivial for any modern CPU.
Backoff only engages after repeated consecutive failures (threshold 2) - a healthy stream's behavior is unchanged beyond the software-decode preference.