Re: [PATCH] imx219: selection compliance fixes

[Hans Verkuil <hverkuil@xs4all.nl>]

On 16/07/2020 14:59, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Thu, Jul 16, 2020 at 11:48:19AM +0200, Hans Verkuil wrote:
>> On 15/07/2020 09:19, Jacopo Mondi wrote:
>>> On Wed, Jul 15, 2020 at 02:49:38AM +0300, Laurent Pinchart wrote:
>>>> On Tue, Jul 14, 2020 at 02:31:46PM +0200, Jacopo Mondi wrote:
>>>>> On Thu, Jul 02, 2020 at 03:50:04PM +0200, Hans Verkuil wrote:
>>>>>> The top/left crop coordinates were 0, 0 instead of 8, 8 in the
>>>>>> supported_modes array. This was a mismatch with the default values,
>>>>>> so this is corrected. Found with v4l2-compliance.
>>>>>>
>>>>>> Also add V4L2_SEL_TGT_CROP_BOUNDS support: CROP_DEFAULT and CROP_BOUNDS
>>>>>> always go together. Found with v4l2-compliance.
>>>>>
>>>>> I actually introduced this with
>>>>> e6d4ef7d58aa ("media: i2c: imx219: Implement get_selection")
>>>>>
>>>>>>
>>>>>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>>>>>> ---
>>>>>>  drivers/media/i2c/imx219.c | 17 +++++++++--------
>>>>>>  1 file changed, 9 insertions(+), 8 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
>>>>>> index 0a546b8e466c..935e2a258ce5 100644
>>>>>> --- a/drivers/media/i2c/imx219.c
>>>>>> +++ b/drivers/media/i2c/imx219.c
>>>>>> @@ -473,8 +473,8 @@ static const struct imx219_mode supported_modes[] = {
>>>>>>  		.width = 3280,
>>>>>>  		.height = 2464,
>>>>>>  		.crop = {
>>>>>> -			.left = 0,
>>>>>> -			.top = 0,
>>>>>> +			.left = 8,
>>>>>> +			.top = 8,
>>>>>
>>>>> Mmmm, why this change ?
>>>>> This values are used to report V4L2_SEL_TGT_CROP rectangle, which
>>>>> according to the documentation is defined as
>>>>> "Crop rectangle. Defines the cropped area."
>>>>> (not a much extensive description :)
>>
>> Unless changed by calling S_SELECTION(TGT_CROP) the initial crop is equal
>> to TGT_CROP_DEFAULT, and but TGT_CROP and TGT_CROP_DEFAULT shall be inside
>> TGT_CROP_BOUNDS. CROP_BOUNDS may be larger than CROP_DEFAULT and describes
>> the whole area from which you can crop. I.e. in the case of sensors you can
>> set the crop rectangle to include optical blanks active pixels.
>>
>> In this driver the initial TGT_CROP rectangle as specified in supported_modes
>> (aligned with the top-left pixel) was outside CROP_BOUNDS (centered) and also
>> a mismatch with CROP_DEFAULT (also centered).
>>
>>>>> Clearly this is a faulty definition, and I know from experience how
>>>>> hard is proving to define pixel array properties and in which extent
>>>>> the documentation has to go:
>>>>> https://lists.libcamera.org/pipermail/libcamera-devel/2020-June/009115.html
>>>>>
>>>>> My understanding is that target should report the current crop
>>>>> rectangle, defined from the rectangle retrieved with the
>>>>> V4L2_SEL_TGT_CROP_DEFAULT target, which, according documentation
>>>>> reports the:
>>>>> "Suggested cropping rectangle that covers the “whole picture”.
>>>>> This includes only active pixels and excludes other non-active pixels such
>>>>> as black pixels"
>>>>>
>>>>> The TGT_CROP_DEFAULT then reports the active pixel array portion, and
>>>>> needs to be defined in respect to the TGT_NATIVE_SIZE, which reports
>>>>> the dimensions of the whole pixel matrix, including non-active pixels,
>>>>> optical blanks active and non-active pixels.
>>
>> The relationship between NATIVE_SIZE and CROP_BOUNDS is not properly defined,
>> but I would expect that CROP_BOUNDS is inside the NATIVE_SIZE target rectangle.
>>
>> If NATIVE_SIZE is larger than BOUNDS, then I would expect that the additional
>> margins are pixels that are invalid or otherwise useless.
>>
>> The hard rule though is that you can crop anywhere within the CROP_BOUNDS area.
>>
>> Historically CROP_BOUNDS originated with analog SDTV video capture where it was
>> possible to capture more data than just the typical 720x576/480 PAL/NTSC active
>> video area. Analog video was often overscanned, i.e. there was more video data
>> outside the 'active' video area. That was how CRTs worked. So you could move the
>> crop window around within the CROP_BOUNDS area, or just capture the full CROP_BOUNDS
>> are. Although this was often poorly tested/implemented. The bttv driver is one of
>> the few that could do this.
>>
>> This is actually simplified since you could do weird things with the horizontal
>> sample rate as well, effectively changing the pixel aspect ratio, making things
>> really complicated. It's analog video so while the video lines were discrete,
>> horizontally you are just sampling a waveform, so you could sample at different
>> rates if you wanted to. I doubt anyone ever used it since doing that would give
>> you a huge headache :-)
>>
>> With digital video interfaces (HDMI, DVI, SDI, DP, etc.) that no longer applies and
>> for those receivers the initial CROP/CROP_DEFAULT/CROP_BOUNDS rectangles are all
>> the same, e.g. 1920x1080 for 1080p HDMI video.
>>
>>>>>
>>>>> The TGT_CROP rectangle is hence defined from the CROP_DEFAULT one, and
>>>>> if the 'whole active area' is selected, its top-left corner is placed
>>>>> in position (0, 0) (what's the point of defining it in respect to an
>>>>> area which cannot be read anyway ?)
>>>>>
>>>>> Unless TGT_CROP should be defined in respect to the NATIVE_SIZE
>>>>> rectangle too, but that's not specified anywhere.
>>>>>
>>>>> Anyway, those selection targets badly apply to image sensors, are
>>>>> ill-defined as the definition of active pixels, optical blank (active
>>>>> and non-active) pixels is not provided anywhere, and it's not specified
>>>>> anywhere what is the reference area for each of those rectangles, so I
>>>>> might very well got them wrongly.
>>>>
>>>> My understanding is that both TGT_CROP_DEFAULT and TGT_CROP_BOUNDS are
>>>> relative to TGT_NATIVE_SIZE. BOUNDS defines all the pixels that can be
>>>
>>> And what is TGT_CROP reference in your understanding ?
>>
>> That's the rectangle you are actually cropping. Initially CROP == CROP_DEFAULT
>> and CROP shall always be inside CROP_BOUNDS. And CROP_BOUNDS shall be equal
>> or larger than CROP_DEFAULT.
> 
> I think you've missed the point of Jacopo's question. He wasn't asking
> if CROP needed to be inside CROP_BOUNDS, but what the reference was for
> the left and top coordinates. That is, for all the crop rectangles, what
> is the location of the (0,0) point ? Do they all refer to the same
> location, or are they relative to each other ? This is not defined.

Ah, I misunderstood.

For analog video it was actually undefined. It could be anything, although typically
the initial crop rectangle was at (0, 0). That meant that the larger BOUNDS area
could be at (-8, -8).

For sensors nothing is defined at the moment, but IMHO the largest rectangle
(i.e. TGT_NATIVE_SIZE) should be at (0, 0). I think negative top-left coordinates
are just weird and can potentially cause signedness issues.

In any case, all target rectangles are relative to the same point since you need
to know where the BOUNDS rectangle is within the larger NATIVE_SIZE rectangle
(ugly name BTW), and ditto for CROP/CROP_DEFAULT within the larger CROP_BOUNDS.

Regards,

	Hans

> 
>>>> captured, including optical black and invalid pixels, while DEFAULT
>>>> defines the active area, excluding optical black and invalida pixels. To
>>>> put it another way, DEFAULT is what the kernel recommends applications
>>>> to use if they have no specific requirement and/or no specific knowledge
>>>> about the sensor.
>>>>
>>>> I fully agree this is very under-documented, which also means that my
>>>> understanding may be wrong :-)
>>>
>>> With some consensus on this interpretation I would be happy to update
>>> the documentation. I already considered that, but the selection API
>>> does not apply to image sensors only, and giving a description which
>>> is about the pixel array properties might be not totally opportune as
>>> it would rule out other devices like bridges or muxers.
>>
>> And m2m devices like codecs.
>>
>>>>>>  			.width = 3280,
>>>>>>  			.height = 2464
>>>>>>  		},
>>>>>> @@ -489,8 +489,8 @@ static const struct imx219_mode supported_modes[] = {
>>>>>>  		.width = 1920,
>>>>>>  		.height = 1080,
>>>>>>  		.crop = {
>>>>>> -			.left = 680,
>>>>>> -			.top = 692,
>>>>>> +			.left = 8 + 680,
>>>>>> +			.top = 8 + 692,
>>>>>>  			.width = 1920,
>>>>>>  			.height = 1080
>>>>>>  		},
>>>>>> @@ -505,8 +505,8 @@ static const struct imx219_mode supported_modes[] = {
>>>>>>  		.width = 1640,
>>>>>>  		.height = 1232,
>>>>>>  		.crop = {
>>>>>> -			.left = 0,
>>>>>> -			.top = 0,
>>>>>> +			.left = 8,
>>>>>> +			.top = 8,
>>>>>>  			.width = 3280,
>>>>>>  			.height = 2464
>>>>>>  		},
>>>>>> @@ -521,8 +521,8 @@ static const struct imx219_mode supported_modes[] = {
>>>>>>  		.width = 640,
>>>>>>  		.height = 480,
>>>>>>  		.crop = {
>>>>>> -			.left = 1000,
>>>>>> -			.top = 752,
>>>>>> +			.left = 8 + 1000,
>>>>>> +			.top = 8 + 752,
>>>>>>  			.width = 1280,
>>>>>>  			.height = 960
>>>>>>  		},
>>>>>> @@ -1014,6 +1014,7 @@ static int imx219_get_selection(struct v4l2_subdev *sd,
>>>>>>  		return 0;
>>>>>>
>>>>>>  	case V4L2_SEL_TGT_CROP_DEFAULT:
>>>>>> +	case V4L2_SEL_TGT_CROP_BOUNDS:
>>>>>
>>>>> Still not getting what is the purpose of two targets if the "always
>>>>> have to go together" :)
>>>>>
>>>>>>  		sel->r.top = IMX219_PIXEL_ARRAY_TOP;
>>>>>>  		sel->r.left = IMX219_PIXEL_ARRAY_LEFT;
>>>>>>  		sel->r.width = IMX219_PIXEL_ARRAY_WIDTH;
>