Skip to content

Main thread isolation in VideoView #659

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

Merged
merged 4 commits into from
Apr 4, 2025
Merged

Main thread isolation in VideoView #659

merged 4 commits into from
Apr 4, 2025

Conversation

pblazej
Copy link
Contributor

@pblazej pblazej commented Apr 4, 2025

This is a tiny change, but prevents a crash in .v6 mode by fixing an important inconsistency.

  • OnDidMutate is called within the critical section - so from any thread
  • for Sendable classes, it doesn't make much difference though
  • for @MainActor methods like mainSyncOrAsync it does, as they:
    • were still @MainActor isolated
    • were called from bg threads - which is checked at runtime now

So the idea is mainSyncOrAsync signature now reflects what really happens here:

  • it must be nonisolated itself as it's called from a @Sendable OnDidMutate 💚
  • the body is compile-time @MainActor 💚
  • the "switch" happens inside the method, not "accidentally outside"

The crash will happen during video track publication, the stack trace is pretty clear:

Screenshot 2025-04-04 at 11 18 53 AM

@pblazej pblazej requested a review from hiroshihorie April 4, 2025 09:28
Copy link

ilo-nanpa bot commented Apr 4, 2025

it seems like you haven't added any nanpa changeset files to this PR.

if this pull request includes changes to code, make sure to add a changeset, by writing a file to .nanpa/<unique-name>.kdl:

minor type="added" "Introduce frobnication algorithm"

refer to the manpage for more information.

@pblazej pblazej requested a review from ladvoc April 4, 2025 09:31
@@ -285,7 +285,7 @@ public class VideoView: NativeView, Loggable {

// Enter .main only if UI updates are required
if trackDidUpdate || shouldRenderDidUpdate || renderModeDidUpdate {
self.mainSyncOrAsync {
self.mainSyncOrAsync { @MainActor in
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This MainActor is necessary only for <5.9 due to lack of assumeIsolate as you see below 🫳

Copy link
Member

@hiroshihorie hiroshihorie left a comment

Choose a reason for hiding this comment

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

I couldn't repo the crash with main, how can I repo the crash ?

@pblazej
Copy link
Contributor Author

pblazej commented Apr 4, 2025

I couldn't repo the crash with main, how can I repo the crash ?

I used Xcode 16.2 with Package 6.0 - every video track publish will fail 💣

@pblazej
Copy link
Contributor Author

pblazej commented Apr 4, 2025

@hiroshihorie still unable to pull that one spec.dependency("LiveKitWebRTC", "= 125.6422.28")

@hiroshihorie
Copy link
Member

@hiroshihorie still unable to pull that one spec.dependency("LiveKitWebRTC", "= 125.6422.28")

I haven't pushed that pod lib yet

@pblazej pblazej merged commit cfea735 into main Apr 4, 2025
17 of 20 checks passed
@pblazej pblazej deleted the blaze/video-view-crash branch April 4, 2025 12:49
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.

2 participants