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

Changes from 1 commit
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
Prev Previous commit
Next Next commit
format
  • Loading branch information
@tarrinneal
tarrinneal committed Apr 24, 2023
commit c6861f4da1f883a1bb7f2b3bd581760b63c3ac58
16 changes: 8 additions & 8 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 @@ -422,14 +422,14 @@ - (int64_t)duration {
- (void)seekTo:(int)location completionHandler:(void (^)(BOOL))completionHandler {
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;
[_player seekToTime:locationCMT
toleranceBefore:tolerance
toleranceAfter:tolerance
completionHandler:completionHandler];
// 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.

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

- (void)setIsLooping:(BOOL)isLooping {
Expand Down