[libcamera-devel] [PATCH v2 1/4] documentation: Introduce Camera Sensor Model

[Kieran Bingham]

Quoting Jacopo Mondi via libcamera-devel (2023-07-31 12:31:12)
> Introduce a documentation page about the 'camera sensor model'
> implemented by libcamera.
> 
> The camera sensor model serves to provide to applications a reference
> description of the processing steps that take place in a camera sensor
> in order to precisely control the sensor configuration through the
> forthcoming SensorConfiguration class.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> Reviewed-by: Naushir Patuck <naush at raspberrypi.com>
> ---
>  Documentation/binning.png             | Bin 0 -> 66004 bytes
>  Documentation/camera-sensor-model.png | Bin 0 -> 74270 bytes
>  Documentation/camera-sensor-model.rst | 200 ++++++++++++++++++++++++++
>  Documentation/index.rst               |   1 +
>  Documentation/meson.build             |   1 +
>  Documentation/skipping.png            | Bin 0 -> 67218 bytes
>  6 files changed, 202 insertions(+)
>  create mode 100644 Documentation/binning.png
>  create mode 100644 Documentation/camera-sensor-model.png
>  create mode 100644 Documentation/camera-sensor-model.rst
>  create mode 100644 Documentation/skipping.png
> 
> diff --git a/Documentation/binning.png b/Documentation/binning.png
> new file mode 100644
> index 0000000000000000000000000000000000000000..b91dd92f76061a20490a66e23affa79948767d51

<snip encoded images>

> 
> literal 0
> HcmV?d00001
> 
> diff --git a/Documentation/camera-sensor-model.rst b/Documentation/camera-sensor-model.rst
> new file mode 100644
> index 000000000000..2a6da54ac91e
> --- /dev/null
> +++ b/Documentation/camera-sensor-model.rst
> @@ -0,0 +1,200 @@
> +.. SPDX-License-Identifier: CC-BY-SA-4.0
> +
> +.. _camera-sensor-model:
> +
> +The libcamera camera sensor model
> +=================================
> +
> +libcamera defines an abstracted camera sensor model in order to provide
> +a description of the several processing steps that result in image data being

'of the several' ?

Perhaps just

- of the processing steps
or
- of each of the processing steps

or if 'several' was really intended to be a limiting statement:

- of some of the processing steps

> +sent on the media bus and that form the image stream delivered to applications.
> +
> +Applications should use the here defined abstracted camera sensor model to
> +precisely control the operations of the camera sensor.


"should use the here defined"

Applications should use the abstracted camera sensor model defined here
to ...

> +
> +The libcamera camera sensor model targets image sensors producing frames in
> +RAW format, delivered through a MIPI CSI-2 compliant bus implementation.
> +
> +The abstract sensor model maps libcamera components to the characteristics and
> +operations of an image sensor, and serves as a reference to model the libcamera
> +CameraSensor class and SensorConfiguration classes and operations.
> +
> +In order to control the configuration of the camera sensor through the
> +SensorConfiguration class, applications should understand the here defined model
> +and map it to the combination of image sensor and kernel driver in use.


applications should understand this model and map ...

> +
> +The here defined camera sensor model is based on the *MIPI CCS

And third use ... "The here defined" isn't something I would write in
English. I'm not sure if this is a representation of a different term or
just something I'm not familiar with - but at three times it wasn't an
accident ;-)


I would put either:

This camera sensor model is based on ...

or

The camera sensor model defined here is based on ...


> +specification*, particularly on *Section 8.2 - Image readout* of
> +*Chapter 8 - Video Timings*.
> +
> +.. image:: camera-sensor-model.png
> +
> +Gloassary

Glossary

> +---------
> +
> +- *Pixel array*: The full grid of pixels, active and inactive ones
> +
> +- *Pixel array active area*: The portion(s) of the pixel array that

Why is this (s) ? Can there be multiple distinct portions?


