[libcamera-devel,09/11] libcamera: camera_sensor: Add a new class to model a camera sensor

990 diff mbox series

The CameraSensor class abstracts camera sensors and provides helper
functions to ease interactions with them. It is currently limited to
sensors that expose a single subdev, and offer the same frame sizes for
all media bus codes, but will be extended to support more complex
sensors as the needs arise.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/libcamera/camera_sensor.cpp       | 245 ++++++++++++++++++++++++++
 src/libcamera/include/camera_sensor.h |  56 ++++++
 src/libcamera/meson.build             |   2 +
 3 files changed, 303 insertions(+)
 create mode 100644 src/libcamera/camera_sensor.cpp
 create mode 100644 src/libcamera/include/camera_sensor.h

Hi Laurent,

Thanks for your work.

On 2019-04-15 19:56:58 +0300, Laurent Pinchart wrote:
> The CameraSensor class abstracts camera sensors and provides helper
> functions to ease interactions with them. It is currently limited to
> sensors that expose a single subdev, and offer the same frame sizes for
> all media bus codes, but will be extended to support more complex
> sensors as the needs arise.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/libcamera/camera_sensor.cpp       | 245 ++++++++++++++++++++++++++
>  src/libcamera/include/camera_sensor.h |  56 ++++++
>  src/libcamera/meson.build             |   2 +
>  3 files changed, 303 insertions(+)
>  create mode 100644 src/libcamera/camera_sensor.cpp
>  create mode 100644 src/libcamera/include/camera_sensor.h
> 
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> new file mode 100644
> index 000000000000..aca9e77fd986
> --- /dev/null
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -0,0 +1,245 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * camera_sensor.cpp - A camera sensor
> + */
> +
> +#include <algorithm>
> +#include <float.h>
> +#include <limits.h>
> +#include <math.h>
> +
> +#include "camera_sensor.h"
> +#include "formats.h"
> +#include "v4l2_subdevice.h"
> +
> +/**
> + * \file camera_sensor.h
> + * \brief A camera sensor
> + */
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(CameraSensor);
> +
> +/**
> + * \class CameraSensor
> + * \brief A camera sensor
> + *
> + * The CameraSensor class eases handling of sensors for pipeline handlers by
> + * hiding the details of the V4L2 subdevice kernel API and caching sensor
> + * information.
> + *
> + * The implementation is currently limited to sensors that expose a single V4L2
> + * subdevice with a single pad, and support the same frame sizes for all
> + * supported media bus codes. It will be extended to support more complex
> + * devices as the needs arise.
> + */
> +
> +/**
> + * \brief Construct a CameraSensor
> + * \param[in] entity The media entity for the camera sensor
> + *
> + * Once constructed the instance must be initialized with init().
> + */
> +CameraSensor::CameraSensor(const MediaEntity *entity)
> +	: entity_(entity)
> +{
> +	subdev_ = new V4L2Subdevice(entity);
> +}
> +
> +/**
> + * \brief Destroy a CameraSensor
> + */
> +CameraSensor::~CameraSensor()
> +{
> +	delete subdev_;
> +}
> +
> +/**
> + * \brief Initialize the camera sensor instance
> + *
> + * This methods perform the initialisation steps of the CameraSensor that may
> + * fail. It shall be called once and only once after constructing the instance.
> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +int CameraSensor::init()
> +{
> +	int ret;
> +
> +	if (entity_->pads().size() != 1) {
> +		LOG(CameraSensor, Error)
> +			<< "Sensors with more than one pad are not supported"
> +			<< std::endl;
> +		return -EINVAL;
> +	}
> +
> +	ret = subdev_->open();
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Enumerate and cache media bus codes and sizes. */
> +	const FormatEnum formats = subdev_->formats(0);
> +	if (formats.empty()) {
> +		LOG(CameraSensor, Error)
> +			<< "No image format found" << std::endl;
> +		return -EINVAL;
> +	}
> +
> +	std::transform(formats.begin(), formats.end(),
> +		       std::back_inserter(mbusCodes_),
> +		       [](decltype(*formats.begin()) f) { return f.first; });
> +
> +	const std::vector<SizeRange> &sizes = formats.begin()->second;

I might add a comment somewhere around here mentioning the limitation 
and why only sizes from formats.begin() are considered. I had to reread 
the commit message to figure it out. Not a big issue as it's documented 
elsewhere so it won't be forgotten. Maybe me pointing it out here for 
other reviewers benefit is enough.

> +	std::transform(sizes.begin(), sizes.end(), std::back_inserter(sizes_),
> +		       [](const SizeRange &range) { return range.max; });
> +
> +	/*
> +	 * Verify the assumption that all available media bus codes support the
> +	 * same frame sizes.
> +	 */
> +	for (auto it = ++formats.begin(); it != formats.end(); ++it) {
> +		if (it->second != sizes) {
> +			LOG(CameraSensor, Error)
> +				<< "Frame sizes differ between media bus codes"
> +				<< std::endl;
> +			return -EINVAL;
> +		}
> +	}
> +
> +	/* Sort the sizes. */
> +	std::sort(sizes_.begin(), sizes_.end());
> +
> +	return 0;
> +}
> +
> +/**
> + * \fn CameraSensor::entity()
> + * \brief Retrieve the sensor media entity
> + * \return The sensor media entity
> + */
> +
> +/**
> + * \fn CameraSensor::mbusCodes()
> + * \brief Retrieve the media bus codes supported by the camera sensor
> + * \return The supported media bus codes
> + */
> +
> +/**
> + * \fn CameraSensor::sizes()
> + * \brief Retrieve the frame sizes supported by the camera sensor
> + * \return The supported frame sizes
> + */
> +
> +/**
> + * \fn CameraSensor::resolution()
> + * \brief Retrieve the camera sensor resolution
> + * \return The camera sensor resolution in pixels
> + */

Maybe move the function implementation here adding a comment that the 
sizes_ vector is sorted so that the resolution will always be the 
largest one supported.

