Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

iOS fixes #3167

Merged
merged 6 commits into from
Jul 10, 2023
Merged

iOS fixes #3167

merged 6 commits into from
Jul 10, 2023

Conversation

cguino
Copy link
Contributor

@cguino cguino commented Jul 6, 2023

#3166 Memory leaks
#3085 onFullscreen call backs are never fired
#3040 [Bug] Fullscreen broken on iOS due to #3017
#2744 Unblock the main thread while replacing the source (cf comment)

@freeboub
Copy link
Collaborator

freeboub commented Jul 8, 2023

thank for this PR, it looks good except #2744 Unblock the main thread while replacing the source (#2744 (comment))

We already experiment this change, but it causes crash when fast changing source...
can you revert this change and I will merge this soon.

The good fix has be be discussed (I think I know the good fix but this is a big architecture change ...)

cguino added 2 commits July 9, 2023 20:44
…ios-fix

* commit '23a39e8f5fffbbdcfc72a4c0d7777763e273cd6d':
  chore: fix build issue
  Update issue templates
  feat: RN 0.73 support
  Fix ids in exo_player_control_view
  fix: remove dummy nativeOnly
@cguino
Copy link
Contributor Author

cguino commented Jul 9, 2023

Thank you, I revert #2744

@marcel-happyfloat
Copy link
Contributor

@freeboub the main thread blocking makes this library unusable for us in the real world with bad internet connection trying to scroll a feed of video items. Do you think the fix is safe to use for multiple video elements, with only one fixed source at a time? (or mounting/unmounting with new source). Thank you.
I really appreciate all your work, unfortunately I can't help debugging on the native side.

@Trashpants
Copy link

Also in the same boat with the blocking main thread, without knowing too much about the internals can we have this as an optional so additional prop such as dangerouslySwapIosVideoSource as a bool on the player component?

@freeboub
Copy link
Collaborator

I totally understand the issue of startup I revert following change: 1f27ffb
which was causing the crash. I really think it will cause the same issue.
This code also causes an issue when rapidly changing source...
There is clearly a concurrency issue in react native video.
I fixed it on android in my company fork but it is a nightmare to backport, I need to find time to reimplement it here.
moreover the PR of new arch will also conflict with it :/
Let see.
Notice that I advise you to patch the package (with patch-package) or create your own fork if you really want this change.
Personally I prefer latency than crash ...

@freeboub freeboub merged commit 9914faf into TheWidlarzGroup:master Jul 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants