[libcamera-devel] [PATCH 09/11] libcamera: camera_sensor: Add a new class to model a camera sensor
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Apr 16 22:52:42 CEST 2019
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 ?
> > + *
> > + * 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',
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list