Merge pull request #899 from OliverZhaohaibin/codex/live-photo-detail-performance-fix

Fix Live Photo playback and detail view regression

Author: OliverZhaohaibin

src/iPhoto/gui/coordinators/playback_coordinator.py

@@ -237,6 +237,8 @@ def __init__(
         self._play_debounce.timeout.connect(self._execute_pending_play)
 
         self._connect_signals()
+        self._setup_zoom_handler()
+        self._restore_filmstrip_preference()
 
     def rebind_library(
         self,
@@ -285,8 +287,6 @@ def _is_async_token_current(self, token: PlaybackAsyncToken | None) -> bool:
             token == getattr(self, "_active_async_token", None)
             and token.library_epoch == self._current_library_epoch()
         )
-        self._setup_zoom_handler()
-        self._restore_filmstrip_preference()
 
     def set_navigation_coordinator(self, nav: NavigationCoordinator) -> None:
         self._navigation = nav
@@ -691,6 +691,10 @@ def _render_presentation(self, presentation: DetailPresentation) -> None:
         self._zoom_slider.setValue(100)
         self._zoom_slider.blockSignals(False)
 
+        self._is_playing = False
+        self._player_bar.set_playback_state(False)
+        self._player_bar.set_position(0)
+
         if presentation.is_video:
             self._hide_face_name_overlay(clear_annotations=True)
             if self._is_location_video_write_inflight(source):
@@ -725,6 +729,7 @@ def _render_presentation(self, presentation: DetailPresentation) -> None:
                     has_trim=has_trim,
                 )
                 self._player_view.video_area.play()
+                self._is_playing = True
                 self._player_bar.setEnabled(True)
                 self._zoom_handler.set_viewer(self._player_view.video_area)
                 self._player_view.video_area.reset_zoom()
@@ -766,10 +771,6 @@ def _render_presentation(self, presentation: DetailPresentation) -> None:
                 self._player_view.set_live_replay_enabled(False)
                 self._refresh_face_name_overlay_for_presentation(presentation)
 
-        self._is_playing = False
-        self._player_bar.set_playback_state(False)
-        self._player_bar.set_position(0)
-
         if self._info_panel and presentation.info_panel_visible:
             self._refresh_info_panel(presentation.info)
             self._info_panel.show()

src/iPhoto/gui/ui/controllers/player_view_controller.py

@@ -91,9 +91,9 @@ def run(self) -> None:  # pragma: no cover - executed on a worker thread
         try:
             adjustments_started = time.perf_counter()
             adjustments = {}
-            self.color_stats = compute_color_statistics(image)
             self.source_identity = self.source_identity.repair_revision_from_stat()
             if self._edit_service is not None and self._edit_service.sidecar_exists(self._source):
