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.