[video_player] Improve KVO handling on iOS

[stuartmorgan-g]

This improves the KVO handling in FVPVideoPlayer in several ways:

• Eliminates the use of methods to do KVO registration and unregistration, since that involved calling self during init and dealloc, which is a dangerous anti-pattern.
• Eliminates NSKeyValueObservingOptionInitial from the registration, as this was non-obviously creating a call to self, within init, for every registration. In practice, none of them were necessary from what I could determine:
  • The handlers for most keys just collect information to send to the event listener, but there is no event listener during init (it's set just after), so those were expensive no-ops.
  • The handlers related to initialization require the item to be in the AVPlayerItemStatusReadyToPlay state, and items start in AVPlayerItemStatusUnknown, so the initial state call won't do anything useful.
• Stores a dictionary of registrations, which is used for unregistration, so that it's impossible to add a new key to the observation set but forget to unregister it. Previously the two lists of strings had to be kept in sync. New static functions (to avoid self calls) process those lists.
• Clears the list after unregistering to avoid double-unregistration locally to the class, instead of requiring a subclass to contain a workaround to avoid problems in the superclass code.

Pre-Review Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene page, which explains my responsibilities.
• I read and followed the relevant style guides and ran the auto-formatter.
• I signed the CLA.
• The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
• I linked to at least one issue that this PR fixes in the description above.
• I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or I have commented below to indicate which version change exemption this PR falls under¹.
• I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style, or I have commented below to indicate which CHANGELOG exemption this PR falls under¹.
• I updated/added any relevant documentation (doc comments with ///).
• I added new tests to check the change I am making, or I have commented below to indicate which test exemption this PR falls under¹.
• All existing and new tests are passing.

Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the gemini-code-assist bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.

Footnotes

1. Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩² ↩³

[flutter-dashboard]

It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging.

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.If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

[gemini-code-assist]

Code Review

This pull request refactors the Key-Value Observing (KVO) handling in FVPVideoPlayer to improve robustness and maintainability. The changes eliminate calls to self from init and dealloc by introducing static helper functions for observer registration and removal. It also moves to a dictionary-based approach for managing observers, which prevents mismatches between registration and unregistration, and makes the dispose method idempotent to avoid crashes on hot restart. The changes are well-structured and address a common anti-pattern in Objective-C. I have one minor suggestion to fix a typo in a comment.

[stuartmorgan-g]

test-exempt: code refactor with no semantic change

[LongCatIsLooong]

It looks like the original implementation specified NSKeyValueObservingOptionInitial, but here I don't see how the initial values are going to be reported?

[LongCatIsLooong]

In the PR description it says:

The handlers for most keys just collect information to send to the event listener, but there is no event listener during init (it's set just after), so those were expensive no-ops.
The handlers related to initialization require the item to be in the AVPlayerItemStatusReadyToPlay state, and items start in AVPlayerItemStatusUnknown, so the initial state call won't do anything useful.

So this means the item is always in .unknown state when the event listener is set? Is it possible to get an item that's already .failed?

[stuartmorgan-g]

Not according to the docs:

When a player item is created, its status is AVPlayerItem.Status.unknown, meaning its media hasn’t been loaded and has not yet been enqueued for playback. Associating a player item with an AVPlayer immediately begins enqueuing the item’s media and preparing it for playback.

[LongCatIsLooong]

Hmm I feel that doesn't guarantee that AVPlayer.status can't change synchronously especially when the KVO registration happens after _player = [avFactory playerWithPlayerItem:item]; (which I assume is when the player item is associated with an AVPlayer). On the other hand I agree that it probably doesn't make too much sense for AVFoundation to change the status synchronously (maybe if the AVPlayer implementation has logic that does simple sanity checks as soon as a new item is assigned?). Should we add an assert before the registration to make sure the status is .unknown?

[stuartmorgan-g]

especially when the KVO registration happens after _player = [avFactory playerWithPlayerItem:item]; (which I assume is when the player item is associated with an AVPlayer)

That's a great point; I wasn't considering the order of operations there if the internals make synchronous status changes. Rather than relying on undocumented behavior and asserting, I've instead moved the KVO registration to setEventListener:. That makes things much easier to reason about, because we're no longer doing any of this from init, so we can call whatever we want, and unlike the assert means clients won't get incorrect runtime behavior if AVPlayer ever starts actually doing that.

Rather than add back NSKeyValueObservingOptionInitial (which would cause a bunch of initial state event updates in Dart that didn't used to exist, and in a random order), I've added an explicit call to check the state and, if it has already changed, send an initialization event (which is now safe because this isn't calling methods from init) before registering the listeners.

(The listener is set right after init, so in practice this will have almost no effect on when any of this happens, but I think this structure is much cleaner.)

[LongCatIsLooong]

LGTM (I think you mentioned that the player instances will be re-created on hot-restart right?)

[stuartmorgan-g]

LGTM (I think you mentioned that the player instances will be re-created on hot-restart right?)

Yes, there's Dart code in the plugin that re-initializes the native code if a hot restart happens.