Report the frame sizes the driver actually accepts (merge after #329 and #330) - #331
Open
vrilutza wants to merge 1 commit into
Open
Report the frame sizes the driver actually accepts (merge after #329 and #330)#331vrilutza wants to merge 1 commit into
vrilutza wants to merge 1 commit into
Conversation
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>
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
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.
VIDIOC_ENUM_FRAMESIZESadvertises a single discrete size whilefthd_v4l2_adjust_format()clamps toFTHD_MIN_WIDTH/HEIGHTat the bottom and to the detected sensor size at the top, with the scaler covering everything in between.VIDIOC_ENUM_FRAMEINTERVALSalready 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. That is what #323 describes — "the driver doesn't expose the camera in ways that an app can adjust resolution" — and what #243 is looking at when it prints
Size: Discrete 1280x720. In #52 the reporter had to hand-edit Skype'sshared.xmlto force 640x480 and then 320x240 before the camera would work at all, and #36 is someone asking how to force a lower resolution; neither needed a driver change to capture at the smaller sizes, only a way to find out they exist.This reports a stepwise range matching what
adjust_format()does, keeping the sensor detection from 98b55fd as the upper bound:FTHD_MIN_WIDTHto the sensor width in steps of 8,FTHD_MIN_HEIGHTto the sensor height in steps of 1. The horizontal step of 8 is the constraintenum_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.Measured
MacBookPro14,1, where
CISP_CMD_CH_CAMERA_CONFIG_GETreports a 1296x736 sensor:and
v4l2-compliance1.32.0 goes fromtest Scaling: FAILtotest Scaling: OK.Why it goes last
ALIGN(pix->width, 7)fix in Two user-visible fixes: controls discarded at every STREAMON, width aligned to 7 #329. Without it the driver advertises a step of 8 thatadjust_format()does not honour, soTRY_FMTwould keep handing out widths this enumeration says are invalid.Taken in the order #329 → #330 → this, each step is an improvement on its own.