fix(ffmpeg-ipc): report mid-encode CancelEncode as a clean cancel; extract PrefetchSlot#2033
Merged
Merged
Conversation
…el-during-encode reporting
Extract the hand-duplicated double-buffer prefetch/seek state-machine from IpcFrameProvider and
IpcSampleProvider into a shared MIT-side PrefetchSlot<TKey, TValue> helper — a request whose key
matches the armed prefetch consumes it, any other key drains the stale prefetch (await-and-discard)
before a fresh request is issued — and unify both providers onto it.
Fix the worker-side cancel-during-encode reporting bug. The host injects CancelEncode with a fresh
id (FFmpegEncodingControllerProxy) while the worker is blocked in the non-multiplexed
SendAndReceiveAsync awaiting a frame/sample response, so it is read in place of that response,
trips the id-match check, and surfaced as InvalidOperationException("Response ID mismatch") —
which HandleStartAsync reported as EncodeComplete{Error='Response ID mismatch'} instead of a clean
cancel. SendAndReceiveAsync now recognizes CancelEncode before the id-match check and raises
OperationCanceledException, so HandleStartAsync reports "Cancelled". The resulting dead CancelEncode
branches in both providers are removed.
Tests: PrefetchSlotTests pins the slot mechanics (green); new fresh-id cancel regression tests for
both providers (red against the unmodified id-mismatch path, green after the fix) join the existing
contract suite.
Refs: Project #9
…metry, and Arm precondition Apply design-review polish (doc/comment/assert only, behavior unchanged): - Add <exception> tags to IpcConnection.SendAndReceiveAsync documenting the OperationCanceledException raised when a host CancelEncode arrives in place of the response, plus the existing IOException / FFmpegWorkerException / InvalidOperationException throws. - Note on SendAndReceiveMultiplexedAsync that CancelEncode-as-cancellation is intentionally recognized only on the sequential path because the encode connection is never multiplexed. - Add a Debug.Assert guard in PrefetchSlot.Arm for the empty-slot precondition.
📝 WalkthroughWalkthroughThis PR adds a shared keyed prefetch slot, moves frame and sample IPC providers to use it, updates sequential IPC receive handling to surface ChangesIPC prefetch and cancellation handling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes worker-side cancellation reporting on non-multiplexed IPC connections by treating an out-of-band CancelEncode message as a clean cancellation (instead of an ID-mismatch error), and removes duplicated prefetch/seek state-machine code by extracting a shared PrefetchSlot<TKey, TValue> helper used by both IPC providers.
Changes:
- Update
IpcConnection.SendAndReceiveAsyncto recognizeCancelEncodebefore the response ID match check and throwOperationCanceledException. - Refactor
IpcFrameProvider/IpcSampleProviderto use a sharedPrefetchSlotfor prefetch consume/drain/detach behavior. - Add regression/contract tests for “fresh-id cancel” and a dedicated
PrefetchSlotunit test suite.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Beutl.FFmpegIpc.Tests/PrefetchSlotTests.cs | Adds unit tests pinning PrefetchSlot consume/drain/detach/fault behavior. |
| tests/Beutl.FFmpegIpc.Tests/IpcProviderContractTests.cs | Adds regression tests reproducing host cancel injection with a fresh ID for both frame and sample paths. |
| src/Beutl.FFmpegIpc/Transport/IpcConnection.cs | Recognizes CancelEncode as cancellation on the sequential (non-multiplexed) send/receive path. |
| src/Beutl.FFmpegIpc/Providers/PrefetchSlot.cs | Introduces shared single-slot prefetch state abstraction used by both providers. |
| src/Beutl.FFmpegIpc/Providers/IpcSampleProvider.cs | Replaces hand-rolled prefetch fields/state with PrefetchSlot and removes provider-level CancelEncode handling. |
| src/Beutl.FFmpegIpc/Providers/IpcFrameProvider.cs | Replaces hand-rolled prefetch fields/state with PrefetchSlot and removes provider-level CancelEncode handling. |
💡 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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/Beutl.FFmpegIpc/Providers/PrefetchSlot.cs (1)
36-41: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winEnforce the single-slot invariant in release builds.
Line 38 only asserts in debug builds; in release, a double
Armsilently overwrites the old in-flight task, which can leave stale IPC work unobserved.Proposed guard
public void Arm(TKey key, int bufferIndex, Task<TValue> task) { Debug.Assert(_task is null, "PrefetchSlot armed while a prefetch was already in flight"); + if (_task is not null) + throw new InvalidOperationException("PrefetchSlot already has a prefetch in flight."); + _key = key; _bufferIndex = bufferIndex; _task = task; }🤖 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/PrefetchSlot.cs` around lines 36 - 41, The single-slot invariant in PrefetchSlot.Arm is only checked with Debug.Assert, so a second Arm call can overwrite an in-flight task in release builds. Update Arm to enforce the _task-is-null precondition at runtime, using the existing PrefetchSlot members (_task, _key, _bufferIndex) to reject or fail fast on double-arming before assigning the new values.
🤖 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.
Inline comments:
In `@src/Beutl.FFmpegIpc/Providers/IpcFrameProvider.cs`:
- Around line 47-49: Discard stale prefetch worker errors before sending the
fresh frame request in IpcFrameProvider.GetFrameAsync. The stale task returned
by _prefetch.TryDetachStale(frame) should be awaited only to observe and
suppress its failure, not allowed to rethrow and abort the seek; handle or
ignore any exception from the detached stale worker, then continue to request
the target frame normally.
In `@src/Beutl.FFmpegIpc/Providers/IpcSampleProvider.cs`:
- Around line 142-146: The stale prefetch path in IpcSampleProvider should not
surface errors from a discarded chunk before the requested fetch runs. Update
the logic around TryDetachStale and FetchChunk so that awaiting the detached
stale task swallows or ignores its failure after disposal, then continue to load
the current chunk normally. Keep the fresh chunk request from being blocked by
any exception coming from the stale prefetch worker.
---
Nitpick comments:
In `@src/Beutl.FFmpegIpc/Providers/PrefetchSlot.cs`:
- Around line 36-41: The single-slot invariant in PrefetchSlot.Arm is only
checked with Debug.Assert, so a second Arm call can overwrite an in-flight task
in release builds. Update Arm to enforce the _task-is-null precondition at
runtime, using the existing PrefetchSlot members (_task, _key, _bufferIndex) to
reject or fail fast on double-arming before assigning the new values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7d4777b6-e988-4666-934f-e38fdbfc8ad8
📒 Files selected for processing (6)
src/Beutl.FFmpegIpc/Providers/IpcFrameProvider.cssrc/Beutl.FFmpegIpc/Providers/IpcSampleProvider.cssrc/Beutl.FFmpegIpc/Providers/PrefetchSlot.cssrc/Beutl.FFmpegIpc/Transport/IpcConnection.cstests/Beutl.FFmpegIpc.Tests/IpcProviderContractTests.cstests/Beutl.FFmpegIpc.Tests/PrefetchSlotTests.cs
The new test file lacked the UTF-8 BOM that .editorconfig (charset=utf-8-bom) requires; the CI dotnet-format check (whole-solution) failed with CHARSET. Re-applied dotnet format Beutl.slnx.
When a seek (or any non-sequential request) drains the armed-but-stale prefetch before issuing the fresh request, the providers awaited the detached task directly. On a worker-error response that await re-threw the discarded prefetch's FFmpegWorkerException, aborting the fresh request that should have succeeded: a seek away from a frame/chunk whose prefetch failed would surface the discarded frame's error and never request the target. The drain has already consumed the stale response off the pipe (the non-multiplexed SendAndReceiveAsync reads and releases the lock before faulting), so the await is needed only for ordering, not its result. Catch and discard FFmpegWorkerException at both drain sites (IpcFrameProvider, IpcSampleProvider); IOException and OperationCanceledException still propagate. Adds red-baseline regression tests for both providers. Also hardens PrefetchSlot: Arm throws InvalidOperationException on a double-arm (was Debug.Assert, compiled out in Release) so a silent overwrite cannot strand an undrained response; key equality uses EqualityComparer<TKey>.Default for null-safety; Detach clears the key and buffer-index metadata.
Contributor
|
No TODO comments were found. |
Contributor
Minimum allowed line rate is |
Member
Author
Ready for human merge — all gates green
State:
Bot-review findings resolved during settle:
A clean one-click squash-merge whenever you're ready. |
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 worker-side cancellation-reporting bug and removes the duplication that surfaced it, all within the MIT
Beutl.FFmpegIpclayer.The bug (fix)
The worker's
IpcConnectionis non-multiplexed, and during an active encode the provider'sSendAndReceiveAsyncis the only pipe reader. The host injectsCancelEncodewith a fresh id (FFmpegEncodingControllerProxyviaNextId()), so it arrives in place of the awaited frame/sample response, fails the response-id-match check, and was thrown asInvalidOperationException("Response ID mismatch")— whichEncodingHandler.HandleStartAsyncthen reported asEncodeComplete{Error="Response ID mismatch"}instead of a clean cancel. (User-visible cancel still worked because the host throwsOperationCanceledExceptionon its side, so impact was cosmetic/misleading worker-side reporting.)Fix:
IpcConnection.SendAndReceiveAsyncnow recognizes aCancelEncodemessage before the id-match check and throwsOperationCanceledException, so a mid-encode cancel surfaces asEncodeComplete{Error="Cancelled"}. The now-deadCancelEncodebranches in both providers are removed.The refactor
Extracted the hand-duplicated double-buffer prefetch/seek state machine from
IpcFrameProviderandIpcSampleProviderinto a newinternal PrefetchSlot<TKey, TValue>(Providers/PrefetchSlot.cs) — arm a prefetch, consume it on a matching key, drain the stale one on a mismatch — and unified both providers onto it.Tests
IpcProviderContractTests):RenderFrame/Samplewhen the host cancels with a fresh id — these fail against the unmodified code (InvalidOperationException) and pass after the fix (OperationCanceledException).PrefetchSlotTests(8): pin the consume/drain/detach behavior.Beutl.FFmpegIpc.Testssuite: 46/46 green; the GPL worker builds against the unchanged public provider constructors.Scope note
The board item also proposed an
IIpcConnectionseam (part 1). It was deliberately scoped out: the providers are already MIT-side and unit-tested via a real-pipe fake-host harness (PR #2012), so a consumer-less interface would be a speculative abstraction (and is cheap to extract later, internally, when a real consumer appears). The two reviewers concurred.Review notes
Reviewed by
@beutl-design-reviewerand@beutl-reviewer— both approve, no blocking findings. GPL/MIT boundary clean (all changes in MITBeutl.FFmpegIpc, no newProjectReference, provider ctors unchanged so the GPL worker still builds). The cancel recognition is correct and carries no false-cancel risk (CancelEncodeis never a legitimate reply to a frame/sample request). Not a breaking API change (no public signature changed; the only observable change is the exception type in a previously-broken scenario), sofix:notfix!:. Design-review polish applied before opening:<exception>XML docs onSendAndReceiveAsync, a note on the multiplexed path explaining the deliberate non-multiplexed-only cancel recognition, and a throwing empty-slot guard onPrefetchSlot.Arm(it throwsInvalidOperationExceptionif armed while a prefetch is already in flight — active in all build configurations, not compiled out in Release — covered byArm_WhenAlreadyArmed_Throws).Left for human merge by
/beutl-loop: this is a larger diff (~405 LOC, 6 files) that changes a public method's exception contract in the encode/IPC-critical path — above the loop's auto-merge size/sensitivity threshold, so a human makes the final call.Board item: Project #9 — Refactor: extract IpcConnection seam + shared prefetch state-machine; fix CancelEncode-as-id-mismatch reporting. Opened autonomously by
/beutl-loop.Summary by CodeRabbit