[video_player_avfoundation] send video load failure even when eventsink was initialized late #7194

misos1 · 2024-07-22T16:24:43Z

Event initialized is sent from onListenWithArguments by calling setupEventSinkIfReadyToPlay if event sink was nil during observeValueForKeyPath. This change also sends failure in a similar way. Adds more details to the error message returned by VideoPlayerController.initialize().

VideoPlayerController::initialize can return future which never resolves nor errors when called indirectly from StatefulWidget::initState flutter#151475
[video_player] VideoPlayerController.initialize() never return if the file is invalid (text file) flutter#147707
[video_player] Better exception message on iOS devices if the video fails to play flutter#56665 - now error message may look like: Failed to load video: Operation Stopped: The server is not correctly configured.: The operation couldn’t be completed. (CoreMediaErrorDomain error -12939 - byte range and no content length - error code is 200)

Tests testSeekToWhilePausedStartsDisplayLinkTemporarily and testSeekToWhilePlayingDoesNotStopDisplayLink are failing on the main branch on ios. Seems position is correctly set to 1234 in seekTo completion handler but right after [mockDisplayLink setRunning:YES] is set again to 0 and after some time back to 1234 so those two tests sometimes fail (seems more often when running all tests).

Pre-launch Checklist

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

hellohuanlin · 2024-07-25T19:03:59Z

...on/darwin/video_player_avfoundation/Sources/video_player_avfoundation/FVPVideoPlayerPlugin.m

@@ -406,9 +400,46 @@ - (void)updatePlayingState {
  _displayLink.running = _isPlaying || self.waitingForFrame;
 }

+- (void)sendFailedToLoadVideoEvent:(NSError *)error {
+  if (!NSThread.isMainThread) {


it looks like sendFailedToLoadVideoEvent is only called from background queue. Can this function simply be:

- (void)sendFailedToLoadVideoEventOnMainQueue:(NSError *)error { dispatch_async(dispatch_get_main_queue(), ^{ // all logic here } }

It is also called from setupEventSinkIfReadyToPlay and observeValueForKeyPath. I can make it without any "dispatch" and use dispatch_async just in trackCompletionHandler (or maybe performSelector like is already used in this function here performSelector:_cmd).

hellohuanlin · 2024-08-02T04:30:41Z

I have a hard time understanding this in the PR description:

Sends also failure similarly as is sent event initialized from onListenWithArguments through setupEventSinkIfReadyToPlay if event sink was not initialized during observeValueForKeyPath.

Could you rephrase a bit? That'd be helpful for a review. Thanks!

hellohuanlin · 2024-08-02T04:34:01Z

...on/darwin/video_player_avfoundation/Sources/video_player_avfoundation/FVPVideoPlayerPlugin.m

-                                stringByAppendingString:[item.error localizedDescription]]
-                    details:nil]);
-        }
+        [self sendFailedToLoadVideoEvent:item.error];


nit: sendFailedToLoadVideoEventIfNeeded since you moved the check inside

I would not say "if needed" sounds right here. Meaning of that check is rather like "if can" or "if possible".

hellohuanlin · 2024-08-02T04:34:44Z

...on/darwin/video_player_avfoundation/Sources/video_player_avfoundation/FVPVideoPlayerPlugin.m

+  };
+  NSError *underlyingError = error.userInfo[NSUnderlyingErrorKey];
+  // add more details to error message
+  // https://github.com/flutter/flutter/issues/56665


i dont think we need this comment

can remove this link.

hellohuanlin · 2024-08-02T04:36:10Z

...on/darwin/video_player_avfoundation/Sources/video_player_avfoundation/FVPVideoPlayerPlugin.m

+    // if status is Failed here then this was called from onListenWithArguments which means
+    // _eventSink was nil when was called observeValueForKeyPath with updated Failed status
+    // and error was not sent and never will be so it is needed to send it here to have
+    // future returned by initialize resolved instead of in state of infinite waiting,


Can you separate these 4 lines into multiple sentences? I have a hard time understanding this.

hellohuanlin · 2024-08-02T04:37:49Z

...on/darwin/video_player_avfoundation/Sources/video_player_avfoundation/FVPVideoPlayerPlugin.m

-          return;
+          case AVKeyValueStatusFailed:
+            // if something failed then future should result in error
+            [self sendFailedToLoadVideoEvent:error];


can you also specify which thread this is on

On some AVFoundation background queue (this is also mentioned in a comment a few lines above):

https://developer.apple.com/documentation/avfoundation/avasynchronouskeyvalueloading/1387321-loadvaluesasynchronouslyforkeys

hellohuanlin · 2024-08-19T20:55:09Z

