[libcamera-devel] [PATCH v3 6/6] libcamera: camera: Add a validation API to the CameraConfiguration class
Jacopo Mondi
jacopo at jmondi.org
Tue May 21 23:53:40 CEST 2019
Hi Laurent,
On Tue, May 21, 2019 at 10:27:40PM +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 semantic 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>
> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
With comments on IPU3 roles assignement clarified and the API
discussion finalized in one way or another:
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
Thanks
j
> ---
> Changes since v2:
>
> - Pass StreamConfiguration & to IPU3CameraConfiguration::adjustStream()
> - Refactor generateConfiguration() to save one indentation level in
> pipeline handlers
> - Include <array> where needed
> ---
> include/libcamera/camera.h | 17 +-
> src/cam/main.cpp | 2 +-
> src/libcamera/camera.cpp | 80 ++++---
> src/libcamera/pipeline/ipu3/ipu3.cpp | 253 ++++++++++++++++++-----
> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 150 +++++++++++---
> src/libcamera/pipeline/uvcvideo.cpp | 51 ++++-
> src/libcamera/pipeline/vimc.cpp | 66 +++++-
> 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, 514 insertions(+), 140 deletions(-)
>
> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> index a3a7289a7aa7..fb2f7ba3423c 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 535c2420893e..5ecd7e0e38d7 100644
> --- a/src/cam/main.cpp
> +++ b/src/cam/main.cpp
> @@ -116,7 +116,7 @@ static std::unique_ptr<CameraConfiguration> prepareCameraConfig()
> }
>
> std::unique_ptr<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;
> return nullptr;
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 0e5fd7f57271..b25a80bce1a2 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
> @@ -577,10 +612,9 @@ std::unique_ptr<CameraConfiguration> Camera::generateConfiguration(const StreamR
> * 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.
> @@ -605,7 +639,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..05005c42106b 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(StreamConfiguration &cfg, 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,151 @@ private:
> MediaDevice *imguMediaDev_;
> };
>
> +IPU3CameraConfiguration::IPU3CameraConfiguration(Camera *camera,
> + IPU3CameraData *data)
> + : CameraConfiguration()
> +{
> + camera_ = camera->shared_from_this();
> + data_ = data;
> +}
> +
> +void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale)
> +{
> + /* 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(config_[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 +381,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 +468,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 +499,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 +510,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 8b279e76c0b0..9b3eea2f6dd3 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -5,6 +5,8 @@
> * rkisp1.cpp - Pipeline handler for Rockchip ISP1
> */
>
> +#include <algorithm>
> +#include <array>
> #include <iomanip>
> #include <memory>
> #include <vector>
> @@ -45,6 +47,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 +93,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 +111,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,7 +221,7 @@ 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())
> return config;
> @@ -117,29 +229,23 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera
> 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 +273,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 4ffe52aa70d7..45260f34c8f5 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,7 +123,7 @@ PipelineHandlerUVC::PipelineHandlerUVC(CameraManager *manager)
> CameraConfiguration *PipelineHandlerUVC::generateConfiguration(Camera *camera,
> const StreamRoles &roles)
> {
> - CameraConfiguration *config = new CameraConfiguration();
> + CameraConfiguration *config = new UVCCameraConfiguration();
>
> if (roles.empty())
> return config;
> @@ -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 ed5b1ad4502a..0e4eede351d8 100644
> --- a/src/libcamera/pipeline/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc.cpp
> @@ -5,6 +5,9 @@
> * vimc.cpp - Pipeline handler for the vimc device
> */
>
> +#include <algorithm>
> +#include <array>
> +
> #include <libcamera/camera.h>
> #include <libcamera/request.h>
> #include <libcamera/stream.h>
> @@ -39,6 +42,14 @@ public:
> Stream stream_;
> };
>
> +class VimcCameraConfiguration : public CameraConfiguration
> +{
> +public:
> + VimcCameraConfiguration();
> +
> + Status validate() override;
> +};
> +
> class PipelineHandlerVimc : public PipelineHandler
> {
> public:
> @@ -68,6 +79,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,7 +138,7 @@ PipelineHandlerVimc::PipelineHandlerVimc(CameraManager *manager)
> CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera,
> const StreamRoles &roles)
> {
> - CameraConfiguration *config = new CameraConfiguration();
> + CameraConfiguration *config = new VimcCameraConfiguration();
>
> if (roles.empty())
> return config;
> @@ -88,6 +150,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 bb7d380cdc1a..c0835c250c65 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 8a767d2321e0..ce2ec5d02e7b 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;
> return TestFail;
> }
> @@ -32,7 +32,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;
> return TestFail;
> diff --git a/test/camera/configuration_set.cpp b/test/camera/configuration_set.cpp
> index ece987c7752a..9f10f795a5d8 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190521/185e2380/attachment-0001.sig>
More information about the libcamera-devel
mailing list