Skip to content

fix: Let the Frame Output thread exit once nothing uses it - #4227

Open
chocky335 wants to merge 1 commit into
margelo:mainfrom
chocky335:android/frame-output-executor-leak
Open

chocky335 wants to merge 1 commit into
margelo:mainfrom
chocky335:android/frame-output-executor-leak

Conversation

@chocky335

@chocky335 chocky335 commented Oct 19, 2026 •

Copy link
Copy Markdown

What

On Android, each frame output keeps its com.margelo.camera.frame thread after the camera closes, so every mount leaks one thread.

How

IdentifiableExecutor now lets that thread exit after 5 s while no frame callback is set, and shuts it down once nothing references the executor. useFrameOutput removes its callbacks on unmount, hide and option changes, and never disposes the output, because React can hand the same output to effects again.

Review focus: after 5 s without a frame callback, the Android frame output can run on a new thread (the NativeThread docs allow this), and 5 s is a tuning choice.

On iOS, setOnFrameDroppedCallback now sets the callback on the output's queue, where didDrop reads it. The #4212 test stays red on iOS until HybridNativeThread.swift rejects work after dispose(). Callback removal for useDepthOutput is a follow-up.

Closes #4213

Related Pull Requests

@chocky335

chocky335 commented Oct 19, 2026 •

Copy link
Copy Markdown
Author

These results are for the first version of this PR (a8ddb69), which disposed the output in the hook. The next comment has the results for the current head.

Thread count on a Pixel 8 Pro, from adb shell ps -T -p <pid> sampled about every 2 s, over 10 mount and unmount cycles of <Camera outputs={[useFrameOutput(...)]}> and then a 25 s hold.

Build During the cycles After 10 cycles
Before rises from 1 to 10 10 (16 samples)
After 0 or 1 0 (17 samples)

Harness runs on the same device: both new tests fail on the baseline APK and pass with the fix (hooks and frame suites, 19 passed, 3 skipped). The two camera-view failures (initial zoom, initial torch) fail on the baseline too.

@vercel

vercel Bot commented Oct 19, 2026

Copy link
Copy Markdown

@chocky335 is attempting to deploy a commit to the Margelo Team on Vercel.

A member of the Team first needs to authorize it.

@chocky335 chocky335 changed the title fix: Release the Frame Output thread on dispose and unmount fix: Let the Frame Output thread exit once nothing uses it Oct 20, 2026
@chocky335

chocky335 commented Oct 20, 2026 •

Copy link
Copy Markdown
Author

Device results for the lifetime change, Pixel 8 Pro (Android 17), debug build, adb shell ps -T -p <pid> sampled once a second.

Check Runs Before After
Frame threads after 10 mount and unmount cycles of a frame processor, then a 7 s hold 10 cycles 10 (main), 0 only with the hook-side dispose 0, with no dispose
<Activity> hide and show with a frame processor 10 cycles with 2 s hides, 10 with 7 s hides reveal reused a disposed output frames resume every time; 2 s hides keep the same thread, 7 s hides start a new one
Effect-only reveal (memoized parent, child with a real <Camera> and with useCamera) 2 reveals each child effects saw a disposed output same live output, frames resume, no error
Session that outlives the component, 6 forced Hermes GCs plus ART GC signals during a 5 s hold 1 n/a no FATAL EXCEPTION, no failed frame callback
SkiaCamera inside <Activity>, 7 s hide 1 n/a rendering resumes, no GL error in logcat
useAsyncRunner mounted and unmounted 10 times, then a Hermes GC and ART GC signals 1 10 async-runner threads stay 10 during the cycles, 0 after the GCs

The hook stopped disposing the output because a cleanup cannot tell a hide from an unmount. When React shows a hidden tree again, it reconnects child effects before parent effects, with the props they committed before the hide. So <Camera> gets the object the hook returned earlier, whatever the hook does in its own effects. A subtree deleted while hidden runs no cleanup at all. The executor knows when it is idle and when nothing references it, so the thread lifetime belongs there.

Two guards keep a frame from reaching a dead worklet runtime. The Kotlin analyzer reads the callback for each frame, because CameraX runs the analyzer it captured when it posted the frame. The worklets provider queues a small task on the frame thread behind every callback removal, so the runtime stays reachable until that removal has run.

finalize() on the executor is safe because the worker thread references only the pool, never the executor wrapper. An unreachable wrapper is collected and shuts its pool down.

Red check: with the three Kotlin files and the two JS files from this PR's previous version, 8 of the new harness tests fail. At this head, all 34 pass (3 skipped on Android).

@chocky335

Copy link
Copy Markdown
Author

Independent confirmation on #4213: guuhgalvao ported this branch onto 5.1.1 and saw one camera.frame thread per session and none after close over 10 open/close cycles, also on a 3 GB emulator (#4213 (comment)).

This branch has not been deployed

No deployments
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.

🐛 [Android] Each FrameOutput leaks its native thread (dispose() doesn't shut down the executor; useFrameOutput never disposes)

1 participant