packages icon indicating copy to clipboard operation
packages copied to clipboard

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

Open misos1 opened this issue 1 year ago • 1 comments

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().

  • https://github.com/flutter/flutter/issues/151475
  • https://github.com/flutter/flutter/issues/147707
  • https://github.com/flutter/flutter/issues/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.

misos1 avatar Jul 22 '24 16:07 misos1

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 avatar Aug 02 '24 04:08 hellohuanlin

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

stuartmorgan-g avatar Nov 08 '24 16:11 stuartmorgan-g

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

misos1 avatar Nov 22 '24 16:11 misos1

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.

stuartmorgan-g avatar Nov 22 '24 16:11 stuartmorgan-g

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 https://github.com/flutter/flutter/issues/56665#issuecomment-1280557707. 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).

misos1 avatar Nov 22 '24 17:11 misos1