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

[Jacopo Mondi]

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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190417/fe264020/attachment-0001.sig>