feat: Add proxy media workflow#2040
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds the proxy-media workflow end to end: specs and review notes, proxy contracts and storage/resolution, proxy-aware decoding/rendering and thumbnailing, FFmpeg-backed generation, configuration and UI wiring, export preflight checks, and test coverage for the new behavior. ChangesProxy Media Workflow
Estimated code review effort: 5 (Critical) | ~150 minutes Sequence Diagram(s)sequenceDiagram
participant VideoSource
participant DecoderRegistry
participant IProxyResolver
participant Decoder
VideoSource->>DecoderRegistry: OpenMediaFile(file, PreferProxy=true)
DecoderRegistry->>IProxyResolver: Resolve(fileUri, preset)
IProxyResolver-->>DecoderRegistry: ProxyResolution or null
alt proxy found
DecoderRegistry->>IProxyResolver: Pin(resolution)
DecoderRegistry->>Decoder: Open(proxy path)
DecoderRegistry-->>VideoSource: ProxyMediaReader
else no proxy
DecoderRegistry->>Decoder: Open(original file)
DecoderRegistry-->>VideoSource: MediaReader
end
sequenceDiagram
participant Caller
participant ProxyJobQueue
participant IProxyGenerator
participant IProxyStore
Caller->>ProxyJobQueue: EnqueueAsync(source, preset)
ProxyJobQueue->>ProxyJobQueue: deduplicate by (source,preset)
ProxyJobQueue-->>Caller: ProxyJob (Queued)
ProxyJobQueue->>IProxyGenerator: GenerateAsync(job)
IProxyGenerator-->>ProxyJobQueue: success / Skipped / Unavailable
ProxyJobQueue->>IProxyStore: Register(entry)
ProxyJobQueue->>Caller: JobChanged event
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![drift-check[bot]](https://avatars.githubusercontent.com/in/4198242?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Second review round on PR #2040: - Scope FFmpeg-missing detection to proxy generation: FFmpegDecoderInfo.Open returns null again (so DecoderRegistry can fall back to another decoder for a regular open) but records the missing-libraries condition; FFmpegProxyGenerator translates a fallback-less open failure into ProxyGeneratorUnavailableException. - Propagate DisableResourceShare into NodeGraphFilterEffect graph evaluation so a VideoSourceNode keeps reader isolation during export/full-scale renders. - Enumerate VideoSourceNode inputs inside NodeGraphFilterEffect filter chains in ProxySourceEnumerator (Proxies tab, delete/usage accounting, frame-cache). - Timeline proxy badge resolves its source through ProxySourceEnumerator so graph/referenced-scene/animated-only elements show the badge. - Protect animated (keyframed) project sources from proxy eviction. - Size the proxied thumbnail strip from LogicalFrameSize so a half/quarter proxy is not clipped (non-proxy path stays byte-identical). - Resolve symlinks before the frame-cache-invalidation path comparison via a shared ProxyFingerprint.ResolveComparableKey (also used by the eviction service). - Fix two stale comments/names flagged by drift-check.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Code Review BotDocumentation driftFound 5 possible documentation drift(s) (partially analyzed) — see the bot's latest pull request review for details. These are advisory. Reviewed 99 file(s); skipped 55. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/Beutl.UnitTests/Editor/ElementViewModelProxyStateTests.cs (1)
140-156: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen assertion to verify fingerprint content, not just count.
This test only asserts
resolved.Count == 2, which proves a recompute happened but doesn't confirm the returned fingerprints are correctly mapped per URI (e.g.,s_path→first). Since this covers cache-key logic explicitly flagged as a prior bug area ("Fix 1"), asserting the actual values would guard against subtler regressions like swapped/duplicated entries.♻️ Proposed strengthening
IReadOnlyList<ProxyFingerprint> resolved = ElementViewModel.ResolveCachedFingerprints( [new Uri(s_path), new Uri(otherPath)], false, ref cachedKey, ref cached, stat); - Assert.That(resolved, Has.Count.EqualTo(2)); + Assert.Multiple(() => + { + Assert.That(resolved, Has.Count.EqualTo(2)); + Assert.That(resolved, Does.Contain(first)); + });🤖 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.UnitTests/Editor/ElementViewModelProxyStateTests.cs` around lines 140 - 156, The test ResolveCachedFingerprints_ChangedUriSet_ReStatsWithoutInvalidate only checks the result count, so it does not verify that ElementViewModel.ResolveCachedFingerprints returns the correct fingerprints for each URI. Strengthen this assertion by validating the resolved collection contents against the expected ProxyFingerprint values for s_path and otherPath, ensuring the mapping/order is correct and not swapped or duplicated. Use the existing first, stat, and resolved variables in this test to assert actual fingerprint identity or matching path/size details.
🤖 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.UnitTests/Editor/ElementViewModelProxyStateTests.cs`:
- Around line 140-156: The test
ResolveCachedFingerprints_ChangedUriSet_ReStatsWithoutInvalidate only checks the
result count, so it does not verify that
ElementViewModel.ResolveCachedFingerprints returns the correct fingerprints for
each URI. Strengthen this assertion by validating the resolved collection
contents against the expected ProxyFingerprint values for s_path and otherPath,
ensuring the mapping/order is correct and not swapped or duplicated. Use the
existing first, stat, and resolved variables in this test to assert actual
fingerprint identity or matching path/size details.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8ff503bb-7ed7-4058-ae15-77cb80b72054
📒 Files selected for processing (11)
src/Beutl.Editor.Components/ProxiesTab/ViewModels/ProxiesTabViewModel.cssrc/Beutl.Editor.Components/TimelineTab/ViewModels/ElementViewModel.cssrc/Beutl.Engine/Media/Source/VideoSource.cssrc/Beutl.Extensions.FFmpeg/Proxy/FFmpegProxyGenerator.cssrc/Beutl/Services/ProxyMediaServices.cssrc/Beutl/ViewModels/EditViewModel.cssrc/Beutl/ViewModels/PlayerViewModel.cstests/Beutl.UnitTests/Editor/ElementViewModelProxyStateTests.cstests/Beutl.UnitTests/Editor/ProxiesTabViewModelTests.cstests/Beutl.UnitTests/Engine/Graphics/SourceVideoThumbnailProxyTests.cstests/Beutl.UnitTests/Extensions/FFmpeg/ProxyGenerationE2ETests.cs
🚧 Files skipped from review as they are similar to previous changes (10)
- src/Beutl/ViewModels/PlayerViewModel.cs
- tests/Beutl.UnitTests/Engine/Graphics/SourceVideoThumbnailProxyTests.cs
- tests/Beutl.UnitTests/Extensions/FFmpeg/ProxyGenerationE2ETests.cs
- src/Beutl/ViewModels/EditViewModel.cs
- src/Beutl/Services/ProxyMediaServices.cs
- src/Beutl.Editor.Components/TimelineTab/ViewModels/ElementViewModel.cs
- src/Beutl.Engine/Media/Source/VideoSource.cs
- tests/Beutl.UnitTests/Editor/ProxiesTabViewModelTests.cs
- src/Beutl.Extensions.FFmpeg/Proxy/FFmpegProxyGenerator.cs
- src/Beutl.Editor.Components/ProxiesTab/ViewModels/ProxiesTabViewModel.cs
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
…omposer The video-side SceneRenderer already passed forceOriginalSource: true, but the audio-side SceneComposer used the 2-arg overload and silently defaulted to false. Audio opens do not consult the proxy resolver today, so this was latent, but the export path must enforce the no-proxy contract itself — matching OutputViewModel. Extract the composer construction into an internal factory so the invariant is testable, and add coverage. Also delete the dead private LayerSkipModel struct left behind in ExportSourceValidator when the shared SceneLayerSkipModel was extracted.
There was a problem hiding this comment.
Documentation drift
Code Review Bot flagged 5 possible documentation drift(s) (partially analyzed). These are advisory.
README.md(info):The changes introduce a new 'Proxy Media' feature, including a new configuration setting 'PreviewSourceMode' to choose between proxy and original media for previews, but the README.md does not mention this feature.— suggested:Add a section to the 'Features' part of the README.md describing the proxy media functionality and the ability to switch between proxy and original sources for previews.README.md(info):The code changes introduce a new 'proxy indicator' feature (UI badge and logic for tracking proxy states like Ready, Generating, Stale, etc.), which is a significant functional addition not mentioned in the README.md features list.— suggested:Add a 'Proxy' or 'Performance' section to the Features list in README.md describing the ability to use and track the state of proxy files for improved editing performance.AGENTS.md(warning):A new project 'Beutl.Extensions.FFmpeg' was introduced in the code changes (seen in the namespace and file path), but it is not listed in the Module boundary map in AGENTS.md.— suggested:Add 'Beutl.Extensions.FFmpeg' to the Module boundary map in AGENTS.md with an appropriate role description.README.md(info):The code introduces a new 'Proxy media' feature with associated settings (store location, max size, and default preset), which is not mentioned in the README's Features section.— suggested:Add a new subsection to the Features section in README.md describing the Proxy Media functionality.README.md(info):The code introduces a new 'Proxies' feature (including proxy generation, quality settings, and preview source options), but the README.md 'Features' section does not mention this functionality.— suggested:Add a new section under 'Features' describing the proxy media system, allowing users to generate lower-resolution versions of their media for smoother editing and previewing.
Results are for commit e9da407. On newer commits, the bot's summary comment reflects the latest run.
|
Code review by qodo was updated up to the latest commit e9da407 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e9da407e42
ℹ️ 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".
~MediaReader calls Dispose(false), which skipped pin.Dispose(), so an abandoned (never-disposed) proxied reader left the resolver's pin count elevated forever and eviction could never reclaim that proxy file. PinHandle only touches Interlocked/ConcurrentDictionary state, so it is safe to release from the finalizer thread; inner stays disposing-only.
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
|
Code review by qodo was updated up to the latest commit 4c0f1ba |
IProxyResolver is a public seam, so the pin handle may be a third-party IDisposable with no no-throw guarantee; an exception escaping the MediaReader finalizer would terminate the process. The disposing path still propagates the failure.
|
Code review by qodo was updated up to the latest commit f95fb8f |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…lable catch Every other _generatorAvailability access synchronizes on _lock; the terminal-skip-vs-requeue decision read it bare, so a racing InvalidateGenerator could be observed stale. Synchronization-only change; both branches are already characterized by ProxyJobQueueTests.
|
Code review by qodo was updated up to the latest commit d64303e |
…ed mid-generate The unavailable-exception catch treated a null _generatorAvailability as 'no availability signal -> terminal skip', but InvalidateGenerator also nulls it, so a generator swap/unload racing an in-flight job silently dropped the queued work. Snapshot _generator alongside the availability under _lock and reserve the terminal skip for a still-cached generator without a signal; an invalidated generator requeues like the not-yet-registered path. Regression test verified red on the unfixed code.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Code review by qodo was updated up to the latest commit d94a30a |
…eue reason ProxyMediaReader: run base.Dispose(disposing) in an outer finally so an inner reader whose Dispose throws still completes the dispose chain (base is a no-op today; structural robustness), and cover the pin-release-on-inner-failure path with a test. ProxyJobQueue: set StatusMessage when requeueing on generator unavailability so the Proxies tab shows why a job is parked instead of a plain Queued entry; the next dispatch already clears it.
|
No TODO comments were found. |
Minimum allowed line rate is |
|
Code review by qodo was updated up to the latest commit 59cd3e9 |
Description
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
Test plan
dotnet build src/Beutl.Editor.Components/Beutl.Editor.Components.csproj --no-restoredotnet test tests/Beutl.UnitTests/Beutl.UnitTests.csproj -f net10.0 --no-restore(3,878 passed, 2 skipped)dotnet test tests/Beutl.FFmpegIpc.Tests/Beutl.FFmpegIpc.Tests.csproj -f net10.0 --no-restore(38 passed)Fixed issues / References
docs/specs/002-proxy-mediaSummary by CodeRabbit
New Features
Bug Fixes
Tests