Repository navigation
vpx: release rejected frames back to the video decoder - #71
Open
camilo-celis wants to merge 6 commits into
Open
camilo-celis wants to merge 6 commits into
camilo-celis wants to merge 6 commits into
Conversation
The terminal error exits of gst_vpx_dec_handle_frame dropped only the handler reference on the frame. GstVideoDecoder had already pushed its own reference onto the pending frame queue, so every rejected frame and its input buffer stayed queued for the lifetime of the decoder. A long run of undecodable frames therefore grew memory without bound and made the pending queue permanently non empty. All four exits now call gst_video_decoder_release_frame, which is what the base class itself uses when it discards an input frame. Expected behaviour: return values, warnings, element errors and the request sync point ordering are unchanged, so a finite max errors still reaches GST_FLOW_ERROR. Release is used rather than drop, so no QoS drop messages are posted for rejected frames. Tradeoff: events carried by a released frame now move to the decoder pending event list instead of being destroyed with the orphaned frame. Pending sticky events therefore follow the same flush semantics as every existing release and drop path in the base class, and events of rejected frames are now forwarded ahead of a GAP instead of being lost. This addresses the amplification of decode failures into retained memory. It is not a fix for whatever produced the undecodable VP9 input.
New check suite exercising both decoders through the real encoders. It covers the four terminal error exits of gst_vpx_dec_handle_frame, frame retention and input buffer lifetime via weak references, a sustained run of rejected frames, absence of QoS drop messages, a finite max errors still ending in GST_FLOW_ERROR, sync point request counting, delta frames while awaiting a sync point, recovery output with its caps, timestamps and flags, direct rendering on and off, event delivery across rejected frames, event forwarding ahead of a GAP, flush and EOS behaviour and teardown during corruption. The unmappable input, open codec and unsupported format exits are reached through a local allocator and a throw away subclass registered by the test, so no production code is altered. Against the unpatched decoder 35 of the 37 tests fail. The two that pass are the QoS assertions, which hold in both cases by design.
Review follow up on the previous commit, test only. The registration helper for the injecting decoder subclass returned a string the caller could not own. It now returns the key held by the registration table and frees the temporary name, so repeated calls no longer allocate a new copy each time. The recovery test compared a single luma sample against a guessed tolerance that was wide enough to accept the previous picture, which contradicted what the test claimed to prove. It now decodes the very same recovery bitstream on a pristine decoder and compares every active pixel of every plane, stride aware, against that reference. Pointing the comparison at a different frame makes the test fail, so it discriminates. The delta frame test set the delta unit flag on payloads it had not inspected. It now asserts that the encoder marked those buffers as inter frames instead of relabelling them. The suite was compiled behind HAVE_VP8_ENCODER and HAVE_VP9_ENCODER, which nothing defines, so it silently registered no tests at all. The meson entry already gates the binary on the encoders and decoders being present, so the guards are gone and a new first test asserts the four elements really are available. Also corrected the sync point comment, which promised one request per rejected frame while the assertions correctly allow throttling.
Not forcing keyframes still let the encoder pick one on its own, which made the inter frame assertions depend on encoder heuristics. Automatic placement is now disabled explicitly for that fixture. Fixtures are also generated with the realtime settings the product uses.
vpxdec.c becomes a helper with no suite of its own and is compiled into the vp8dec and the new vp9dec test binaries, which register the cases for their own codec. A build with only VP8 or only VP9 now still runs the error path coverage for the codec it has.
camilo-celis
force-pushed
the
camilo/vpx-frame-cleanup
branch
from
September 30, 2026 04:33
971922c to
026bd47
Compare
camilo-celis
marked this pull request as ready for review
September 30, 2026 04:37
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The unsupported-format path reads the decoded image after calling vpx_img_free().
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Fixes retained VPX decoder frames on terminal error paths and adds regression coverage.
Changes:
- Releases rejected frames through
GstVideoDecoder. - Adds shared VP8/VP9 error-path and recovery tests.
- Registers and builds the expanded decoder suites.
| File | Description |
|---|---|
ext/vpx/gstvpxdec.c |
Releases frames on four error paths. |
tests/check/meson.build |
Builds shared VPX tests. |
tests/check/elements/vpxdec.c |
Adds shared regression tests. |
tests/check/elements/vpxdec.h |
Declares test registration hooks. |
tests/check/elements/vp8dec.c |
Registers VP8 tests. |
tests/check/elements/vp9dec.c |
Adds the VP9 test suite. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The unsupported format exit read img->fmt after vpx_img_free (img), so the message depended on memory the decoder had already handed back. Emit the error first, then free. Flow, return value and frame release are unchanged. The tests now require the posted error to name the format the rejected image actually had.
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.
gst_video_decoder_decode_frame pushes an extra reference to the frame onto the decoder's pending queue. The four terminal error exits in gst_vpx_dec_handle_frame only dropped the handler's own reference with gst_video_codec_frame_unref, so every rejected frame stayed queued for the lifetime of the element, along with its input buffer.
Under sustained corrupt input this retains one frame per decode error. In pexip/mcu#51035, 3319 decode errors left 3319 pending frames holding roughly 60 MB. This amplifies an earlier failure; it is not the cause of the original VP9 corrupt frame or memory error, which remains unexplained.
Change: The terminal error exits (open codec fatal error, input buffer map failure, vpx_codec_decode failure, unsupported color format) now call gst_video_decoder_release_frame, which the base class already uses for its own discard path. Return values, warnings and force key unit ordering are unchanged, and gstvideodecoder.c is untouched.
Tradeoff: release_frame posts no QoS drop message, so no new dropped-buffer reporting appears. Events carried by a rejected frame are now forwarded ahead of a following GAP instead of being destroyed with the frame. They become pending sticky events, following the same flush semantics as every other release and drop path in the decoder.