Skip to content

[video_player] foundation - reduce seek accuracy to fix seek to end bug #3784

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
auto-submit merged 20 commits into from
May 11, 2023
Merged

[video_player] foundation - reduce seek accuracy to fix seek to end bug #3784

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
45fb3a5
remove seek accuracy
tarrinneal Apr 20, 2023
426c067
minimal tolerance
tarrinneal Apr 20, 2023
7d167e2
tolerance only when seeking to duration
tarrinneal Apr 20, 2023
b78ae9b
format
tarrinneal Apr 21, 2023
cc5b173
changelog and comment
tarrinneal Apr 21, 2023
aa9fbbd
tests?
tarrinneal Apr 21, 2023
041dcb3
clean up
tarrinneal Apr 24, 2023
c6861f4
format
tarrinneal Apr 24, 2023
747a421
DI
tarrinneal May 3, 2023
2e12e1a
test set up
tarrinneal May 5, 2023
d130337
move interface
tarrinneal May 5, 2023
7e8fada
Pesky *, I'm sure I'll revert this before it lands
tarrinneal May 5, 2023
b61eace
no tolerance test
tarrinneal May 8, 2023
6a586d1
Finish tests
tarrinneal May 9, 2023
c9bd628
Merge 'main' into video-player-seekToEnd-bug
tarrinneal May 9, 2023
ce41a1f
review changes
tarrinneal May 10, 2023
3ef8801
format
tarrinneal May 10, 2023
46f3c7d
Nits and stub change
tarrinneal May 11, 2023
fb9b9b8
rename plugin registrar
tarrinneal May 11, 2023
7aeb3db
format
tarrinneal May 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/video_player/video_player_avfoundation/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 2.4.5

* Fixes hang when seeking to end of video.

## 2.4.4

* Updates pigeon to fix warnings with clang 15.
Expand Down
24 changes: 19 additions & 5 deletions ...es/video_player/video_player_avfoundation/example/integration_test/video_player_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,13 +71,27 @@ void main() {
expect(await controller.position, greaterThan(Duration.zero));
});

testWidgets('can seek', (WidgetTester tester) async {
await controller.initialize();
testWidgets(
'can seek',
(WidgetTester tester) async {
await controller.initialize();

await controller.seekTo(const Duration(seconds: 3));
await controller.seekTo(const Duration(seconds: 3));

expect(await controller.position, const Duration(seconds: 3));
});
expect(controller.value.position, const Duration(seconds: 3));
},
);

testWidgets(
'can seek to end',
(WidgetTester tester) async {
await controller.initialize();

await controller.seekTo(controller.value.duration);

expect(controller.value.duration, controller.value.position);
},
);

testWidgets('can be paused', (WidgetTester tester) async {
await controller.initialize();
Expand Down
12 changes: 9 additions & 3 deletions packages/video_player/video_player_avfoundation/ios/Classes/FLTVideoPlayerPlugin.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -420,9 +420,15 @@ - (int64_t)duration {
}

- (void)seekTo:(int)location completionHandler:(void (^)(BOOL))completionHandler {
[_player seekToTime:CMTimeMake(location, 1000)
toleranceBefore:kCMTimeZero
toleranceAfter:kCMTimeZero
CMTime locationCMT = CMTimeMake(location, 1000);
CMTimeValue duration = _player.currentItem.asset.duration.value;
// Without adding tolerance when seeking to duration,
// seekToTime will never complete, and this call will hang.
// see issue https://github.com/flutter/flutter/issues/124475.
CMTime tolerance = location == duration ? CMTimeMake(1, 1000) : kCMTimeZero;
Copy link
Contributor

@hellohuanlin hellohuanlin Apr 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you write a unit test (XCTest) for this behavior?

Copy link
Collaborator

@stuartmorgan-g stuartmorgan-g Apr 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea what I didn't think of that when @tarrinneal and I were talking about testing for this 🤦🏻

It looks like we don't currently have a way to mock out the underlying AVPlayer, which significantly reduces testability. I would suggest wrapping the current call to [AVPlayer playerWithPlayerItem:] in a factory object, and do DI of that factory. So you'd make a test-only header that declared:

  • A protocol for the AVPlayer instance factory (which it looks like would just be a single method)
  • A new init that takes an instance of the factory protocol.

The, similar to the other DI you did recently, you'd have a private implementation of that protocol that's the default version, just wrapping [AVPlayer playerWithPlayerItem:], but tests could replace it with something vending mock AVPlayer instances where you could validate the tolerance parameter.

Copy link
Contributor

@hellohuanlin hellohuanlin Apr 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can also try stubbing the constructor directly if it's easier:

id mockPlayer = OCMClassMock([AVPlayer class]);
OCMStub([AVPlayer playerWithPlayerItem:OCMOCM_ANY]).andReturn(mockPlayer);

Copy link
Collaborator

@stuartmorgan-g stuartmorgan-g Apr 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't. OCMock class mocking is giant foot-gun; it's incredibly easy to accidentally leak mocking across tests.

Copy link
Contributor

@hellohuanlin hellohuanlin Apr 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gotcha. i did that a few times for camera plugin iirc. i should clean that up sometime.

Comment on lines +442 to +447
Copy link
Contributor

@misos1 misos1 Sep 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if _player.currentItem.asset.duration.timescale is not 1000? This comparison compares just numerators of fractions.

Copy link
Contributor

@misos1 misos1 Sep 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Video https://flutter.github.io/assets-for-api-docs/assets/videos/bee.mp4 from tests gives 2422 and 600 for _player.currentItem.asset.duration value and timescale.

Copy link
Collaborator

@stuartmorgan-g stuartmorgan-g Sep 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@misos1 This is a PR that landed a year and a half ago; comments here aren't actionable. If there's a bug, please file an issue with details.

[_player seekToTime:locationCMT
toleranceBefore:tolerance
toleranceAfter:tolerance
completionHandler:completionHandler];
}

Expand Down
2 changes: 1 addition & 1 deletion packages/video_player/video_player_avfoundation/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ name: video_player_avfoundation
description: iOS implementation of the video_player plugin.
repository: https://github.com/flutter/packages/tree/main/packages/video_player/video_player_avfoundation
issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+video_player%22
version: 2.4.4
version: 2.4.5

environment:
sdk: ">=2.18.0 <4.0.0"
Expand Down