Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

[video_player] [iOS] Fixed a memory leak in the video player plugin. #1660

Merged
cyanglaz merged 13 commits into from
Jul 29, 2019
Merged

[video_player] [iOS] Fixed a memory leak in the video player plugin. #1660

cyanglaz merged 13 commits into from
Jul 29, 2019

Conversation

@shihchanghsiungsonos
Copy link
Contributor

@shihchanghsiungsonos shihchanghsiungsonos commented May 30, 2019

Description

This fixes a memory leak in iOS.

Related Issues

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the Flutter Style Guide.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
  • I updated CHANGELOG.md to add a description of the change.
  • I signed the CLA.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  • No, this is not a breaking change.

cyanglaz
cyanglaz suggested changes Jul 9, 2019
View reviewed changes
Copy link
Contributor

@cyanglaz cyanglaz left a comment

Choose a reason for hiding this comment

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

Good catch! I have left some comments. Also, could you please update the pubspec and CHANGELOG.md with a new version?

packages/video_player/ios/Classes/VideoPlayerPlugin.m Outdated
context:playbackBufferFullContext];

[[NSNotificationCenter defaultCenter] addObserverForName:AVPlayerItemDidPlayToEndTimeNotification
_notificationObserverId = [[NSNotificationCenter defaultCenter] addObserverForName:AVPlayerItemDidPlayToEndTimeNotification
Copy link
Contributor

@cyanglaz cyanglaz Jul 9, 2019

Choose a reason for hiding this comment

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

It might be cleaner to just use the addObserver:self here since we are just using mainQueue. This way we can separate the observing implementation out of the addObservers method.

packages/video_player/ios/Classes/VideoPlayerPlugin.m Outdated
context:playbackBufferFullContext];
[_player replaceCurrentItemWithPlayerItem:nil];
[[NSNotificationCenter defaultCenter] removeObserver:self];
[[NSNotificationCenter defaultCenter] removeObserver:_notificationObserverId];
Copy link
Contributor

@cyanglaz cyanglaz Jul 9, 2019

Choose a reason for hiding this comment

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

This can be unchanged if we use the addObserver: method as I mentioned in the other comment.

@cyanglaz cyanglaz self-assigned this Jul 9, 2019
@cyanglaz cyanglaz added the submit queue The Flutter team is in the process of landing this PR. label Jul 9, 2019
cyanglaz
cyanglaz reviewed Jul 9, 2019
View reviewed changes
packages/video_player/ios/Classes/VideoPlayerPlugin.m Outdated
AVPlayerItem* p = [note object];
[p seekToTime:kCMTimeZero
completionHandler:nil];
[p seekToTime:kCMTimeZero];
Copy link
Contributor

@cyanglaz cyanglaz Jul 9, 2019

Choose a reason for hiding this comment

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

It seems not related.

@shihchanghsiungsonos
Copy link
Contributor Author

shihchanghsiungsonos commented Jul 10, 2019

@cyanglaz Thanks for the feedback. We will update the PR.

cyanglaz
cyanglaz approved these changes Jul 16, 2019
View reviewed changes
Copy link
Contributor

@cyanglaz cyanglaz left a comment

Choose a reason for hiding this comment

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

Thanks for the quick turn around. LGTM overall.

packages/video_player/ios/Classes/VideoPlayerPlugin.m Outdated
object:item];
}

- (void)itemDidPlayToEndTime:(NSNotification*)note {
Copy link
Contributor

@cyanglaz cyanglaz Jul 16, 2019

Choose a reason for hiding this comment

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

nits: renaming note -> notification

Copy link
Contributor Author

@shihchanghsiungsonos shihchanghsiungsonos Jul 16, 2019

Choose a reason for hiding this comment

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

Sure.

packages/video_player/ios/Classes/VideoPlayerPlugin.m Outdated

- (void)itemDidPlayToEndTime:(NSNotification*)note {
AVPlayerItem* p = [note object];
if (self->_isLooping) {
Copy link
Contributor

@cyanglaz cyanglaz Jul 16, 2019

Choose a reason for hiding this comment

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

nits: Same as below for _eventSink

Suggested change
if (self->_isLooping) {
if (_isLooping) {

Copy link
Contributor Author

@shihchanghsiungsonos shihchanghsiungsonos Jul 16, 2019

Choose a reason for hiding this comment

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

Will update. Thanks for the feedback!

cyanglaz
cyanglaz reviewed Jul 16, 2019
View reviewed changes
packages/video_player/CHANGELOG.md Outdated
@@ -1,3 +1,7 @@
## 0.10.1+5

* [iOS] Fixed a memory leak in the video player plugin.
Copy link
Contributor

@cyanglaz cyanglaz Jul 16, 2019

Choose a reason for hiding this comment

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

Suggested change
* [iOS] Fixed a memory leak in the video player plugin.
* [iOS] Fixed a memory leak with notification observing.

@cyanglaz cyanglaz merged commit 5814e2a into flutter:master Jul 29, 2019
mithun-mondal pushed a commit to bKash-developer/archived_plugins that referenced this pull request Aug 6, 2019
@shihchanghsiungsonos @mithun-mondal
[video_player] [iOS] Fixed a memory leak in the video player plugin. (f…
6a31b63 
…lutter#1660)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@iskakaushik iskakaushik Awaiting requested review from iskakaushik

1 more reviewer

@cyanglaz cyanglaz cyanglaz approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@cyanglaz cyanglaz

Labels

cla: yes submit queue The Flutter team is in the process of landing this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@shihchanghsiungsonos @cyanglaz @googlebot