[libcamera-devel] [PATCH 1/4] media: docs: Describe pixel array properties

[Hans Verkuil]

On 06/08/2020 11:50, Jacopo Mondi wrote:
> Hi Hans,
> 
> On Thu, Aug 06, 2020 at 10:05:37AM +0200, Hans Verkuil wrote:
>> Hi Jacopo,
>>
>> Some review comments below:
>>
>> On 05/08/2020 12:57, Jacopo Mondi wrote:
>>> The V4L2 selection API are also used to access the pixel array
>>
>> are -> is
>>
>>> properties of an image sensor, such as the size and position of active
>>> pixels and the cropped area of the pixel matrix used to produce images.
>>>
>>> Currently no clear definition of the different areas that compose an
>>> image sensor pixel array matrix is provided in the specification, and
>>> the actual meaning of each selection target when applied to an image
>>> sensor was not provided.
>>>
>>> Provide in the sub-device documentation the definition of the pixel
>>> matrix properties and the selection target associated to each of them.
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
>>> ---
>>>  .../userspace-api/media/v4l/dev-subdev.rst    | 81 +++++++++++++++++++
>>>  1 file changed, 81 insertions(+)
>>>
>>> diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst
>>> index 134d2fb909fa4..c47861dff9b9b 100644
>>> --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst
>>> +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst
>>> @@ -386,6 +386,87 @@ requests on all selection targets, unless specifically told otherwise.
>>>  ``V4L2_SEL_FLAG_GE`` and ``V4L2_SEL_FLAG_LE`` flags may be used to round
>>>  the image size either up or down. :ref:`v4l2-selection-flags`
>>>
>>> +.. _v4l2-subdev-pixel-array-properties:
>>> +
>>> +Selection targets for image sensors properties
>>> +----------------------------------------------
>>> +
>>> +The V4L2 selection API can be used on sub-devices that represent an image
>>> +sensor to retrieve the sensor's pixel array matrix properties by using the
>>> +:ref:`selection <VIDIOC_SUBDEV_G_SELECTION>` ioctls.
>>> +
>>> +Sub-device drivers for image sensor usually register a single source pad, but in
>>> +the case they expose more, the pixel array properties can be accessed from
>>> +any of them.
>>> +
>>> +The ``V4L2_SEL_TGT_NATIVE``, ``V4L2_SEL_TGT_CROP_BOUNDS``,
>>
>> V4L2_SEL_TGT_NATIVE -> V4L2_SEL_TGT_NATIVE_SIZE
>>
>> (same mistake is made elsewhere).
> 
> Ah ups, I used TGT_NATIVE consistently, seems like I thought that was
> the right name
> 
>>
>>> +``V4L2_SEL_TGT_CROP_DEFAULT`` and ``V4L2_TGT_CROP`` targets are used to retrieve
>>> +the immutable properties of the several different areas that compose the sensor
>>> +pixel array matrix. Each area describes a rectangle of logically adjacent pixel
>>> +units. The logical disposition of pixels is defined by the sensor read-out
>>> +starting point and direction, and may differ from the physical disposition of
>>> +the pixel units in the pixel array matrix.
>>> +
>>> +Each pixel matrix portion is contained in a larger rectangle, with the most
>>> +largest being the one that describes the pixel matrix physical size. This
>>> +defines a hierarchical positional system, where each rectangle is defined
>>> +relatively to the largest available one among the ones exposed by the
>>> +sub-device driver. Each selection target and the associated pixel array portion
>>> +it represents are below presented in order from the largest to the smallest one.
>>> +
>>> +Pixel array physical size
>>> +^^^^^^^^^^^^^^^^^^^^^^^^^
>>> +
>>> +The image sensor chip is composed by a number of physical pixels, not all of
>>> +them readable by the application processor. Invalid or unreadable lines might
>>> +not be transmitted on the data bus at all, or in case on CSI-2 capable sensors
>>> +they might be tagged with an invalid data type (DT) so that the receiver
>>> +automatically discard them. The size of the whole pixel matrix area is
>>> +retrieved using the V4L2_SEL_TGT_NATIVE target, which has its top-left corner
>>> +defined as position (0, 0). All the other selection targets are defined
>>> +relatively to this, larger, rectangle. The rectangle returned by
>>> +V4L2_SEL_TGT_NATIVE describes an immutable property of the image sensor, it
>>> +does not change at run-time and cannot be modified from userspace.
>>
>> It is a good idea to mention that if there are no invalid or unreadable pixels/lines,
>> then V4L2_SEL_TGT_NATIVE_SIZE == V4L2_SEL_TGT_CROP_BOUNDS.
> 
> Yes it is! I'll add it here
> 
>>
>>> +
>>> +Pixel array readable area
>>> +^^^^^^^^^^^^^^^^^^^^^^^^^
>>> +
>>> +The V4L2_SEL_TGT_CROP_BOUNDS targets returns size and position of the readable
>>> +area of the pixel array matrix, including pixels with valid image data and pixel
>>> +used for calibration purposes, such as optical black pixels. It is not unlikely
>>> +that valid pixels and optical black pixels are surrounded by non-readable rows
>>> +and columns of pixels. Those does not concur in the definition of the
>>> +V4L2_SEL_TGT_CROP_BOUNDS rectangle. The rectangle returned by
>>> +V4L2_SEL_TGT_CROP_BOUNDS describes an immutable property of the image sensor, it
>>> +does not change at run-time and cannot be modified from userspace.
>>
>> Mention that BOUNDS is enclosed by NATIVE_SIZE.
>>
> 
> I tried to express that in the intro section with
> 
> "Each pixel matrix portion is contained in a larger rectangle, with the most
> largest being the one that describes the pixel matrix physical size."
> 
> But I guess it's worth to express that for each target!
> 
>>> +
>>> +Pixel array active area
>>> +^^^^^^^^^^^^^^^^^^^^^^^
>>> +
>>> +The portion of the pixel array which contains valid image data is defined as the
>>> +active area of the pixel matrix. The active pixel array is is accessed by mean
>>
>> mean -> means
>>
>>> +of the V4L2_SEL_TGT_CROP_DEFAULT target, and is contained in the larger
>>> +V4L2_SEL_TGT_CROP_BOUNDS rectangle. It represents the largest possible frame
>>> +resolution the sensor can produce and defines the dimension of the full
>>> +field-of-view. The rectangle returned by V4L2_SEL_TGT_CROP_BOUNDS describes an
>>
>> BOUNDS -> DEFAULT
>>
> 
> ups
> 
>>> +immutable property of the image sensor, it does not change at run-time and
>>> +cannot be modified from userspace.
>>
>> Mention that CROP_DEFAULT is enclosed by CROP_BOUNDS
>>
>>> +
>>> +Analog crop rectangle
>>
>> Why analog? It's just the crop rectangle, nothing analog about it.
>>
> 
> We used the 'analogCrop' term in libcamera to differentiate the
> cropping which happens on the sensor pixel array matrix to select the
> region to process and produce image from. Sensor with an on-board
> scaler can perform other cropping steps to implement, in example digital
> zoom, so we expect to have a 'digital crop' phase as well. RAW
> sensors, in example, will only have an analogCrop rectangle.
> 
> Quoting the libcamera definition of analog crop:
> 
>  * horizontal and vertical sizes define the portion of the pixel array which
>  * is read-out and provided to the sensor's internal processing pipeline, before
>  * any pixel sub-sampling method, such as pixel binning, skipping and averaging
>  * take place.
> 
> should I keep it or remove it ?

