Making player optional and cleaning up player instance

[SachinAhujaGit]

Intermittently the player instance doesn't clean up and leads to error loading subsequent videos. Setting the current item to nil in destroy() function of AVPlayerEngine.swift fixes this issue.

The app crashes as IMA plugin is trying to access the removed player when the player instance is removed as the ad is loading. So, the player should be made optional.

[kaltura-fe-ci]

Can one of the admins verify this patch?

[kaltura-hooks]

Hi @SachinAhujaGit,
Thank you for contributing this pull request!
Please sign the Kaltura CLA so we can review and merge your contribution.
Learn more at http://bit.ly/KalturaContrib

[itaykinnrot]

@SachinAhujaGit thank you for that - we're looking into it and get back with a solution/merge shortly

[ghost]

@SachinAhujaGit thanks for the feedback.
I need a few things in order to look into this:

• Could you provide us with a sample that reproduces this issue?
• Could you please provide the exact flow of what you do that causes the issue (how you start the player and how you replace the media).

cc: @itaykinnrot, @ElizaSapir

[newmaniese]

@galorlanczyk I shared access to our BitBucket repo with you. You can find the video components here: https://bitbucket.org/grahamdigital/newsreader.ios/src/9078d18c955d989eba7a46ea9e9da6d30751973a/NewsReader/customComponents/VideoPlayer/VideoPlayerViewController.m?at=master&fileviewer=file-view-default

Please let us know if you need more examples. We also have a public beta that we can share that reproduces this problem.

[ghost]

@newmaniese i receive access denied when trying to enter the link, any other ways I could see an example?

[newmaniese]

I shared access with the username galorlanczyk on bitbucket, is this not you?

[ghost]

@newmaniese I tried to look at the class you sent me, it seems fine, but I can't seem to find the steps of the error you guys mentioned.

• Could you provide me a sample that recreates the problem?
• Please explain me the exact steps to reproduce it would help me greatly.

[newmaniese]

Certainly, I sent a TestFlight build to @itaykinnrot and @ElizaSapir and also attempted a guess at your email, but might have been wrong.

It is intermittent, but the most consistent instance of us running into this issue is to play the video on the homepage, then to open an article with a video (Right now "Why was a woman who delivered..."), swipe right to play another video. That video doesn't play, due to this issue.

You can also feel the app getting sluggish as there are some memory implications here.

The fix here might be a little brute force, as it just marks everything for garbage collection. Another fix would be to clean up references to the other classes that are invoked. I haven't dug in as far as @SachinAhujaGit, but I assume too there are IMA variables that are attached to this class that aren't cleared on destroy, which is probably the actual cause of the issue.

[ElizaSapir]

@newmaniese hey,

The app looks great :)

We tried to reproduce the issue on your app with no success.

If you are talking about leaks, it's a known issue on IMA plugin we already in a middle of improvements.

But as we see video always plays, we got black screen only twice when there was some issue with ad tag url and google server.

So let's make it clear to try and reproduce it on our side:

1. You have 2 player instances when video article is opened? (main page + article page)
2. To reproduce the issue we have to destroy the player and recreate (we gave it 12 tries with no success)
3. Just to clarify the issue is that you can't play OR leaks?

Thanks!

cc @galorlanczyk , @itaykinnrot , @OrenMe

[newmaniese]

@ElizaSapir, we have both issues, this proposed fix clears both. To reproduce, just click through a few videos in the app, you will feel the app bog down and eventually see the player become blank. If it were only an IMA issue, you should sometimes see this happen on the first or second instance of the video player. Instead it is always deeper within the application.

The black screen eventually crashes the entire player (we catch and reset it).

The memory leak is certainly with the IMA plugin, disabling it, we cannot reproduce. I am attempting to pull some allocation logs so you can see each instance of the video player holds onto a bunch of memory.

Replacing the class to nil allows all of this memory to be cleaned and fixes both issues. Like I said, this is brute force, but effective.

[ghost]

@newmaniese, @SachinAhujaGit Hey,
Just wanted to keep you posted on the issue.

After deep examination of the issue I have found a bug with Apple's interoperability in Swift which causes a memory leak.
I already opened a bug report at Apple's bug tracker and Swift Jira (report #30478887 at apple bug tracker and in Swift Jira SR-3935).

The issue is caused when initializing a Swift object from a protocol.Type with the @objc attribute (happens both in swift and objc projects using swift), this causes the object to automatically receive an extra strong reference and therefore it never gets deallocated.
This caused the IMAPlugin to never get deallocated.

I already implemented a solution and will update you when the new version will be available.

cc: @ElizaSapir, @itaykinnrot, @OrenMe

[ghost]

@newmaniese, @SachinAhujaGit Hey,

We released a new version with the fix inside + made some changes with IMA.
Please make sure to pod update and make the changes stated in the release notes.

For the fix commit: 49f150c

Thank you for your cooperation.