From def6c0e59052c4ae88fea800e64f56c3c59e6815 Mon Sep 17 00:00:00 2001 From: Oliver Date: Sat, 5 Sep 2026 00:24:45 +0200 Subject: [PATCH 1/5] fix: bound Windows playback reveal --- docs/WINDOWS_SCAN_PLAYBACK_DIAGNOSTICS.md | 9 +- docs/architecture.md | 6 + .../DETAIL_OPEN_BENCHMARK_RUNBOOK.md | 8 + .../desktop_coordinator_runtime.py | 5 + src/iPhoto/gui/main.py | 7 + .../ui/controllers/player_view_controller.py | 242 +++++++++++++++++- .../ui/widgets/gl_image_viewer/__init__.py | 4 +- .../gui/ui/widgets/gl_image_viewer/widget.py | 60 +++++ tests/gui/test_startup_import_boundary.py | 29 +++ .../test_player_view_init_cover.py | 191 ++++++++++++++ 10 files changed, 551 insertions(+), 10 deletions(-) diff --git a/docs/WINDOWS_SCAN_PLAYBACK_DIAGNOSTICS.md b/docs/WINDOWS_SCAN_PLAYBACK_DIAGNOSTICS.md index b12aba476..3dc82a20b 100644 --- a/docs/WINDOWS_SCAN_PLAYBACK_DIAGNOSTICS.md +++ b/docs/WINDOWS_SCAN_PLAYBACK_DIAGNOSTICS.md @@ -75,8 +75,13 @@ failed graphics contract, not a successful reproduction run. The extra active-surface `frameSubmitted` is a QRhi submission heuristic, not a DXGI/DWM presentation fence. It verifies the application state machine and an -additional Qt composition submission; only the real Windows repetition matrix -can determine whether it fixes the user-visible first-frame leak. +additional Qt composition submission. The reveal wait is bounded: after the +matching current-generation media submission, a missing extra composition may +end with `post_submit_deadline` followed by `surface_revealed` with +`reason=deadline`. A stale transition must instead record +`post_submit_discarded` and must not reveal its media. Only the real Windows +repetition matrix can determine whether this preserves the user-visible +first-frame leak fix. The default timeout is 30 minutes. Override it with `-MaxMinutes 60` if the scan takes longer. The expanded directory is retained beside the ZIP so its contents can be reviewed before diff --git a/docs/architecture.md b/docs/architecture.md index 30ac214b6..de2063267 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -225,6 +225,12 @@ macOS, and Linux share one QRhi lifecycle: the final native surface hierarchy is prepared pre-show, while `ensure_feature("detail")` completes its non-native UI and QtMultimedia runtime post-paint. Platform allowlists, post-show surface creation, parentless surface warm-up, and hide/show workarounds are forbidden. +Windows OpenGL Python-module warm-up is enabled only after startup reaches its +terminal state and is triggered by a later Detail hover/click; it is never a +startup prerequisite. QRhi/context creation and GPU resource allocation remain +on the active Detail surface. Windows reveal waits for the additional QRhi +submission heuristic, but that wait is bounded and may fail open only after the +matching current-generation media content was already submitted. The pre-show hierarchy is part of shell construction: failure is terminal and non-recoverable for that process. Startup-generation retry applies only after a valid visible shell exists; it must not claim to reconstruct the native diff --git a/docs/requirements/DETAIL_OPEN_BENCHMARK_RUNBOOK.md b/docs/requirements/DETAIL_OPEN_BENCHMARK_RUNBOOK.md index f16ee5e9e..9f4a5a2d2 100644 --- a/docs/requirements/DETAIL_OPEN_BENCHMARK_RUNBOOK.md +++ b/docs/requirements/DETAIL_OPEN_BENCHMARK_RUNBOOK.md @@ -58,7 +58,15 @@ Phase 2 still 采样必须同时保留以下事件，并按 `asset_id + generati - `decode_fallback`：统计 `imageio_to_qt`、`wic_to_qt`、`pillow`、`qt_full_scale`、`half`、`full`、`full_level`；没有 fallback 的样本也要计入分母。 - `surface_ready`：核对最终 detached surface 的宽高与 decode level。 +- Windows/OpenGL 冷首开同时保留 `gl_runtime_preloaded`、 + `image_surface_init_requested`、`qrhi_initialize_started/finished` 与 + `post_submit_scheduled/armed`。模块预热只能由 startup terminal 之后的 + hover/click 触发；不得出现在无用户交互的 startup profile 中。 - `presented`：仍是 click-to-present 的终点；stale generation 不得产生该事件。 +- 正常 Windows reveal 记录 `surface_revealed(reason=composition)`；缺少额外 + composition 时允许在匹配内容已提交后记录 + `post_submit_deadline`、`surface_revealed(reason=deadline)`。过期 epoch、 + generation、source 或 surface 只能产生 `post_submit_discarded`，不得 reveal。 - RAW 冷解码同时保留 `raw_probe`、`raw_candidate_selected`、`raw_thumb_decode`、 `raw_postprocess`、`raw_surface_convert` 和 `color_stats`。未知几何必须先由 `raw_probe` 修复后再产生 `level_selected`；每个 cache miss 只能选择 embedded、half、full 之一，不得在同一请求中连续执行 diff --git a/src/iPhoto/gui/coordinators/desktop_coordinator_runtime.py b/src/iPhoto/gui/coordinators/desktop_coordinator_runtime.py index f2f437130..32f9fb4e6 100644 --- a/src/iPhoto/gui/coordinators/desktop_coordinator_runtime.py +++ b/src/iPhoto/gui/coordinators/desktop_coordinator_runtime.py @@ -421,6 +421,11 @@ def start(self): self._theme_controller.apply_current_theme() self._view_router.show_gallery() + def enable_detail_interaction_warmup(self) -> None: + """Enable demand-driven Detail imports after startup is complete.""" + + self._player_view_controller.enable_interaction_warmup() + # ------------------------------------------------------------------ # Lazy Edit lifecycle used by the Detail immersive port # ------------------------------------------------------------------ diff --git a/src/iPhoto/gui/main.py b/src/iPhoto/gui/main.py index 0f818b865..ab36a9062 100644 --- a/src/iPhoto/gui/main.py +++ b/src/iPhoto/gui/main.py @@ -981,6 +981,13 @@ def _run_idle_startup_jobs() -> None: _logger.info("Starting deferred startup scan") starter() startup.complete() + enable_detail_warmup = getattr( + coordinator_runtime, + "enable_detail_interaction_warmup", + None, + ) + if callable(enable_detail_warmup): + enable_detail_warmup() def _enqueue_idle_startup_jobs() -> None: nonlocal startup_scan_enqueued diff --git a/src/iPhoto/gui/ui/controllers/player_view_controller.py b/src/iPhoto/gui/ui/controllers/player_view_controller.py index 01b6fa449..ab86755a8 100644 --- a/src/iPhoto/gui/ui/controllers/player_view_controller.py +++ b/src/iPhoto/gui/ui/controllers/player_view_controller.py @@ -50,7 +50,7 @@ from ....gui.detail_surface_cache import CachedStillDecodeBackend from ....gui.detail_surface_residency import SurfaceResidencyTracker from ....gui.i18n import tr -from ..widgets.gl_image_viewer import GLImageViewer +from ..widgets.gl_image_viewer import GLImageViewer, preload_opengl_python_runtime from ..widgets.live_badge import LiveBadge from ..widgets.video_area import VideoArea @@ -201,6 +201,20 @@ def run(self) -> None: # pragma: no cover - executed on a worker thread self._signals.finished.emit(self) +class _RenderRuntimeWarmupWorker(QRunnable): + """Import the Windows OpenGL Python runtime after user interaction.""" + + def run(self) -> None: # pragma: no cover - executed on a worker thread + try: + preload_opengl_python_runtime() + except Exception as exc: # noqa: BLE001 - best-effort import warm-up + emit_detail_event( + "gl_runtime_preload_failed", + generation=0, + error_type=type(exc).__name__, + ) + + class _AdjustmentPreparationSignals(QObject): started = Signal(object) ready = Signal(object, object) @@ -348,6 +362,8 @@ class PlayerViewController(QObject): stillFramePresented = Signal(object, int) """Emitted after the requested viewport surface is presented.""" + _POST_SUBMIT_REVEAL_DEADLINE_MS = 250 + def __init__( self, player_stack: QStackedWidget, @@ -413,6 +429,8 @@ def __init__( self._preparation_entry_by_worker: dict[int, _PreparationEntry] = {} self._preparation_prefetch_queue: list[_PreparedRequestIntent] = [] self._preparation_shutting_down = False + self._interaction_warmup_enabled = False + self._render_runtime_warmup_requested = False self._raw_source_probe_cache: OrderedDict[ tuple, AssetSourceIdentity, @@ -481,6 +499,7 @@ def __init__( self._post_submit_kind: Literal["image", "video"] | None = None self._post_submit_payload: tuple[object, ...] = () self._post_submit_armed = False + self._post_submit_deadline_timer: QTimer | None = None self._image_viewer.firstFrameReady.connect(self._on_image_first_render) self._video_area.firstFrameReady.connect(self._on_video_first_render) @@ -525,6 +544,7 @@ def _default_placeholder_text(self) -> str | None: def _advance_surface_transition_epoch(self) -> int: """Invalidate delayed cover releases owned by an older transition.""" + self._cancel_post_submit_deadline() self._surface_transition_epoch += 1 self._post_submit_epoch = None self._post_submit_kind = None @@ -532,6 +552,21 @@ def _advance_surface_transition_epoch(self) -> int: self._post_submit_armed = False return self._surface_transition_epoch + def _cancel_post_submit_deadline(self) -> None: + timer = self._post_submit_deadline_timer + if timer is not None: + timer.stop() + + def _start_post_submit_deadline(self) -> None: + timer = self._post_submit_deadline_timer + if timer is None: + timer = QTimer(self) + timer.setSingleShot(True) + timer.setInterval(self._POST_SUBMIT_REVEAL_DEADLINE_MS) + timer.timeout.connect(self._on_post_submit_deadline) + self._post_submit_deadline_timer = timer + timer.start() + def _schedule_post_submit_release( self, kind: Literal["image", "video"], @@ -553,6 +588,12 @@ def _schedule_post_submit_release( self._post_submit_kind = kind self._post_submit_payload = candidate_payload self._post_submit_armed = False + emit_detail_event( + "post_submit_scheduled", + generation=self._post_submit_generation(kind, candidate_payload), + kind=kind, + epoch=epoch, + ) QTimer.singleShot(0, lambda: self._arm_post_submit_release(epoch, kind)) def _arm_post_submit_release( @@ -572,6 +613,13 @@ def _arm_post_submit_release( if self._player_stack.currentWidget() is not self._image_viewer: return self._post_submit_armed = True + self._start_post_submit_deadline() + emit_detail_event( + "post_submit_armed", + generation=self._post_submit_generation(kind, self._post_submit_payload), + kind=kind, + epoch=epoch, + ) self._image_viewer.update() return if self._player_stack.currentWidget() is not self._video_area: @@ -579,8 +627,28 @@ def _arm_post_submit_release( request_update = getattr(self._video_area, "request_active_surface_update", None) if callable(request_update): self._post_submit_armed = True + self._start_post_submit_deadline() + emit_detail_event( + "post_submit_armed", + generation=self._post_submit_generation(kind, self._post_submit_payload), + kind=kind, + epoch=epoch, + ) request_update() + @staticmethod + def _post_submit_generation( + kind: Literal["image", "video"], + payload: tuple[object, ...], + ) -> int: + if len(payload) != 2: + return 0 + value = payload[1] if kind == "image" else payload[0] + try: + return int(value) + except (TypeError, ValueError): + return 0 + def _peek_post_submit_payload( self, kind: Literal["image", "video"], @@ -608,8 +676,75 @@ def _consume_post_submit_payload( self._post_submit_kind = None self._post_submit_payload = () self._post_submit_armed = False + self._cancel_post_submit_deadline() return True + def _on_post_submit_deadline(self) -> None: + """Fail open only for content already submitted by the current surface.""" + + kind = self._post_submit_kind + if kind not in {"image", "video"}: + return + payload = self._peek_post_submit_payload(kind) + if payload is None or len(payload) != 2: + return + generation = self._post_submit_generation(kind, payload) + emit_detail_event( + "post_submit_deadline", + generation=generation, + kind=kind, + epoch=self._surface_transition_epoch, + ) + if kind == "image": + source = payload[0] + invalid = ( + generation != self._pending_image_generation + or source != self._pending_image_key + or self._player_stack.currentWidget() is not self._image_viewer + ) + if invalid: + self._discard_post_submit_payload("image", payload, generation) + return + if not self._consume_post_submit_payload("image", payload): + return + self._finalize_image_surface_submission( + source, + generation, + reveal_reason="deadline", + ) + return + + content_serial = int(payload[1]) + required_serial = self._pending_video_content_serial + invalid = ( + generation != self._pending_video_generation + or (required_serial is not None and content_serial < required_serial) + or self._player_stack.currentWidget() is not self._video_area + ) + if invalid: + self._discard_post_submit_payload("video", payload, generation) + return + if not self._consume_post_submit_payload("video", payload): + return + self._finalize_video_surface_submission(reveal_reason="deadline") + + def _discard_post_submit_payload( + self, + kind: Literal["image", "video"], + payload: tuple[object, ...], + generation: int, + ) -> None: + """Terminate an expired stale barrier without revealing its content.""" + + if not self._consume_post_submit_payload(kind, payload): + return + emit_detail_event( + "post_submit_discarded", + generation=generation, + kind=kind, + epoch=self._surface_transition_epoch, + ) + def _on_image_first_render(self) -> None: """Mark image viewer as initialised; hide cover if it is visible.""" self._image_viewer_rendered = True @@ -687,14 +822,25 @@ def _on_video_surface_frame_submitted( return self._finalize_video_surface_submission() - def _finalize_video_surface_submission(self) -> None: + def _finalize_video_surface_submission( + self, + *, + reveal_reason: str = "content_submission", + ) -> None: """Reveal video after all platform presentation barriers are satisfied.""" + generation = self._pending_video_generation or 0 self._pending_video_generation = None self._pending_video_content_serial = None self._video_renderer_rendered = True self._configure_video_controls(self._video_interactive_when_ready) self._sync_detail_surface_cover() + emit_detail_event( + "surface_revealed", + generation=generation, + kind="video", + reason=reveal_reason, + ) def _on_video_composition_submitted(self) -> None: payload = self._peek_post_submit_payload("video") @@ -710,7 +856,7 @@ def _on_video_composition_submitted(self) -> None: return if not self._consume_post_submit_payload("video", payload): return - self._finalize_video_surface_submission() + self._finalize_video_surface_submission(reveal_reason="composition") def _sync_detail_surface_cover(self) -> None: """Show the cover while the current surface has an unsatisfied barrier.""" @@ -802,6 +948,25 @@ def show_image_surface(self) -> None: if self._image_viewer_rendered: self._sync_detail_surface_cover() + def _begin_image_transition(self, generation: int) -> None: + """Expose the still QRhi surface under the cover while decode runs.""" + + self._advance_surface_transition_epoch() + self._pending_image_generation = int(generation) + self._pending_image_key = None + self._pending_video_generation = None + self._pending_video_content_serial = None + self._show_detail_init_cover() + if self._player_stack.currentWidget() is not self._image_viewer: + self._player_stack.setCurrentWidget(self._image_viewer) + if not self._player_stack.isVisible(): + self._player_stack.show() + self._image_viewer.update() + emit_detail_event( + "image_surface_init_requested", + generation=int(generation), + ) + def _configure_video_controls(self, interactive: bool) -> None: self._video_area.set_controls_enabled(interactive) if interactive: @@ -870,7 +1035,14 @@ def display_image( self._loading_source = source self._loading_started_at = time.perf_counter() - self.show_placeholder("") + schedule_runtime_warmup = getattr( + self, + "_schedule_render_runtime_warmup", + None, + ) + if callable(schedule_runtime_warmup): + schedule_runtime_warmup() + self._begin_image_transition(request_generation) emit_detail_event( "decode_started", generation=request_generation, @@ -888,6 +1060,7 @@ def display_image( if not scheduled: self._loading_source = None self._loading_started_at = None + self.show_placeholder("") return scheduled def _cancel_stale_image_workers(self) -> None: @@ -913,6 +1086,14 @@ def prefetch_image( ) -> bool: """Warm exactly one viewport-surface candidate at low priority.""" + schedule_runtime_warmup = getattr( + self, + "_schedule_render_runtime_warmup", + None, + ) + if callable(schedule_runtime_warmup): + schedule_runtime_warmup() + if isinstance(descriptor, DetailPrefetchDescriptor): asset_id = descriptor.asset_id source = descriptor.path @@ -938,6 +1119,13 @@ def prefetch_images( ) -> bool: """Warm the previous/next window without occupying both decode lanes.""" + schedule_runtime_warmup = getattr( + self, + "_schedule_render_runtime_warmup", + None, + ) + if callable(schedule_runtime_warmup): + schedule_runtime_warmup() if self._preparation_shutting_down: return False self._residency_window_generation += 1 @@ -963,6 +1151,33 @@ def prefetch_images( ) or accepted return accepted + def _schedule_render_runtime_warmup(self) -> None: + """Best-effort preload after hover/click, never during app startup.""" + + if ( + not self._interaction_warmup_enabled + or self._render_runtime_warmup_requested + or self._preparation_shutting_down + or sys.platform != "win32" + ): + return + backend_name = getattr(self._image_viewer, "render_backend_name", None) + if ( + not callable(backend_name) + or str(backend_name()).strip().lower() != "opengl" + ): + return + self._render_runtime_warmup_requested = True + try: + self._preparation_pool.start(_RenderRuntimeWarmupWorker(), -2) + except RuntimeError: + self._render_runtime_warmup_requested = False + + def enable_interaction_warmup(self) -> None: + """Allow later hover/click work after startup reached its terminal state.""" + + self._interaction_warmup_enabled = True + def _schedule_adjustment_preparation(self, intent: _PreparedRequestIntent) -> bool: if self._preparation_shutting_down: return False @@ -1539,6 +1754,8 @@ def _finalize_image_surface_submission( self, source: object, generation: int, + *, + reveal_reason: str = "content_submission", ) -> None: """Reveal still content after all platform barriers are satisfied.""" @@ -1546,6 +1763,12 @@ def _finalize_image_surface_submission( self._image_viewer_rendered = True self._sync_detail_surface_cover() self._accept_still_frame_presented(source, generation) + emit_detail_event( + "surface_revealed", + generation=int(generation), + kind="image", + reason=reveal_reason, + ) def _on_image_composition_submitted(self) -> None: payload = self._peek_post_submit_payload("image") @@ -1560,7 +1783,11 @@ def _on_image_composition_submitted(self) -> None: return if not self._consume_post_submit_payload("image", payload): return - self._finalize_image_surface_submission(source, generation) + self._finalize_image_surface_submission( + source, + generation, + reveal_reason="composition", + ) def _accept_still_frame_presented(self, source: object, generation: int) -> None: generation = int(generation) @@ -2251,7 +2478,9 @@ def _on_adjusted_image_ready( self._loading_source = None self._loading_started_at = None self._image_viewer.set_image(None, {}) - self.imageLoadingFailed.emit(source, "Image decoder returned an empty frame") + message = "Image decoder returned an empty frame" + self.show_placeholder(message) + self.imageLoadingFailed.emit(source, message) return if self._defer_still_updates and self._player_stack.currentWidget() is self._video_area: @@ -2277,6 +2506,7 @@ def _on_adjusted_image_failed(self, source: Path, message: str) -> None: self._loading_source = None self._loading_started_at = None self._image_viewer.set_image(None) + self.show_placeholder(message) self.imageLoadingFailed.emit(source, message) def _apply_still_frame( diff --git a/src/iPhoto/gui/ui/widgets/gl_image_viewer/__init__.py b/src/iPhoto/gui/ui/widgets/gl_image_viewer/__init__.py index 8be5adcb7..9798a33c2 100644 --- a/src/iPhoto/gui/ui/widgets/gl_image_viewer/__init__.py +++ b/src/iPhoto/gui/ui/widgets/gl_image_viewer/__init__.py @@ -8,6 +8,6 @@ compatibility with the original single-file implementation. """ -from .widget import GLImageViewer +from .widget import GLImageViewer, preload_opengl_python_runtime -__all__ = ["GLImageViewer"] +__all__ = ["GLImageViewer", "preload_opengl_python_runtime"] diff --git a/src/iPhoto/gui/ui/widgets/gl_image_viewer/widget.py b/src/iPhoto/gui/ui/widgets/gl_image_viewer/widget.py index b90a669c3..62504a853 100644 --- a/src/iPhoto/gui/ui/widgets/gl_image_viewer/widget.py +++ b/src/iPhoto/gui/ui/widgets/gl_image_viewer/widget.py @@ -171,6 +171,24 @@ def _load_gl_renderer_class(): GLRenderer = _GLRenderer return GLRenderer + +def preload_opengl_python_runtime() -> None: + """Import raw-OpenGL helpers without creating Qt or GPU resources. + + The interaction-triggered Detail warm-up runs this on a worker thread. It + must stay limited to Python imports: QRhi/context access and renderer + resource allocation remain in ``initialize()`` on the GUI/render path. + """ + + started = time.perf_counter() + _load_gl_module() + _load_gl_renderer_class() + emit_detail_event( + "gl_runtime_preloaded", + generation=0, + duration_ms=(time.perf_counter() - started) * 1000.0, + ) + # 如果你的工程没有这个函数，可以改成固定背景色 try: from ...palette import viewer_surface_color # type: ignore @@ -1450,9 +1468,23 @@ def initialize(self, cb) -> None: # type: ignore[override] self.complete_runtime() if self._gl_initialized: return + initialize_started = time.perf_counter() + backend = self.render_backend_name() + emit_detail_event( + "qrhi_initialize_started", + generation=0, + backend=backend, + ) rhi = self.rhi() if rhi is None: _LOGGER.warning("QRhi not available - image rendering disabled") + emit_detail_event( + "qrhi_initialize_finished", + generation=0, + backend=backend, + success=False, + duration_ms=(time.perf_counter() - initialize_started) * 1000.0, + ) return if not self._uses_raw_gl: renderer = RhiImageRenderer() @@ -1460,12 +1492,26 @@ def initialize(self, cb) -> None: # type: ignore[override] renderer.initialize_resources(rhi, self.renderTarget().renderPassDescriptor(), cb) except Exception: _LOGGER.exception("Failed to initialise QRhi image renderer") + emit_detail_event( + "qrhi_initialize_finished", + generation=0, + backend=backend, + success=False, + duration_ms=(time.perf_counter() - initialize_started) * 1000.0, + ) return self._renderer = renderer self._adjustment_applicator.invalidate_cache() self._adjustment_applicator.update_curve_lut_if_needed(self._adjustments) self._adjustment_applicator.update_levels_lut_if_needed(self._adjustments) self._gl_initialized = True + emit_detail_event( + "qrhi_initialize_finished", + generation=0, + backend=backend, + success=True, + duration_ms=(time.perf_counter() - initialize_started) * 1000.0, + ) return # Make the underlying OpenGL context current so we can issue raw GL @@ -1474,6 +1520,13 @@ def initialize(self, cb) -> None: # type: ignore[override] current_context = QOpenGLContext.currentContext() if current_context is None: _LOGGER.warning("Current OpenGL context unavailable - image rendering disabled") + emit_detail_event( + "qrhi_initialize_finished", + generation=0, + backend=backend, + success=False, + duration_ms=(time.perf_counter() - initialize_started) * 1000.0, + ) return gf = current_context.extraFunctions() self._gl_funcs = gf @@ -1491,6 +1544,13 @@ def initialize(self, cb) -> None: # type: ignore[override] dpr = self.devicePixelRatioF() gf.glViewport(0, 0, int(self.width() * dpr), int(self.height() * dpr)) self._gl_initialized = True + emit_detail_event( + "qrhi_initialize_finished", + generation=0, + backend=backend, + success=True, + duration_ms=(time.perf_counter() - initialize_started) * 1000.0, + ) def releaseResources(self) -> None: # type: ignore[override] """QRhiWidget override: release renderer resources.""" diff --git a/tests/gui/test_startup_import_boundary.py b/tests/gui/test_startup_import_boundary.py index 77693d018..beae8696f 100644 --- a/tests/gui/test_startup_import_boundary.py +++ b/tests/gui/test_startup_import_boundary.py @@ -38,6 +38,35 @@ def test_gui_entry_import_keeps_heavy_features_unloaded() -> None: assert result.returncode == 0, result.stderr or result.stdout +def test_startup_preloader_does_not_warm_opengl_playback_runtime() -> None: + source = ( + Path(__file__).resolve().parents[2] + / "src/iPhoto/gui/main.py" + ).read_text(encoding="utf-8") + preload_body = source.split( + "def _preload_startup_modules() -> object:", + 1, + )[1].split("def _start_startup_imports", 1)[0] + + assert "OpenGL.GL" not in preload_body + assert "iPhoto.gui.ui.widgets.gl_renderer" not in preload_body + assert "preload_opengl_python_runtime" not in preload_body + + +def test_interaction_warmup_is_enabled_only_after_startup_terminal() -> None: + source = ( + Path(__file__).resolve().parents[2] + / "src/iPhoto/gui/main.py" + ).read_text(encoding="utf-8") + idle_body = source.split("def _run_idle_startup_jobs() -> None:", 1)[1].split( + "def _enqueue_idle_startup_jobs", 1 + )[0] + + assert idle_body.index("startup.complete()") < idle_body.index( + "enable_detail_interaction_warmup" + ) + + def test_main_window_import_keeps_optional_features_unloaded() -> None: project_root = Path(__file__).resolve().parents[2] env = dict(os.environ) diff --git a/tests/ui/controllers/test_player_view_init_cover.py b/tests/ui/controllers/test_player_view_init_cover.py index 638b46439..835798945 100644 --- a/tests/ui/controllers/test_player_view_init_cover.py +++ b/tests/ui/controllers/test_player_view_init_cover.py @@ -140,6 +140,9 @@ def set_image(self, *args, **kwargs): def current_image_source(self): return self._current_source + def render_backend_name(self) -> str: + return "opengl" + def set_adjustments(self, adjustments): self._adjustments = dict(adjustments) @@ -267,6 +270,62 @@ def test_still_decode_uses_dedicated_bounded_pool(self, controller): assert controller._pool.maxThreadCount() == 2 assert controller._preparation_pool.maxThreadCount() == 2 + def test_image_decode_primes_qrhi_surface_under_pending_cover( + self, + controller, + tmp_path, + mocker, + ): + source = tmp_path / "cold-open.jpg" + source.write_bytes(b"source") + update = mocker.patch.object(controller._image_viewer, "update") + mocker.patch.object(controller, "_schedule_render_runtime_warmup") + mocker.patch.object( + controller, + "_schedule_adjustment_preparation", + return_value=True, + ) + + assert controller.display_image(source, request_generation=12) + + assert controller._player_stack.currentWidget() is controller._image_viewer + assert controller._pending_image_generation == 12 + assert controller._pending_image_key is None + update.assert_called_once() + + def test_primed_image_decode_failure_returns_to_placeholder( + self, + controller, + tmp_path, + ): + source = (tmp_path / "broken.jpg").absolute() + controller._loading_source = source + controller._loading_started_at = time.perf_counter() + controller._begin_image_transition(13) + + controller._on_adjusted_image_failed(source, "decode failed") + + assert controller._player_stack.currentWidget() is controller._placeholder + assert controller._pending_image_generation is None + assert controller._pending_image_key is None + assert controller._placeholder.text() == "decode failed" + + def test_runtime_warmup_is_disabled_until_startup_terminal( + self, + controller, + monkeypatch, + mocker, + ): + monkeypatch.setattr( + "iPhoto.gui.ui.controllers.player_view_controller.sys.platform", + "win32", + ) + start = mocker.patch.object(controller._preparation_pool, "start") + + controller._schedule_render_runtime_warmup() + + start.assert_not_called() + def test_latest_preparation_bypasses_one_blocked_raw_worker( self, controller, @@ -1231,6 +1290,93 @@ def test_windows_image_reveal_waits_for_next_composition( assert controller._pending_image_generation is None hide_cover.assert_called_once() + def test_windows_image_reveal_deadline_is_bounded( + self, + controller, + qapp, + mocker, + ): + controller._requires_post_submit_frame = True + mocker.patch.object(controller._image_viewer, "update") + hide_cover = mocker.patch.object(controller, "_hide_detail_init_cover") + controller._player_stack.setCurrentWidget(controller._image_viewer) + controller._present_generation = 45 + controller._present_started_at = time.perf_counter() + controller._present_source = Path("image-45.jpg") + controller._arm_image_transition(45, "image-45") + + controller._image_viewer.stillFrameSubmitted.emit("image-45", 45) + qapp.processEvents() + + assert controller._post_submit_deadline_timer is not None + assert controller._post_submit_deadline_timer.isActive() + controller._on_post_submit_deadline() + + assert controller._pending_image_generation is None + assert not controller._post_submit_deadline_timer.isActive() + hide_cover.assert_called_once() + + def test_matching_composition_cancels_windows_reveal_deadline( + self, + controller, + qapp, + mocker, + ): + controller._requires_post_submit_frame = True + mocker.patch.object(controller._image_viewer, "update") + controller._player_stack.setCurrentWidget(controller._image_viewer) + controller._arm_image_transition(46, "image-46") + controller._image_viewer.stillFrameSubmitted.emit("image-46", 46) + qapp.processEvents() + + controller._image_viewer.frameSubmitted.emit() + + assert controller._post_submit_deadline_timer is not None + assert not controller._post_submit_deadline_timer.isActive() + assert controller._pending_image_generation is None + + def test_new_transition_cancels_windows_reveal_deadline( + self, + controller, + qapp, + mocker, + ): + controller._requires_post_submit_frame = True + mocker.patch.object(controller._image_viewer, "update") + controller._player_stack.setCurrentWidget(controller._image_viewer) + controller._arm_image_transition(47, "image-47") + controller._image_viewer.stillFrameSubmitted.emit("image-47", 47) + qapp.processEvents() + assert controller._post_submit_deadline_timer.isActive() + + controller._arm_image_transition(48, "image-48") + + assert not controller._post_submit_deadline_timer.isActive() + assert controller._pending_image_generation == 48 + assert controller._pending_image_key == "image-48" + + def test_stale_windows_deadline_discards_barrier_without_reveal( + self, + controller, + qapp, + mocker, + ): + controller._requires_post_submit_frame = True + mocker.patch.object(controller._image_viewer, "update") + hide_cover = mocker.patch.object(controller, "_hide_detail_init_cover") + controller._player_stack.setCurrentWidget(controller._image_viewer) + controller._arm_image_transition(50, "image-50") + controller._image_viewer.stillFrameSubmitted.emit("image-50", 50) + qapp.processEvents() + controller._pending_image_generation = 51 + controller._pending_image_key = "image-51" + + controller._on_post_submit_deadline() + + assert controller._peek_post_submit_payload("image") is None + assert controller._pending_image_generation == 51 + hide_cover.assert_not_called() + def test_delayed_old_composition_does_not_consume_new_still_generation( self, controller, @@ -1371,6 +1517,51 @@ def test_windows_video_reveal_waits_for_next_active_surface_composition( controls_enabled.assert_called_with(True) hide_cover.assert_called_once() + def test_windows_video_reveal_deadline_is_bounded( + self, + controller, + qapp, + mocker, + ): + controller._requires_post_submit_frame = True + mocker.patch.object( + controller._video_area, + "request_active_surface_update", + ) + hide_cover = mocker.patch.object(controller, "_hide_detail_init_cover") + controller.begin_video_transition(49, interactive_when_ready=True) + controller._video_area.surfaceFrameSubmitted.emit(49, 3) + qapp.processEvents() + + controller._on_post_submit_deadline() + + assert controller._pending_video_generation is None + assert controller._pending_video_content_serial is None + assert not controller._post_submit_deadline_timer.isActive() + hide_cover.assert_called_once() + + def test_interaction_schedules_windows_opengl_import_warmup_once( + self, + controller, + monkeypatch, + ): + warmed = Event() + monkeypatch.setattr( + "iPhoto.gui.ui.controllers.player_view_controller.sys.platform", + "win32", + ) + monkeypatch.setattr( + "iPhoto.gui.ui.controllers.player_view_controller.preload_opengl_python_runtime", + warmed.set, + ) + + controller.enable_interaction_warmup() + controller._schedule_render_runtime_warmup() + controller._schedule_render_runtime_warmup() + + assert warmed.wait(5) + assert controller._render_runtime_warmup_requested is True + def test_invalid_composition_does_not_consume_post_submit_payload( self, controller, From 08cd94b79f8dc1094fc02a696f37b55929d7ba9f Mon Sep 17 00:00:00 2001 From: Oliver Date: Sat, 5 Sep 2026 15:10:22 +0200 Subject: [PATCH 2/5] fix: suppress stale playback frames during transitions --- docs/WINDOWS_SCAN_PLAYBACK_DIAGNOSTICS.md | 7 + docs/architecture.md | 11 +- .../DETAIL_OPEN_BENCHMARK_RUNBOOK.md | 8 +- .../gui/coordinators/playback_coordinator.py | 11 +- .../ui/controllers/player_view_controller.py | 234 +++++++++++++--- src/iPhoto/gui/ui/widgets/detail_page.py | 14 + .../gui/ui/widgets/gl_image_viewer/widget.py | 95 ++++++- src/iPhoto/gui/ui/widgets/video_area.py | 13 + .../gui/ui/widgets/video_renderer_widget.py | 70 ++++- .../coordinators/test_playback_coordinator.py | 3 +- .../test_player_view_init_cover.py | 249 ++++++++++++++++-- .../test_gl_image_viewer_post_load_signal.py | 136 +++++++++- tests/ui/widgets/test_video_area.py | 127 +++++++++ 13 files changed, 916 insertions(+), 62 deletions(-) diff --git a/docs/WINDOWS_SCAN_PLAYBACK_DIAGNOSTICS.md b/docs/WINDOWS_SCAN_PLAYBACK_DIAGNOSTICS.md index 3dc82a20b..9ae34e85f 100644 --- a/docs/WINDOWS_SCAN_PLAYBACK_DIAGNOSTICS.md +++ b/docs/WINDOWS_SCAN_PLAYBACK_DIAGNOSTICS.md @@ -83,6 +83,13 @@ end with `post_submit_deadline` followed by `surface_revealed` with repetition matrix can determine whether this preserves the user-visible first-frame leak fix. +Transition traces must show `presentation_suppressed` before the exposed +surface's blank submission and `presentation_resumed` only after matching new +content is installed. A rapid A→B switch must record `post_submit_discarded` +for any scheduled or armed A barrier, including `reason`, `state`, and the old +epoch. During the suppressed interval a real compositor screenshot must contain +the opaque Detail background, never A's pixels or media-specific overlays. + The default timeout is 30 minutes. Override it with `-MaxMinutes 60` if the scan takes longer. The expanded directory is retained beside the ZIP so its contents can be reviewed before sharing. diff --git a/docs/architecture.md b/docs/architecture.md index de2063267..01d831597 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -227,10 +227,17 @@ and QtMultimedia runtime post-paint. Platform allowlists, post-show surface creation, parentless surface warm-up, and hide/show workarounds are forbidden. Windows OpenGL Python-module warm-up is enabled only after startup reaches its terminal state and is triggered by a later Detail hover/click; it is never a -startup prerequisite. QRhi/context creation and GPU resource allocation remain -on the active Detail surface. Windows reveal waits for the additional QRhi +startup prerequisite. It uses a lazily created, isolated one-thread pool and +may retry one failed import on a later interaction without occupying still +preparation lanes. QRhi/context creation and GPU resource allocation remain on +the active Detail surface. Windows reveal waits for the additional QRhi submission heuristic, but that wait is bounded and may fail open only after the matching current-generation media content was already submitted. +Every still, native-video, and adjusted-video transition suppresses media draws +by generation before exposing its QRhi surface. Suppressed renders submit an +opaque clear frame while preserving still residency; only installation of +matching-generation content may resume media drawing. Media-specific overlays +remain suppressed until that generation reaches its reveal terminal. The pre-show hierarchy is part of shell construction: failure is terminal and non-recoverable for that process. Startup-generation retry applies only after a valid visible shell exists; it must not claim to reconstruct the native diff --git a/docs/requirements/DETAIL_OPEN_BENCHMARK_RUNBOOK.md b/docs/requirements/DETAIL_OPEN_BENCHMARK_RUNBOOK.md index 9f4a5a2d2..7cf201b52 100644 --- a/docs/requirements/DETAIL_OPEN_BENCHMARK_RUNBOOK.md +++ b/docs/requirements/DETAIL_OPEN_BENCHMARK_RUNBOOK.md @@ -60,13 +60,17 @@ Phase 2 still 采样必须同时保留以下事件，并按 `asset_id + generati - `surface_ready`：核对最终 detached surface 的宽高与 decode level。 - Windows/OpenGL 冷首开同时保留 `gl_runtime_preloaded`、 `image_surface_init_requested`、`qrhi_initialize_started/finished` 与 - `post_submit_scheduled/armed`。模块预热只能由 startup terminal 之后的 - hover/click 触发；不得出现在无用户交互的 startup profile 中。 + `presentation_suppressed/resumed`、`post_submit_scheduled/armed`。模块预热 + 只能由 startup terminal 之后的 hover/click 触发；不得出现在无用户交互的 + startup profile 中。suppressed 区间不得产生旧 generation 的 + `gpu_upload`、`presented` 或 media submission。 - `presented`：仍是 click-to-present 的终点；stale generation 不得产生该事件。 - 正常 Windows reveal 记录 `surface_revealed(reason=composition)`；缺少额外 composition 时允许在匹配内容已提交后记录 `post_submit_deadline`、`surface_revealed(reason=deadline)`。过期 epoch、 generation、source 或 surface 只能产生 `post_submit_discarded`，不得 reveal。 + 快速切换主动取消 scheduled/armed barrier 时也必须记录 discard 的旧 epoch、 + `state=scheduled|armed` 与取消 reason，不能只依赖 deadline 路径记录。 - RAW 冷解码同时保留 `raw_probe`、`raw_candidate_selected`、`raw_thumb_decode`、 `raw_postprocess`、`raw_surface_convert` 和 `color_stats`。未知几何必须先由 `raw_probe` 修复后再产生 `level_selected`；每个 cache miss 只能选择 embedded、half、full 之一，不得在同一请求中连续执行 diff --git a/src/iPhoto/gui/coordinators/playback_coordinator.py b/src/iPhoto/gui/coordinators/playback_coordinator.py index e2dc7ab4a..5d67599d9 100644 --- a/src/iPhoto/gui/coordinators/playback_coordinator.py +++ b/src/iPhoto/gui/coordinators/playback_coordinator.py @@ -1272,7 +1272,9 @@ def _render_presentation(self, presentation: DetailPresentation) -> None: if presentation.is_live: self._hide_face_name_overlay(clear_annotations=False) - self._player_view.show_live_badge() + self._player_view.defer_live_badge_until_ready( + presentation.request_generation + ) self._player_view.set_live_replay_enabled(True) self._autoplay_live_motion(presentation) else: @@ -1573,7 +1575,12 @@ def _restore_live_still(self, *, stop_motion: bool = False) -> bool: else: self._player_view.display_image(still, asset_id=asset_id) self._player_bar.setEnabled(False) - self._player_view.show_live_badge() + badge_generation = ( + transaction.generation + if transaction is not None + else getattr(self, "_detail_request_generation", 0) + ) + self._player_view.defer_live_badge_until_ready(badge_generation) self._player_view.set_live_replay_enabled(True) self._is_playing = False return True diff --git a/src/iPhoto/gui/ui/controllers/player_view_controller.py b/src/iPhoto/gui/ui/controllers/player_view_controller.py index ab86755a8..5442484e6 100644 --- a/src/iPhoto/gui/ui/controllers/player_view_controller.py +++ b/src/iPhoto/gui/ui/controllers/player_view_controller.py @@ -201,18 +201,25 @@ def run(self) -> None: # pragma: no cover - executed on a worker thread self._signals.finished.emit(self) +class _RenderRuntimeWarmupSignals(QObject): + finished = Signal(object, bool, str) + + class _RenderRuntimeWarmupWorker(QRunnable): """Import the Windows OpenGL Python runtime after user interaction.""" + def __init__(self, signals: _RenderRuntimeWarmupSignals) -> None: + super().__init__() + self.setAutoDelete(False) + self.signals = signals + def run(self) -> None: # pragma: no cover - executed on a worker thread try: preload_opengl_python_runtime() except Exception as exc: # noqa: BLE001 - best-effort import warm-up - emit_detail_event( - "gl_runtime_preload_failed", - generation=0, - error_type=type(exc).__name__, - ) + self.signals.finished.emit(self, False, type(exc).__name__) + else: + self.signals.finished.emit(self, True, "") class _AdjustmentPreparationSignals(QObject): @@ -430,7 +437,13 @@ def __init__( self._preparation_prefetch_queue: list[_PreparedRequestIntent] = [] self._preparation_shutting_down = False self._interaction_warmup_enabled = False - self._render_runtime_warmup_requested = False + self._render_runtime_warmup_state: Literal[ + "idle", "running", "completed" + ] = "idle" + self._render_runtime_warmup_attempts = 0 + self._render_runtime_warmup_pool: QThreadPool | None = None + self._render_runtime_warmup_worker: _RenderRuntimeWarmupWorker | None = None + self._render_runtime_warmup_signals: _RenderRuntimeWarmupSignals | None = None self._raw_source_probe_cache: OrderedDict[ tuple, AssetSourceIdentity, @@ -493,6 +506,7 @@ def __init__( self._pending_video_generation: int | None = None self._pending_video_content_serial: int | None = None self._video_interactive_when_ready = False + self._pending_live_badge_generation: int | None = None self._requires_post_submit_frame = sys.platform == "win32" self._surface_transition_epoch = 0 self._post_submit_epoch: int | None = None @@ -541,10 +555,22 @@ def _default_placeholder_text(self) -> str | None: return tr("DetailPage", "Select a photo or video to preview.") return self._placeholder_default_text - def _advance_surface_transition_epoch(self) -> int: + def _advance_surface_transition_epoch(self, reason: str) -> int: """Invalidate delayed cover releases owned by an older transition.""" - self._cancel_post_submit_deadline() + kind = self._post_submit_kind + payload = self._post_submit_payload + if kind is not None and payload: + self._discard_post_submit_payload( + kind, + payload, + self._post_submit_generation(kind, payload), + reason=reason, + epoch=self._post_submit_epoch, + state="armed" if self._post_submit_armed else "scheduled", + ) + else: + self._cancel_post_submit_deadline() self._surface_transition_epoch += 1 self._post_submit_epoch = None self._post_submit_kind = None @@ -703,7 +729,12 @@ def _on_post_submit_deadline(self) -> None: or self._player_stack.currentWidget() is not self._image_viewer ) if invalid: - self._discard_post_submit_payload("image", payload, generation) + self._discard_post_submit_payload( + "image", + payload, + generation, + reason="deadline_stale", + ) return if not self._consume_post_submit_payload("image", payload): return @@ -722,7 +753,12 @@ def _on_post_submit_deadline(self) -> None: or self._player_stack.currentWidget() is not self._video_area ) if invalid: - self._discard_post_submit_payload("video", payload, generation) + self._discard_post_submit_payload( + "video", + payload, + generation, + reason="deadline_stale", + ) return if not self._consume_post_submit_payload("video", payload): return @@ -733,16 +769,31 @@ def _discard_post_submit_payload( kind: Literal["image", "video"], payload: tuple[object, ...], generation: int, + *, + reason: str, + epoch: int | None = None, + state: str | None = None, ) -> None: """Terminate an expired stale barrier without revealing its content.""" - if not self._consume_post_submit_payload(kind, payload): + if ( + self._post_submit_kind != kind + or self._post_submit_payload != payload + or self._post_submit_epoch is None + ): return + self._post_submit_epoch = None + self._post_submit_kind = None + self._post_submit_payload = () + self._post_submit_armed = False + self._cancel_post_submit_deadline() emit_detail_event( "post_submit_discarded", generation=generation, kind=kind, - epoch=self._surface_transition_epoch, + epoch=self._surface_transition_epoch if epoch is None else int(epoch), + state=state or "armed", + reason=reason, ) def _on_image_first_render(self) -> None: @@ -760,7 +811,7 @@ def _on_image_resources_invalidated(self) -> None: self._image_viewer_rendered = False if self._player_stack.currentWidget() is self._image_viewer: - self._advance_surface_transition_epoch() + self._advance_surface_transition_epoch("image_resources_invalidated") if ( self._player_stack.currentWidget() is self._image_viewer and self._pending_image_generation is None @@ -771,17 +822,30 @@ def _on_image_resources_invalidated(self) -> None: self._pending_image_key = current_source self._sync_detail_surface_cover() - def _arm_image_transition(self, generation: int, content_key: object) -> None: - """Cover the still surface until the identified content is submitted.""" + def _bind_image_transition_key( + self, + generation: int, + content_key: object, + ) -> bool: + """Bind decoded content identity without starting a second transition.""" - self._advance_surface_transition_epoch() - self._pending_image_generation = int(generation) + generation = int(generation) + if generation != self._pending_image_generation: + return False self._pending_image_key = content_key self._show_detail_init_cover() + return True def _cancel_image_transition(self) -> None: self._pending_image_generation = None self._pending_image_key = None + cancel_suppression = getattr( + self._image_viewer, + "cancel_presentation_transition", + None, + ) + if callable(cancel_suppression): + cancel_suppression() def _on_video_surface_invalidated( self, @@ -792,7 +856,7 @@ def _on_video_surface_invalidated( self._video_renderer_rendered = False if self._player_stack.currentWidget() is self._video_area and generation > 0: - self._advance_surface_transition_epoch() + self._advance_surface_transition_epoch("video_resources_invalidated") self._pending_video_generation = int(generation) self._pending_video_content_serial = ( int(content_serial) if content_serial > 0 else None @@ -835,6 +899,7 @@ def _finalize_video_surface_submission( self._video_renderer_rendered = True self._configure_video_controls(self._video_interactive_when_ready) self._sync_detail_surface_cover() + self._restore_transition_ui(generation) emit_detail_event( "surface_revealed", generation=generation, @@ -904,7 +969,7 @@ def _show_detail_init_cover(self) -> None: def show_placeholder(self, message: str | None = None) -> None: """Display the placeholder widget and clear any previous image.""" - self._advance_surface_transition_epoch() + self._advance_surface_transition_epoch("placeholder") if isinstance(self._placeholder, QLabel): self._placeholder.setText( self._default_placeholder_text() if message is None else message @@ -951,11 +1016,20 @@ def show_image_surface(self) -> None: def _begin_image_transition(self, generation: int) -> None: """Expose the still QRhi surface under the cover while decode runs.""" - self._advance_surface_transition_epoch() - self._pending_image_generation = int(generation) + self._advance_surface_transition_epoch("image_transition_started") + generation = int(generation) + suppress_presentation = getattr( + self._image_viewer, + "begin_presentation_transition", + None, + ) + if callable(suppress_presentation): + suppress_presentation(generation) + self._pending_image_generation = generation self._pending_image_key = None self._pending_video_generation = None self._pending_video_content_serial = None + self._suppress_transition_ui(generation) self._show_detail_init_cover() if self._player_stack.currentWidget() is not self._image_viewer: self._player_stack.setCurrentWidget(self._image_viewer) @@ -964,9 +1038,36 @@ def _begin_image_transition(self, generation: int) -> None: self._image_viewer.update() emit_detail_event( "image_surface_init_requested", - generation=int(generation), + generation=generation, ) + def _suppress_transition_ui(self, generation: int) -> None: + """Hide media-specific overlays before exposing a transition surface.""" + + if self._pending_live_badge_generation != int(generation): + self._pending_live_badge_generation = None + self._set_detail_media_overlays_suppressed(True) + self._live_badge.hide() + self._video_area.hide_controls(animate=False) + + def _restore_transition_ui(self, generation: int) -> None: + self._set_detail_media_overlays_suppressed(False) + if self._pending_live_badge_generation != int(generation): + return + self._pending_live_badge_generation = None + self._live_badge.show() + self._live_badge.raise_() + + def _set_detail_media_overlays_suppressed(self, suppressed: bool) -> None: + from ..widgets.detail_page import DetailPageWidget + + widget = self._player_stack.parent() + while widget is not None: + if isinstance(widget, DetailPageWidget): + widget.set_media_overlays_suppressed(suppressed) + return + widget = widget.parent() + def _configure_video_controls(self, interactive: bool) -> None: self._video_area.set_controls_enabled(interactive) if interactive: @@ -982,11 +1083,12 @@ def begin_video_transition( ) -> None: """Expose the loading surface behind a generation-bound opaque cover.""" - self._advance_surface_transition_epoch() + self._advance_surface_transition_epoch("video_transition_started") self._pending_video_generation = int(request_generation) self._pending_video_content_serial = None self._cancel_image_transition() self._video_interactive_when_ready = bool(interactive_when_ready) + self._suppress_transition_ui(request_generation) self._configure_video_controls(False) self._show_detail_init_cover() if self._player_stack.currentWidget() is not self._video_area: @@ -1156,7 +1258,8 @@ def _schedule_render_runtime_warmup(self) -> None: if ( not self._interaction_warmup_enabled - or self._render_runtime_warmup_requested + or self._render_runtime_warmup_state != "idle" + or self._render_runtime_warmup_attempts >= 2 or self._preparation_shutting_down or sys.platform != "win32" ): @@ -1167,11 +1270,48 @@ def _schedule_render_runtime_warmup(self) -> None: or str(backend_name()).strip().lower() != "opengl" ): return - self._render_runtime_warmup_requested = True + pool = self._render_runtime_warmup_pool + if pool is None: + pool = QThreadPool(self) + pool.setMaxThreadCount(1) + pool.setThreadPriority(QThread.Priority.LowPriority) + self._render_runtime_warmup_pool = pool + signals = _RenderRuntimeWarmupSignals(self) + worker = _RenderRuntimeWarmupWorker(signals) + signals.finished.connect(self._on_render_runtime_warmup_finished) + self._render_runtime_warmup_state = "running" + self._render_runtime_warmup_attempts += 1 + self._render_runtime_warmup_worker = worker + self._render_runtime_warmup_signals = signals try: - self._preparation_pool.start(_RenderRuntimeWarmupWorker(), -2) + pool.start(worker) except RuntimeError: - self._render_runtime_warmup_requested = False + self._render_runtime_warmup_state = "idle" + self._render_runtime_warmup_worker = None + self._render_runtime_warmup_signals = None + signals.deleteLater() + + def _on_render_runtime_warmup_finished( + self, + worker: object, + succeeded: bool, + error_type: str, + ) -> None: + if worker is not self._render_runtime_warmup_worker: + return + signals = self._render_runtime_warmup_signals + self._render_runtime_warmup_worker = None + self._render_runtime_warmup_signals = None + self._render_runtime_warmup_state = "completed" if succeeded else "idle" + if signals is not None: + signals.deleteLater() + if not succeeded: + emit_detail_event( + "gl_runtime_preload_failed", + generation=0, + error_type=error_type, + attempt=self._render_runtime_warmup_attempts, + ) def enable_interaction_warmup(self) -> None: """Allow later hover/click work after startup reached its terminal state.""" @@ -1408,7 +1548,7 @@ def _dispatch_prepared_intent( resident_activated = False if not defer_presentation and callable(activate_resident): if request.reason == "initial": - self._arm_image_transition(request.generation, decode_key) + self._bind_image_transition_key(request.generation, decode_key) resident_activated = activate_resident( decode_key, render_adjustments, @@ -1419,13 +1559,17 @@ def _dispatch_prepared_intent( reset_view=request.reason == "initial", generation=request.generation, ) - if not resident_activated and request.reason == "initial": - self._cancel_image_transition() - self._sync_detail_surface_cover() if resident_activated: self._present_generation = request.generation self._present_started_at = self._loading_started_at self._present_source = request.source_identity.path + complete_suppression = getattr( + self._image_viewer, + "complete_presentation_transition", + None, + ) + if callable(complete_suppression): + complete_suppression(request.generation) self.show_image_surface() self._loading_source = None self._loading_started_at = None @@ -1762,6 +1906,7 @@ def _finalize_image_surface_submission( self._cancel_image_transition() self._image_viewer_rendered = True self._sync_detail_surface_cover() + self._restore_transition_ui(generation) self._accept_still_frame_presented(source, generation) emit_detail_event( "surface_revealed", @@ -1948,6 +2093,10 @@ def shutdown(self, *, timeout_ms: int = 1500) -> None: self._preparation_shutting_down = True self.cancel_pending_image_requests() + warmup_pool = self._render_runtime_warmup_pool + if warmup_pool is not None: + warmup_pool.clear() + warmup_pool.waitForDone(min(max(0, int(timeout_ms)), 500)) self._preparation_pool.clear() preparation_done = self._preparation_pool.waitForDone(max(0, int(timeout_ms))) if preparation_done: @@ -2341,7 +2490,7 @@ def current_full_image(self) -> QImage | None: def cancel_pending_image_requests(self) -> None: """Invalidate still work when Detail or the current library is left.""" - self._advance_surface_transition_epoch() + self._advance_surface_transition_epoch("requests_cancelled") self._request_generation += 1 self._residency_window_generation += 1 self._preparation_prefetch_queue.clear() @@ -2388,7 +2537,7 @@ def apply_pending_still(self) -> bool: def clear_image(self) -> None: """Remove any pixmap currently shown in the image viewer.""" # 清空而非传空图像，避免一帧“空绘制/空上传” - self._advance_surface_transition_epoch() + self._advance_surface_transition_epoch("image_cleared") self._current_full_image = None self._cancel_image_transition() self._image_viewer.set_image(None, {}) @@ -2399,12 +2548,20 @@ def clear_image(self) -> None: def show_live_badge(self) -> None: """Ensure the Live Photo badge is visible and raised above overlays.""" + self._pending_live_badge_generation = None self._live_badge.show() self._live_badge.raise_() + def defer_live_badge_until_ready(self, generation: int) -> None: + """Restore the Live badge only after the owning media is revealed.""" + + self._pending_live_badge_generation = int(generation) + self._live_badge.hide() + def hide_live_badge(self) -> None: """Hide the Live Photo badge.""" + self._pending_live_badge_generation = None self._live_badge.hide() def is_live_badge_visible(self) -> bool: @@ -2522,11 +2679,10 @@ def _apply_still_frame( image = surface.image reason = self._request_reason_by_generation.get(self._present_generation) if reason not in {"zoom", "resize"}: - self._arm_image_transition( + self._bind_image_transition_key( self._present_generation, surface.decode_key, ) - self.show_image_surface() self._current_full_image = QImage(image) session_key = self._render_session_key_for_surface(surface) session = self._render_sessions.get(session_key) @@ -2552,6 +2708,14 @@ def _apply_still_frame( source_size=surface.source_size, reset_view=reset_view, ) + complete_suppression = getattr( + self._image_viewer, + "complete_presentation_transition", + None, + ) + if callable(complete_suppression): + complete_suppression(self._present_generation) + self.show_image_surface() self._image_viewer.update() log_detail_profile( "player_view", diff --git a/src/iPhoto/gui/ui/widgets/detail_page.py b/src/iPhoto/gui/ui/widgets/detail_page.py index d0cec7da9..dbca92a2f 100644 --- a/src/iPhoto/gui/ui/widgets/detail_page.py +++ b/src/iPhoto/gui/ui/widgets/detail_page.py @@ -172,6 +172,7 @@ def __init__( # Initialised in ``_build_player_area()``; set here so that # ``hide_rhi_init_cover()`` and ``resizeEvent`` always find the attr. self._rhi_init_cover: QWidget | None = None + self._media_overlays_suppressed = False self._edit_bundle_created = False self._feature_completed = False self._main_window = main_window @@ -819,6 +820,17 @@ def show_rhi_init_cover(self) -> None: self._rhi_init_cover.raise_() self._raise_player_overlays() + def set_media_overlays_suppressed(self, suppressed: bool) -> None: + """Prevent old media overlays from crossing a surface transition.""" + + self._media_overlays_suppressed = bool(suppressed) + if not self._media_overlays_suppressed: + return + if self.face_name_overlay is not None: + self.face_name_overlay.set_overlay_active(False) + if self.live_badge is not None: + self.live_badge.hide() + def _configure_rhi_init_cover(self, cover: QWidget) -> None: cover.setAutoFillBackground(True) cover.setAttribute(Qt.WidgetAttribute.WA_TranslucentBackground, False) @@ -826,6 +838,8 @@ def _configure_rhi_init_cover(self, cover: QWidget) -> None: cover.setStyleSheet("background-color: palette(window);") def _raise_player_overlays(self) -> None: + if self._media_overlays_suppressed: + return face_name_overlay = self.face_name_overlay live_badge = self.live_badge if face_name_overlay is None or live_badge is None: diff --git a/src/iPhoto/gui/ui/widgets/gl_image_viewer/widget.py b/src/iPhoto/gui/ui/widgets/gl_image_viewer/widget.py index 62504a853..d01c43d28 100644 --- a/src/iPhoto/gui/ui/widgets/gl_image_viewer/widget.py +++ b/src/iPhoto/gui/ui/widgets/gl_image_viewer/widget.py @@ -282,6 +282,7 @@ def __init__( self._content_revision = 0 self._rendered_content_identity: tuple[str, object, int, int] | None = None self._last_composed_content_identity: tuple[str, object, int, int] | None = None + self._presentation_suppressed_generation: int | None = None self._source_image_dimensions: tuple[int, int] | None = None self._video_frame = None self._pending_video_image: QImage | None = None @@ -418,6 +419,53 @@ def render_backend_name(self) -> str: return qrhi_api_name(self._rhi_api) + def begin_presentation_transition(self, generation: int) -> None: + """Suppress media draws while preserving all resident GPU resources.""" + + generation = int(generation) + if generation <= 0: + raise ValueError("presentation transition generation must be positive") + self._presentation_suppressed_generation = generation + self._still_presentation_pending = False + self._video_frame_presentation_pending = False + self._rendered_content_identity = None + emit_detail_event( + "presentation_suppressed", + generation=generation, + renderer="gl_image_viewer", + ) + + def complete_presentation_transition(self, generation: int) -> bool: + """Resume media draws only for the transition that owns suppression.""" + + if int(generation) != self._presentation_suppressed_generation: + return False + self._presentation_suppressed_generation = None + emit_detail_event( + "presentation_resumed", + generation=int(generation), + renderer="gl_image_viewer", + ) + return True + + def cancel_presentation_transition(self) -> None: + """Drop presentation suppression without changing texture residency.""" + + generation = self._presentation_suppressed_generation + self._presentation_suppressed_generation = None + self._still_presentation_pending = False + self._video_frame_presentation_pending = False + self._rendered_content_identity = None + if generation is not None: + emit_detail_event( + "presentation_suppression_cancelled", + generation=generation, + renderer="gl_image_viewer", + ) + + def _presentation_is_suppressed(self) -> bool: + return getattr(self, "_presentation_suppressed_generation", None) is not None + def render_device_name(self) -> str: """Return the QRhi adapter name used to reject software benchmark runs.""" @@ -1313,6 +1361,13 @@ def _pass_clear_color(self) -> QColor: bg = self._fullscreen_handler.backdrop_color return QColor.fromRgbF(bg.redF(), bg.greenF(), bg.blueF(), 1.0) + def _transition_clear_color(self) -> QColor: + """Return an opaque clear colour for generation transitions.""" + + color = QColor(self._pass_clear_color()) + color.setAlpha(255) + return color + def _gl_clear_rgba(self) -> tuple[float, float, float, float]: """Return the OpenGL clear colour matching the QRhi pass clear.""" @@ -1597,7 +1652,11 @@ def render(self, cb) -> None: # type: ignore[override] # window's WA_TranslucentBackground. cb.beginPass( self.renderTarget(), - self._pass_clear_color(), + ( + self._transition_clear_color() + if GLImageViewer._presentation_is_suppressed(self) + else self._pass_clear_color() + ), QRhiDepthStencilClearValue(), ) cb.endPass() @@ -1608,7 +1667,11 @@ def render(self, cb) -> None: # type: ignore[override] if gf is None or self._renderer is None: cb.beginPass( self.renderTarget(), - self._pass_clear_color(), + ( + self._transition_clear_color() + if GLImageViewer._presentation_is_suppressed(self) + else self._pass_clear_color() + ), QRhiDepthStencilClearValue(), ) cb.endPass() @@ -1629,6 +1692,17 @@ def render(self, cb) -> None: # type: ignore[override] return self._last_render_target_size = QSize(output_size) + if GLImageViewer._presentation_is_suppressed(self): + cb.beginPass( + self.renderTarget(), + self._transition_clear_color(), + QRhiDepthStencilClearValue(), + ) + cb.endPass() + self._queue_first_frame_ready() + self._rendered_content_identity = None + return + # Start a QRhi render pass (required by QRhiWidget) then immediately # switch to raw OpenGL via beginExternal()/endExternal(). This lets # us keep all existing GL 3.3 shader code unchanged while both @@ -1836,7 +1910,11 @@ def _render_rhi(self, cb) -> None: if not self._gl_initialized or self._renderer is None: cb.beginPass( self.renderTarget(), - self._pass_clear_color(), + ( + self._transition_clear_color() + if GLImageViewer._presentation_is_suppressed(self) + else self._pass_clear_color() + ), QRhiDepthStencilClearValue(), ) cb.endPass() @@ -1849,6 +1927,17 @@ def _render_rhi(self, cb) -> None: return self._last_render_target_size = QSize(output_size) + if GLImageViewer._presentation_is_suppressed(self): + cb.beginPass( + self.renderTarget(), + self._transition_clear_color(), + QRhiDepthStencilClearValue(), + ) + cb.endPass() + self._queue_first_frame_ready() + self._rendered_content_identity = None + return + vw = max(1, output_size.width()) vh = max(1, output_size.height()) diff --git a/src/iPhoto/gui/ui/widgets/video_area.py b/src/iPhoto/gui/ui/widgets/video_area.py index 5b02598f8..be555db7f 100644 --- a/src/iPhoto/gui/ui/widgets/video_area.py +++ b/src/iPhoto/gui/ui/widgets/video_area.py @@ -838,6 +838,11 @@ def begin_load(self, path: Path, request_generation: int) -> int: load_started = time.perf_counter() prev_source = self._current_source previous_duration_ms = self._current_duration_ms + next_media_generation = self._media_generation + 1 + for surface in (self._renderer, self._edit_viewer): + suppress = getattr(surface, "begin_presentation_transition", None) + if callable(suppress): + suppress(next_media_generation) media_generation = self._begin_media_generation() self._detach_video_output() if sys.platform == "darwin" and prev_source is not None: @@ -1335,6 +1340,14 @@ def _submit_video_frame_to_surface( ) -> None: """Queue one identified frame on exactly one QRhi child surface.""" + complete_transition = getattr( + surface, + "complete_presentation_transition", + None, + ) + if callable(complete_transition): + complete_transition(self._media_generation) + if surface is self._edit_viewer: resolved_rotation_cw = _resolve_frame_rotation_cw( frame.surfaceFormat(), diff --git a/src/iPhoto/gui/ui/widgets/video_renderer_widget.py b/src/iPhoto/gui/ui/widgets/video_renderer_widget.py index 83d61be2f..a89517628 100644 --- a/src/iPhoto/gui/ui/widgets/video_renderer_widget.py +++ b/src/iPhoto/gui/ui/widgets/video_renderer_widget.py @@ -49,6 +49,8 @@ ) from PySide6.QtWidgets import QRhiWidget, QWidget +from iPhoto.gui.detail_profile import emit_detail_event + QVideoFrame = None # type: ignore[assignment, misc] QVideoFrameFormat = None # type: ignore[assignment, misc] @@ -326,6 +328,7 @@ def __init__(self, parent: Optional[QWidget] = None) -> None: self._frame_content_revision = 0 self._rendered_content_identity: tuple[int, int, int] | None = None self._last_composed_content_identity: tuple[int, int, int] | None = None + self._presentation_suppressed_generation: int | None = None self._viewport_fill_enabled = False self._zoom_factor = 1.0 self._transparent_rounded_clip_enabled = False @@ -383,6 +386,51 @@ def render_backend_name(self) -> str: return qrhi_api_name(self._rhi_api) + def begin_presentation_transition(self, generation: int) -> None: + """Suppress video draws while retaining allocated QRhi textures.""" + + generation = int(generation) + if generation <= 0: + raise ValueError("presentation transition generation must be positive") + self._presentation_suppressed_generation = generation + self._frame_presentation_pending = False + self._rendered_content_identity = None + emit_detail_event( + "presentation_suppressed", + generation=generation, + renderer="video_renderer", + ) + + def complete_presentation_transition(self, generation: int) -> bool: + """Resume video draws only for the owning media generation.""" + + if int(generation) != self._presentation_suppressed_generation: + return False + self._presentation_suppressed_generation = None + emit_detail_event( + "presentation_resumed", + generation=int(generation), + renderer="video_renderer", + ) + return True + + def cancel_presentation_transition(self) -> None: + """Cancel suppression without destroying QRhi resources.""" + + generation = self._presentation_suppressed_generation + self._presentation_suppressed_generation = None + self._frame_presentation_pending = False + self._rendered_content_identity = None + if generation is not None: + emit_detail_event( + "presentation_suppression_cancelled", + generation=generation, + renderer="video_renderer", + ) + + def _presentation_is_suppressed(self) -> bool: + return getattr(self, "_presentation_suppressed_generation", None) is not None + # ------------------------------------------------------------------ # Public API # ------------------------------------------------------------------ @@ -760,7 +808,11 @@ def render(self, cb) -> None: # type: ignore[override] # target. Normal playback stays opaque; preview popups stay clear. cb.beginPass( self.renderTarget(), - self._pass_clear_color(self._letterbox_color), + ( + self._transition_clear_color() + if VideoRendererWidget._presentation_is_suppressed(self) + else self._pass_clear_color(self._letterbox_color) + ), QRhiDepthStencilClearValue(), ) cb.endPass() @@ -776,6 +828,17 @@ def render(self, cb) -> None: # type: ignore[override] if output_size.isEmpty(): return + if VideoRendererWidget._presentation_is_suppressed(self): + cb.beginPass( + self.renderTarget(), + self._transition_clear_color(), + QRhiDepthStencilClearValue(), + ) + cb.endPass() + self._queue_first_frame_ready() + self._rendered_content_identity = None + return + # When no video frame has been loaded (or after clear_frame()), fill # the render target with the opaque letterbox colour. This prevents # stale texture data from a previously played video from flashing on @@ -859,6 +922,11 @@ def _pass_clear_color(self, fallback: QColor) -> QColor: color.setAlpha(255) return color + def _transition_clear_color(self) -> QColor: + color = QColor(self._letterbox_color) + color.setAlpha(255) + return color + def releaseResources(self) -> None: # type: ignore[override] """Destroy resources owned by the current QRhi before it disappears.""" diff --git a/tests/gui/coordinators/test_playback_coordinator.py b/tests/gui/coordinators/test_playback_coordinator.py index c11df3cc4..a5d3b6071 100644 --- a/tests/gui/coordinators/test_playback_coordinator.py +++ b/tests/gui/coordinators/test_playback_coordinator.py @@ -1200,6 +1200,7 @@ def test_live_photo_motion_preparation_failure_restores_pending_still() -> None: apply_pending_still=Mock(return_value=True), display_image=Mock(), show_live_badge=Mock(), + defer_live_badge_until_ready=Mock(), set_live_replay_enabled=Mock(), show_placeholder=Mock(), ) @@ -1225,7 +1226,7 @@ def test_live_photo_motion_preparation_failure_restores_pending_still() -> None: coordinator._player_view.apply_pending_still.assert_called_once_with() coordinator._player_view.display_image.assert_not_called() coordinator._player_view.show_placeholder.assert_not_called() - coordinator._player_view.show_live_badge.assert_called_once_with() + coordinator._player_view.defer_live_badge_until_ready.assert_called_once_with(7) coordinator._player_view.set_live_replay_enabled.assert_called_once_with(True) PlaybackCoordinator._on_still_frame_presented(coordinator, still, 7) diff --git a/tests/ui/controllers/test_player_view_init_cover.py b/tests/ui/controllers/test_player_view_init_cover.py index 835798945..5abc34dbc 100644 --- a/tests/ui/controllers/test_player_view_init_cover.py +++ b/tests/ui/controllers/test_player_view_init_cover.py @@ -132,6 +132,7 @@ def __init__(self, parent: QWidget | None = None) -> None: self._has_image_content = True self._current_source = None self._adjustments = {} + self._presentation_suppressed_generation = None self.setMouseTracking(True) def set_image(self, *args, **kwargs): @@ -143,6 +144,18 @@ def current_image_source(self): def render_backend_name(self) -> str: return "opengl" + def begin_presentation_transition(self, generation: int) -> None: + self._presentation_suppressed_generation = int(generation) + + def complete_presentation_transition(self, generation: int) -> bool: + if self._presentation_suppressed_generation != int(generation): + return False + self._presentation_suppressed_generation = None + return True + + def cancel_presentation_transition(self) -> None: + self._presentation_suppressed_generation = None + def set_adjustments(self, adjustments): self._adjustments = dict(adjustments) @@ -291,8 +304,72 @@ def test_image_decode_primes_qrhi_surface_under_pending_cover( assert controller._player_stack.currentWidget() is controller._image_viewer assert controller._pending_image_generation == 12 assert controller._pending_image_key is None + assert controller._image_viewer._presentation_suppressed_generation == 12 update.assert_called_once() + def test_resident_miss_keeps_transition_suppressed_until_decode( + self, + controller, + tmp_path, + mocker, + monkeypatch, + ): + source = tmp_path / "resident-miss.jpg" + normalized_source = source.absolute() + identity = AssetSourceIdentity.create( + source, + width=1600, + height=1200, + source_mtime_ns=1, + ) + controller._begin_image_transition(14) + controller._loading_source = normalized_source + controller._loading_started_at = time.perf_counter() + mocker.patch.object( + controller, + "_viewport_metrics", + return_value=((800, 600), 1.0), + ) + activate = mocker.Mock(return_value=False) + monkeypatch.setattr( + controller._image_viewer, + "activate_resident_surface", + activate, + raising=False, + ) + request = mocker.patch.object( + controller._still_scheduler, + "request", + return_value=True, + ) + + assert controller._dispatch_prepared_intent( + _PreparedRequestIntent("asset-14", identity, 14, "initial"), + {}, + ) + + assert controller._pending_image_generation == 14 + assert controller._pending_image_key is not None + assert controller._image_viewer._presentation_suppressed_generation == 14 + assert controller._player_stack.currentWidget() is controller._image_viewer + activate.assert_called_once() + request.assert_called_once() + + def test_transition_badge_restores_only_for_matching_generation( + self, + controller, + ): + controller.show_live_badge() + controller._begin_image_transition(15) + controller.defer_live_badge_until_ready(15) + + assert controller.is_live_badge_visible() is False + controller._restore_transition_ui(14) + assert controller.is_live_badge_visible() is False + + controller._restore_transition_ui(15) + assert controller._live_badge.isHidden() is False + def test_primed_image_decode_failure_returns_to_placeholder( self, controller, @@ -325,6 +402,9 @@ def test_runtime_warmup_is_disabled_until_startup_terminal( controller._schedule_render_runtime_warmup() start.assert_not_called() + assert controller._render_runtime_warmup_pool is None + assert controller._render_runtime_warmup_state == "idle" + assert controller._render_runtime_warmup_attempts == 0 def test_latest_preparation_bypasses_one_blocked_raw_worker( self, @@ -1084,7 +1164,8 @@ def test_image_transition_waits_for_matching_content_submission( mock_show_cover = mocker.patch.object(controller, "_show_detail_init_cover") mock_hide_cover = mocker.patch.object(controller, "_hide_detail_init_cover") controller._image_viewer_rendered = True - controller._arm_image_transition(22, "image-b") + controller._begin_image_transition(22) + controller._bind_image_transition_key(22, "image-b") controller.show_image_surface() controller._image_viewer.firstFrameReady.emit() @@ -1111,19 +1192,19 @@ def test_still_content_barrier_is_armed_before_surface_switch_and_upload( ) controller._present_generation = 24 controller._request_reason_by_generation[24] = "initial" - arm = mocker.patch.object(controller, "_arm_image_transition") + controller._begin_image_transition(24) + bind = mocker.patch.object(controller, "_bind_image_transition_key") show = mocker.patch.object(controller, "show_image_surface") upload = mocker.patch.object(controller._image_viewer, "set_image") calls = mocker.Mock() - calls.attach_mock(arm, "arm") + calls.attach_mock(bind, "bind") calls.attach_mock(show, "show") calls.attach_mock(upload, "upload") controller._apply_still_frame(surface, {}) assert calls.mock_calls[:3] == [ - call.arm(24, surface.decode_key), - call.show(), + call.bind(24, surface.decode_key), call.upload( surface.image, {}, @@ -1131,6 +1212,7 @@ def test_still_content_barrier_is_armed_before_surface_switch_and_upload( source_size=surface.source_size, reset_view=True, ), + call.show(), ] def test_stale_still_generation_cannot_consume_current_presentation( @@ -1271,7 +1353,8 @@ def test_windows_image_reveal_waits_for_next_composition( controller._requires_post_submit_frame = True update = mocker.patch.object(controller._image_viewer, "update") hide_cover = mocker.patch.object(controller, "_hide_detail_init_cover") - controller._arm_image_transition(40, "image-40") + controller._begin_image_transition(40) + controller._bind_image_transition_key(40, "image-40") controller.show_image_surface() update.reset_mock() @@ -1303,7 +1386,8 @@ def test_windows_image_reveal_deadline_is_bounded( controller._present_generation = 45 controller._present_started_at = time.perf_counter() controller._present_source = Path("image-45.jpg") - controller._arm_image_transition(45, "image-45") + controller._begin_image_transition(45) + controller._bind_image_transition_key(45, "image-45") controller._image_viewer.stillFrameSubmitted.emit("image-45", 45) qapp.processEvents() @@ -1325,7 +1409,8 @@ def test_matching_composition_cancels_windows_reveal_deadline( controller._requires_post_submit_frame = True mocker.patch.object(controller._image_viewer, "update") controller._player_stack.setCurrentWidget(controller._image_viewer) - controller._arm_image_transition(46, "image-46") + controller._begin_image_transition(46) + controller._bind_image_transition_key(46, "image-46") controller._image_viewer.stillFrameSubmitted.emit("image-46", 46) qapp.processEvents() @@ -1344,17 +1429,74 @@ def test_new_transition_cancels_windows_reveal_deadline( controller._requires_post_submit_frame = True mocker.patch.object(controller._image_viewer, "update") controller._player_stack.setCurrentWidget(controller._image_viewer) - controller._arm_image_transition(47, "image-47") + controller._begin_image_transition(47) + controller._bind_image_transition_key(47, "image-47") controller._image_viewer.stillFrameSubmitted.emit("image-47", 47) qapp.processEvents() assert controller._post_submit_deadline_timer.isActive() - controller._arm_image_transition(48, "image-48") + controller._begin_image_transition(48) + controller._bind_image_transition_key(48, "image-48") assert not controller._post_submit_deadline_timer.isActive() assert controller._pending_image_generation == 48 assert controller._pending_image_key == "image-48" + def test_new_generation_records_discarded_armed_barrier( + self, + controller, + qapp, + mocker, + ): + controller._requires_post_submit_frame = True + mocker.patch.object(controller._image_viewer, "update") + emit = mocker.patch( + "iPhoto.gui.ui.controllers.player_view_controller.emit_detail_event" + ) + controller._begin_image_transition(60) + controller._bind_image_transition_key(60, "image-60") + controller._image_viewer.stillFrameSubmitted.emit("image-60", 60) + qapp.processEvents() + + controller._begin_image_transition(61) + + discard_calls = [ + item + for item in emit.call_args_list + if item.args and item.args[0] == "post_submit_discarded" + ] + assert len(discard_calls) == 1 + assert discard_calls[0].kwargs["generation"] == 60 + assert discard_calls[0].kwargs["kind"] == "image" + assert discard_calls[0].kwargs["state"] == "armed" + assert discard_calls[0].kwargs["reason"] == "image_transition_started" + + def test_same_turn_navigation_records_discarded_scheduled_barrier( + self, + controller, + mocker, + ): + controller._requires_post_submit_frame = True + mocker.patch.object(controller._image_viewer, "update") + emit = mocker.patch( + "iPhoto.gui.ui.controllers.player_view_controller.emit_detail_event" + ) + controller._begin_image_transition(62) + controller._bind_image_transition_key(62, "image-62") + controller._image_viewer.stillFrameSubmitted.emit("image-62", 62) + + controller._begin_image_transition(63) + + discard_calls = [ + item + for item in emit.call_args_list + if item.args and item.args[0] == "post_submit_discarded" + ] + assert len(discard_calls) == 1 + assert discard_calls[0].kwargs["generation"] == 62 + assert discard_calls[0].kwargs["state"] == "scheduled" + assert discard_calls[0].kwargs["reason"] == "image_transition_started" + def test_stale_windows_deadline_discards_barrier_without_reveal( self, controller, @@ -1365,7 +1507,8 @@ def test_stale_windows_deadline_discards_barrier_without_reveal( mocker.patch.object(controller._image_viewer, "update") hide_cover = mocker.patch.object(controller, "_hide_detail_init_cover") controller._player_stack.setCurrentWidget(controller._image_viewer) - controller._arm_image_transition(50, "image-50") + controller._begin_image_transition(50) + controller._bind_image_transition_key(50, "image-50") controller._image_viewer.stillFrameSubmitted.emit("image-50", 50) qapp.processEvents() controller._pending_image_generation = 51 @@ -1404,7 +1547,8 @@ def test_delayed_old_composition_does_not_consume_new_still_generation( controller._present_generation = 40 controller._present_source = path controller._present_started_at = time.perf_counter() - controller._arm_image_transition(40, old_key) + controller._begin_image_transition(40) + controller._bind_image_transition_key(40, old_key) controller._image_viewer.stillFrameSubmitted.emit(old_key, 40) qapp.processEvents() @@ -1454,7 +1598,8 @@ def test_current_still_submission_is_accepted_before_old_composition( controller._present_generation = 40 controller._present_source = path controller._present_started_at = time.perf_counter() - controller._arm_image_transition(40, old_key) + controller._begin_image_transition(40) + controller._bind_image_transition_key(40, old_key) controller._image_viewer.stillFrameSubmitted.emit(old_key, 40) qapp.processEvents() @@ -1544,6 +1689,7 @@ def test_interaction_schedules_windows_opengl_import_warmup_once( self, controller, monkeypatch, + qapp, ): warmed = Event() monkeypatch.setattr( @@ -1560,7 +1706,51 @@ def test_interaction_schedules_windows_opengl_import_warmup_once( controller._schedule_render_runtime_warmup() assert warmed.wait(5) - assert controller._render_runtime_warmup_requested is True + assert _spin_until( + qapp, + lambda: controller._render_runtime_warmup_state == "completed", + ) + assert controller._render_runtime_warmup_attempts == 1 + assert controller._render_runtime_warmup_pool is not controller._preparation_pool + + def test_failed_windows_opengl_warmup_can_retry_once( + self, + controller, + monkeypatch, + qapp, + ): + attempts = 0 + + def preload() -> None: + nonlocal attempts + attempts += 1 + if attempts == 1: + raise RuntimeError("transient") + + monkeypatch.setattr( + "iPhoto.gui.ui.controllers.player_view_controller.sys.platform", + "win32", + ) + monkeypatch.setattr( + "iPhoto.gui.ui.controllers.player_view_controller.preload_opengl_python_runtime", + preload, + ) + controller.enable_interaction_warmup() + + controller._schedule_render_runtime_warmup() + assert _spin_until( + qapp, + lambda: controller._render_runtime_warmup_state == "idle" + and controller._render_runtime_warmup_attempts == 1, + ) + controller._schedule_render_runtime_warmup() + + assert _spin_until( + qapp, + lambda: controller._render_runtime_warmup_state == "completed", + ) + assert attempts == 2 + assert controller._render_runtime_warmup_attempts == 2 def test_invalid_composition_does_not_consume_post_submit_payload( self, @@ -1600,7 +1790,8 @@ def test_new_transition_cancels_deferred_windows_cover_release( ): controller._requires_post_submit_frame = True update = mocker.patch.object(controller._image_viewer, "update") - controller._arm_image_transition(42, "image-42") + controller._begin_image_transition(42) + controller._bind_image_transition_key(42, "image-42") controller.show_image_surface() update.reset_mock() controller._image_viewer.stillFrameSubmitted.emit("image-42", 42) @@ -1751,6 +1942,34 @@ def test_init_cover_stays_below_face_name_overlay_and_does_not_take_mouse( QApplication.restoreOverrideCursor() +def test_detail_transition_suppresses_old_media_overlays(qapp, monkeypatch): + monkeypatch.setattr( + "iPhoto.gui.ui.widgets.detail_page.VideoArea", + _FakeVideoArea, + ) + main_window = QWidget() + detail = DetailPageWidget(main_window, image_viewer=_FakeImageViewer()) + detail.resize(640, 480) + detail.show() + detail.face_name_overlay.set_overlay_active(True) + detail.live_badge.show() + qapp.processEvents() + + detail.set_media_overlays_suppressed(True) + detail.show_rhi_init_cover() + qapp.processEvents() + + assert detail.face_name_overlay.isHidden() + assert detail.live_badge.isHidden() + + detail.set_media_overlays_suppressed(False) + detail.show_rhi_init_cover() + qapp.processEvents() + + assert detail.face_name_overlay.isHidden() + assert detail.live_badge.isHidden() + + class TestPlaceholderMessage: def test_show_placeholder_supports_custom_message(self, controller): controller.show_placeholder("Writing data, please wait...") diff --git a/tests/ui/widgets/test_gl_image_viewer_post_load_signal.py b/tests/ui/widgets/test_gl_image_viewer_post_load_signal.py index 52b252f12..8fcfa1d1f 100644 --- a/tests/ui/widgets/test_gl_image_viewer_post_load_signal.py +++ b/tests/ui/widgets/test_gl_image_viewer_post_load_signal.py @@ -5,13 +5,15 @@ pytest.importorskip("PySide6", reason="PySide6 is required for GL image viewer tests") import os +import sys +import time from types import SimpleNamespace from unittest.mock import Mock, patch from PySide6.QtCore import QPointF, QSize from PySide6.QtGui import QImage from PySide6.QtTest import QSignalSpy -from PySide6.QtWidgets import QApplication +from PySide6.QtWidgets import QApplication, QGridLayout, QWidget from iPhoto.gui.ui.widgets.gl_image_viewer import GLImageViewer from iPhoto.gui.ui.widgets.gl_image_viewer.widget import _crop_preview_adjustments @@ -74,6 +76,134 @@ def test_gl_image_viewer_maps_image_geometry_before_texture_upload(qapp) -> None assert image_point.y() == pytest.approx(160.0) +def test_presentation_transition_is_generation_bound_and_preserves_texture(qapp) -> None: + viewer = GLImageViewer() + image = QImage(32, 24, QImage.Format.Format_RGBA8888) + image.fill(0xFFFF0000) + viewer.set_image(image, {}, image_source="old-still") + + viewer.begin_presentation_transition(8) + + assert viewer.current_image_source() == "old-still" + assert viewer.has_image_content() is True + assert viewer.complete_presentation_transition(7) is False + assert viewer._presentation_suppressed_generation == 8 + assert viewer.complete_presentation_transition(8) is True + assert viewer.current_image_source() == "old-still" + + +def test_raw_gl_suppression_clears_without_drawing_or_mutating_residency() -> None: + viewer = Mock() + viewer._uses_raw_gl = True + viewer._gl_initialized = True + viewer._gl_funcs = Mock() + viewer._presentation_suppressed_generation = 9 + viewer._renderer.has_texture.return_value = True + viewer._pending_resident_activation = "new-still" + viewer._pending_warm_surfaces = ["neighbor"] + target = Mock() + target.pixelSize.return_value = QSize(320, 240) + viewer.renderTarget.return_value = target + command_buffer = Mock() + + GLImageViewer.render(viewer, command_buffer) + + command_buffer.beginPass.assert_called_once() + command_buffer.beginExternal.assert_not_called() + viewer._renderer.render.assert_not_called() + viewer._texture_manager.activate_resident_texture.assert_not_called() + viewer._texture_manager.needs_texture_upload.assert_not_called() + assert viewer._pending_resident_activation == "new-still" + assert viewer._pending_warm_surfaces == ["neighbor"] + assert viewer._rendered_content_identity is None + + +def test_rhi_suppression_clears_without_drawing_or_consuming_new_surface() -> None: + viewer = Mock() + viewer._gl_initialized = True + viewer._presentation_suppressed_generation = 10 + viewer._pending_resident_activation = "new-still" + viewer._pending_warm_surfaces = ["neighbor"] + target = Mock() + target.pixelSize.return_value = QSize(320, 240) + viewer.renderTarget.return_value = target + command_buffer = Mock() + + GLImageViewer._render_rhi(viewer, command_buffer) + + command_buffer.beginPass.assert_called_once() + viewer._renderer.render.assert_not_called() + viewer._texture_manager.activate_resident_texture.assert_not_called() + viewer._texture_manager.needs_texture_upload.assert_not_called() + assert viewer._pending_resident_activation == "new-still" + assert viewer._pending_warm_surfaces == ["neighbor"] + assert viewer._rendered_content_identity is None + + +@pytest.mark.gpu +@pytest.mark.windows_compositor +def test_visible_windows_transition_never_exposes_previous_still(qapp) -> None: + """Validate transition pixels on a real visible Windows QRhi compositor.""" + + if sys.platform != "win32": + pytest.skip("requires a visible Windows compositor integration runner") + if QApplication.platformName().lower() in {"offscreen", "minimal"}: + pytest.skip("requires a visible platform QRhi compositor") + + host = QWidget() + layout = QGridLayout(host) + layout.setContentsMargins(0, 0, 0, 0) + viewer = GLImageViewer(host) + layout.addWidget(viewer, 0, 0) + host.resize(320, 180) + host.show() + qapp.processEvents() + + def wait_for(spy: QSignalSpy, count: int) -> None: + deadline = time.monotonic() + 5.0 + while spy.count() < count and time.monotonic() < deadline: + qapp.processEvents() + time.sleep(0.005) + assert spy.count() >= count + + def center_pixel(): + screen = qapp.primaryScreen() + assert screen is not None + image = screen.grabWindow(int(host.winId())).toImage() + assert not image.isNull() + return image.pixelColor(image.width() // 2, image.height() // 2) + + submitted = QSignalSpy(viewer.stillFrameSubmitted) + composed = QSignalSpy(viewer.frameSubmitted) + red = QImage(320, 180, QImage.Format.Format_RGBA8888) + red.fill(0xFFFF0000) + viewer._still_generation_by_key["red"] = 1 + viewer.set_image(red, {}, image_source="red") + viewer.update() + wait_for(submitted, 1) + red_pixel = center_pixel() + assert red_pixel.red() > red_pixel.blue() + + viewer.begin_presentation_transition(2) + composed_before = composed.count() + viewer.update() + wait_for(composed, composed_before + 1) + transition_pixel = center_pixel() + assert transition_pixel.red() < 200 + + blue = QImage(320, 180, QImage.Format.Format_RGBA8888) + blue.fill(0xFF0000FF) + viewer._still_generation_by_key["blue"] = 2 + viewer.set_image(blue, {}, image_source="blue") + assert viewer.complete_presentation_transition(2) + viewer.update() + wait_for(submitted, 2) + blue_pixel = center_pixel() + host.close() + + assert blue_pixel.blue() > blue_pixel.red() + + def test_still_surface_retains_transaction_generation_until_gpu_upload(qapp) -> None: viewer = GLImageViewer() image = QImage(32, 24, QImage.Format.Format_RGBA8888) @@ -135,6 +265,7 @@ def test_rhi_render_without_pending_upload_has_defined_presentation_flags() -> N """Regression: an idle Metal render must not read an unbound local.""" viewer = Mock() + viewer._presentation_suppressed_generation = None viewer._gl_initialized = True viewer._renderer.has_texture.return_value = True viewer._using_video_frame_source = False @@ -186,6 +317,7 @@ def test_rhi_render_presents_video_uploaded_before_render() -> None: """A video draw is acknowledged only after window-frame submission.""" viewer = Mock() + viewer._presentation_suppressed_generation = None viewer._gl_initialized = True viewer._renderer.has_texture.return_value = True viewer._using_video_frame_source = True @@ -287,6 +419,7 @@ def test_rhi_first_texture_failure_is_reported_before_no_texture_return() -> Non """A failed first allocation must trigger LOD fallback without an old texture.""" viewer = Mock() + viewer._presentation_suppressed_generation = None viewer._gl_initialized = True viewer._using_video_frame_source = False viewer._video_frame_dirty = False @@ -328,6 +461,7 @@ def test_windows_gl_first_texture_failure_is_reported_before_no_texture_return( """The Windows raw-GL path must also report a failed first allocation.""" viewer = Mock() + viewer._presentation_suppressed_generation = None viewer._uses_raw_gl = True viewer._gl_initialized = True viewer._using_video_frame_source = False diff --git a/tests/ui/widgets/test_video_area.py b/tests/ui/widgets/test_video_area.py index faa5ac7e7..58b587979 100644 --- a/tests/ui/widgets/test_video_area.py +++ b/tests/ui/widgets/test_video_area.py @@ -19,6 +19,7 @@ from PySide6.QtCore import QPointF, QRectF, QSize, QSizeF, Qt, QTimer from PySide6.QtGui import QColor, QImage, QKeyEvent, QRhiCommandBuffer, QShowEvent from PySide6.QtMultimedia import QMediaPlayer, QVideoFrame, QVideoFrameFormat +from PySide6.QtTest import QSignalSpy from PySide6.QtWidgets import ( QApplication, QGridLayout, @@ -739,6 +740,83 @@ def _on_composed() -> None: assert cover_visible_at_submission == expected_visibility assert cover_released + @pytest.mark.gpu + @pytest.mark.windows_compositor + @pytest.mark.parametrize("surface_kind", ("native", "adjusted")) + def test_visible_windows_video_transition_never_exposes_previous_frame( + self, + qapp, + surface_kind, + ): + if sys.platform != "win32": + pytest.skip("requires a visible Windows compositor integration runner") + if QApplication.platformName().lower() in {"offscreen", "minimal"}: + pytest.skip("requires a visible platform QRhi compositor") + + host = QWidget() + layout = QGridLayout(host) + layout.setContentsMargins(0, 0, 0, 0) + surface = VideoRendererWidget(host) if surface_kind == "native" else GLImageViewer(host) + layout.addWidget(surface, 0, 0) + host.resize(320, 180) + host.show() + qapp.processEvents() + + presented = QSignalSpy(surface.videoFramePresented) + composed = QSignalSpy(surface.frameSubmitted) + + def wait_for(spy: QSignalSpy, count: int) -> None: + deadline = time.monotonic() + 5.0 + while spy.count() < count and time.monotonic() < deadline: + qapp.processEvents() + time.sleep(0.005) + assert spy.count() >= count + + def center_pixel(): + screen = qapp.primaryScreen() + assert screen is not None + image = screen.grabWindow(int(host.winId())).toImage() + assert not image.isNull() + return image.pixelColor(image.width() // 2, image.height() // 2) + + def submit_color(color: int, generation: int) -> None: + image = QImage(320, 180, QImage.Format.Format_RGBA8888) + image.fill(color) + frame = QVideoFrame(image) + if surface_kind == "native": + surface.update_frame( + frame, + content_generation=generation, + content_serial=1, + ) + else: + surface.set_video_frame( + frame, + {}, + content_generation=generation, + content_serial=1, + ) + + submit_color(0xFFFF0000, 1) + wait_for(presented, 1) + red_pixel = center_pixel() + assert red_pixel.red() > red_pixel.blue() + + surface.begin_presentation_transition(2) + composed_before = composed.count() + surface.update() + wait_for(composed, composed_before + 1) + transition_pixel = center_pixel() + assert transition_pixel.red() < 200 + + assert surface.complete_presentation_transition(2) + submit_color(0xFF0000FF, 2) + wait_for(presented, 2) + blue_pixel = center_pixel() + host.close() + + assert blue_pixel.blue() > blue_pixel.red() + def test_transparent_rounded_clip_toggles_widget_attributes(self, qapp): """Preview clipping should switch the renderer into transparent output mode.""" w = VideoRendererWidget() @@ -858,6 +936,55 @@ def test_renderer_uses_platform_qrhi_api(self, qapp): va = VideoArea() assert va._renderer.render_backend_name() == selected_rhi_backend_name() + def test_native_renderer_transition_is_generation_bound(self, qapp): + renderer = VideoRendererWidget() + + renderer.begin_presentation_transition(4) + + assert renderer.complete_presentation_transition(3) is False + assert renderer._presentation_suppressed_generation == 4 + assert renderer.complete_presentation_transition(4) is True + + def test_native_renderer_suppression_clears_without_drawing(self): + renderer = Mock() + renderer._initialized = True + renderer._presentation_suppressed_generation = 5 + renderer._has_frame = True + retained_frame = object() + renderer._current_frame = retained_frame + renderer.rhi.return_value = Mock() + target = Mock() + target.pixelSize.return_value = QSize(320, 240) + renderer.renderTarget.return_value = target + command_buffer = Mock() + + VideoRendererWidget.render(renderer, command_buffer) + + command_buffer.beginPass.assert_called_once() + command_buffer.setGraphicsPipeline.assert_not_called() + command_buffer.draw.assert_not_called() + assert renderer._current_frame is retained_frame + assert renderer._has_frame is True + assert renderer._rendered_content_identity is None + + def test_begin_load_suppresses_both_video_surfaces(self, qapp, mocker): + va = VideoArea() + native_suppress = mocker.patch.object( + va._renderer, + "begin_presentation_transition", + ) + adjusted_suppress = mocker.patch.object( + va._edit_viewer, + "begin_presentation_transition", + ) + expected_generation = va._media_generation + 1 + + actual_generation = va.begin_load(Path("/fake/transition.mov"), 20) + + assert actual_generation == expected_generation + native_suppress.assert_called_once_with(expected_generation) + adjusted_suppress.assert_called_once_with(expected_generation) + def test_opaque_widget_attributes(self, qapp): """VideoArea and renderer must block WA_TranslucentBackground cascade.""" va = VideoArea() From a2a34bbad14130b2c825f12e119ca71a64fd95ec Mon Sep 17 00:00:00 2001 From: Oliver Date: Sat, 5 Sep 2026 19:58:07 +0200 Subject: [PATCH 3/5] Fix Windows compositor transition blanking and expand coverage --- .github/workflows/test.yml | 5 +- docs/WINDOWS_SCAN_PLAYBACK_DIAGNOSTICS.md | 27 ++- docs/architecture.md | 4 + .../DETAIL_OPEN_BENCHMARK_RUNBOOK.md | 4 + .../ui/controllers/player_view_controller.py | 11 + src/iPhoto/gui/ui/widgets/video_area.py | 24 ++- .../test_player_view_init_cover.py | 36 ++++ .../test_gl_image_viewer_post_load_signal.py | 50 +++-- tests/ui/widgets/test_video_area.py | 203 +++++++++++++++--- 9 files changed, 301 insertions(+), 63 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 9bf9581f7..64b85db06 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -255,7 +255,7 @@ jobs: QT_QPA_PLATFORM: offscreen NUMBA_DISABLE_JIT: "1" run: >- - python -m pytest -q + python -m pytest -q -m "not windows_compositor" tests/gui/test_detail_pipeline.py tests/gui/test_detail_render_coordinator.py tests/gui/test_detail_decode_backend.py @@ -264,7 +264,10 @@ jobs: tests/gui/test_detail_surface_residency.py tests/gui/test_detail_render_session.py tests/ui/controllers/test_player_view_controller_adjustments.py + tests/ui/controllers/test_player_view_init_cover.py + tests/ui/widgets/test_gl_image_viewer_post_load_signal.py tests/ui/widgets/test_still_texture_residency.py + tests/ui/widgets/test_video_area.py tests/test_detail_benchmark.py tests/test_detail_surface_cache_benchmark.py diff --git a/docs/WINDOWS_SCAN_PLAYBACK_DIAGNOSTICS.md b/docs/WINDOWS_SCAN_PLAYBACK_DIAGNOSTICS.md index 9ae34e85f..bb792a932 100644 --- a/docs/WINDOWS_SCAN_PLAYBACK_DIAGNOSTICS.md +++ b/docs/WINDOWS_SCAN_PLAYBACK_DIAGNOSTICS.md @@ -84,12 +84,35 @@ repetition matrix can determine whether this preserves the user-visible first-frame leak fix. Transition traces must show `presentation_suppressed` before the exposed -surface's blank submission and `presentation_resumed` only after matching new -content is installed. A rapid A→B switch must record `post_submit_discarded` +surface's `video_surface_blank_requested`/blank submission and +`presentation_resumed` only after matching new content is installed. A rapid +A→B switch must record `post_submit_discarded` for any scheduled or armed A barrier, including `reason`, `state`, and the old epoch. During the suppressed interval a real compositor screenshot must contain the opaque Detail background, never A's pixels or media-specific overlays. +Run the real-pixel contracts manually from a normal interactive Windows +desktop. Setting the platform explicitly is required because the unit-test +fixtures otherwise default to `offscreen`: + +```powershell +$env:QT_QPA_PLATFORM = "windows" +$env:IPHOTO_RHI_BACKEND = "opengl" +$env:IPHOTO_WINDOWS_COMPOSITOR_CYCLES = "100" +$tests = @( + "tests/ui/widgets/test_gl_image_viewer_post_load_signal.py", + "tests/ui/widgets/test_video_area.py" +) +1..30 | ForEach-Object { + python -m pytest -q -m windows_compositor $tests + if ($LASTEXITCODE -ne 0) { throw "Compositor run $_ failed" } +} +``` + +The video pixel contract drives `VideoArea.begin_load()` followed by +`PlayerViewController.begin_video_transition()`; it must not request the blank +frame directly from the tested QRhi child. + The default timeout is 30 minutes. Override it with `-MaxMinutes 60` if the scan takes longer. The expanded directory is retained beside the ZIP so its contents can be reviewed before sharing. diff --git a/docs/architecture.md b/docs/architecture.md index 01d831597..6f3a9ec35 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -238,6 +238,10 @@ by generation before exposing its QRhi surface. Suppressed renders submit an opaque clear frame while preserving still residency; only installation of matching-generation content may resume media drawing. Media-specific overlays remain suppressed until that generation reaches its reveal terminal. +After `VideoArea.begin_load()` suppresses both video renderers, the active +surface is explicitly updated under the Detail cover so its retained backing +texture is replaced by the opaque clear frame before decode completes. Video +suppression is released only after the new frame is installed successfully. The pre-show hierarchy is part of shell construction: failure is terminal and non-recoverable for that process. Startup-generation retry applies only after a valid visible shell exists; it must not claim to reconstruct the native diff --git a/docs/requirements/DETAIL_OPEN_BENCHMARK_RUNBOOK.md b/docs/requirements/DETAIL_OPEN_BENCHMARK_RUNBOOK.md index 7cf201b52..9a5aeaa23 100644 --- a/docs/requirements/DETAIL_OPEN_BENCHMARK_RUNBOOK.md +++ b/docs/requirements/DETAIL_OPEN_BENCHMARK_RUNBOOK.md @@ -64,6 +64,10 @@ Phase 2 still 采样必须同时保留以下事件，并按 `asset_id + generati 只能由 startup terminal 之后的 hover/click 触发；不得出现在无用户交互的 startup profile 中。suppressed 区间不得产生旧 generation 的 `gpu_upload`、`presented` 或 media submission。 +- video transition 必须在 active surface 确定后记录 + `video_surface_blank_requested`；对应的 `presentation_resumed` 只能出现在 + current-generation frame 已成功安装之后。frame rotation/staging/install + 失败时 suppression 必须保持有效。 - `presented`：仍是 click-to-present 的终点；stale generation 不得产生该事件。 - 正常 Windows reveal 记录 `surface_revealed(reason=composition)`；缺少额外 composition 时允许在匹配内容已提交后记录 diff --git a/src/iPhoto/gui/ui/controllers/player_view_controller.py b/src/iPhoto/gui/ui/controllers/player_view_controller.py index 5442484e6..aa91909f2 100644 --- a/src/iPhoto/gui/ui/controllers/player_view_controller.py +++ b/src/iPhoto/gui/ui/controllers/player_view_controller.py @@ -1095,6 +1095,17 @@ def begin_video_transition( self._player_stack.setCurrentWidget(self._video_area) if not self._player_stack.isVisible(): self._player_stack.show() + request_surface_update = getattr( + self._video_area, + "request_active_surface_update", + None, + ) + if callable(request_surface_update): + request_surface_update() + emit_detail_event( + "video_surface_blank_requested", + generation=int(request_generation), + ) self._video_area.video_view().setFocus() # ------------------------------------------------------------------ diff --git a/src/iPhoto/gui/ui/widgets/video_area.py b/src/iPhoto/gui/ui/widgets/video_area.py index be555db7f..e0f30e46c 100644 --- a/src/iPhoto/gui/ui/widgets/video_area.py +++ b/src/iPhoto/gui/ui/widgets/video_area.py @@ -1340,14 +1340,6 @@ def _submit_video_frame_to_surface( ) -> None: """Queue one identified frame on exactly one QRhi child surface.""" - complete_transition = getattr( - surface, - "complete_presentation_transition", - None, - ) - if callable(complete_transition): - complete_transition(self._media_generation) - if surface is self._edit_viewer: resolved_rotation_cw = _resolve_frame_rotation_cw( frame.surfaceFormat(), @@ -1367,7 +1359,6 @@ def _submit_video_frame_to_surface( ) self._edit_viewer.set_pending_video_source_rotation(resolved_rotation_cw) reset_view = self._adjusted_first_frame_pending - self._adjusted_first_frame_pending = False self._edit_viewer.set_video_frame( frame, self._current_adjustments, @@ -1375,6 +1366,14 @@ def _submit_video_frame_to_surface( content_generation=self._media_generation, content_serial=content_serial, ) + self._adjusted_first_frame_pending = False + complete_transition = getattr( + surface, + "complete_presentation_transition", + None, + ) + if callable(complete_transition): + complete_transition(self._media_generation) self._surface_stack.update() self.update() else: @@ -1383,6 +1382,13 @@ def _submit_video_frame_to_surface( content_generation=self._media_generation, content_serial=content_serial, ) + complete_transition = getattr( + surface, + "complete_presentation_transition", + None, + ) + if callable(complete_transition): + complete_transition(self._media_generation) def _queue_retained_frame_for_surface( self, diff --git a/tests/ui/controllers/test_player_view_init_cover.py b/tests/ui/controllers/test_player_view_init_cover.py index 5abc34dbc..933575bec 100644 --- a/tests/ui/controllers/test_player_view_init_cover.py +++ b/tests/ui/controllers/test_player_view_init_cover.py @@ -1307,6 +1307,38 @@ def test_video_transition_waits_for_matching_submitted_generation( mock_show_cover.assert_called() mock_hide_cover.assert_called_once() + def test_video_transition_requests_blank_after_surface_is_active( + self, + controller, + mocker, + ): + events: list[str] = [] + mocker.patch.object( + controller, + "_show_detail_init_cover", + side_effect=lambda: events.append("cover"), + ) + + def request_blank() -> None: + assert controller._player_stack.currentWidget() is controller._video_area + assert not controller._player_stack.isHidden() + events.append("blank") + + request = mocker.patch.object( + controller._video_area, + "request_active_surface_update", + side_effect=request_blank, + ) + emit = mocker.patch( + "iPhoto.gui.ui.controllers.player_view_controller.emit_detail_event" + ) + + controller.begin_video_transition(18, interactive_when_ready=True) + + request.assert_called_once_with() + assert events == ["cover", "blank"] + emit.assert_any_call("video_surface_blank_requested", generation=18) + def test_placeholder_cancels_pending_video_transition(self, controller, mocker): mock_hide_cover = mocker.patch.object(controller, "_hide_detail_init_cover") controller.begin_video_transition(23, interactive_when_ready=True) @@ -1639,6 +1671,8 @@ def test_windows_video_reveal_waits_for_next_active_surface_composition( "set_controls_enabled", ) controller.begin_video_transition(41, interactive_when_ready=True) + request_update.assert_called_once_with() + request_update.reset_mock() controller._video_area.surfaceFrameSubmitted.emit(41, 7) controller._video_area.surfaceCompositionSubmitted.emit() @@ -1816,6 +1850,8 @@ def test_video_invalidation_replaces_deferred_windows_release_epoch( ) hide_cover = mocker.patch.object(controller, "_hide_detail_init_cover") controller.begin_video_transition(43, interactive_when_ready=False) + request_update.assert_called_once_with() + request_update.reset_mock() controller._video_area.surfaceFrameSubmitted.emit(43, 8) controller._video_area.surfaceInvalidated.emit(43, 8) diff --git a/tests/ui/widgets/test_gl_image_viewer_post_load_signal.py b/tests/ui/widgets/test_gl_image_viewer_post_load_signal.py index 8fcfa1d1f..f27331009 100644 --- a/tests/ui/widgets/test_gl_image_viewer_post_load_signal.py +++ b/tests/ui/widgets/test_gl_image_viewer_post_load_signal.py @@ -183,26 +183,40 @@ def center_pixel(): wait_for(submitted, 1) red_pixel = center_pixel() assert red_pixel.red() > red_pixel.blue() - - viewer.begin_presentation_transition(2) - composed_before = composed.count() - viewer.update() - wait_for(composed, composed_before + 1) - transition_pixel = center_pixel() - assert transition_pixel.red() < 200 - - blue = QImage(320, 180, QImage.Format.Format_RGBA8888) - blue.fill(0xFF0000FF) - viewer._still_generation_by_key["blue"] = 2 - viewer.set_image(blue, {}, image_source="blue") - assert viewer.complete_presentation_transition(2) - viewer.update() - wait_for(submitted, 2) - blue_pixel = center_pixel() + cycles = max( + 1, + min(100, int(os.environ.get("IPHOTO_WINDOWS_COMPOSITOR_CYCLES", "1"))), + ) + for index in range(cycles): + generation = index + 2 + viewer.begin_presentation_transition(generation) + composed_before = composed.count() + viewer.update() + wait_for(composed, composed_before + 1) + transition_pixel = center_pixel() + expected_background = viewer._transition_clear_color() + assert transition_pixel.alpha() == 255 + assert abs(transition_pixel.red() - expected_background.red()) <= 20 + assert abs(transition_pixel.green() - expected_background.green()) <= 20 + assert abs(transition_pixel.blue() - expected_background.blue()) <= 20 + + next_is_blue = index % 2 == 0 + color = 0xFF0000FF if next_is_blue else 0xFFFF0000 + source = f"transition-{generation}" + image = QImage(320, 180, QImage.Format.Format_RGBA8888) + image.fill(color) + viewer._still_generation_by_key[source] = generation + viewer.set_image(image, {}, image_source=source) + assert viewer.complete_presentation_transition(generation) + viewer.update() + wait_for(submitted, index + 2) + presented_pixel = center_pixel() + if next_is_blue: + assert presented_pixel.blue() > presented_pixel.red() + else: + assert presented_pixel.red() > presented_pixel.blue() host.close() - assert blue_pixel.blue() > blue_pixel.red() - def test_still_surface_retains_transaction_generation_until_gpu_upload(qapp) -> None: viewer = GLImageViewer() diff --git a/tests/ui/widgets/test_video_area.py b/tests/ui/widgets/test_video_area.py index 58b587979..bb5c42bb8 100644 --- a/tests/ui/widgets/test_video_area.py +++ b/tests/ui/widgets/test_video_area.py @@ -3,6 +3,7 @@ from __future__ import annotations import gc +import os import struct import sys import time @@ -32,6 +33,8 @@ import iPhoto.gui.ui.widgets.video_area as video_area_module import iPhoto.gui.ui.widgets.gl_texture_manager as gl_texture_manager_module from iPhoto.gui.ui.widgets.gl_image_viewer import GLImageViewer +from iPhoto.gui.ui.controllers.player_view_controller import PlayerViewController +from iPhoto.gui.ui.widgets.live_badge import LiveBadge from iPhoto.gui.ui.widgets.gl_texture_manager import TextureManager from iPhoto.gui.render_backend import selected_rhi_backend_name from iPhoto.gui.detail_pipeline import VideoPresentationState @@ -747,6 +750,7 @@ def test_visible_windows_video_transition_never_exposes_previous_frame( self, qapp, surface_kind, + mocker, ): if sys.platform != "win32": pytest.skip("requires a visible Windows compositor integration runner") @@ -756,15 +760,28 @@ def test_visible_windows_video_transition_never_exposes_previous_frame( host = QWidget() layout = QGridLayout(host) layout.setContentsMargins(0, 0, 0, 0) - surface = VideoRendererWidget(host) if surface_kind == "native" else GLImageViewer(host) - layout.addWidget(surface, 0, 0) + player_stack = QStackedWidget(host) + placeholder = QWidget(player_stack) + image_viewer = GLImageViewer(player_stack) + video_area = VideoArea(player_stack) + player_stack.addWidget(placeholder) + player_stack.addWidget(image_viewer) + player_stack.addWidget(video_area) + player_stack.setCurrentWidget(video_area) + live_badge = LiveBadge(host) + controller = PlayerViewController( + player_stack, + image_viewer, + video_area, + placeholder, + live_badge, + ) + mocker.patch.object(video_area._player, "setSource") + layout.addWidget(player_stack, 0, 0) host.resize(320, 180) host.show() qapp.processEvents() - presented = QSignalSpy(surface.videoFramePresented) - composed = QSignalSpy(surface.frameSubmitted) - def wait_for(spy: QSignalSpy, count: int) -> None: deadline = time.monotonic() + 5.0 while spy.count() < count and time.monotonic() < deadline: @@ -779,44 +796,61 @@ def center_pixel(): assert not image.isNull() return image.pixelColor(image.width() // 2, image.height() // 2) - def submit_color(color: int, generation: int) -> None: + def submit_color(color: int, content_serial: int) -> None: image = QImage(320, 180, QImage.Format.Format_RGBA8888) image.fill(color) frame = QVideoFrame(image) - if surface_kind == "native": - surface.update_frame( - frame, - content_generation=generation, - content_serial=1, - ) - else: - surface.set_video_frame( - frame, - {}, - content_generation=generation, - content_serial=1, - ) + video_area._submit_video_frame_to_surface( + frame, + video_area.video_view(), + content_serial=content_serial, + ) + video_area.begin_load(Path("/fake/video-a.mov"), 1) + video_area.set_adjusted_preview_enabled(surface_kind == "adjusted") + surface = video_area.video_view() + presented = QSignalSpy(surface.videoFramePresented) + composed = QSignalSpy(surface.frameSubmitted) submit_color(0xFFFF0000, 1) wait_for(presented, 1) red_pixel = center_pixel() assert red_pixel.red() > red_pixel.blue() - - surface.begin_presentation_transition(2) - composed_before = composed.count() - surface.update() - wait_for(composed, composed_before + 1) - transition_pixel = center_pixel() - assert transition_pixel.red() < 200 - - assert surface.complete_presentation_transition(2) - submit_color(0xFF0000FF, 2) - wait_for(presented, 2) - blue_pixel = center_pixel() + cycles = max( + 1, + min(100, int(os.environ.get("IPHOTO_WINDOWS_COMPOSITOR_CYCLES", "1"))), + ) + for index in range(cycles): + request_generation = index + 2 + video_area.begin_load( + Path(f"/fake/video-{request_generation}.mov"), + request_generation, + ) + video_area.set_adjusted_preview_enabled(surface_kind == "adjusted") + assert video_area.video_view() is surface + composed_before = composed.count() + controller.begin_video_transition( + request_generation, + interactive_when_ready=True, + ) + wait_for(composed, composed_before + 1) + transition_pixel = center_pixel() + expected_background = surface._transition_clear_color() + assert transition_pixel.alpha() == 255 + assert abs(transition_pixel.red() - expected_background.red()) <= 20 + assert abs(transition_pixel.green() - expected_background.green()) <= 20 + assert abs(transition_pixel.blue() - expected_background.blue()) <= 20 + + next_is_blue = index % 2 == 0 + submit_color(0xFF0000FF if next_is_blue else 0xFFFF0000, 1) + wait_for(presented, index + 2) + presented_pixel = center_pixel() + if next_is_blue: + assert presented_pixel.blue() > presented_pixel.red() + else: + assert presented_pixel.red() > presented_pixel.blue() + controller.shutdown(timeout_ms=500) host.close() - assert blue_pixel.blue() > blue_pixel.red() - def test_transparent_rounded_clip_toggles_widget_attributes(self, qapp): """Preview clipping should switch the renderer into transparent output mode.""" w = VideoRendererWidget() @@ -1661,6 +1695,109 @@ def test_begin_load_never_probes_on_gui_thread(self, qapp, mocker): assert va._end_detection_armed_media_generation is None assert mock_set_rot.call_args_list[-1] == call(90, 1920, 1440) + @pytest.mark.parametrize("surface_kind", ("native", "adjusted")) + def test_video_frame_is_installed_before_suppression_completes( + self, + qapp, + mocker, + surface_kind, + ): + va = VideoArea() + va._media_generation = 9 + image = QImage(64, 48, QImage.Format.Format_RGBA8888) + image.fill(0xFF123456) + frame = QVideoFrame(image) + surface = va._renderer if surface_kind == "native" else va._edit_viewer + calls = mocker.Mock() + complete = mocker.patch.object(surface, "complete_presentation_transition") + calls.attach_mock(complete, "complete") + + if surface_kind == "native": + install = mocker.patch.object(surface, "update_frame") + calls.attach_mock(install, "install") + else: + mocker.patch( + "iPhoto.gui.ui.widgets.video_area._resolve_frame_rotation_cw", + return_value=0, + ) + mocker.patch.object(surface, "set_pending_video_source_rotation") + install = mocker.patch.object(surface, "set_video_frame") + calls.attach_mock(install, "install") + + va._submit_video_frame_to_surface(frame, surface, content_serial=3) + + assert [item[0] for item in calls.mock_calls] == ["install", "complete"] + complete.assert_called_once_with(9) + + def test_native_frame_install_failure_keeps_suppression(self, qapp, mocker): + va = VideoArea() + va._media_generation = 10 + surface = va._renderer + surface.begin_presentation_transition(10) + complete = mocker.patch.object( + surface, + "complete_presentation_transition", + wraps=surface.complete_presentation_transition, + ) + mocker.patch.object( + surface, + "update_frame", + side_effect=RuntimeError("native install failed"), + ) + frame = QVideoFrame(QImage(64, 48, QImage.Format.Format_RGBA8888)) + + with pytest.raises(RuntimeError, match="native install failed"): + va._submit_video_frame_to_surface(frame, surface, content_serial=4) + + complete.assert_not_called() + assert surface._presentation_suppressed_generation == 10 + assert surface._rendered_content_identity is None + + @pytest.mark.parametrize("failure_stage", ("rotation", "install")) + def test_adjusted_frame_failure_keeps_suppression( + self, + qapp, + mocker, + failure_stage, + ): + va = VideoArea() + va._media_generation = 11 + va._adjusted_first_frame_pending = True + surface = va._edit_viewer + surface.begin_presentation_transition(11) + complete = mocker.patch.object( + surface, + "complete_presentation_transition", + wraps=surface.complete_presentation_transition, + ) + if failure_stage == "rotation": + mocker.patch( + "iPhoto.gui.ui.widgets.video_area._resolve_frame_rotation_cw", + side_effect=RuntimeError("rotation failed"), + ) + else: + mocker.patch( + "iPhoto.gui.ui.widgets.video_area._resolve_frame_rotation_cw", + return_value=0, + ) + mocker.patch.object(surface, "set_pending_video_source_rotation") + mocker.patch.object( + surface, + "set_video_frame", + side_effect=RuntimeError("install failed"), + ) + image = QImage(64, 48, QImage.Format.Format_RGBA8888) + image.fill(0xFF654321) + frame = QVideoFrame(image) + + with pytest.raises(RuntimeError): + va._submit_video_frame_to_surface(frame, surface, content_serial=5) + + complete.assert_not_called() + assert surface._presentation_suppressed_generation == 11 + assert surface._rendered_content_identity is None + assert va._adjusted_first_frame_pending is True + def test_load_video_handles_probe_failure(self, qapp, mocker): """load_video should still work when ffprobe returns no rotation.""" va = VideoArea() From 8faa18361c756330dd0bf14c4396c1f49a4ea485 Mon Sep 17 00:00:00 2001 From: Oliver Date: Sat, 5 Sep 2026 20:57:05 +0200 Subject: [PATCH 4/5] fix: blank async adjusted video transitions --- docs/WINDOWS_SCAN_PLAYBACK_DIAGNOSTICS.md | 4 ++ docs/architecture.md | 3 + .../DETAIL_OPEN_BENCHMARK_RUNBOOK.md | 4 +- .../gui/ui/widgets/gl_image_viewer/widget.py | 5 ++ src/iPhoto/gui/ui/widgets/video_area.py | 14 ++++ .../gui/ui/widgets/video_renderer_widget.py | 5 ++ .../test_player_view_init_cover.py | 3 + .../test_gl_image_viewer_post_load_signal.py | 2 + tests/ui/widgets/test_video_area.py | 72 ++++++++++++++++++- 9 files changed, 108 insertions(+), 4 deletions(-) diff --git a/docs/WINDOWS_SCAN_PLAYBACK_DIAGNOSTICS.md b/docs/WINDOWS_SCAN_PLAYBACK_DIAGNOSTICS.md index bb792a932..3bcbb57e6 100644 --- a/docs/WINDOWS_SCAN_PLAYBACK_DIAGNOSTICS.md +++ b/docs/WINDOWS_SCAN_PLAYBACK_DIAGNOSTICS.md @@ -90,6 +90,10 @@ A→B switch must record `post_submit_discarded` for any scheduled or armed A barrier, including `reason`, `state`, and the old epoch. During the suppressed interval a real compositor screenshot must contain the opaque Detail background, never A's pixels or media-specific overlays. +When async preparation changes the active video renderer, the trace must add a +second `video_surface_blank_requested` with `reason=surface_switch`, the current +Detail generation, the current media generation, and `surface=adjusted|native`. +Its matching `presentation_resumed` must follow successful frame installation. Run the real-pixel contracts manually from a normal interactive Windows desktop. Setting the platform explicitly is required because the unit-test diff --git a/docs/architecture.md b/docs/architecture.md index 6f3a9ec35..01a742978 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -242,6 +242,9 @@ After `VideoArea.begin_load()` suppresses both video renderers, the active surface is explicitly updated under the Detail cover so its retained backing texture is replaced by the opaque clear frame before decode completes. Video suppression is released only after the new frame is installed successfully. +If asynchronous presentation preparation switches native/adjusted renderers, +the newly active renderer submits its own suppressed clear frame before any new +content is installed. The pre-show hierarchy is part of shell construction: failure is terminal and non-recoverable for that process. Startup-generation retry applies only after a valid visible shell exists; it must not claim to reconstruct the native diff --git a/docs/requirements/DETAIL_OPEN_BENCHMARK_RUNBOOK.md b/docs/requirements/DETAIL_OPEN_BENCHMARK_RUNBOOK.md index 9a5aeaa23..befd31c0c 100644 --- a/docs/requirements/DETAIL_OPEN_BENCHMARK_RUNBOOK.md +++ b/docs/requirements/DETAIL_OPEN_BENCHMARK_RUNBOOK.md @@ -67,7 +67,9 @@ Phase 2 still 采样必须同时保留以下事件，并按 `asset_id + generati - video transition 必须在 active surface 确定后记录 `video_surface_blank_requested`；对应的 `presentation_resumed` 只能出现在 current-generation frame 已成功安装之后。frame rotation/staging/install - 失败时 suppression 必须保持有效。 + 失败时 suppression 必须保持有效。异步 preparation 切换 native/adjusted + renderer 时必须再记录 `reason=surface_switch`、当前 media generation 和最终 + surface；正常非 transition 切换不得产生该事件。 - `presented`：仍是 click-to-present 的终点；stale generation 不得产生该事件。 - 正常 Windows reveal 记录 `surface_revealed(reason=composition)`；缺少额外 composition 时允许在匹配内容已提交后记录 diff --git a/src/iPhoto/gui/ui/widgets/gl_image_viewer/widget.py b/src/iPhoto/gui/ui/widgets/gl_image_viewer/widget.py index d01c43d28..ca7607c76 100644 --- a/src/iPhoto/gui/ui/widgets/gl_image_viewer/widget.py +++ b/src/iPhoto/gui/ui/widgets/gl_image_viewer/widget.py @@ -448,6 +448,11 @@ def complete_presentation_transition(self, generation: int) -> bool: ) return True + def presentation_transition_active(self, generation: int) -> bool: + """Return whether this generation still owns presentation suppression.""" + + return int(generation) == self._presentation_suppressed_generation + def cancel_presentation_transition(self) -> None: """Drop presentation suppression without changing texture residency.""" diff --git a/src/iPhoto/gui/ui/widgets/video_area.py b/src/iPhoto/gui/ui/widgets/video_area.py index e0f30e46c..7affdaf6b 100644 --- a/src/iPhoto/gui/ui/widgets/video_area.py +++ b/src/iPhoto/gui/ui/widgets/video_area.py @@ -466,6 +466,20 @@ def set_adjusted_preview_enabled(self, enabled: bool) -> None: self._edit_viewer.update() else: self._edit_mode_active = False + transition_active = getattr( + target_surface, + "presentation_transition_active", + None, + ) + if callable(transition_active) and transition_active(self._media_generation): + target_surface.update() + emit_detail_event( + "video_surface_blank_requested", + generation=self._detail_request_generation, + media_generation=self._media_generation, + reason="surface_switch", + surface="adjusted" if target_surface is self._edit_viewer else "native", + ) if required_serial > 0 and self._last_presented_video_frame is not None: self._queue_retained_frame_for_surface( target_surface, diff --git a/src/iPhoto/gui/ui/widgets/video_renderer_widget.py b/src/iPhoto/gui/ui/widgets/video_renderer_widget.py index a89517628..bb2051120 100644 --- a/src/iPhoto/gui/ui/widgets/video_renderer_widget.py +++ b/src/iPhoto/gui/ui/widgets/video_renderer_widget.py @@ -414,6 +414,11 @@ def complete_presentation_transition(self, generation: int) -> bool: ) return True + def presentation_transition_active(self, generation: int) -> bool: + """Return whether this generation still owns presentation suppression.""" + + return int(generation) == self._presentation_suppressed_generation + def cancel_presentation_transition(self) -> None: """Cancel suppression without destroying QRhi resources.""" diff --git a/tests/ui/controllers/test_player_view_init_cover.py b/tests/ui/controllers/test_player_view_init_cover.py index 933575bec..e15491741 100644 --- a/tests/ui/controllers/test_player_view_init_cover.py +++ b/tests/ui/controllers/test_player_view_init_cover.py @@ -1161,6 +1161,7 @@ def test_image_transition_waits_for_matching_content_submission( controller, mocker, ): + controller._requires_post_submit_frame = False mock_show_cover = mocker.patch.object(controller, "_show_detail_init_cover") mock_hide_cover = mocker.patch.object(controller, "_hide_detail_init_cover") controller._image_viewer_rendered = True @@ -1284,6 +1285,7 @@ def test_video_transition_waits_for_matching_submitted_generation( controller, mocker, ): + controller._requires_post_submit_frame = False mock_show_cover = mocker.patch.object(controller, "_show_detail_init_cover") mock_hide_cover = mocker.patch.object(controller, "_hide_detail_init_cover") controls_enabled = mocker.patch.object( @@ -1365,6 +1367,7 @@ def test_latest_surface_submission_completes_rearmed_switch_barrier( controller, mocker, ): + controller._requires_post_submit_frame = False mock_hide_cover = mocker.patch.object(controller, "_hide_detail_init_cover") controller._player_stack.setCurrentWidget(controller._video_area) diff --git a/tests/ui/widgets/test_gl_image_viewer_post_load_signal.py b/tests/ui/widgets/test_gl_image_viewer_post_load_signal.py index f27331009..43be9a7db 100644 --- a/tests/ui/widgets/test_gl_image_viewer_post_load_signal.py +++ b/tests/ui/widgets/test_gl_image_viewer_post_load_signal.py @@ -86,6 +86,8 @@ def test_presentation_transition_is_generation_bound_and_preserves_texture(qapp) assert viewer.current_image_source() == "old-still" assert viewer.has_image_content() is True + assert viewer.presentation_transition_active(7) is False + assert viewer.presentation_transition_active(8) is True assert viewer.complete_presentation_transition(7) is False assert viewer._presentation_suppressed_generation == 8 assert viewer.complete_presentation_transition(8) is True diff --git a/tests/ui/widgets/test_video_area.py b/tests/ui/widgets/test_video_area.py index bb5c42bb8..45356bd99 100644 --- a/tests/ui/widgets/test_video_area.py +++ b/tests/ui/widgets/test_video_area.py @@ -806,8 +806,22 @@ def submit_color(color: int, content_serial: int) -> None: content_serial=content_serial, ) + def commit_state(generation: int, adjusted: bool) -> None: + assert video_area.commit_presentation( + VideoPresentationState( + generation, + {}, + None, + adjusted, + 0, + 0, + 0, + False, + ) + ) + video_area.begin_load(Path("/fake/video-a.mov"), 1) - video_area.set_adjusted_preview_enabled(surface_kind == "adjusted") + commit_state(1, surface_kind == "adjusted") surface = video_area.video_view() presented = QSignalSpy(surface.videoFramePresented) composed = QSignalSpy(surface.frameSubmitted) @@ -825,13 +839,13 @@ def submit_color(color: int, content_serial: int) -> None: Path(f"/fake/video-{request_generation}.mov"), request_generation, ) - video_area.set_adjusted_preview_enabled(surface_kind == "adjusted") - assert video_area.video_view() is surface composed_before = composed.count() controller.begin_video_transition( request_generation, interactive_when_ready=True, ) + commit_state(request_generation, surface_kind == "adjusted") + assert video_area.video_view() is surface wait_for(composed, composed_before + 1) transition_pixel = center_pixel() expected_background = surface._transition_clear_color() @@ -975,6 +989,8 @@ def test_native_renderer_transition_is_generation_bound(self, qapp): renderer.begin_presentation_transition(4) + assert renderer.presentation_transition_active(3) is False + assert renderer.presentation_transition_active(4) is True assert renderer.complete_presentation_transition(3) is False assert renderer._presentation_suppressed_generation == 4 assert renderer.complete_presentation_transition(4) is True @@ -1019,6 +1035,56 @@ def test_begin_load_suppresses_both_video_surfaces(self, qapp, mocker): native_suppress.assert_called_once_with(expected_generation) adjusted_suppress.assert_called_once_with(expected_generation) + @pytest.mark.parametrize("target_kind", ("native", "adjusted")) + def test_suppressed_surface_switch_requests_target_blank( + self, + qapp, + mocker, + target_kind, + ): + va = VideoArea() + va._media_generation = 12 + va._detail_request_generation = 22 + va._current_duration_ms = 0 + mocker.patch.object(va._edit_viewer, "set_adjustments") + if target_kind == "native": + va._adjusted_preview_enabled = True + va._surface_stack.setCurrentWidget(va._edit_viewer) + target_surface = va._renderer + else: + va._adjusted_preview_enabled = False + va._surface_stack.setCurrentWidget(va._renderer) + target_surface = va._edit_viewer + target_surface.begin_presentation_transition(12) + update = mocker.patch.object(target_surface, "update") + emit = mocker.patch( + "iPhoto.gui.ui.widgets.video_area.emit_detail_event" + ) + + va.set_adjusted_preview_enabled(target_kind == "adjusted") + + assert va.video_view() is target_surface + update.assert_called_once_with() + emit.assert_any_call( + "video_surface_blank_requested", + generation=22, + media_generation=12, + reason="surface_switch", + surface=target_kind, + ) + + def test_completed_surface_switch_does_not_request_blank(self, qapp, mocker): + va = VideoArea() + va._media_generation = 13 + va._current_duration_ms = 0 + mocker.patch.object(va._edit_viewer, "set_adjustments") + update = mocker.patch.object(va._edit_viewer, "update") + + va.set_adjusted_preview_enabled(True) + + assert va.video_view() is va._edit_viewer + update.assert_not_called() + def test_opaque_widget_attributes(self, qapp): """VideoArea and renderer must block WA_TranslucentBackground cascade.""" va = VideoArea() From 088d9d4581f0bd5efbecce81ec86bd6d8335f09d Mon Sep 17 00:00:00 2001 From: Oliver Date: Sun, 6 Sep 2026 13:41:34 +0200 Subject: [PATCH 5/5] Preserve generation-bound playback transitions across retries --- docs/WINDOWS_SCAN_PLAYBACK_DIAGNOSTICS.md | 4 + docs/architecture.md | 7 +- .../DETAIL_OPEN_BENCHMARK_RUNBOOK.md | 5 +- .../gui/coordinators/playback_coordinator.py | 12 +- .../ui/controllers/player_view_controller.py | 20 ++- .../gui/ui/widgets/gl_image_viewer/widget.py | 145 ++++++++++++++++-- src/iPhoto/gui/ui/widgets/video_area.py | 14 -- .../gui/ui/widgets/video_renderer_widget.py | 52 ++++++- .../coordinators/test_playback_coordinator.py | 4 + ...test_player_view_controller_adjustments.py | 2 +- .../test_player_view_init_cover.py | 42 +++++ .../test_gl_image_viewer_post_load_signal.py | 139 +++++++++++++++++ tests/ui/widgets/test_video_area.py | 131 +++++++++++++++- 13 files changed, 518 insertions(+), 59 deletions(-) diff --git a/docs/WINDOWS_SCAN_PLAYBACK_DIAGNOSTICS.md b/docs/WINDOWS_SCAN_PLAYBACK_DIAGNOSTICS.md index 3bcbb57e6..15f379777 100644 --- a/docs/WINDOWS_SCAN_PLAYBACK_DIAGNOSTICS.md +++ b/docs/WINDOWS_SCAN_PLAYBACK_DIAGNOSTICS.md @@ -103,6 +103,7 @@ fixtures otherwise default to `offscreen`: $env:QT_QPA_PLATFORM = "windows" $env:IPHOTO_RHI_BACKEND = "opengl" $env:IPHOTO_WINDOWS_COMPOSITOR_CYCLES = "100" +$env:IPHOTO_WINDOWS_COMPOSITOR_FORCE_UPLOAD_FAILURE = "1" $tests = @( "tests/ui/widgets/test_gl_image_viewer_post_load_signal.py", "tests/ui/widgets/test_video_area.py" @@ -116,6 +117,9 @@ $tests = @( The video pixel contract drives `VideoArea.begin_load()` followed by `PlayerViewController.begin_video_transition()`; it must not request the blank frame directly from the tested QRhi child. +Forced first-upload failures must record `video_gpu_upload_retry`, remain on the +opaque background, and emit `presentation_resumed` only after the retained +current-generation frame uploads and draws successfully on retry. The default timeout is 30 minutes. Override it with `-MaxMinutes 60` if the scan takes longer. The expanded directory is retained beside the ZIP so its contents can be reviewed before diff --git a/docs/architecture.md b/docs/architecture.md index 01a742978..557fbea61 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -238,10 +238,15 @@ by generation before exposing its QRhi surface. Suppressed renders submit an opaque clear frame while preserving still residency; only installation of matching-generation content may resume media drawing. Media-specific overlays remain suppressed until that generation reaches its reveal terminal. +Live Photo stills decoded while motion is active retain their request +generation and re-enter the same image transition/reveal terminal when motion +ends, so generation-bound overlays cannot remain deferred indefinitely. After `VideoArea.begin_load()` suppresses both video renderers, the active surface is explicitly updated under the Detail cover so its retained backing texture is replaced by the opaque clear frame before decode completes. Video -suppression is released only after the new frame is installed successfully. +suppression is released only after the new frame's GPU upload and draw are +recorded successfully. Upload failure keeps the retryable CPU frame and submits +only the opaque clear surface. If asynchronous presentation preparation switches native/adjusted renderers, the newly active renderer submits its own suppressed clear frame before any new content is installed. diff --git a/docs/requirements/DETAIL_OPEN_BENCHMARK_RUNBOOK.md b/docs/requirements/DETAIL_OPEN_BENCHMARK_RUNBOOK.md index befd31c0c..5dcfb4142 100644 --- a/docs/requirements/DETAIL_OPEN_BENCHMARK_RUNBOOK.md +++ b/docs/requirements/DETAIL_OPEN_BENCHMARK_RUNBOOK.md @@ -66,8 +66,9 @@ Phase 2 still 采样必须同时保留以下事件，并按 `asset_id + generati `gpu_upload`、`presented` 或 media submission。 - video transition 必须在 active surface 确定后记录 `video_surface_blank_requested`；对应的 `presentation_resumed` 只能出现在 - current-generation frame 已成功安装之后。frame rotation/staging/install - 失败时 suppression 必须保持有效。异步 preparation 切换 native/adjusted + current-generation frame 的 GPU upload 和 draw 均成功之后。GPU upload + 失败必须记录 `video_gpu_upload_retry`、保留 retryable frame 并只提交 opaque + clear。frame rotation/staging/install 失败时 suppression 必须保持有效。异步 preparation 切换 native/adjusted renderer 时必须再记录 `reason=surface_switch`、当前 media generation 和最终 surface；正常非 transition 切换不得产生该事件。 - `presented`：仍是 click-to-present 的终点；stale generation 不得产生该事件。 diff --git a/src/iPhoto/gui/coordinators/playback_coordinator.py b/src/iPhoto/gui/coordinators/playback_coordinator.py index 5d67599d9..dd7befbac 100644 --- a/src/iPhoto/gui/coordinators/playback_coordinator.py +++ b/src/iPhoto/gui/coordinators/playback_coordinator.py @@ -1555,12 +1555,18 @@ def _restore_live_still(self, *, stop_motion: bool = False) -> bool: still = self._active_live_still asset_id = self._active_live_asset_id transaction = getattr(self, "_detail_render_transaction", None) + badge_generation = ( + transaction.generation + if transaction is not None + else getattr(self, "_detail_request_generation", 0) + ) if stop_motion: self._player_view.video_area.stop() self._active_live_motion = None self._active_live_still = None self._active_live_asset_id = "" self._active_live_media_generation = None + self._player_view.defer_live_badge_until_ready(badge_generation) self._player_view.defer_still_updates(False) if not self._player_view.apply_pending_still(): if ( @@ -1575,12 +1581,6 @@ def _restore_live_still(self, *, stop_motion: bool = False) -> bool: else: self._player_view.display_image(still, asset_id=asset_id) self._player_bar.setEnabled(False) - badge_generation = ( - transaction.generation - if transaction is not None - else getattr(self, "_detail_request_generation", 0) - ) - self._player_view.defer_live_badge_until_ready(badge_generation) self._player_view.set_live_replay_enabled(True) self._is_playing = False return True diff --git a/src/iPhoto/gui/ui/controllers/player_view_controller.py b/src/iPhoto/gui/ui/controllers/player_view_controller.py index aa91909f2..a37111243 100644 --- a/src/iPhoto/gui/ui/controllers/player_view_controller.py +++ b/src/iPhoto/gui/ui/controllers/player_view_controller.py @@ -475,7 +475,7 @@ def __init__( self._loading_source: Path | None = None self._loading_started_at: float | None = None self._defer_still_updates = False - self._pending_still: tuple[DecodedSurface, dict] | None = None + self._pending_still: tuple[int, DecodedSurface, dict] | None = None self._current_full_image: QImage | None = None self._render_sessions: OrderedDict[tuple, PhotoRenderSessionHandle] = OrderedDict() self._current_render_session: PhotoRenderSessionHandle | None = None @@ -1550,7 +1550,11 @@ def _dispatch_prepared_intent( self._present_generation = request.generation self._present_started_at = self._loading_started_at self._present_source = request.source_identity.path - self._pending_still = (deferred_surface, render_adjustments) + self._pending_still = ( + request.generation, + deferred_surface, + render_adjustments, + ) self._loading_source = None self._loading_started_at = None return True @@ -2540,8 +2544,12 @@ def apply_pending_still(self) -> bool: """Apply any deferred still frame if available.""" if self._pending_still is None: return False - surface, adjustments = self._pending_still + generation, surface, adjustments = self._pending_still self._pending_still = None + if generation != self._request_generation: + return False + self._present_generation = generation + self._begin_image_transition(generation) self._apply_still_frame(surface, adjustments) return True @@ -2652,7 +2660,11 @@ def _on_adjusted_image_ready( return if self._defer_still_updates and self._player_stack.currentWidget() is self._video_area: - self._pending_still = (surface, adjustments) + self._pending_still = ( + self._present_generation, + surface, + adjustments, + ) else: self._apply_still_frame( surface, diff --git a/src/iPhoto/gui/ui/widgets/gl_image_viewer/widget.py b/src/iPhoto/gui/ui/widgets/gl_image_viewer/widget.py index ca7607c76..0e9323b61 100644 --- a/src/iPhoto/gui/ui/widgets/gl_image_viewer/widget.py +++ b/src/iPhoto/gui/ui/widgets/gl_image_viewer/widget.py @@ -14,6 +14,7 @@ import weakref from collections import OrderedDict from collections.abc import Mapping +from dataclasses import dataclass from typing import Any from PySide6.QtCore import QPointF, QRectF, QSize, Qt, QTimer, Signal @@ -139,6 +140,12 @@ def _load_video_frame_types() -> None: GLRenderer: Any | None = None +@dataclass(frozen=True, slots=True) +class _PendingVideoUploadState: + pre_rotated: bool + final_rotation: int + + def _crop_preview_adjustments(adjustments: Mapping[str, float]) -> dict[str, float]: """Return adjustments that expose the full transformed source in Crop mode.""" @@ -471,6 +478,27 @@ def cancel_presentation_transition(self) -> None: def _presentation_is_suppressed(self) -> bool: return getattr(self, "_presentation_suppressed_generation", None) is not None + def _suppressed_video_upload_generation(self) -> int | None: + generation = getattr(self, "_presentation_suppressed_generation", None) + if ( + generation is None + or not self._using_video_frame_source + or not self._video_frame_dirty + or self._video_frame_content_generation != generation + or (self._video_frame is None and self._pending_video_image is None) + ): + return None + return int(generation) + + def _emit_video_gpu_upload_retry(self, generation: int, stage: str) -> None: + emit_detail_event( + "video_gpu_upload_retry", + generation=int(generation), + renderer="gl_image_viewer", + backend=self.render_backend_name(), + failure_stage=stage, + ) + def render_device_name(self) -> str: """Return the QRhi adapter name used to reject software benchmark runs.""" @@ -929,11 +957,11 @@ def _is_image_snapshot_prerotated(frame, image: QImage) -> bool: height = int(fmt.frameHeight()) return width > 0 and height > 0 and image.width() == height and image.height() == width - def _upload_pending_video_source(self) -> bool: - """Upload pending video source and return whether snapshot path was pre-rotated.""" + def _stage_pending_video_source(self) -> _PendingVideoUploadState | None: + """Upload/stage a video source while retaining retryable input state.""" if self._renderer is None: - return False + return None pending_rotation = self._pending_source_rotate90_steps if pending_rotation is None: @@ -943,30 +971,49 @@ def _upload_pending_video_source(self) -> bool: if self._pending_video_image is not None: self._renderer.upload_texture(self._pending_video_image) pre_rotated = self._pending_video_image_pre_rotated - self._pending_video_image = None - self._pending_video_image_pre_rotated = False - self._video_frame = None elif self._video_frame is not None: self._renderer.upload_video_frame(self._video_frame) pre_rotated = self._renderer.last_video_upload_pre_rotated() else: - return False + return None final_rotation = 0 if pre_rotated else pending_rotation self._apply_video_source_rotation_steps( final_rotation, request_update=False, ) - self._pending_source_rotate90_steps = None - self._video_frame = None - self._video_frame_dirty = False - self._video_frame_presentation_pending = True straighten, rotate_steps, _ = self._rotation_parameters() self._update_cover_scale(straighten, rotate_steps) if self._pending_video_reset_view: - self._pending_video_reset_view = False self.reset_zoom() - return pre_rotated + return _PendingVideoUploadState( + pre_rotated=pre_rotated, + final_rotation=final_rotation, + ) + + def _commit_pending_video_source( + self, + state: _PendingVideoUploadState, + ) -> None: + """Consume staged input only after its new texture was drawn successfully.""" + + del state + self._pending_video_image = None + self._pending_video_image_pre_rotated = False + self._pending_source_rotate90_steps = None + self._video_frame = None + self._video_frame_dirty = False + self._video_frame_presentation_pending = True + self._pending_video_reset_view = False + + def _upload_pending_video_source(self) -> bool: + """Upload and consume a video source outside a suppressed transition.""" + + state = self._stage_pending_video_source() + if state is None: + return False + self._commit_pending_video_source(state) + return state.pre_rotated def _upload_video_frame_immediately_if_possible(self) -> None: """Best-effort immediate Linux upload for edit-preview video frames. @@ -980,6 +1027,8 @@ def _upload_video_frame_immediately_if_possible(self) -> None: if not sys.platform.startswith("linux"): return + if GLImageViewer._presentation_is_suppressed(self): + return if not self._using_video_frame_source or not self._video_frame_dirty: return if ( @@ -1697,7 +1746,11 @@ def render(self, cb) -> None: # type: ignore[override] return self._last_render_target_size = QSize(output_size) - if GLImageViewer._presentation_is_suppressed(self): + suppressed = GLImageViewer._presentation_is_suppressed(self) + suppressed_video_generation = ( + GLImageViewer._suppressed_video_upload_generation(self) + ) + if suppressed and suppressed_video_generation is None: cb.beginPass( self.renderTarget(), self._transition_clear_color(), @@ -1730,6 +1783,7 @@ def render(self, cb) -> None: # type: ignore[override] gf.glClear(gl_module.GL_COLOR_BUFFER_BIT) uploaded_new_still_texture = False + staged_video_upload: _PendingVideoUploadState | None = None if self._pending_resident_activation is not None: key = self._pending_resident_activation self._pending_resident_activation = None @@ -1755,7 +1809,13 @@ def render(self, cb) -> None: # type: ignore[override] self._diag_video_frame_summary(self._video_frame), ) try: - pre_rotated = self._upload_pending_video_source() + if suppressed_video_generation is not None: + staged_video_upload = self._stage_pending_video_source() + if staged_video_upload is None: + raise RuntimeError("No matching video source available for upload") + pre_rotated = staged_video_upload.pre_rotated + else: + pre_rotated = self._upload_pending_video_source() if sys.platform.startswith("linux") and self._should_log_diag_frame(self._diag_video_render_count): logical_tex_w, logical_tex_h = self._display_texture_dimensions() _LOGGER.warning( @@ -1772,6 +1832,16 @@ def render(self, cb) -> None: # type: ignore[override] ) except Exception: _LOGGER.exception("Failed to upload video frame into GLImageViewer") + if suppressed_video_generation is not None: + self._emit_video_gpu_upload_retry( + suppressed_video_generation, + "upload", + ) + cb.endExternal() + cb.endPass() + self._queue_first_frame_ready() + self._rendered_content_identity = None + return elif ( self._image is not None and not self._image.isNull() @@ -1825,6 +1895,11 @@ def render(self, cb) -> None: # type: ignore[override] cb.endPass() self._queue_first_frame_ready() self._rendered_content_identity = None + if suppressed_video_generation is not None: + self._emit_video_gpu_upload_retry( + suppressed_video_generation, + "no_texture", + ) return effective_scale = self._transform_controller.get_effective_scale() @@ -1902,6 +1977,9 @@ def render(self, cb) -> None: # type: ignore[override] # --- End raw OpenGL block --- cb.endExternal() cb.endPass() + if staged_video_upload is not None and suppressed_video_generation is not None: + self._commit_pending_video_source(staged_video_upload) + self.complete_presentation_transition(suppressed_video_generation) self._queue_first_frame_ready() rendered_identity = self._take_pending_content_submission() if rendered_identity is not None: @@ -1932,7 +2010,11 @@ def _render_rhi(self, cb) -> None: return self._last_render_target_size = QSize(output_size) - if GLImageViewer._presentation_is_suppressed(self): + suppressed = GLImageViewer._presentation_is_suppressed(self) + suppressed_video_generation = ( + GLImageViewer._suppressed_video_upload_generation(self) + ) + if suppressed and suppressed_video_generation is None: cb.beginPass( self.renderTarget(), self._transition_clear_color(), @@ -1947,6 +2029,7 @@ def _render_rhi(self, cb) -> None: vh = max(1, output_size.height()) uploaded_new_still_texture = False + staged_video_upload: _PendingVideoUploadState | None = None if self._pending_resident_activation is not None: key = self._pending_resident_activation self._pending_resident_activation = None @@ -1959,9 +2042,28 @@ def _render_rhi(self, cb) -> None: ): self._diag_video_render_count += 1 try: - self._upload_pending_video_source() + if suppressed_video_generation is not None: + staged_video_upload = self._stage_pending_video_source() + if staged_video_upload is None: + raise RuntimeError("No matching video source available for upload") + else: + self._upload_pending_video_source() except Exception: _LOGGER.exception("Failed to upload video frame into QRhi image viewer") + if suppressed_video_generation is not None: + self._emit_video_gpu_upload_retry( + suppressed_video_generation, + "upload", + ) + cb.beginPass( + self.renderTarget(), + self._transition_clear_color(), + QRhiDepthStencilClearValue(), + ) + cb.endPass() + self._queue_first_frame_ready() + self._rendered_content_identity = None + return elif ( self._image is not None and not self._image.isNull() @@ -2007,6 +2109,11 @@ def _render_rhi(self, cb) -> None: cb.endPass() self._queue_first_frame_ready() self._rendered_content_identity = None + if suppressed_video_generation is not None: + self._emit_video_gpu_upload_retry( + suppressed_video_generation, + "no_texture", + ) return effective_scale = self._transform_controller.get_effective_scale() @@ -2051,6 +2158,10 @@ def _render_rhi(self, cb) -> None: crop_faded=crop_faded, ) + if staged_video_upload is not None and suppressed_video_generation is not None: + self._commit_pending_video_source(staged_video_upload) + self.complete_presentation_transition(suppressed_video_generation) + self._queue_first_frame_ready() rendered_identity = self._take_pending_content_submission() if rendered_identity is not None: diff --git a/src/iPhoto/gui/ui/widgets/video_area.py b/src/iPhoto/gui/ui/widgets/video_area.py index 7affdaf6b..503ae3b88 100644 --- a/src/iPhoto/gui/ui/widgets/video_area.py +++ b/src/iPhoto/gui/ui/widgets/video_area.py @@ -1381,13 +1381,6 @@ def _submit_video_frame_to_surface( content_serial=content_serial, ) self._adjusted_first_frame_pending = False - complete_transition = getattr( - surface, - "complete_presentation_transition", - None, - ) - if callable(complete_transition): - complete_transition(self._media_generation) self._surface_stack.update() self.update() else: @@ -1396,13 +1389,6 @@ def _submit_video_frame_to_surface( content_generation=self._media_generation, content_serial=content_serial, ) - complete_transition = getattr( - surface, - "complete_presentation_transition", - None, - ) - if callable(complete_transition): - complete_transition(self._media_generation) def _queue_retained_frame_for_surface( self, diff --git a/src/iPhoto/gui/ui/widgets/video_renderer_widget.py b/src/iPhoto/gui/ui/widgets/video_renderer_widget.py index bb2051120..9ef6a8a84 100644 --- a/src/iPhoto/gui/ui/widgets/video_renderer_widget.py +++ b/src/iPhoto/gui/ui/widgets/video_renderer_widget.py @@ -833,7 +833,15 @@ def render(self, cb) -> None: # type: ignore[override] if output_size.isEmpty(): return - if VideoRendererWidget._presentation_is_suppressed(self): + suppressed_generation = self._presentation_suppressed_generation + suppressed_frame_ready = bool( + suppressed_generation is not None + and self._has_frame + and self._frame_dirty + and self._frame_content_generation == suppressed_generation + and self._current_frame is not None + ) + if suppressed_generation is not None and not suppressed_frame_ready: cb.beginPass( self.renderTarget(), self._transition_clear_color(), @@ -860,18 +868,44 @@ def render(self, cb) -> None: # type: ignore[override] return ru = rhi.nextResourceUpdateBatch() + staged_suppressed_frame = False # Upload frame data if dirty. # Only clear the dirty flag *after* the upload succeeds so that a # failed map/upload attempt is retried on the next render cycle # instead of leaving uninitialized textures on screen. if self._frame_dirty: - if self._upload_frame(rhi, ru): - self._frame_dirty = False - # Release the decoded frame reference immediately so the - # hardware decoder can recycle its buffer. All pixel data - # has already been copied into GPU textures. - self._current_frame = None + try: + upload_succeeded = self._upload_frame(rhi, ru) + except Exception: + if suppressed_generation is None: + raise + upload_succeeded = False + if upload_succeeded: + staged_suppressed_frame = suppressed_generation is not None + if not staged_suppressed_frame: + self._frame_dirty = False + # Release the decoded frame reference immediately so the + # hardware decoder can recycle its buffer. All pixel data + # has already been copied into GPU textures. + self._current_frame = None + elif suppressed_generation is not None: + emit_detail_event( + "video_gpu_upload_retry", + generation=int(suppressed_generation), + renderer="video_renderer", + backend=self.render_backend_name(), + failure_stage="upload", + ) + cb.beginPass( + self.renderTarget(), + self._transition_clear_color(), + QRhiDepthStencilClearValue(), + ) + cb.endPass() + self._queue_first_frame_ready() + self._rendered_content_identity = None + return # Update uniform buffer self._update_uniforms(ru, output_size) @@ -891,6 +925,10 @@ def render(self, cb) -> None: # type: ignore[override] cb.setVertexInput(0, vbuf_binding) cb.draw(6) # 6 vertices = 2 triangles cb.endPass() + if staged_suppressed_frame and suppressed_generation is not None: + self._frame_dirty = False + self._current_frame = None + self.complete_presentation_transition(suppressed_generation) self._queue_first_frame_ready() if self._frame_presentation_pending and not self._frame_dirty: self._frame_presentation_pending = False diff --git a/tests/gui/coordinators/test_playback_coordinator.py b/tests/gui/coordinators/test_playback_coordinator.py index a5d3b6071..94503bba1 100644 --- a/tests/gui/coordinators/test_playback_coordinator.py +++ b/tests/gui/coordinators/test_playback_coordinator.py @@ -1228,6 +1228,10 @@ def test_live_photo_motion_preparation_failure_restores_pending_still() -> None: coordinator._player_view.show_placeholder.assert_not_called() coordinator._player_view.defer_live_badge_until_ready.assert_called_once_with(7) coordinator._player_view.set_live_replay_enabled.assert_called_once_with(True) + player_calls = [item[0] for item in coordinator._player_view.method_calls] + assert player_calls.index("defer_live_badge_until_ready") < player_calls.index( + "apply_pending_still" + ) PlaybackCoordinator._on_still_frame_presented(coordinator, still, 7) diff --git a/tests/ui/controllers/test_player_view_controller_adjustments.py b/tests/ui/controllers/test_player_view_controller_adjustments.py index 006306ef4..c6cbd71e7 100644 --- a/tests/ui/controllers/test_player_view_controller_adjustments.py +++ b/tests/ui/controllers/test_player_view_controller_adjustments.py @@ -297,7 +297,7 @@ def test_resident_live_photo_still_is_deferred_while_motion_is_visible() -> None assert PlayerViewController._dispatch_prepared_intent(controller, intent, {}) - assert controller._pending_still == (surface, {"Exposure": 0.25}) + assert controller._pending_still == (2, surface, {"Exposure": 0.25}) image_viewer.activate_resident_surface.assert_not_called() controller.show_image_surface.assert_not_called() scheduler.request.assert_not_called() diff --git a/tests/ui/controllers/test_player_view_init_cover.py b/tests/ui/controllers/test_player_view_init_cover.py index e15491741..d09082795 100644 --- a/tests/ui/controllers/test_player_view_init_cover.py +++ b/tests/ui/controllers/test_player_view_init_cover.py @@ -370,6 +370,48 @@ def test_transition_badge_restores_only_for_matching_generation( controller._restore_transition_ui(15) assert controller._live_badge.isHidden() is False + def test_pending_live_still_uses_image_transition_and_restores_badge( + self, + controller, + tmp_path, + ): + source = tmp_path / "live-still.jpg" + surface = _surface( + source, + QImage(64, 48, QImage.Format.Format_RGBA8888), + ) + controller._requires_post_submit_frame = False + controller._request_generation = 16 + controller._present_generation = 16 + controller._present_source = source.absolute() + controller._present_started_at = time.perf_counter() + controller._request_reason_by_generation[16] = "initial" + controller._pending_still = (16, surface, {}) + controller._player_stack.setCurrentWidget(controller._video_area) + controller.defer_live_badge_until_ready(16) + + assert controller.apply_pending_still() + + assert controller._pending_image_generation == 16 + assert controller._pending_image_key == surface.decode_key + assert controller._player_stack.currentWidget() is controller._image_viewer + controller._image_viewer.stillFrameSubmitted.emit(surface.decode_key, 16) + + assert controller._pending_image_generation is None + assert controller._live_badge.isHidden() is False + + def test_stale_pending_live_still_is_rejected(self, controller, tmp_path): + surface = _surface( + tmp_path / "stale-live-still.jpg", + QImage(64, 48, QImage.Format.Format_RGBA8888), + ) + controller._request_generation = 18 + controller._pending_still = (17, surface, {}) + + assert controller.apply_pending_still() is False + assert controller._pending_still is None + assert controller._pending_image_generation is None + def test_primed_image_decode_failure_returns_to_placeholder( self, controller, diff --git a/tests/ui/widgets/test_gl_image_viewer_post_load_signal.py b/tests/ui/widgets/test_gl_image_viewer_post_load_signal.py index 43be9a7db..7b6297c1f 100644 --- a/tests/ui/widgets/test_gl_image_viewer_post_load_signal.py +++ b/tests/ui/widgets/test_gl_image_viewer_post_load_signal.py @@ -142,6 +142,145 @@ def test_rhi_suppression_clears_without_drawing_or_consuming_new_surface() -> No assert viewer._rendered_content_identity is None +@pytest.mark.parametrize("uses_raw_gl", (True, False)) +def test_adjusted_video_resumes_only_after_gpu_draw( + qapp, + mocker, + uses_raw_gl, +) -> None: + viewer = GLImageViewer() + viewer._uses_raw_gl = uses_raw_gl + viewer._gl_initialized = True + viewer._gl_funcs = mocker.Mock() + viewer._renderer = mocker.Mock() + viewer._renderer.has_texture.return_value = True + viewer._renderer.texture_size.return_value = (64, 48) + viewer._renderer.last_video_upload_pre_rotated.return_value = False + viewer._renderer.take_still_upload_result.return_value = None + retained_frame = mocker.Mock() + viewer._using_video_frame_source = True + viewer._video_frame = retained_frame + viewer._pending_video_image = None + viewer._video_frame_dirty = True + viewer._video_frame_content_generation = 9 + viewer._video_frame_content_serial = 4 + viewer._video_frame_presentation_pending = False + viewer._pending_source_rotate90_steps = 0 + viewer._pending_video_reset_view = False + viewer._pending_resident_activation = None + viewer._pending_warm_surfaces = [] + viewer._image = None + viewer.begin_presentation_transition(9) + target = mocker.Mock() + target.pixelSize.return_value = QSize(320, 240) + mocker.patch.object(viewer, "renderTarget", return_value=target) + mocker.patch( + "iPhoto.gui.ui.widgets.gl_image_viewer.widget._load_gl_module", + return_value=SimpleNamespace(GL_COLOR_BUFFER_BIT=0x4000), + ) + command_buffer = mocker.Mock() + + viewer.render(command_buffer) + + viewer._renderer.upload_video_frame.assert_called_once_with(retained_frame) + viewer._renderer.render.assert_called_once() + assert viewer._presentation_suppressed_generation is None + assert viewer._video_frame_dirty is False + assert viewer._video_frame is None + assert viewer._rendered_content_identity is not None + assert viewer._rendered_content_identity[:3] == ("video", 9, 4) + + +@pytest.mark.parametrize("uses_raw_gl", (True, False)) +def test_adjusted_video_upload_failure_keeps_suppression_and_retry_input( + qapp, + mocker, + uses_raw_gl, +) -> None: + viewer = GLImageViewer() + viewer._uses_raw_gl = uses_raw_gl + viewer._gl_initialized = True + viewer._gl_funcs = mocker.Mock() + viewer._renderer = mocker.Mock() + viewer._renderer.has_texture.return_value = True + viewer._renderer.upload_video_frame.side_effect = RuntimeError("upload failed") + viewer._renderer.take_still_upload_result.return_value = None + retained_frame = mocker.Mock() + viewer._using_video_frame_source = True + viewer._video_frame = retained_frame + viewer._pending_video_image = None + viewer._video_frame_dirty = True + viewer._video_frame_content_generation = 10 + viewer._video_frame_content_serial = 5 + viewer._pending_source_rotate90_steps = 0 + viewer._pending_video_reset_view = False + viewer._pending_resident_activation = None + viewer._pending_warm_surfaces = [] + viewer._image = None + viewer.begin_presentation_transition(10) + target = mocker.Mock() + target.pixelSize.return_value = QSize(320, 240) + mocker.patch.object(viewer, "renderTarget", return_value=target) + mocker.patch( + "iPhoto.gui.ui.widgets.gl_image_viewer.widget._load_gl_module", + return_value=SimpleNamespace(GL_COLOR_BUFFER_BIT=0x4000), + ) + + viewer.render(mocker.Mock()) + + viewer._renderer.render.assert_not_called() + assert viewer._presentation_suppressed_generation == 10 + assert viewer._video_frame_dirty is True + assert viewer._video_frame is retained_frame + assert viewer._rendered_content_identity is None + + +@pytest.mark.parametrize("uses_raw_gl", (True, False)) +def test_adjusted_video_draw_failure_keeps_suppression_and_retry_input( + qapp, + mocker, + uses_raw_gl, +) -> None: + viewer = GLImageViewer() + viewer._uses_raw_gl = uses_raw_gl + viewer._gl_initialized = True + viewer._gl_funcs = mocker.Mock() + viewer._renderer = mocker.Mock() + viewer._renderer.has_texture.return_value = True + viewer._renderer.texture_size.return_value = (64, 48) + viewer._renderer.last_video_upload_pre_rotated.return_value = False + viewer._renderer.take_still_upload_result.return_value = None + viewer._renderer.render.side_effect = RuntimeError("draw failed") + retained_frame = mocker.Mock() + viewer._using_video_frame_source = True + viewer._video_frame = retained_frame + viewer._pending_video_image = None + viewer._video_frame_dirty = True + viewer._video_frame_content_generation = 11 + viewer._video_frame_content_serial = 6 + viewer._pending_source_rotate90_steps = 0 + viewer._pending_video_reset_view = False + viewer._pending_resident_activation = None + viewer._pending_warm_surfaces = [] + viewer._image = None + viewer.begin_presentation_transition(11) + target = mocker.Mock() + target.pixelSize.return_value = QSize(320, 240) + mocker.patch.object(viewer, "renderTarget", return_value=target) + mocker.patch( + "iPhoto.gui.ui.widgets.gl_image_viewer.widget._load_gl_module", + return_value=SimpleNamespace(GL_COLOR_BUFFER_BIT=0x4000), + ) + + with pytest.raises(RuntimeError, match="draw failed"): + viewer.render(mocker.Mock()) + + assert viewer._presentation_suppressed_generation == 11 + assert viewer._video_frame_dirty is True + assert viewer._video_frame is retained_frame + assert viewer._rendered_content_identity is None + + @pytest.mark.gpu @pytest.mark.windows_compositor def test_visible_windows_transition_never_exposes_previous_still(qapp) -> None: diff --git a/tests/ui/widgets/test_video_area.py b/tests/ui/widgets/test_video_area.py index 45356bd99..84580e27a 100644 --- a/tests/ui/widgets/test_video_area.py +++ b/tests/ui/widgets/test_video_area.py @@ -825,6 +825,35 @@ def commit_state(generation: int, adjusted: bool) -> None: surface = video_area.video_view() presented = QSignalSpy(surface.videoFramePresented) composed = QSignalSpy(surface.frameSubmitted) + force_upload_failure = os.environ.get( + "IPHOTO_WINDOWS_COMPOSITOR_FORCE_UPLOAD_FAILURE", + "", + ).strip().lower() in {"1", "true", "yes", "on"} + failure_budget = {"remaining": 0} + if force_upload_failure and surface_kind == "native": + real_upload = surface._upload_frame + + def fail_native_upload_once(*args, **kwargs): + if failure_budget["remaining"] > 0: + failure_budget["remaining"] -= 1 + return False + return real_upload(*args, **kwargs) + + mocker.patch.object(surface, "_upload_frame", side_effect=fail_native_upload_once) + elif force_upload_failure: + real_upload = surface._renderer.upload_video_frame + + def fail_adjusted_upload_once(*args, **kwargs): + if failure_budget["remaining"] > 0: + failure_budget["remaining"] -= 1 + raise RuntimeError("forced compositor upload failure") + return real_upload(*args, **kwargs) + + mocker.patch.object( + surface._renderer, + "upload_video_frame", + side_effect=fail_adjusted_upload_once, + ) submit_color(0xFFFF0000, 1) wait_for(presented, 1) red_pixel = center_pixel() @@ -855,7 +884,19 @@ def commit_state(generation: int, adjusted: bool) -> None: assert abs(transition_pixel.blue() - expected_background.blue()) <= 20 next_is_blue = index % 2 == 0 + failed_upload_composed_before = composed.count() + if force_upload_failure: + failure_budget["remaining"] = 1 submit_color(0xFF0000FF if next_is_blue else 0xFFFF0000, 1) + if force_upload_failure: + wait_for(composed, failed_upload_composed_before + 1) + failed_upload_pixel = center_pixel() + expected_background = surface._transition_clear_color() + assert failed_upload_pixel.alpha() == 255 + assert abs(failed_upload_pixel.red() - expected_background.red()) <= 20 + assert abs(failed_upload_pixel.green() - expected_background.green()) <= 20 + assert abs(failed_upload_pixel.blue() - expected_background.blue()) <= 20 + surface.update() wait_for(presented, index + 2) presented_pixel = center_pixel() if next_is_blue: @@ -1017,6 +1058,86 @@ def test_native_renderer_suppression_clears_without_drawing(self): assert renderer._has_frame is True assert renderer._rendered_content_identity is None + def test_native_suppressed_upload_failure_keeps_frame_and_does_not_draw(self): + renderer = Mock() + renderer._initialized = True + renderer._presentation_suppressed_generation = 6 + renderer._has_frame = True + renderer._frame_dirty = True + renderer._frame_content_generation = 6 + renderer._frame_content_serial = 2 + renderer._frame_content_revision = 1 + renderer._frame_presentation_pending = True + retained_frame = object() + renderer._current_frame = retained_frame + renderer.rhi.return_value = Mock() + renderer._upload_frame.return_value = False + target = Mock() + target.pixelSize.return_value = QSize(320, 240) + renderer.renderTarget.return_value = target + command_buffer = Mock() + + VideoRendererWidget.render(renderer, command_buffer) + + command_buffer.draw.assert_not_called() + renderer.complete_presentation_transition.assert_not_called() + assert renderer._frame_dirty is True + assert renderer._current_frame is retained_frame + assert renderer._presentation_suppressed_generation == 6 + assert renderer._rendered_content_identity is None + + def test_native_suppressed_upload_success_draws_before_resuming(self): + renderer = Mock() + renderer._initialized = True + renderer._presentation_suppressed_generation = 7 + renderer._has_frame = True + renderer._frame_dirty = True + renderer._frame_content_generation = 7 + renderer._frame_content_serial = 3 + renderer._frame_content_revision = 2 + renderer._frame_presentation_pending = True + renderer._current_frame = object() + renderer.rhi.return_value = Mock() + renderer._upload_frame.return_value = True + target = Mock() + target.pixelSize.return_value = QSize(320, 240) + renderer.renderTarget.return_value = target + command_buffer = Mock() + + VideoRendererWidget.render(renderer, command_buffer) + + renderer._upload_frame.assert_called_once() + command_buffer.resourceUpdate.assert_called_once() + command_buffer.draw.assert_called_once_with(6) + renderer.complete_presentation_transition.assert_called_once_with(7) + assert renderer._frame_dirty is False + assert renderer._current_frame is None + assert renderer._rendered_content_identity == (7, 3, 2) + + def test_native_suppressed_draw_failure_keeps_retryable_frame(self): + renderer = Mock() + renderer._initialized = True + renderer._presentation_suppressed_generation = 8 + renderer._has_frame = True + renderer._frame_dirty = True + renderer._frame_content_generation = 8 + renderer._current_frame = object() + renderer.rhi.return_value = Mock() + renderer._upload_frame.return_value = True + target = Mock() + target.pixelSize.return_value = QSize(320, 240) + renderer.renderTarget.return_value = target + command_buffer = Mock() + command_buffer.draw.side_effect = RuntimeError("draw failed") + + with pytest.raises(RuntimeError, match="draw failed"): + VideoRendererWidget.render(renderer, command_buffer) + + renderer.complete_presentation_transition.assert_not_called() + assert renderer._frame_dirty is True + assert renderer._current_frame is not None + assert renderer._presentation_suppressed_generation == 8 + def test_begin_load_suppresses_both_video_surfaces(self, qapp, mocker): va = VideoArea() native_suppress = mocker.patch.object( @@ -1762,7 +1883,7 @@ def test_begin_load_never_probes_on_gui_thread(self, qapp, mocker): assert mock_set_rot.call_args_list[-1] == call(90, 1920, 1440) @pytest.mark.parametrize("surface_kind", ("native", "adjusted")) - def test_video_frame_is_installed_before_suppression_completes( + def test_video_area_stages_frame_without_completing_suppression( self, qapp, mocker, @@ -1774,13 +1895,10 @@ def test_video_frame_is_installed_before_suppression_completes( image.fill(0xFF123456) frame = QVideoFrame(image) surface = va._renderer if surface_kind == "native" else va._edit_viewer - calls = mocker.Mock() complete = mocker.patch.object(surface, "complete_presentation_transition") - calls.attach_mock(complete, "complete") if surface_kind == "native": install = mocker.patch.object(surface, "update_frame") - calls.attach_mock(install, "install") else: mocker.patch( "iPhoto.gui.ui.widgets.video_area._resolve_frame_rotation_cw", @@ -1788,12 +1906,11 @@ def test_video_frame_is_installed_before_suppression_completes( ) mocker.patch.object(surface, "set_pending_video_source_rotation") install = mocker.patch.object(surface, "set_video_frame") - calls.attach_mock(install, "install") va._submit_video_frame_to_surface(frame, surface, content_serial=3) - assert [item[0] for item in calls.mock_calls] == ["install", "complete"] - complete.assert_called_once_with(9) + install.assert_called_once() + complete.assert_not_called() def test_native_frame_install_failure_keeps_suppression(self, qapp, mocker): va = VideoArea()