Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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.2

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

## 2.6.1

* Adds files to make include directory permanent.
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,46 @@ - (void)updatePlayingState {
_displayLink.running = _isPlaying || self.waitingForFrame;
}

- (void)sendFailedToLoadVideoEvent:(NSError *)error {
if (!NSThread.isMainThread) {
Copy link
Contributor

Choose a reason for hiding this comment

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

it looks like sendFailedToLoadVideoEvent is only called from background queue. Can this function simply be:

- (void)sendFailedToLoadVideoEventOnMainQueue:(NSError *)error {
  dispatch_async(dispatch_get_main_queue(), ^{
    // all logic here
  }
}

Copy link
Contributor Author

@misos1 misos1 Jul 25, 2024

Choose a reason for hiding this comment

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

It is also called from setupEventSinkIfReadyToPlay and observeValueForKeyPath. I can make it without any "dispatch" and use dispatch_async just in trackCompletionHandler (or maybe performSelector like is already used in this function here performSelector:_cmd).

dispatch_async(dispatch_get_main_queue(), ^{
[self sendFailedToLoadVideoEvent:error];
});
return;
}
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];
// add more details to error message
// 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 here then this was called from onListenWithArguments which means
// _eventSink was nil when was called observeValueForKeyPath with updated Failed status
// and error was not sent and never will be so it is needed to send it here to have
// future returned by initialize resolved instead of in state of infinite waiting,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you separate these 4 lines into multiple sentences? I have a hard time understanding this.

// 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 +448,24 @@ - (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
[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.

can you also specify which thread this is on

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 some AVFoundation background queue (this is also mentioned in a comment a few lines above):

https://developer.apple.com/documentation/avfoundation/avasynchronouskeyvalueloading/1387321-loadvaluesasynchronouslyforkeys

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.2.3
Expand Down