[libcamera-devel,v4,4/7] libcamera: camera_sensor: Define CameraSensorInfo

3573 diff mbox series

Define the CameraSensorInfo structure that reports the current image sensor
configuration.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/libcamera/camera_sensor.cpp       | 84 +++++++++++++++++++++++++++
 src/libcamera/include/camera_sensor.h | 13 +++++
 2 files changed, 97 insertions(+)

Hi Jacopo,

Thanks for your work.

On 2020-04-27 23:32:33 +0200, Jacopo Mondi wrote:
> Define the CameraSensorInfo structure that reports the current image sensor
> configuration.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/libcamera/camera_sensor.cpp       | 84 +++++++++++++++++++++++++++
>  src/libcamera/include/camera_sensor.h | 13 +++++
>  2 files changed, 97 insertions(+)
> 
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index ce585cab1a4f..4fd1ee84eb47 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -28,6 +28,90 @@ 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

s/here//
s/describe/describes/

> + * 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.

I see no reference to the 'mode' in the structure, can this paragraph be 
reworked to not mention it or will it be extended to cover mode later ?

> + *
> + * Instances of this structure are meant to be assembled by the CameraSensor
> + * class by inspecting the sensor static properties as well as the currently
> + * configured sensor mode.
> + */
> +
> +/**
> + * \var CameraSensorInfo::model
> + * \brief The image sensor model name
> + *
> + * The sensor model name is a free-formed string that uniquely identifies the
> + * sensor model.
> + */
> +
> +/**
> + * \var CameraSensorInfo::bitsPerPixel
> + * \brief The number of bits per pixel of the image format produced by the
> + * image sensor
> + */
> +
> +/**
> + * \var CameraSensorInfo::activeAreaSize
> + * \brief The size of the pixel array active area of the sensor
> + */
> +
> +/**
> + * \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 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.
> + */
> +
> +/**
> + * \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 output image size is defined as the end
> + * result of the sensor's internal image processing pipeline stages, applied 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 output image size.
> + */
> +
> +/**
> + * \var CameraSensorInfo::pixelRate
> + * \brief The number of pixel produced in a second
> + *
> + * The pixel read out frequency in Hz. The property describes how many pixels
> + * per second are produced by the camera sensor, including blanking.
> + *
> + * To obtain the read-out time in second of a full line:
> + *
> + * \verbatim
> +	lineDuration(s) = lineLength(pixel) / pixelRate(Hz)
> +   \endverbatim
> + */
> +
> +/**
> + * \var CameraSensorInfo::lineLength
> + * \brief Total line length in pixels
> + *
> + * The total line length in pixel clock periods, including blanking.
> + */
> +
>  /**
>   * \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 5277f7f7fe87..28ebfaa30c22 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 model;

I'm hearing whispers we can drop the model field which would make me 
happy :-)

> +
> +	uint32_t bitsPerPixel;
> +
> +	Size activeAreaSize;
> +	Rectangle analogCrop;
> +	Size outputSize;
> +
> +	uint32_t pixelRate;
> +	uint32_t lineLength;
> +};
> +
>  class CameraSensor : protected Loggable
>  {
>  public:
> -- 
> 2.26.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Jacopo and Niklas,

On Tue, Apr 28, 2020 at 02:00:05AM +0200, Niklas Söderlund wrote:
> On 2020-04-27 23:32:33 +0200, Jacopo Mondi wrote:
> > Define the CameraSensorInfo structure that reports the current image sensor
> > configuration.
> > 
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  src/libcamera/camera_sensor.cpp       | 84 +++++++++++++++++++++++++++
> >  src/libcamera/include/camera_sensor.h | 13 +++++
> >  2 files changed, 97 insertions(+)
> > 
> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > index ce585cab1a4f..4fd1ee84eb47 100644
> > --- a/src/libcamera/camera_sensor.cpp
> > +++ b/src/libcamera/camera_sensor.cpp
> > @@ -28,6 +28,90 @@ 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
> 
> s/here//
> s/describe/describes/
> 
> > + * 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.
> 
> I see no reference to the 'mode' in the structure, can this paragraph be 
> reworked to not mention it or will it be extended to cover mode later ?

It already covers the mode, which consists on the active configuration
of the sensor's internal pipeline (formats and selection rectangles). I
think sensor mode is a well known term, but there would also be pedantic
arguments to not use it, as not all drivers are mode-based (but it could
also be argued that they configure a mode in any case). I'll let Jacopo
decide if he prefers to keep the text as is or write something along the
lines of "as well as information relative to the current configuration".

> > + *
> > + * Instances of this structure are meant to be assembled by the CameraSensor
> > + * class by inspecting the sensor static properties as well as the currently
> > + * configured sensor mode.
> > + */
> > +
> > +/**
> > + * \var CameraSensorInfo::model
> > + * \brief The image sensor model name
> > + *
> > + * The sensor model name is a free-formed string that uniquely identifies the
> > + * sensor model.
> > + */
> > +
> > +/**
> > + * \var CameraSensorInfo::bitsPerPixel
> > + * \brief The number of bits per pixel of the image format produced by the
> > + * image sensor
> > + */
> > +
> > +/**
> > + * \var CameraSensorInfo::activeAreaSize
> > + * \brief The size of the pixel array active area of the sensor
> > + */
> > +
> > +/**
> > + * \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 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.
> > + */
> > +
> > +/**
> > + * \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 output image size is defined as the end
> > + * result of the sensor's internal image processing pipeline stages, applied 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 output image size.
> > + */
> > +
> > +/**
> > + * \var CameraSensorInfo::pixelRate
> > + * \brief The number of pixel produced in a second

s/pixel/pixels/

> > + *
> > + * The pixel read out frequency in Hz. The property describes how many pixels

Technically speaking the unit is pixels per second, not Hz, but I'll
pretend not to know :-) If you want to fix it, I'd just drop the first
sentence as it would duplicate the second one with Hz replaced with
pixels per second.

> > + * per second are produced by the camera sensor, including blanking.
> > + *
> > + * To obtain the read-out time in second of a full line:

s/second/seconds/

> > + *
> > + * \verbatim
> > +	lineDuration(s) = lineLength(pixel) / pixelRate(Hz)

And here that would be s/Hz/pixels per second/

> > +   \endverbatim
> > + */
> > +
> > +/**
> > + * \var CameraSensorInfo::lineLength
> > + * \brief Total line length in pixels
> > + *
> > + * The total line length in pixel clock periods, including blanking.
> > + */
> > +
> >  /**
> >   * \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 5277f7f7fe87..28ebfaa30c22 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 model;
> 
> I'm hearing whispers we can drop the model field which would make me 
> happy :-)

I thought we could, but then realized we still need it at least to
select the right embedded data parser. We may be able to revisit that
later, but it will take time.

Great work Jacopo !

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> > +
> > +	uint32_t bitsPerPixel;
> > +
> > +	Size activeAreaSize;
> > +	Rectangle analogCrop;
> > +	Size outputSize;
> > +
> > +	uint32_t pixelRate;
> > +	uint32_t lineLength;
> > +};
> > +
> >  class CameraSensor : protected Loggable
> >  {
> >  public:

HI Laurent, Niklas,

On Tue, Apr 28, 2020 at 04:50:15AM +0300, Laurent Pinchart wrote:
> Hi Jacopo and Niklas,
>
> On Tue, Apr 28, 2020 at 02:00:05AM +0200, Niklas Söderlund wrote:
> > On 2020-04-27 23:32:33 +0200, Jacopo Mondi wrote:
> > > Define the CameraSensorInfo structure that reports the current image sensor
> > > configuration.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > >  src/libcamera/camera_sensor.cpp       | 84 +++++++++++++++++++++++++++
> > >  src/libcamera/include/camera_sensor.h | 13 +++++
> > >  2 files changed, 97 insertions(+)
> > >
> > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > > index ce585cab1a4f..4fd1ee84eb47 100644
> > > --- a/src/libcamera/camera_sensor.cpp
> > > +++ b/src/libcamera/camera_sensor.cpp
> > > @@ -28,6 +28,90 @@ 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
> >
> > s/here//
> > s/describe/describes/
> >
> > > + * 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.
> >
> > I see no reference to the 'mode' in the structure, can this paragraph be
> > reworked to not mention it or will it be extended to cover mode later ?
>
> It already covers the mode, which consists on the active configuration
> of the sensor's internal pipeline (formats and selection rectangles). I
> think sensor mode is a well known term, but there would also be pedantic
> arguments to not use it, as not all drivers are mode-based (but it could
> also be argued that they configure a mode in any case). I'll let Jacopo
> decide if he prefers to keep the text as is or write something along the
> lines of "as well as information relative to the current configuration".

I'll drop mode to please pedantic reviewers if it's confusing, but to
me mode is not even the 'mode based sensor driver' Laurent mentioned,
it's just the sensor configuration :/

>
> > > + *
> > > + * Instances of this structure are meant to be assembled by the CameraSensor
> > > + * class by inspecting the sensor static properties as well as the currently
> > > + * configured sensor mode.
> > > + */
> > > +
> > > +/**
> > > + * \var CameraSensorInfo::model
> > > + * \brief The image sensor model name
> > > + *
> > > + * The sensor model name is a free-formed string that uniquely identifies the
> > > + * sensor model.
> > > + */
> > > +
> > > +/**
> > > + * \var CameraSensorInfo::bitsPerPixel
> > > + * \brief The number of bits per pixel of the image format produced by the
> > > + * image sensor
> > > + */
> > > +
> > > +/**
> > > + * \var CameraSensorInfo::activeAreaSize
> > > + * \brief The size of the pixel array active area of the sensor
> > > + */
> > > +
> > > +/**
> > > + * \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 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.
> > > + */
> > > +
> > > +/**
> > > + * \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 output image size is defined as the end
> > > + * result of the sensor's internal image processing pipeline stages, applied 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 output image size.
> > > + */
> > > +
> > > +/**
> > > + * \var CameraSensorInfo::pixelRate
> > > + * \brief The number of pixel produced in a second
>
> s/pixel/pixels/
>
> > > + *
> > > + * The pixel read out frequency in Hz. The property describes how many pixels
>
> Technically speaking the unit is pixels per second, not Hz, but I'll
> pretend not to know :-) If you want to fix it, I'd just drop the first
> sentence as it would duplicate the second one with Hz replaced with
> pixels per second.
>

That's why I kept Hz, to be able to provide a bit more of
documentation, but I get it can be safely removed...

> > > + * per second are produced by the camera sensor, including blanking.
> > > + *
> > > + * To obtain the read-out time in second of a full line:
>
> s/second/seconds/
>
> > > + *
> > > + * \verbatim
> > > +	lineDuration(s) = lineLength(pixel) / pixelRate(Hz)
>
> And here that would be s/Hz/pixels per second/
>
> > > +   \endverbatim
> > > + */
> > > +
> > > +/**
> > > + * \var CameraSensorInfo::lineLength
> > > + * \brief Total line length in pixels
> > > + *
> > > + * The total line length in pixel clock periods, including blanking.
> > > + */
> > > +
> > >  /**
> > >   * \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 5277f7f7fe87..28ebfaa30c22 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 model;
> >
> > I'm hearing whispers we can drop the model field which would make me
> > happy :-)
>
> I thought we could, but then realized we still need it at least to
> select the right embedded data parser. We may be able to revisit that
> later, but it will take time.
>

Yeah, it's easy to drop if we have to

Thanks
  j

> Great work Jacopo !
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> > > +
> > > +	uint32_t bitsPerPixel;
> > > +
> > > +	Size activeAreaSize;
> > > +	Rectangle analogCrop;
> > > +	Size outputSize;
> > > +
> > > +	uint32_t pixelRate;
> > > +	uint32_t lineLength;
> > > +};
> > > +
> > >  class CameraSensor : protected Loggable
> > >  {
> > >  public:
>
> --
> Regards,
>
> Laurent Pinchart