fix(pipeline): 16-px-align the keep-aspect precrop so VENC does not stall

[vertexodessa]

Summary

pipeline_common_compute_precrop() aligned the keep-aspect crop to 2 px. VIF/VPE capture geometry must be 16-px aligned in size and 8-px in offset; an unaligned crop is accepted by every MI_* call and VPE then silently emits nothing, so the daemon sits at "waiting for encoder data" forever with no error logged anywhere.

Reproduced on Star6E with imx415 1472x816@120 and video0.size=1280x720: the 2-px-aligned crop is 1450x816@x=10 and VENC never produced a frame. 1280x704, whose crop is full-width 1472x808@y=4 with x=0, streamed immediately. The stab crop path (star6e_framing_stab.c) has always used width & ~7 with x & ~15 and is known good; the precrop now floors every size and offset to 16 px, a superset of that rule and of the working case.

On the review finding about aspect drift: flooring changes the crop aspect by at most 16/size, 0.74 % for this case, which the scaler absorbs invisibly. Rounding to nearest would halve that but replace the device-verified 1440x816@x=16 geometry with an unverified one, so the floor stays and the bound is documented in pipeline_common.h.

Changes

• src/pipeline_common.c: & ~1u -> & ~15u on sizes and offsets, with a comment recording the repro and the aspect bound.
• include/pipeline_common.h: contract now states the 16-px floor and the aspect bound (was "2-pixel alignment").
• tests/test_pipeline_common.c: pins 1472x816 -> 1280x720 = 1440x816@x=16 and -> 1280x704 = 1472x800@y=0, plus alignment and fits-in-sensor checks. Existing expectations (1440x1080@x=240, 2560x1440@y=240) are already aligned and unchanged.
• VERSION 0.65.2 -> 0.65.3, HISTORY.md entry.

Verification

• make test: 2491 passed, 0 failed
• make build SOC_BUILD=star6e: clean
• On device (Star6E, imx415, 1472x816@120): 1280x720 streams after the fix; before it, only 1280x704 did.

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Fix keep-aspect precrop alignment (16/8) to prevent Star6E VENC stall

🐞 Bug fix 🧪 Tests 📝 Documentation ⚙️ Configuration changes 🕐 20-40 Minutes

AI Description

• Align keep-aspect precrop size/offset to Star6E VIF/VPE 16/8 requirements
• Add regression tests for 1472x816→1280x720 and 1472x816→1280x704 cases
• Bump version to 0.65.3 and document the VENC-stall fix in history

Diagram

graph TD
  A["Sensor mode"] --> B["Precrop compute"] --> C["MI_* config"] --> D["VIF/VPE"] --> E["VENC"] --> F["Output frames"]
  G["Unit tests"] --> B

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Validate-and-fail fast on unaligned geometry

• ➕ Would surface driver/pipeline misconfiguration immediately (clear error logs)
• ➕ Protects against future regressions if alignment requirements change
• ➖ Does not proactively produce a working crop (still requires caller changes)
• ➖ Could break existing setups that currently rely on silent acceptance

2. Make alignment requirements platform-configurable

• ➕ Avoids hard-coding Star6E-specific constraints into common logic
• ➕ Easier to support SoCs with different alignment constraints
• ➖ Adds configuration/plumbing complexity and more test surface area
• ➖ Still needs robust defaults and platform detection

Recommendation: The PR’s approach (enforcing 16-px size and 8-px offset alignment directly in keep-aspect precrop) is the most pragmatic fix because it prevents the silent VPE no-output failure while preserving caller behavior. Consider a follow-up to add runtime validation/logging for alignment to improve diagnosability if downstream constraints evolve.

Files changed (4) +44 / -5

Bug fix (1) +11 / -4

pipeline_common.cAlign keep-aspect precrop to 16-px size and 8-px offset +11/-4

Align keep-aspect precrop to 16-px size and 8-px offset

• Changes keep-aspect precrop rounding from 2-pixel alignment to hardware-required alignment: widths/heights are masked to 16-pixel boundaries and x/y offsets to 8-pixel boundaries. Adds an in-code comment capturing the Star6E reproduction and the failure mode (MI_* accepts it but VPE emits nothing).

src/pipeline_common.c

Tests (1) +19 / -0

test_pipeline_common.cAdd regression coverage for 1472x816 keep-aspect alignment constraints +19/-0

Add regression coverage for 1472x816 keep-aspect alignment constraints

• Adds test vectors for 1472x816→1280x720 and 1472x816→1280x704 keep-aspect crops and asserts the resulting rectangles and alignment (size multiple of 16, offset multiple of 8). Keeps existing aligned expectations unchanged.

