[libcamera-devel] [PATCH v3 07/13] libcamera: camera_sensor: Define CameraSensorInfo

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Fri, Apr 24, 2020 at 11:52:58PM +0200, Jacopo Mondi wrote:
> Define the CameraSensorInfo structure that reports the current image sensor
> configuration.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/camera_sensor.cpp       | 99 +++++++++++++++++++++++++++
>  src/libcamera/include/camera_sensor.h | 13 ++++
>  2 files changed, 112 insertions(+)
> 
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index a54751fecf5a..e39f277622ae 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -28,6 +28,105 @@ namespace libcamera {
>  
>  LOG_DEFINE_CATEGORY(CameraSensor);
>  
> +/**
> + * \struct CameraSensorInfo
> + * \brief Report the image sensor characteristics
> + *
> + * The structure reports image sensor characteristics used by IPA modules to
> + * tune their algorithms based on the image sensor model currently in use and
> + * its configuration.
> + *
> + * The here reported information describe the sensor's intrinsics
> + * characteristics, such as its pixel array size and the sensor model name,
> + * as well as information relative to the currently configured mode, such as
> + * the produced image size and the bit depth of the requested image format.
> + *
> + * Instances of this structure are meant to be assembled by the CameraSensor
> + * class and its specialized subclasses by inspecting the sensor static

I think we can now drop "and its specialized subclasses".

> + * properties as well as the currently configured sensor mode.
> + */
> +
> +/**
> + * \var CameraSensorInfo::name
> + * \brief The image sensor name

This is a tad ambiguous. We both know that this is the sensor model
name, in practice taken from the entity name with bus information
removed. Let's make it explicit:

 * \brief The image sensor model name
 *
 * The sensor model name is a free-formed string that uniquely identifies the
 * sensor model.
 */

Should we also rename the "name" field to "model" ?

> + */
> +
> +/**
> + * \var CameraSensorInfo::bitsPerPixel
> + * \brief The bits per-pixel of the image format produced by the image sensor

s/bits per-pixel/bits per pixel/ or "number of bits per pixel".

Do we need to also convey the CFA pattern ? I briefly thought we could
use the media bus code to do both, but I think not exposing V4L2
concepts here is a better idea, so bitsPerPixel is fine.

> + */
> +
> +/**
> + * \var CameraSensorInfo::activeAreaSize
> + * \brief The size of the active pixel array area of the sensor

s/active pixel array area/pixel array active area/ to match the term
used below ?

> + * \sa properties::PixelArray
> + */
> +
> +/**
> + * \var CameraSensorInfo::analogCrop
> + * \brief The portion of the pixel array active area which is read-out and
> + * processed
> + *
> + * The analog crop rectangle top-left corner is defined as the displacement
> + * from the top-left corner of the pixel array active area. The rectangle
> + * horizontal and vertical sizes define the portion of the pixel matrix which

s/matrix/array/ ?

> + * is read-out and provided to the sensor's on-board processing pipeline, before

Maybe s/on-board/internal/ as it's more on-chip than on-board ? :-)

> + * any pixel sub-sampling method, such as pixel binning, skipping and averaging
> + * take place.
> + */
> +
> +/**
> + * \var CameraSensorInfo::outputSize
> + * \brief The size of the images produced by the camera sensor
> + *
> + * The output image size defines the horizontal and vertical sizes of the images
> + * produced by the image sensor. The final output image size is defined as the

I would s/final // as otherwise one could wonder if "final output image
size" is different that "output image size".

> + * end result of the sensor's on-board image processing pipeline stages, applied

Same here regarding on-board.

> + * on the pixel array portion defined by the analog crop rectangle. Each image
> + * processing stage that performs pixel sub-sampling techniques, such as pixel
> + * binning or skipping, or perform any additional digital scaling concur in
> + * the definition of the final output image size.
> + */
> +
> +/**
> + * \var CameraSensorInfo::lineLength
> + * \brief Total line length in pixels

s/in pixels/in pixel clock periods/ (and same in the next line) as
technically speaking there's no pixels during the blanking period
(especially on buses such as CSI-2).

> + *
> + * The total line length in pixels, including blanking and synchronism signal

s/synchronism/synchronization/

> + * active state duration.

I would however drop " and synchronization ..." completely. The
synchronization signals are defined by the bus protocol, they're not an
additional part of the line time in addition to the active length and
the blanking. The full line length is the sum of the active and blanking
lengths, there's nothing else.

> + *
> + * In example:

s/In example/For example/ or just s/In example/Example/

> + *
> + * \verbatim
> +                _______     _______     ___
> +  HREF  ... ___|       |___|       |___|     ...
> +              _           _           _
> +  HSYNC ... _| |_________| |_________| |__   ...
> +
> +  Valid ... ....xxxxxxxx....xxxxxxxx....xxx  ...
> +  Data
> +
> +  Line	  ... /-----------/-----------/---  ...
> +  Length
> +
> +  \endverbatim

If we drop the mention of synchronization signals the example may not be
needed anymore.

> + */
> +
> +/**
> + * \var CameraSensorInfo::pixelClock
> + * \brief The pixel clock in Hz
> + *
> + * The pixel read out frequency in Hz. The property describes how many pixel

s/how many pixel/how many pixels/

> + * per second are produced by the camera sensor, including blankings and
> + * synchronism signal active state duration.

Same comment as above.

> + *
> + * To obtain the read-out time in second of a full line:
> + *
> + * \verbatim
> +	LineDuration(s) = lineLength(pixel) / pixelClock(Hz)

s/LineDuration/lineDuration/

> +   \endverbatim
> + */

lineLength and pixelClock should be swapped here, the order in the
structure below is the inverse.

> +
>  /**
>   * \class CameraSensor
>   * \brief A camera sensor based on V4L2 subdevices
> diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h
> index 8fa4d450f959..e19852d4cabe 100644
> --- a/src/libcamera/include/camera_sensor.h
> +++ b/src/libcamera/include/camera_sensor.h
> @@ -22,6 +22,19 @@ class V4L2Subdevice;
>  
>  struct V4L2SubdeviceFormat;
>  
> +struct CameraSensorInfo {
> +	std::string name;
> +
> +	uint32_t bitsPerPixel;
> +
> +	Size activeAreaSize;
> +	Rectangle analogCrop;
> +	Size outputSize;
> +
> +	uint32_t pixelClock;
> +	uint32_t lineLength;
> +};
> +
>  class CameraSensor : protected Loggable
>  {
>  public:

-- 
Regards,

Laurent Pinchart