From caee3817219d70bd056d87cc6380f4e7c0a144d5 Mon Sep 17 00:00:00 2001 From: mertalev <101130780+mertalev@users.noreply.github.com> Date: Tue, 12 Nov 2024 12:32:08 -0500 Subject: [PATCH] extra smooth seeking, add comments --- .../lib/pages/common/gallery_viewer.page.dart | 1 + .../common/native_video_viewer.page.dart | 53 ++++++++++------- .../video_player_value_provider.dart | 10 ++++ mobile/lib/utils/debounce.dart | 57 +++++++++++++++++-- .../widgets/asset_viewer/video_position.dart | 8 ++- 5 files changed, 104 insertions(+), 25 deletions(-) diff --git a/mobile/lib/pages/common/gallery_viewer.page.dart b/mobile/lib/pages/common/gallery_viewer.page.dart index 2ef23e77f3..97f2e9dfec 100644 --- a/mobile/lib/pages/common/gallery_viewer.page.dart +++ b/mobile/lib/pages/common/gallery_viewer.page.dart @@ -292,6 +292,7 @@ class GalleryViewerPage extends HookConsumerWidget { } PhotoViewGalleryPageOptions buildVideo(BuildContext context, Asset asset) { + // This key is to prevent the video player from being re-initialized during the hero animation final key = GlobalKey(); final tag = getHeroTag(asset); return PhotoViewGalleryPageOptions.customChild( diff --git a/mobile/lib/pages/common/native_video_viewer.page.dart b/mobile/lib/pages/common/native_video_viewer.page.dart index 663f4e0774..73ae77dabc 100644 --- a/mobile/lib/pages/common/native_video_viewer.page.dart +++ b/mobile/lib/pages/common/native_video_viewer.page.dart @@ -12,8 +12,8 @@ import 'package:immich_mobile/providers/asset_viewer/video_player_value_provider import 'package:immich_mobile/services/api.service.dart'; import 'package:immich_mobile/services/app_settings.service.dart'; import 'package:immich_mobile/services/asset.service.dart'; +import 'package:immich_mobile/utils/debounce.dart'; import 'package:immich_mobile/utils/hooks/interval_hook.dart'; -import 'package:immich_mobile/utils/throttle.dart'; import 'package:immich_mobile/widgets/asset_viewer/custom_video_player_controls.dart'; import 'package:logging/logging.dart'; import 'package:native_video_player/native_video_player.dart'; @@ -43,6 +43,11 @@ class NativeVideoViewerPage extends HookConsumerWidget { final controller = useState(null); final lastVideoPosition = useRef(-1); final isBuffering = useRef(false); + + // When a video is opened through the timeline, `isCurrent` will immediately be true. + // When swiping from video A to video B, `isCurrent` will initially be true for video A and false for video B. + // If the swipe is completed, `isCurrent` will be true for video B after a delay. + // If the swipe is canceled, `currentAsset` will not have changed and video A will continue to play. final currentAsset = useState(ref.read(currentAssetProvider)); final isCurrent = currentAsset.value == asset; @@ -193,8 +198,12 @@ class NativeVideoViewerPage extends HookConsumerWidget { }); // When the position changes, seek to the position - final seekThrottler = - useThrottler(interval: const Duration(milliseconds: 200)); + // Debounce the seek to avoid seeking too often + // But also don't delay the seek too much to maintain visual feedback + final seekDebouncer = useDebouncer( + interval: const Duration(milliseconds: 100), + maxWaitTime: const Duration(milliseconds: 200), + ); ref.listen(videoPlayerControlsProvider.select((value) => value.position), (_, position) async { final playerController = controller.value; @@ -208,21 +217,10 @@ class NativeVideoViewerPage extends HookConsumerWidget { } // Find the position to seek to - final int seek = (asset.duration * (position / 100.0)).inSeconds; + final seek = position ~/ 1; if (seek != playbackInfo.position) { - try { - final maybeSeek = - seekThrottler.run(() => playerController.seekTo(seek)); - if (maybeSeek != null) { - await maybeSeek; - } - } catch (error) { - log.severe('Error seeking to position $position: $error'); - } + seekDebouncer.run(() => playerController.seekTo(seek)); } - - ref.read(videoPlaybackValueProvider.notifier).position = - Duration(seconds: seek); }); // // When the custom video controls pause or play @@ -233,6 +231,12 @@ class NativeVideoViewerPage extends HookConsumerWidget { return; } + // Make sure the last seek is complete before pausing or playing + // Otherwise, `onPlaybackPositionChanged` can receive outdated events + if (seekDebouncer.isActive) { + await seekDebouncer.drain(); + } + try { if (pause) { await videoController.pause(); @@ -250,6 +254,10 @@ class NativeVideoViewerPage extends HookConsumerWidget { return; } + final videoPlayback = + VideoPlaybackValue.fromNativeController(videoController); + ref.read(videoPlaybackValueProvider.notifier).value = videoPlayback; + try { await videoController.play(); await videoController.setVolume(0.9); @@ -266,11 +274,12 @@ class NativeVideoViewerPage extends HookConsumerWidget { final videoPlayback = VideoPlaybackValue.fromNativeController(videoController); + // No need to update the UI when it's about to loop if (videoPlayback.state == VideoPlaybackState.completed && loopVideo) { return; } - ref.read(videoPlaybackValueProvider.notifier).value = videoPlayback; - + ref.read(videoPlaybackValueProvider.notifier).status = + videoPlayback.state; if (videoPlayback.state == VideoPlaybackState.playing) { // Sync with the controls playing WakelockPlus.enable(); @@ -281,6 +290,11 @@ class NativeVideoViewerPage extends HookConsumerWidget { } void onPlaybackPositionChanged() { + // When seeking, these events sometimes move the slider to an older position + if (seekDebouncer.isActive) { + return; + } + final videoController = controller.value; if (videoController == null || !context.mounted) { return; @@ -388,7 +402,7 @@ class NativeVideoViewerPage extends HookConsumerWidget { return Stack( children: [ - placeholder, + placeholder, // this is always under the video to avoid flickering Center( key: ValueKey('player-${asset.hashCode}'), child: aspectRatio.value != null @@ -404,7 +418,6 @@ class NativeVideoViewerPage extends HookConsumerWidget { ) : null, ), - // covers the video with the placeholder if (showControls) Center( key: ValueKey('controls-${asset.hashCode}'), diff --git a/mobile/lib/providers/asset_viewer/video_player_value_provider.dart b/mobile/lib/providers/asset_viewer/video_player_value_provider.dart index b4f690fa22..3c9ac0b99a 100644 --- a/mobile/lib/providers/asset_viewer/video_player_value_provider.dart +++ b/mobile/lib/providers/asset_viewer/video_player_value_provider.dart @@ -125,6 +125,16 @@ class VideoPlaybackValueState extends StateNotifier { ); } + set status(VideoPlaybackState value) { + if (state.state == value) return; + state = VideoPlaybackValue( + position: state.position, + duration: state.duration, + state: value, + volume: state.volume, + ); + } + void reset() { state = videoPlaybackValueDefault; } diff --git a/mobile/lib/utils/debounce.dart b/mobile/lib/utils/debounce.dart index ca5f8fc2be..78870151a6 100644 --- a/mobile/lib/utils/debounce.dart +++ b/mobile/lib/utils/debounce.dart @@ -3,20 +3,52 @@ import 'dart:async'; import 'package:flutter_hooks/flutter_hooks.dart'; /// Used to debounce function calls with the [interval] provided. +/// If [maxWaitTime] is provided, the first [run] call as well as the next call since [maxWaitTime] has passed will be immediately executed, even if [interval] is not satisfied. class Debouncer { - Debouncer({required this.interval}); + Debouncer({required this.interval, this.maxWaitTime}); final Duration interval; + final Duration? maxWaitTime; Timer? _timer; FutureOr Function()? _lastAction; + DateTime? _lastActionTime; + Future? _actionFuture; void run(FutureOr Function() action) { _lastAction = action; _timer?.cancel(); + + if (maxWaitTime != null && + // _actionFuture == null && // TODO: should this check be here? + (_lastActionTime == null || + DateTime.now().difference(_lastActionTime!) > maxWaitTime!)) { + _callAndRest(); + return; + } _timer = Timer(interval, _callAndRest); } + Future? drain() { + if (_timer != null && _timer!.isActive) { + _timer!.cancel(); + if (_lastAction != null) { + _callAndRest(); + } + } + return _actionFuture; + } + + @pragma('vm:prefer-inline') void _callAndRest() { - _lastAction?.call(); + _lastActionTime = DateTime.now(); + final action = _lastAction; + _lastAction = null; + + final result = action!(); + if (result is Future) { + _actionFuture = result.whenComplete(() { + _actionFuture = null; + }); + } _timer = null; } @@ -24,31 +56,48 @@ class Debouncer { _timer?.cancel(); _timer = null; _lastAction = null; + _lastActionTime = null; + _actionFuture = null; } + + bool get isActive => + _actionFuture != null || (_timer != null && _timer!.isActive); } /// Creates a [Debouncer] that will be disposed automatically. If no [interval] is provided, a /// default interval of 300ms is used to debounce the function calls Debouncer useDebouncer({ Duration interval = const Duration(milliseconds: 300), + Duration? maxWaitTime, List? keys, }) => - use(_DebouncerHook(interval: interval, keys: keys)); + use( + _DebouncerHook( + interval: interval, + maxWaitTime: maxWaitTime, + keys: keys, + ), + ); class _DebouncerHook extends Hook { const _DebouncerHook({ required this.interval, + this.maxWaitTime, super.keys, }); final Duration interval; + final Duration? maxWaitTime; @override HookState> createState() => _DebouncerHookState(); } class _DebouncerHookState extends HookState { - late final debouncer = Debouncer(interval: hook.interval); + late final debouncer = Debouncer( + interval: hook.interval, + maxWaitTime: hook.maxWaitTime, + ); @override Debouncer build(_) => debouncer; diff --git a/mobile/lib/widgets/asset_viewer/video_position.dart b/mobile/lib/widgets/asset_viewer/video_position.dart index ef309b9c85..b1f70b8686 100644 --- a/mobile/lib/widgets/asset_viewer/video_position.dart +++ b/mobile/lib/widgets/asset_viewer/video_position.dart @@ -56,10 +56,16 @@ class VideoPosition extends HookConsumerWidget { ref.read(videoPlayerControlsProvider.notifier).play(); } }, - onChanged: (position) { + onChanged: (value) { + final inSeconds = + (duration * (value / 100.0)).inSeconds; + final position = inSeconds.toDouble(); ref .read(videoPlayerControlsProvider.notifier) .position = position; + // This immediately updates the slider position without waiting for the video to update + ref.read(videoPlaybackValueProvider.notifier).position = + Duration(seconds: inSeconds); }, ), ),