tests/test_pipeline_common.c

Documentation (1) +13 / -0

HISTORY.mdDocument Star6E VENC stall fix and 16/8 precrop alignment +13/-0

Document Star6E VENC stall fix and 16/8 precrop alignment

• Adds a 0.65.3 release-note entry describing the root cause (unaligned keep-aspect precrop) and the observed Star6E/imx415 reproduction scenario, documenting why 16/8 alignment is required.

HISTORY.md

Other (1) +1 / -1

VERSIONBump version to 0.65.3 +1/-1

Bump version to 0.65.3

• Updates the project version from 0.65.2 to 0.65.3 to reflect the bug fix release.

VERSION

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📜 Skill insights (0)

1. keep_aspect may distort 🐞 Bug ≡ Correctness

Description

By always flooring the computed crop dimension to a 16px multiple, the resulting crop aspect ratio
can deviate enough from the target that downstream scaling becomes meaningfully non-uniform even
when keep_aspect=true (e.g., 1472x816→1280x720 becomes 1440x816).

Code

src/pipeline_common.c[R226-227]

+		rect.w = (uint16_t)((sensor_h * image_w / image_h) & ~15u);
+		rect.x = (uint16_t)(((sensor_w - rect.w) / 2) & ~7u);

Evidence

The code floors the computed crop width/height to a 16px multiple, and the new test locks in a crop
(1440x816) for a 16:9 target, proving the crop AR no longer matches the target AR closely in that
case.

src/pipeline_common.c[224-234]
tests/test_pipeline_common.c[62-73]

Agent prompt

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

### Issue description
`pipeline_common_compute_precrop()` currently aligns by flooring (`& ~15u`) the computed crop size. This can increase aspect-ratio error and cause non-uniform scaling despite `keep_aspect=true`.

### Issue Context
Example from the updated tests: 1472x816 → 1280x720 yields a crop of 1440x816, which is not 16:9; scaling to 1280x720 will therefore scale X and Y by different factors.

### Fix Focus Areas
- src/pipeline_common.c[203-236]
- tests/test_pipeline_common.c[62-73]

### Suggested implementation direction
When computing the cropped axis (w or h), consider evaluating multiple aligned candidates (e.g., align-down vs align-up within sensor bounds) and choosing the one with the smallest aspect-ratio error, while still ensuring the derived centered offset satisfies the 8px alignment constraint.

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

---

2. Incorrect repro crop comment 🐞 Bug ⚙ Maintainability

Description

The documentation is now inconsistent with the keep-aspect precrop implementation: a new block
comment claims a “16/8-aligned 1472x808@y=4 crop for 1280x704”, while the code actually produces
16px size / 8px offset alignment resulting in 1472x800@y=8, and the public header still describes
the keep-aspect path as only “2-pixel alignment”. These mismatches can mislead callers, future
maintainers, and debugging/validation efforts about the true API contract and computed geometry.

Code

src/pipeline_common.c[R220-223]

+	 * imx415 1472x816 -> 1280x720: the 2-px-aligned 1450x816@x=10 crop
+	 * left VENC waiting forever, while the 16/8-aligned 1472x808@y=4
+	 * crop for 1280x704 streamed immediately.  Same constants as the
+	 * stab crop path (star6e_pipeline.c ALIGN / XY_ALIGN). */

Evidence

The added comment describes a specific crop for the 1280x704 case as 1472x808@y=4 and labels it
“16/8-aligned,” but the implementation aligns width/height by masking with & ~15u (multiples of
16) and aligns offsets by masking with & ~7u (multiples of 8), which for that case yields h==800
and y==8 as reflected by the corresponding test expectation. Separately, the public header still
states the keep-aspect precrop uses “2-pixel alignment,” which contradicts the updated 16/8
alignment behavior encoded in the implementation masks.

src/pipeline_common.c[217-234]
tests/test_pipeline_common.c[74-80]
src/pipeline_common.c[203-236]
include/pipeline_common.h[40-47]

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 keep-aspect precrop documentation no longer matches the implementation: a new explanatory comment in `pipeline_common_compute_precrop()` claims an incorrect “16/8-aligned” example crop (`1472x808@y=4` for the 1280x704 case), while the current logic aligns size to 16 and offsets to 8 (producing `1472x800@y=8`), and the public header still documents the keep-aspect path as “2-pixel alignment.” Update these comments so the documented API contract and the example geometry accurately reflect what the code computes.

## Issue Context
This is primarily documentation/comment correctness, but it directly describes the behavior and rationale of the keep-aspect precrop path. Callers rely on `include/pipeline_common.h` for the contract, and maintainers rely on the in-code comment and tests for validating regressions; mismatches here can lead to incorrect assumptions during debugging and validation.

