[libcamera-devel] [PATCH v2 6/6] libcamera: camera: Add a validation API to the CameraConfiguration class
Niklas Söderlund
niklas.soderlund at ragnatech.se
Wed May 22 13:02:30 CEST 2019
On 2019-05-21 15:49:55 +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Tue, May 21, 2019 at 12:05:06AM +0200, Jacopo Mondi wrote:
> > Hi Laurent,
> >
> > In order to validate if the API is dump-proof enough, I'm starting
> > the review from the last patch, so I might be missing some pieces
> > here and there...
> >
> > 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.
> >
> > s/semantics/semantic ?
>
> I think you're right, I'll fix that.
>
> > > 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<>.
> >
> > If I got this right we'll have a
> >
> > CameraConfiguration::validate() and a
> > Camera.configure()
> >
> > and CameraConfiguration has to keep a reference to the Camera to
> > access it.
> >
> > Can we provide a Camera::validate(StreamConfiguration *) instead?
> > We could handle the shared reference in the Camera and not let
> > CameraConfiguration subclasses have to deal with it in this way?
>
> I assume you meant a Camera::validate(CameraConfiguration *), as we want
> to validate the whole configuration, not stream per stream (the idea of
> this series is to support cross-stream validation constraints).
>
> First of all, the method would need to be called
> Camera::validateConfiguration(). This would need to be delegated to
> PipelineHandler::validateConfiguration(Camera *, CameraConfiguration *).
> The pipeline handler would then need to cast the camera configuration to
> its custom configuration derived class (opening the door to applications
> calling Camera::validateConfiguration() with a configuration for the
> wrong camera, but that's already possible with Camera::configure() I
> supposed). We could try that, but I think it would make the API less
> clean for applications, for little benefits in my opinion (but we may
> have a different view of the benefits :-)).
>
> > >
> > > 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
> > > @@ -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 ?
> >
> > I'm not sure I did get why it gets in the way of configure()
>
> Because Camera::configure() calls validate() and returns an error if the
> status is Adjusted or Invalid, as the application is responsible for
> passing a fully valid configuration to Camera::configure().
>
> > > + *
> > > + * \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.
> >
> > nit: instead of describing how the returned configuration looks like,
> > do you have an example of application provided parameters which might
> > trigger and invalid use case ?
>
> Not at the moment, no. I have no other case in mind, but we may find
> some in the future.
>
> > > + * \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)
> >
> > Could you pass here the StreanConfiguration & instead of i ?
>
> I had another use for the index before, now that it has gone, I'll pass
> the stream configuration.
>
> > > +{
> > > + 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;
> >
> > Same question as below, is it safe to use the sensor provided sizes?
> > In here, in example, 2592 is a multiple of 8, but I'm not sure it
> > works well.
> >
> > (sorry, you should read below first)
>
> I'll reply below :-)
>
> > > + 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];
> >
> > for (StreamConfiguration &cfg : config_) ?
>
> You answered this yourself below :-)
>
> > > + const unsigned int pixelFormat = cfg.pixelFormat;
> > > + const Size size = cfg.size;
> > > + const IPU3Stream *stream;
> > > +
> > > + if (cfg.size == sensorFormat_.size)
> >
> > (here, more precisely)
> >
> > I'm not sure about this. I always seen the IPU3 used with maximum size
> > (for ov5670) as 2560x1920 while the sensor resolution is actually a
> > bit larger 2592x1944. Same for the back ov13858 camera. I never had the
> > full sensor resolution working properly and I assumed it was related to
> > some IPU3 alignement or constraints I didn't know about. Also, the
> > Intel provided xml configuration file does limit the maximum
> > available resolution to 2560x1920, but that could be for other reasons
> > maybe... Have you tested capture at sensor resolution sizes?
>
> Not yet. I'm 100% confident that this code isn't good enough :-) What
> we're missing is documentation from Intel, and then I'll fix it.
>
> > > + stream = &data_->outStream_;
> > > + else
> > > + stream = &data_->vfStream_;
> > > +
> > > + if (availableStreams.find(stream) == availableStreams.end())
> > > + stream = *availableStreams.begin();
> >
> > This works as long as we have 2 streams only, maybe a todo ?
> >
> > > +
> > > + LOG(IPU3, Debug)
> > > + << "Assigned '" << stream->name_ << "' to stream " << i;
> >
> > Ah, maybe it's better not to use the range-based for loop, you need 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();
> > > +
> >
> > If the idea of providing a Camera::validate(CameraConfiguration *) has
> > any ground, we should then have a private CameraConfiguration::validate()
> > operation accessible by the Camera class through a friend declarator,
> > so that pipeline handlers implementation cannot call it, restricting
> > that method to the application facing API only. The
>
> That's not very neat, as it would polute the application-facing headers
> with friends :-( I think we should try to remove them instead.
I agree that we should try to remove friend statements wherever
possible. And once we are a bit more API stable maybe even set out to
redesign parts to get rid of them completely ;-)
>
> > CameraConfiguration subclasses' validate() implementation could then
> > call into the same operation that pipeline handler would use here at
> > generateConfiguration() time as you did here, if desirable.
> >
> > Also, again if Camera::validate(CameraConfiguration *) makes any
> > sense, we might want to group basic validations, if any. So we could
> > make Camera::validate() call into the private CameraConfiguration::validate()
> > operation, which performs basic operations, and calls into a protected
> > virtual PipelineHandler::__validate() (or whatever name is more
> > appropriate)
> >
> > Would this be doable?
>
> If you manage to find a good API that I like, sure :-) What you describe
> here make the API more cumbersome in my opinion.
>
> > > 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];
> >
> > nit: the fact you can get both Stream * and StreamConfiguration * by index
> > it's a bit ugly. What about config->streams(i) and
> > config->configuration(i).
> >
> > I think what's fishy lies in the fact IPu3CameraConfiguration indexes both
> > Stream and Configurations in two different vactors and both accessed by
> > index. Do you need Streams in CameraConfiguration ? Can't you store
> > them in CameraData, as 'activeStreams_' maybe?
>
> I can't, because configuration objects are separated from the active
> state of the camera. You can create as many configuration you want, toy
> with them in any way, and finally destroy them, without any side effect.
> The API guarantees that Camera::configure() will associate streams with
> configuration entries, but I think pipeline handlers should be able to
> do it ahead of time too, in validate(), if it's easier for them. I thus
> don't think that storing that information in CameraData is a good idea.
>
> Furthermore, I would like to move the Stream class away from the
> application-facing API, so more refactoring is to come. I could however,
> in the meantime, replace IPU3CameraConfiguration::streams() with
> IPU3CameraConfiguration::stream(unsigned int index) if you think that's
> desirable (let's remember that that method is private to the IPU3), but
> a CameraConfiguration::configuration(unsigned int index) would make the
> API more complex for applications I think (it would need to be called
> CameraConfiguration::streamConfiguration(), and wouldn't let
> applications iterate over the stream configurations using a range-based
> loop).
>
> > > +
> > > + 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
> >
> > An impressive rework, I like where it's going even if it puts
> > a bit of a burden on pipeline handlers, it's for the benefit of the
> > application facing APIs.
>
> Thank you.
>
> > > @@ -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,
Niklas Söderlund
More information about the libcamera-devel
mailing list