fix: avoid memory leak on iOS

[vladvlasov256]

Summary

The PR fixes memory leaks on iOS.

The RCTVideo and ReactNativeVideoManager create a reference cycle because ReactNativeVideoManager retains the views but never release them.

Motivation

Reducing the memory usage in the apps.

Changes

Removes redundant instanceList in ReactNativeVideoManager but keep the counter logic. React Native retain the view references so no need to have an extra list.

Test plan

1. In the basic app, push and pop the video screen 8 times.
2. Check RCTVideo instances in the memory graph.

Expected result: only a single RCTVideo instance is presented.

⚠️ In the sample app, the CLANG_ADDRESS_SANITIZER is enabled in debug. So Xcode doesn't show a proper object list. I had to remove Yes value for test purposes.

[vladvlasov256]

There might be a bug in the instance counter logic on both platforms. Please take a look at the Android code:

 fun registerView(newInstance: ReactExoplayerViewManager) {
    if (instanceList.size > 2) {
        DebugLog.d(TAG, "multiple Video displayed ?")
    }
    instanceList.add(newInstance)
}

As I understand, a user gets a debug output only after adding the 4th video to the screen:

Added the 1st video. instanceList.size = 0, no warning.
Added the 2nd video. instanceList.size = 1, no warning.
Added the 3rd video. instanceList.size = 2, no warning.
Added the 4th video. instanceList.size = 3, the warning is displayed.

Should the condition be instanceList.size > 0 instead? So then the user gets the warning after adding the 2nd video.

@freeboub what do you think?

[freeboub]

Why is there 2 init functions 🤔
BTW looks good to me

[KrzysztofMoch]

That one with coder is for compatibility with older versions of swift and storyboard - this init in general should be never called

[vladvlasov256]

Removed this call. I added it just for init/deinit symmetry.

[freeboub]

There might be a bug in the instance counter logic on both platforms. Please take a look at the Android code:

 fun registerView(newInstance: ReactExoplayerViewManager) {
    if (instanceList.size > 2) {
        DebugLog.d(TAG, "multiple Video displayed ?")
    }
    instanceList.add(newInstance)
}

As I understand, a user gets a debug output only after adding the 4th video to the screen:

Added the 1st video. instanceList.size = 0, no warning. Added the 2nd video. instanceList.size = 1, no warning. Added the 3rd video. instanceList.size = 2, no warning. Added the 4th video. instanceList.size = 3, the warning is displayed.

Should the condition be instanceList.size > 0 instead? So then the user gets the warning after adding the 2nd video.

@freeboub what do you think?

Yes I may do better. I put this, as is, because, from my experience, 2 parallel video decoding on a device is working pretty fine in most of case... but It is not absolutely true. In another fork, I implemented a limitation to ensure we don't use more than 'X' Video in the same time by droping previous instance (for safety).

[vladvlasov256]

Yes I may do better. I put this, as is, because, from my experience, 2 parallel video decoding on a device is working pretty fine in most of case... but It is not absolutely true

@freeboub, in this PR, the maximum value is changed from 10 to 2 to align it with Android. Does this make sense, or should I revert it?

[freeboub]

Yes I may do better. I put this, as is, because, from my experience, 2 parallel video decoding on a device is working pretty fine in most of case... but It is not absolutely true

@freeboub, in this PR, the maximum value is changed from 10 to 2 to align it with Android. Does this make sense, or should I revert it?

No that's good, ios is really more robust at this point (but it's not a reason) :)

[KrzysztofMoch]

It's fine for me

[KrzysztofMoch]

Is this breaking change? If it is let's add "!" to commit -> fix!: avoid memory leak on iOS

[vladvlasov256]

No, it doesn't break the existing API. But the argument is unused so _ makes the linter happy.