Skip to content

[video_player] foundation - reduce seek accuracy to fix seek to end bug #3784

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

Merged
merged 20 commits into from
May 11, 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.6

* Fixes hang when seeking to end of video.

## 2.4.5

* Updates functions without a prototype to avoid deprecation warning.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,13 +71,27 @@ void main() {
expect(await controller.position, greaterThan(Duration.zero));
});

testWidgets('can seek', (WidgetTester tester) async {
await controller.initialize();
testWidgets(
'can seek',
(WidgetTester tester) async {
await controller.initialize();

await controller.seekTo(const Duration(seconds: 3));
await controller.seekTo(const Duration(seconds: 3));

expect(await controller.position, const Duration(seconds: 3));
});
expect(controller.value.position, const Duration(seconds: 3));
},
);

testWidgets(
'can seek to end',
(WidgetTester tester) async {
await controller.initialize();

await controller.seekTo(controller.value.duration);

expect(controller.value.duration, controller.value.position);
},
);

testWidgets('can be paused', (WidgetTester tester) async {
await controller.initialize();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

#import <OCMock/OCMock.h>
#import <video_player_avfoundation/AVAssetTrackUtils.h>
#import <video_player_avfoundation/FLTVideoPlayerPlugin_Test.h>

@interface FLTVideoPlayer : NSObject <FlutterStreamHandler>
@property(readonly, nonatomic) AVPlayer *player;
Expand Down Expand Up @@ -61,6 +62,46 @@ - (CGAffineTransform)preferredTransform {
@interface VideoPlayerTests : XCTestCase
@end

@interface StubAVPlayer : AVPlayer
@property(readonly, nonatomic) NSNumber *beforeTolerance;
@property(readonly, nonatomic) NSNumber *afterTolerance;
@end

@implementation StubAVPlayer

- (void)seekToTime:(CMTime)time
toleranceBefore:(CMTime)toleranceBefore
toleranceAfter:(CMTime)toleranceAfter
completionHandler:(void (^)(BOOL finished))completionHandler {
_beforeTolerance = [NSNumber numberWithLong:toleranceBefore.value];
_afterTolerance = [NSNumber numberWithLong:toleranceAfter.value];
completionHandler(YES);
}

@end

@interface StubFVPPlayerFactory : NSObject <FVPPlayerFactory>

@property(nonatomic, strong) StubAVPlayer *stubAVPlayer;

- (instancetype)initWithPlayer:(StubAVPlayer *)stubAVPlayer;

@end

@implementation StubFVPPlayerFactory

- (instancetype)initWithPlayer:(StubAVPlayer *)stubAVPlayer {
self = [super init];
_stubAVPlayer = stubAVPlayer;
return self;
}

- (AVPlayer *)playerWithPlayerItem:(AVPlayerItem *)playerItem {
return _stubAVPlayer;
}

@end

@implementation VideoPlayerTests

- (void)testBlankVideoBugWithEncryptedVideoStreamAndInvertedAspectRatioBugForSomeVideoStream {
Expand Down Expand Up @@ -226,6 +267,81 @@ - (void)testTransformFix {
[self validateTransformFixForOrientation:UIImageOrientationRightMirrored];
}

- (void)testSeekToleranceWhenNotSeekingToEnd {
NSObject<FlutterPluginRegistry> *registry =
(NSObject<FlutterPluginRegistry> *)[[UIApplication sharedApplication] delegate];
NSObject<FlutterPluginRegistrar> *registrar = [registry registrarForPlugin:@"TestSeekTolerance"];

StubAVPlayer *stubAVPlayer = [[StubAVPlayer alloc] init];
StubFVPPlayerFactory *stubFVPPlayerFactory =
[[StubFVPPlayerFactory alloc] initWithPlayer:stubAVPlayer];
FLTVideoPlayerPlugin *pluginWithMockAVPlayer =
[[FLTVideoPlayerPlugin alloc] initWithPlayerFactory:stubFVPPlayerFactory registrar:registrar];

FlutterError *error;
[pluginWithMockAVPlayer 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 = [pluginWithMockAVPlayer create:create error:&error];
NSNumber *textureId = textureMessage.textureId;

XCTestExpectation *initializedExpectation =
[self expectationWithDescription:@"seekTo has zero tolerance when seeking not to end"];
FLTPositionMessage *message = [FLTPositionMessage makeWithTextureId:textureId position:@1234];
[pluginWithMockAVPlayer seekTo:message
completion:^(FlutterError *_Nullable error) {
[initializedExpectation fulfill];
}];

[self waitForExpectationsWithTimeout:30.0 handler:nil];
XCTAssertEqual([stubAVPlayer.beforeTolerance intValue], 0);
XCTAssertEqual([stubAVPlayer.afterTolerance intValue], 0);
}

- (void)testSeekToleranceWhenSeekingToEnd {
NSObject<FlutterPluginRegistry> *registry =
(NSObject<FlutterPluginRegistry> *)[[UIApplication sharedApplication] delegate];
NSObject<FlutterPluginRegistrar> *registrar =
[registry registrarForPlugin:@"TestSeekToEndTolerance"];

StubAVPlayer *stubAVPlayer = [[StubAVPlayer alloc] init];
StubFVPPlayerFactory *stubFVPPlayerFactory =
[[StubFVPPlayerFactory alloc] initWithPlayer:stubAVPlayer];
FLTVideoPlayerPlugin *pluginWithMockAVPlayer =
[[FLTVideoPlayerPlugin alloc] initWithPlayerFactory:stubFVPPlayerFactory registrar:registrar];

FlutterError *error;
[pluginWithMockAVPlayer 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 = [pluginWithMockAVPlayer create:create error:&error];
NSNumber *textureId = textureMessage.textureId;

XCTestExpectation *initializedExpectation =
[self expectationWithDescription:@"seekTo has non-zero tolerance when seeking to end"];
// The duration of this video is "0" due to the non standard initiliatazion process.
FLTPositionMessage *message = [FLTPositionMessage makeWithTextureId:textureId position:@0];
[pluginWithMockAVPlayer seekTo:message
completion:^(FlutterError *_Nullable error) {
[initializedExpectation fulfill];
}];
[self waitForExpectationsWithTimeout:30.0 handler:nil];
XCTAssertGreaterThan([stubAVPlayer.beforeTolerance intValue], 0);
XCTAssertGreaterThan([stubAVPlayer.afterTolerance intValue], 0);
}

- (NSDictionary<NSString *, id> *)testPlugin:(FLTVideoPlayerPlugin *)videoPlayerPlugin
uri:(NSString *)uri {
FlutterError *error;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// found in the LICENSE file.

#import "FLTVideoPlayerPlugin.h"
#import "FLTVideoPlayerPlugin_Test.h"

#import <AVFoundation/AVFoundation.h>
#import <GLKit/GLKit.h>
Expand Down Expand Up @@ -33,6 +34,16 @@ - (void)onDisplayLink:(CADisplayLink *)link {
}
@end

@interface FVPDefaultPlayerFactory : NSObject <FVPPlayerFactory>
@end

@implementation FVPDefaultPlayerFactory
- (AVPlayer *)playerWithPlayerItem:(AVPlayerItem *)playerItem {
return [AVPlayer playerWithPlayerItem:playerItem];
}

@end

@interface FLTVideoPlayer : NSObject <FlutterTexture, FlutterStreamHandler>
@property(readonly, nonatomic) AVPlayer *player;
@property(readonly, nonatomic) AVPlayerItemVideoOutput *videoOutput;
Expand All @@ -52,7 +63,8 @@ @interface FLTVideoPlayer : NSObject <FlutterTexture, FlutterStreamHandler>
@property(nonatomic, readonly) BOOL isInitialized;
- (instancetype)initWithURL:(NSURL *)url
frameUpdater:(FLTFrameUpdater *)frameUpdater
httpHeaders:(nonnull NSDictionary<NSString *, NSString *> *)headers;
httpHeaders:(nonnull NSDictionary<NSString *, NSString *> *)headers
playerFactory:(id<FVPPlayerFactory>)playerFactory;
@end

static void *timeRangeContext = &timeRangeContext;
Expand All @@ -65,9 +77,14 @@ - (instancetype)initWithURL:(NSURL *)url
static void *rateContext = &rateContext;

@implementation FLTVideoPlayer
- (instancetype)initWithAsset:(NSString *)asset frameUpdater:(FLTFrameUpdater *)frameUpdater {
- (instancetype)initWithAsset:(NSString *)asset
frameUpdater:(FLTFrameUpdater *)frameUpdater
playerFactory:(id<FVPPlayerFactory>)playerFactory {
NSString *path = [[NSBundle mainBundle] pathForResource:asset ofType:nil];
return [self initWithURL:[NSURL fileURLWithPath:path] frameUpdater:frameUpdater httpHeaders:@{}];
return [self initWithURL:[NSURL fileURLWithPath:path]
frameUpdater:frameUpdater
httpHeaders:@{}
playerFactory:playerFactory];
}

- (void)addObserversForItem:(AVPlayerItem *)item player:(AVPlayer *)player {
Expand Down Expand Up @@ -203,18 +220,20 @@ - (void)createVideoOutputAndDisplayLink:(FLTFrameUpdater *)frameUpdater {

- (instancetype)initWithURL:(NSURL *)url
frameUpdater:(FLTFrameUpdater *)frameUpdater
httpHeaders:(nonnull NSDictionary<NSString *, NSString *> *)headers {
httpHeaders:(nonnull NSDictionary<NSString *, NSString *> *)headers
playerFactory:(id<FVPPlayerFactory>)playerFactory {
NSDictionary<NSString *, id> *options = nil;
if ([headers count] != 0) {
options = @{@"AVURLAssetHTTPHeaderFieldsKey" : headers};
}
AVURLAsset *urlAsset = [AVURLAsset URLAssetWithURL:url options:options];
AVPlayerItem *item = [AVPlayerItem playerItemWithAsset:urlAsset];
return [self initWithPlayerItem:item frameUpdater:frameUpdater];
return [self initWithPlayerItem:item frameUpdater:frameUpdater playerFactory:playerFactory];
}

- (instancetype)initWithPlayerItem:(AVPlayerItem *)item
frameUpdater:(FLTFrameUpdater *)frameUpdater {
frameUpdater:(FLTFrameUpdater *)frameUpdater
playerFactory:(id<FVPPlayerFactory>)playerFactory {
self = [super init];
NSAssert(self, @"super init cannot be nil");

Expand Down Expand Up @@ -247,7 +266,7 @@ - (instancetype)initWithPlayerItem:(AVPlayerItem *)item
}
};

_player = [AVPlayer playerWithPlayerItem:item];
_player = [playerFactory playerWithPlayerItem:item];
_player.actionAtItemEnd = AVPlayerActionAtItemEndNone;

// This is to fix 2 bugs: 1. blank video for encrypted video streams on iOS 16
Expand Down Expand Up @@ -420,9 +439,15 @@ - (int64_t)duration {
}

- (void)seekTo:(int)location completionHandler:(void (^)(BOOL))completionHandler {
[_player seekToTime:CMTimeMake(location, 1000)
toleranceBefore:kCMTimeZero
toleranceAfter:kCMTimeZero
CMTime locationCMT = CMTimeMake(location, 1000);
CMTimeValue duration = _player.currentItem.asset.duration.value;
// Without adding tolerance when seeking to duration,
// seekToTime will never complete, and this call will hang.
// see issue https://github.com/flutter/flutter/issues/124475.
CMTime tolerance = location == duration ? CMTimeMake(1, 1000) : kCMTimeZero;
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 write a unit test (XCTest) for this behavior?

Copy link
Contributor

@stuartmorgan-g stuartmorgan-g Apr 25, 2023

Choose a reason for hiding this comment

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

I have no idea what I didn't think of that when @tarrinneal and I were talking about testing for this 🤦🏻

It looks like we don't currently have a way to mock out the underlying AVPlayer, which significantly reduces testability. I would suggest wrapping the current call to [AVPlayer playerWithPlayerItem:] in a factory object, and do DI of that factory. So you'd make a test-only header that declared:

  • A protocol for the AVPlayer instance factory (which it looks like would just be a single method)
  • A new init that takes an instance of the factory protocol.

The, similar to the other DI you did recently, you'd have a private implementation of that protocol that's the default version, just wrapping [AVPlayer playerWithPlayerItem:], but tests could replace it with something vending mock AVPlayer instances where you could validate the tolerance parameter.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can also try stubbing the constructor directly if it's easier:

id mockPlayer = OCMClassMock([AVPlayer class]);
OCMStub([AVPlayer playerWithPlayerItem:OCMOCM_ANY]).andReturn(mockPlayer);

Copy link
Contributor

@stuartmorgan-g stuartmorgan-g Apr 26, 2023

Choose a reason for hiding this comment

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

Please don't. OCMock class mocking is giant foot-gun; it's incredibly easy to accidentally leak mocking across tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

gotcha. i did that a few times for camera plugin iirc. i should clean that up sometime.

Comment on lines +442 to +447
Copy link
Contributor

Choose a reason for hiding this comment

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

What if _player.currentItem.asset.duration.timescale is not 1000? This comparison compares just numerators of fractions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Video https://flutter.github.io/assets-for-api-docs/assets/videos/bee.mp4 from tests gives 2422 and 600 for _player.currentItem.asset.duration value and timescale.

Copy link
Contributor

Choose a reason for hiding this comment

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

@misos1 This is a PR that landed a year and a half ago; comments here aren't actionable. If there's a bug, please file an issue with details.

[_player seekToTime:locationCMT
toleranceBefore:tolerance
toleranceAfter:tolerance
completionHandler:completionHandler];
}

Expand Down Expand Up @@ -523,6 +548,7 @@ @interface FLTVideoPlayerPlugin () <FLTAVFoundationVideoPlayerApi>
@property(readonly, strong, nonatomic)
NSMutableDictionary<NSNumber *, FLTVideoPlayer *> *playersByTextureId;
@property(readonly, strong, nonatomic) NSObject<FlutterPluginRegistrar> *registrar;
@property(nonatomic, strong) id<FVPPlayerFactory> playerFactory;
@end

@implementation FLTVideoPlayerPlugin
Expand All @@ -533,11 +559,17 @@ + (void)registerWithRegistrar:(NSObject<FlutterPluginRegistrar> *)registrar {
}

- (instancetype)initWithRegistrar:(NSObject<FlutterPluginRegistrar> *)registrar {
return [self initWithPlayerFactory:[[FVPDefaultPlayerFactory alloc] init] registrar:registrar];
}

- (instancetype)initWithPlayerFactory:(id<FVPPlayerFactory>)playerFactory
registrar:(NSObject<FlutterPluginRegistrar> *)registrar {
self = [super init];
NSAssert(self, @"super init cannot be nil");
_registry = [registrar textures];
_messenger = [registrar messenger];
_registrar = registrar;
_playerFactory = playerFactory;
_playersByTextureId = [NSMutableDictionary dictionaryWithCapacity:1];
return self;
}
Expand Down Expand Up @@ -588,12 +620,15 @@ - (FLTTextureMessage *)create:(FLTCreateMessage *)input error:(FlutterError **)e
} else {
assetPath = [_registrar lookupKeyForAsset:input.asset];
}
player = [[FLTVideoPlayer alloc] initWithAsset:assetPath frameUpdater:frameUpdater];
player = [[FLTVideoPlayer alloc] initWithAsset:assetPath
frameUpdater:frameUpdater
playerFactory:_playerFactory];
return [self onPlayerSetup:player frameUpdater:frameUpdater];
} else if (input.uri) {
player = [[FLTVideoPlayer alloc] initWithURL:[NSURL URLWithString:input.uri]
frameUpdater:frameUpdater
httpHeaders:input.httpHeaders];
httpHeaders:input.httpHeaders
playerFactory:_playerFactory];
return [self onPlayerSetup:player frameUpdater:frameUpdater];
} else {
*error = [FlutterError errorWithCode:@"video_player" message:@"not implemented" details:nil];
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// Copyright 2013 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#import "FLTVideoPlayerPlugin.h"

#import <AVFoundation/AVFoundation.h>

// Protocol for an AVPlayer instance factory. Used for injecting players in tests.
@protocol FVPPlayerFactory
- (AVPlayer *)playerWithPlayerItem:(AVPlayerItem *)playerItem;
@end

@interface FLTVideoPlayerPlugin ()

- (instancetype)initWithPlayerFactory:(id<FVPPlayerFactory>)playerFactory
registrar:(NSObject<FlutterPluginRegistrar> *)registrar;
@end
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.5
version: 2.4.6

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