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

tarrinneal · 2023-04-20T23:29:11Z

Reduces avplayer seek accuracy when seeking to end of video to fix seekToTime never running completion handler.

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

flutter-dashboard · 2023-04-21T00:27:03Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

tarrinneal · 2023-04-21T01:55:03Z

Having a hard time writing tests that fail with the old code...

packages/video_player/video_player/example/integration_test/video_player_test.dart

packages/video_player/video_player_avfoundation/ios/Classes/FLTVideoPlayerPlugin.m

hellohuanlin · 2023-04-24T18:43:18Z

packages/video_player/video_player_avfoundation/ios/Classes/FLTVideoPlayerPlugin.m

+  // 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;


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

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.

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

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

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

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

tarrinneal · 2023-05-09T09:13:19Z

@stuartmorgan Tests are written, I'm certain the objc code can be better. So I'd love to get some thorough feedback, if you have time.

stuartmorgan-g

LGTM with nits

packages/video_player/video_player_avfoundation/ios/Classes/FLTVideoPlayerPlugin_Test.h

packages/video_player/video_player_avfoundation/example/ios/RunnerTests/VideoPlayerTests.m

stuartmorgan-g

LGTM

auto-submit · 2023-05-11T17:36:08Z

auto label is removed for flutter/packages, pr: 3784, due to - The status or check suite repo_checks has failed. Please fix the issues identified (or deflake) before re-applying this label.

