[libcamera-devel] [PATCH v2 1/4] documentation: Introduce Camera Sensor Model
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Fri Sep 15 09:57:30 CEST 2023
Hi Kieran,
thanks for the review
On Thu, Sep 14, 2023 at 11:13:08PM +0100, Kieran Bingham via libcamera-devel wrote:
> 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 ;-)
>
No, it wasn't an accident, I thought it made sense in English :)
>
> 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?
>
Yes, see properties::PixelArrayActiveAreas
>
> > + 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'?
>
Done
> > +
> > +- *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.
>
Good point, moved to 3.
>
> > +
> > + .. 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.
>
Yes, good spot. I can remove an empty line here but I still need two
empty lines after a code-block:: or a figure::
>
> > + .. 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:/
>
I recall I had troubles with ':' at the end of paragraphs. Seems I
don't anymore...
> > +
> > + .. 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" ?
>
Not sure I got your comment. It just "shouldn't", it actually gets
applied. But maybe your comment it's because "and it is" is not
correct English, so I'll fix this anyhow
>
> > +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>
Thanks!
j
>
> >
> > literal 0
> > HcmV?d00001
> >
> > --
> > 2.40.1
> >
More information about the libcamera-devel
mailing list