isp: crop a centred window with the output's aspect ratio - #338
Open
vrilutza wants to merge 2 commits into
Open
Conversation
CISP_CMD_CH_CROP_SET does not take two corners: the third and fourth values are the window's width and height, not the coordinates of its far edge, and the first two are the offset. Rename the fields and the function's parameters to match; the only existing caller (0, 0, width, height) is unaffected. No functional change. Signed-off-by: Viorel Cernateanu <vrilutza@gmail.com>
fthd_start_channel() cropped a window the size of the negotiated output, at the sensor origin, then asked for an output of that same size -- leaving nothing to scale, so any size below the sensor came back as the top-left corner of the frame. Crop the largest centred window that has the output's aspect ratio instead, and let the ISP scale it down. The window and its offset are derived from the sensor's own reported size, so neither can exceed it -- unlike the old hardcoded 1280x720, which overflows the 12-inch MacBook's 848x588 sensor and throws SIF errors. The masks are not cosmetic. Measured on a MacBookPro14,1 by driving the crop command directly: the ISP stops delivering frames, and the camera stays dead until the module is reloaded, when x and the width are both odd -- at every scale factor tried, 1.0000 through 1.5333 -- and also when x alone is odd and the crop has the same width as the output, so the scaler is bypassed. An odd y or an odd height is harmless here, but is masked too rather than relying on one machine. CISP_CMD_CH_CROP_SET returns 0 in all of those cases, so there is nothing to test for at runtime. Signed-off-by: Viorel Cernateanu <vrilutza@gmail.com>
vrilutza
force-pushed
the
crop-window
branch
from
September 4, 2026 12:59
9bc5e39 to
d0b9709
Compare
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.
Sits on top of #330 (merged) — the patch mentioned there as not sent yet.
Commit 1 renames the crop command's fields to match what they hold. Commit 2 is the actual fix:
fthd_start_channel()cropped a window the size of the output, leaving nothing for the ISP to scale, so any size below the sensor came back as the top-left corner of the frame.Verified on a MacBookPro14,1 by recovering the actual crop window from captured frames.
On the
& ~1umasks. Measured on a MacBookPro14,1 by driving the crop command directly: the ISP stops delivering frames, and the camera stays dead until the module is reloaded, whenxand the width are both odd — at every scale factor tried, 1.0000 through 1.5333 — and also whenxalone is odd and the crop has the same width as the output, so the scaler is bypassed. An oddyor an odd height is harmless on this machine, but is masked too rather than relying on one camera.CISP_CMD_CH_CROP_SETreturns 0 in all of those cases, so there is nothing to test for at runtime.