This repository has been archived by the owner on Feb 22, 2023. It is now read-only.
[video_player] Remove KVO observer on AVPlayerItem on iOS #4683
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jmagman
commented
Jan 20, 2022
Comment on lines
-15
to
-18
| - (void)testPlugin { | ||
| FLTVideoPlayerPlugin *plugin = [[FLTVideoPlayerPlugin alloc] init]; | ||
| XCTAssertNotNil(plugin); | ||
| } |
There was a problem hiding this comment.
This smoke test is no longer needed, the other tests also validate plugin instantiation.
Comment on lines
+40
to
+43
| @property(nonatomic, readonly) BOOL disposed; | ||
| @property(nonatomic, readonly) BOOL isPlaying; | ||
| @property(nonatomic) BOOL isLooping; | ||
| @property(nonatomic, readonly) BOOL isInitialized; |
Comment on lines
-47
to
-50
| - (void)play; | ||
| - (void)pause; | ||
| - (void)setIsLooping:(bool)isLooping; | ||
| - (void)updatePlayingState; |
There was a problem hiding this comment.
No need to declare methods in the interface when the methods are only used within the class.
Comment on lines
-220
to
-222
| _isInitialized = false; | ||
| _isPlaying = false; | ||
| _disposed = false; |
There was a problem hiding this comment.
No need to set ivars to false in init. Should be NO anyway.
Comment on lines
+458
to
+459
| [currentItem removeObserver:self forKeyPath:@"presentationSize"]; | ||
| [currentItem removeObserver:self forKeyPath:@"duration"]; |
There was a problem hiding this comment.
These two lines are the actual fix.
| @@ -486,7 +475,8 @@ - (void)dispose { | |||
| @interface FLTVideoPlayerPlugin () <FLTVideoPlayerApi> | |||
| @property(readonly, weak, nonatomic) NSObject<FlutterTextureRegistry>* registry; | |||
| @property(readonly, weak, nonatomic) NSObject<FlutterBinaryMessenger>* messenger; | |||
| @property(readonly, strong, nonatomic) NSMutableDictionary* players; | |||
| @property(readonly, strong, nonatomic) | |||
| NSMutableDictionary<NSNumber*, FLTVideoPlayer*>* playersByTextureId; | |||
There was a problem hiding this comment.
Lightweight generic, rename to indicate the key is the textureId.
Comment on lines
+529
to
+531
| [self.playersByTextureId | ||
| enumerateKeysAndObjectsUsingBlock:^(NSNumber* textureId, FLTVideoPlayer* player, BOOL* stop) { | ||
| [self.registry unregisterTexture:textureId.unsignedIntegerValue]; |
There was a problem hiding this comment.
Use enumerateKeysAndObjectsUsingBlock instead of looking up the value inside the key loop.
| [player seekTo:[input.position intValue]]; | ||
| [_registry textureFrameAvailable:input.textureId.intValue]; | ||
| FLTVideoPlayer* player = self.playersByTextureId[input.textureId]; | ||
| [player seekTo:input.position.intValue]; |
There was a problem hiding this comment.
I didn't scrub this file for dot notation, just updated a few spots where it's inconsistent within the same line.
| // Input range [-pi, pi] or [-180, 180] | ||
| CGFloat degrees = GLKMathRadiansToDegrees((float)radians); | ||
| if (degrees < 0) { | ||
| // Convert -90 to 270 and -180 to 180 | ||
| return degrees + 360; | ||
| } | ||
| // Output degrees in between [0, 360[ | ||
| // Output degrees in between [0, 360] |
jmagman
force-pushed
the
video-player-kvo
branch
from
bb43277 to
fd76289
Compare
cyanglaz
approved these changes
Jan 20, 2022
jmagman
added
the
waiting for tree to go green
| @@ -1,3 +1,7 @@ | |||
| ## 2.2.14 | |||
|
|
|||
| * Remove KVO observer on AVPlayerItem on iOS. | |||
There was a problem hiding this comment.
Nit: Removes
(Per the new CHANGELOG style guide for the repo)
jmagman
commented
Jan 21, 2022
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Jan 21, 2022
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In #4438 the
AVPlayerItempresentationSizeanddurationproperties are KVO observed, but the observation was not removed when the texture is disposed. Remove the observer.Also clean up a few things in the plugin, like using
BOOLinstead ofbool, lightweight generics, renameplayersdictionary to the more descriptiveplayersByTextureId, preferNS_INLINEtostatic inline.Fixes flutter/flutter#96849
Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.