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
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
## NEXT
## 2.6.2

* Fixes VideoPlayerController.initialize() future never resolving with invalid video file.
* Adds more details to the error message returned by VideoPlayerController.initialize().
* Updates minimum supported SDK version to Flutter 3.19/Dart 3.3.

## 2.6.1
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:item.error];
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: sendFailedToLoadVideoEventIfNeeded since you moved the check inside

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

break;
case AVPlayerItemStatusUnknown:
break;
Expand Down Expand Up @@ -406,9 +400,38 @@ - (void)updatePlayingState {
_displayLink.running = _isPlaying || self.waitingForFrame;
}

- (void)sendFailedToLoadVideoEvent:(NSError *)error {
if (_eventSink == nil) {
return;
}
__block NSString *message = @"Failed to load video";
void (^add)(NSString *) = ^(NSString *detail) {
if (detail != nil) {
message = [message stringByAppendingFormat:@": %@", detail];
}
};
NSError *underlyingError = error.userInfo[NSUnderlyingErrorKey];
// https://github.com/flutter/flutter/issues/56665
Copy link
Contributor

Choose a reason for hiding this comment

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

i dont think we need this comment

Copy link
Contributor

Choose a reason for hiding this comment

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

can remove this link.

add(error.localizedDescription);
add(error.localizedFailureReason);
add(underlyingError.localizedDescription);
add(underlyingError.localizedFailureReason);
_eventSink([FlutterError errorWithCode:@"VideoError" message:message details:nil]);
}

- (void)setupEventSinkIfReadyToPlay {
if (_eventSink && !_isInitialized) {
AVPlayerItem *currentItem = self.player.currentItem;
// if status is Failed this was probably called from onListenWithArguments, which means
// _eventSink was initialized to non-nil probably only after observeValueForKeyPath already
// observed Failed status and it did not send error, therefore we need to send it here
// see comment in onListenWithArguments
// https://github.com/flutter/flutter/issues/151475
// https://github.com/flutter/flutter/issues/147707
if (currentItem.status == AVPlayerItemStatusFailed) {
[self sendFailedToLoadVideoEvent:currentItem.error];
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

or can you update the function name to setupEventSinkIfReadyToPlayOrReportFailure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

return;
}
CGSize size = currentItem.presentationSize;
CGFloat width = size.width;
CGFloat height = size.height;
Expand All @@ -417,13 +440,27 @@ - (void)setupEventSinkIfReadyToPlay {
AVAsset *asset = currentItem.asset;
if ([asset statusOfValueForKey:@"tracks" error:nil] != AVKeyValueStatusLoaded) {
void (^trackCompletionHandler)(void) = ^{
if ([asset statusOfValueForKey:@"tracks" error:nil] != AVKeyValueStatusLoaded) {
NSError *error;
switch ([asset statusOfValueForKey:@"tracks" error:&error]) {
case AVKeyValueStatusLoaded:
// This completion block will run on an AVFoundation background queue.
// Hop back to the main thread to set up event sink.
[self performSelector:_cmd
onThread:NSThread.mainThread
withObject:self
waitUntilDone:NO];
break;
// Cancelled, or something failed.
return;
case AVKeyValueStatusFailed: {
// if something failed then future should result in error
dispatch_async(dispatch_get_main_queue(), ^{
[self sendFailedToLoadVideoEvent:error];
Copy link
Contributor

Choose a reason for hiding this comment

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

is this code the key change that fixed the bug?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

});
break;
}
default:
break;
}
// This completion block will run on an AVFoundation background queue.
// Hop back to the main thread to set up event sink.
[self performSelector:_cmd onThread:NSThread.mainThread withObject:self waitUntilDone:NO];
};
[asset loadValuesAsynchronouslyForKeys:@[ @"tracks" ]
completionHandler:trackCompletionHandler];
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.1
version: 2.6.2

environment:
sdk: ^3.3.0
Expand Down