…to end bug (flutter/packages#3784)

flutter/packages@6328cfb...6608b60 2023-05-15 [email protected] [ci] Remove unnecessary Linux libraries (flutter/packages#3975) 2023-05-15 [email protected] [ci] Enforce a minimum Kotlin version in examples (flutter/packages#3979) 2023-05-15 [email protected] [ci] Disable Windows repo tool tests (flutter/packages#3980) 2023-05-12 [email protected] [ci] Add LUCI repo tool tests (flutter/packages#3964) 2023-05-12 [email protected] [url_launcher] Set broadcast reciever visability as required by target api 34 (flutter/packages#3973) 2023-05-12 [email protected] Update build.gradle with a comment explaining version (flutter/packages#3972) 2023-05-12 [email protected] Roll Flutter from 8c5a1ea to a76dbe4 (12 revisions) (flutter/packages#3958) 2023-05-12 [email protected] [image_picker] Fix Android lints (flutter/packages#3887) 2023-05-12 [email protected] [various] Update minimum Flutter version to 3.3 (flutter/packages#3967) 2023-05-12 [email protected] [tool] Target specific Android unit tests (flutter/packages#3955) 2023-05-12 [email protected] [camerax] Add a dependency on kotlin-bom to align versions of kotlin-stdlib (flutter/packages#3960) 2023-05-12 [email protected] [local_auth] Improve iOS test DI (flutter/packages#3959) 2023-05-12 [email protected] [ci] Ensure scripts fail if a command fails (flutter/packages#3963) 2023-05-11 [email protected] [ci] Pin the Flutter version for `release` (flutter/packages#3965) 2023-05-11 [email protected] [video_player] foundation - reduce seek accuracy to fix seek to end bug (flutter/packages#3784) 2023-05-11 [email protected] [image_picker] Fix use_build_context_synchronously violations (flutter/packages#3969) 2023-05-11 [email protected] [tool] Ensure that publish credential path is available (flutter/packages#3970) 2023-05-11 [email protected] [go_router] change `fix_data.yaml` uris to be absolute to fix lints (flutter/packages#3877) 2023-05-11 [email protected] [tool] Use new pub cache location for publish (flutter/packages#3962) 2023-05-10 [email protected] Roll Flutter (stable) from f72efea to 84a1e90 (1920 revisions) (flutter/packages#3961) 2023-05-10 [email protected] [image_picker_android] Improved Bitmap resize on Android (flutter/packages#3423) 2023-05-10 [email protected] [camerax] Add Kotlin duplicate classes known error to README.md (flutter/packages#3957) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…ug (flutter#3784) Reduces avplayer seek accuracy when seeking to end of video to fix `seekToTime` never running completion handler. fixes flutter/flutter#124475

misos1 · 2024-09-16T18:32:00Z

packages/video_player/video_player_avfoundation/ios/Classes/FLTVideoPlayerPlugin.m

+  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;


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

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.

@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.

remove seek accuracy

45fb3a5

github-actions bot added p: video_player platform-ios labels Apr 20, 2023

tarrinneal added 4 commits April 20, 2023 16:53

minimal tolerance

426c067

tolerance only when seeking to duration

7d167e2

format

b78ae9b

changelog and comment

cc5b173

tarrinneal marked this pull request as ready for review April 21, 2023 00:25

tarrinneal requested a review from hellohuanlin as a code owner April 21, 2023 00:25

tarrinneal requested a review from stuartmorgan-g April 21, 2023 00:25

tarrinneal changed the title ~~[video_player] foundation - remove seek accuracy to fix seek to end bug~~ [video_player] foundation - reduce seek accuracy to fix seek to end bug Apr 21, 2023

flutter-dashboard bot added the needs tests label Apr 21, 2023

tests?

aa9fbbd

tarrinneal removed the needs tests label Apr 21, 2023

stuartmorgan-g requested changes Apr 22, 2023

View reviewed changes

tarrinneal requested a review from stuartmorgan-g April 24, 2023 16:21

clean up

041dcb3

tarrinneal force-pushed the video-player-seekToEnd-bug branch from b6c7651 to 041dcb3 Compare April 24, 2023 16:27

format

c6861f4

hellohuanlin reviewed Apr 24, 2023

View reviewed changes

tarrinneal added 7 commits May 2, 2023 17:09

DI

747a421

test set up

2e12e1a

move interface

d130337

Pesky *, I'm sure I'll revert this before it lands

7e8fada

no tolerance test

b61eace

Finish tests

6a586d1

Merge 'main' into video-player-seekToEnd-bug

c9bd628

format

3ef8801

tarrinneal requested a review from stuartmorgan-g May 10, 2023 08:12

stuartmorgan-g approved these changes May 10, 2023

View reviewed changes

Nits and stub change

46f3c7d

stuartmorgan-g approved these changes May 11, 2023

View reviewed changes

rename plugin registrar

fb9b9b8

tarrinneal added the autosubmit Merge PR when tree becomes green via auto submit App label May 11, 2023

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label May 11, 2023

format

7aeb3db

tarrinneal added the autosubmit Merge PR when tree becomes green via auto submit App label May 11, 2023

auto-submit bot merged commit 2323aa4 into flutter:main May 11, 2023

reidbaker mentioned this pull request May 12, 2023

[url_launcher] Set broadcast reciever visability as required by target api 34 #3973

Merged

11 tasks

engine-flutter-autoroll mentioned this pull request May 15, 2023

Roll Packages from 6328cfbd2a46 to 8febbba0c387 (21 revisions) flutter/flutter#126832

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 15, 2023

2323aa4 [video_player] foundation - reduce seek accuracy to fix seek …

952c6ac

…to end bug (flutter/packages#3784)

engine-flutter-autoroll mentioned this pull request May 15, 2023

Roll Packages from 6328cfbd2a46 to 8febbba0c387 (21 revisions) flutter/flutter#126859

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 15, 2023

2323aa4 [video_player] foundation - reduce seek accuracy to fix seek …

7e1ac57

…to end bug (flutter/packages#3784)

engine-flutter-autoroll mentioned this pull request May 15, 2023

Roll Packages from 6328cfbd2a46 to 6608b604b0e7 (22 revisions) flutter/flutter#126865

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 15, 2023

2323aa4 [video_player] foundation - reduce seek accuracy to fix seek …

beafbca

…to end bug (flutter/packages#3784)

This was referenced Jul 7, 2023

[Tool] New tool to download android dependencies #4408

Merged

[camera] tree fix format 2023 05 12 #4450

Closed

[tooling] mark infra steps as infra steps in yaml files #4473

Merged

reidbaker mentioned this pull request Jul 26, 2023

[Tooling] Add google owned cache for dependencies as an option in ci #4567

Merged

11 tasks

reidbaker mentioned this pull request Aug 4, 2023

[All] Expand artifact hub to all plugins #4645

Merged

11 tasks

misos1 reviewed Sep 16, 2024

View reviewed changes

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

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

Uh oh!

Conversation

tarrinneal commented Apr 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Apr 21, 2023

Uh oh!

tarrinneal commented Apr 21, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hellohuanlin Apr 24, 2023

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Apr 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hellohuanlin Apr 25, 2023

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Apr 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hellohuanlin Apr 26, 2023

Choose a reason for hiding this comment

Uh oh!

tarrinneal commented May 9, 2023

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

auto-submit bot commented May 11, 2023

Uh oh!

misos1 Sep 16, 2024

Choose a reason for hiding this comment

Uh oh!

misos1 Sep 16, 2024

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Sep 16, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tarrinneal commented Apr 20, 2023 •

edited

Loading

stuartmorgan-g Apr 25, 2023 •

edited

Loading

stuartmorgan-g Apr 26, 2023 •

edited

Loading