fix(ffmpeg-ipc): harden IpcFrameProvider/IpcSampleProvider (leak guard, size validation, prefetch, EOF clamp)#2012
Merged
Merged
Conversation
BuildBitmap allocated the native Bitmap and then called SharedMemoryBuffer.Read with no try/catch. Read bounds-checks against the shared-buffer Capacity (the DataLength==Width*Height*8 guard does not), so a frame exceeding Capacity threw ArgumentOutOfRangeException after allocation, leaking the native bitmap. Wrap the Read in try/catch that disposes the bitmap before propagating, mirroring IpcSampleProvider.FetchChunk. Also remove provably-dead defensive code in RenderFrame: SendAndReceiveAsync never returns null (it throws IOException on a closed connection) and converts error responses to FFmpegWorkerException before returning, so the `?? throw IOException` and the `if (response.Error != null) throw InvalidOperationException` were unreachable. The CancelEncode check and the GetPayload null guard stay (both are live). Refs: Project #9 "AI Review" item "IpcFrameProvider.BuildBitmap leaks Bitmap on Read throw"
The encoder requests its final audio chunk with frame.NbSamples, which can run past SampleCount near the timeline end. When SampleCount aligns to a chunk boundary the cross-boundary path loaded an empty next chunk and sliced a 0-length span, throwing ArgumentOutOfRangeException; with a non-aligned EOF it silently copied out-of-range data. Clamp the effective length to the real remaining samples (Math.Max(0, SampleCount - offset)) and return a short final Pcm (offset >= SampleCount yields an empty Pcm). The consumer (FFmpegEncodingController.GetAudioFrame) copies only NumSamples * SampleSize bytes and advances NextPts by the requested NbSamples regardless, so a clamped Pcm keeps the encoder correct. Refs: Project #9 "AI Review" item "IpcSampleProvider.Sample throws past end of timeline"
…ead guards Three hardening fixes to IpcSampleProvider, all in the sample-load path: - FetchChunk used the worker-reported DataLength as the Read length without validating it against the native Pcm capacity (NumSamples * 8), so an oversized DataLength overran the allocation (native heap corruption). Add a Stereo32BitFloatBytesPerSample const, reject a negative NumSamples and any DataLength that disagrees with NumSamples * 8, and Read using the validated length. Mirrors IpcFrameProvider's RgbaF16BytesPerPixel guard. - EnsureChunkLoaded's prefetch-hit branch awaited _prefetchTask before clearing it, so a faulted prefetch pinned the provider to a re-throwing task forever. Reorder to clear-before-await, matching the stale-drain branch and IpcFrameProvider.RenderFrame. - Remove provably-dead defensive code in FetchChunk: SendAndReceiveAsync never returns null (throws IOException on a closed connection) and converts error responses to FFmpegWorkerException before returning, so the `?? throw IOException` and `if (response.Error != null) throw InvalidOperationException` were unreachable. The CancelEncode check and GetPayload null guard stay. Refs: Project #9 "AI Review" item "IpcSampleProvider FetchChunk wild-write and pinned prefetch"
Add NUnit coverage for the IpcFrameProvider / IpcSampleProvider hardening: - RenderFrame_WhenFrameExceedsBufferCapacity_ThrowsFromReadAndDoesNotLeak: a 32x32 frame (8192 B > 4096 B buffer) passes the DataLength guard but overflows SharedMemoryBuffer.Read; pins that the overflow surfaces as a throw (the fix's catch disposes the bitmap so it can't leak). - Sample_WhenWorkerReportsMismatchedDataLength_Throws: an over/under-sized DataLength vs NumSamples is rejected before the Read. - Sample_WhenHostErrors_ThrowsFFmpegWorkerException: an injected worker error surfaces as FFmpegWorkerException, not InvalidOperationException. - Sample_WhenPrefetchFaults_RecoversOnRetry: a faulted sample prefetch is not pinned; a retry recovers. - Sample_WhenRequestStraddlesAlignedEof_ClampsWithoutThrowing: a request past an aligned EOF clamps to the real samples instead of throwing. Adds a sample-side fake-host harness mirroring the existing frame-side helpers. Refs: Project #9 "AI Review" item "Harden Beutl.FFmpegIpc providers"
…etch drain Address review nits on the IPC provider hardening branch: - Move the Stereo32BitFloatBytesPerSample test constant up beside the other per-format size constants instead of declaring it mid-file. - Make the EnsureChunkLoaded stale-drain comment explicit that the field is cleared before the await, so a faulted stale prefetch propagates once and does not pin the provider. Refs: Project #9 "AI Review" items 264/267
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughFrame IPC now treats non-cancel worker failures as exceptions and disposes bitmaps on read failure. Sample IPC now clamps requests to available data, clears prefetched tasks before awaiting, validates worker-reported sizes, and preserves zero-filled tails past EOF. Contract tests cover these paths. ChangesIPC frame and sample validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the FFmpeg IPC client providers (IpcFrameProvider / IpcSampleProvider) against malformed worker responses and prefetch failures, and extends the IPC provider contract tests to cover the new guards and regressions around EOF/prefetch behavior.
Changes:
- Add leak-guards and size validation when reading frame/sample payloads from shared memory.
- Fix faulted-prefetch “pinning” by clearing the prefetch task field before awaiting.
- Expand
IpcProviderContractTeststo cover oversized frames, mismatched sample lengths, injected errors, prefetch fault recovery, and EOF behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/Beutl.FFmpegIpc.Tests/IpcProviderContractTests.cs | Adds sample-side host harness and new contract tests for IPC providers (validation, prefetch fault recovery, EOF). |
| src/Beutl.FFmpegIpc/Providers/IpcSampleProvider.cs | Adds sample DataLength/NumSamples validation, fixes prefetch task pinning, and introduces EOF clamp logic. |
| src/Beutl.FFmpegIpc/Providers/IpcFrameProvider.cs | Removes unreachable null/error guards and adds a dispose-on-failure leak guard when reading into a newly allocated Bitmap. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/Beutl.FFmpegIpc.Tests/IpcProviderContractTests.cs (1)
344-371: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest asserts the throw but not the "DoesNotLeak" guarantee.
RenderFrame_WhenFrameExceedsBufferCapacity_ThrowsFromReadAndDoesNotLeakonly verifies theArgumentOutOfRangeExceptionsurfaces; it does not actually observe thatBuildBitmap'scatchdisposed the freshly allocatedBitmap. The name implies a no-leak assertion that isn't made, so a regression that drops thebmp.Dispose()would still pass. If disposal can be observed (e.g., via a test seam or a disposal hook onBitmap), consider asserting it; otherwise a comment noting the limitation would avoid a false sense of coverage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Beutl.FFmpegIpc.Tests/IpcProviderContractTests.cs` around lines 344 - 371, The test RenderFrame_WhenFrameExceedsBufferCapacity_ThrowsFromReadAndDoesNotLeak currently only checks that IpcFrameProvider.RenderFrame throws ArgumentOutOfRangeException and does not verify the no-leak part of the scenario. Update this test to also observe that the Bitmap allocated inside BuildBitmap is disposed when SharedMemoryBuffer.Read throws, using an available test seam or disposal hook on Bitmap if possible; otherwise adjust the test name or add a clarifying comment so the coverage claim matches what is actually asserted.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/Beutl.FFmpegIpc.Tests/IpcProviderContractTests.cs`:
- Around line 344-371: The test
RenderFrame_WhenFrameExceedsBufferCapacity_ThrowsFromReadAndDoesNotLeak
currently only checks that IpcFrameProvider.RenderFrame throws
ArgumentOutOfRangeException and does not verify the no-leak part of the
scenario. Update this test to also observe that the Bitmap allocated inside
BuildBitmap is disposed when SharedMemoryBuffer.Read throws, using an available
test seam or disposal hook on Bitmap if possible; otherwise adjust the test name
or add a clarifying comment so the coverage claim matches what is actually
asserted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ebdd75d2-a806-42d3-99a7-ccbce8fa45f3
📒 Files selected for processing (3)
src/Beutl.FFmpegIpc/Providers/IpcFrameProvider.cssrc/Beutl.FFmpegIpc/Providers/IpcSampleProvider.cstests/Beutl.FFmpegIpc.Tests/IpcProviderContractTests.cs
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9778773022
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
2 tasks
…f returning a short Pcm The end-of-timeline handling returned a shortened Pcm when the final request straddled EOF. FFmpegEncodingController.GetAudioFrame copies pcm.NumSamples into a single reused, fixed-size MediaFrame but still converts/encodes frame.NbSamples and advances by it, so a short Pcm left stale samples from the previous frame in the tail of the final encoded audio frame. It also diverged from the ISampleProvider convention that SampleProviderImpl already follows (return the requested length, zero-filling past EOF). Sample now always returns a Pcm of the requested length: the available prefix is read via the extracted SampleExact (which never slices past EOF), and the past-EOF tail is left as the zero-fill Pcm<T> already allocates (silence). Update the straddle test to assert the requested length, the preserved real prefix, and the silent tail; rename it to Sample_WhenRequestStraddlesEof_ReturnsRequestedLengthWithSilenceTail. Also assert FramesRendered == 0 in the oversized-frame test: the native Bitmap dispose on the throw path can't be observed in-process, but a failed render must not count as a completed frame. Addresses PR #2012 review feedback (Copilot, Codex P2).
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Beutl.FFmpegIpc/Providers/IpcSampleProvider.cs (1)
195-205: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winValidate
NumSamplesagainst the requested chunk length.A worker can return self-consistent metadata with the wrong length, e.g. request 4 samples but return
NumSamples = 2andDataLength = 16. That passes the current guard, but breaks_currentChunkOffset + _currentChunk.NumSampleschunk invariants and can duplicate/slice the wrong data inSampleExact.Proposed fix
if (sampleInfo.NumSamples < 0) throw new InvalidOperationException( $"Sample chunk has a negative NumSamples {sampleInfo.NumSamples}."); + if (sampleInfo.NumSamples != chunkLength) + throw new InvalidOperationException( + $"Sample chunk has {sampleInfo.NumSamples} samples, but {chunkLength} were requested."); + long expected = (long)sampleInfo.NumSamples * Stereo32BitFloatBytesPerSample;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Beutl.FFmpegIpc/Providers/IpcSampleProvider.cs` around lines 195 - 205, `IpcSampleProvider.SampleExact` only validates that `sampleInfo.NumSamples` and `sampleInfo.DataLength` are self-consistent, but it does not verify that the returned `NumSamples` matches the requested chunk length. Add a guard in this method, near the existing `NumSamples` and `DataLength` checks, that compares `sampleInfo.NumSamples` against the requested chunk size used for the current read and throws if they differ. Keep the validation before constructing `Pcm<Stereo32BitFloat>` so `_currentChunkOffset` and `_currentChunk.NumSamples` remain consistent.
🧹 Nitpick comments (1)
tests/Beutl.FFmpegIpc.Tests/IpcProviderContractTests.cs (1)
616-618: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a self-consistent wrong-
NumSamplescase.These cases only reject
DataLength != NumSamples * 8. Please also cover a worker response likeNumSamples = 2, DataLength = 16for a request of 4 samples, which currently passes the size check but violates the chunk contract. As per coding guidelines, “New logic must ship with a NUnit test undertests/in the matching test project.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Beutl.FFmpegIpc.Tests/IpcProviderContractTests.cs` around lines 616 - 618, The current Sample_WhenWorkerReportsMismatchedDataLength_Throws test only covers raw size mismatches, so add a self-consistent wrong-NumSamples case in IpcProviderContractTests that matches the byte count but violates the chunk contract. Update the existing test or add a new NUnit case around the same Sample_WhenWorkerReportsMismatchedDataLength_Throws path to simulate a 4-sample request with a worker response like NumSamples = 2 and DataLength = 16, and assert the contract violation is rejected by the relevant IpcProvider sample-handling logic.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/Beutl.FFmpegIpc/Providers/IpcSampleProvider.cs`:
- Around line 195-205: `IpcSampleProvider.SampleExact` only validates that
`sampleInfo.NumSamples` and `sampleInfo.DataLength` are self-consistent, but it
does not verify that the returned `NumSamples` matches the requested chunk
length. Add a guard in this method, near the existing `NumSamples` and
`DataLength` checks, that compares `sampleInfo.NumSamples` against the requested
chunk size used for the current read and throws if they differ. Keep the
validation before constructing `Pcm<Stereo32BitFloat>` so `_currentChunkOffset`
and `_currentChunk.NumSamples` remain consistent.
---
Nitpick comments:
In `@tests/Beutl.FFmpegIpc.Tests/IpcProviderContractTests.cs`:
- Around line 616-618: The current
Sample_WhenWorkerReportsMismatchedDataLength_Throws test only covers raw size
mismatches, so add a self-consistent wrong-NumSamples case in
IpcProviderContractTests that matches the byte count but violates the chunk
contract. Update the existing test or add a new NUnit case around the same
Sample_WhenWorkerReportsMismatchedDataLength_Throws path to simulate a 4-sample
request with a worker response like NumSamples = 2 and DataLength = 16, and
assert the contract violation is rejected by the relevant IpcProvider
sample-handling logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4a551f14-6a6e-48d8-99f9-d70cead5b9fa
📒 Files selected for processing (2)
src/Beutl.FFmpegIpc/Providers/IpcSampleProvider.cstests/Beutl.FFmpegIpc.Tests/IpcProviderContractTests.cs
Drop the inline "// Final frame straddles EOF: fill the available prefix..."
note, which restated the three lines below it (the EOF zero-fill strategy is
already documented at the top of Sample), and the test-only cross-reference
phrases ("mirrors the frame-side helpers above", "Mirrors the same-named const
in IpcSampleProvider"). Comment text only; no code change.
Contributor
|
No TODO comments were found. |
Contributor
Minimum allowed line rate is |
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.
Description
Hardens the MIT-side IPC client providers (
IpcFrameProvider/IpcSampleProvider) against malformed/oversized worker responses and faulted prefetch, mirroring the guards each already had on its sibling. All findings come from a code review of the IPC same-frame-reuse work and are tracked on the AI Review board (project #9).IpcFrameProvider.BuildBitmapallocated the native-backedBitmapand then calledSharedMemoryBuffer.Readwith notry/catch. The DataLength guard validates against the destination bitmap size, butReadseparately bounds-checks against the shared-buffer capacity; a frame larger than the shared buffer throwsArgumentOutOfRangeExceptionafter allocation and leaked the bitmap. Now wrapped intry { … } catch { bmp.Dispose(); throw; }, matchingIpcSampleProvider.FetchChunk.FetchChunkcopiedsampleInfo.DataLengthbytes into aPcm<Stereo32BitFloat>(capacityNumSamples * 8) without validating the reported size. An oversized/negativeDataLengthoverran the native allocation. Added aNumSamples >= 0guard and aDataLength == NumSamples * 8check before the read, and the copy now uses the validatedexpectedlength — the same shape as the frame-side guard.EnsureChunkLoadedawaited_prefetchTaskbefore clearing it, so a faulted prefetch pinned the provider to a re-throwing task forever. Now clears the field before awaiting, like the stale-drain branch andIpcFrameProvider.RenderFrame.IpcConnection.SendAndReceiveAsyncalready surfaces a closed connection asIOExceptionand an error response asFFmpegWorkerException, so the downstream?? throw new IOException(...)andif (response.Error != null) throw new InvalidOperationException(...)guards in both providers were unreachable. Removed them, leaving a comment documenting the contract; only the liveCancelEncoderesponse still needs handling.IpcSampleProvider.Samplenow clamps the request to the real timeline (SampleCount). The encoder issues the finalSamplewithframe.NbSamples, which can straddle EOF; an unclamped length either sliced a zero-length next chunk whenSampleCountaligns to a chunk boundary (ArgumentOutOfRangeException) or copied out-of-range data when it didn't. The consumer tolerates a short finalPcm, so clamping (rather than zero-padding) keeps the encoder correct.Affected areas
Beutl.Engine(rendering / scene / track)Beutl.ProjectSystem(project / document persistence)Beutl.Editor,Beutl.Editor.Components,Beutl.Controls)Beutl.Extensibility(plugin abstractions)Beutl.NodeGraph(node editor)Beutl.FFmpegIpc/Beutl.FFmpegWorker(media IPC boundary)Beutl.Api(server API client)Breaking changes
None. All changes are internal to
Beutl.FFmpegIpc(the providers areinternal sealed) and behavior-preserving for valid worker responses — they only add guards on malformed/oversized input and tighten timeline-edge handling.Test plan
All in
tests/Beutl.FFmpegIpc.Tests/IpcProviderContractTests.cs, driven by a fake host over a realNamedPipe. New tests added on top of the existing frame-side suite:RenderFrame_WhenFrameExceedsBufferCapacity_ThrowsFromReadAndDoesNotLeak— 32x32 frame > 4096-byte buffer surfaces theReadcapacity overflow as a throw (bitmap disposed in the catch).Sample_WhenWorkerReportsMismatchedDataLength_Throws([TestCase]over/under-sized) — DataLength disagreeing withNumSamples * 8is rejected before the read.Sample_WhenHostErrors_ThrowsFFmpegWorkerException— an injected worker error surfaces asFFmpegWorkerException(proves the dropped guard did not change the error type).Sample_WhenPrefetchFaults_RecoversOnRetry— a faulted sample prefetch is not pinned; a retry recovers.Sample_WhenRequestStraddlesAlignedEof_ClampsWithoutThrowing— a request past an aligned EOF clamps to the real remaining samples.Result:
成功! 失敗: 0、合格: 16、スキップ: 0、合計: 16(net10.0).dotnet format --verify-no-changesclean on the three changed files.Fixed issues / References
Summary by CodeRabbit