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] fix: add missing isPlaybackLikelyToKeepUp check. #3826

Merged
merged 12 commits into from
Jul 17, 2023
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,7 @@
## 2.4.8

* Fixes missing `isPlaybackLikelyToKeepUp` check for iOS video player `bufferingEnd` event and `bufferingStart` event.

## 2.4.7

* Updates minimum supported SDK version to Flutter 3.3/Dart 2.18.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,52 @@ - (void)testDeregistersFromPlayer {
[self waitForExpectationsWithTimeout:30.0 handler:nil];
}

- (void)testBufferingStateFromPlayer {
NSObject<FlutterPluginRegistry> *registry =
(NSObject<FlutterPluginRegistry> *)[[UIApplication sharedApplication] delegate];
NSObject<FlutterPluginRegistrar> *registrar =
[registry registrarForPlugin:@"testLiveStreamBufferEndFromPlayer"];
FLTVideoPlayerPlugin *videoPlayerPlugin =
(FLTVideoPlayerPlugin *)[[FLTVideoPlayerPlugin alloc] initWithRegistrar:registrar];

FlutterError *error;
[videoPlayerPlugin initialize:&error];
XCTAssertNil(error);

FLTCreateMessage *create = [FLTCreateMessage
makeWithAsset:nil
uri:@"https://flutter.github.io/assets-for-api-docs/assets/videos/bee.mp4"
packageName:nil
formatHint:nil
httpHeaders:@{}];
FLTTextureMessage *textureMessage = [videoPlayerPlugin create:create error:&error];
XCTAssertNil(error);
XCTAssertNotNil(textureMessage);
FLTVideoPlayer *player = videoPlayerPlugin.playersByTextureId[textureMessage.textureId];
XCTAssertNotNil(player);
AVPlayer *avPlayer = player.player;
[avPlayer play];

[player onListenWithArguments:nil
eventSink:^(NSDictionary<NSString *, id> *event) {
if ([event[@"event"] isEqualToString:@"bufferingEnd"]) {
XCTAssertTrue(avPlayer.currentItem.isPlaybackLikelyToKeepUp);
}

if ([event[@"event"] isEqualToString:@"bufferingStart"]) {
XCTAssertFalse(avPlayer.currentItem.isPlaybackLikelyToKeepUp);
}
}];
XCTestExpectation *bufferingStateExpectation =
[self expectationWithDescription:@"bufferingState"];
NSTimeInterval timeout = 10;
dispatch_time_t delay = dispatch_time(DISPATCH_TIME_NOW, timeout * NSEC_PER_SEC);
dispatch_after(delay, dispatch_get_main_queue(), ^{
[bufferingStateExpectation fulfill];
});
[self waitForExpectationsWithTimeout:timeout + 1 handler:nil];
}

- (void)testVideoControls {
NSObject<FlutterPluginRegistry> *registry =
(NSObject<FlutterPluginRegistry> *)[[UIApplication sharedApplication] delegate];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,6 @@ - (instancetype)initWithURL:(NSURL *)url
static void *presentationSizeContext = &presentationSizeContext;
static void *durationContext = &durationContext;
static void *playbackLikelyToKeepUpContext = &playbackLikelyToKeepUpContext;
static void *playbackBufferEmptyContext = &playbackBufferEmptyContext;
static void *playbackBufferFullContext = &playbackBufferFullContext;
static void *rateContext = &rateContext;

@implementation FLTVideoPlayer
Expand Down Expand Up @@ -108,14 +106,6 @@ - (void)addObserversForItem:(AVPlayerItem *)item player:(AVPlayer *)player {
forKeyPath:@"playbackLikelyToKeepUp"
options:NSKeyValueObservingOptionInitial | NSKeyValueObservingOptionNew
context:playbackLikelyToKeepUpContext];
[item addObserver:self
forKeyPath:@"playbackBufferEmpty"
options:NSKeyValueObservingOptionInitial | NSKeyValueObservingOptionNew
context:playbackBufferEmptyContext];
[item addObserver:self
forKeyPath:@"playbackBufferFull"
options:NSKeyValueObservingOptionInitial | NSKeyValueObservingOptionNew
context:playbackBufferFullContext];

// Add observer to AVPlayer instead of AVPlayerItem since the AVPlayerItem does not have a "rate"
// property
Expand Down Expand Up @@ -330,19 +320,15 @@ - (void)observeValueForKeyPath:(NSString *)path
[self updatePlayingState];
}
} else if (context == playbackLikelyToKeepUpContext) {
[self updatePlayingState];
if ([[_player currentItem] isPlaybackLikelyToKeepUp]) {
[self updatePlayingState];
if (_eventSink != nil) {
_eventSink(@{@"event" : @"bufferingEnd"});
}
}
} else if (context == playbackBufferEmptyContext) {
if (_eventSink != nil) {
_eventSink(@{@"event" : @"bufferingStart"});
}
} else if (context == playbackBufferFullContext) {
if (_eventSink != nil) {
_eventSink(@{@"event" : @"bufferingEnd"});
} else {
if (_eventSink != nil) {
_eventSink(@{@"event" : @"bufferingStart"});
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 explain in the comment why we need to check isPlaybackLikelyToKeepUp here? (rather than the existing code?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hellohuanlin Thanks for the suggestion. Already updated.

}
}
} else if (context == rateContext) {
// Important: Make sure to cast the object to AVPlayer when observing the rate property,
Expand Down Expand Up @@ -528,8 +514,6 @@ - (void)disposeSansEventChannel {
[currentItem removeObserver:self forKeyPath:@"presentationSize"];
[currentItem removeObserver:self forKeyPath:@"duration"];
[currentItem removeObserver:self forKeyPath:@"playbackLikelyToKeepUp"];
[currentItem removeObserver:self forKeyPath:@"playbackBufferEmpty"];
[currentItem removeObserver:self forKeyPath:@"playbackBufferFull"];

[self.player replaceCurrentItemWithPlayerItem:nil];
[[NSNotificationCenter defaultCenter] removeObserver:self];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ name: video_player_avfoundation
description: iOS 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.4.7
version: 2.4.8

environment:
sdk: ">=2.18.0 <4.0.0"
Expand Down