Repository navigation
feat: S is the playback mode (stream / key frames) in every view; Q is the view (#153) - #155
Conversation
|
/review |
Co-Authored-By: Claude <noreply@anthropic.com>
|
/review |
Code Review by Qodo
1. Healthy streams can fall back to TCP
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| * decoder, the rest is dropped; after S is turned off, until the next key frame. | ||
| */ | ||
| private boolean skipForKeyFrames(Frame buffer) { | ||
| if (!slides && !singleWaitKey) return false; |
There was a problem hiding this comment.
1. Single-view mode changes may be ignored 🐞 Bug ☼ Reliability
skipForKeyFrames() reads slides on the video worker, but setSlides() writes that non-volatile field on the UI thread without synchronization. When S is toggled in single view, the worker is not guaranteed to observe the new value, so it can keep passing every frame or keep dropping inter frames after the menu shows the opposite mode.
Agent Prompt
## Issue description
The single-view video worker reads the UI-owned `slides` field without a visibility guarantee, so toggling S may not change frame selection.
## Fix Focus Areas
- android/app/src/main/java/com/openipc/decoder/Decoder.java[103-103]
- android/app/src/main/java/com/openipc/decoder/Decoder.java[1742-1745]
- android/app/src/main/java/com/openipc/decoder/Decoder.java[1789-1795]
## Recommended Fix
Make the playback-mode state safely visible to the video worker, for example by declaring `slides` volatile, and use that state consistently in both the MJPEG throttle and NAL filter.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| : codecH265 ? "H.265" : "H.264"); | ||
| single.put("audio", audioCodec); | ||
| // Frames shown so far and the playback mode of S (#153) | ||
| single.put("frames_out", videoDecoder.framesOut() + jpegFramesOut); |
There was a problem hiding this comment.
2. Camera stats include earlier streams 🐞 Bug ≡ Correctness
debugState() assigns single.frames_out the sum of two counters that persist across stream and decoder shutdowns. After a camera change or an MJPEG-to-video transition, the current single camera's stats include frames output by the previous stream, so the value cannot be read as that camera's output count.
Agent Prompt
## Issue description
The new single-view output count includes frames from earlier streams because its source counters are lifetime totals.
## Fix Focus Areas
- android/app/src/main/java/com/openipc/decoder/Decoder.java[1453-1459]
- android/app/src/main/java/com/openipc/decoder/Decoder.java[1851-1867]
## Recommended Fix
Record a baseline of the combined output counters when a single-view stream starts and subtract it in `debugState()`. Preserve the lifetime counters for existing consumers that use their differences.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Code Review by Qodo
1. Single-view playback can use the wrong mode
|
…ad reads it); frames_out is documented as a running total Co-Authored-By: Claude <noreply@anthropic.com>
|
Qodo → fix: 1. |
|
/review |
Co-Authored-By: Claude <noreply@anthropic.com>
Code Review by QodoNew Review StartedThis review has been superseded by a new analysis |
|
/review |
Agreed on both points:
The remaining open concerns are |
|
Code review by qodo was updated up to the latest commit 8f605dc |
…otal, the key-frame wait ends only with a frame that reaches the decoder, the MJPEG second counts from a frame shown Co-Authored-By: Claude <noreply@anthropic.com>
|
Qodo round 2 → 7be441e: 4. the key-frame wait ends only with a frame that passes the surface check to the decoder (filter after |
|
/review |
The round-2 changes address the listed points:
The view remains single throughout; |
|
Code review by qodo was updated up to the latest commit 7be441e |
…am (Qodo on #155) Co-Authored-By: Claude <noreply@anthropic.com>
|
/review |
| int type = NalAssembler.fragment(buffer.data()[4], codecH265); | ||
| if (!QuadCell.isKeyNal(type, codecH265)) return true; | ||
| if (!slides && QuadCell.isKeyFrameNal(type, codecH265)) singleWaitKey = false; |
There was a problem hiding this comment.
1. Healthy streams can fall back to tcp 🐞 Bug ≡ Correctness
skipForKeyFrames() drops non-key frames in single-view S mode, but singleUdpCheck still treats eight seconds without a rendered frame as a broken UDP stream. On a healthy UDP feed with key frames more than eight seconds apart and continuing packet traffic, the check reports a false stall, enables packet-loss salvage, and can force Auto transport onto TCP.
Agent Prompt
## Issue description
Single-view S mode intentionally drops non-key frames, but the UDP watchdog interprets the resulting gap in rendered frames as a transport failure.
## Fix Focus Areas
- android/app/src/main/java/com/openipc/decoder/Decoder.java[1794-1801]
- android/app/src/main/java/com/openipc/decoder/Decoder.java[1830-1854]
## Recommended Fix
Make the single-view UDP stall check distinguish intentional key-frame-only output gaps from evidence of packet loss before enabling salvage or reporting UDP broken. Preserve detection of genuinely stalled streams.
ⓘ 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 52ac0eb |
…ad reads it); frames_out is documented as a running total Co-Authored-By: Claude <noreply@anthropic.com>
…otal, the key-frame wait ends only with a frame that reaches the decoder, the MJPEG second counts from a frame shown Co-Authored-By: Claude <noreply@anthropic.com>
…am (Qodo on #155) Co-Authored-By: Claude <noreply@anthropic.com>
feat: S is the playback mode (stream / key frames) in every view; Q is the view (#153)
1 participant
Closes #153.
/stats.jsonsingle view:frames_outanddecode(full/key frames).Device test (AG-216, CI build of this branch):
decode: key frames, 1 fpsfull, 30 fpskey frames, 1 fpsVersion: 2.01 (versionCode 26).