+                self.color_stats = compute_color_statistics(image)
                 adjustments = self._edit_service.describe_adjustments(
                     self._source,
                     color_stats=self.color_stats,
@@ -109,7 +109,6 @@ def run(self) -> None:  # pragma: no cover - executed on a worker thread
             self._signals.failed.emit(self._source, str(exc))
             return
 
-        # Pass the raw image and adjustments to the main thread. The GL viewer
         # Pass the raw image and adjustments to the main thread. The GL viewer
         # will apply the adjustments on the GPU.
         log_detail_profile(

tests/gui/coordinators/test_playback_coordinator.py

@@ -57,6 +57,35 @@ def _make_presentation(
     )
 
 
+def test_constructor_initializes_detail_controls(qapp) -> None:
+    location_search = SimpleNamespace(
+        suggestionsReady=Mock(connect=Mock()),
+        searchFailed=Mock(connect=Mock()),
+    )
+    arguments = [Mock() for _ in range(18)]
+
+    with patch.object(PlaybackCoordinator, "_connect_signals"), patch.object(
+        PlaybackCoordinator,
+        "_setup_zoom_handler",
+    ) as setup_zoom, patch.object(
+        PlaybackCoordinator,
+        "_restore_filmstrip_preference",
+    ) as restore_filmstrip, patch.object(
+        playback_coordinator_module,
+        "LocationSearchController",
+        return_value=location_search,
+    ):
+        coordinator = PlaybackCoordinator(
+            *arguments,
+            people_service=Mock(),
+            pet_service=Mock(),
+        )
+
+    setup_zoom.assert_called_once_with()
+    restore_filmstrip.assert_called_once_with()
+    coordinator.shutdown = Mock()
+
+
 def test_play_asset_dispatches_immediately_when_idle() -> None:
     coordinator = PlaybackCoordinator.__new__(PlaybackCoordinator)
     coordinator._asset_model = Mock(rowCount=Mock(return_value=3))
@@ -414,6 +443,60 @@ def test_render_presentation_stops_video_area_before_showing_still() -> None:
     coordinator._player_bar.setEnabled.assert_called_once_with(False)
 
 
+def test_render_live_presentation_keeps_motion_playback_active(tmp_path: Path) -> None:
+    still = tmp_path / "photo.heic"
+    motion = tmp_path / "motion.mov"
+    still.write_bytes(b"still")
+    motion.write_bytes(b"motion")
+    presentation = replace(
+        _make_presentation(path=str(still), is_video=False, is_live=True),
+        live_motion_rel=Path("motion.mov"),
+        live_motion_abs=motion,
+    )
+    coordinator = PlaybackCoordinator.__new__(PlaybackCoordinator)
+    coordinator._detail_render_lifecycle = DetailRenderCoordinator()
+    coordinator._detail_generation = 0
+    coordinator._live_transaction = None
+    coordinator._active_async_token = None
+    coordinator._player_view = Mock(
+        video_area=Mock(
+            has_video=Mock(return_value=False),
+            load_video=Mock(),
+            play=Mock(),
+        ),
+        image_viewer=Mock(reset_zoom=Mock()),
+    )
+    coordinator._favorite_button = Mock(setEnabled=Mock())
+    coordinator._info_button = Mock(setEnabled=Mock())
+    coordinator._share_button = Mock(setEnabled=Mock())
+    coordinator._edit_button = Mock(setEnabled=Mock())
+    coordinator._rotate_button = Mock(setEnabled=Mock())
+    coordinator._update_favorite_icon = Mock()
+    coordinator._zoom_slider = Mock(blockSignals=Mock(), setValue=Mock())
+    coordinator._player_bar = Mock(
+        setEnabled=Mock(),
+        set_playback_state=Mock(),
+        set_position=Mock(),
+    )
+    coordinator._zoom_handler = Mock(set_viewer=Mock())
+    coordinator._zoom_widget = Mock(show=Mock())
+    coordinator._info_panel = None
+    coordinator._clear_play_profile = Mock()
+    coordinator._hide_face_name_overlay = Mock()
+    coordinator._is_playing = False
+
+    PlaybackCoordinator._render_presentation(coordinator, presentation)
+
+    coordinator._player_view.video_area.load_video.assert_called_once_with(
+        motion,
+        adjustments=None,
+        trim_range_ms=None,
+        adjusted_preview=False,
+    )
+    coordinator._player_view.video_area.play.assert_called_once_with()
+    assert coordinator._is_playing is True
+
+
 def _live_lifecycle_coordinator(
     tmp_path: Path,
 ) -> tuple[PlaybackCoordinator, DetailPresentation, Path, Path]:

tests/ui/controllers/test_player_view_controller_adjustments.py

@@ -22,7 +22,7 @@
 )
 
 
-def test_adjusted_image_worker_collects_shared_stats_without_sidecar() -> None:
+def test_adjusted_image_worker_skips_color_stats_without_sidecar() -> None:
     source = Path("/tmp/photo.jpg")
     signals = Mock()
     edit_service = Mock()
@@ -39,7 +39,7 @@ def test_adjusted_image_worker_collects_shared_stats_without_sidecar() -> None:
         worker.run()
 
     edit_service.describe_adjustments.assert_not_called()
-    compute_stats.assert_called_once_with(image)
+    compute_stats.assert_not_called()
     signals.completed.emit.assert_called_once_with(source, image, {})