Pause video on end reached, don't remove listeners

[sharnik]

Problem

That issues exists on iOS only, on Android it behaves us expected.

When you play a video all the way till the end, onEndReached we'll remove the time observers that update the onProgress handlers. But, if the user just seeks back to the beginning of the video (or anywhere on the timeline), the video will keep playing (as it was never stopped), but no onProgress callbacks/events will be triggered.

Solution

On reaching the end of the video (unless set to repeat) instead of removing the time observers we just pause it. This will stop the onProgress events firing after the video finished. If the user chooses to seek back on the timeline to replay the video, they'd have to press "play" and the video will start normally and the onProgress events will fire correctly.

Test plan

This can be checked by running a simple player on iOS that is:

• not on repeat
• using built in controls
• with an onProgress callback (for example: onProgress={() => { console.log('test') }})

And then watching (or seeking) all the way till the end and then (without pausing or resuming it) seeking back to the beginning.

[seyedmostafahasani]

I think we should avoid removing the time observer to keep the player in memory and prevent reinitialization, ensuring smoother playback and better performance.

Suggested change

	_player?.rate = 0.0
	_player?.pause()
	_player?.rate = 0.0
	_playerObserver.removePlayerTimeObserver()

[KrzysztofMoch]

I think that this change is good! But we need to move _playerObserver.removePlayerTimeObserver() to other place as this was the only place where it was called. We probably should move it do deinit - Can you handle it @sharnik ?

[freeboub]

On my side I think you can just remove the observer, why do you force pause ?
Addtionnaly, if you still receive the progress event, The best fix would be to ensure in sendProgressUpdate that we never send the same event (with same data) with multiple times.

[sharnik]

@freeboub Removing the observer is the problem here - if I do that and user decides to replay the video, no progress events are triggered, even though the video is playing.

@KrzysztofMoch I'll add the removePlayerTimeObserver to deinit and update the PR 👍

[freeboub]

@sharnik I agree, you should remove the observer !
I wonder why you pause the playback. I suspect this is an (incorrect) workaround for another issue.
I patch a similar issue recently to keep behavior coherent between android and ios related to this end playback use case, I didn't catch the missing progress event.
Thank you

[sharnik]

If I don't pause, the progress events keep coming in, even though the video is at its end and nothing's playing.

[freeboub]

@sharnik OK, That's clear! I don't think that this pause is the good way to handle this side effect.
I am thinking for use case like: user reach the end, pause stream and resume it -> we can be in a messy state
I would prefer filtering the events in sendProgressUpdate
BTW, I tested the changes I don't see side effects ... so let's accept !
and @KrzysztofMoch There are already a lot of security about removing the event handler, so no real need to remove somewhere else I think

[sharnik]

@KrzysztofMoch I've checked and when player is set to nil we run the observer cleanup in RCTPlayerObserver .

And deinit already calls the clearPlayer that sets it to nil.

This should be enough. No need to add additional manual cleanup, right?

[KrzysztofMoch]

Yeah make sense - Thanks for PR!