[libcamera-devel] [PATCH v2 6/6] libcamera: camera: Add a validation API to the CameraConfiguration class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed May 22 18:19:08 CEST 2019
Hi Niklas,
On Wed, May 22, 2019 at 12:55:16PM +0200, Niklas Söderlund wrote:
> On 2019-05-21 15:51:09 +0300, Laurent Pinchart wrote:
> > On Tue, May 21, 2019 at 10:49:50AM +0200, Jacopo Mondi wrote:
> >> 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 ?
>
> For cam in it's current form there is no strict need to call validate at
> this point. The current design is that cam should fail if the format is
> adjusted. We might wish to change that in the future (like I do in some
> patch IIRC).
>
> If you wish to squash that change here I'm fine doing so. I think
> however that it's fine for this patch to move forward with or without
> that change.
Then let's do it on top.
> >> (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
More information about the libcamera-devel
mailing list