## Fix Focus Areas
- include/pipeline_common.h[40-47]
- src/pipeline_common.c[217-234]
- tests/test_pipeline_common.c[74-80]

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

---

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 ↗

[snokvist]

Adversarial review at bd11baf2 — not merge-ready yet.

The Star6E before/after observation is useful and credible as a regression repro, but the patch and PR text infer a stronger alignment rule than the experiment establishes:

1. The cited known-good control, 1472x808@y=4, violates the claimed 16-pixel size / 8-pixel offset requirement. The failing case changes width and offset at once (1450@x=10 -> 1440@x=16), so it does not isolate which constraint caused the stall or establish that both axes and every stage require 16/8.
2. The implementation aligns offsets to 16, not the stated 8. For 1472x816 -> 1280x704, it returns 1472x800@y=0; with a 16-pixel size and 8-pixel offset rule, the centered result is 1472x800@y=8. The new test pins the off-centre result, contradicting the public center-crop contract.
3. This is implemented in the shared precrop helper, so it also changes Maruko SCL source crops and CV610 VPSS group crops. Only the Star6E VIF/VPE path was hardware-tested, while the Maruko path explicitly documents and device-verifies 2-pixel crop rounding. A Star6E-specific hardware constraint should not silently become a universal backend contract without evidence.

Please scope or parameterize the alignment by backend/stage, preserve the centered-crop invariant, and add tests that distinguish size alignment from offset alignment (including the 1280x704 y-offset case). The hardware test should report the exact programmed rectangle for each candidate and independently vary width/height versus x/y. It is fine for the final fix to retain only the minimal rule demonstrated on Star6E; Maruko/CV610 hardware coverage is needed only if their geometry is intentionally changed.

I reproduced the host gates: make test = 2491/0 and make verify passes. Those do not resolve the hardware-contract issue above.

[snokvist]

Maruko device test report (device observations only)

• PR head tested: bd11baf2a15d20d4f63821bb94bdb967b01f9130
• Built binary SHA-256: 12f2bea45429f6a6ceb333654c26652806b510178f22adf208aae9351f4b82e9
• Device: SSC378QE/Maruko, IMX335. /api/v1/version: app 0.65.3, contract 0.18.2, backend maruko.
• Transport/measurement: H.265 RTP redirected to an otherwise unused host UDP port; rtp_timing_probe subscribed to sidecar :5602. Captures and probe TSVs were stored on the host, not the device.

With isp.keepAspect=true, video0.size=1280x720, CBR 3000 kbps, each IMX335 mode was run at the maximum FPS reported by /api/v1/modes:

mode	capture/max FPS	programmed SCL crop	probe result
0	2592x1944@30	2592x1456 @ 0,240	240 frames / 8 s, 30.1 fps, 0 RTP gaps
1	2496x1872@50	2496x1392 @ 0,240	300 frames / 6 s, 50.2 fps, 0 RTP gaps
2	2272x1704@60	2272x1264 @ 0,208	360 frames / 6 s, 60.2 fps, 0 RTP gaps
3	1792x1344@90	1792x1008 @ 0,160	538 frames / 6 s, 90.0 fps, 0 RTP gaps
4	1920x1080@100	full 1920x1080 @ 0,0	597 frames / 6 s, 99.7 fps, 0 RTP gaps
5	1536x864@144	full 1536x864 @ 0,0	860 frames / 6 s, 143.5 fps, 0 RTP gaps

Additional mode-0 cases:

• 1280x704: SCL crop 2592x1424 @ 0,256; 180 frames / 6 s, 30.2 fps, 0 RTP gaps.
• 1280x712: SCL crop and runtime.active_precrop both reported 2592x1440 @ 0,240; sustained probe received 900 frames / 30 s at 30.0 fps with 0 RTP gaps. A separate off-device elementary-stream capture decoded as H.265 1280x712 with ffprobe.

The kernel log contained no new fault, timeout, watchdog, Oops, panic, MMU, VIF/VPE/VENC error, or FIFO-full match. It did contain the existing JPEG-device warnings dev8 init width/height too small for the 1280x704 and 1280x712 cases; the main H.265 stream continued at the results above.

Not tested here: Star6E, IMX415, visual centering/image-quality comparison, keepAspect=false, or long-duration soak beyond 30 seconds. The device was restored to its original binary and config; both pre-test SHA-256 hashes matched after restoration, and the original stream restarted.

[snokvist]

Targeted follow-up on the alignment question at the current PR head (bd11baf2).

What the device evidence currently establishes:

