-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Changes from 4 commits
16d3d7a
12a061e
902c3e6
4ab3c53
1e0964f
89db486
1a6fd6f
60330dc
01b21ac
ab57c33
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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]; | ||
break; | ||
case AVPlayerItemStatusUnknown: | ||
break; | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i dont think we need this comment There was a problem hiding this comment. Choose a reason for hiding this commentThe 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]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we move this to the caller, before There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. or can you update the function name to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is also 4th place where this function is called masked by There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually that function does not set up There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On the other hand if status becomes |
||
return; | ||
} | ||
CGSize size = currentItem.presentationSize; | ||
CGFloat width = size.width; | ||
CGFloat height = size.height; | ||
|
@@ -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]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this code the key change that fixed the bug? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems no, I think it is that change in |
||
}); | ||
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]; | ||
|
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.
nit:
sendFailedToLoadVideoEventIfNeeded
since you moved the check insideThere 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.
I would not say "if needed" sounds right here. Meaning of that check is rather like "if can" or "if possible".