Repository navigation
Fix BinaryCodec temporary resource cleanup on errors - #365
Draft
linjiaye929 wants to merge 1 commit into
Draft
linjiaye929 wants to merge 1 commit into
linjiaye929 wants to merge 1 commit into
Conversation
This branch has not been deployed
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.
BinaryCodec currently opens two mkstemp descriptors and removes its temporary files only after successful encoding, decoding and image loading. When an external encoder fails, run_command raises SystemExit and bypasses that cleanup.
This is reproducible on the unmodified upstream head through JPEG2000.run(): FFmpeg/libopenjpeg rejects a valid 1×1 PNG because its default resolution count exceeds the image dimensions. Each failed call leaves two open descriptors and two files. Two calls leave four of each. This is independent of the AV1 input-path fix in #364; no AV1 code is changed here.
Use a TemporaryDirectory for BinaryCodec's encoded output and reconstructed PNG, closing their mkstemp descriptors immediately and cleaning up on both successful and exceptional exits. The files are still pre-created with the same suffixes and permissions. Command construction and exception propagation are unchanged.
Validation with the real FFmpeg/libopenjpeg codec:
BinaryCodec is shared by JPEG2000, BPG and TFCI. Only JPEG2000 received real codec integration tests; BPG/TFCI tools and a decoder-specific failure were not tested. The existing JPEG binary-fixture test, full suite and upstream CI are not verified. VTM/HM/AV1 have separate implementations and are unchanged.
Prepared with AI assistance. Keeping this as a draft pending broader validation.