> +  contains valid and readable pixels; corresponds to the libcamera
> +  properties::PixelArrayActiveAreas
> +
> +- *Analog crop rectangle*: The portion of the *pixel array active area* which
> +  is read-out and passed to further processing stages
> +
> +- *Subsampling*: Pixel processing techniques that reduce the image size by
> +  interpolating (*binning*) or by skipping adjacent pixels
> +
> +- *Digital crop*: Crop of the sub-sampled image data

Should this explicitly state 'before scaling'?

> +
> +- *Digital scaling*: Digital scaling of the image data
> +
> +- *Output crop*: Crop of the scaled image data to form the final output image
> +
> +- *Frame output*: The frame (image) as output on the media bus by the
> +  camera sensor
> +
> +Camera Sensor configuration parameters
> +--------------------------------------
> +
> +The libcamera defined camera sensor model defines parameters that allow users to
> +control:

s/defined// :

"The libcamera camera sensor model defines..."

> +
> +1. The image format bit depth
> +
> +2. The size and position of the  *Analog crop rectangle*
> +
> +3. The subsampling factors used to downscale the pixel array readout data to a
> +   smaller frame size. Two configuration parameters are made available to
> +   control the downscaling factor
> +
> +   - binning
> +      - binning reduces the image resolution by combining adjacent pixels
> +      - reduces the image size without reducing the image *field of view*
> +      - a vertical and horizontal binning factor can be specified, the image
> +        will be downscaled in its vertical and horizontal sizes by the specified
> +        factor
> +
> +      .. figure:: binning.png
> +         :height: 350
> +         :width: 400
> +
> +         Figure 39 from the MIPI CCS Specification (Version 1.1)
> +
> +
> +      .. code-block::
> +         :caption: Definition: The horizontal and vertical binning factors
> +
> +         horizontal_binning = xBin;
> +         vertical_binning = yBin;
> +
> +
> +   - skipping
> +      - reduces the image resolution by skipping the read-out of a
> +        number of adjacent pixels
> +      - the skipping factor is specified by the 'increment' number (number of
> +        pixels to 'skip') in the vertical and horizontal directions and for
> +        even and odd rows and columns


Does this affect the field of view? Binning mentions FoV but skipping
doesn't. That could almost make me imply that skipping does change the
FoV ...

I don't think either skipping nor binning change the FoV as they don't
do a specific crop - so perhaps FoV should be mentioned in the 3.
paragraph that introduces both of these.


> +
> +      .. figure:: skipping.png
> +         :height: 400
> +         :width: 350
> +
> +         Figure 35 from the MIPI CCS Specification (Version 1.1)
> +
> +
> +      .. code-block::
> +         :caption: Definition: The horizontal and vertical skipping factors
> +
> +         horizontal_skipping = (xOddInc + xEvenInc) / 2
> +         vertical_skipping = (yOddInc + yEvenInc) / 2
> +
> +
> +   - binning and skipping can be generically combined
> +   - different sensors perform the binning and skipping stages in different
> +     orders
> +   - for the sake of computing the final output image size the order of
> +     execution is not relevant.
> +
> +   - the overall down-scaling factor is obtained by combining the binning and
> +     skipping factors
> +
> +

Inconsistent new lines above.


> +   .. code-block::
> +      :caption: Definition: The total scaling factor (binning + sub-sampling)
> +
> +      total_horizontal_downscale = horizontal_binning + horizontal_skipping
> +      total_vertical_downscale = vertical_binning + vertical_skipping
> +
> +
> +4. The output data frame size
> +    - the output size is used to specify any additional cropping on the
> +      sub-sampled frame
> +    - \todo Add support for controlling scaling in the digital domain
> +
> +5. The total line length and frame height (*visibile* pixels + *blankings*) as
> +   sent on the MIPI CSI-2 bus
> +
> +6. The pixel transmission rate on the MIPI CSI-2 bus
> +
> +The above parameters are combined to obtain the following high-level
> +configurations

s/configurations/configurations:/  ?

> +
> +- **frame output size**
> +
> +   obtained by applying to the physical pixel array size a first crop in the

Obtained by first applying a crop to the physical pixel array size in
the analog domain, followed by optional binning ...

