diff --git a/lib/src/core/design_system/templates/video_review_page_template.dart b/lib/src/core/design_system/templates/video_review_page_template.dart index f66f3ddb..a83ebde7 100644 --- a/lib/src/core/design_system/templates/video_review_page_template.dart +++ b/lib/src/core/design_system/templates/video_review_page_template.dart @@ -78,6 +78,7 @@ class VideoReviewPageTemplate extends StatelessWidget { leading: AppLeadingButton( color: theme.textTheme.titleLarge?.color, tooltip: l10n.buttonBack, + onPressed: onBack, ), title: Text(title), centerTitle: false, diff --git a/lib/src/features/posting/ui/controllers/video_review_playback_session.dart b/lib/src/features/posting/ui/controllers/video_review_playback_session.dart new file mode 100644 index 00000000..c7f2d324 --- /dev/null +++ b/lib/src/features/posting/ui/controllers/video_review_playback_session.dart @@ -0,0 +1,81 @@ +import 'dart:async'; + +import 'package:video_player/video_player.dart'; + +typedef VideoReviewPlaybackErrorHandler = + void Function(Exception error, StackTrace stackTrace); + +/// Owns looping and teardown for the local player on the video review page. +/// +/// Native looping is deliberately avoided here. Some locally rendered videos +/// can leave the platform decoder displaying stale frames after its first +/// native loop. Restarting after the completion event keeps the operation +/// serialized and lets teardown wait for an in-flight restart. +class VideoReviewPlaybackSession { + VideoReviewPlaybackSession(this.videoController, this._onError) { + videoController.addListener(_handleValueChanged); + } + + final VideoPlayerController videoController; + final VideoReviewPlaybackErrorHandler _onError; + Future? _restartFuture; + bool _isRestarting = false; + bool _isDisposed = false; + + Future play() => videoController.play(); + + void _handleValueChanged() { + if (_isDisposed || _isRestarting || !videoController.value.isCompleted) { + return; + } + + _isRestarting = true; + _restartFuture = _restartAfterCompletion(); + } + + Future _restartAfterCompletion() async { + try { + // video_player starts its own pause-and-seek-to-duration cleanup before + // publishing isCompleted. Queue another pause behind that work so our + // seek to zero is the final seek at the completion boundary. + await videoController.pause(); + if (_isDisposed) return; + + await videoController.seekTo(Duration.zero); + if (!_isDisposed) { + await videoController.play(); + } + } on Exception catch (error, stackTrace) { + if (!_isDisposed) { + _onError(error, stackTrace); + } + } finally { + _isRestarting = false; + } + } + + Future dispose() async { + if (_isDisposed) { + await _restartFuture; + return; + } + + _isDisposed = true; + videoController.removeListener(_handleValueChanged); + await _restartFuture; + + try { + if (videoController.value.isPlaying) { + await videoController.pause(); + } + } on Exception catch (error, stackTrace) { + _onError(error, stackTrace); + } + + try { + await videoController.dispose(); + } on Exception catch (error, stackTrace) { + _onError(error, stackTrace); + } + } +} diff --git a/lib/src/features/posting/ui/pages/video_review_page.dart b/lib/src/features/posting/ui/pages/video_review_page.dart index 274dc21b..c26d048b 100644 --- a/lib/src/features/posting/ui/pages/video_review_page.dart +++ b/lib/src/features/posting/ui/pages/video_review_page.dart @@ -15,9 +15,12 @@ import 'package:spark/src/core/network/atproto/atproto.dart'; import 'package:spark/src/core/routing/app_router.dart'; import 'package:spark/src/core/ui/widgets/alt_text_editor_dialog.dart'; import 'package:spark/src/core/utils/error_messages.dart'; +import 'package:spark/src/core/utils/logging/log_service.dart'; +import 'package:spark/src/core/utils/logging/logger.dart'; import 'package:spark/src/features/auth/providers/auth_providers.dart'; import 'package:spark/src/features/posting/models/mention_controller.dart'; import 'package:spark/src/features/posting/providers/video_upload_provider.dart'; +import 'package:spark/src/features/posting/ui/controllers/video_review_playback_session.dart'; import 'package:spark/src/features/profile/providers/profile_feed_provider.dart'; import 'package:video_player/video_player.dart'; @@ -52,24 +55,29 @@ class _VideoReviewPageState extends ConsumerState { bool _crosspostToBsky = false; late XFile _video; late final FeedRepository _feedRepository; - VideoPlayerController? _player; + late final Future _playerInitialization; + late final SparkLogger _logger; + VideoReviewPlaybackSession? _playbackSession; VideoUploadResult? _uploadResult; String? _uploadErrorMessage; double _uploadProgress = 0; _VideoUploadPhase? _uploadPhase; bool _isUploadingVideo = false; + bool _isLeaving = false; @override void initState() { super.initState(); _video = XFile(widget.videoPath); _feedRepository = GetIt.I().feed; + _logger = GetIt.I().getLogger('VideoReviewPage'); _descriptionController.textController.addListener( _handleDescriptionChanged, ); + _playerInitialization = _initPlayer(); unawaited( - _initPlayer().whenComplete(() { - if (mounted) { + _playerInitialization.whenComplete(() { + if (mounted && !_isLeaving) { _startVideoUpload(); } }), @@ -82,10 +90,10 @@ class _VideoReviewPageState extends ConsumerState { _handleDescriptionChanged, ); _descriptionController.dispose(); - final player = _player; - _player = null; - if (player != null) { - unawaited(_disposePlayer(player)); + final session = _playbackSession; + _playbackSession = null; + if (session != null) { + unawaited(session.dispose()); } super.dispose(); } @@ -96,51 +104,58 @@ class _VideoReviewPageState extends ConsumerState { Future _initPlayer() async { final c = VideoPlayerController.file(File(_video.path)); + final session = VideoReviewPlaybackSession(c, (error, stackTrace) { + _logger.e( + 'Video review playback failed', + error: error, + stackTrace: stackTrace, + ); + }); try { await c.initialize(); - if (!mounted) { - await _disposePlayer(c); - return; - } - - await c.setLooping(true); - if (!mounted) { - await _disposePlayer(c); + if (!mounted || _isLeaving) { + await session.dispose(); return; } await c.setVolume(1); - if (!mounted) { - await _disposePlayer(c); + if (!mounted || _isLeaving) { + await session.dispose(); return; } - setState(() => _player = c); - unawaited(c.play()); - } catch (_) { - await _disposePlayer(c); - if (!mounted) return; + setState(() => _playbackSession = session); + await session.play(); + } on Exception catch (error, stackTrace) { + _playbackSession = null; + await session.dispose(); + _logger.e( + 'Unable to initialize video review playback', + error: error, + stackTrace: stackTrace, + ); + if (!mounted || _isLeaving) return; _showPostError('Unable to preview this video. Please try again.'); } } - Future _pausePlayer(VideoPlayerController player) async { - try { - if (player.value.isPlaying) { - await player.pause(); - } - } catch (_) { - // Best-effort native player cleanup. - } + Future _close() async { + if (!await _prepareToLeave() || !mounted) return; + context.router.pop(); } - Future _disposePlayer(VideoPlayerController player) async { - await _pausePlayer(player); - try { - await player.dispose(); - } catch (_) { - // Best-effort native player cleanup. + Future _prepareToLeave() async { + if (_isLeaving) return false; + final session = _playbackSession; + setState(() { + _isLeaving = true; + _playbackSession = null; + }); + await _playerInitialization; + if (session != null) { + await session.dispose(); } + return mounted; } Future _editAltText() async { @@ -248,7 +263,7 @@ class _VideoReviewPageState extends ConsumerState { } Future _postVideo() async { - if (_isPosting) return; + if (_isPosting || _isLeaving) return; final uploadResult = _uploadResult; if (uploadResult == null) { if (_uploadErrorMessage != null) { @@ -279,11 +294,10 @@ class _VideoReviewPageState extends ConsumerState { ); if (!mounted) return; - setState(() { - _isPosting = false; - }); - if (postRef == null) { + setState(() { + _isPosting = false; + }); _showPostError('Unable to create post. Please try again'); return; } @@ -299,11 +313,7 @@ class _VideoReviewPageState extends ConsumerState { ); } - final player = _player; - if (player != null) { - await _pausePlayer(player); - if (!mounted) return; - } + if (!await _prepareToLeave() || !mounted) return; final router = context.router; router.popUntilRoot(); @@ -327,7 +337,7 @@ class _VideoReviewPageState extends ConsumerState { } MediaAspectRatio? get _videoAspectRatio { - final player = _player; + final player = _playbackSession?.videoController; if (player == null) return null; final size = player.value.size; @@ -341,7 +351,7 @@ class _VideoReviewPageState extends ConsumerState { final metadataAspectRatio = _videoAspectRatio?.value; if (metadataAspectRatio != null) return metadataAspectRatio; - final rawAspectRatio = _player?.value.aspectRatio; + final rawAspectRatio = _playbackSession?.videoController.value.aspectRatio; return rawAspectRatio != null && rawAspectRatio > 0 ? rawAspectRatio : 1.0; } @@ -354,41 +364,50 @@ class _VideoReviewPageState extends ConsumerState { final uploadStatusLabel = _uploadStatusLabel(l10n); final canPost = !_isPosting && + !_isLeaving && _uploadResult != null && _uploadErrorMessage == null && !isOverLimit; - return VideoReviewPageTemplate( - title: l10n.pageTitleReviewVideo, - onBack: () => context.maybePop(), - aspectRatio: ar, - videoPreview: _player == null - ? const Center(child: CircularProgressIndicator()) - : VideoPlayer(_player!), - onAltEdit: _editAltText, - uploadProgress: _uploadProgress, - uploadStatusLabel: uploadStatusLabel, - uploadIndeterminate: _uploadPhase == _VideoUploadPhase.processing, - hasUploadError: _uploadErrorMessage != null, - onUploadRetry: _uploadErrorMessage == null - ? null - : () => _startVideoUpload(), - mentionController: _descriptionController, - onMentionsChanged: (mentions) { - // Mentions are automatically tracked in the controller + return PopScope( + canPop: false, + onPopInvokedWithResult: (didPop, _) { + if (!didPop) { + unawaited(_close()); + } }, - descriptionMaxChars: AppConstants.postDescriptionMaxChars, - showCrossPost: !widget.storyMode, - crossPostValue: _crosspostToBsky, - onCrossPostChanged: (v) => setState(() => _crosspostToBsky = v), - postLabel: _postLabel(l10n), - isPosting: _isPosting, - isOverLimit: isOverLimit, - onPost: canPost - ? () async { - await _postVideo(); - } - : null, + child: VideoReviewPageTemplate( + title: l10n.pageTitleReviewVideo, + onBack: () => unawaited(_close()), + aspectRatio: ar, + videoPreview: _playbackSession == null + ? const Center(child: CircularProgressIndicator()) + : VideoPlayer(_playbackSession!.videoController), + onAltEdit: _editAltText, + uploadProgress: _uploadProgress, + uploadStatusLabel: uploadStatusLabel, + uploadIndeterminate: _uploadPhase == _VideoUploadPhase.processing, + hasUploadError: _uploadErrorMessage != null, + onUploadRetry: _uploadErrorMessage == null + ? null + : () => _startVideoUpload(), + mentionController: _descriptionController, + onMentionsChanged: (mentions) { + // Mentions are automatically tracked in the controller + }, + descriptionMaxChars: AppConstants.postDescriptionMaxChars, + showCrossPost: !widget.storyMode, + crossPostValue: _crosspostToBsky, + onCrossPostChanged: (v) => setState(() => _crosspostToBsky = v), + postLabel: _postLabel(l10n), + isPosting: _isPosting, + isOverLimit: isOverLimit, + onPost: canPost + ? () async { + await _postVideo(); + } + : null, + ), ); } } diff --git a/test/src/features/posting/ui/controllers/video_review_playback_session_test.dart b/test/src/features/posting/ui/controllers/video_review_playback_session_test.dart new file mode 100644 index 00000000..1d70a299 --- /dev/null +++ b/test/src/features/posting/ui/controllers/video_review_playback_session_test.dart @@ -0,0 +1,111 @@ +import 'dart:async'; + +import 'package:flutter_test/flutter_test.dart'; +import 'package:spark/src/features/posting/ui/controllers/video_review_playback_session.dart'; +import 'package:video_player/video_player.dart'; + +void main() { + test('restarts after video_player completion cleanup', () async { + final controller = _ControlledVideoPlayerController(); + final session = VideoReviewPlaybackSession( + controller, + (error, stackTrace) => fail('$error\n$stackTrace'), + ); + + await session.play(); + final platformCleanup = controller.completePlaybackLikeVideoPlayer(); + controller.repeatCompletionNotification(); + await Future.delayed(Duration.zero); + + expect(controller.seekTargets, [controller.value.duration, Duration.zero]); + expect(controller.playCount, 1); + + controller.completeNextSeek(); + await platformCleanup; + controller.completeNextSeek(); + await Future.delayed(Duration.zero); + + expect(controller.playCount, 2); + await session.dispose(); + }); + + test('dispose waits for an in-flight restart and does not replay', () async { + final controller = _ControlledVideoPlayerController(); + final session = VideoReviewPlaybackSession( + controller, + (error, stackTrace) => fail('$error\n$stackTrace'), + ); + + await session.play(); + final platformCleanup = controller.completePlaybackLikeVideoPlayer(); + await Future.delayed(Duration.zero); + + var disposed = false; + final disposal = session.dispose().then((_) => disposed = true); + await Future.delayed(Duration.zero); + expect(disposed, isFalse); + + controller.completeNextSeek(); + await platformCleanup; + controller.completeNextSeek(); + await disposal; + + expect(controller.playCount, 1); + expect(controller.disposeCount, 1); + }); +} + +class _ControlledVideoPlayerController extends VideoPlayerController { + _ControlledVideoPlayerController() : super.asset('unused') { + value = const VideoPlayerValue( + duration: Duration(seconds: 10), + isInitialized: true, + ); + } + + final List seekTargets = []; + final List> _seeks = []; + var playCount = 0; + var pauseCount = 0; + var disposeCount = 0; + + @override + Future play() async { + playCount++; + value = value.copyWith(isPlaying: true, isCompleted: false); + } + + @override + Future pause() async { + pauseCount++; + value = value.copyWith(isPlaying: false); + } + + @override + Future seekTo(Duration position) { + seekTargets.add(position); + final completer = Completer(); + _seeks.add(completer); + return completer.future; + } + + @override + Future dispose() async { + disposeCount++; + await super.dispose(); + } + + Future completePlaybackLikeVideoPlayer() { + final platformCleanup = pause().then((_) => seekTo(value.duration)); + value = value.copyWith(isPlaying: false, isCompleted: true); + return platformCleanup; + } + + void repeatCompletionNotification() { + notifyListeners(); + } + + void completeNextSeek() { + _seeks.removeAt(0).complete(); + } +}