Skip to content

fix: decoder recovery and lifecycle — Quad H.265, resume, thread leak, pause (#20 #21 #22 #23 #32) - #48

Merged
compot-dev merged 13 commits into
v2from
fix/decoder-lifecycle
Oct 12, 2026
Merged

compot-dev merged 13 commits into
v2from
fix/decoder-lifecycle

Conversation

@compot-dev

Copy link
Copy Markdown
Collaborator

Fixes #20, fixes #21, fixes #22, fixes #23, fixes #32.

⚠️ Do not merge before testing on a device (label awaiting device test). These are the parts of the app that JVM tests cannot exercise: MediaCodec, Surface, Activity lifecycle.

Issue Problem Fix
#21 After any MediaCodec error the decoder was never re-created; RTP kept flowing, so there was no reconnect either DecoderRetry: the next attempt waits 0.5 → 1 → 2 → 4 → 8 → 10 s, and a decoded frame resets the back-off. A new decoder skips input until the first SPS/VPS/PPS
#20 In Quad an H.265 camera stays black: the codec-switch callback on the first packet closed the decoder, and it was never created again The cell owns one MediaCodecManager; the decode thread creates a decoder whenever there is none. Creation on the network thread is gone (it was also a source of a race)
#22 The saved camera and Quad mode were never read back; after a pause in Quad mode the screen was blank loadSettings() reads active and quad_enabled. In single view the Quad layout is hidden. A shared link opens in single view
#23 Every camera switch or URL save leaked 2 threads startListener() shuts down the previous executor
#32 onPause() blocked the UI thread for seconds MediaCodecManager.setEnabled(false) releases the decoder and blocks creating a new one, so there is nothing to wait for. Only the audio thread is waited for, at most 300 ms. QuadCell.stop() does not wait

Also: the frame that triggered decoder creation (usually the SPS) is passed to the new decoder; previously it was lost and the picture waited for the next GOP.

Tests

91 JVM tests in total; 5 are new in DecoderRetryTest:

assembleDebug and lintDebug pass with no new issues.

Device checklist

A MediaCodec exception used to set decoderFailed for good: no decoder was
created again until the user switched camera, while RTP kept flowing so
the watchdog never fired either (fixes #21).

- DecoderRetry: the next attempt after a failure waits 0.5 s, 1, 2, 4,
  8 s, at most 10 s; a decoded frame resets it. Failed creation backs
  off the same way.
- A new decoder skips input until the first parameter set (SPS/VPS/PPS),
  instead of getting P-frames it cannot decode.
- setEnabled(false) releases the decoder and refuses to create another,
  so a late decode thread cannot leave one open after a pause or stop.
A Quad cell created its MediaCodecManager once (on the network thread)
and returned early from initDecoder() when it already existed. On the
first H.265 packet the codec-switch callback closed the decoder, and it
was never created again: H.265 cameras stayed black (fixes #20).

The cell now owns one MediaCodecManager for its lifetime; the decode
thread creates the decoder whenever it is missing and feeds it the frame
that triggered creation. stop() disables the manager instead of waiting
up to 2 s for the executor.
…ause

- loadSettings() reads back "active" and "quad_enabled", which were
  saved but never loaded; single view hides the Quad layout again. A
  shared link opens in single view (fixes #22).
- startListener() shuts down the previous executor, so switching camera
  or saving a URL no longer leaks two idle threads (fixes #23).
- onPause() disables the decoder and waits at most 300 ms for the audio
  thread instead of 2 s per executor, and Quad cells no longer block
  (fixes #32).
- The frame that triggers decoder creation is passed to the new decoder.

Tests: back-off delays and reset, the enable/disable gate, slot clamping.
@compot-dev compot-dev added the awaiting device test PR ready, merge only after testing on a real device label Oct 12, 2026
@qodo-free-for-open-source-projects

Copy link
Copy Markdown

PR Summary by Qodo

Recover video decoders and restore playback across lifecycle changes

🐞 Bug fix 🧪 Tests 🕐 40+ Minutes

Grey Divider

AI Description

• Retry failed decoders and recreate Quad decoders after codec switches, preserving the triggering
 frame.
• Restore the selected camera and view mode after pause, including correct shared-link behavior.
• Stop abandoned worker pools and bound pause-time waiting; add JVM coverage for retry and lifecycle
 gates.
Diagram

graph TD
  Activity["Activity lifecycle"] --> Single["Single decode"] --> Manager["Decoder manager"] --> Retry{"Retry eligible?"} -- "yes" --> Codec["MediaCodec"] --> Surface["Video surface"]
  Activity --> Quad["Quad cells"] --> Manager
  Retry -- "wait" --> Manager
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Reconnect RTSP after decoder errors
  • ➕ Could obtain fresh stream configuration from the camera.
  • ➖ Interrupts a healthy RTP connection and does not directly address MediaCodec failure or Quad decoder ownership.

Recommendation: Keep decoder recovery local to MediaCodecManager: RTP can continue flowing after a codec error, so reconnecting the stream is unnecessary for the primary failure. Require the stated device testing before merge because JVM tests cannot validate codec, surface, or Activity timing.

Files changed (5) +220 / -96

Bug fix (4) +143 / -96
Decoder.javaRestore playback state and bound single-view shutdown +38/-18

Restore playback state and bound single-view shutdown

• Loads the saved camera slot and Quad setting, clamps invalid slots, and opens shared links in single view. Shuts down previous listener pools, re-feeds frames that trigger decoder creation, and limits pause-time waiting before releasing audio.

android/app/src/main/java/com/openipc/decoder/Decoder.java

DecoderRetry.javaAdd capped decoder-recreation back-off +44/-0

Add capped decoder-recreation back-off

• Introduces thread-safe retry timing that increases after failures, caps at ten seconds, and resets after a decoded frame.

android/app/src/main/java/com/openipc/decoder/DecoderRetry.java

MediaCodecManager.javaRecover failed codecs and gate creation while disabled +39/-11

Recover failed codecs and gate creation while disabled

• Replaces permanent decoder failure with timed retries for creation and decode errors. Disabling releases the decoder and prevents replacement; newly created decoders skip input until a parameter-set NAL arrives.

android/app/src/main/java/com/openipc/decoder/MediaCodecManager.java

QuadCell.javaRecreate Quad decoders on the decode thread +22/-67

Recreate Quad decoders on the decode thread

• Gives each cell one persistent decoder manager and creates a decoder when needed, including after an H.265 codec switch. Re-feeds the triggering frame and disables decoding on stop without waiting for executor termination.

android/app/src/main/java/com/openipc/decoder/QuadCell.java

Tests (1) +77 / -0
DecoderRetryTest.javaTest retry timing, disable gate, and saved-slot fallback +77/-0

Test retry timing, disable gate, and saved-slot fallback

• Adds five JVM tests for capped back-off, retry eligibility and reset, disabled-manager behavior, and invalid saved camera slots.

android/app/src/test/java/com/openipc/decoder/DecoderRetryTest.java

@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Oct 12, 2026 •

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Action required

1. Pausing the app can still stall the UI ✓ Resolved
Description
setEnabled(false) synchronously calls closeDecoder() while decodeFrame() holds decoderLock
during dequeueInputBuffer(5_000). If that codec call waits, onPause() and QuadCell.stop() can
block the main thread for several seconds before shutdown and audio cleanup proceed.
Code

android/app/src/main/java/com/openipc/decoder/MediaCodecManager.java[R56-60]

+    /** Enable or disable decoding; disabling also releases the decoder. */
+    void setEnabled(boolean enabled) {
+        this.enabled = enabled;
+        if (!enabled) closeDecoder();
+    }
Evidence
The lifecycle code invokes setEnabled(false) directly from pause and cell-stop paths. The method
immediately calls closeDecoder(), which uses the same lock held by decodeFrame() while it
performs a five-second MediaCodec input dequeue, so the lifecycle caller can synchronously wait on a
potentially blocking codec operation.

Keep onPause from blocking the UI thread
android/app/src/main/java/com/openipc/decoder/MediaCodecManager.java[56-60]
android/app/src/main/java/com/openipc/decoder/MediaCodecManager.java[149-160]
android/app/src/main/java/com/openipc/decoder/Decoder.java[737-745]
android/app/src/main/java/com/openipc/decoder/QuadCell.java[173-178]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`setEnabled(false)` calls `closeDecoder()` synchronously, but `decodeFrame()` can hold `decoderLock` while waiting up to five seconds for MediaCodec input. Calling this from `onPause()` or `QuadCell.stop()` can therefore stall the UI thread despite the intended bounded shutdown.
## Fix Focus Areas
- android/app/src/main/java/com/openipc/decoder/MediaCodecManager.java[56-60]
- android/app/src/main/java/com/openipc/decoder/MediaCodecManager.java[149-160]
- android/app/src/main/java/com/openipc/decoder/Decoder.java[737-745]
- android/app/src/main/java/com/openipc/decoder/QuadCell.java[173-178]
## Recommended Fix
Avoid holding `decoderLock` across blocking MediaCodec operations, or move decoder shutdown and codec operations off the UI thread. Make the disable operation signal the decode worker to stop, then release the codec after the worker exits while keeping lifecycle waits bounded and interruptible.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. Audio can keep playing after a pause ✓ Resolved
Description
onPause() calls closeAudio() after waiting at most 300 ms for the audio worker, without ensuring
that worker has finished creating or writing to an AudioTrack. If the worker remains inside audio
playback past the wait, it can publish a new track after closeAudio() has released the old one,
leaving playback active after pause or overlapping a quick resume.
Code

android/app/src/main/java/com/openipc/decoder/Decoder.java[745]

+                ex.awaitTermination(300, TimeUnit.MILLISECONDS);
Evidence
The new wait ends after 300 ms even if the worker is still in playPcmFrame(). Audio creation later
publishes its track through audioTrackRef.getAndSet(track), whereas closeAudio() only releases
the track present when it runs.

android/app/src/main/java/com/openipc/decoder/Decoder.java[741-750]
android/app/src/main/java/com/openipc/decoder/AudioManager.java[130-151]
android/app/src/main/java/com/openipc/decoder/AudioManager.java[234-274]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The bounded pause wait allows the audio worker to create an AudioTrack after `closeAudio()`.
## Fix Focus Areas
- android/app/src/main/java/com/openipc/decoder/Decoder.java[737-750]
- android/app/src/main/java/com/openipc/decoder/AudioManager.java[234-274]
## Recommended Fix
Add a shutdown gate synchronized with AudioTrack creation and release so an outstanding worker cannot publish a track after pause, without requiring an unbounded UI-thread wait.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Missing parameter sets can break recovery ✓ Resolved
Description
decodeFrame() clears awaitingConfig as soon as it receives one configuration NAL, rather than
waiting for all required parameter sets. If packet loss leaves a fresh H.265 decoder with only VPS
or SPS, or an H.264 decoder with only SPS, subsequent dependent frames reach it before the remaining
configuration arrives.
Code

android/app/src/main/java/com/openipc/decoder/MediaCodecManager.java[R153-158]

+            } else if (awaitingConfig && !isConfigNal) {
+                // P-frames without parameter sets only produce errors in a new decoder
+                return false;
 } else {
+                awaitingConfig = false;
     try {
Evidence
The code recognizes multiple distinct parameter-set types but clears the gate for all of them
identically. The assembler emits NALs as separate frames, so a missing parameter set is not supplied
implicitly with the first one.

android/app/src/main/java/com/openipc/decoder/MediaCodecManager.java[140-158]
android/app/src/main/java/com/openipc/decoder/NalAssembler.java[164-184]
android/app/src/main/java/com/openipc/decoder/QuadCell.java[213-221]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A fresh decoder stops filtering dependent frames after any single parameter-set NAL, even when other required sets are missing.
## Fix Focus Areas
- android/app/src/main/java/com/openipc/decoder/MediaCodecManager.java[140-158]
## Recommended Fix
Track the parameter sets received since decoder creation for the selected codec, and keep filtering dependent frames until the complete required configuration has been queued.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


4. A cancelled decoder delays video on resume ✓ Resolved
Description
createDecoder() records a setup failure without checking whether setEnabled(false) cancelled
that creation while setup ran outside decoderLock. If pause resets retry state before the
in-flight setup fails and the activity resumes immediately, the stale failure imposes a 500 ms
creation delay on the resumed stream.
Code

android/app/src/main/java/com/openipc/decoder/MediaCodecManager.java[R112-113]

+            retry.onFailure(nowMs());
+            Log.e(TAG, "Cannot setup decoder (attempt " + retry.failures() + "): " + e.getMessage());
Evidence
Creation checks enabled under the lock but performs setup outside it. Disabling resets retry state
through closeDecoder(), while the setup exception handler can subsequently call
retry.onFailure() unconditionally.

android/app/src/main/java/com/openipc/decoder/MediaCodecManager.java[57-67]
android/app/src/main/java/com/openipc/decoder/MediaCodecManager.java[83-125]
android/app/src/main/java/com/openipc/decoder/Decoder.java[737-745]
android/app/src/main/java/com/openipc/decoder/Decoder.java[769-772]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
An in-flight decoder setup can reinstate retry backoff after pause has disabled and reset the manager.
## Fix Focus Areas
- android/app/src/main/java/com/openipc/decoder/MediaCodecManager.java[57-65]
- android/app/src/main/java/com/openipc/decoder/MediaCodecManager.java[83-125]
## Recommended Fix
Associate each creation attempt with a lifecycle generation, and record setup failures only if that attempt still belongs to the enabled generation.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


View review recommended (1)
5. Shared-link view jumps back to Quad after screen off/on ✓ Resolved
Description
loadSettings() sets quadEnabled = false for a shared link only in memory and removes the intent
extra, but quad_enabled stays true in preferences, and the single-view branch of onResume()
does not write it back. On the next pause/resume (screen off/on, Home → back), loadSettings()
reads quad_enabled=true again, so the app quietly returns to Quad mode and the shared stream the
user was watching is gone, because it was never saved.
Code

android/app/src/main/java/com/openipc/decoder/Decoder.java[R341-343]

+            // A shared link is meant to be watched: show it in single view
+            quadEnabled = false;
 intent.removeExtra(Intent.EXTRA_TEXT);
Evidence
loadSettings now restores quadEnabled from prefs on every onResume (line 331). The shared-link
override (line 342) is never persisted: the onResume else-branch (lines 792-803) only changes
visibility and starts the stream. Only startQuad/stopQuad write quad_enabled (lines 488-490,
493-495). The intent extra is removed (line 343), so on the next resume the override no longer
applies and the pref value wins.

android/app/src/main/java/com/openipc/decoder/Decoder.java[329-346]
android/app/src/main/java/com/openipc/decoder/Decoder.java[767-803]
android/app/src/main/java/com/openipc/decoder/Decoder.java[488-496]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
When a shared link arrives, `loadSettings()` sets `quadEnabled = false` only in memory. The `quad_enabled` preference stays true, so the next onResume restores Quad mode and the shared stream is lost.
## Fix Focus Areas
- android/app/src/main/java/com/openipc/decoder/Decoder.java[338-344]
## Recommended Fix
In the `link != null` branch, after `quadEnabled = false;`, also run `pref.edit().putBoolean("quad_enabled", false).apply();`. You can also persist the shared host (`host_<mActive>`) so it survives the next resume, if that is the intended behaviour.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Tip of the day
💡 Did you know, you can commit Qodo's fix in one click with committable suggestions (GitHub & GitLab)

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread android/app/src/main/java/com/openipc/decoder/Decoder.java Outdated
Comment thread android/app/src/main/java/com/openipc/decoder/MediaCodecManager.java Outdated
Comment thread android/app/src/main/java/com/openipc/decoder/Decoder.java Outdated
- setEnabled(false) takes the decoder under the lock and releases it on
  a short-lived thread, so MediaCodec stop()/release() never run on the
  UI thread.
- A new decoder waits for the complete parameter set (SPS+PPS for
  H.264, VPS+SPS+PPS for H.265) before it gets other frames.
- Each decoder creation belongs to a lifecycle generation: one cancelled
  by a pause neither installs a decoder nor records a failure that would
  delay the next start.
- AudioManager.setEnabled(false) on pause: an audio worker still running
  cannot create or publish an AudioTrack or an AAC decoder afterwards;
  onPause no longer waits for it at all.
- A shared link is saved (slot URL and quad_enabled=false), so the next
  pause/resume does not jump back to Quad and lose it.
Disabling only sets the flags; the decoder is detached under decoderLock
and released on the decoder-release thread, so the UI thread does not
wait even for the few milliseconds a decode step holds the lock. While
disabled, decodeFrame() feeds nothing.
clearQuadViews() cast each cell's parent to FrameLayout, but the cells
were added straight to a LinearLayout row. The code never ran before,
because Quad mode was never restored on resume; with #22 fixed it would
throw ClassCastException on the first resume in Quad mode.

Each TextureView now sits in its own FrameLayout, and the black
placeholder overlays are added to and found in that container. Before,
findViewWithTag() was called on the TextureView itself, so the overlays
never worked.
@compot-dev

Copy link
Copy Markdown
Collaborator Author

Found while preparing #12 and fixed in 0a2e550: clearQuadViews() cast each Quad cell's parent to FrameLayout, but the cells sat directly in a LinearLayout. This code path never ran before (Quad mode was not restored on resume); with #22 fixed it would have crashed with ClassCastException on the first resume in Quad mode. Each cell is now wrapped in its own FrameLayout, and the placeholder overlays (which never worked, because findViewWithTag was called on the TextureView) live in that container.

Added to the device checklist: Home → back while in Quad mode does not crash, and the black placeholders disappear when each camera starts.

Found on the device (#53):

- A link shared while the app is already open was delivered through
  onNewIntent(), which was not overridden, so getIntent() still held the
  old intent and the link was ignored. onNewIntent() now calls
  setIntent(); onResume() applies it.
- RTSP response lines and URL errors are redacted before logging:
  user:password@ and password=/pass=/pwd= parameters. XM cameras put the
  password in the URL path, and Content-Base echoed it to logcat
  (fixes #55).
- A connection closed by stop() is no longer logged as
  "RTSP connection failed".
- QuadCell logs when its surface is ready and the session starts (to
  diagnose a black Quad cell seen once on the device).
On Android 10 the device delivered the SEND intent to the resumed
activity through onNewIntent() without onPause()/onResume() (confirmed on
the AG-216 box: no Link: log after the share). onNewIntent() now restarts
the streams itself, which re-reads the settings and the new intent.
onPause()/onResume() bodies moved into stopStreams()/startStreams().
With the standard launch mode, a SEND intent with FLAG_ACTIVITY_NEW_TASK
(as 'am start' and some apps send it) only brought the existing task to
the front: nothing was delivered and the link was lost (seen on the
AG-216 box). singleTop delivers it to the running activity through
onNewIntent(); shares without NEW_TASK still create a new instance in
the sharing app's task, as before.
… H.265

Found on the device (#53):

- startQuad() shut the executor down without bumping listenerGen. The
  audio loop kept running, and playPcmFrame() returned at once on every
  call with the interrupt flag set: one core at 100% for as long as Quad
  was shown. stopExecutor() now invalidates the generation first, and
  the loop also stops when interrupted.
- NalAssembler treated the first H.265 packet as a switch from H.264
  and closed the decoder that had just been created (two "Start video
  decoder" 0.6 s apart). The first packet now only sets the codec.
e8a7f2d made stopExecutor() bump listenerGen, but startListener() read the
generation before calling it, so the new decode and audio threads exited
at once and single view showed no video (caught by the device smoke test:
0 decoder starts).
On the Android 10 test box the Quad container sometimes stayed at 0x0
after being made visible, with no traversal scheduled, so no cell got a
surface and Quad stayed black until the next resume (seen after a start
brought the task to the front). 500 ms after showing Quad, if it still
has no size, force a layout pass and log it.
RtspSessionTest iterated server.requests (a synchronized list) while the
server thread was still appending, which threw ConcurrentModification-
Exception now and then (failed once in CI on #54). Tests now read a copy
taken under the list's lock.
@compot-dev

Copy link
Copy Markdown
Collaborator Author

Device-tested on the AG-216 Android 10 box with 3 cameras (results in #53, runs 2–4): #20, #21, #22, #23, #32 and #55 pass. Fixes found during testing are included: a shared link reaches the running app (singleTop + onNewIntent), password redaction in logcat, the spinning audio thread, the double decoder start on H.265, and a Quad layout safeguard (#56 deferred).

@compot-dev compot-dev removed the awaiting device test PR ready, merge only after testing on a real device label Oct 12, 2026
@compot-dev
compot-dev merged commit 5f208f7 into v2 Oct 12, 2026
4 checks passed
@compot-dev
compot-dev deleted the fix/decoder-lifecycle branch October 13, 2026 17:16
compot-dev added a commit that referenced this pull request Oct 15, 2026
fix: decoder recovery and lifecycle — Quad H.265, resume, thread leak, pause (#20 #21 #22 #23 #32)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant