Vegas mode: reclaim dead space and pace the rotation - #423
Conversation
On a wide panel Vegas mode spent much of its time showing black. At 50px/s on a 512px display, one display width of blank is 10.2 seconds, which makes several long-standing behaviours expensive: - ScrollHelper prepended a full display width of black as an "initial gap", charged once per cycle — 10.2s of black at the start of every rotation. - Plugins without get_vegas_content() are captured off a full-display canvas, so their blank margins entered the ticker too. Measured: of-the-day drew 35px of "No Data" on a 512px canvas (92% blank), youtube-stats 142px of content with 185px of black either side. Only the scroll_helper path had any trimming. - Cycle transitions deliberately pushed a blank frame and then recomposed synchronously: 84ms at best, 4.8s at worst, every millisecond of it black. - buffer_ahead doubled as the cycle size, so a 21-plugin install showed 3 plugins per cycle and took ~7 cycles to come around. - separator_width was applied between every image rather than at plugin boundaries, so a per-row ticker like the F1 scoreboard (116 images, which it renders 4px apart internally) got a 32px chasm between each row — and the width budget didn't count those gaps, so the plugin quietly occupied far more of the panel than intended. Changes: - src/vegas_mode/geometry.py: numpy column-ink primitives shared by the trimmer and the audit tool, so the number reported is the number acted on. A Python per-column loop over a 17,000px strip is far too slow for the render path. - PluginAdapter trims every content path, not just scroll_helper. Only outer edges are cropped: interior blank columns are the plugin's own layout (logo left, score right) and closing them would corrupt the design. A plugin on a non-black background is inherently unaffected. - ScrollHelper.create_scrolling_image takes an explicit lead_gap, still defaulting to display_width so the many standalone-ticker callers are unchanged. Vegas passes lead_in_width (default 0). - Cycle end holds the last rendered frame instead of blanking, turning the recompose into a brief freeze rather than the panel switching off. - plugins_per_cycle (default 6) is split from buffer_ahead, which goes back to being only a prefetch low-water mark. - max_plugin_width_ratio (default 3x display width) caps one plugin's share of a cycle. Overflow is deferred, not discarded: a rotation offset advances each fetch so later rows appear on subsequent cycles. Single oversized images are cropped at a blank column so the cut misses glyphs. - Composition groups images by plugin: rows are joined by intra_plugin_gap (default 8) and separator_width applies only between plugins. The width budget now counts those gaps. - Plugin data updates no longer run on the Vegas render path. All new settings are user-configurable in Display -> Vegas Scroll, including min/max cycle duration and dynamic duration, which previously existed in code but were reachable only by hand-editing config.json. Measured with scripts/dev/vegas_audit.py on a 512x64 panel: mean ink coverage 42.7% -> 69.4% fully blank 5.9% -> 0% reads as empty 13.6% -> 0% worst blank stretch 4.8s -> 0s full rotation 414s -> 123s plugins per cycle 3 -> 6 Note the metric choice: a "fully blank" scan (>=95% black viewport) reported only 0.4% and badly understated the problem, because two full-width segments with mid-canvas content never fully blank the viewport — they hold it at ~28%. window_coverage_stats grades every viewport position by how much ink it carries, which is what tracks perceived dead time. Known remaining: cycle transitions still freeze ~3.5s while the next cycle is fetched. Fixing that needs background prefetch, which is deferred because the fallback-capture path mutates the shared display_manager.image and racing it against the render loop risks torn frames. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
|
Warning Review limit reached
Next review available in: 47 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughVegas Mode gains expanded configuration, content trimming, per-plugin width budgeting, grouped cycle composition, dead-space geometry metrics, an offline audit tool, updated buffering and transition behavior, and corresponding UI and test coverage. ChangesVegas density and scroll behavior
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant ConfigUI
participant API
participant VegasModeCoordinator
participant PluginAdapter
participant RenderPipeline
participant Display
ConfigUI->>API: submit Vegas scroll settings
API->>VegasModeCoordinator: apply updated configuration
VegasModeCoordinator->>PluginAdapter: replace config and invalidate cache
RenderPipeline->>PluginAdapter: request finalized plugin content
PluginAdapter-->>RenderPipeline: trimmed and budgeted content
RenderPipeline->>Display: render composed scrolling cycle
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 151 |
| Duplication | 4 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/vegas_mode/config.py (1)
74-74: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
max_cycle_durationdefault disagrees with the template and the new UI copy.Code default is 600,
config/config.template.jsonnow ships 240, and the new Max Cycle Time help text says "Default: 240". Users reading either surface get a different answer than a config without the key. Align the dataclass default (and template) on one value.🔧 Suggested alignment
- max_cycle_duration: int = 600 # Maximum seconds per full cycle + max_cycle_duration: int = 240 # Maximum seconds per full cycleAlso update
from_config()'smax_cycle_durationfallback to match.🤖 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/vegas_mode/config.py` at line 74, Align the max_cycle_duration default across the configuration dataclass, config template, and from_config() fallback, using 240 seconds to match the UI help text. Update the max_cycle_duration declaration and its fallback handling without changing other configuration behavior.
🧹 Nitpick comments (3)
src/vegas_mode/render_pipeline.py (1)
154-158: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnused loop variable flagged by Ruff (B007).
♻️ Rename to `_plugin_id`
- for plugin_id, images in grouped: + for _plugin_id, images in grouped: total_rows += len(images) blocks.append(self._join_plugin_rows(images))🤖 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/vegas_mode/render_pipeline.py` around lines 154 - 158, Rename the unused plugin_id loop variable in the grouped iteration within the render pipeline to _plugin_id, leaving total_rows accumulation and _join_plugin_rows processing unchanged.Source: Linters/SAST tools
src/common/scroll_helper.py (1)
268-273: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale invariant comments now that
lead_gapis configurable.Lines 268-272 (and the same claim at Line 329 and Lines 587-589) still state the image "already includes
display_widthpixels of blank padding at the start (added bycreate_scrolling_image)". Withlead_gap=0from Vegas mode that is no longer true. Behaviour is unaffected here, but the next reader will reason from a false premise — andcalculate_dynamic_duration()still adds a fulldisplay_widthof travel on top oftotal_scroll_widthfor the same reason.📝 Suggested wording
- # Calculate required total distance: total_scroll_width only. - # The image already includes display_width pixels of blank padding at the start - # (added by create_scrolling_image), so once scroll_position reaches - # total_scroll_width the last card has fully scrolled off the left edge. - # Adding display_width here would cause 1-2 extra wrap-arounds on wide chains. + # Calculate required total distance: total_scroll_width only. + # Any leading blank margin (lead_gap, display_width by default but 0 in + # Vegas mode) is already part of total_scroll_width, so once + # scroll_position reaches it the last card has scrolled off the left + # edge. Adding display_width here would cause 1-2 extra wrap-arounds.🤖 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/common/scroll_helper.py` around lines 268 - 273, Update the stale invariant comments near required_total_distance, the corresponding comment around line 329, and the comment near line 587 to reflect that the leading blank padding depends on configurable lead_gap and may be zero in Vegas mode. Also review calculate_dynamic_duration() and remove its unconditional extra display_width travel, preserving distance calculations based on the actual configured image padding and total_scroll_width.scripts/dev/vegas_audit.py (1)
124-136: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
join_rowsre-implementsRenderPipeline._join_plugin_rowsverbatim.The docstring itself says "matching RenderPipeline._join_plugin_rows" — that's an explicit coupling this audit tool depends on for accuracy, but nothing enforces it. If the production join logic changes (e.g. gap handling, height calc), this copy will silently drift and the audit will report numbers that no longer reflect production, undermining the tool's own purpose ("pulls each one's content through the real
PluginAdapter... composes... through the realScrollHelper").Consider extracting this into a small shared helper (e.g. in
src/vegas_mode/geometry.pyor a newrender_utils.py) that bothRenderPipelineand this script import.🤖 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 `@scripts/dev/vegas_audit.py` around lines 124 - 136, Extract the duplicated row-composition logic from join_rows into a shared production helper, then update both join_rows and RenderPipeline._join_plugin_rows to call it. Preserve the existing single-image behavior, nonnegative gap handling, RGB canvas creation, width/height calculations, and paste ordering so the audit uses the exact production implementation.
🤖 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 `@scripts/dev/vegas_audit.py`:
- Around line 208-211: Update the PluginAdapter construction in the audit setup
to pass the already loaded Vegas configuration as its second argument. Keep the
existing display_manager and manager initialization unchanged so trimming,
finalization, and width budgeting use the same config.json settings as
production.
In `@web_interface/blueprints/api_v3.py`:
- Around line 966-970: Update the numeric_fields entries for vegas_scroll_speed,
vegas_target_fps, and vegas_buffer_ahead to match the bounds enforced by
VegasModeConfig.validate() and the UI: allow scroll speed through 200, require
target FPS to start at 30, and cap buffer_ahead at 5. Leave the other numeric
field ranges unchanged.
---
Outside diff comments:
In `@src/vegas_mode/config.py`:
- Line 74: Align the max_cycle_duration default across the configuration
dataclass, config template, and from_config() fallback, using 240 seconds to
match the UI help text. Update the max_cycle_duration declaration and its
fallback handling without changing other configuration behavior.
---
Nitpick comments:
In `@scripts/dev/vegas_audit.py`:
- Around line 124-136: Extract the duplicated row-composition logic from
join_rows into a shared production helper, then update both join_rows and
RenderPipeline._join_plugin_rows to call it. Preserve the existing single-image
behavior, nonnegative gap handling, RGB canvas creation, width/height
calculations, and paste ordering so the audit uses the exact production
implementation.
In `@src/common/scroll_helper.py`:
- Around line 268-273: Update the stale invariant comments near
required_total_distance, the corresponding comment around line 329, and the
comment near line 587 to reflect that the leading blank padding depends on
configurable lead_gap and may be zero in Vegas mode. Also review
calculate_dynamic_duration() and remove its unconditional extra display_width
travel, preserving distance calculations based on the actual configured image
padding and total_scroll_width.
In `@src/vegas_mode/render_pipeline.py`:
- Around line 154-158: Rename the unused plugin_id loop variable in the grouped
iteration within the render pipeline to _plugin_id, leaving total_rows
accumulation and _join_plugin_rows processing unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 27075b10-e5c2-4d13-9756-2824bb26e483
📒 Files selected for processing (14)
config/config.template.jsonscripts/dev/vegas_audit.pysrc/common/scroll_helper.pysrc/plugin_system/testing/visual_display_manager.pysrc/vegas_mode/config.pysrc/vegas_mode/coordinator.pysrc/vegas_mode/geometry.pysrc/vegas_mode/plugin_adapter.pysrc/vegas_mode/render_pipeline.pysrc/vegas_mode/stream_manager.pytest/test_vegas_density.pytest/test_vegas_geometry.pyweb_interface/blueprints/api_v3.pyweb_interface/templates/v3/partials/display.html
| 'vegas_scroll_speed': ('scroll_speed', 1, 100), | ||
| 'vegas_separator_width': ('separator_width', 0, 500), | ||
| 'vegas_intra_plugin_gap': ('intra_plugin_gap', 0, 128), | ||
| 'vegas_target_fps': ('target_fps', 1, 200), | ||
| 'vegas_buffer_ahead': ('buffer_ahead', 1, 20), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validation ranges here disagree with VegasModeConfig.validate() and the UI.
Within the rewritten numeric_fields map:
vegas_scroll_speedcaps at 100, but the slider indisplay.htmlgoes to 200 andvalidate()allows up to 200 — a user saving 150 gets a 400.vegas_target_fpsaccepts 1–200 whilevalidate()requires ≥ 30;vegas_buffer_aheadaccepts 1–20 whilevalidate()caps at 5. Either accepts values that later makeVegasModeCoordinator.start()bail out on validation with only a log line.
The other new entries line up with validate(); these three should too.
🔧 Align the ranges
- 'vegas_scroll_speed': ('scroll_speed', 1, 100),
+ 'vegas_scroll_speed': ('scroll_speed', 1, 200),
'vegas_separator_width': ('separator_width', 0, 500),
'vegas_intra_plugin_gap': ('intra_plugin_gap', 0, 128),
- 'vegas_target_fps': ('target_fps', 1, 200),
- 'vegas_buffer_ahead': ('buffer_ahead', 1, 20),
+ 'vegas_target_fps': ('target_fps', 30, 200),
+ 'vegas_buffer_ahead': ('buffer_ahead', 1, 5),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 'vegas_scroll_speed': ('scroll_speed', 1, 100), | |
| 'vegas_separator_width': ('separator_width', 0, 500), | |
| 'vegas_intra_plugin_gap': ('intra_plugin_gap', 0, 128), | |
| 'vegas_target_fps': ('target_fps', 1, 200), | |
| 'vegas_buffer_ahead': ('buffer_ahead', 1, 20), | |
| 'vegas_scroll_speed': ('scroll_speed', 1, 200), | |
| 'vegas_separator_width': ('separator_width', 0, 500), | |
| 'vegas_intra_plugin_gap': ('intra_plugin_gap', 0, 128), | |
| 'vegas_target_fps': ('target_fps', 30, 200), | |
| 'vegas_buffer_ahead': ('buffer_ahead', 1, 5), |
🤖 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 `@web_interface/blueprints/api_v3.py` around lines 966 - 970, Update the
numeric_fields entries for vegas_scroll_speed, vegas_target_fps, and
vegas_buffer_ahead to match the bounds enforced by VegasModeConfig.validate()
and the UI: allow scroll speed through 200, require target FPS to start at 30,
and cap buffer_ahead at 5. Leave the other numeric field ranges unchanged.
Flagged by Codacy (F401). Any, Dict and List are all still used. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
Both from review feedback on #423. The web API's accepted ranges disagreed with VegasModeConfig.validate(), which is what actually gates Vegas starting: scroll_speed 1-100 -> 1-200 (a slider value of 150 returned 400) separator_width 0-500 -> 0-128 target_fps 1-200 -> 30-200 buffer_ahead 1-20 -> 1-5 The three loose ones were the dangerous direction: the value saved with a 200, then VegasModeCoordinator.start() failed validation with only a log line, so the ticker silently never ran. The UI already matched validate() in all four cases, so the API was the odd one out. test_vegas_api_bounds_match_validate parses the numeric_fields map out of api_v3 and asserts every bound against validate(), plus that validate() accepts both endpoints and rejects just outside them, so these cannot drift apart again. That test immediately caught a missing upper bound on min_plugin_width, now added — unbounded it would drop every segment and leave a blank ticker. Separately, vegas_audit.py constructed PluginAdapter without the config, so it fell back to VegasModeConfig() defaults and would report trimming and width-budget behaviour that differed from the user's config.json. It now passes the loaded config exactly as the coordinator does. This is the same class of drift the explicit lead_gap and grouping arguments already guard against. Output is unchanged on a rig whose config matches the defaults. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
|
Thanks — both findings were real and are fixed in d69dfbb. Validation range mismatches. Confirmed, and there was a fourth you didn't flag:
You correctly identified the asymmetry in how these fail. The loose ones are the worse direction: the value saved with a 200, then Added Audit config plumbing. Also correct. |
1 participant
Problem
On a wide panel Vegas mode spent much of its time showing black. The conversion that makes this concrete: at 50px/s on a 512px display, one display width of blank is 10.2 seconds.
Measured on a 512×64 rig before any changes:
Five root causes, each verified against the running service:
ScrollHelper.create_scrolling_imageunconditionally prependeddisplay_widthas an "initial gap". Only the multi-display-sync path skipped it, so with sync off nothing did — 10.2s of black per rotation.get_vegas_content()are captured off a display-sized canvas.of-the-daydrew 35px of "No Data" on a 512px canvas (92% blank);youtube-statshad 142px of content with 185px of black either side. A trim helper existed but ran only on thescroll_helperpath.plugin.update_data()network calls on the render path. Measured 84ms at best, 4.8s at worst, all of it black.buffer_aheaddoubled as the cycle size. A 21-plugin install showed 3 plugins per cycle, ~7 cycles to come around.separator_widthwas applied between every image, not at plugin boundaries. The F1 scoreboard returns 116 images (one per standings row) which it renders 4px apart internally; Vegas forced 32px between each. The width budget also didn't count those gaps, so the plugin occupied far more of the panel than its budget allowed.Result
Live examples from the rig:
of-the-day512px → 65px (87% reclaimed),countdown512px → 87px,ledmatrix-stocks7360px capped to a rotating 1536px window,f1-scoreboard11 of 116 rows per cycle with the rest deferred. Plugins that genuinely fill the screen (odds-ticker,tide-display) were correctly left untouched.Approach
src/vegas_mode/geometry.py— numpy column-ink primitives shared by the trimmer and the audit tool, so the number reported is the number acted on. A Python per-column loop over a 17,000px strip is far too slow for the render path.create_scrolling_imagegains an explicitlead_gap, still defaulting todisplay_widthso the dozen-plus standalone-ticker callers behave identically. Only Vegas passeslead_in_width(default 0). Chosen over resettingscroll_position, which would leavetotal_scroll_widthoverstated and need matching fixes to the cycle-complete threshold.intra_plugin_gap(default 8) into one block, soScrollHelpersees one item per plugin andseparator_widthlands only at handoffs.Configurability
Every new knob is exposed in Display → Vegas Scroll, plus
min_cycle_duration,max_cycle_durationanddynamic_duration_enabledwhich already existed in code but were reachable only by hand-editingconfig.json. Save and validation verified end-to-end (200 persisting, 400 on out-of-range).On the metric
Worth flagging for reviewers: my first metric was a "fully blank" scan (≥95% black viewport). It reported 0.4% and badly understated the problem, because two full-width segments with mid-canvas content never fully blank the viewport — they hold it at ~28%, which still reads as an empty panel.
window_coverage_statsgrades every viewport position by how much ink it carries; that is the number that tracks perceived dead time. Both live ingeometry.pywith tests pinning the distinction.Testing
test_vegas_geometry.pyandtest_vegas_density.py, asserting exact column layouts (row row [8px] row row [32px] row row) rather than just totals.test_display_dirty_tracking,test_web_api::test_get_system_status, and two intest_state_reconciliation.scripts/dev/vegas_audit.pyis included as a repeatable tool; it drives the realPluginAdapterandScrollHelperand produced every number above. It runs off-hardware, so it is safe alongside a live display.Known remaining
Cycle transitions still freeze ~3.5s while the next cycle's content is fetched. Fixing that needs background prefetch, deliberately deferred: the fallback-capture path mutates the shared
display_manager.image, and racing that against the render loop risks torn frames and visible flashes. The contained version backgrounds only the native paths (where nearly all the time is spent) and is better reviewed separately.🤖 Generated with Claude Code
https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
Summary by CodeRabbit
New Features
Bug Fixes