Repository navigation
fix: set up one decoder at a time — nine at once stalled SurfaceFlinger (#76) - #127
compot-dev wants to merge 3 commits into
Conversation
…re its surface goes away (#76)
|
/review |
Code Review by Qodo
1. Codec shutdown can race worker exit
|
…d SurfaceFlinger (#76)
|
First device run (48e5449, surface fix only): not enough — 10 cycles still gave stalls of 15, 3, 30, 21 and 49 s and an ANR. The longest one (67.8 s in a second run) came on single → grid, i.e. while the grid is built and up to nine hardware decoders are created at once. Second commit: decoders are set up one after another, at least 250 ms apart (a static lock in |
|
/agentic_review |
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| // The holder callback stays until surfaceDestroyed(): there the decoder is closed | ||
| // before the surface goes away. Removing it here let the grid's surfaces be destroyed | ||
| // while up to nine decoders were still being released on their own threads, and | ||
| // SurfaceFlinger then stalled the window's buffers for tens of seconds (#76) |
There was a problem hiding this comment.
1. Codec shutdown can race worker exit 📎 Requirement gap ☼ Reliability
QuadCell.stop() starts asynchronous codec release and immediately calls shutdownNow() without waiting for the decoder worker to terminate. A worker still inside decoding or starting a codec can overlap with release when the stopped cell's surface is destroyed, reaching the codec lifecycle from both threads.
Agent Prompt
## Issue description
Stopped cells retain asynchronous codec release while their decode executors are only interrupted, not awaited, so codec operations can overlap with a worker that has not exited.
## Fix Focus Areas
- android/app/src/main/java/com/openipc/decoder/QuadCell.java[208-229]
- android/app/src/main/java/com/openipc/decoder/MediaCodecManager.java[190-242]
## Recommended Fix
Coordinate worker termination and codec release so the codec is released only after the decode worker has exited. Keep `onPause()` and `stop()` nonblocking by completing the wait and release on a worker thread, and ensure a stopped cell cannot create or use another decoder during that transition.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| // The holder callback stays until surfaceDestroyed(): there the decoder is closed | ||
| // before the surface goes away. Removing it here let the grid's surfaces be destroyed | ||
| // while up to nine decoders were still being released on their own threads, and | ||
| // SurfaceFlinger then stalled the window's buffers for tens of seconds (#76) |
There was a problem hiding this comment.
3. Grid surfaces can outlive decoder release 🐞 Bug ≡ Correctness
QuadCell.stop() retains the callback so surfaceDestroyed() can call closeDecoder(), but setEnabled(false) can detach the decoder under decoderLock and release it after unlocking. If that release thread detaches first, closeDecoder() sees no decoder and returns while codec release is still running, allowing grid teardown to proceed.
Agent Prompt
## Issue description
A stopped cell's surface-destruction callback can return while the asynchronous decoder release is still running.
## Fix Focus Areas
- android/app/src/main/java/com/openipc/decoder/QuadCell.java[171-174]
- android/app/src/main/java/com/openipc/decoder/MediaCodecManager.java[198-249]
## Recommended Fix
Track completion of an in-flight decoder release so teardown cannot treat a detached decoder as a completed release. Arrange grid hiding only after outstanding releases finish.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 4414500 |
|
Qodo review — decisions: findings 1–3 are all about the first commit (the stopped cell keeping its holder callback). It did not help on the device either, and finding 3 shows why it could not: |
compot-dev
changed the title
Understood. Findings 1–3 concern the reverted stopped-cell/holder-callback change and do not apply to the current diff. The remaining change serializes decoder setup under |
1 participant
Closes #76 (if the device check confirms it).
Cause (likely):
QuadCell.stop()removed the SurfaceHolder callback and released the decoder asynchronously (setEnabled(false)hands the release to a short-lived thread, #32). Right after,stopQuad()hides the grid, so up to nine SurfaceViews lose their surfaces while their MediaCodec instances are still attached and being released — and since the callback was gone,surfaceDestroyed()could not stop rendering first, which Android requires. On the AG-216 this matches what #76 shows: SurfaceFlinger does not take the window's buffers for tens of seconds (Davey! … QueueBufferDuration=48550217000), and after many cyclesmedia.codechung inDstate (load average 74, only a reboot helped). Reproduced today with a script (grid → single → grid): stalls of 26 s, 9 s, 2.7 s and 1 s in 8 cycles.Fix: a stopped cell keeps its holder callback until
surfaceDestroyed(); there it closes the decoder synchronously (it waits for the release in progress — at most one 5 ms poll since #106) and only then removes the callback.surfaceCreated()ignores a stopped cell.Device check: the same script, 10+ cycles on Aceline-1 — no
Daveyover 1 s, no ANR,media.codecnot inDstate.