[video_player] Improve macOS frame management #5078

stuartmorgan · 2023-10-05T20:04:57Z

Fixes some logic around getting new frames on macOS:

The logic to copy a frame had a race condition; the macOS code checks that a frame is available for the current playback time before informing the engine that a frame is available (it's not clear why the iOS code doesn't do this; apparently the engine tolerates returning NULL frames on iOS?), but failed to account for the fact that the current playback time when the (async) request from the engine comes in can be different. This fixes it to remember the last time that was known to be available, and uses that when the current time isn't available. This fixes flickering on playback (since returning NULL on macOS causes the video to vanish until a new frame is available).
Fixes seek to temporarily restart the display link if the video is paused, rather than telling the engine that a frame is available, because it might not be. This is changed for both macOS and iOS since I don't see any reason this bug couldn't affect iOS as well (although in practice I'm only aware of it being reproducible on macOS).

This extracts the display link code for macOS and iOS into an abstraction, eliminating most of the ifdefing, in order to support the latter (since more code needs to be able to play/pause the display link), which also resolves a TODO from the initial implementation.

There is also some non-trivial addition of factory injection in order to make the code more testable. This is definitely not complete, but it incrementally moves the code toward being more testable than it was before, and allows for testing the display link behavior.

Lastly, this moves some code used by tests to the existing _Test.h header, removing redeclarations from unit test files, since we already have a test header and that's our preferred approach for accessing private details in ObjC tests. (Longer term the multi-class mega-file should be broken up more to reduce the need for that.)

Fixes flutter/flutter#136027
Improves flutter/flutter#135999

Pre-launch Checklist

While the display-link-driven frame updater will only inform the engine that a frame is available when the underlying player has a frame for the current time, there's no guarantee that the player will have a frame for the time that is current when the engine actually requests the frame. To avoid flickering from returning no frame, if a frame isn't available for the current time when it is requested, provide the frame that was known to be available when the request was triggered in the first place. Fixes flutter/flutter#135999

stuartmorgan · 2023-10-05T20:06:19Z

I can pull a1d46cb out into a prequel PR if it would help with review, but hopefully just having it as a separate commit here makes it easy enough.

stuartmorgan · 2023-10-05T20:09:28Z

(I can also do some of the testability refactoring, like moving things to the Test header and adjusting the AV* factory injection, in a prequel PR if desired.)

hellohuanlin · 2023-10-09T19:28:32Z

CC: @hellohuanlin

packages/video_player/video_player_avfoundation/darwin/Classes/ios/FVPDisplayLink.m

hellohuanlin · 2023-11-13T22:27:45Z

packages/video_player/video_player_avfoundation/darwin/Classes/FVPVideoPlayerPlugin.m

+    if ([self.videoOutput hasNewPixelBufferForItemTime:outputItemTime]) {
+      _lastKnownAvailableTime = outputItemTime;
+      reportFrame = YES;
+    }


Reset in else?

else { _lastKnownAvailableTime = kCMTimeInvalid; reportFrame = NO; }

reportFrame is a local initialized to NO, so there's no need to set it to NO.

Why would we reset _lastKnownAvailableTime? If we don't have a new time, then the last known time is still the last known time.

hellohuanlin · 2023-11-13T22:36:51Z

packages/video_player/video_player_avfoundation/darwin/Classes/FVPVideoPlayerPlugin.m

+    }
+  }
+
+  if (self.waitingForFrame && buffer) {


When will buffer be nil (seeking to the end?)

should this be just if (self.waitingForFrame) so that displayLink is guaranteed to shutdown?

When will buffer be nil (seeking to the end?)

Seeking to the end should work. It could be nil on iOS since iOS just fires the display link all the time and relies on the engine (apparently) allowing null frames. There may also be edge cases on macOS where we still can get nulls.

should this be just if (self.waitingForFrame) so that displayLink is guaranteed to shutdown?

But if we're waiting for a frame and we didn't get one yet, then we need to wait longer.

Consider iOS seeking while paused, for instance, and then firing the display link indiscriminately before the video frame is actually ready. In that case, we don't want to stop the display link or the frame will be stuck on the old location. Instead we need to wait until the video decoding catches up and there's a frame to show.

It may be worth documenting the macOS edge cases here in a comment.

To clarify, I don't actually know what the macOS edge cases are. Recent comments in flutter/flutter#135999 suggest they still exist, but I don't yet know when/why.

hellohuanlin

LGTM!

packages/video_player/video_player_avfoundation/darwin/Classes/FVPVideoPlayerPlugin.m

cbracken

lgtm modulo a couple tiny doc nits.

auto-submit · 2023-11-14T20:23:39Z

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

stuartmorgan · 2023-11-14T20:25:41Z

Hm, looks like one of the new tests may be flaky. I'll investigate that.

stuartmorgan · 2023-11-16T16:18:42Z

I couldn't get it to flake locally in many attempts, and there's no info about what failed on the bot, so I'm going to land it and see if it was a fluke, or if it shows up again (hopefully with details about what actually failed).

…es#5078)

flutter/packages@07b4b29...c5443ad 2023-11-20 JeroenWeener@users.noreply.github.com [webview_flutter] Support for handling basic authentication requests (Platform Interface) (flutter/packages#5362) 2023-11-18 engine-flutter-autoroll@skia.org Roll Flutter from 53a57ad to 6cf9ab0 (31 revisions) (flutter/packages#5426) 2023-11-18 kevmoo@users.noreply.github.com [shared_preferences_web] migrate to pkg:web (flutter/packages#5414) 2023-11-18 stuartmorgan@google.com [ci] Roll minimum allowable Flutter to 3.10 (flutter/packages#5425) 2023-11-18 43054281+camsim99@users.noreply.github.com [path_provider_android] Run tests on AVDs running Android 34 (flutter/packages#5222) 2023-11-17 tarrinneal@gmail.com [pigeon] isEnum, isClass, fix swift casting, default values, optional method arguments, named method arguments (flutter/packages#5355) 2023-11-17 stuartmorgan@google.com [plugin_platform_interface] Switch mixin to `mixin class` (flutter/packages#5420) 2023-11-17 stuartmorgan@google.com [go_router] Fixes use of `Iterable` (flutter/packages#5421) 2023-11-17 stuartmorgan@google.com [pigeon] Adds `analyzer` 6.x compatibility (flutter/packages#5418) 2023-11-17 stuartmorgan@google.com Update release step to 3.16 (flutter/packages#5416) 2023-11-17 kevmoo@users.noreply.github.com [file_selector_web] migrate to pkg:web (flutter/packages#5413) 2023-11-16 stuartmorgan@google.com [video_player] Improve macOS frame management (flutter/packages#5078) 2023-11-16 engine-flutter-autoroll@skia.org Roll Flutter from e8c2bb1 to 53a57ad (39 revisions) (flutter/packages#5412) 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 flutter-ecosystem@google.com,rmistry@google.com 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://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

Fixes some logic around getting new frames on macOS: - The logic to copy a frame had a race condition; the macOS code checks that a frame is available for the current playback time before informing the engine that a frame is available (it's not clear why the iOS code doesn't do this; apparently the engine tolerates returning NULL frames on iOS?), but failed to account for the fact that the current playback time when the (async) request from the engine comes in can be different. This fixes it to remember the last time that was known to be available, and uses that when the current time isn't available. This fixes flickering on playback (since returning NULL on macOS causes the video to vanish until a new frame is available). - Fixes seek to temporarily restart the display link if the video is paused, rather than telling the engine that a frame is available, because it might not be. This is changed for both macOS and iOS since I don't see any reason this bug couldn't affect iOS as well (although in practice I'm only aware of it being reproducible on macOS). This extracts the display link code for macOS and iOS into an abstraction, eliminating most of the ifdefing, in order to support the latter (since more code needs to be able to play/pause the display link), which also resolves a TODO from the initial implementation. There is also some non-trivial addition of factory injection in order to make the code more testable. This is definitely not complete, but it incrementally moves the code toward being more testable than it was before, and allows for testing the display link behavior. Lastly, this moves some code used by tests to the existing `_Test.h` header, removing redeclarations from unit test files, since we already have a test header and that's our preferred approach for accessing private details in ObjC tests. (Longer term the multi-class mega-file should be broken up more to reduce the need for that.) Fixes flutter/flutter#136027 Improves flutter/flutter#135999

stuartmorgan added 7 commits October 4, 2023 17:47

Factor out display link code for easier use

a1d46cb

Fix seeking while paused

eba10b9

Minor improvements

b24e792

Fix and add tests

1a489e5

Autoformat

419859d

Version bump

109ca02

stuartmorgan requested a review from cbracken October 5, 2023 20:04

stuartmorgan requested a review from hellohuanlin as a code owner October 5, 2023 20:04

github-actions bot added platform-ios platform-macos p: video_player labels Oct 5, 2023

stuartmorgan added 2 commits October 5, 2023 20:13

Bump iOS OCMock version

1a59cc1

Fix accidental registrar duplication in tests

c9ca774

stuartmorgan added 2 commits November 9, 2023 11:03

Merge branch 'main' into video-player-macos-fallback-frame

b17b65a

Update new unit tests for changes on main

8295980

stuartmorgan force-pushed the video-player-macos-fallback-frame branch from 64d2898 to 8295980 Compare November 9, 2023 16:09

hellohuanlin reviewed Nov 13, 2023

View reviewed changes

stuartmorgan added 3 commits November 14, 2023 10:48

Merge branch 'main' into video-player-macos-fallback-frame

08d7497

Add running implementations

8f7dcbe

Added more comments

b6a55a0

stuartmorgan requested a review from hellohuanlin November 14, 2023 16:08

hellohuanlin approved these changes Nov 14, 2023

View reviewed changes

cbracken reviewed Nov 14, 2023

View reviewed changes

packages/video_player/video_player_avfoundation/darwin/Classes/FVPVideoPlayerPlugin.m Outdated Show resolved Hide resolved

cbracken approved these changes Nov 14, 2023

View reviewed changes

Add issue link

9ce3e8c

stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 14, 2023

stuartmorgan mentioned this pull request Nov 14, 2023

[video_player_avfoundation] Flickering on macOS flutter/flutter#135999

Open

2 tasks

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

stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 16, 2023

auto-submit bot merged commit 25574f9 into flutter:main Nov 16, 2023
79 checks passed

stuartmorgan deleted the video-player-macos-fallback-frame branch November 16, 2023 16:19

engine-flutter-autoroll mentioned this pull request Nov 17, 2023

Roll Packages from 07b4b298face to 179928a1f7cc (4 revisions) flutter/flutter#138622

Closed

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

25574f9 [video_player] Improve macOS frame management (flutter/packag…

2e60bba

…es#5078)

engine-flutter-autoroll mentioned this pull request Nov 17, 2023

Roll Packages from 07b4b298face to 179928a1f7cc (4 revisions) flutter/flutter#138631

Closed

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

25574f9 [video_player] Improve macOS frame management (flutter/packag…

345ef85

…es#5078)

engine-flutter-autoroll mentioned this pull request Nov 20, 2023

Roll Packages from 07b4b298face to c5443ad7e7e0 (13 revisions) flutter/flutter#138724

Merged

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

25574f9 [video_player] Improve macOS frame management (flutter/packag…

31b72f7

…es#5078)

reidbaker mentioned this pull request Jan 12, 2024

Dialog Debug #5881

Closed

reidbaker mentioned this pull request Jan 19, 2024

[in_app_purchase] Add play country code api #5941

Merged

12 tasks

reidbaker mentioned this pull request Feb 19, 2024

[in_app_purchase_android] Add UserChoiceBilling mode. #6162

Merged

16 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[video_player] Improve macOS frame management #5078

[video_player] Improve macOS frame management #5078

stuartmorgan commented Oct 5, 2023 •

edited

Loading

stuartmorgan commented Oct 5, 2023

stuartmorgan commented Oct 5, 2023

hellohuanlin commented Oct 9, 2023

hellohuanlin Nov 13, 2023

stuartmorgan Nov 14, 2023

hellohuanlin Nov 13, 2023

stuartmorgan Nov 14, 2023

cbracken Nov 14, 2023

stuartmorgan Nov 14, 2023

hellohuanlin left a comment

cbracken left a comment

auto-submit bot commented Nov 14, 2023

stuartmorgan commented Nov 14, 2023

stuartmorgan commented Nov 16, 2023

[video_player] Improve macOS frame management #5078

[video_player] Improve macOS frame management #5078

Conversation

stuartmorgan commented Oct 5, 2023 • edited Loading

Pre-launch Checklist

stuartmorgan commented Oct 5, 2023

stuartmorgan commented Oct 5, 2023

hellohuanlin commented Oct 9, 2023

hellohuanlin Nov 13, 2023

Choose a reason for hiding this comment

stuartmorgan Nov 14, 2023

Choose a reason for hiding this comment

hellohuanlin Nov 13, 2023

Choose a reason for hiding this comment

stuartmorgan Nov 14, 2023

Choose a reason for hiding this comment

cbracken Nov 14, 2023

Choose a reason for hiding this comment

stuartmorgan Nov 14, 2023

Choose a reason for hiding this comment

hellohuanlin left a comment

Choose a reason for hiding this comment

cbracken left a comment

Choose a reason for hiding this comment

auto-submit bot commented Nov 14, 2023

stuartmorgan commented Nov 14, 2023

stuartmorgan commented Nov 16, 2023

stuartmorgan commented Oct 5, 2023 •

edited

Loading