isp: assign y1 in the crop command - #330
Merged
Merged
Conversation
fthd_isp_cmd_channel_crop_set() fills the command with cmd.x1 = x1; cmd.y2 = y2; cmd.x2 = x2; cmd.y2 = y2; so cmd.y1 keeps the zero left by the memset and the y1 argument is dropped, while y2 is written twice. The single caller passes y1 = 0, which is why nothing misbehaves today, but the parameter cannot be used at all until this is fixed. Signed-off-by: Viorel Cernateanu <vrilutza@gmail.com>
vrilutza
added a commit
to vrilutza/facetimehd
that referenced
this pull request
Aug 22, 2026
VIDIOC_ENUM_FRAMESIZES advertises a single discrete size while fthd_v4l2_adjust_format() clamps to FTHD_MIN_WIDTH/HEIGHT at the bottom and to the detected sensor size at the top, with the scaler covering everything between. VIDIOC_ENUM_FRAMEINTERVALS already accepts any width that is a multiple of eight up to the maximum, so the enumeration is the only place claiming the device does one size and nothing else. Applications that pick a resolution from the enumeration therefore never offer anything below the sensor's native size, even though the hardware scales down. In issue patjak#52 the reporter had to hand-edit Skype's shared.xml to force 640x480 and then 320x240 before the camera would work at all, and issue patjak#243 is a user asking why the enumeration shows one size; neither needed a driver change to capture at the smaller sizes, only a way to find out they exist. Report a stepwise range matching what adjust_format() does, keeping the sensor detection from 98b55fd as the upper bound: FTHD_MIN_WIDTH to the sensor width in steps of 8, FTHD_MIN_HEIGHT to the sensor height in steps of 1. The horizontal step of 8 is the constraint enum_frameintervals() has always enforced. Nothing enforces one vertically, and heights of 241, 245 and 481 were verified to capture correctly, so the vertical step is 1. Tested on a MacBookPro14,1, where CISP_CMD_CH_CAMERA_CONFIG_GET reports a 1296x736 sensor, so the upper bound comes from the detection rather than from FTHD_MAX_WIDTH/HEIGHT: [0]: 'YUYV' (YUYV 4:2:2) Size: Stepwise 320x240 - 1296x736 with step 8/1 and it takes v4l2-compliance 1.32.0 from "test Scaling: FAIL" to "test Scaling: OK". Depends on "v4l2: align the width to 8, not to 7" (patjak#329); without it the driver would advertise a step of 8 that adjust_format() does not honour. It should also not go in ahead of "isp: crop a centred window with the aspect ratio of the output" (patjak#330). Until that one is in, fthd_start_channel() asks the ISP for a crop window the size of the output placed at the sensor origin, so every size below the sensor comes back as the top left corner of the frame. Advertising the range first would hand applications exactly the resolutions that are framed wrong. Signed-off-by: Viorel Cernateanu <vrilutza@gmail.com> (cherry picked from commit 9992af1)
vrilutza
changed the title
This was referenced Aug 22, 2026
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
2 participants
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.
fthd_isp_cmd_channel_crop_set()writescmd.y2twice and never writescmd.y1, so they1argument is silently dropped:One line. Latent today because the only caller passes 0, but the parameter cannot be used until this is fixed — which is why it comes first, ahead of the crop rewrite that needs it.
The series
Seven PRs on this driver, all on master (
364b1c6), all found while debugging a camera problem on a MacBookPro14,1:break, hardcoded buffer countVIDIOC_STREAMONcmd.y1never assigned in the crop command — one lineALIGN(width, 7)aligns nothing; second commit re-does 545cb18 and should waitThey can be taken in any order, with one exception: the second commit of #331 should not be taken yet — reasons in that PR.
(Until 23 August there was a second exception: #328 and #334 both carried the same
mdelay→msleepcommit, so whichever merged first made the other conflict. I have dropped it from #328; it lives in #334. The seven are now independent of each other.)There is one more patch not sent yet: the channel-start crop returns the top left corner of the frame at any size below the sensor. It is measured and ready, and it sits on top of #330. I am holding it back rather than adding an eighth PR to the pile.