> +   analog domain, followed by optional binning and sub-sampling (in any order),
> +   followed by an optional crop step in the output digital domain.
> +
> +   \todo Add support for controlling scaling in the digital domain
> +
> +- **frame rate**
> +
> +   the combination of the *total frame size*, the image format *bit depth* and

s/the/The/ - I would capitalise the start of paragraph / descriptions.


> +   the *pixel rate* of the data sent on the MIPI CSI-2 bus allows to compute the
> +   image stream frame rate. The equation is the well known

s/well known/well known:/

> +
> +   .. code-block::
> +
> +      frame_duration = total_frame_size / pixel_rate
> +      frame_rate = 1 / frame_duration
> +
> +
> +   where the *pixel_rate* parameter is the result of the sensor's configuration
> +   of the MIPI CSI-2 bus *(the following formula applies to MIPI CSI-2 when
> +   used on MIPI D-PHY physical protocol layer only)*
> +
> +   .. code-block::
> +
> +      pixel_rate = CSI-2_link_freq * 2 * nr_of_lanes / bits_per_sample
> +
> +
> +The SensorConfiguration class
> +-----------------------------
> +
> +Applications can control the camera sensor configuration by populating the
> +sensorConfig member of the CameraConfiguration class.
> +
> +Camera applications that specify a SensorConfiguration are assumed to be
> +highly-specialized applications that know what sensor they're dealing with and
> +which modes are valid for the sensor in use.


Or also just users that know specifically which mode they want to use to
capture...

It could be a generic application but a specific requirement of the
user. Anyway, I think the above text is fine for now though.


> +
> +\todo The sensorConfig instance should be fully specified in all its fields
> +and it is applied in full to the camera sensor. For now only consider bit-depth

and it is ?

Is it already? or should this todo say "and it should be applied" ?


> +and output size as the kernel interface doesn't allow to fully control the
> +sensor configuration.
> +
> +\todo If any of the parameters the application has populated the sensorConfig
> +with cannot be applied as they are to the camera sensor, the CameraConfiguration
> +is considered to be *Invalid*.

If the application has populated the sensorConfig with any parameter
which can not be applied unchanged on the camera sensor, the
CameraConfiguration is considered Invalid and will be rejected by the
Camera::configure() call.


> +
> +If the application provides a valid SensorConfiguration, its configuration
> +takes precedence over any conflicting StreamConfiguration request.
> +
> +In example:

s/In example:/For example:/

> +
> +- If the platform cannot upscale, all the processed streams should be smaller or
> +  equal in size than the requested sensor configuration

s/than/of/

s/configuration/configuration./


> +
> +- If the platform produces RAW streams using the frames from the camera sensor
> +  without any additional processing, the RAW stream format should be adjusted to
> +  match the one configured by the sensorConfig

s/sensorConfig/sensorConfig./

> diff --git a/Documentation/index.rst b/Documentation/index.rst
> index 43d8b017d3b4..63fac72d11ed 100644
> --- a/Documentation/index.rst
> +++ b/Documentation/index.rst
> @@ -23,3 +23,4 @@
>     Sensor driver requirements <sensor_driver_requirements>
>     Lens driver requirements <lens_driver_requirements>
>     Python Bindings <python-bindings>
> +   Camera Sensor Model <camera-sensor-model>
> diff --git a/Documentation/meson.build b/Documentation/meson.build
> index b2a5bf15e6ea..7c1502592baa 100644
> --- a/Documentation/meson.build
> +++ b/Documentation/meson.build
> @@ -63,6 +63,7 @@ endif
>  
>  if sphinx.found()
>      docs_sources = [
> +        'camera-sensor-model.rst',
>          'coding-style.rst',
>          'conf.py',
>          'contributing.rst',
> diff --git a/Documentation/skipping.png b/Documentation/skipping.png
> new file mode 100644
> index 0000000000000000000000000000000000000000..b5ff2268da01d9508349a1c3faf822554a823da9
> GIT binary patch
> literal 67218


Most of that is small stuff so with those fixed,


Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

> 
> literal 0
> HcmV?d00001
> 
> -- 
> 2.40.1
>