Skip to content

Commit

Permalink
[video_player] Improve macOS frame management (#5078)
Browse files Browse the repository at this point in the history
Fixes some logic around getting new frames on macOS:
- The logic to copy a frame had a race condition; the macOS code checks that a frame is available for the current playback time before informing the engine that a frame is available (it's not clear why the iOS code doesn't do this; apparently the engine tolerates returning NULL frames on iOS?), but failed to account for the fact that the current playback time when the (async) request from the engine comes in can be different. This fixes it to remember the last time that was known to be available, and uses that when the current time isn't available. This fixes flickering on playback (since returning NULL on macOS causes the video to vanish until a new frame is available).
- Fixes seek to temporarily restart the display link if the video is paused, rather than telling the engine that a frame is available, because it might not be. This is changed for both macOS and iOS since I don't see any reason this bug couldn't affect iOS as well (although in practice I'm only aware of it being reproducible on macOS).

This extracts the display link code for macOS and iOS into an abstraction, eliminating most of the ifdefing, in order to support the latter (since more code needs to be able to play/pause the display link), which also resolves a TODO from the initial implementation.

There is also some non-trivial addition of factory injection in order to make the code more testable. This is definitely not complete, but it incrementally moves the code toward being more testable than it was before, and allows for testing the display link behavior.

Lastly, this moves some code used by tests to the existing `_Test.h` header, removing redeclarations from unit test files, since we already have a test header and that's our preferred approach for accessing private details in ObjC tests. (Longer term the multi-class mega-file should be broken up more to reduce the need for that.)

Fixes flutter/flutter#136027
Improves flutter/flutter#135999
  • Loading branch information
stuartmorgan authored Nov 16, 2023
1 parent 52a5342 commit 25574f9
Show file tree
Hide file tree
Showing 11 changed files with 546 additions and 165 deletions.
4 changes: 4 additions & 0 deletions packages/video_player/video_player_avfoundation/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 2.5.2

* Fixes flickering and seek-while-paused on macOS.

## 2.5.1

* Updates to Pigeon 13.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
// 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 <Foundation/Foundation.h>

#if TARGET_OS_OSX
#import <FlutterMacOS/FlutterMacOS.h>
#else
#import <Flutter/Flutter.h>
#endif

// A cross-platform display link abstraction.
@interface FVPDisplayLink : NSObject

/**
* Whether the display link is currently running (i.e., firing events).
*
* Defaults to NO.
*/
@property(nonatomic, assign) BOOL running;

/**
* Initializes a display link that calls the given callback when fired.
*
* The display link starts paused, so must be started, by setting 'running' to YES, before the
* callback will fire.
*/
- (instancetype)initWithRegistrar:(id<FlutterPluginRegistrar>)registrar
callback:(void (^)(void))callback NS_DESIGNATED_INITIALIZER;

- (instancetype)init NS_UNAVAILABLE;

@end
Loading

0 comments on commit 25574f9

Please sign in to comment.