Repository navigation
feat: say when the device's video decoder stands still (#145) - #147
Merged
Merged
Conversation
…e in several cells, none decoded for 20 s (#145) A banner over the grid asks to restart the device; /stats.json has decoder_stuck and the debug page shows it in red. Cells losing packets on the network do not count. Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Collaborator
Author
|
/review |
Code Review by Qodo
1. Slow cameras can trigger a false alarm
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+1047
to
+1049
| /** | ||
| * Seen on the AG-216: every cell receives but no decoder gives a frame, and only a reboot | ||
| * of the device helps (#145). Said on screen instead of black cells and "0.0 fps". |
There was a problem hiding this comment.
2. The decoder lockup remains unexplained 📎 Requirement gap ☼ Reliability
The new decoder-stuck handling records that the issue was seen on one device and that rebooting helped, but it documents no reproduction path or likely cause. Because the lockup cannot be reproduced from the added evidence, investigators still cannot determine what lifecycle disruption leaves the decoder service stuck.
Agent Prompt
## Issue description
The decoder-stuck change notes an observed lockup and reboot recovery but does not document how to reproduce the failure or what likely causes it, leaving the decoder-service investigation incomplete.
## Fix Focus Areas
- android/app/src/main/java/com/openipc/decoder/Decoder.java[1047-1049]
## Recommended Fix
Document the lifecycle sequence, app and camera state, stream configuration, timing, and device conditions that reproduce the lockup, along with the suspected decoder-service failure and evidence supporting that hypothesis. Add the investigation notes to the appropriate project documentation or issue rather than relying only on an inline observation.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
…put of any cell counts, any RTP gap marks a cell lossy, leaving the grid resets the detector Co-Authored-By: Claude <noreply@anthropic.com>
Collaborator
Author
|
Qodo → 2e1df42:
|
Collaborator
Author
|
/review |
Your assessment is correct:
|
| * network may decode nothing for that reason alone (#113, #133). | ||
| */ | ||
| boolean isReceiving() { | ||
| return pausedReason == null && "connected".equals(state) && inFps >= 1 && !networkLoss; |
There was a problem hiding this comment.
2. One-frame-per-second cameras hide a stall 🐞 Bug ≡ Correctness
isReceiving() requires a sampled inFps of at least 1, but sample() divides the frame count by the actual interval between stats callbacks. When a camera sends one frame per second and callbacks run slightly more than one second apart, its measured rate falls below 1 and repeatedly resets the detector, even if two such cameras keep receiving frames without decoding.
Agent Prompt
## Issue description
A one-frame-per-second stream can measure below 1 FPS because the sampling interval is slightly longer than one second, preventing the stalled-decoder detector from counting it.
## Fix Focus Areas
- android/app/src/main/java/com/openipc/decoder/QuadCell.java[272-289]
- android/app/src/main/java/com/openipc/decoder/QuadCell.java[422-428]
## Recommended Fix
Base receiving eligibility on frame arrivals over the sampled interval, with timing tolerance appropriate for one-frame-per-second streams, rather than requiring the computed floating-point rate to reach exactly 1.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+1046
to
+1048
| static int stuckStep(int seconds, int receiving, boolean anyOutput) { | ||
| return receiving >= 2 && !anyOutput ? seconds + 1 : 0; | ||
| } |
There was a problem hiding this comment.
3. Delayed grid ticks postpone the warning 🐞 Bug ≡ Correctness
stuckStep() adds one to stuckSeconds per invocation rather than measuring elapsed time. Because the next stats callback is posted one second after the current callback finishes, UI work or scheduling delays make twenty counted ticks take longer than twenty seconds even while receiving cells produce no output.
Agent Prompt
## Issue description
The detector counts stats callbacks as seconds, so delayed callbacks postpone the warning beyond the intended duration.
## Fix Focus Areas
- android/app/src/main/java/com/openipc/decoder/Decoder.java[1027-1032]
- android/app/src/main/java/com/openipc/decoder/Decoder.java[1037-1071]
## Recommended Fix
Track the monotonic time at which consecutive eligible, output-free observations began and compare elapsed time against the threshold. Clear that start time when the condition breaks or the grid resets.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| private int dropped; | ||
| /** Of those, dropped because a packet of them was lost (not for size) (#113). */ | ||
| private int lostDropped; | ||
| private int gaps; |
There was a problem hiding this comment.
4. Packet losses can escape the clean check 🐞 Bug ☼ Reliability
NalAssembler.gaps is incremented on the packet-processing thread and read through gapCount() on the UI thread without a visibility guarantee. When a sampling read observes a stale count, sample() leaves networkLoss false for that interval, allowing a lossy cell to contribute to a device-wide stuck diagnosis.
Agent Prompt
## Issue description
The new packet-gap counter is shared between packet processing and UI sampling without a visibility guarantee, so a sample can miss a loss.
## Fix Focus Areas
- android/app/src/main/java/com/openipc/decoder/NalAssembler.java[44-67]
- android/app/src/main/java/com/openipc/decoder/NalAssembler.java[90-93]
- android/app/src/main/java/com/openipc/decoder/QuadCell.java[283-289]
## Recommended Fix
Give gap-count writes and reads a thread-safe publication mechanism, such as an atomic counter, and retain the per-sample comparison in the cell.
ⓘ 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 2e1df42 |
…come from its JPEG assembler Co-Authored-By: Claude <noreply@anthropic.com>
Collaborator
Author
|
/review |
|
Code review by qodo was updated up to the latest commit 0ea5a2e |
…assembler, the threshold is time not ticks, the gap count is published to the UI thread Co-Authored-By: Claude <noreply@anthropic.com>
Collaborator
Author
|
Qodo → b009f85:
|
Collaborator
Author
|
/review |
The changes address findings 4, 5, 6, and 7:
Finding 3 is not fully addressed by this diff. I don’t see a |
Comment on lines
+507
to
+509
| boolean isReceiving() { | ||
| return pausedReason == null && "connected".equals(state) && unitsArrived | ||
| && !networkLoss; |
There was a problem hiding this comment.
1. Slow cameras can trigger a false alarm 📎 Requirement gap ≡ Correctness
isReceiving() counts any assembled unit during the last sample but never enforces the required incoming rate of at least 1 fps. Two connected, unpaused cells sending below that rate can still satisfy the detector across successive samples and eventually show a device-decoder warning even though the stated detection condition is not met.
Agent Prompt
## Issue description
The receiving predicate accepts any assembled unit in a sampling interval but does not require the incoming rate to be at least 1 fps, allowing slow streams to trigger the device-wide decoder-stuck warning.
## Fix Focus Areas
- android/app/src/main/java/com/openipc/decoder/QuadCell.java[507-509]
## Recommended Fix
Include the measured incoming frame rate in `isReceiving()` and require it to be at least 1 fps, while preserving the connected, unpaused, assembled-unit, and no-network-loss checks.
ⓘ 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 b009f85 |
…unt for the stuck-decoder check; JPEG drop count published to the UI thread Co-Authored-By: Claude <noreply@anthropic.com>
Collaborator
Author
|
Qodo → 6e41104:
|
Collaborator
Author
|
/review |
Your rationale appears to address the current open findings:
Based on the supplied diff, findings 4–6 look addressed, and finding 1 is a requirements/interpretation dispute rather than an unambiguously missing implementation. The published review has not yet reflected these conclusions. |
|
Code review by qodo was updated up to the latest commit 6e41104 |
…ll with its own decoder problem is left out of the stuck-decoder check Co-Authored-By: Claude <noreply@anthropic.com>
Collaborator
Author
|
Qodo → 72f5be6: Long key-frame waits — on key frames only (or waiting for one), only key slices count as arriving; Decoder errors — a cell with its own |
Collaborator
Author
|
/review |
Confirmed in |
|
Code review by qodo was updated up to the latest commit 72f5be6 |
Collaborator
Author
|
/review |
Comment on lines
+1034
to
+1035
| static final long STUCK_MS = 20_000; | ||
| /** Since when the condition holds, or -1 (monotonic ms; Qodo on #147: time, not ticks). */ |
There was a problem hiding this comment.
4. Decoder warning timing cannot be changed 📎 Requirement gap ⚙ Maintainability
The stalled-decoder detector hard-codes STUCK_MS to 20 seconds instead of obtaining the interval from configuration. isStuck() always applies that fixed value, so deployments cannot tune the detection window to their stream or device characteristics.
Agent Prompt
## Issue description
The stalled-decoder detector uses a compile-time 20-second threshold, so deployments cannot configure the interval required by the decoder-stall detection criteria.
## Fix Focus Areas
- android/app/src/main/java/com/openipc/decoder/Decoder.java[1034-1035]
- android/app/src/main/java/com/openipc/decoder/Decoder.java[1048-1049]
## Recommended Fix
Read the stall interval from the app's existing configuration or preferences, provide a documented default of 20 seconds, and use that configured value when evaluating whether the detector has been stuck long enough.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+518
to
+519
| return pausedReason == null && problem == null && "connected".equals(state) | ||
| && unitsArrived && !networkLoss; |
There was a problem hiding this comment.
5. Missing video surfaces prompt a reboot 🐞 Bug ≡ Correctness
isReceiving() counts a connected cell with arriving picture slices even when its rendering surface has been destroyed. If this persists in two cells for 20 seconds, neither can create a decoder, yet the grid displays a device-restart warning.
Agent Prompt
## Issue description
A connected cell can keep receiving slices after its surface is destroyed. Two such cells can trigger the device-decoder warning even though decoder creation is blocked by missing surfaces.
## Fix Focus Areas
- android/app/src/main/java/com/openipc/decoder/QuadCell.java[516-520]
- android/app/src/main/java/com/openipc/decoder/QuadCell.java[213-225]
## Recommended Fix
Require a non-null, valid surface in `isReceiving()` before counting a cell. Do not require an already-created decoder, since a genuinely stuck decoder may never start.
ⓘ 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 8fd5206 |
compot-dev
added a commit
that referenced
this pull request
Oct 15, 2026
…put of any cell counts, any RTP gap marks a cell lossy, leaving the grid resets the detector Co-Authored-By: Claude <noreply@anthropic.com>
compot-dev
added a commit
that referenced
this pull request
Oct 15, 2026
…assembler, the threshold is time not ticks, the gap count is published to the UI thread Co-Authored-By: Claude <noreply@anthropic.com>
compot-dev
added a commit
that referenced
this pull request
Oct 15, 2026
…unt for the stuck-decoder check; JPEG drop count published to the UI thread Co-Authored-By: Claude <noreply@anthropic.com>
compot-dev
added a commit
that referenced
this pull request
Oct 15, 2026
…ll with its own decoder problem is left out of the stuck-decoder check Co-Authored-By: Claude <noreply@anthropic.com>
compot-dev
added a commit
that referenced
this pull request
Oct 15, 2026
feat: say when the device's video decoder stands still (#145)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #145 (detection; the cause stays open).
When at least two multi-view cells receive picture data (connected, not paused, H.264/H.265 slices arrived since the last sample, no RTP packet lost that second; MJPEG cells do not count) and none decodes a frame for 20 s in a row, the app shows a banner over the grid:
/stats.json:decoder_stuck(true/false) in multi-view;video decoder not responding — restart the devicein the header;decoderStuckNeedsSeveralReceivingCellsAndNoOutput.Device test (AG-216, CI build 2b9185d): normal 3×3 grid for 90 s —
decoder_stuck: falsethroughout, no banner. The stuck state itself (seen once today, fixed only by a reboot) cannot be produced on demand, so the positive path is covered by the unit test and review.