-
Notifications
You must be signed in to change notification settings - Fork 3.4k
[video_player_avfoundation] Make sure the AVPlayerItem
is .readyToPlay
before emitting an initialized
event
#9534
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
[video_player_avfoundation] Make sure the AVPlayerItem
is .readyToPlay
before emitting an initialized
event
#9534
Conversation
AVPlayerItem
is .readyToPlay
before emitting an initialized
eventAVPlayerItem
is .readyToPlay
before emitting an initialized
event
I'm still trying to generate a sample video with 0 duration (which seems to be an AVFoundation bug as the video plays in chrome, but not in quicktime) so I can add a test. But much of this change should already be covered by existing tests added for the workarounds in the past. |
CGSize size = currentItem.presentationSize; | ||
CGFloat width = size.width; | ||
CGFloat height = size.height; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the duration / size checks since now the caller has to guarantee the item is .readyToPlay
and I think those checks shouldn't be necessary.
Before this you may enter this if
block even when the item's status is .unknown
and I believe that's why we added the workarounds (size / duration checks).
_eventSink = nil; | ||
_isInitialized = NO; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_isInitialized
is now reset to NO
on onListenWithArguments
. According to the documentation:
The channel implementation may call this method with nil arguments to separate a pair of two consecutive set up requests. Such request pairs may occur during Flutter hot restart.
So this makes sure the new event sink gets an initialized
event in case it's a hot restart (in which case I think the dart vm will also restart erasing all user states?). But if the engine is allowed to call onCancelWithArguments
and onListenWithArguments
whenever it wants then the dart side may receive more than one initialized
event.
@@ -1,3 +1,7 @@ | |||
## 2.8.1 | |||
|
|||
* Removes unnecessary workarounds, fixes "initialized" event not firing when the duration of the media is 0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removes unnecessary workarounds
What workarounds?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The size / duration checks removed in the sendInitialized
method.
} else if (context == statusContext) { | ||
AVPlayerItem *item = (AVPlayerItem *)object; | ||
switch (item.status) { | ||
case AVPlayerItemStatusFailed: | ||
[self sendFailedToLoadVideoEvent]; | ||
break; | ||
case AVPlayerItemStatusUnknown: | ||
break; | ||
case AVPlayerItemStatusReadyToPlay: | ||
[item addOutput:_videoOutput]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If status is changed to AVPlayerItemStatusReadyToPlay
before the event sink is set up then it seems this addOutput
will not be called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops thanks for catching this!
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request simplifies the video initialization logic by correctly using the AVPlayerItem.status
. The removal of workarounds makes the code cleaner and easier to understand. The added test and updated documentation are also valuable contributions.
I've added a few suggestions to further improve robustness and maintainability in the Objective-C code, mainly around defensive checks and handling of enum values in switch
statements. Overall, this is a solid improvement.
switch (self.player.currentItem.status) { | ||
case AVPlayerItemStatusUnknown: | ||
// When this method is called when the media is still loading, do nothing: | ||
// sendVideoInitializedEvent or sendFailedToLoadVideoEvent will be called | ||
// by KVO on status updates. | ||
return nil; | ||
case AVPlayerItemStatusReadyToPlay: | ||
[self sendVideoInitializedEvent]; | ||
return nil; | ||
case AVPlayerItemStatusFailed: | ||
[self sendFailedToLoadVideoEvent]; | ||
return nil; | ||
} | ||
[self setupEventSinkIfReadyToPlay]; | ||
return nil; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is declared to return a FlutterError *
(or nil
), but it lacks a default
case in the switch
statement. If self.player.currentItem.status
has a value not covered by the case
statements (e.g., if the enum is extended in a future SDK), this function would have undefined behavior. Adding a default
case that returns nil
will fix this correctness issue.
switch (self.player.currentItem.status) {
case AVPlayerItemStatusUnknown:
// When this method is called when the media is still loading, do nothing:
// sendVideoInitializedEvent or sendFailedToLoadVideoEvent will be called
// by KVO on status updates.
return nil;
case AVPlayerItemStatusReadyToPlay:
[self sendVideoInitializedEvent];
return nil;
case AVPlayerItemStatusFailed:
[self sendFailedToLoadVideoEvent];
return nil;
default:
NSAssert(NO, @"Unknown AVPlayerItemStatus: %ld", (long)self.player.currentItem.status);
return nil;
}
} else if (context == statusContext) { | ||
AVPlayerItem *item = (AVPlayerItem *)object; | ||
switch (item.status) { | ||
case AVPlayerItemStatusFailed: | ||
[self sendFailedToLoadVideoEvent]; | ||
break; | ||
case AVPlayerItemStatusUnknown: | ||
break; | ||
case AVPlayerItemStatusReadyToPlay: | ||
[item addOutput:_videoOutput]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To improve robustness, it's safer to check if the video output has already been added to the player item before calling addOutput:
. This check prevents potential issues if the player item status transitions in unexpected ways.
if (![item.outputs containsObject:_videoOutput]) {
[item addOutput:_videoOutput];
}
Fixes flutter/flutter#166914. The duration check was first introduced here but zero division should probably be handled by the client not the plugin.
Also I suspect that flutter/flutter#91975 wasn't caused by an AVFoundation bug but rather a bug in
video_player_avfoundation
:The
_isInitialized
flag was roughly equivalent to.readyToPlay
a long time ago. The flag was later re-purposed to indicate whether aninitialized
message has already been sent (to make sure the message is only sent once), and that PR also introduced a size check to determine whether the media is loaded (which doesn't seem to be correct,.readyToPlay
is the public API for this, see Apple's AVPlayerDemo). But that PR didn't update theonListenWithArguments
implementation so the plugin started sendinginitialized
events on first subscription even theAVPlayerItem
isn't ready to play. So that looks like a bug in the plugin not in AVFoundation and that allows us to remove a bunch of workarounds.Couldn't reproduce flutter/flutter#91975 on iOS16 with this patch (I can't install iOS15 sim runtime, it fails silently, but on iOS16 it still hits the size + duration check callpath so I think not much has changed from iOS15 to iOS16 as far as AVFoundation stuff is concerned).
I didn't try to reproduce flutter/flutter#19197 with this patch since the issue doesn't have a minimal repro. I did not change the
_isInitialized
semantic so I assume the patch won't regress that fix.More on
isInitialized
:The player implementation doesn't have to rely on this property, if
AVPlayerItem.Status
can not recover from.failed
to.readyToPlay
(i.e. it can only change to.readyToPlay
once). But the documentation doesn't say if that is the case.Pre-Review Checklist
[shared_preferences]
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 under1.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 under1.///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2 ↩3