Introduce pre-chain stage with area downsampling pass#60
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds an optional pre-chain downsampling stage to the render pipeline. A new DownsamplePass and area-filter shader run before the RetroArch passes when a source resolution is configured; FilterChain and backend/config/CLI are extended to create, record, and thread pre-chain passes and framebuffers into the main chain. Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant Backend as VulkanBackend
participant Chain as FilterChain
participant Pass as DownsamplePass
participant GPU as Vulkan GPU
App->>App: Parse CLI (--app-width/--app-height)
App->>Backend: init_filter_chain(source_resolution)
Backend->>Chain: create(..., source_resolution)
Chain->>Chain: store m_source_resolution
alt source_resolution non-zero
Chain->>Pass: create(DownsamplePass)
Pass->>GPU: Allocate sampler & descriptors
Pass->>GPU: Create pipeline layout & graphics pipeline
Chain->>Chain: add pass to m_prechain_passes
end
rect rgba(100,150,200,0.5)
Note over Chain,GPU: Per-frame rendering
App->>Chain: record(cmd_buffer, frame_index)
Chain->>Chain: ensure_prechain_passes(captured_extent)
Chain->>Chain: record_prechain(original_view, original_extent, frame_index)
Chain->>Pass: record(cmd, PassContext)
Pass->>GPU: Bind descriptor with source texture
Pass->>GPU: Push downsample constants
Pass->>GPU: Draw area-filtered downsample
Pass-->>Chain: Return downsampled image view/extent
Chain->>Chain: Use prechain output as original_view for RetroArch passes
Chain->>GPU: Record downstream passes with downsampled input
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
…downsample - Add m_prechain_passes/m_prechain_framebuffers vectors to FilterChain - Create DownsamplePass class with area filter algorithm - Propagate source_width/source_height through Config and RenderSettings - Update CLI semantics: --app-width/--app-height set filter chain input resolution - Support lazy single-dimension resolution with aspect-ratio preservation
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
|||||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||||||||||
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@openspec/changes/add-prechain-downsample/specs/render-pipeline/spec.md`:
- Around line 80-86: The scenario example's computed width is off by one
compared to the implementation: update the spec to match the code's rounding
behavior (std::round) by changing the expected width from 854 to 853 (since
round(480 * 1920 / 1080) = round(853.333...) = 853), or explicitly note that
implementations using std::round will produce 853; reference the implementation
in filter_chain.cpp and the use of std::round when making this change.
In `@src/render/chain/downsample_pass.cpp`:
- Around line 85-93: The code computes inverses for source and target dimensions
in DownsamplePushConstants
(pc.source_inv_width/pc.source_inv_height/pc.target_inv_width/pc.target_inv_height)
without checking for zero; add a defensive guard around ctx.source_extent and
ctx.output_extent width/height: if a dimension is zero (or extremely close to
zero) set the corresponding inverse to 0.0f (or 1.0f/epsilon) and/or
early-return/skip the downsample dispatch, otherwise compute 1.0f/width as
before; update all uses of pc.* to assume these safe values.
🧹 Nitpick comments (3)
shaders/internal/downsample.frag.slang (1)
38-41: Consider centering the sample window for extreme downsampling ratios.When the source region exceeds 8×8 pixels, the current approach truncates from the end (
i_max = i_min + sample_range), which may skip high-overlap pixels at the upper boundary. For extreme downsampling ratios, centering the 8×8 window on the box midpoint could improve quality:int max_samples = 8; int2 sample_range = min(i_max - i_min, int2(max_samples, max_samples)); - i_max = i_min + sample_range; + int2 excess = (i_max - i_min) - sample_range; + i_min = i_min + excess / 2; + i_max = i_min + sample_range;This is a minor quality improvement for edge cases with large scale factors.
src/app/main.cpp (1)
375-380: Consider clarifying the log message for single-dimension specification.When only one dimension is specified (e.g.,
--app-width 640), the log will showSource resolution: 640x0, which may confuse users. Consider logging only the specified dimension or indicating that the other will be computed:if (cli_opts.app_width != 0 || cli_opts.app_height != 0) { config.render.source_width = cli_opts.app_width; config.render.source_height = cli_opts.app_height; - GOGGLES_LOG_INFO("Source resolution: {}x{}", config.render.source_width, - config.render.source_height); + if (config.render.source_width != 0 && config.render.source_height != 0) { + GOGGLES_LOG_INFO("Source resolution: {}x{}", config.render.source_width, + config.render.source_height); + } else if (config.render.source_width != 0) { + GOGGLES_LOG_INFO("Source width: {} (height from aspect ratio)", + config.render.source_width); + } else { + GOGGLES_LOG_INFO("Source height: {} (width from aspect ratio)", + config.render.source_height); + } }This is a minor UX improvement for log clarity.
src/util/config.hpp (1)
67-68: Consider adding documentation comments for the new fields.The other fields in
Config::Renderare self-explanatory, but the semantics ofsource_width/source_height(pre-chain downsampling target, 0 = disabled) could benefit from brief documentation:uint32_t integer_scale = 0; + uint32_t source_width = 0; ///< Target width for pre-chain downsampling (0 = disabled) + uint32_t source_height = 0; ///< Target height for pre-chain downsampling (0 = disabled) - uint32_t source_width = 0; - uint32_t source_height = 0;
openspec/changes/add-prechain-downsample/specs/render-pipeline/spec.md
Outdated
Show resolved
Hide resolved
6189d83 to
7506faf
Compare
User description
Why
The
--app-width/--app-heightCLI options currently only work with WSI proxy mode. Users need a way to control the source resolution fed into the RetroArch filter chain regardless of capture mode. A pre-chain downsampling stage enables resolution control for shader effects that benefit from lower-res input (CRT simulation, pixel art upscalers) without requiring WSI proxy overhead.What Changes
downsample.frag.slangshader inshaders/internal/using area filtering for high-quality downsamplingPreChaininfrastructure inFilterChainto run internal passes before RetroArch shader passesDownsamplePassas the first pre-chain pass when--app-width/--app-heightare specifiedFilterPass/Framebufferinfrastructure for pre-chain passes--app-width/--app-heightsemantics: these options now set the source resolution for the filter chain input (applies to all capture modes, not just WSI proxy)Config::Capturefor use byFilterChainImpact
shaders/internal/downsample.frag.slang(new)src/render/chain/filter_chain.hpp- add pre-chain pass storagesrc/render/chain/filter_chain.cpp- implement pre-chain recordingsrc/app/cli.cpp- update option descriptionssrc/util/config.hpp- add source resolution fieldsPR Type
Enhancement
Description
Introduce extensible pre-chain pass infrastructure in FilterChain
Add DownsamplePass with area filtering for resolution control
Extend --app-width/--app-height to work in all capture modes
Support single-dimension specification with aspect-ratio preservation
Propagate source resolution through Config and RenderSettings
Diagram Walkthrough
File Walkthrough
11 files
New DownsamplePass class header definitionImplement area-filter downsampling pass logicAdd pre-chain pass and framebuffer vectorsImplement generic pre-chain recording infrastructureAdd source_resolution field to RenderSettingsPropagate source resolution through backend initializationPass source resolution to RenderSettingsUpdate CLI option descriptions and remove validationParse and propagate source resolution from CLIAdd source_width/height fields to Config::RenderNew area-filter downsampling shader implementation1 files
Update debug log prefix from Vulkan to VVL1 files
Add downsample_pass.cpp to build targets3 files
Document pre-chain and downsample feature proposalDefine requirements for pre-chain and downsampleTrack implementation tasks for pre-chain featureSummary by CodeRabbit
New Features
Tests
Chores
Style
✏️ Tip: You can customize this high-level summary in your review settings.