...on/darwin/video_player_avfoundation/Sources/video_player_avfoundation/FVPVideoPlayerPlugin.m

+    // https://github.com/flutter/flutter/issues/151475
+    // https://github.com/flutter/flutter/issues/147707
+    if (currentItem.status == AVPlayerItemStatusFailed) {
+      [self sendFailedToLoadVideoEvent:currentItem.error];


can we move this to the caller, before setupEventSinkIfReadyToPlay is called?

or can you update the function name to setupEventSinkIfReadyToPlayOrReportFailure

There is also 4th place where this function is called masked by _cmd here self performSelector:_cmd, I would rather not put that check outside of that function.

Actually that function does not set up _eventsink, I would say it is set up by onListenWithArguments. This function just sends an event through that event sink.

On the other hand if status becomes failed on that second call it should be already caught in the key observer where the event sink is not nil and so it would send an error from there.

hellohuanlin · 2024-09-24T18:53:22Z

...on/darwin/video_player_avfoundation/Sources/video_player_avfoundation/FVPVideoPlayerPlugin.m

+          case AVKeyValueStatusFailed: {
+            // if something failed then future should result in error
+            dispatch_async(dispatch_get_main_queue(), ^{
+              [self sendFailedToLoadVideoEvent:error];


is this code the key change that fixed the bug?

Seems no, I think it is that change in onListenWithArguments. But without that change it seems also this change works as I now tested it with "Code sample 1" from flutter/flutter#151475.

hellohuanlin · 2024-09-24T18:58:16Z

...on/darwin/video_player_avfoundation/Sources/video_player_avfoundation/FVPVideoPlayerPlugin.m

@@ -587,6 +613,13 @@ - (FlutterError *_Nullable)onListenWithArguments:(id _Nullable)arguments
  // This line ensures the 'initialized' event is sent when the event
  // 'AVPlayerItemStatusReadyToPlay' fires before _eventSink is set (this function
  // onListenWithArguments is called)
+  // and also send error in similar case with 'AVPlayerItemStatusFailed'


by "similar case", do you mean AVPlayerItemStatusReadyToPlay fires before _eventSink is set? Can you be more explicit here in the comment? (Sorry I have some trouble understanding this bug fix)

Yes I was following up with that comment above. But this time with AVPlayerItemStatusFailed instead of AVPlayerItemStatusReadyToPlay. If anything fires before is event sink set then it could not be sent. Would that comment be better if I added "in similar case as above"?

hellohuanlin

Sorry for the delay.

hellohuanlin · 2024-11-06T19:04:35Z

...on/darwin/video_player_avfoundation/Sources/video_player_avfoundation/FVPVideoPlayerPlugin.m

+  // and also send error in similar case with 'AVPlayerItemStatusFailed'
+  // https://github.com/flutter/flutter/issues/151475
+  // https://github.com/flutter/flutter/issues/147707
+  if (self.player.currentItem.status == AVPlayerItemStatusFailed) {


So this is the part that actually fix the bug? Can you add a comment explaining how we ended up with Failed state here?

Sorry I did not manage to proceed with this. Yes when AVPlayerItemStatusFailed fires before _eventSink is set. AVPlayerItem is "started" to do its things and can asynchronously become ReadyToPlay or Failed before the dart side starts to listen where onListenWithArguments is called. My comment here follows up that comment before which tries to explain this but that previous fix connected with that comment omitted Failed state as possible state here which this line fixes (if player item can end up with success here it can also end up with failure, so that previous comment should explain it).

stuartmorgan · 2024-11-08T16:31:35Z

@hellohuanlin Please add tarrinneal (the ecosystem-level CODEOWNER) as the secondary reviewer once this has passed iOS review; the iOS team review is primary since this is internal to the iOS implementation.

hellohuanlin

LGTM after the nit on comment

…en eventsink was initialized late (flutter/packages#7194)

flutter/packages@e95f6d8...913b99e 2024-11-21 brackenavaron@gmail.com [vector_graphics] handle errors from bytes loader (flutter/packages#8080) 2024-11-21 8276375+derdilla@users.noreply.github.com [flutter_svg] Fix SvgNetworkLoader not closing internal http client (flutter/packages#8126) 2024-11-20 30872003+misos1@users.noreply.github.com [video_player_avfoundation] send video load failure even when eventsink was initialized late (flutter/packages#7194) 2024-11-20 kevmoo@users.noreply.github.com [flutter_markdown] enable Wasm support (flutter/packages#8120) 2024-11-20 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[url_launcher] Add Swift Package Manager integration to example app (#8128)" (flutter/packages#8136) 2024-11-20 737941+loic-sharma@users.noreply.github.com [url_launcher] Add Swift Package Manager integration to example app (flutter/packages#8128) 2024-11-20 stuartmorgan@google.com [pigeon] Enable example app build in CI (flutter/packages#8119) 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 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

* main: (64 commits) [quick_actions_plaform_interface] add localizedSubtitle (flutter#8112) [tools] Don't check license of generated Swift package (flutter#8137) Roll Flutter from 8536b96ebb3e to 93d772c5cdd8 (37 revisions) (flutter#8147) [go_router] Fix: Consistent PopScope Handling on Root Routes issue #140869 (flutter#8045) [in_app_purchase_storekit] fix price displayed with wrong precision (flutter#8127) [pigeon] Removes the `@protected` annotation from the InstanceManager field of the `PigeonInternalProxyApiBaseClass` (flutter#8125) [google_maps_flutter] Use structured Pigeon data on iOS (flutter#8142) [vector_graphics] handle errors from bytes loader (flutter#8080) [flutter_svg] Fix SvgNetworkLoader not closing internal http client (flutter#8126) [video_player_avfoundation] send video load failure even when eventsink was initialized late (flutter#7194) [flutter_markdown] enable Wasm support (flutter#8120) Reverts "[url_launcher] Add Swift Package Manager integration to example app (flutter#8128)" (flutter#8136) [url_launcher] Add Swift Package Manager integration to example app (flutter#8128) [pigeon] Enable example app build in CI (flutter#8119) [in_app_purchase_storekit] disallow ios versions lower than supported from enabling storekit (flutter#8110) [interactive_media_ads]: Bump com.google.ads.interactivemedia.v3:interactivemedia from 3.35.1 to 3.36.0 in /packages/interactive_media_ads/android (flutter#8046) [interactive_media_ads]: Bump androidx.annotation:annotation from 1.8.2 to 1.9.1 in /packages/interactive_media_ads/android (flutter#7980) [webview_flutter_android] Updates plugin to use `ProxyApis`s (flutter#7794) [interactive_media_ads] Adds support to define parameters that control the rendering of ads (flutter#8057) Roll Flutter from b3818f6b5979 to 8536b96ebb3e (22 revisions) (flutter#8124) ... # Conflicts: # packages/quick_actions/quick_actions_platform_interface/CHANGELOG.md

misos1 · 2024-11-22T16:07:00Z

I forgot to prepend those 3 issues with fixes. They can probably be closed now (or at least those which this PR covers in full).

stuartmorgan · 2024-11-22T16:37:34Z

Thanks, I closed one, and left a comment in another that is likely fixed but needs re-testing (unless you've already tested that one? If so we can close it). For the third it's not clear to me whether it's fully resolved.

misos1 · 2024-11-22T17:32:38Z

That first which you closed actually contained another behaviour which is not covered by this PR. But I think it is rather not a fault in the package or flutter sdk but rather with something how flutter runs apps on ios devices. Seems sometimes the app "disappears" after running but after closer inspection it seems it was just probably somehow "pushed" to background.

I probably tested the second with some text file but I am not sure and probably not with that original file and probably not on macos either. Anyway the best would be confirmation from the author.

That third is a mess. Seems there are several different groups who interpret that issue differently. Sticking to what author wrote in description and repeated in comments seems that the issue is just about better error message flutter/flutter#56665 (comment). So the question is whether issue should be interpreted differently if a lot of users expect that that issue should be closed by fixing underlying behaviour (which probably cannot be done from flutter or video_player side as it does not have anything to do with http communication here, if of course there is not some flag somewhere in AVFoundation). Maybe better to ask the author also here (or maybe wait and see if any new comments are added in next few months).

…nk was initialized late (flutter#7194) Event `initialized` is sent from `onListenWithArguments` by calling `setupEventSinkIfReadyToPlay` if event sink was `nil` during `observeValueForKeyPath`. This change also sends failure in a similar way. Adds more details to the error message returned by `VideoPlayerController.initialize()`. - flutter/flutter#151475 - flutter/flutter#147707 - flutter/flutter#56665 - now error message may look like: `Failed to load video: Operation Stopped: The server is not correctly configured.: The operation couldnâ��t be completed. (CoreMediaErrorDomain error -12939 - byte range and no content length - error code is 200)` Tests `testSeekToWhilePausedStartsDisplayLinkTemporarily` and `testSeekToWhilePlayingDoesNotStopDisplayLink` are failing on the main branch on ios. Seems position is correctly set to 1234 in `seekTo` completion handler but right after `[mockDisplayLink setRunning:YES]` is set again to 0 and after some time back to 1234 so those two tests sometimes fail (seems more often when running all tests).

github-actions bot added platform-ios platform-macos p: video_player labels Jul 22, 2024

misos1 force-pushed the complete_on_error branch 3 times, most recently from 3ea2335 to 756f3b9 Compare July 22, 2024 16:49

send video load failure even when eventsink was initialized late

16d3d7a

misos1 force-pushed the complete_on_error branch from 756f3b9 to 16d3d7a Compare July 22, 2024 17:20

misos1 marked this pull request as ready for review July 23, 2024 18:25

misos1 requested a review from hellohuanlin as a code owner July 23, 2024 18:25

hellohuanlin requested a review from tarrinneal July 24, 2024 21:27

hellohuanlin reviewed Jul 25, 2024

View reviewed changes

hellohuanlin reviewed Aug 2, 2024

View reviewed changes

misos1 added 3 commits August 15, 2024 20:25

move dispatch_async and change comments

12a061e

Merge branch 'main' into complete_on_error

902c3e6

add braces

4ab3c53

hellohuanlin reviewed Aug 19, 2024

View reviewed changes

misos1 added 2 commits August 20, 2024 17:09

rename function

1e0964f

move failure check outside

89db486

misos1 requested a review from hellohuanlin August 27, 2024 20:18

hellohuanlin reviewed Sep 24, 2024

View reviewed changes

hellohuanlin requested review from stuartmorgan and removed request for tarrinneal September 24, 2024 18:58

misos1 added 4 commits October 17, 2024 20:30

prefer to use error from tracks loading

1a6fd6f

fix format

60330dc

Merge branch 'main' into complete_on_error

01b21ac

fix format

ab57c33

misos1 requested a review from hellohuanlin October 17, 2024 19:07

hellohuanlin reviewed Nov 6, 2024

View reviewed changes

stuartmorgan added the triage-ios Should be looked at in iOS triage label Nov 8, 2024

stuartmorgan removed their request for review November 8, 2024 16:30

hellohuanlin approved these changes Nov 8, 2024

View reviewed changes

hellohuanlin requested a review from tarrinneal November 8, 2024 19:11

tarrinneal approved these changes Nov 19, 2024

View reviewed changes

cbracken added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 20, 2024

auto-submit bot merged commit 2703b67 into flutter:main Nov 20, 2024
76 checks passed

engine-flutter-autoroll mentioned this pull request Nov 21, 2024

Roll Packages from e95f6d8fce58 to 913b99ed8542 (7 revisions) flutter/flutter#159268

Merged

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

2703b67eb [video_player_avfoundation] send video load failure even wh…

0917225

…en eventsink was initialized late (flutter/packages#7194)

stuartmorgan mentioned this pull request Nov 22, 2024

[video_player] VideoPlayerController.initialize() never return if the file is invalid (text file) flutter/flutter#147707

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[video_player_avfoundation] send video load failure even when eventsink was initialized late #7194

[video_player_avfoundation] send video load failure even when eventsink was initialized late #7194

misos1 commented Jul 22, 2024 •

edited

Loading

hellohuanlin Jul 25, 2024

misos1 Jul 25, 2024 •

edited

Loading

hellohuanlin commented Aug 2, 2024

hellohuanlin Aug 2, 2024

misos1 Aug 15, 2024

hellohuanlin Aug 2, 2024

hellohuanlin Aug 19, 2024

hellohuanlin Aug 2, 2024

hellohuanlin Aug 2, 2024

misos1 Aug 15, 2024

hellohuanlin Aug 19, 2024

hellohuanlin Aug 19, 2024

misos1 Aug 20, 2024

misos1 Aug 20, 2024

misos1 Aug 20, 2024

hellohuanlin Sep 24, 2024

misos1 Oct 14, 2024

hellohuanlin Sep 24, 2024

misos1 Oct 14, 2024

hellohuanlin left a comment

hellohuanlin Nov 6, 2024

misos1 Nov 22, 2024

stuartmorgan commented Nov 8, 2024

hellohuanlin left a comment

misos1 commented Nov 22, 2024

stuartmorgan commented Nov 22, 2024

misos1 commented Nov 22, 2024

[video_player_avfoundation] send video load failure even when eventsink was initialized late #7194

[video_player_avfoundation] send video load failure even when eventsink was initialized late #7194

Conversation

misos1 commented Jul 22, 2024 • edited Loading

Pre-launch Checklist

Choose a reason for hiding this comment

misos1 Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

hellohuanlin commented Aug 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hellohuanlin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartmorgan commented Nov 8, 2024

hellohuanlin left a comment

Choose a reason for hiding this comment

misos1 commented Nov 22, 2024

stuartmorgan commented Nov 22, 2024

misos1 commented Nov 22, 2024

misos1 commented Jul 22, 2024 •

edited

Loading

misos1 Jul 25, 2024 •

edited

Loading