Skip to content
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

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

Merged
merged 10 commits into from
Nov 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
## 2.6.3

* Fixes VideoPlayerController.initialize() future never resolving with invalid video file.
* Adds more details to the error message returned by VideoPlayerController.initialize().

## 2.6.2

* Updates Pigeon for non-nullable collection type support.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -790,6 +790,39 @@ - (void)testPublishesInRegistration {
XCTAssertTrue([publishedValue isKindOfClass:[FVPVideoPlayerPlugin class]]);
}

- (void)testFailedToLoadVideoEventShouldBeAlwaysSent {
NSObject<FlutterPluginRegistrar> *registrar =
[GetPluginRegistry() registrarForPlugin:@"testFailedToLoadVideoEventShouldBeAlwaysSent"];
FVPVideoPlayerPlugin *videoPlayerPlugin =
[[FVPVideoPlayerPlugin alloc] initWithRegistrar:registrar];
FlutterError *error;

[videoPlayerPlugin initialize:&error];

FVPCreationOptions *create = [FVPCreationOptions makeWithAsset:nil
uri:@""
packageName:nil
formatHint:nil
httpHeaders:@{}];
NSNumber *textureId = [videoPlayerPlugin createWithOptions:create error:&error];
FVPVideoPlayer *player = videoPlayerPlugin.playersByTextureId[textureId];
XCTAssertNotNil(player);

[self keyValueObservingExpectationForObject:(id)player.player.currentItem
keyPath:@"status"
expectedValue:@(AVPlayerItemStatusFailed)];
[self waitForExpectationsWithTimeout:10.0 handler:nil];

XCTestExpectation *failedExpectation = [self expectationWithDescription:@"failed"];
[player onListenWithArguments:nil
eventSink:^(FlutterError *event) {
if ([event isKindOfClass:FlutterError.class]) {
[failedExpectation fulfill];
}
}];
[self waitForExpectationsWithTimeout:10.0 handler:nil];
}

#if TARGET_OS_IOS
- (void)validateTransformFixForOrientation:(UIImageOrientation)orientation {
AVAssetTrack *track = [[FakeAVAssetTrack alloc] initWithOrientation:orientation];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -345,13 +345,7 @@ - (void)observeValueForKeyPath:(NSString *)path
AVPlayerItem *item = (AVPlayerItem *)object;
switch (item.status) {
case AVPlayerItemStatusFailed:
if (_eventSink != nil) {
_eventSink([FlutterError
errorWithCode:@"VideoError"
message:[@"Failed to load video: "
stringByAppendingString:[item.error localizedDescription]]
details:nil]);
}
[self sendFailedToLoadVideoEvent];
break;
case AVPlayerItemStatusUnknown:
break;
Expand Down Expand Up @@ -406,6 +400,32 @@ - (void)updatePlayingState {
_displayLink.running = _isPlaying || self.waitingForFrame;
}

- (void)sendFailedToLoadVideoEvent {
if (_eventSink == nil) {
return;
}
// Prefer more detailed error information from tracks loading.
NSError *error;
if ([self.player.currentItem.asset statusOfValueForKey:@"tracks"
error:&error] != AVKeyValueStatusFailed) {
error = self.player.currentItem.error;
}
__block NSMutableOrderedSet<NSString *> *details =
[NSMutableOrderedSet orderedSetWithObject:@"Failed to load video"];
void (^add)(NSString *) = ^(NSString *detail) {
if (detail != nil) {
[details addObject:detail];
}
};
NSError *underlyingError = error.userInfo[NSUnderlyingErrorKey];
add(error.localizedDescription);
add(error.localizedFailureReason);
add(underlyingError.localizedDescription);
add(underlyingError.localizedFailureReason);
NSString *message = [details.array componentsJoinedByString:@": "];
_eventSink([FlutterError errorWithCode:@"VideoError" message:message details:nil]);
}

- (void)setupEventSinkIfReadyToPlay {
if (_eventSink && !_isInitialized) {
AVPlayerItem *currentItem = self.player.currentItem;
Expand Down Expand Up @@ -587,6 +607,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'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"?

// https://github.com/flutter/flutter/issues/151475
// https://github.com/flutter/flutter/issues/147707
if (self.player.currentItem.status == AVPlayerItemStatusFailed) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

[self sendFailedToLoadVideoEvent];
return nil;
}
[self setupEventSinkIfReadyToPlay];
return nil;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ name: video_player_avfoundation
description: iOS and macOS implementation of the video_player plugin.
repository: https://github.com/flutter/packages/tree/main/packages/video_player/video_player_avfoundation
issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+video_player%22
version: 2.6.2
version: 2.6.3

environment:
sdk: ^3.3.0
Expand Down