feat(viewer): UI-driven screen rotation for portrait/landscape#2882
Merged
Conversation
…x86) Issue #2856. Adds a "Screen rotation" dropdown (0/90/180/270) to the Settings page and plumbs it through the viewer playback layer so the same setting drives both Pi (Qt linuxfb) and x86 (cage/wayland) targets. On Pi the viewer bakes `:rotation=N` into QT_QPA_PLATFORM when it spawns AnthiasWebview; on x86 it pushes the wlroots transform via wlr-randr. mpv picks up rotation through `--video-rotate` only on Pi (x86 inherits the compositor transform); VLC on the SD Pi boards gets the transform video filter. Changing the dropdown publishes the existing `reload` pub/sub message, which now also re-applies rotation live. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The rotation tests patched ``viewer.settings`` which mypy flags as ``Module "anthias_viewer" does not explicitly export attribute "settings"`` — ``settings`` is re-imported into the viewer module via ``from … import settings`` (not ``import settings as settings``), and mypy treats only the explicit-alias form as a re-export. Importing directly from the source keeps the test honest about where the symbol lives. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds a persisted screen rotation device setting (0/90/180/270) surfaced in the Settings UI and propagated through the viewer so rotation is applied appropriately on both linuxfb (Pi) and Wayland/cage (x86) targets.
Changes:
- Adds
screen_rotationto server settings defaults, Settings page UI, page context, and v2 device settings API (GET + PATCH validation). - Implements viewer-side rotation application: linuxfb via
QT_QPA_PLATFORM=linuxfb:rotation=N(webview restart on change) and x86 viawlr-randr --transform(live reapply on reload). - Updates media playback to respect rotation where needed (mpv
--video-rotateon non-x86; VLC transform filter on Pi 1/2/3) and adds unit tests for the new behavior.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/image_builder/utils.py | Adds wlr-randr to x86 viewer image dependencies for compositor-level transforms. |
| src/anthias_server/settings.py | Introduces screen_rotation default setting. |
| src/anthias_server/app/views.py | Validates/clamps screen_rotation from Settings form POST and triggers viewer reload. |
| src/anthias_server/app/templates/settings.html | Adds “Screen rotation” dropdown to Settings UI. |
| src/anthias_server/app/page_context.py | Exposes screen_rotation to the Settings template context. |
| src/anthias_server/api/serializers/v2.py | Adds screen_rotation to device settings schema and enforces cardinal-angle choices on PATCH. |
| src/anthias_server/api/views/v2.py | Includes screen_rotation in GET response and persists it on PATCH. |
| src/anthias_viewer/init.py | Applies rotation at startup and on reload; linuxfb uses QPA env + restart, x86 uses wlr-randr. |
| src/anthias_viewer/media_player.py | Adds rotation-aware mpv/VLC flags/filters and a MediaPlayerProxy reset hook. |
| tests/test_viewer.py | Adds unit tests covering rotation clamping, env building, wlr-randr invocation, and reload behavior. |
| tests/test_media_player.py | Adds unit tests ensuring mpv rotation args are applied/skipped correctly and MediaPlayerProxy.reset works. |
| tests/test_template_views.py | Extends settings template context tests and adds Settings POST round-trip for rotation. |
| src/anthias_server/api/tests/test_v2_endpoints.py | Extends API tests for device settings GET/PATCH coverage of rotation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Three concerns raised on PR #2882: 1. _apply_wlr_transform() blanket-logged "Applied" even when wlr-randr exited non-zero. cage may not be up yet at viewer startup, an EDID-renamed output can vanish between list and apply, or wlroots can reject the transform for an unsupported output — surface stderr on failure so a silently-broken rotation is debuggable. 2. The linuxfb rotation-change path terminated AnthiasWebview but didn't update _last_applied_rotation until load_browser() respawned it. A second `reload` arriving in the gap would treat rotation as still-changed and re-fire terminate()/skip_event on the dying process. Latch the new value immediately; load_browser() still re-reads and re-assigns it after the respawn so the variable stays accurate. 3. test_mpv_skips_video_rotate_on_x86 patched get_device_type() to 'pi5' while DEVICE_TYPE=x86 — get_alsa_audio_device() routes through get_device_type(), so the test would call into _detect_hdmi_audio_device() and stat /sys/class/drm on the host. Patch to 'x86' so the test is deterministic. Adds coverage for the wlr-randr non-zero-exit warning path and a repeat-reload test that guards against re-firing terminate() on a dying webview. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three concerns from Copilot's second review of PR #2882: 1. Thread safety: _maybe_reapply_rotation() runs on the ViewerSubscriber background thread but the linuxfb branch directly called browser.terminate() and mutated current_browser_url — both owned by the main asset_loop thread and used mid-D-Bus by view_image()/view_webpage(). Replaced with a flag-based handoff: the subscriber now only latches _last_applied_rotation and sets _rotation_bounce_pending; asset_loop calls _consume_pending_rotation_bounce() at the top of each tick to actually terminate the browser and clear current_browser_url on the main thread. 2. Wayland latch on failure: _apply_wlr_transform() returned None and the caller latched _last_applied_rotation unconditionally. If cage wasn't ready yet (no outputs listed) or every wlr-randr invocation failed, we'd stick at the unrotated state forever until the user changed the setting. Returns bool now (true = at least one output rotated); the Wayland path in _maybe_reapply_rotation only latches on success, otherwise the next reload retries. load_browser() uses a -1 sentinel to mark "unsuccessful first apply" so the next reload also retries. 3. Serializer asymmetry: DeviceSettingsSerializerV2 declared screen_rotation as an unconstrained IntegerField while UpdateDeviceSettingsSerializerV2 used ChoiceField. The OpenAPI schema now advertises the same enum on both directions. Adds tests for the new thread-safe handoff (subscriber sets flag but doesn't terminate; main thread consumes flag; asset_loop runs the consume helper before each tick) and the wlr-randr retry-on-failure path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two more concerns from Copilot's third review of PR #2882: 1. _build_webview_env() did ``qpa.split(':', 1)[0]`` to strip the rotation= suffix, which also discarded *every other* QPA option an operator might have set (``linuxfb:fb=/dev/fb1,tty=/dev/tty1`` would lose both fb and tty). And it only ran when rotation != 0, so dialing rotation back to 0 left a stale rotation=N from the previous launch in place. Replaced with _set_qpa_rotation() which parses the comma-separated option list, removes any preexisting rotation= entry, and re-appends only when rotation > 0 — invoked unconditionally so the 0° case also clears stale state. 2. _apply_wlr_transform()'s docstring claimed the non-Wayland branch "cannot happen here" while the implementation explicitly returns True there (tests rely on that behaviour). Rewrote the docstring to enumerate the three return cases accurately. Adds tests for the option-parser: preserving unrelated QPA options through a rotation change, and removing a stale rotation= when the operator dials back to 0°. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two more concerns from Copilot's fourth review of PR #2882: 1. If wlr-randr failed during load_browser() (e.g. cage's wayland socket not yet up at viewer startup), _last_applied_rotation latched to -1 and nothing retried until a server-side `reload` arrived. On an idle system that might never happen, leaving the display unrotated indefinitely. Added _retry_wayland_rotation_if_ pending(), called from asset_loop on every tick. Cheap when already applied (single int comparison); retries the apply when _last_applied_rotation != _rotation_value(). Linuxfb is skipped — its env-var path is synchronous and can't fail half-applied. 2. _wlr_output_names() returned every connector wlr-randr listed, including ones with `Enabled: no`. _apply_wlr_transform() then tried to rotate them, generating wlr-randr warnings on the journal that obscure real failures. Parser now tracks each block's `Enabled:` line and drops disabled outputs (with a conservative implicit-enabled default for wlr-randr versions that don't print the Enabled: field). Adds tests for the disabled-output filter and the four states of the retry helper (already-applied, sentinel-then-success, sentinel- stays-on-failure, no-op on linuxfb), plus pins the asset_loop contract so future refactors don't drop the retry call. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/anthias_viewer/init.py:633
_retry_wayland_rotation_if_pending()’s docstring describes a sentinel-1-based retry mechanism, but the implementation retries on any mismatch between_rotation_value()and_last_applied_rotation(not just when_last_applied_rotation == -1). Please align the docstring with the actual behavior, or gate retries on an explicit "pending" state to match the documentation.
def _retry_wayland_rotation_if_pending() -> None:
"""Main-thread retry for an unsuccessful Wayland rotation apply.
load_browser() at viewer startup tries to push the wlr-randr
transform before AnthiasWebview spawns, but cage may not have
fully come up at that point — its wayland socket can be missing
or its compositor not yet listing outputs. The first apply
returns False, load_browser() latches ``_last_applied_rotation
= -1`` (sentinel for "needs retry"), and without this helper the
display would stay unrotated until the operator next changed the
setting and triggered a `reload` (Copilot review of #2882).
Called from asset_loop on every tick. The early-return guard
means once the rotation has actually taken effect we drop back
to zero overhead. Linuxfb is unaffected (env-var path is
synchronous at QPA init, so it can't fail half-applied).
"""
if not _is_wayland_board():
return
global _last_applied_rotation
rotation = _rotation_value()
if rotation == _last_applied_rotation:
return
if _apply_wlr_transform(rotation):
_last_applied_rotation = rotation
Copilot review of #2882: the failure log said "will retry on the next reload" but the retry actually also fires from asset_loop via _retry_wayland_rotation_if_pending() on every tick whenever the in-memory latch disagrees with the on-disk setting. Updated both the log message and the corresponding comment in load_browser() so operators reading the journal aren't misled about when recovery will happen. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copilot's sixth review of PR #2882 flagged that the four cardinal angles (0/90/180/270) and the int-coerce-then-validate logic lived in four places — v2 serializer, form save handler, viewer's _rotation_value(), and media_player's _screen_rotation() — and could drift over time (e.g. when adding flipped transforms later). Moved the constant + a shared clamp_screen_rotation() helper to anthias_common.utils. The viewer and media_player wrappers shrink to a one-line settings-dict lookup + delegation; the v2 serializer imports SCREEN_ROTATION_CHOICES instead of redeclaring it; the HTML form save replaces a try/except/in-tuple block with a single helper call. Allowed set is now declared exactly once. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three more concerns from Copilot's seventh review of PR #2882: 1. MediaPlayerProxy.reset() ran on every rotation change, including Wayland. On x86 mpv's --vo=gpu --gpu-context=wayland VO inherits the compositor transform from cage automatically — so the in- flight video doesn't need to be restarted. reset() killed mpv mid-play and view_video() sat blocked on the asset's original duration with the screen on the 'null' black image (no skip_event on the Wayland branch). Gate reset() on _is_wayland_board()==False. 2. v2 API GET returned int(settings['screen_rotation']) which doesn't clamp — the OpenAPI schema advertises an enum {0,90,180,270}, but a stale 45 on disk would slip through. Route through the shared clamp_screen_rotation() helper so the response always matches the schema. 3. Same issue in page_context.device_settings(): the dropdown's {% if screen_rotation == N %} ladder picked nothing for an out- of-set value, leaving the Settings page in an inconsistent state vs the viewer's runtime clamp. Same helper, same fix. Adds tests pinning that the Wayland rotation-change path does NOT call MediaPlayerProxy.reset() and that the linuxfb path does. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
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.
Issues Fixed
Closes #2856 (consolidates #2693, #2326, #1997, #1545).
Description
Adds a Screen rotation dropdown (0 / 90 / 180 / 270°) to the Settings page and plumbs it through the viewer playback layer so a single setting drives every supported target:
QT_QPA_PLATFORM=linuxfb:rotation=N. The Qt plugin rotates the framebuffer natively (no perf cost). mpv on Pi adds--video-rotate=N; VLC on SD Pi boards adds--video-filter=transform.wlr-randr --output … --transform N. Qt's wayland QPA and mpv's wayland VO both inherit the compositor transform; no per-process flag needed.Changing the dropdown reuses the existing
reloadpub/sub. On x86 the transform is re-pushed live; on Pi we bounce AnthiasWebview so the new env takes effect on the nextasset_looptick.MediaPlayerProxy.reset()drops the cached VLC instance so the next video re-inits with the new transform filter.wlr-randris added to the x86 apt list (tools/image_builder/utils.py).Checklist
🤖 Generated with Claude Code