refactor!: make IFrameProvider/ISampleProvider IDisposable and guard IPC providers post-dispose#2013
Merged
Merged
Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThe PR documents encoding/provider disposal rules, makes IPC frame and sample providers track disposed state and observe faulted prefetch work during teardown, disposes those providers in the worker handler, and adds tests that verify no unobserved task exceptions surface. ChangesEncoding disposal and prefetch observation
Sequence Diagram(s)sequenceDiagram
participant EncodingHandler
participant EncodingController
participant IpcFrameProvider
participant IpcSampleProvider
EncodingHandler->>EncodingController: Encode(frameProvider, sampleProvider, outputFile)
EncodingController-->>EncodingHandler: returns or throws
EncodingHandler->>IpcFrameProvider: Dispose()
EncodingHandler->>IpcSampleProvider: Dispose()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
![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: 83e4535808
ℹ️ 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".
yuto-trd
force-pushed
the
yuto-trd/ipc-provider-idisposable
branch
2 times, most recently
from
6f0c57c to
d304835
Compare
…inistic teardown Encoder frame/sample providers can hold an in-flight background prefetch (the IPC providers double-buffer the next frame/chunk). Without a disposal contract, a faulted prefetch that nobody awaits surfaces later as an UnobservedTaskException when the GC reclaims the task. Make IFrameProvider and ISampleProvider extend IDisposable so providers can be torn down deterministically, and migrate every implementer: - IpcFrameProvider / IpcSampleProvider gain an idempotent Dispose() that *observes* (does not block on) the in-flight _prefetchTask via a fault-swallowing continuation, so a faulted prefetch is marked observed and Dispose returns promptly even if the pipe is already torn down (a synchronous .Wait() could deadlock/rethrow). IpcSampleProvider additionally disposes the cached _currentChunk and the Pcm a successfully-completing prefetch produces. - EncodingHandler (GPL worker) now wraps both providers in `using` so they are disposed after Encode returns or throws. - FrameProviderImpl / SampleProviderImpl / AudioBufferSampleProvider already implemented IDisposable and their owners already dispose them (OutputViewModel via `using`); FFmpegEncodingControllerProxy keeps NOT disposing the caller-owned providers (ownership stays with the app-side creators). - Test doubles (GradientFrameProvider, SineSampleProvider, SweepSampleProvider, CancelAfterFirstFrameProvider) gain a Dispose(). Adds IpcProviderContractTests proving Dispose() observes a faulted in-flight prefetch so no UnobservedTaskException attributable to the provider is raised (frame and sample side), driven via internal IsPrefetchFaultedForTest probes. BREAKING CHANGE: IFrameProvider and ISampleProvider in Beutl.Extensibility now extend IDisposable. Out-of-tree implementers must implement IDisposable. If the provider starts a background prefetch, Dispose() must drain (observe) it — e.g. attach a fault-swallowing continuation that touches Task.Exception — so a faulted prefetch cannot later raise an UnobservedTaskException; do not block on the task synchronously. Stateless providers may use an empty Dispose(). Refs: Project #9 "AI Review" item 266.
…ment Dispose asymmetry Address review findings on the IDisposable change: - Throw ObjectDisposedException from IpcFrameProvider.RenderFrame and IpcSampleProvider.Sample after Dispose, matching the public FrameProviderImpl / SampleProviderImpl implementations now that the interfaces are IDisposable. - Document why IpcFrameProvider.Dispose uses OnlyOnFaulted while IpcSampleProvider also disposes on success: the frame prefetch yields a managed IpcMessage (the Bitmap is built lazily on consumption, nothing to release), whereas the sample prefetch yields a native Pcm that must be disposed on the success path. The asymmetry is intentional, not an oversight — a reviewer read the frame prefetch as Task<Bitmap>; it is Task<IpcMessage>, so there is no Bitmap to leak. Refs: Project #9 "AI Review" item 266
…Controller.Encode Now that IFrameProvider/ISampleProvider are IDisposable, an EncodingController implementation receives IDisposable arguments with no documented signal about who owns them. Following the "last user disposes" heuristic and disposing a provider inside Encode would tear down resources the caller still owns (e.g. FrameProviderImpl.Dispose cancels the background producer task), so document on the abstract Encode that the caller retains ownership and implementations must not dispose either argument — closing the footgun the IDisposable change adds. Also clarify IpcSampleProvider.Dispose: its continuation handles all three terminal states (Faulted / CompletedSuccessfully / Canceled) and why ExecuteSynchronously is safe (t.Result.Dispose() is a single P/Invoke free). Docs/comments only; no logic or API change. Tests unchanged (18/18 green).
…mment Drop the "is deliberate ... is intentional" reviewer-facing framing; keep the substance — the frame prefetch holds a managed IpcMessage (Bitmap built lazily on consume) so OnlyOnFaulted leaks nothing, contrasted with the sample provider's native Pcm that its continuation disposes on success. Comment only.
yuto-trd
force-pushed
the
yuto-trd/ipc-provider-idisposable
branch
from
d304835 to
50d7413
Compare
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
1 participant
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
Stacked on #2012 (base =
yuto-trd/ipc-provider-hardening). Gives the frame/sample provider abstractions a deterministic teardown contract and hardens the IPC implementations against use-after-dispose.IFrameProvider/ISampleProvidernow implementIDisposable. An in-flight prefetch could previously outlive the encode and surface as anUnobservedTaskException; consumers can now dispose the provider, which neutralizes the pending prefetch (observing any fault and releasing a nativePcmon the success path).IpcFrameProvider/IpcSampleProvider:Dispose()is idempotent and attaches a continuation to the in-flight prefetch instead of blocking on a possibly-closing pipe. Public entry points (RenderFrame/Sample) nowObjectDisposedException.ThrowIfafter dispose. The intentional success-path asymmetry between the two providers (managedIpcMessagevs nativePcm) is documented inline.Beutl.FFmpegWorker.EncodingHandlerwraps the providers inusingso the worker tears them down deterministically.Affected areas
Beutl.Extensibility(plugin abstractions)Beutl.FFmpegIpc/Beutl.FFmpegWorker(media IPC boundary)Breaking changes
IFrameProviderandISampleProvidernow extendIDisposable. Out-of-tree implementations must add aDispose()method (it may be a no-op for stateless providers), and owners of a provider instance should dispose it.refactor!:+BREAKING CHANGE:footer present on the implementing commit.Test plan
tests/Beutl.FFmpegIpc.Tests/IpcProviderContractTests.cs(+116): two Dispose tests that drop an in-flight, deterministically-faulted prefetch (gated on anIsPrefetchFaultedForTest()probe, no sleeps) under anUnobservedTaskExceptionwatcher; existing serve/seek/cancel coverage retained.tests/Beutl.Extensions.AVFoundation.Tests/TestProviders.cs+EncodingCancellationTests.cs: test doubles updated for the newIDisposablemember.Beutl.FFmpegIpc.Tests18/18 green.Fixed issues / References
Summary by CodeRabbit
Bug Fixes
Documentation