• On Star6E/IMX415, 1450x816 @ x=10 stalled, while 1440x816 @ x=16 streamed.
• The author's reported 1472x808 @ y=4 also streamed. That geometry is only 8-pixel aligned in height and 4-pixel aligned in offset, so the tests do not establish a universal 16-pixel requirement.
• On Maruko/IMX335, the exact PR branch streamed all six max-FPS modes using the PR's 16-aligned crops. The existing production binary also streams 2592x1458 @ y=242, which is 2-aligned but not 4/8/16-aligned. Thus 16 is tolerated on Maruko, but is not shown to be necessary there.
• CV610 has not been device-tested for this PR.

The narrow conclusion is therefore: 2-pixel alignment is not sufficient for the one tested Star6E horizontal-crop path, and 16-pixel alignment is sufficient for the tested replacement. The minimum constraint, and whether it applies to width, offset, or both, remain unproven.

Could you isolate that with a small one-variable-at-a-time Star6E test on the same failing path (height 816, y=0, output 1280x720)? A useful candidate matrix would be:

1. Hold width at 1440; test x=10, x=12, and x=16 to isolate offset alignment.
2. Hold x=16; test widths 1450, 1448, and 1440 to isolate width alignment.

These can be temporary/manual test overrides; they do not need to be production crop choices. For each candidate, please report the exact programmed rectangle, whether the first encoded access unit arrives within a bounded timeout, and a sustained frame/RTP count over the same short interval.

Once the minimum working constraint is known, the implementation should apply it only to the Star6E VIF/VPE precrop path and select the nearest legal centered rectangle. Maruko should retain its existing 2-pixel behavior, and CV610 should remain unchanged until there is device evidence for a stricter rule. If the immediate fix must remain conservative, 16-pixel alignment on Star6E only is device-verified as sufficient for the tested case, but should be described as a workaround rather than a universal hardware rule.

[snokvist]

Heads-up: #114 (parity sync v0.65.2 → v0.67.1, 54 commits) is open and will conflict with this one on bookkeeping.

This PR claims VERSION 0.65.3, which #114 already uses. Once it lands the next free version is 0.67.2, so this will need its VERSION and HISTORY.md entry renumbered.

The code itself doesn't overlap at all — #114 doesn't touch pipeline_common.{c,h}, so only VERSION/HISTORY need attention.

Sorry for the churn — this bundle had been accumulating on the fork for a while. Happy to help with the rebase if useful.

[snokvist]

#114 is merged, so this now shows as conflicting. The good news: the overlap is only VERSION and HISTORY.md — pipeline_common.{c,h} and your test are untouched by the sync, so the substantive change rebases clean.

After git fetch upstream && git rebase upstream/master, take upstream's side on both files and re-cut: VERSION → 0.67.2, and move your HISTORY.md entry to the top as ## [0.67.2]. Nothing else should need attention.

[vertexodessa]

Closing this one too, for the same reason: I can't commit the bench time to finish it properly, and it should not sit open half-argued.

Your review was right on all three counts, and I want to record that clearly rather than let it lapse quietly.

The alignment rule is inferred, not measured. The failing and working crops differ in width and offset at once (1450 @ x=10 vs 1440 @ x=16), so nothing in my evidence says which one VPE choked on, or that 16 is the minimum rather than merely sufficient. My own second data point argues against the rule as written: 1472x808 @ y=4 streamed fine under the old 2-px code, and that is neither 16-aligned in size nor 8-aligned in offset. So the vertical direction plainly does not need what the patch imposes on it.

The patch also contradicts its own description. The text says offsets align to 8; pipeline_common.c aligns them to 16. That is why 1280x704 comes out at y=0 instead of the centred y=8, and why the test I added pins an off-centre result against a function whose contract promises a centred crop. That part is just wrong and needed fixing regardless of the evidence question.

And it lands in the shared helper, so it silently changes Maruko SCL and CV610 VPSS geometry off a single Star6E observation. Your Maruko pass showed 16 is tolerated there, which is not the same as needed, and CV610 was never tested at all.

What I do think stands, for whoever picks this up: on Star6E with IMX415 at 1472x816, asking for video0.size=1280x720 produces a 2-px-aligned crop of 1450x816 @ x=10, and VENC then never produces a frame — no error from any MI_* call, just "waiting for encoder data" forever. 1280x704 streams. That failure is reproducible on demand, since it is a config rather than a link condition, and the matrix you proposed — vary width and offset one at a time from the failing geometry — would settle the actual constraint in an afternoon on that rig.

Happy for the stall report itself to live on as an issue if that is useful to you; say the word and I will open one. The branch stays up either way.