From 93693415b4e053741bb26deaab6da3f6f0ddd6a0 Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Mon, 10 Jun 2024 13:03:47 -0400 Subject: [PATCH 1/3] [video_player] Try to address test flake This attempts to address two sources of flake: - A test that playing doesn't continue past the duration flakily fails, at least on Android, with a position a small amount past the duration. This seems like an unexpected library behavior that we wouldn't want to expose to clients, so rather than change the test, this makes the logic that updates the `Value` clamp the position to the duration. - A test that asset videos play has been restructured to actually wait for the future that should start playing to complete before checking whether it's playing. The test was previously not actually waiting for anything other than animations to complete, and there's no reason the placeholder layout couldn't have completed before the asset loaded. The fact that the test was already disabled for iOS is strong evidence that the flake we are seeing on Android is a problem with the test itself, so hopefully this addresses both platforms. Fixes https://github.com/flutter/flutter/issues/86915 --- packages/video_player/video_player/CHANGELOG.md | 3 ++- .../example/integration_test/video_player_test.dart | 8 +++++--- packages/video_player/video_player/lib/video_player.dart | 6 ++++++ packages/video_player/video_player/pubspec.yaml | 2 +- 4 files changed, 14 insertions(+), 5 deletions(-) diff --git a/packages/video_player/video_player/CHANGELOG.md b/packages/video_player/video_player/CHANGELOG.md index 1eb23d6a378e..777c9f2f03dc 100644 --- a/packages/video_player/video_player/CHANGELOG.md +++ b/packages/video_player/video_player/CHANGELOG.md @@ -1,5 +1,6 @@ -## NEXT +## 2.8.7 +* Ensures that value.position never reports a value larger than value.duration. * Updates minimum supported SDK version to Flutter 3.16/Dart 3.2. ## 2.8.6 diff --git a/packages/video_player/video_player/example/integration_test/video_player_test.dart b/packages/video_player/video_player/example/integration_test/video_player_test.dart index d475a1ac5ef1..0acebb548c24 100644 --- a/packages/video_player/video_player/example/integration_test/video_player_test.dart +++ b/packages/video_player/video_player/example/integration_test/video_player_test.dart @@ -194,9 +194,11 @@ void main() { testWidgets('test video player view with local asset', (WidgetTester tester) async { + final Completer loaded = Completer(); Future started() async { await controller.initialize(); await controller.play(); + loaded.complete(); return true; } @@ -221,12 +223,12 @@ void main() { ), )); + await loaded.future; await tester.pumpAndSettle(); expect(controller.value.isPlaying, true); }, - skip: kIsWeb || // Web does not support local assets. - // Extremely flaky on iOS: https://github.com/flutter/flutter/issues/86915 - defaultTargetPlatform == TargetPlatform.iOS); + // Web does not support local assets. + skip: kIsWeb); }); group('file-based videos', () { diff --git a/packages/video_player/video_player/lib/video_player.dart b/packages/video_player/video_player/lib/video_player.dart index 007c6ba14e5d..65743c00e7ed 100644 --- a/packages/video_player/video_player/lib/video_player.dart +++ b/packages/video_player/video_player/lib/video_player.dart @@ -743,6 +743,12 @@ class VideoPlayerController extends ValueNotifier { } void _updatePosition(Duration position) { + // The underlying native implementation on some platforms sometimes reports + // a position slightly past the reported max duration. Clamp to the duration + // to insulate clients from this behavior. + if (position.compareTo(value.duration) > 0) { + position = value.duration; + } value = value.copyWith( position: position, caption: _getCaptionAt(position), diff --git a/packages/video_player/video_player/pubspec.yaml b/packages/video_player/video_player/pubspec.yaml index 0dbdd0c9062d..9ed1b6f7e292 100644 --- a/packages/video_player/video_player/pubspec.yaml +++ b/packages/video_player/video_player/pubspec.yaml @@ -3,7 +3,7 @@ description: Flutter plugin for displaying inline video with other Flutter widgets on Android, iOS, and web. repository: https://github.com/flutter/packages/tree/main/packages/video_player/video_player issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+video_player%22 -version: 2.8.6 +version: 2.8.7 environment: sdk: ">=3.2.3 <4.0.0" From f8fa0da72650a87e7496ba5c1dac63833a2cda88 Mon Sep 17 00:00:00 2001 From: stuartmorgan Date: Mon, 10 Jun 2024 14:06:12 -0400 Subject: [PATCH 2/3] backticks --- packages/video_player/video_player/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/video_player/video_player/CHANGELOG.md b/packages/video_player/video_player/CHANGELOG.md index 777c9f2f03dc..7de2bc64061b 100644 --- a/packages/video_player/video_player/CHANGELOG.md +++ b/packages/video_player/video_player/CHANGELOG.md @@ -1,6 +1,6 @@ ## 2.8.7 -* Ensures that value.position never reports a value larger than value.duration. +* Ensures that `value.position` never reports a value larger than `value.duration`. * Updates minimum supported SDK version to Flutter 3.16/Dart 3.2. ## 2.8.6 From 6cf07e3eea460cff9be66a3968fb10475cf7d962 Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Mon, 10 Jun 2024 14:10:15 -0400 Subject: [PATCH 3/3] Simplify check --- packages/video_player/video_player/lib/video_player.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/video_player/video_player/lib/video_player.dart b/packages/video_player/video_player/lib/video_player.dart index 65743c00e7ed..4d7f68e72729 100644 --- a/packages/video_player/video_player/lib/video_player.dart +++ b/packages/video_player/video_player/lib/video_player.dart @@ -746,7 +746,7 @@ class VideoPlayerController extends ValueNotifier { // The underlying native implementation on some platforms sometimes reports // a position slightly past the reported max duration. Clamp to the duration // to insulate clients from this behavior. - if (position.compareTo(value.duration) > 0) { + if (position > value.duration) { position = value.duration; } value = value.copyWith(