refactor(ffmpeg): extract seek/prefetch decisions as pure predicates + tests#2014
Merged
Conversation
…+ tests Hoist two FFmpeg worker failure-path decisions into a pure, native-free helper so they get direct NUnit coverage without spawning the worker or loading FFmpeg natives, following the existing FFmpegFrameValidation dual-compile pattern (#if BEUTL_FFMPEG_WORKER namespace switch + <Compile Include Link> into the GPL worker csproj): - FFmpegSeekDecision.ShouldReseek(currentUsable, skip): mirrors the inline re-seek condition in FFmpegReader.ReadVideoCore (drop the active frame and force a fresh seek when it is unusable or the skip is out of range). - FFmpegSeekDecision.HasPrefetchTarget(baseFrame, cachedAhead, totalFrames, out nextFrame): mirrors the inline prefetch-advance condition in VideoRingBuffer.StartPrefetch (next target exists below EOF for a non-negative base). Behavior-preserving extraction; no FFmpeg-native call is moved. Adds table-driven MIT NUnit tests over the decision boundaries. Refs: Project #9 "AI Review" item "Add direct NUnit coverage for PR #1985 seek-past-EOF failure-path decisions in the GPL worker"
…tchTarget Address review nit: document that the fast-path early break and the extracted HasPrefetchTarget predicate intentionally re-check the same baseFrame<0 guard, so the predicate stays self-contained and unit-testable. Refs: Project #9 "AI Review" item 246
…call mode Beutl.FFmpegBenchmarks compiles the worker decoder directly by source-linking FFmpegReader.cs (with BEUTL_FFMPEG_WORKER defined). The predicate extraction moved the reseek/prefetch logic into a new FFmpegSeekDecision.cs that FFmpegReader.cs now references, so the benchmark project must source-link that file too or it fails with CS0103 (FFmpegSeekDecision not found).
|
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)
📝 WalkthroughWalkthroughA shared ChangesFFmpeg seek decision extraction
Sequence Diagram(s)sequenceDiagram
participant Reader as FFmpegReader.ReadVideoCore
participant Prefetch as VideoRingBuffer.StartPrefetch
participant Decision as FFmpegSeekDecision
Reader->>Decision: ShouldReseek(currentUsable, skip)
Decision-->>Reader: reseek decision
Prefetch->>Decision: HasPrefetchTarget(baseFrame, cachedAhead, totalFrames, out nextFrame)
Decision-->>Prefetch: target availability and nextFrame
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 refactors FFmpeg direct-call decoding by extracting the reseek and prefetch branching logic into a small pure helper (FFmpegSeekDecision) so it can be unit-tested without invoking FFmpeg, and adds regression coverage around seek/prefetch edge conditions (including seek-past-EOF–adjacent behavior from the #1985 follow-up context).
Changes:
- Introduces
FFmpegSeekDecisionwith pure predicates (ShouldReseek,HasPrefetchTarget) and dual-namespace compilation across MIT extension vs GPL worker via#if BEUTL_FFMPEG_WORKER. - Updates
FFmpegReader.ReadVideoCoreandVideoRingBuffer.StartPrefetchto call the extracted predicates (behavior-preserving). - Adds table-driven unit tests and source-links the new helper into the worker and benchmark projects.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Beutl.UnitTests/Extensions/FFmpeg/FFmpegSeekDecisionTests.cs | Adds unit coverage for reseek threshold logic and prefetch target selection. |
| tests/Beutl.FFmpegBenchmarks/Beutl.FFmpegBenchmarks.csproj | Source-links the new helper so linked worker reader code continues to compile in benchmarks. |
| src/Beutl.FFmpegWorker/Handlers/VideoRingBuffer.cs | Replaces inline prefetch EOF/negative-base branching with FFmpegSeekDecision.HasPrefetchTarget. |
| src/Beutl.FFmpegWorker/Decoding/FFmpegReader.cs | Replaces inline reseek condition with FFmpegSeekDecision.ShouldReseek. |
| src/Beutl.FFmpegWorker/Beutl.FFmpegWorker.csproj | Source-links the helper into the GPL worker build (consistent with existing boundary approach). |
| src/Beutl.Extensions.FFmpeg/Decoding/FFmpegSeekDecision.cs | New internal helper implementing the extracted pure predicates with boundary-safe namespace switching. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…icate comments FFmpegReader and VideoRingBuffer now call ShouldReseek / HasPrefetchTarget, so the "mirrors the inline condition in X" framing on those docs is stale (X no longer holds an inline condition). Trim it, keeping the per-condition rationale and the out-param contract. Drop the VideoRingBuffer fast-path note, which only justified a behavior-preserving duplicate baseFrame<0 guard.
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
Extracts the reseek/prefetch branch conditions in the FFmpeg direct-call decoder into a small pure helper so they can be unit-tested without spinning up FFmpeg, and adds the seek-past-EOF regression coverage requested as a PR #1985 follow-up.
FFmpegSeekDecision(inBeutl.Extensions.FFmpeg/Decoding, namespace-switched toBeutl.FFmpegWorker.DecodingunderBEUTL_FFMPEG_WORKER):ShouldReseek(bool currentUsable, long skip)andHasPrefetchTarget(...)capture the exact inline conditions fromFFmpegReader.ReadVideoCore/VideoRingBuffer.FFmpegReader.csandVideoRingBuffer.csnow call the helper; behavior is preserved (abaseFrame < 0invariant note added where the predicate is shared).Beutl.FFmpegWorker.csprojandBeutl.FFmpegBenchmarks.csproj(which source-linksFFmpegReader.csdirectly) both source-link the new file.Affected areas
Beutl.FFmpegIpc/Beutl.FFmpegWorker(media IPC boundary)Breaking changes
None — behavior-preserving extraction.
Test plan
tests/Beutl.UnitTests/Extensions/FFmpeg/FFmpegSeekDecisionTests.cs(new, 17 table-driven cases) covering the reseek thresholds (currentUsable, large/negativeskip,MaxSequentialSkipboundary) and prefetch-target selection.Fixed issues / References
Summary by CodeRabbit