[libcamera-devel] [PATCH v2 6/6] libcamera: camera: Add a validation API to the CameraConfiguration class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue May 21 14:51:09 CEST 2019
Hi Jacopo,
On Tue, May 21, 2019 at 10:49:50AM +0200, Jacopo Mondi wrote:
> Hi Laurent,
> one more thing, sorry
>
> On Sun, May 19, 2019 at 06:00:47PM +0300, Laurent Pinchart wrote:
> > The CameraConfiguration class implements a simple storage of
> > StreamConfiguration with internal validation limited to verifying that
> > the stream configurations are not empty. Extend this mechanism by
> > implementing a smart validate() method backed by pipeline handlers.
> >
> > This new mechanism changes the semantics of the camera configuration.
> > The Camera::generateConfiguration() operation still generates a default
> > configuration based on roles, but now also supports generating empty
> > configurations to be filled by applications. Applications can inspect
> > the configuration, optionally modify it, and validate it. The validation
> > implements "try" semantics and adjusts invalid configurations instead of
> > rejecting them completely. Applications then decide whether to accept
> > the modified configuration, or try again with a different set of
> > parameters. Once the configuration is valid, it is passed to
> > Camera::configure(), and pipeline handlers are guaranteed that the
> > configuration they receive is valid.
> >
> > A reference to the Camera may need to be stored in the
> > CameraConfiguration derived classes in order to access it from their
> > validate() implementation. This must be stored as a std::shared_ptr<> as
> > the CameraConfiguration instances belong to applications. In order to
> > make this possible, make the Camera class inherit from
> > std::shared_from_this<>.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > include/libcamera/camera.h | 17 +-
> > src/cam/main.cpp | 2 +-
> > src/libcamera/camera.cpp | 80 +++++--
> > src/libcamera/pipeline/ipu3/ipu3.cpp | 255 ++++++++++++++++++-----
> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 149 ++++++++++---
> > src/libcamera/pipeline/uvcvideo.cpp | 53 ++++-
> > src/libcamera/pipeline/vimc.cpp | 67 +++++-
> > src/libcamera/pipeline_handler.cpp | 17 +-
> > test/camera/capture.cpp | 7 +-
> > test/camera/configuration_default.cpp | 4 +-
> > test/camera/configuration_set.cpp | 7 +-
> > 11 files changed, 516 insertions(+), 142 deletions(-)
> >
> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > index 144133c5de9f..8c0049a1dd94 100644
> > --- a/include/libcamera/camera.h
> > +++ b/include/libcamera/camera.h
> > @@ -25,14 +25,19 @@ class Request;
> > class CameraConfiguration
> > {
> > public:
> > + enum Status {
> > + Valid,
> > + Adjusted,
> > + Invalid,
> > + };
> > +
> > using iterator = std::vector<StreamConfiguration>::iterator;
> > using const_iterator = std::vector<StreamConfiguration>::const_iterator;
> >
> > - CameraConfiguration();
> > + virtual ~CameraConfiguration();
> >
> > void addConfiguration(const StreamConfiguration &cfg);
> > -
> > - bool isValid() const;
> > + virtual Status validate() = 0;
> >
> > StreamConfiguration &at(unsigned int index);
> > const StreamConfiguration &at(unsigned int index) const;
> > @@ -53,11 +58,13 @@ public:
> > bool empty() const;
> > std::size_t size() const;
> >
> > -private:
> > +protected:
> > + CameraConfiguration();
> > +
> > std::vector<StreamConfiguration> config_;
> > };
> >
> > -class Camera final
> > +class Camera final : public std::enable_shared_from_this<Camera>
> > {
> > public:
> > static std::shared_ptr<Camera> create(PipelineHandler *pipe,
> > diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> > index 7550ae4f3428..23da5c687d89 100644
> > --- a/src/cam/main.cpp
> > +++ b/src/cam/main.cpp
>
> Shouldn't you add a config->validate() call before calling
> camera->configure(config) for the cam application?
Niklas has submitted a patch for them, do you think I should squash it
with this one, or apply it separately ?
> (I've not checked qcam, I'm sorry, but it might apply there too)
That should be fixed too, yes.
> > @@ -116,7 +116,7 @@ static CameraConfiguration *prepareCameraConfig()
> > }
> >
> > CameraConfiguration *config = camera->generateConfiguration(roles);
> > - if (!config || !config->isValid()) {
> > + if (!config || config->size() != roles.size()) {
> > std::cerr << "Failed to get default stream configuration"
> > << std::endl;
> > delete config;
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index 0e80691ed862..9da5d4f613f4 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -52,6 +52,28 @@ LOG_DECLARE_CATEGORY(Camera)
> > * operator[](int) returns a reference to the StreamConfiguration based on its
> > * insertion index. Accessing a stream configuration with an invalid index
> > * results in undefined behaviour.
> > + *
> > + * CameraConfiguration instances are retrieved from the camera with
> > + * Camera::generateConfiguration(). Applications may then inspect the
> > + * configuration, modify it, and possibly add new stream configuration entries
> > + * with addConfiguration(). Once the camera configuration satisfies the
> > + * application, it shall be validated by a call to validate(). The validation
> > + * implements "try" semantics: it adjusts invalid configurations to the closest
> > + * achievable parameters instead of rejecting them completely. Applications
> > + * then decide whether to accept the modified configuration, or try again with
> > + * a different set of parameters. Once the configuration is valid, it is passed
> > + * to Camera::configure().
> > + */
> > +
> > +/**
> > + * \enum CameraConfiguration::Status
> > + * \brief Validity of a camera configuration
> > + * \var CameraConfiguration::Valid
> > + * The configuration is fully valid
> > + * \var CameraConfiguration::Adjusted
> > + * The configuration has been adjusted to a valid configuration
> > + * \var CameraConfiguration::Invalid
> > + * The configuration is invalid and can't be adjusted automatically
> > */
> >
> > /**
> > @@ -73,6 +95,10 @@ CameraConfiguration::CameraConfiguration()
> > {
> > }
> >
> > +CameraConfiguration::~CameraConfiguration()
> > +{
> > +}
> > +
> > /**
> > * \brief Add a stream configuration to the camera configuration
> > * \param[in] cfg The stream configuration
> > @@ -83,27 +109,31 @@ void CameraConfiguration::addConfiguration(const StreamConfiguration &cfg)
> > }
> >
> > /**
> > - * \brief Check if the camera configuration is valid
> > + * \fn CameraConfiguration::validate()
> > + * \brief Validate and possibly adjust the camera configuration
> > *
> > - * A camera configuration is deemed to be valid if it contains at least one
> > - * stream configuration and all stream configurations contain valid information.
> > - * Stream configurations are deemed to be valid if all fields are none zero.
> > + * This method adjusts the camera configuration to the closest valid
> > + * configuration and returns the validation status.
> > *
> > - * \return True if the configuration is valid
> > + * \todo: Define exactly when to return each status code. Should stream
> > + * parameters set to 0 by the caller be adjusted without returning Adjusted ?
> > + * This would potentially be useful for applications but would get in the way
> > + * in Camera::configure(). Do we need an extra status code to signal this ?
> > + *
> > + * \todo: Handle validation of buffers count when refactoring the buffers API.
> > + *
> > + * \return A CameraConfiguration::Status value that describes the validation
> > + * status.
> > + * \retval CameraConfiguration::Invalid The configuration is invalid and can't
> > + * be adjusted. This may only occur in extreme cases such as when the
> > + * configuration is empty.
> > + * \retval CameraConfigutation::Adjusted The configuration has been adjusted
> > + * and is now valid. Parameters may have changed for any stream, and stream
> > + * configurations may have been removed. The caller shall check the
> > + * configuration carefully.
> > + * \retval CameraConfiguration::Valid The configuration was already valid and
> > + * hasn't been adjusted.
> > */
> > -bool CameraConfiguration::isValid() const
> > -{
> > - if (empty())
> > - return false;
> > -
> > - for (const StreamConfiguration &cfg : config_) {
> > - if (cfg.size.width == 0 || cfg.size.height == 0 ||
> > - cfg.pixelFormat == 0 || cfg.bufferCount == 0)
> > - return false;
> > - }
> > -
> > - return true;
> > -}
> >
> > /**
> > * \brief Retrieve a reference to a stream configuration
> > @@ -218,6 +248,11 @@ std::size_t CameraConfiguration::size() const
> > return config_.size();
> > }
> >
> > +/**
> > + * \var CameraConfiguration::config_
> > + * \brief The vector of stream configurations
> > + */
> > +
> > /**
> > * \class Camera
> > * \brief Camera device
> > @@ -575,10 +610,9 @@ CameraConfiguration *Camera::generateConfiguration(const StreamRoles &roles)
> > * The caller specifies which streams are to be involved and their configuration
> > * by populating \a config.
> > *
> > - * The easiest way to populate the array of config is to fetch an initial
> > - * configuration from the camera with generateConfiguration() and then change
> > - * the parameters to fit the caller's need and once all the streams parameters
> > - * are configured hand that over to configure() to actually setup the camera.
> > + * The configuration is created by generateConfiguration(), and adjusted by the
> > + * caller with CameraConfiguration::validate(). This method only accepts fully
> > + * valid configurations and returns an error if \a config is not valid.
> > *
> > * Exclusive access to the camera shall be ensured by a call to acquire() prior
> > * to calling this function, otherwise an -EACCES error will be returned.
> > @@ -603,7 +637,7 @@ int Camera::configure(CameraConfiguration *config)
> > if (!stateBetween(CameraAcquired, CameraConfigured))
> > return -EACCES;
> >
> > - if (!config->isValid()) {
> > + if (config->validate() != CameraConfiguration::Valid) {
> > LOG(Camera, Error)
> > << "Can't configure camera with invalid configuration";
> > return -EINVAL;
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 5b46fb68ced2..56265385a1e7 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -164,6 +164,33 @@ public:
> > IPU3Stream vfStream_;
> > };
> >
> > +class IPU3CameraConfiguration : public CameraConfiguration
> > +{
> > +public:
> > + IPU3CameraConfiguration(Camera *camera, IPU3CameraData *data);
> > +
> > + Status validate() override;
> > +
> > + const V4L2SubdeviceFormat &sensorFormat() { return sensorFormat_; }
> > + const std::vector<const IPU3Stream *> &streams() { return streams_; }
> > +
> > +private:
> > + static constexpr unsigned int IPU3_BUFFER_COUNT = 4;
> > +
> > + void adjustStream(unsigned int index, bool scale);
> > +
> > + /*
> > + * The IPU3CameraData instance is guaranteed to be valid as long as the
> > + * corresponding Camera instance is valid. In order to borrow a
> > + * reference to the camera data, store a new reference to the camera.
> > + */
> > + std::shared_ptr<Camera> camera_;
> > + const IPU3CameraData *data_;
> > +
> > + V4L2SubdeviceFormat sensorFormat_;
> > + std::vector<const IPU3Stream *> streams_;
> > +};
> > +
> > class PipelineHandlerIPU3 : public PipelineHandler
> > {
> > public:
> > @@ -186,8 +213,6 @@ public:
> > bool match(DeviceEnumerator *enumerator) override;
> >
> > private:
> > - static constexpr unsigned int IPU3_BUFFER_COUNT = 4;
> > -
> > IPU3CameraData *cameraData(const Camera *camera)
> > {
> > return static_cast<IPU3CameraData *>(
> > @@ -202,6 +227,153 @@ private:
> > MediaDevice *imguMediaDev_;
> > };
> >
> > +IPU3CameraConfiguration::IPU3CameraConfiguration(Camera *camera,
> > + IPU3CameraData *data)
> > + : CameraConfiguration()
> > +{
> > + camera_ = camera->shared_from_this();
> > + data_ = data;
> > +}
> > +
> > +void IPU3CameraConfiguration::adjustStream(unsigned int index, bool scale)
> > +{
> > + StreamConfiguration &cfg = config_[index];
> > +
> > + /* The only pixel format the driver supports is NV12. */
> > + cfg.pixelFormat = V4L2_PIX_FMT_NV12;
> > +
> > + if (scale) {
> > + /*
> > + * Provide a suitable default that matches the sensor aspect
> > + * ratio.
> > + */
> > + if (!cfg.size.width || !cfg.size.height) {
> > + cfg.size.width = 1280;
> > + cfg.size.height = 1280 * sensorFormat_.size.height
> > + / sensorFormat_.size.width;
> > + }
> > +
> > + /*
> > + * \todo: Clamp the size to the hardware bounds when we will
> > + * figure them out.
> > + *
> > + * \todo: Handle the scaler (BDS) restrictions. The BDS can
> > + * only scale with the same factor in both directions, and the
> > + * scaling factor is limited to a multiple of 1/32. At the
> > + * moment the ImgU driver hides these constraints by applying
> > + * additional cropping, this should be fixed on the driver
> > + * side, and cropping should be exposed to us.
> > + */
> > + } else {
> > + /*
> > + * \todo: Properly support cropping when the ImgU driver
> > + * interface will be cleaned up.
> > + */
> > + cfg.size = sensorFormat_.size;
> > + }
> > +
> > + /*
> > + * Clamp the size to match the ImgU alignment constraints. The width
> > + * shall be a multiple of 8 pixels and the height a multiple of 4
> > + * pixels.
> > + */
> > + if (cfg.size.width % 8 || cfg.size.height % 4) {
> > + cfg.size.width &= ~7;
> > + cfg.size.height &= ~3;
> > + }
> > +
> > + cfg.bufferCount = IPU3_BUFFER_COUNT;
> > +}
> > +
> > +CameraConfiguration::Status IPU3CameraConfiguration::validate()
> > +{
> > + const CameraSensor *sensor = data_->cio2_.sensor_;
> > + Status status = Valid;
> > +
> > + if (config_.empty())
> > + return Invalid;
> > +
> > + /* Cap the number of entries to the available streams. */
> > + if (config_.size() > 2) {
> > + config_.resize(2);
> > + status = Adjusted;
> > + }
> > +
> > + /*
> > + * Select the sensor format by collecting the maximum width and height
> > + * and picking the closest larger match, as the IPU3 can downscale
> > + * only. If no resolution is requested for any stream, or if no sensor
> > + * resolution is large enough, pick the largest one.
> > + */
> > + Size size = {};
> > +
> > + for (const StreamConfiguration &cfg : config_) {
> > + if (cfg.size.width > size.width)
> > + size.width = cfg.size.width;
> > + if (cfg.size.height > size.height)
> > + size.height = cfg.size.height;
> > + }
> > +
> > + if (!size.width || !size.height)
> > + size = sensor->resolution();
> > +
> > + sensorFormat_ = sensor->getFormat({ MEDIA_BUS_FMT_SBGGR10_1X10,
> > + MEDIA_BUS_FMT_SGBRG10_1X10,
> > + MEDIA_BUS_FMT_SGRBG10_1X10,
> > + MEDIA_BUS_FMT_SRGGB10_1X10 },
> > + size);
> > + if (!sensorFormat_.size.width || !sensorFormat_.size.height)
> > + sensorFormat_.size = sensor->resolution();
> > +
> > + /*
> > + * Verify and update all configuration entries, and assign a stream to
> > + * each of them. The viewfinder stream can scale, while the output
> > + * stream can crop only, so select the output stream when the requested
> > + * resolution is equal to the sensor resolution, and the viewfinder
> > + * stream otherwise.
> > + */
> > + std::set<const IPU3Stream *> availableStreams = {
> > + &data_->outStream_,
> > + &data_->vfStream_,
> > + };
> > +
> > + streams_.clear();
> > + streams_.reserve(config_.size());
> > +
> > + for (unsigned int i = 0; i < config_.size(); ++i) {
> > + StreamConfiguration &cfg = config_[i];
> > + const unsigned int pixelFormat = cfg.pixelFormat;
> > + const Size size = cfg.size;
> > + const IPU3Stream *stream;
> > +
> > + if (cfg.size == sensorFormat_.size)
> > + stream = &data_->outStream_;
> > + else
> > + stream = &data_->vfStream_;
> > +
> > + if (availableStreams.find(stream) == availableStreams.end())
> > + stream = *availableStreams.begin();
> > +
> > + LOG(IPU3, Debug)
> > + << "Assigned '" << stream->name_ << "' to stream " << i;
> > +
> > + bool scale = stream == &data_->vfStream_;
> > + adjustStream(i, scale);
> > +
> > + if (cfg.pixelFormat != pixelFormat || cfg.size != size) {
> > + LOG(IPU3, Debug)
> > + << "Stream " << i << " configuration adjusted to "
> > + << cfg.toString();
> > + status = Adjusted;
> > + }
> > +
> > + streams_.push_back(stream);
> > + availableStreams.erase(stream);
> > + }
> > +
> > + return status;
> > +}
> > +
> > PipelineHandlerIPU3::PipelineHandlerIPU3(CameraManager *manager)
> > : PipelineHandler(manager), cio2MediaDev_(nullptr), imguMediaDev_(nullptr)
> > {
> > @@ -211,12 +383,14 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
> > const StreamRoles &roles)
> > {
> > IPU3CameraData *data = cameraData(camera);
> > - CameraConfiguration *config = new CameraConfiguration();
> > + IPU3CameraConfiguration *config;
> > std::set<IPU3Stream *> streams = {
> > &data->outStream_,
> > &data->vfStream_,
> > };
> >
> > + config = new IPU3CameraConfiguration(camera, data);
> > +
> > for (const StreamRole role : roles) {
> > StreamConfiguration cfg = {};
> > IPU3Stream *stream = nullptr;
> > @@ -296,71 +470,25 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
> >
> > streams.erase(stream);
> >
> > - cfg.pixelFormat = V4L2_PIX_FMT_NV12;
> > - cfg.bufferCount = IPU3_BUFFER_COUNT;
> > -
> > config->addConfiguration(cfg);
> > }
> >
> > + config->validate();
> > +
> > return config;
> > }
> >
> > -int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *config)
> > +int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
> > {
> > + IPU3CameraConfiguration *config =
> > + static_cast<IPU3CameraConfiguration *>(c);
> > IPU3CameraData *data = cameraData(camera);
> > IPU3Stream *outStream = &data->outStream_;
> > IPU3Stream *vfStream = &data->vfStream_;
> > CIO2Device *cio2 = &data->cio2_;
> > ImgUDevice *imgu = data->imgu_;
> > - Size sensorSize = {};
> > int ret;
> >
> > - outStream->active_ = false;
> > - vfStream->active_ = false;
> > - for (StreamConfiguration &cfg : *config) {
> > - /*
> > - * Pick a stream for the configuration entry.
> > - * \todo: This is a naive temporary implementation that will be
> > - * reworked when implementing camera configuration validation.
> > - */
> > - IPU3Stream *stream = vfStream->active_ ? outStream : vfStream;
> > -
> > - /*
> > - * Verify that the requested size respects the IPU3 alignment
> > - * requirements (the image width shall be a multiple of 8
> > - * pixels and its height a multiple of 4 pixels) and the camera
> > - * maximum sizes.
> > - *
> > - * \todo: Consider the BDS scaling factor requirements: "the
> > - * downscaling factor must be an integer value multiple of 1/32"
> > - */
> > - if (cfg.size.width % 8 || cfg.size.height % 4) {
> > - LOG(IPU3, Error)
> > - << "Invalid stream size: bad alignment";
> > - return -EINVAL;
> > - }
> > -
> > - const Size &resolution = cio2->sensor_->resolution();
> > - if (cfg.size.width > resolution.width ||
> > - cfg.size.height > resolution.height) {
> > - LOG(IPU3, Error)
> > - << "Invalid stream size: larger than sensor resolution";
> > - return -EINVAL;
> > - }
> > -
> > - /*
> > - * Collect the maximum width and height: IPU3 can downscale
> > - * only.
> > - */
> > - if (cfg.size.width > sensorSize.width)
> > - sensorSize.width = cfg.size.width;
> > - if (cfg.size.height > sensorSize.height)
> > - sensorSize.height = cfg.size.height;
> > -
> > - stream->active_ = true;
> > - cfg.setStream(stream);
> > - }
> > -
> > /*
> > * \todo: Enable links selectively based on the requested streams.
> > * As of now, enable all links unconditionally.
> > @@ -373,6 +501,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *config)
> > * Pass the requested stream size to the CIO2 unit and get back the
> > * adjusted format to be propagated to the ImgU output devices.
> > */
> > + const Size &sensorSize = config->sensorFormat().size;
> > V4L2DeviceFormat cio2Format = {};
> > ret = cio2->configure(sensorSize, &cio2Format);
> > if (ret)
> > @@ -383,8 +512,22 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *config)
> > return ret;
> >
> > /* Apply the format to the configured streams output devices. */
> > - for (StreamConfiguration &cfg : *config) {
> > - IPU3Stream *stream = static_cast<IPU3Stream *>(cfg.stream());
> > + outStream->active_ = false;
> > + vfStream->active_ = false;
> > +
> > + for (unsigned int i = 0; i < config->size(); ++i) {
> > + /*
> > + * Use a const_cast<> here instead of storing a mutable stream
> > + * pointer in the configuration to let the compiler catch
> > + * unwanted modifications of camera data in the configuration
> > + * validate() implementation.
> > + */
> > + IPU3Stream *stream = const_cast<IPU3Stream *>(config->streams()[i]);
> > + StreamConfiguration &cfg = (*config)[i];
> > +
> > + stream->active_ = true;
> > + cfg.setStream(stream);
> > +
> > ret = imgu->configureOutput(stream->device_, cfg);
> > if (ret)
> > return ret;
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index a1a4f205b4aa..42944c64189b 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -5,6 +5,7 @@
> > * rkisp1.cpp - Pipeline handler for Rockchip ISP1
> > */
> >
> > +#include <algorithm>
> > #include <iomanip>
> > #include <memory>
> > #include <vector>
> > @@ -45,6 +46,29 @@ public:
> > CameraSensor *sensor_;
> > };
> >
> > +class RkISP1CameraConfiguration : public CameraConfiguration
> > +{
> > +public:
> > + RkISP1CameraConfiguration(Camera *camera, RkISP1CameraData *data);
> > +
> > + Status validate() override;
> > +
> > + const V4L2SubdeviceFormat &sensorFormat() { return sensorFormat_; }
> > +
> > +private:
> > + static constexpr unsigned int RKISP1_BUFFER_COUNT = 4;
> > +
> > + /*
> > + * The RkISP1CameraData instance is guaranteed to be valid as long as the
> > + * corresponding Camera instance is valid. In order to borrow a
> > + * reference to the camera data, store a new reference to the camera.
> > + */
> > + std::shared_ptr<Camera> camera_;
> > + const RkISP1CameraData *data_;
> > +
> > + V4L2SubdeviceFormat sensorFormat_;
> > +};
> > +
> > class PipelineHandlerRkISP1 : public PipelineHandler
> > {
> > public:
> > @@ -68,8 +92,6 @@ public:
> > bool match(DeviceEnumerator *enumerator) override;
> >
> > private:
> > - static constexpr unsigned int RKISP1_BUFFER_COUNT = 4;
> > -
> > RkISP1CameraData *cameraData(const Camera *camera)
> > {
> > return static_cast<RkISP1CameraData *>(
> > @@ -88,6 +110,95 @@ private:
> > Camera *activeCamera_;
> > };
> >
> > +RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,
> > + RkISP1CameraData *data)
> > + : CameraConfiguration()
> > +{
> > + camera_ = camera->shared_from_this();
> > + data_ = data;
> > +}
> > +
> > +CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> > +{
> > + static const std::array<unsigned int, 8> formats{
> > + V4L2_PIX_FMT_YUYV,
> > + V4L2_PIX_FMT_YVYU,
> > + V4L2_PIX_FMT_VYUY,
> > + V4L2_PIX_FMT_NV16,
> > + V4L2_PIX_FMT_NV61,
> > + V4L2_PIX_FMT_NV21,
> > + V4L2_PIX_FMT_NV12,
> > + V4L2_PIX_FMT_GREY,
> > + };
> > +
> > + const CameraSensor *sensor = data_->sensor_;
> > + Status status = Valid;
> > +
> > + if (config_.empty())
> > + return Invalid;
> > +
> > + /* Cap the number of entries to the available streams. */
> > + if (config_.size() > 1) {
> > + config_.resize(1);
> > + status = Adjusted;
> > + }
> > +
> > + StreamConfiguration &cfg = config_[0];
> > +
> > + /* Adjust the pixel format. */
> > + if (std::find(formats.begin(), formats.end(), cfg.pixelFormat) ==
> > + formats.end()) {
> > + LOG(RkISP1, Debug) << "Adjusting format to NV12";
> > + cfg.pixelFormat = V4L2_PIX_FMT_NV12;
> > + status = Adjusted;
> > + }
> > +
> > + /* Select the sensor format. */
> > + sensorFormat_ = sensor->getFormat({ MEDIA_BUS_FMT_SBGGR12_1X12,
> > + MEDIA_BUS_FMT_SGBRG12_1X12,
> > + MEDIA_BUS_FMT_SGRBG12_1X12,
> > + MEDIA_BUS_FMT_SRGGB12_1X12,
> > + MEDIA_BUS_FMT_SBGGR10_1X10,
> > + MEDIA_BUS_FMT_SGBRG10_1X10,
> > + MEDIA_BUS_FMT_SGRBG10_1X10,
> > + MEDIA_BUS_FMT_SRGGB10_1X10,
> > + MEDIA_BUS_FMT_SBGGR8_1X8,
> > + MEDIA_BUS_FMT_SGBRG8_1X8,
> > + MEDIA_BUS_FMT_SGRBG8_1X8,
> > + MEDIA_BUS_FMT_SRGGB8_1X8 },
> > + cfg.size);
> > + if (!sensorFormat_.size.width || !sensorFormat_.size.height)
> > + sensorFormat_.size = sensor->resolution();
> > +
> > + /*
> > + * Provide a suitable default that matches the sensor aspect
> > + * ratio and clamp the size to the hardware bounds.
> > + *
> > + * \todo: Check the hardware alignment constraints.
> > + */
> > + const Size size = cfg.size;
> > +
> > + if (!cfg.size.width || !cfg.size.height) {
> > + cfg.size.width = 1280;
> > + cfg.size.height = 1280 * sensorFormat_.size.height
> > + / sensorFormat_.size.width;
> > + }
> > +
> > + cfg.size.width = std::max(32U, std::min(4416U, cfg.size.width));
> > + cfg.size.height = std::max(16U, std::min(3312U, cfg.size.height));
> > +
> > + if (cfg.size != size) {
> > + LOG(RkISP1, Debug)
> > + << "Adjusting size from " << size.toString()
> > + << " to " << cfg.size.toString();
> > + status = Adjusted;
> > + }
> > +
> > + cfg.bufferCount = RKISP1_BUFFER_COUNT;
> > +
> > + return status;
> > +}
> > +
> > PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)
> > : PipelineHandler(manager), dphy_(nullptr), isp_(nullptr),
> > video_(nullptr)
> > @@ -109,37 +220,31 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera
> > const StreamRoles &roles)
> > {
> > RkISP1CameraData *data = cameraData(camera);
> > - CameraConfiguration *config = new CameraConfiguration();
> > + CameraConfiguration *config = new RkISP1CameraConfiguration(camera, data);
> >
> > if (!roles.empty()) {
> > StreamConfiguration cfg{};
> >
> > cfg.pixelFormat = V4L2_PIX_FMT_NV12;
> > cfg.size = data->sensor_->resolution();
> > - cfg.bufferCount = RKISP1_BUFFER_COUNT;
> >
> > config->addConfiguration(cfg);
> > }
> >
> > + config->validate();
> > +
> > return config;
> > }
> >
> > -int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *config)
> > +int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
> > {
> > + RkISP1CameraConfiguration *config =
> > + static_cast<RkISP1CameraConfiguration *>(c);
> > RkISP1CameraData *data = cameraData(camera);
> > StreamConfiguration &cfg = config->at(0);
> > CameraSensor *sensor = data->sensor_;
> > int ret;
> >
> > - /* Verify the configuration. */
> > - const Size &resolution = sensor->resolution();
> > - if (cfg.size.width > resolution.width ||
> > - cfg.size.height > resolution.height) {
> > - LOG(RkISP1, Error)
> > - << "Invalid stream size: larger than sensor resolution";
> > - return -EINVAL;
> > - }
> > -
> > /*
> > * Configure the sensor links: enable the link corresponding to this
> > * camera and disable all the other sensor links.
> > @@ -167,21 +272,7 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *config
> > * Configure the format on the sensor output and propagate it through
> > * the pipeline.
> > */
> > - V4L2SubdeviceFormat format;
> > - format = sensor->getFormat({ MEDIA_BUS_FMT_SBGGR12_1X12,
> > - MEDIA_BUS_FMT_SGBRG12_1X12,
> > - MEDIA_BUS_FMT_SGRBG12_1X12,
> > - MEDIA_BUS_FMT_SRGGB12_1X12,
> > - MEDIA_BUS_FMT_SBGGR10_1X10,
> > - MEDIA_BUS_FMT_SGBRG10_1X10,
> > - MEDIA_BUS_FMT_SGRBG10_1X10,
> > - MEDIA_BUS_FMT_SRGGB10_1X10,
> > - MEDIA_BUS_FMT_SBGGR8_1X8,
> > - MEDIA_BUS_FMT_SGBRG8_1X8,
> > - MEDIA_BUS_FMT_SGRBG8_1X8,
> > - MEDIA_BUS_FMT_SRGGB8_1X8 },
> > - cfg.size);
> > -
> > + V4L2SubdeviceFormat format = config->sensorFormat();
> > LOG(RkISP1, Debug) << "Configuring sensor with " << format.toString();
> >
> > ret = sensor->setFormat(&format);
> > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> > index 8254e1fdac1e..f7ffeb439cf3 100644
> > --- a/src/libcamera/pipeline/uvcvideo.cpp
> > +++ b/src/libcamera/pipeline/uvcvideo.cpp
> > @@ -39,6 +39,14 @@ public:
> > Stream stream_;
> > };
> >
> > +class UVCCameraConfiguration : public CameraConfiguration
> > +{
> > +public:
> > + UVCCameraConfiguration();
> > +
> > + Status validate() override;
> > +};
> > +
> > class PipelineHandlerUVC : public PipelineHandler
> > {
> > public:
> > @@ -68,6 +76,45 @@ private:
> > }
> > };
> >
> > +UVCCameraConfiguration::UVCCameraConfiguration()
> > + : CameraConfiguration()
> > +{
> > +}
> > +
> > +CameraConfiguration::Status UVCCameraConfiguration::validate()
> > +{
> > + Status status = Valid;
> > +
> > + if (config_.empty())
> > + return Invalid;
> > +
> > + /* Cap the number of entries to the available streams. */
> > + if (config_.size() > 1) {
> > + config_.resize(1);
> > + status = Adjusted;
> > + }
> > +
> > + StreamConfiguration &cfg = config_[0];
> > +
> > + /* \todo: Validate the configuration against the device capabilities. */
> > + const unsigned int pixelFormat = cfg.pixelFormat;
> > + const Size size = cfg.size;
> > +
> > + cfg.pixelFormat = V4L2_PIX_FMT_YUYV;
> > + cfg.size = { 640, 480 };
> > +
> > + if (cfg.pixelFormat != pixelFormat || cfg.size != size) {
> > + LOG(UVC, Debug)
> > + << "Adjusting configuration from " << cfg.toString()
> > + << " to " << cfg.size.toString() << "-YUYV";
> > + status = Adjusted;
> > + }
> > +
> > + cfg.bufferCount = 4;
> > +
> > + return status;
> > +}
> > +
> > PipelineHandlerUVC::PipelineHandlerUVC(CameraManager *manager)
> > : PipelineHandler(manager)
> > {
> > @@ -76,10 +123,10 @@ PipelineHandlerUVC::PipelineHandlerUVC(CameraManager *manager)
> > CameraConfiguration *PipelineHandlerUVC::generateConfiguration(Camera *camera,
> > const StreamRoles &roles)
> > {
> > - CameraConfiguration *config = new CameraConfiguration();
> > + CameraConfiguration *config = new UVCCameraConfiguration();
> >
> > if (!roles.empty()) {
> > - StreamConfiguration cfg;
> > + StreamConfiguration cfg{};
> >
> > cfg.pixelFormat = V4L2_PIX_FMT_YUYV;
> > cfg.size = { 640, 480 };
> > @@ -88,6 +135,8 @@ CameraConfiguration *PipelineHandlerUVC::generateConfiguration(Camera *camera,
> > config->addConfiguration(cfg);
> > }
> >
> > + config->validate();
> > +
> > return config;
> > }
> >
> > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> > index 2bf85d0a0b32..2a61d2893e3a 100644
> > --- a/src/libcamera/pipeline/vimc.cpp
> > +++ b/src/libcamera/pipeline/vimc.cpp
> > @@ -5,6 +5,8 @@
> > * vimc.cpp - Pipeline handler for the vimc device
> > */
> >
> > +#include <algorithm>
> > +
> > #include <libcamera/camera.h>
> > #include <libcamera/request.h>
> > #include <libcamera/stream.h>
> > @@ -39,6 +41,14 @@ public:
> > Stream stream_;
> > };
> >
> > +class VimcCameraConfiguration : public CameraConfiguration
> > +{
> > +public:
> > + VimcCameraConfiguration();
> > +
> > + Status validate() override;
> > +};
> > +
> > class PipelineHandlerVimc : public PipelineHandler
> > {
> > public:
> > @@ -68,6 +78,57 @@ private:
> > }
> > };
> >
> > +VimcCameraConfiguration::VimcCameraConfiguration()
> > + : CameraConfiguration()
> > +{
> > +}
> > +
> > +CameraConfiguration::Status VimcCameraConfiguration::validate()
> > +{
> > + static const std::array<unsigned int, 3> formats{
> > + V4L2_PIX_FMT_BGR24,
> > + V4L2_PIX_FMT_RGB24,
> > + V4L2_PIX_FMT_ARGB32,
> > + };
> > +
> > + Status status = Valid;
> > +
> > + if (config_.empty())
> > + return Invalid;
> > +
> > + /* Cap the number of entries to the available streams. */
> > + if (config_.size() > 1) {
> > + config_.resize(1);
> > + status = Adjusted;
> > + }
> > +
> > + StreamConfiguration &cfg = config_[0];
> > +
> > + /* Adjust the pixel format. */
> > + if (std::find(formats.begin(), formats.end(), cfg.pixelFormat) ==
> > + formats.end()) {
> > + LOG(VIMC, Debug) << "Adjusting format to RGB24";
> > + cfg.pixelFormat = V4L2_PIX_FMT_RGB24;
> > + status = Adjusted;
> > + }
> > +
> > + /* Clamp the size based on the device limits. */
> > + const Size size = cfg.size;
> > +
> > + cfg.size.width = std::max(16U, std::min(4096U, cfg.size.width));
> > + cfg.size.height = std::max(16U, std::min(2160U, cfg.size.height));
> > +
> > + if (cfg.size != size) {
> > + LOG(VIMC, Debug)
> > + << "Adjusting size to " << cfg.size.toString();
> > + status = Adjusted;
> > + }
> > +
> > + cfg.bufferCount = 4;
> > +
> > + return status;
> > +}
> > +
> > PipelineHandlerVimc::PipelineHandlerVimc(CameraManager *manager)
> > : PipelineHandler(manager)
> > {
> > @@ -76,10 +137,10 @@ PipelineHandlerVimc::PipelineHandlerVimc(CameraManager *manager)
> > CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera,
> > const StreamRoles &roles)
> > {
> > - CameraConfiguration *config = new CameraConfiguration();
> > + CameraConfiguration *config = new VimcCameraConfiguration();
> >
> > if (!roles.empty()) {
> > - StreamConfiguration cfg;
> > + StreamConfiguration cfg{};
> >
> > cfg.pixelFormat = V4L2_PIX_FMT_RGB24;
> > cfg.size = { 640, 480 };
> > @@ -88,6 +149,8 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera,
> > config->addConfiguration(cfg);
> > }
> >
> > + config->validate();
> > +
> > return config;
> > }
> >
> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > index de46e98880a2..dd56907d817e 100644
> > --- a/src/libcamera/pipeline_handler.cpp
> > +++ b/src/libcamera/pipeline_handler.cpp
> > @@ -248,17 +248,14 @@ void PipelineHandler::unlock()
> > * is the Camera class which will receive configuration to apply from the
> > * application.
> > *
> > - * Each pipeline handler implementation is responsible for validating
> > - * that the configuration requested in \a config can be achieved
> > - * exactly. Any difference in pixel format, frame size or any other
> > - * parameter shall result in the -EINVAL error being returned, and no
> > - * change in configuration being applied to the pipeline. If
> > - * configuration of a subset of the streams can't be satisfied, the
> > - * whole configuration is considered invalid.
> > + * The configuration is guaranteed to have been validated with
> > + * CameraConfiguration::valid(). The pipeline handler implementation shall not
> > + * perform further validation and may rely on any custom field stored in its
> > + * custom CameraConfiguration derived class.
> > *
> > - * Once the configuration is validated and the camera configured, the pipeline
> > - * handler shall associate a Stream instance to each StreamConfiguration entry
> > - * in the CameraConfiguration with the StreamConfiguration::setStream() method.
> > + * When configuring the camera the pipeline handler shall associate a Stream
> > + * instance to each StreamConfiguration entry in the CameraConfiguration using
> > + * the StreamConfiguration::setStream() method.
> > *
> > * \return 0 on success or a negative error code otherwise
> > */
> > diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp
> > index 137aa649a505..f122f79bb1ec 100644
> > --- a/test/camera/capture.cpp
> > +++ b/test/camera/capture.cpp
> > @@ -45,7 +45,7 @@ protected:
> > CameraTest::init();
> >
> > config_ = camera_->generateConfiguration({ StreamRole::VideoRecording });
> > - if (!config_) {
> > + if (!config_ || config_->size() != 1) {
> > cout << "Failed to generate default configuration" << endl;
> > CameraTest::cleanup();
> > return TestFail;
> > @@ -58,11 +58,6 @@ protected:
> > {
> > StreamConfiguration &cfg = config_->at(0);
> >
> > - if (!config_->isValid()) {
> > - cout << "Failed to read default configuration" << endl;
> > - return TestFail;
> > - }
> > -
> > if (camera_->acquire()) {
> > cout << "Failed to acquire the camera" << endl;
> > return TestFail;
> > diff --git a/test/camera/configuration_default.cpp b/test/camera/configuration_default.cpp
> > index d5cefc1127c9..81055da1d513 100644
> > --- a/test/camera/configuration_default.cpp
> > +++ b/test/camera/configuration_default.cpp
> > @@ -22,7 +22,7 @@ protected:
> >
> > /* Test asking for configuration for a video stream. */
> > config = camera_->generateConfiguration({ StreamRole::VideoRecording });
> > - if (!config || !config->isValid()) {
> > + if (!config || config->size() != 1) {
> > cout << "Default configuration invalid" << endl;
> > delete config;
> > return TestFail;
> > @@ -35,7 +35,7 @@ protected:
> > * stream roles returns an empty camera configuration.
> > */
> > config = camera_->generateConfiguration({});
> > - if (!config || config->isValid()) {
> > + if (!config || config->size() != 0) {
> > cout << "Failed to retrieve configuration for empty roles list"
> > << endl;
> > delete config;
> > diff --git a/test/camera/configuration_set.cpp b/test/camera/configuration_set.cpp
> > index 23c611a93355..a4e2da16a88b 100644
> > --- a/test/camera/configuration_set.cpp
> > +++ b/test/camera/configuration_set.cpp
> > @@ -21,7 +21,7 @@ protected:
> > CameraTest::init();
> >
> > config_ = camera_->generateConfiguration({ StreamRole::VideoRecording });
> > - if (!config_) {
> > + if (!config_ || config_->size() != 1) {
> > cout << "Failed to generate default configuration" << endl;
> > CameraTest::cleanup();
> > return TestFail;
> > @@ -34,11 +34,6 @@ protected:
> > {
> > StreamConfiguration &cfg = config_->at(0);
> >
> > - if (!config_->isValid()) {
> > - cout << "Failed to read default configuration" << endl;
> > - return TestFail;
> > - }
> > -
> > if (camera_->acquire()) {
> > cout << "Failed to acquire the camera" << endl;
> > return TestFail;
> > --
> > Regards,
> >
> > Laurent Pinchart
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list