It's a very confusing term. Especially since this API can also be used with analog
video capture devices (Composite/S-Video) where the video signal actually is analog.

In the V4L2 API there is no such thing as 'analog crop', so please remove it.

> 
>>> +^^^^^^^^^^^^^^^^^^^^^
>>> +
>>> +The sensor driver might decide, in order to adjust the image resolution to best
>>> +match the one requested by applications, to only process a part of the active
>>> +pixel array matrix. The selected area is read-out and processed by the image
>>> +sensor on-board ISP in order to produce images of the desired size and
>>> +resolution while possible maintaing the largest possible field-of-view. The
>>
>> maintaing -> maintaining
>>
>> Actually, I'd drop 'while possible maintaing the largest possible field-of-view'
>> entirely. It doesn't make much sense.
> 
> Ack
> 
>>
>>> +cropped portion of the pixel array which is used to produce images is returned
>>> +by the V4L2_SEL_TGT_CROP target and represent the only information that can
>>
>> represent -> represents
>>
>>> +change at runtime as it depends on the currently configured sensor mode and
>>> +desired image resolution. If the sub-device driver supports that, userspace
>>> +can set the analog crop rectangle to select which portion of the pixel array
>>
>> s/analog//
>>
>>> +to read out.
>>
>> Mention that CROP is enclosed by CROP_BOUNDS and defaults to CROP_DEFAULT.
>>
>> Make a note that CROP can also be used to obtain optical black pixels.
>>
> 
> What about:
> 
> +desired image resolution. If the sub-device driver supports that, userspace
> +can set the analog crop rectangle to select which portion of the pixel array
> +to read out including, if supported, optical black pixels.

Hmm, that's a bit awkward. How about:

+desired image resolution. If supported by the sub-device driver, userspace
+can set the crop rectangle to select which portion of the pixel array
+to read out. This may include optical black pixels if those are part of
+V4L2_SEL_TGT_CROP_BOUNDS.

Regards,

	Hans

> 
> ?
> 
> Thanks
>   j
> 
>>> +
>>>
>>>  Types of selection targets
>>>  --------------------------
>>>
>>
>> Regards,
>>
>> 	Hans