With or without the above nit-pick comments addressed,

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> +
> +/**
> + * \brief Retrieve the best sensor format for a desired output
> + * \param[in] mbusCodes The list of acceptable media bus codes
> + * \param[in] size The desired size
> + *
> + * Media bus codes are selected from \a mbusCodes, which lists all acceptable
> + * codes in decreasing order of preference. This method selects the first code
> + * from the list that is supported by the sensor. If none of the desired codes
> + * is supported, it returns an error.
> + *
> + * \a size indicates the desired size at the output of the sensor. This method
> + * selects the best size supported by the sensor according to the following
> + * criteria.
> + *
> + * - The desired \a size shall fit in the sensor output size to avoid the need
> + *   to up-scale.
> + * - The sensor output size shall match the desired aspect ratio to avoid the
> + *   need to crop the field of view.
> + * - The sensor output size shall be as small as possible to lower the required
> + *   bandwidth.
> + *
> + * The use of this method is optional, as the above criteria may not match the
> + * needs of all pipeline handlers. Pipeline handlers may implement custom
> + * sensor format selection when needed.
> + *
> + * The returned sensor output format is guaranteed to be acceptable by the
> + * setFormat() method without any modification.
> + *
> + * \return The best sensor output format matching the desired media bus codes
> + * and size on success, or an empty format otherwise.
> + */
> +V4L2SubdeviceFormat CameraSensor::getFormat(const std::vector<unsigned int> &mbusCodes,
> +					    const Size &size) const
> +{
> +	V4L2SubdeviceFormat format{};
> +
> +	for (unsigned int code : mbusCodes_) {
> +		if (std::any_of(mbusCodes.begin(), mbusCodes.end(),
> +				[code](unsigned int c) { return c == code; })) {
> +			format.mbus_code = code;
> +			break;
> +		}
> +	}
> +
> +	if (!format.mbus_code) {
> +		LOG(CameraSensor, Debug)
> +			<< "No supported format found" << std::endl;
> +		return format;
> +	}
> +
> +	unsigned int desiredArea = size.width * size.height;
> +	unsigned int bestArea = UINT_MAX;
> +	float desiredRatio = static_cast<float>(size.width) / size.height;
> +	float bestRatio = FLT_MAX;
> +	const Size *bestSize = nullptr;
> +
> +	for (const Size &sz : sizes_) {
> +		if (sz.width < size.width || sz.height < size.height)
> +			continue;
> +
> +		float ratio = static_cast<float>(sz.width) / sz.height;
> +		float ratioDiff = fabsf(ratio - desiredRatio);
> +		unsigned int area = sz.width * sz.height;
> +		unsigned int areaDiff = area - desiredArea;
> +
> +		if (ratioDiff > bestRatio)
> +			continue;
> +
> +		if (ratioDiff < bestRatio || areaDiff < bestArea) {
> +			bestRatio = ratioDiff;
> +			bestArea = areaDiff;
> +			bestSize = &sz;
> +		}
> +	}
> +
> +	if (!bestSize) {
> +		LOG(CameraSensor, Debug)
> +			<< "No supported size found" << std::endl;
> +		return format;
> +	}
> +
> +	format.width = bestSize->width;
> +	format.height = bestSize->height;
> +
> +	return format;
> +}
> +
> +/**
> + * \brief Set the sensor output format
> + * \param[in] format The desired sensor output format
> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +int CameraSensor::setFormat(V4L2SubdeviceFormat *format)
> +{
> +	return subdev_->setFormat(0, format);
> +}
> +
> +std::string CameraSensor::logPrefix() const
> +{
> +	return "'" + subdev_->entity()->name() + "'";
> +}
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h
> new file mode 100644
> index 000000000000..7d92af05248c
> --- /dev/null
> +++ b/src/libcamera/include/camera_sensor.h
> @@ -0,0 +1,56 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * camera_sensor.h - A camera sensor
> + */
> +#ifndef __LIBCAMERA_CAMERA_SENSOR_H__
> +#define __LIBCAMERA_CAMERA_SENSOR_H__
> +
> +#include <string>
> +#include <vector>
> +
> +#include <libcamera/geometry.h>
> +
> +#include "log.h"
> +#include "v4l2_subdevice.h"
> +
> +namespace libcamera {
> +
> +class MediaEntity;
> +class V4L2SubdeviceFormat;
> +
> +class CameraSensor : public Loggable
> +{
> +public:
> +	explicit CameraSensor(const MediaEntity *entity);
> +	~CameraSensor();
> +
> +	CameraSensor(const CameraSensor &) = delete;
> +	CameraSensor &operator=(const CameraSensor &) = delete;
> +
> +	int init();
> +
> +	const MediaEntity *entity() const { return entity_; }
> +	const std::vector<unsigned int> &mbusCodes() const { return mbusCodes_; }
> +	const std::vector<Size> &sizes() const { return sizes_; }
> +	const Size &resolution() const { return sizes_.back(); }
> +
> +	V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes,
> +				      const Size &size) const;
> +	int setFormat(V4L2SubdeviceFormat *format);
> +
> +protected:
> +	std::string logPrefix() const;
> +
> +private:
> +	const MediaEntity *entity_;
> +	V4L2Subdevice *subdev_;
> +
> +	std::vector<unsigned int> mbusCodes_;
> +	std::vector<Size> sizes_;
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_CAMERA_SENSOR_H__ */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index cd36ac307518..cf4edec05755 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -2,6 +2,7 @@ libcamera_sources = files([
>      'buffer.cpp',
>      'camera.cpp',
>      'camera_manager.cpp',
> +    'camera_sensor.cpp',
>      'device_enumerator.cpp',
>      'event_dispatcher.cpp',
>      'event_dispatcher_poll.cpp',
> @@ -23,6 +24,7 @@ libcamera_sources = files([
>  ])
>  
>  libcamera_headers = files([
> +    'include/camera_sensor.h',
>      'include/device_enumerator.h',
>      'include/event_dispatcher_poll.h',
>      'include/formats.h',
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Niklas,

On Mon, Apr 15, 2019 at 11:39:29PM +0200, Niklas Söderlund wrote:
> On 2019-04-15 19:56:58 +0300, Laurent Pinchart wrote:
> > The CameraSensor class abstracts camera sensors and provides helper
> > functions to ease interactions with them. It is currently limited to
> > sensors that expose a single subdev, and offer the same frame sizes for
> > all media bus codes, but will be extended to support more complex
> > sensors as the needs arise.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  src/libcamera/camera_sensor.cpp       | 245 ++++++++++++++++++++++++++
> >  src/libcamera/include/camera_sensor.h |  56 ++++++
> >  src/libcamera/meson.build             |   2 +
> >  3 files changed, 303 insertions(+)
> >  create mode 100644 src/libcamera/camera_sensor.cpp
> >  create mode 100644 src/libcamera/include/camera_sensor.h
> > 
> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > new file mode 100644
> > index 000000000000..aca9e77fd986
> > --- /dev/null
> > +++ b/src/libcamera/camera_sensor.cpp
> > @@ -0,0 +1,245 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * camera_sensor.cpp - A camera sensor
> > + */
> > +
> > +#include <algorithm>
> > +#include <float.h>
> > +#include <limits.h>
> > +#include <math.h>
> > +
> > +#include "camera_sensor.h"
> > +#include "formats.h"
> > +#include "v4l2_subdevice.h"
> > +
> > +/**
> > + * \file camera_sensor.h
> > + * \brief A camera sensor
> > + */
> > +
> > +namespace libcamera {
> > +
> > +LOG_DEFINE_CATEGORY(CameraSensor);
> > +
> > +/**
> > + * \class CameraSensor
> > + * \brief A camera sensor
> > + *
> > + * The CameraSensor class eases handling of sensors for pipeline handlers by
> > + * hiding the details of the V4L2 subdevice kernel API and caching sensor
> > + * information.
> > + *
> > + * The implementation is currently limited to sensors that expose a single V4L2
> > + * subdevice with a single pad, and support the same frame sizes for all
> > + * supported media bus codes. It will be extended to support more complex
> > + * devices as the needs arise.
> > + */
> > +
> > +/**
> > + * \brief Construct a CameraSensor
> > + * \param[in] entity The media entity for the camera sensor
> > + *
> > + * Once constructed the instance must be initialized with init().
> > + */
> > +CameraSensor::CameraSensor(const MediaEntity *entity)
> > +	: entity_(entity)
> > +{
> > +	subdev_ = new V4L2Subdevice(entity);
> > +}
> > +
> > +/**
> > + * \brief Destroy a CameraSensor
> > + */
> > +CameraSensor::~CameraSensor()
> > +{
> > +	delete subdev_;
> > +}
> > +
> > +/**
> > + * \brief Initialize the camera sensor instance
> > + *
> > + * This methods perform the initialisation steps of the CameraSensor that may
> > + * fail. It shall be called once and only once after constructing the instance.
> > + *
> > + * \return 0 on success or a negative error code otherwise
> > + */
> > +int CameraSensor::init()
> > +{
> > +	int ret;
> > +
> > +	if (entity_->pads().size() != 1) {
> > +		LOG(CameraSensor, Error)
> > +			<< "Sensors with more than one pad are not supported"
> > +			<< std::endl;
> > +		return -EINVAL;
> > +	}
> > +
> > +	ret = subdev_->open();
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	/* Enumerate and cache media bus codes and sizes. */
> > +	const FormatEnum formats = subdev_->formats(0);
> > +	if (formats.empty()) {
> > +		LOG(CameraSensor, Error)
> > +			<< "No image format found" << std::endl;
> > +		return -EINVAL;
> > +	}
> > +
> > +	std::transform(formats.begin(), formats.end(),
> > +		       std::back_inserter(mbusCodes_),
> > +		       [](decltype(*formats.begin()) f) { return f.first; });
> > +
> > +	const std::vector<SizeRange> &sizes = formats.begin()->second;
> 
> I might add a comment somewhere around here mentioning the limitation 
> and why only sizes from formats.begin() are considered. I had to reread 
> the commit message to figure it out. Not a big issue as it's documented 
> elsewhere so it won't be forgotten. Maybe me pointing it out here for 
> other reviewers benefit is enough.

I'll add the following comment:

        /*
         * Extract the supported sizes from the first format as we only support
         * sensors that offer the same frame sizes for all media bus codes.
         * Verify this assumption and reject the sensor if it isn't true.
         */

> > +	std::transform(sizes.begin(), sizes.end(), std::back_inserter(sizes_),
> > +		       [](const SizeRange &range) { return range.max; });
> > +
> > +	/*
> > +	 * Verify the assumption that all available media bus codes support the
> > +	 * same frame sizes.
> > +	 */

And will remove this one.

> > +	for (auto it = ++formats.begin(); it != formats.end(); ++it) {
> > +		if (it->second != sizes) {
> > +			LOG(CameraSensor, Error)
> > +				<< "Frame sizes differ between media bus codes"
> > +				<< std::endl;
> > +			return -EINVAL;
> > +		}
> > +	}
> > +
> > +	/* Sort the sizes. */
> > +	std::sort(sizes_.begin(), sizes_.end());
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * \fn CameraSensor::entity()
> > + * \brief Retrieve the sensor media entity
> > + * \return The sensor media entity
> > + */
> > +
> > +/**
> > + * \fn CameraSensor::mbusCodes()
> > + * \brief Retrieve the media bus codes supported by the camera sensor
> > + * \return The supported media bus codes
> > + */
> > +
> > +/**
> > + * \fn CameraSensor::sizes()
> > + * \brief Retrieve the frame sizes supported by the camera sensor
> > + * \return The supported frame sizes
> > + */
> > +
> > +/**
> > + * \fn CameraSensor::resolution()
> > + * \brief Retrieve the camera sensor resolution
> > + * \return The camera sensor resolution in pixels
> > + */
> 
> Maybe move the function implementation here adding a comment that the 
> sizes_ vector is sorted so that the resolution will always be the 
> largest one supported.

Done.

> With or without the above nit-pick comments addressed,
> 
> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> 
> > +
> > +/**
> > + * \brief Retrieve the best sensor format for a desired output
> > + * \param[in] mbusCodes The list of acceptable media bus codes
> > + * \param[in] size The desired size
> > + *
> > + * Media bus codes are selected from \a mbusCodes, which lists all acceptable
> > + * codes in decreasing order of preference. This method selects the first code
> > + * from the list that is supported by the sensor. If none of the desired codes
> > + * is supported, it returns an error.
> > + *
> > + * \a size indicates the desired size at the output of the sensor. This method
> > + * selects the best size supported by the sensor according to the following
> > + * criteria.
> > + *
> > + * - The desired \a size shall fit in the sensor output size to avoid the need
> > + *   to up-scale.
> > + * - The sensor output size shall match the desired aspect ratio to avoid the
> > + *   need to crop the field of view.
> > + * - The sensor output size shall be as small as possible to lower the required
> > + *   bandwidth.
> > + *
> > + * The use of this method is optional, as the above criteria may not match the
> > + * needs of all pipeline handlers. Pipeline handlers may implement custom
> > + * sensor format selection when needed.
> > + *
> > + * The returned sensor output format is guaranteed to be acceptable by the
> > + * setFormat() method without any modification.
> > + *
> > + * \return The best sensor output format matching the desired media bus codes
> > + * and size on success, or an empty format otherwise.
> > + */
> > +V4L2SubdeviceFormat CameraSensor::getFormat(const std::vector<unsigned int> &mbusCodes,
> > +					    const Size &size) const
> > +{
> > +	V4L2SubdeviceFormat format{};
> > +
> > +	for (unsigned int code : mbusCodes_) {
> > +		if (std::any_of(mbusCodes.begin(), mbusCodes.end(),
> > +				[code](unsigned int c) { return c == code; })) {
> > +			format.mbus_code = code;
> > +			break;
> > +		}
> > +	}
> > +
> > +	if (!format.mbus_code) {
> > +		LOG(CameraSensor, Debug)
> > +			<< "No supported format found" << std::endl;
> > +		return format;
> > +	}
> > +
> > +	unsigned int desiredArea = size.width * size.height;
> > +	unsigned int bestArea = UINT_MAX;
> > +	float desiredRatio = static_cast<float>(size.width) / size.height;
> > +	float bestRatio = FLT_MAX;
> > +	const Size *bestSize = nullptr;
> > +
> > +	for (const Size &sz : sizes_) {
> > +		if (sz.width < size.width || sz.height < size.height)
> > +			continue;
> > +
> > +		float ratio = static_cast<float>(sz.width) / sz.height;
> > +		float ratioDiff = fabsf(ratio - desiredRatio);
> > +		unsigned int area = sz.width * sz.height;
> > +		unsigned int areaDiff = area - desiredArea;
> > +
> > +		if (ratioDiff > bestRatio)
> > +			continue;
> > +
> > +		if (ratioDiff < bestRatio || areaDiff < bestArea) {
> > +			bestRatio = ratioDiff;
> > +			bestArea = areaDiff;
> > +			bestSize = &sz;
> > +		}
> > +	}
> > +
> > +	if (!bestSize) {
> > +		LOG(CameraSensor, Debug)
> > +			<< "No supported size found" << std::endl;
> > +		return format;
> > +	}
> > +
> > +	format.width = bestSize->width;
> > +	format.height = bestSize->height;
> > +
> > +	return format;
> > +}
> > +
> > +/**
> > + * \brief Set the sensor output format
> > + * \param[in] format The desired sensor output format
> > + *
> > + * \return 0 on success or a negative error code otherwise
> > + */
> > +int CameraSensor::setFormat(V4L2SubdeviceFormat *format)
> > +{
> > +	return subdev_->setFormat(0, format);
> > +}
> > +
> > +std::string CameraSensor::logPrefix() const
> > +{
> > +	return "'" + subdev_->entity()->name() + "'";
> > +}
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h
> > new file mode 100644
> > index 000000000000..7d92af05248c
> > --- /dev/null
> > +++ b/src/libcamera/include/camera_sensor.h
> > @@ -0,0 +1,56 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * camera_sensor.h - A camera sensor
> > + */
> > +#ifndef __LIBCAMERA_CAMERA_SENSOR_H__
> > +#define __LIBCAMERA_CAMERA_SENSOR_H__
> > +
> > +#include <string>
> > +#include <vector>
> > +
> > +#include <libcamera/geometry.h>
> > +
> > +#include "log.h"
> > +#include "v4l2_subdevice.h"
> > +
> > +namespace libcamera {
> > +
> > +class MediaEntity;
> > +class V4L2SubdeviceFormat;
> > +
> > +class CameraSensor : public Loggable
> > +{
> > +public:
> > +	explicit CameraSensor(const MediaEntity *entity);
> > +	~CameraSensor();
> > +
> > +	CameraSensor(const CameraSensor &) = delete;
> > +	CameraSensor &operator=(const CameraSensor &) = delete;
> > +
> > +	int init();
> > +
> > +	const MediaEntity *entity() const { return entity_; }
> > +	const std::vector<unsigned int> &mbusCodes() const { return mbusCodes_; }
> > +	const std::vector<Size> &sizes() const { return sizes_; }
> > +	const Size &resolution() const { return sizes_.back(); }
> > +
> > +	V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes,
> > +				      const Size &size) const;
> > +	int setFormat(V4L2SubdeviceFormat *format);
> > +
> > +protected:
> > +	std::string logPrefix() const;
> > +
> > +private:
> > +	const MediaEntity *entity_;
> > +	V4L2Subdevice *subdev_;
> > +
> > +	std::vector<unsigned int> mbusCodes_;
> > +	std::vector<Size> sizes_;
> > +};
> > +
> > +} /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_CAMERA_SENSOR_H__ */
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index cd36ac307518..cf4edec05755 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -2,6 +2,7 @@ libcamera_sources = files([
> >      'buffer.cpp',
> >      'camera.cpp',
> >      'camera_manager.cpp',
> > +    'camera_sensor.cpp',
> >      'device_enumerator.cpp',
> >      'event_dispatcher.cpp',
> >      'event_dispatcher_poll.cpp',
> > @@ -23,6 +24,7 @@ libcamera_sources = files([
> >  ])
> >  
> >  libcamera_headers = files([
> > +    'include/camera_sensor.h',
> >      'include/device_enumerator.h',
> >      'include/event_dispatcher_poll.h',
> >      'include/formats.h',

Hi Laurent,

On Mon, Apr 15, 2019 at 07:56:58PM +0300, Laurent Pinchart wrote:
> The CameraSensor class abstracts camera sensors and provides helper
> functions to ease interactions with them. It is currently limited to
> sensors that expose a single subdev, and offer the same frame sizes for
> all media bus codes, but will be extended to support more complex
> sensors as the needs arise.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/libcamera/camera_sensor.cpp       | 245 ++++++++++++++++++++++++++
>  src/libcamera/include/camera_sensor.h |  56 ++++++
>  src/libcamera/meson.build             |   2 +
>  3 files changed, 303 insertions(+)
>  create mode 100644 src/libcamera/camera_sensor.cpp
>  create mode 100644 src/libcamera/include/camera_sensor.h
>
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> new file mode 100644
> index 000000000000..aca9e77fd986
> --- /dev/null
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -0,0 +1,245 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * camera_sensor.cpp - A camera sensor
> + */
> +
> +#include <algorithm>
> +#include <float.h>
> +#include <limits.h>
> +#include <math.h>
> +
> +#include "camera_sensor.h"
> +#include "formats.h"
> +#include "v4l2_subdevice.h"
> +
> +/**
> + * \file camera_sensor.h
> + * \brief A camera sensor
> + */
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(CameraSensor);
> +
> +/**
> + * \class CameraSensor
> + * \brief A camera sensor

a very brief \brief this one

> + *
> + * The CameraSensor class eases handling of sensors for pipeline handlers by
> + * hiding the details of the V4L2 subdevice kernel API and caching sensor
> + * information.

"information such as sizes and supported image formats" ?

> + *
> + * The implementation is currently limited to sensors that expose a single V4L2
> + * subdevice with a single pad, and support the same frame sizes for all
> + * supported media bus codes. It will be extended to support more complex
> + * devices as the needs arise.
> + */
> +
> +/**
> + * \brief Construct a CameraSensor
> + * \param[in] entity The media entity for the camera sensor

s/for/backing ?

> + *
> + * Once constructed the instance must be initialized with init().
> + */
> +CameraSensor::CameraSensor(const MediaEntity *entity)
> +	: entity_(entity)
> +{
> +	subdev_ = new V4L2Subdevice(entity);
> +}
> +
> +/**
> + * \brief Destroy a CameraSensor
> + */
> +CameraSensor::~CameraSensor()
> +{
> +	delete subdev_;
> +}
> +
> +/**
> + * \brief Initialize the camera sensor instance
> + *
> + * This methods perform the initialisation steps of the CameraSensor that may

s/methods/method
s/perform/performs

Or have I missed why the plural here?

> + * fail. It shall be called once and only once after constructing the instance.
> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +int CameraSensor::init()
> +{
> +	int ret;
> +
> +	if (entity_->pads().size() != 1) {
> +		LOG(CameraSensor, Error)
> +			<< "Sensors with more than one pad are not supported"
> +			<< std::endl;

Here and in the rest of the series, LOG() does not need an std::endl
unless you want an additional empty line after the message.

> +		return -EINVAL;
> +	}
> +
> +	ret = subdev_->open();

Does this need to be closed before deleting it?

> +	if (ret < 0)
> +		return ret;
> +
> +	/* Enumerate and cache media bus codes and sizes. */
> +	const FormatEnum formats = subdev_->formats(0);
> +	if (formats.empty()) {
> +		LOG(CameraSensor, Error)
> +			<< "No image format found" << std::endl;
> +		return -EINVAL;
> +	}
> +
> +	std::transform(formats.begin(), formats.end(),
> +		       std::back_inserter(mbusCodes_),
> +		       [](decltype(*formats.begin()) f) { return f.first; });
> +
> +	const std::vector<SizeRange> &sizes = formats.begin()->second;
> +	std::transform(sizes.begin(), sizes.end(), std::back_inserter(sizes_),
> +		       [](const SizeRange &range) { return range.max; });

neat! I hope we could keep something similar when supporting
per-format sizes...

> +
> +	/*
> +	 * Verify the assumption that all available media bus codes support the
> +	 * same frame sizes.
> +	 */
> +	for (auto it = ++formats.begin(); it != formats.end(); ++it) {
> +		if (it->second != sizes) {
> +			LOG(CameraSensor, Error)
> +				<< "Frame sizes differ between media bus codes"
> +				<< std::endl;
> +			return -EINVAL;
> +		}
> +	}
> +
> +	/* Sort the sizes. */
> +	std::sort(sizes_.begin(), sizes_.end());
> +
> +	return 0;
> +}
> +
> +/**
> + * \fn CameraSensor::entity()
> + * \brief Retrieve the sensor media entity

I was sure we enforced a blank line here, but some classes do and some
other does not :(

> + * \return The sensor media entity
> + */
> +
> +/**
> + * \fn CameraSensor::mbusCodes()
> + * \brief Retrieve the media bus codes supported by the camera sensor
> + * \return The supported media bus codes
> + */

mbusCode is V4L2-specific, isn't it? I can't suggest a better name
though, "formats" is confusing, just "codes" is not nice... Up to
you...

> +
> +/**
> + * \fn CameraSensor::sizes()
> + * \brief Retrieve the frame sizes supported by the camera sensor
> + * \return The supported frame sizes
> + */
> +
> +/**
> + * \fn CameraSensor::resolution()
> + * \brief Retrieve the camera sensor resolution
> + * \return The camera sensor resolution in pixels
> + */

This is the maximum resolution the sensor supports, right?

> +
> +/**
> + * \brief Retrieve the best sensor format for a desired output
> + * \param[in] mbusCodes The list of acceptable media bus codes
> + * \param[in] size The desired size
> + *
> + * Media bus codes are selected from \a mbusCodes, which lists all acceptable
> + * codes in decreasing order of preference. This method selects the first code
> + * from the list that is supported by the sensor. If none of the desired codes
> + * is supported, it returns an error.
> + *
> + * \a size indicates the desired size at the output of the sensor. This method
> + * selects the best size supported by the sensor according to the following
> + * criteria.
> + *
> + * - The desired \a size shall fit in the sensor output size to avoid the need
> + *   to up-scale.
> + * - The sensor output size shall match the desired aspect ratio to avoid the
> + *   need to crop the field of view.
> + * - The sensor output size shall be as small as possible to lower the required
> + *   bandwidth.
> + *
> + * The use of this method is optional, as the above criteria may not match the
> + * needs of all pipeline handlers. Pipeline handlers may implement custom
> + * sensor format selection when needed.
> + *
> + * The returned sensor output format is guaranteed to be acceptable by the
> + * setFormat() method without any modification.
> + *
> + * \return The best sensor output format matching the desired media bus codes
> + * and size on success, or an empty format otherwise.
> + */
> +V4L2SubdeviceFormat CameraSensor::getFormat(const std::vector<unsigned int> &mbusCodes,
> +					    const Size &size) const
> +{
> +	V4L2SubdeviceFormat format{};
> +
> +	for (unsigned int code : mbusCodes_) {
> +		if (std::any_of(mbusCodes.begin(), mbusCodes.end(),
> +				[code](unsigned int c) { return c == code; })) {
> +			format.mbus_code = code;
> +			break;
> +		}
> +	}
> +
> +	if (!format.mbus_code) {
> +		LOG(CameraSensor, Debug)
> +			<< "No supported format found" << std::endl;
> +		return format;
> +	}
> +
> +	unsigned int desiredArea = size.width * size.height;

will we need a Size::area() ?

> +	unsigned int bestArea = UINT_MAX;
> +	float desiredRatio = static_cast<float>(size.width) / size.height;
> +	float bestRatio = FLT_MAX;
> +	const Size *bestSize = nullptr;
> +
> +	for (const Size &sz : sizes_) {
> +		if (sz.width < size.width || sz.height < size.height)

Couldn't you just compare "sz < size" ?

 * - A size with smaller width and smaller height is smaller.
 * - A size with smaller area is smaller.
 * - A size with smaller width is smaller.

doesn't it apply here?

> +			continue;
> +
> +		float ratio = static_cast<float>(sz.width) / sz.height;
> +		float ratioDiff = fabsf(ratio - desiredRatio);
> +		unsigned int area = sz.width * sz.height;
> +		unsigned int areaDiff = area - desiredArea;
> +
> +		if (ratioDiff > bestRatio)
> +			continue;
> +
> +		if (ratioDiff < bestRatio || areaDiff < bestArea) {

This is puzzling, but most probably what you want. If ratio is <= than
the best one, then if ratio or area is < than the current best one
then select the format. So there is no priority between ratio and
area differences, right?

> +			bestRatio = ratioDiff;
> +			bestArea = areaDiff;
> +			bestSize = &sz;
> +		}
> +	}
> +
> +	if (!bestSize) {
> +		LOG(CameraSensor, Debug)
> +			<< "No supported size found" << std::endl;
> +		return format;
> +	}
> +
> +	format.width = bestSize->width;
> +	format.height = bestSize->height;
> +
> +	return format;
> +}
> +
> +/**
> + * \brief Set the sensor output format
> + * \param[in] format The desired sensor output format
> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +int CameraSensor::setFormat(V4L2SubdeviceFormat *format)
> +{
> +	return subdev_->setFormat(0, format);
> +}
> +
> +std::string CameraSensor::logPrefix() const
> +{
> +	return "'" + subdev_->entity()->name() + "'";
> +}
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h
> new file mode 100644
> index 000000000000..7d92af05248c
> --- /dev/null
> +++ b/src/libcamera/include/camera_sensor.h
> @@ -0,0 +1,56 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * camera_sensor.h - A camera sensor
> + */
> +#ifndef __LIBCAMERA_CAMERA_SENSOR_H__
> +#define __LIBCAMERA_CAMERA_SENSOR_H__
> +
> +#include <string>
> +#include <vector>
> +
> +#include <libcamera/geometry.h>
> +
> +#include "log.h"
> +#include "v4l2_subdevice.h"
> +
> +namespace libcamera {
> +
> +class MediaEntity;
> +class V4L2SubdeviceFormat;

As you include "v4l2_subdevice.h" you don't need this

> +
> +class CameraSensor : public Loggable

nit: other classes that inherits Loggable inherits its protected
fields only. Is this desirable?

Thanks
   j

> +{
> +public:
> +	explicit CameraSensor(const MediaEntity *entity);
> +	~CameraSensor();
> +
> +	CameraSensor(const CameraSensor &) = delete;
> +	CameraSensor &operator=(const CameraSensor &) = delete;
> +
> +	int init();
> +
> +	const MediaEntity *entity() const { return entity_; }
> +	const std::vector<unsigned int> &mbusCodes() const { return mbusCodes_; }
> +	const std::vector<Size> &sizes() const { return sizes_; }
> +	const Size &resolution() const { return sizes_.back(); }
> +
> +	V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes,
> +				      const Size &size) const;
> +	int setFormat(V4L2SubdeviceFormat *format);
> +
> +protected:
> +	std::string logPrefix() const;
> +
> +private:
> +	const MediaEntity *entity_;
> +	V4L2Subdevice *subdev_;
> +
> +	std::vector<unsigned int> mbusCodes_;
> +	std::vector<Size> sizes_;
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_CAMERA_SENSOR_H__ */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index cd36ac307518..cf4edec05755 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -2,6 +2,7 @@ libcamera_sources = files([
>      'buffer.cpp',
>      'camera.cpp',
>      'camera_manager.cpp',
> +    'camera_sensor.cpp',
>      'device_enumerator.cpp',
>      'event_dispatcher.cpp',
>      'event_dispatcher_poll.cpp',
> @@ -23,6 +24,7 @@ libcamera_sources = files([
>  ])
>
>  libcamera_headers = files([
> +    'include/camera_sensor.h',
>      'include/device_enumerator.h',
>      'include/event_dispatcher_poll.h',
>      'include/formats.h',
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Jacopo,

On Tue, Apr 16, 2019 at 06:25:48PM +0200, Jacopo Mondi wrote:
> On Mon, Apr 15, 2019 at 07:56:58PM +0300, Laurent Pinchart wrote:
> > The CameraSensor class abstracts camera sensors and provides helper
> > functions to ease interactions with them. It is currently limited to
> > sensors that expose a single subdev, and offer the same frame sizes for
> > all media bus codes, but will be extended to support more complex
> > sensors as the needs arise.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  src/libcamera/camera_sensor.cpp       | 245 ++++++++++++++++++++++++++
> >  src/libcamera/include/camera_sensor.h |  56 ++++++
> >  src/libcamera/meson.build             |   2 +
> >  3 files changed, 303 insertions(+)
> >  create mode 100644 src/libcamera/camera_sensor.cpp
> >  create mode 100644 src/libcamera/include/camera_sensor.h
> >
> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > new file mode 100644
> > index 000000000000..aca9e77fd986
> > --- /dev/null
> > +++ b/src/libcamera/camera_sensor.cpp
> > @@ -0,0 +1,245 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * camera_sensor.cpp - A camera sensor
> > + */
> > +
> > +#include <algorithm>
> > +#include <float.h>
> > +#include <limits.h>
> > +#include <math.h>
> > +
> > +#include "camera_sensor.h"
> > +#include "formats.h"
> > +#include "v4l2_subdevice.h"
> > +
> > +/**
> > + * \file camera_sensor.h
> > + * \brief A camera sensor
> > + */
> > +
> > +namespace libcamera {
> > +
> > +LOG_DEFINE_CATEGORY(CameraSensor);
> > +
> > +/**
> > + * \class CameraSensor
> > + * \brief A camera sensor
> 
> a very brief \brief this one

Isn't it neat ? :-) On a more serious note, is there other information
you think should be included in \brief here ?

> > + *
> > + * The CameraSensor class eases handling of sensors for pipeline handlers by
> > + * hiding the details of the V4L2 subdevice kernel API and caching sensor
> > + * information.
> 
> "information such as sizes and supported image formats" ?

I had this in a previous version :-) Given that the class has limited
features at the moment and will be extended probably sooner than later I
decided to leave specific examples out for now.

> > + *
> > + * The implementation is currently limited to sensors that expose a single V4L2
> > + * subdevice with a single pad, and support the same frame sizes for all
> > + * supported media bus codes. It will be extended to support more complex
> > + * devices as the needs arise.
> > + */
> > +
> > +/**
> > + * \brief Construct a CameraSensor
> > + * \param[in] entity The media entity for the camera sensor
> 
> s/for/backing ?

I think we'll have to update this when the CameraSensor class will be
extended, but for now I lack a better term, so I'll go with your
suggestion.

> > + *
> > + * Once constructed the instance must be initialized with init().
> > + */
> > +CameraSensor::CameraSensor(const MediaEntity *entity)
> > +	: entity_(entity)
> > +{
> > +	subdev_ = new V4L2Subdevice(entity);
> > +}
> > +
> > +/**
> > + * \brief Destroy a CameraSensor
> > + */
> > +CameraSensor::~CameraSensor()
> > +{
> > +	delete subdev_;
> > +}
> > +
> > +/**
> > + * \brief Initialize the camera sensor instance
> > + *
> > + * This methods perform the initialisation steps of the CameraSensor that may
> 
> s/methods/method
> s/perform/performs
> 
> Or have I missed why the plural here?

No, you're right. Fixed.

> > + * fail. It shall be called once and only once after constructing the instance.
> > + *
> > + * \return 0 on success or a negative error code otherwise
> > + */
> > +int CameraSensor::init()
> > +{
> > +	int ret;
> > +
> > +	if (entity_->pads().size() != 1) {
> > +		LOG(CameraSensor, Error)
> > +			<< "Sensors with more than one pad are not supported"
> > +			<< std::endl;
> 
> Here and in the rest of the series, LOG() does not need an std::endl
> unless you want an additional empty line after the message.

Oops. Fixed.

> > +		return -EINVAL;
> > +	}
> > +
> > +	ret = subdev_->open();
> 
> Does this need to be closed before deleting it?

I would have sworn we had a destructor for V4L2Subdevice that would
close the fd. I'll add an extra patch to fix this.

> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	/* Enumerate and cache media bus codes and sizes. */
> > +	const FormatEnum formats = subdev_->formats(0);
> > +	if (formats.empty()) {
> > +		LOG(CameraSensor, Error)
> > +			<< "No image format found" << std::endl;
> > +		return -EINVAL;
> > +	}
> > +
> > +	std::transform(formats.begin(), formats.end(),
> > +		       std::back_inserter(mbusCodes_),
> > +		       [](decltype(*formats.begin()) f) { return f.first; });
> > +
> > +	const std::vector<SizeRange> &sizes = formats.begin()->second;
> > +	std::transform(sizes.begin(), sizes.end(), std::back_inserter(sizes_),
> > +		       [](const SizeRange &range) { return range.max; });
> 
> neat! I hope we could keep something similar when supporting
> per-format sizes...

I pondered for some time on the usage of std::transform with a lambda
versus an explicit loop, and in the end decided to go for it because it
reminded me of Python :-)

> > +
> > +	/*
> > +	 * Verify the assumption that all available media bus codes support the
> > +	 * same frame sizes.
> > +	 */
> > +	for (auto it = ++formats.begin(); it != formats.end(); ++it) {
> > +		if (it->second != sizes) {
> > +			LOG(CameraSensor, Error)
> > +				<< "Frame sizes differ between media bus codes"
> > +				<< std::endl;
> > +			return -EINVAL;
> > +		}
> > +	}
> > +
> > +	/* Sort the sizes. */
> > +	std::sort(sizes_.begin(), sizes_.end());
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * \fn CameraSensor::entity()
> > + * \brief Retrieve the sensor media entity
> 
> I was sure we enforced a blank line here, but some classes do and some
> other does not :(

I've replied to a similar comment in one of your previous reviews, let's
discuss it there.

> > + * \return The sensor media entity
> > + */
> > +
> > +/**
> > + * \fn CameraSensor::mbusCodes()
> > + * \brief Retrieve the media bus codes supported by the camera sensor
> > + * \return The supported media bus codes
> > + */
> 
> mbusCode is V4L2-specific, isn't it? I can't suggest a better name
> though, "formats" is confusing, just "codes" is not nice... Up to
> you...

It is V4L2-specific, true, but I think it's fine. The CameraSensor class
is a helper to handle sensors exposed through the V4L2 API, and it is
supposed to integrate with the rest of a V4L2-based MC pipeline. Usage
of V4L2-specific terms is fine in my opinion, given that the class is
internal to libcamera. I'll add a mention of V4L2 to the class
documentation.

> > +
> > +/**
> > + * \fn CameraSensor::sizes()
> > + * \brief Retrieve the frame sizes supported by the camera sensor
> > + * \return The supported frame sizes
> > + */
> > +
> > +/**
> > + * \fn CameraSensor::resolution()
> > + * \brief Retrieve the camera sensor resolution
> > + * \return The camera sensor resolution in pixels
> > + */
> 
> This is the maximum resolution the sensor supports, right?

Correct. Do you think "sensor resolution" is too vague and would require
a mention of maximum resolution ?

> > +
> > +/**
> > + * \brief Retrieve the best sensor format for a desired output
> > + * \param[in] mbusCodes The list of acceptable media bus codes
> > + * \param[in] size The desired size
> > + *
> > + * Media bus codes are selected from \a mbusCodes, which lists all acceptable
> > + * codes in decreasing order of preference. This method selects the first code
> > + * from the list that is supported by the sensor. If none of the desired codes
> > + * is supported, it returns an error.
> > + *
> > + * \a size indicates the desired size at the output of the sensor. This method
> > + * selects the best size supported by the sensor according to the following
> > + * criteria.
> > + *
> > + * - The desired \a size shall fit in the sensor output size to avoid the need
> > + *   to up-scale.
> > + * - The sensor output size shall match the desired aspect ratio to avoid the
> > + *   need to crop the field of view.
> > + * - The sensor output size shall be as small as possible to lower the required
> > + *   bandwidth.
> > + *
> > + * The use of this method is optional, as the above criteria may not match the
> > + * needs of all pipeline handlers. Pipeline handlers may implement custom
> > + * sensor format selection when needed.
> > + *
> > + * The returned sensor output format is guaranteed to be acceptable by the
> > + * setFormat() method without any modification.
> > + *
> > + * \return The best sensor output format matching the desired media bus codes
> > + * and size on success, or an empty format otherwise.
> > + */
> > +V4L2SubdeviceFormat CameraSensor::getFormat(const std::vector<unsigned int> &mbusCodes,
> > +					    const Size &size) const
> > +{
> > +	V4L2SubdeviceFormat format{};
> > +
> > +	for (unsigned int code : mbusCodes_) {
> > +		if (std::any_of(mbusCodes.begin(), mbusCodes.end(),
> > +				[code](unsigned int c) { return c == code; })) {
> > +			format.mbus_code = code;
> > +			break;
> > +		}
> > +	}
> > +
> > +	if (!format.mbus_code) {
> > +		LOG(CameraSensor, Debug)
> > +			<< "No supported format found" << std::endl;
> > +		return format;
> > +	}
> > +
> > +	unsigned int desiredArea = size.width * size.height;
> 
> will we need a Size::area() ?

Possibly, but in order to be generic, that would need to return a 64-bit
integer. I've limited the computation to 32-bit here on purpose as we
won't need more than that for the foreseable future (4GPixels sensors
?).

> > +	unsigned int bestArea = UINT_MAX;
> > +	float desiredRatio = static_cast<float>(size.width) / size.height;
> > +	float bestRatio = FLT_MAX;
> > +	const Size *bestSize = nullptr;
> > +
> > +	for (const Size &sz : sizes_) {
> > +		if (sz.width < size.width || sz.height < size.height)
> 
> Couldn't you just compare "sz < size" ?
> 
>  * - A size with smaller width and smaller height is smaller.
>  * - A size with smaller area is smaller.
>  * - A size with smaller width is smaller.
> 
> doesn't it apply here?

I did to start with, and then realized it wouldn't work. We want to
select a size that is larger than the size parameter in both width and
height. operator< would accept a size that overlaps with the size
parameter (has a larger height but smaller width for instance), which we
don't want to accept.

> > +			continue;
> > +
> > +		float ratio = static_cast<float>(sz.width) / sz.height;
> > +		float ratioDiff = fabsf(ratio - desiredRatio);
> > +		unsigned int area = sz.width * sz.height;
> > +		unsigned int areaDiff = area - desiredArea;
> > +
> > +		if (ratioDiff > bestRatio)
> > +			continue;
> > +
> > +		if (ratioDiff < bestRatio || areaDiff < bestArea) {
> 
> This is puzzling, but most probably what you want. If ratio is <= than
> the best one, then if ratio or area is < than the current best one
> then select the format. So there is no priority between ratio and
> area differences, right?

There's actually a priority, if the ratio is better but the area isn't,
the size is still picked. The area criteria is only taken into account
when the ratios are identical.

> > +			bestRatio = ratioDiff;
> > +			bestArea = areaDiff;
> > +			bestSize = &sz;
> > +		}
> > +	}
> > +
> > +	if (!bestSize) {
> > +		LOG(CameraSensor, Debug)
> > +			<< "No supported size found" << std::endl;
> > +		return format;
> > +	}
> > +
> > +	format.width = bestSize->width;
> > +	format.height = bestSize->height;
> > +
> > +	return format;
> > +}
> > +
> > +/**
> > + * \brief Set the sensor output format
> > + * \param[in] format The desired sensor output format
> > + *
> > + * \return 0 on success or a negative error code otherwise
> > + */
> > +int CameraSensor::setFormat(V4L2SubdeviceFormat *format)
> > +{
> > +	return subdev_->setFormat(0, format);
> > +}
> > +
> > +std::string CameraSensor::logPrefix() const
> > +{
> > +	return "'" + subdev_->entity()->name() + "'";
> > +}
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h
> > new file mode 100644
> > index 000000000000..7d92af05248c
> > --- /dev/null
> > +++ b/src/libcamera/include/camera_sensor.h
> > @@ -0,0 +1,56 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * camera_sensor.h - A camera sensor
> > + */
> > +#ifndef __LIBCAMERA_CAMERA_SENSOR_H__
> > +#define __LIBCAMERA_CAMERA_SENSOR_H__
> > +
> > +#include <string>
> > +#include <vector>
> > +
> > +#include <libcamera/geometry.h>
> > +
> > +#include "log.h"
> > +#include "v4l2_subdevice.h"
> > +
> > +namespace libcamera {
> > +
> > +class MediaEntity;
> > +class V4L2SubdeviceFormat;
> 
> As you include "v4l2_subdevice.h" you don't need this

I actually don't need to include that header, I'll add a forward
declaration of V4L2Subdevice instead.

> > +
> > +class CameraSensor : public Loggable
> 
> nit: other classes that inherits Loggable inherits its protected
> fields only. Is this desirable?

This isn't about inheriting the protected members only, but about
turning all private members of the base class into protected members of
the derived class. That is, if Loggable has a public foo() method, and
CameraSensor derives from Loggable with the protected qualifier, then
the foo() method of a CameraSensor instance would be protected, not
public.

It makes no difference in this case as the only public method the
Loggable function exposes is a virtual destructor, but I'll change it
for consistency.

> > +{
> > +public:
> > +	explicit CameraSensor(const MediaEntity *entity);
> > +	~CameraSensor();
> > +
> > +	CameraSensor(const CameraSensor &) = delete;
> > +	CameraSensor &operator=(const CameraSensor &) = delete;
> > +
> > +	int init();
> > +
> > +	const MediaEntity *entity() const { return entity_; }
> > +	const std::vector<unsigned int> &mbusCodes() const { return mbusCodes_; }
> > +	const std::vector<Size> &sizes() const { return sizes_; }
> > +	const Size &resolution() const { return sizes_.back(); }
> > +
> > +	V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes,
> > +				      const Size &size) const;
> > +	int setFormat(V4L2SubdeviceFormat *format);
> > +
> > +protected:
> > +	std::string logPrefix() const;
> > +
> > +private:
> > +	const MediaEntity *entity_;
> > +	V4L2Subdevice *subdev_;
> > +
> > +	std::vector<unsigned int> mbusCodes_;
> > +	std::vector<Size> sizes_;
> > +};
> > +
> > +} /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_CAMERA_SENSOR_H__ */
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index cd36ac307518..cf4edec05755 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -2,6 +2,7 @@ libcamera_sources = files([
> >      'buffer.cpp',
> >      'camera.cpp',
> >      'camera_manager.cpp',
> > +    'camera_sensor.cpp',
> >      'device_enumerator.cpp',
> >      'event_dispatcher.cpp',
> >      'event_dispatcher_poll.cpp',
> > @@ -23,6 +24,7 @@ libcamera_sources = files([
> >  ])
> >
> >  libcamera_headers = files([
> > +    'include/camera_sensor.h',
> >      'include/device_enumerator.h',
> >      'include/event_dispatcher_poll.h',
> >      'include/formats.h',

Hi Laurent,

On Tue, Apr 16, 2019 at 11:52:42PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Tue, Apr 16, 2019 at 06:25:48PM +0200, Jacopo Mondi wrote:
> > On Mon, Apr 15, 2019 at 07:56:58PM +0300, Laurent Pinchart wrote:
> > > The CameraSensor class abstracts camera sensors and provides helper
> > > functions to ease interactions with them. It is currently limited to
> > > sensors that expose a single subdev, and offer the same frame sizes for
> > > all media bus codes, but will be extended to support more complex
> > > sensors as the needs arise.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > >  src/libcamera/camera_sensor.cpp       | 245 ++++++++++++++++++++++++++
> > >  src/libcamera/include/camera_sensor.h |  56 ++++++
> > >  src/libcamera/meson.build             |   2 +
> > >  3 files changed, 303 insertions(+)
> > >  create mode 100644 src/libcamera/camera_sensor.cpp
> > >  create mode 100644 src/libcamera/include/camera_sensor.h
> > >
> > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > > new file mode 100644
> > > index 000000000000..aca9e77fd986
> > > --- /dev/null
> > > +++ b/src/libcamera/camera_sensor.cpp
> > > @@ -0,0 +1,245 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2019, Google Inc.
> > > + *
> > > + * camera_sensor.cpp - A camera sensor
> > > + */
> > > +
> > > +#include <algorithm>
> > > +#include <float.h>
> > > +#include <limits.h>
> > > +#include <math.h>
> > > +
> > > +#include "camera_sensor.h"
> > > +#include "formats.h"
> > > +#include "v4l2_subdevice.h"
> > > +
> > > +/**
> > > + * \file camera_sensor.h
> > > + * \brief A camera sensor
> > > + */
> > > +
> > > +namespace libcamera {
> > > +
> > > +LOG_DEFINE_CATEGORY(CameraSensor);
> > > +
> > > +/**
> > > + * \class CameraSensor
> > > + * \brief A camera sensor
> >
> > a very brief \brief this one
>
> Isn't it neat ? :-) On a more serious note, is there other information
> you think should be included in \brief here ?
>

it's fine, there are are short descriptions in other classes

> > > + *
> > > + * The CameraSensor class eases handling of sensors for pipeline handlers by
> > > + * hiding the details of the V4L2 subdevice kernel API and caching sensor
> > > + * information.
> >
> > "information such as sizes and supported image formats" ?
>
> I had this in a previous version :-) Given that the class has limited
> features at the moment and will be extended probably sooner than later I
> decided to leave specific examples out for now.
>

fine with me!

> > > + *
> > > + * The implementation is currently limited to sensors that expose a single V4L2
> > > + * subdevice with a single pad, and support the same frame sizes for all
> > > + * supported media bus codes. It will be extended to support more complex
> > > + * devices as the needs arise.
> > > + */
> > > +
> > > +/**
> > > + * \brief Construct a CameraSensor
> > > + * \param[in] entity The media entity for the camera sensor
> >
> > s/for/backing ?
>
> I think we'll have to update this when the CameraSensor class will be
> extended, but for now I lack a better term, so I'll go with your
> suggestion.
>
> > > + *
> > > + * Once constructed the instance must be initialized with init().
> > > + */
> > > +CameraSensor::CameraSensor(const MediaEntity *entity)
> > > +	: entity_(entity)
> > > +{
> > > +	subdev_ = new V4L2Subdevice(entity);
> > > +}
> > > +
> > > +/**
> > > + * \brief Destroy a CameraSensor
> > > + */
> > > +CameraSensor::~CameraSensor()
> > > +{
> > > +	delete subdev_;
> > > +}
> > > +
> > > +/**
> > > + * \brief Initialize the camera sensor instance
> > > + *
> > > + * This methods perform the initialisation steps of the CameraSensor that may
> >
> > s/methods/method
> > s/perform/performs
> >
> > Or have I missed why the plural here?
>
> No, you're right. Fixed.
>
> > > + * fail. It shall be called once and only once after constructing the instance.
> > > + *
> > > + * \return 0 on success or a negative error code otherwise
> > > + */
> > > +int CameraSensor::init()
> > > +{
> > > +	int ret;
> > > +
> > > +	if (entity_->pads().size() != 1) {
> > > +		LOG(CameraSensor, Error)
> > > +			<< "Sensors with more than one pad are not supported"
> > > +			<< std::endl;
> >
> > Here and in the rest of the series, LOG() does not need an std::endl
> > unless you want an additional empty line after the message.
>
> Oops. Fixed.
>
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	ret = subdev_->open();
> >
> > Does this need to be closed before deleting it?
>
> I would have sworn we had a destructor for V4L2Subdevice that would
> close the fd. I'll add an extra patch to fix this.
>

I see the patch! thanks

> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	/* Enumerate and cache media bus codes and sizes. */
> > > +	const FormatEnum formats = subdev_->formats(0);
> > > +	if (formats.empty()) {
> > > +		LOG(CameraSensor, Error)
> > > +			<< "No image format found" << std::endl;
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	std::transform(formats.begin(), formats.end(),
> > > +		       std::back_inserter(mbusCodes_),
> > > +		       [](decltype(*formats.begin()) f) { return f.first; });
> > > +
> > > +	const std::vector<SizeRange> &sizes = formats.begin()->second;
> > > +	std::transform(sizes.begin(), sizes.end(), std::back_inserter(sizes_),
> > > +		       [](const SizeRange &range) { return range.max; });
> >
> > neat! I hope we could keep something similar when supporting
> > per-format sizes...
>
> I pondered for some time on the usage of std::transform with a lambda
> versus an explicit loop, and in the end decided to go for it because it
> reminded me of Python :-)
>

indeed! this is very similar to a python's map

> > > +
> > > +	/*
> > > +	 * Verify the assumption that all available media bus codes support the
> > > +	 * same frame sizes.
> > > +	 */
> > > +	for (auto it = ++formats.begin(); it != formats.end(); ++it) {
> > > +		if (it->second != sizes) {
> > > +			LOG(CameraSensor, Error)
> > > +				<< "Frame sizes differ between media bus codes"
> > > +				<< std::endl;
> > > +			return -EINVAL;
> > > +		}
> > > +	}
> > > +
> > > +	/* Sort the sizes. */
> > > +	std::sort(sizes_.begin(), sizes_.end());
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +/**
> > > + * \fn CameraSensor::entity()
> > > + * \brief Retrieve the sensor media entity
> >
> > I was sure we enforced a blank line here, but some classes do and some
> > other does not :(
>
> I've replied to a similar comment in one of your previous reviews, let's
> discuss it there.
>
> > > + * \return The sensor media entity
> > > + */
> > > +
> > > +/**
> > > + * \fn CameraSensor::mbusCodes()
> > > + * \brief Retrieve the media bus codes supported by the camera sensor
> > > + * \return The supported media bus codes
> > > + */
> >
> > mbusCode is V4L2-specific, isn't it? I can't suggest a better name
> > though, "formats" is confusing, just "codes" is not nice... Up to
> > you...
>
> It is V4L2-specific, true, but I think it's fine. The CameraSensor class
> is a helper to handle sensors exposed through the V4L2 API, and it is
> supposed to integrate with the rest of a V4L2-based MC pipeline. Usage
> of V4L2-specific terms is fine in my opinion, given that the class is
> internal to libcamera. I'll add a mention of V4L2 to the class
> documentation.
>

I see, fine with me!

> > > +
> > > +/**
> > > + * \fn CameraSensor::sizes()
> > > + * \brief Retrieve the frame sizes supported by the camera sensor
> > > + * \return The supported frame sizes
> > > + */
> > > +
> > > +/**
> > > + * \fn CameraSensor::resolution()
> > > + * \brief Retrieve the camera sensor resolution
> > > + * \return The camera sensor resolution in pixels
> > > + */
> >
> > This is the maximum resolution the sensor supports, right?
>
> Correct. Do you think "sensor resolution" is too vague and would require
> a mention of maximum resolution ?
>

This actually confused me! And I'm not even sure the maximum reported
resolution is actually the "sensor resolution" if what you mean is the pixel
array size here...

I would mention "max" "maximum" in the method name, or at least in the
method documentation, but it's up to you...

> > > +
> > > +/**
> > > + * \brief Retrieve the best sensor format for a desired output
> > > + * \param[in] mbusCodes The list of acceptable media bus codes
> > > + * \param[in] size The desired size
> > > + *
> > > + * Media bus codes are selected from \a mbusCodes, which lists all acceptable
> > > + * codes in decreasing order of preference. This method selects the first code
> > > + * from the list that is supported by the sensor. If none of the desired codes
> > > + * is supported, it returns an error.
> > > + *
> > > + * \a size indicates the desired size at the output of the sensor. This method
> > > + * selects the best size supported by the sensor according to the following
> > > + * criteria.
> > > + *
> > > + * - The desired \a size shall fit in the sensor output size to avoid the need
> > > + *   to up-scale.
> > > + * - The sensor output size shall match the desired aspect ratio to avoid the
> > > + *   need to crop the field of view.
> > > + * - The sensor output size shall be as small as possible to lower the required
> > > + *   bandwidth.
> > > + *
> > > + * The use of this method is optional, as the above criteria may not match the
> > > + * needs of all pipeline handlers. Pipeline handlers may implement custom
> > > + * sensor format selection when needed.
> > > + *
> > > + * The returned sensor output format is guaranteed to be acceptable by the
> > > + * setFormat() method without any modification.
> > > + *
> > > + * \return The best sensor output format matching the desired media bus codes
> > > + * and size on success, or an empty format otherwise.
> > > + */
> > > +V4L2SubdeviceFormat CameraSensor::getFormat(const std::vector<unsigned int> &mbusCodes,
> > > +					    const Size &size) const
> > > +{
> > > +	V4L2SubdeviceFormat format{};
> > > +
> > > +	for (unsigned int code : mbusCodes_) {
> > > +		if (std::any_of(mbusCodes.begin(), mbusCodes.end(),
> > > +				[code](unsigned int c) { return c == code; })) {
> > > +			format.mbus_code = code;
> > > +			break;
> > > +		}
> > > +	}
> > > +
> > > +	if (!format.mbus_code) {
> > > +		LOG(CameraSensor, Debug)
> > > +			<< "No supported format found" << std::endl;
> > > +		return format;
> > > +	}
> > > +
> > > +	unsigned int desiredArea = size.width * size.height;
> >
> > will we need a Size::area() ?
>
> Possibly, but in order to be generic, that would need to return a 64-bit
> integer. I've limited the computation to 32-bit here on purpose as we
> won't need more than that for the foreseable future (4GPixels sensors
> ?).
>
> > > +	unsigned int bestArea = UINT_MAX;
> > > +	float desiredRatio = static_cast<float>(size.width) / size.height;
> > > +	float bestRatio = FLT_MAX;
> > > +	const Size *bestSize = nullptr;
> > > +
> > > +	for (const Size &sz : sizes_) {
> > > +		if (sz.width < size.width || sz.height < size.height)
> >
> > Couldn't you just compare "sz < size" ?
> >
> >  * - A size with smaller width and smaller height is smaller.
> >  * - A size with smaller area is smaller.
> >  * - A size with smaller width is smaller.
> >
> > doesn't it apply here?
>
> I did to start with, and then realized it wouldn't work. We want to
> select a size that is larger than the size parameter in both width and
> height. operator< would accept a size that overlaps with the size
> parameter (has a larger height but smaller width for instance), which we
> don't want to accept.
>

Fine, thanks for clarification!

> > > +			continue;
> > > +
> > > +		float ratio = static_cast<float>(sz.width) / sz.height;
> > > +		float ratioDiff = fabsf(ratio - desiredRatio);
> > > +		unsigned int area = sz.width * sz.height;
> > > +		unsigned int areaDiff = area - desiredArea;
> > > +
> > > +		if (ratioDiff > bestRatio)
> > > +			continue;
> > > +
> > > +		if (ratioDiff < bestRatio || areaDiff < bestArea) {
> >
> > This is puzzling, but most probably what you want. If ratio is <= than
> > the best one, then if ratio or area is < than the current best one
> > then select the format. So there is no priority between ratio and
> > area differences, right?
>
> There's actually a priority, if the ratio is better but the area isn't,
> the size is still picked. The area criteria is only taken into account
> when the ratios are identical.
>

Indeed. Thanks for the clarification!

> > > +			bestRatio = ratioDiff;
> > > +			bestArea = areaDiff;
> > > +			bestSize = &sz;
> > > +		}
> > > +	}
> > > +
> > > +	if (!bestSize) {
> > > +		LOG(CameraSensor, Debug)
> > > +			<< "No supported size found" << std::endl;
> > > +		return format;
> > > +	}
> > > +
> > > +	format.width = bestSize->width;
> > > +	format.height = bestSize->height;
> > > +
> > > +	return format;
> > > +}
> > > +
> > > +/**
> > > + * \brief Set the sensor output format
> > > + * \param[in] format The desired sensor output format
> > > + *
> > > + * \return 0 on success or a negative error code otherwise
> > > + */
> > > +int CameraSensor::setFormat(V4L2SubdeviceFormat *format)
> > > +{
> > > +	return subdev_->setFormat(0, format);
> > > +}
> > > +
> > > +std::string CameraSensor::logPrefix() const
> > > +{
> > > +	return "'" + subdev_->entity()->name() + "'";
> > > +}
> > > +
> > > +} /* namespace libcamera */
> > > diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h
> > > new file mode 100644
> > > index 000000000000..7d92af05248c
> > > --- /dev/null
> > > +++ b/src/libcamera/include/camera_sensor.h
> > > @@ -0,0 +1,56 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2019, Google Inc.
> > > + *
> > > + * camera_sensor.h - A camera sensor
> > > + */
> > > +#ifndef __LIBCAMERA_CAMERA_SENSOR_H__
> > > +#define __LIBCAMERA_CAMERA_SENSOR_H__
> > > +
> > > +#include <string>
> > > +#include <vector>
> > > +
> > > +#include <libcamera/geometry.h>
> > > +
> > > +#include "log.h"
> > > +#include "v4l2_subdevice.h"
> > > +
> > > +namespace libcamera {
> > > +
> > > +class MediaEntity;
> > > +class V4L2SubdeviceFormat;
> >
> > As you include "v4l2_subdevice.h" you don't need this
>
> I actually don't need to include that header, I'll add a forward
> declaration of V4L2Subdevice instead.
>
> > > +
> > > +class CameraSensor : public Loggable
> >
> > nit: other classes that inherits Loggable inherits its protected
> > fields only. Is this desirable?
>
> This isn't about inheriting the protected members only, but about
> turning all private members of the base class into protected members of
s/private/public ?
> the derived class. That is, if Loggable has a public foo() method, and
> CameraSensor derives from Loggable with the protected qualifier, then
> the foo() method of a CameraSensor instance would be protected, not
> public.
>
> It makes no difference in this case as the only public method the
> Loggable function exposes is a virtual destructor, but I'll change it
> for consistency.

yeah, no differences, but I would change it for consistency.

>
> > > +{
> > > +public:
> > > +	explicit CameraSensor(const MediaEntity *entity);
> > > +	~CameraSensor();
> > > +
> > > +	CameraSensor(const CameraSensor &) = delete;
> > > +	CameraSensor &operator=(const CameraSensor &) = delete;
> > > +
> > > +	int init();
> > > +
> > > +	const MediaEntity *entity() const { return entity_; }
> > > +	const std::vector<unsigned int> &mbusCodes() const { return mbusCodes_; }
> > > +	const std::vector<Size> &sizes() const { return sizes_; }
> > > +	const Size &resolution() const { return sizes_.back(); }
> > > +
> > > +	V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes,
> > > +				      const Size &size) const;
> > > +	int setFormat(V4L2SubdeviceFormat *format);
> > > +
> > > +protected:
> > > +	std::string logPrefix() const;
> > > +
> > > +private:
> > > +	const MediaEntity *entity_;
> > > +	V4L2Subdevice *subdev_;
> > > +
> > > +	std::vector<unsigned int> mbusCodes_;
> > > +	std::vector<Size> sizes_;
> > > +};
> > > +
> > > +} /* namespace libcamera */
> > > +
> > > +#endif /* __LIBCAMERA_CAMERA_SENSOR_H__ */
> > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > > index cd36ac307518..cf4edec05755 100644
> > > --- a/src/libcamera/meson.build
> > > +++ b/src/libcamera/meson.build
> > > @@ -2,6 +2,7 @@ libcamera_sources = files([
> > >      'buffer.cpp',
> > >      'camera.cpp',
> > >      'camera_manager.cpp',
> > > +    'camera_sensor.cpp',
> > >      'device_enumerator.cpp',
> > >      'event_dispatcher.cpp',
> > >      'event_dispatcher_poll.cpp',
> > > @@ -23,6 +24,7 @@ libcamera_sources = files([
> > >  ])
> > >
> > >  libcamera_headers = files([
> > > +    'include/camera_sensor.h',
> > >      'include/device_enumerator.h',
> > >      'include/event_dispatcher_poll.h',
> > >      'include/formats.h',
>
> --
> Regards,
>
> Laurent Pinchart