[libcamera-devel] [PATCH v2 4/6] libcamera: camera: Return a pointer from generateConfiguration()
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue May 21 16:15:45 CEST 2019
Hi Jacopo,
On Tue, May 21, 2019 at 11:52:44AM +0200, Jacopo Mondi wrote:
> On Sun, May 19, 2019 at 06:00:45PM +0300, Laurent Pinchart wrote:
> > To prepare for specialising the CameraConfiguration class in pipeline
> > handlers, return a pointer to a camera configuration instead of a
> > reference from Camera::generateConfiguration(). The camera configuration
> > always needs to be allocated from the pipeline handler, and its
> > ownership is passed to the application.
>
> Have you considered enforcing this returning CameraConfiguration as a
> unique_ptr<> to applications ? Ownership is passed to them at
> generateConfiguration() time, and then applications would pass a
> reference accessing the unique_ptr<> at configure() time.
It's a good idea, I hadn't thought about it. The code looks cleaner with
that change (9 files changed, 27 insertions(+), 55 deletions(-)).
> After all, there is no other way from an application to get back the
> same CameraConfiguration from the Camera, so once it has been
> returned, it's responsability of the application to keep a valid
> reference to it. Also, to make it easier for app to deal with that,
> we could abstract it with a d-pointer later (if appropriate)
>
> > For symmetry, change Camera::configure() to take a CameraConfiguration
> > pointer instead of a reference. This aligns with our coding practice of
> > passing parameters that are modified by the callee by pointer.
>
> With unique_ptr this would be Camera::configure(config.get())
>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > include/libcamera/camera.h | 4 +--
> > src/cam/main.cpp | 40 ++++++++++-----------
> > src/libcamera/camera.cpp | 31 +++++++++--------
> > src/libcamera/include/pipeline_handler.h | 6 ++--
> > src/libcamera/pipeline/ipu3/ipu3.cpp | 31 +++++++++--------
> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 25 ++++++++------
> > src/libcamera/pipeline/uvcvideo.cpp | 30 ++++++++--------
> > src/libcamera/pipeline/vimc.cpp | 30 ++++++++--------
> > src/libcamera/pipeline_handler.cpp | 3 +-
> > src/qcam/main_window.cpp | 5 ++-
> > src/qcam/main_window.h | 2 +-
> > test/camera/capture.cpp | 38 +++++++++++++++-----
> > test/camera/configuration_default.cpp | 14 +++++---
> > test/camera/configuration_set.cpp | 44 +++++++++++++++++-------
> > test/camera/statemachine.cpp | 22 ++++++++++--
> > 15 files changed, 202 insertions(+), 123 deletions(-)
> >
> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > index 284e5276a055..144133c5de9f 100644
> > --- a/include/libcamera/camera.h
> > +++ b/include/libcamera/camera.h
> > @@ -77,8 +77,8 @@ public:
> > int release();
> >
> > const std::set<Stream *> &streams() const;
> > - CameraConfiguration generateConfiguration(const StreamRoles &roles);
> > - int configure(CameraConfiguration &config);
> > + CameraConfiguration *generateConfiguration(const StreamRoles &roles);
> > + int configure(CameraConfiguration *config);
> >
> > int allocateBuffers();
> > int freeBuffers();
> > diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> > index cd165bea34cd..7550ae4f3428 100644
> > --- a/src/cam/main.cpp
> > +++ b/src/cam/main.cpp
> > @@ -85,15 +85,13 @@ static int parseOptions(int argc, char *argv[])
> > return 0;
> > }
> >
> > -static int prepareCameraConfig(CameraConfiguration *config)
> > +static CameraConfiguration *prepareCameraConfig()
> > {
> > StreamRoles roles;
> >
> > /* If no configuration is provided assume a single video stream. */
> > - if (!options.isSet(OptStream)) {
> > - *config = camera->generateConfiguration({ StreamRole::VideoRecording });
> > - return 0;
> > - }
> > + if (!options.isSet(OptStream))
> > + return camera->generateConfiguration({ StreamRole::VideoRecording });
> >
> > const std::vector<OptionValue> &streamOptions =
> > options[OptStream].toArray();
> > @@ -113,23 +111,23 @@ static int prepareCameraConfig(CameraConfiguration *config)
> > } else {
> > std::cerr << "Unknown stream role "
> > << conf["role"].toString() << std::endl;
> > - return -EINVAL;
> > + return nullptr;
> > }
> > }
> >
> > - *config = camera->generateConfiguration(roles);
> > -
> > - if (!config->isValid()) {
> > + CameraConfiguration *config = camera->generateConfiguration(roles);
> > + if (!config || !config->isValid()) {
> > std::cerr << "Failed to get default stream configuration"
> > << std::endl;
> > - return -EINVAL;
> > + delete config;
> > + return nullptr;
> > }
> >
> > /* Apply configuration explicitly requested. */
> > unsigned int i = 0;
> > for (auto const &value : streamOptions) {
> > KeyValueParser::Options conf = value.toKeyValues();
> > - StreamConfiguration &cfg = (*config)[i++];
> > + StreamConfiguration &cfg = config->at(i++);
>
> I curious to know why at() here? Just for style reasons ?
Yes, just for style reason. If you convince Niklas that (*config)[] is
fine, I'll switch back to it and drop the at() method.
> >
> > if (conf.isSet("width"))
> > cfg.size.width = conf["width"];
> > @@ -142,7 +140,7 @@ static int prepareCameraConfig(CameraConfiguration *config)
> > cfg.pixelFormat = conf["pixelformat"];
> > }
> >
> > - return 0;
> > + return config;
> > }
> >
> > static void requestComplete(Request *request, const std::map<Stream *, Buffer *> &buffers)
> > @@ -191,25 +189,25 @@ static void requestComplete(Request *request, const std::map<Stream *, Buffer *>
> >
> > static int capture()
> > {
> > - CameraConfiguration config;
> > int ret;
> >
> > - ret = prepareCameraConfig(&config);
> > - if (ret) {
> > + CameraConfiguration *config = prepareCameraConfig();
> > + if (!config) {
> > std::cout << "Failed to prepare camera configuration" << std::endl;
> > - return ret;
> > + return -EINVAL;
> > }
> >
> > ret = camera->configure(config);
> > if (ret < 0) {
> > std::cout << "Failed to configure camera" << std::endl;
> > + delete config;
> > return ret;
> > }
> >
> > streamInfo.clear();
> >
> > - for (unsigned int index = 0; index < config.size(); ++index) {
> > - StreamConfiguration &cfg = config[index];
> > + for (unsigned int index = 0; index < config->size(); ++index) {
> > + StreamConfiguration &cfg = config->at(index);
> > streamInfo[cfg.stream()] = "stream" + std::to_string(index);
> > }
> >
> > @@ -217,6 +215,7 @@ static int capture()
> > if (ret) {
> > std::cerr << "Failed to allocate buffers"
> > << std::endl;
> > + delete config;
>
> using unique_ptr<> would avoid application having to do this. It comes
> at a cost of a more verbose API indeed.
>
> > return ret;
> > }
> >
> > @@ -224,7 +223,7 @@ static int capture()
> >
> > /* Identify the stream with the least number of buffers. */
> > unsigned int nbuffers = UINT_MAX;
> > - for (StreamConfiguration &cfg : config) {
> > + for (StreamConfiguration &cfg : *config) {
> > Stream *stream = cfg.stream();
> > nbuffers = std::min(nbuffers, stream->bufferPool().count());
> > }
> > @@ -244,7 +243,7 @@ static int capture()
> > }
> >
> > std::map<Stream *, Buffer *> map;
> > - for (StreamConfiguration &cfg : config) {
> > + for (StreamConfiguration &cfg : *config) {
> > Stream *stream = cfg.stream();
> > map[stream] = &stream->bufferPool().buffers()[i];
> > }
> > @@ -280,6 +279,7 @@ static int capture()
> > std::cout << "Failed to stop capture" << std::endl;
> > out:
> > camera->freeBuffers();
> > + delete config;
> >
> > return ret;
> > }
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index 5848330f639a..0e80691ed862 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -542,21 +542,24 @@ const std::set<Stream *> &Camera::streams() const
> > * specifies a list of stream roles and the camera returns a configuration
> > * containing suitable streams and their suggested default configurations.
> > *
> > - * \return A valid CameraConfiguration if the requested roles can be satisfied,
> > - * or a invalid one otherwise
> > + * \return A CameraConfiguration if the requested roles can be satisfied, or a
> > + * null pointer otherwise. The ownership of the returned configuration is
> > + * passed to the caller.
> > */
> > -CameraConfiguration
> > -Camera::generateConfiguration(const StreamRoles &roles)
> > +CameraConfiguration *Camera::generateConfiguration(const StreamRoles &roles)
> > {
> > - if (disconnected_ || !roles.size() || roles.size() > streams_.size())
> > - return CameraConfiguration();
> > + if (disconnected_ || roles.size() > streams_.size())
>
> I might have missed it, but I would document that we accept empty
> StreamRoles to generate an empty CameraConfiguration to be filled by
> applications.
I'll fix that.
> I'm not sure but if that's the case, we could check here and return an
> here instantiated empty CameraConfiguration without even going to the
> pipeline handlers.
We can't, as that empty configuration should get filled by applications,
and then validated. We need a specialised instance of
CameraConfiguration for that.
> > + return nullptr;
> >
> > - CameraConfiguration config = pipe_->generateConfiguration(this, roles);
> > + CameraConfiguration *config = pipe_->generateConfiguration(this, roles);
> >
> > std::ostringstream msg("streams configuration:", std::ios_base::ate);
> >
> > - for (unsigned int index = 0; index < config.size(); ++index)
> > - msg << " (" << index << ") " << config[index].toString();
> > + if (config->empty())
> > + msg << " empty";
> > +
> > + for (unsigned int index = 0; index < config->size(); ++index)
> > + msg << " (" << index << ") " << config->at(index).toString();
> >
> > LOG(Camera, Debug) << msg.str();
> >
> > @@ -590,7 +593,7 @@ Camera::generateConfiguration(const StreamRoles &roles)
> > * \retval -EACCES The camera is not in a state where it can be configured
> > * \retval -EINVAL The configuration is not valid
> > */
> > -int Camera::configure(CameraConfiguration &config)
> > +int Camera::configure(CameraConfiguration *config)
> > {
> > int ret;
> >
> > @@ -600,7 +603,7 @@ int Camera::configure(CameraConfiguration &config)
> > if (!stateBetween(CameraAcquired, CameraConfigured))
> > return -EACCES;
> >
> > - if (!config.isValid()) {
> > + if (!config->isValid()) {
> > LOG(Camera, Error)
> > << "Can't configure camera with invalid configuration";
> > return -EINVAL;
> > @@ -608,8 +611,8 @@ int Camera::configure(CameraConfiguration &config)
> >
> > std::ostringstream msg("configuring streams:", std::ios_base::ate);
> >
> > - for (unsigned int index = 0; index < config.size(); ++index) {
> > - StreamConfiguration &cfg = config[index];
> > + for (unsigned int index = 0; index < config->size(); ++index) {
> > + StreamConfiguration &cfg = config->at(index);
> > cfg.setStream(nullptr);
> > msg << " (" << index << ") " << cfg.toString();
> > }
> > @@ -621,7 +624,7 @@ int Camera::configure(CameraConfiguration &config)
> > return ret;
> >
> > activeStreams_.clear();
> > - for (const StreamConfiguration &cfg : config) {
> > + for (const StreamConfiguration &cfg : *config) {
> > Stream *stream = cfg.stream();
> > if (!stream)
> > LOG(Camera, Fatal)
> > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> > index a025905ab68f..7da6df1ab2a0 100644
> > --- a/src/libcamera/include/pipeline_handler.h
> > +++ b/src/libcamera/include/pipeline_handler.h
> > @@ -60,9 +60,9 @@ public:
> > bool lock();
> > void unlock();
> >
> > - virtual CameraConfiguration
> > - generateConfiguration(Camera *camera, const StreamRoles &roles) = 0;
> > - virtual int configure(Camera *camera, CameraConfiguration &config) = 0;
> > + virtual CameraConfiguration *generateConfiguration(Camera *camera,
> > + const StreamRoles &roles) = 0;
> > + virtual int configure(Camera *camera, CameraConfiguration *config) = 0;
> >
> > virtual int allocateBuffers(Camera *camera,
> > const std::set<Stream *> &streams) = 0;
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index ed0ef69de1d1..3acf82ff4665 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -150,9 +150,9 @@ class PipelineHandlerIPU3 : public PipelineHandler
> > public:
> > PipelineHandlerIPU3(CameraManager *manager);
> >
> > - CameraConfiguration
> > - generateConfiguration(Camera *camera, const StreamRoles &roles) override;
> > - int configure(Camera *camera, CameraConfiguration &config) override;
> > + CameraConfiguration *generateConfiguration(Camera *camera,
> > + const StreamRoles &roles) override;
> > + int configure(Camera *camera, CameraConfiguration *config) override;
> >
> > int allocateBuffers(Camera *camera,
> > const std::set<Stream *> &streams) override;
> > @@ -207,12 +207,11 @@ PipelineHandlerIPU3::PipelineHandlerIPU3(CameraManager *manager)
> > {
> > }
> >
> > -CameraConfiguration
> > -PipelineHandlerIPU3::generateConfiguration(Camera *camera,
> > - const StreamRoles &roles)
> > +CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
> > + const StreamRoles &roles)
>
> nit: I would keep this aligned with the first parameter
I'm also in two minds about that, when the method name is long, all
parameters are pushed back far to the right. I don't think the style
used here is perfect, but it has the advantage of being consistently
usable regardless of how long the method name is.
> > {
> > IPU3CameraData *data = cameraData(camera);
> > - CameraConfiguration config = {};
> > + CameraConfiguration *config = new CameraConfiguration();
> > std::set<IPU3Stream *> streams = {
> > &data->outStream_,
> > &data->vfStream_,
> > @@ -290,21 +289,23 @@ PipelineHandlerIPU3::generateConfiguration(Camera *camera,
> > break;
> > }
> >
> > - if (!stream)
> > - return CameraConfiguration{};
> > + if (!stream) {
> > + delete config;
> > + return nullptr;
> > + }
> >
> > streams.erase(stream);
> >
> > cfg.pixelFormat = V4L2_PIX_FMT_NV12;
> > cfg.bufferCount = IPU3_BUFFER_COUNT;
> >
> > - config.addConfiguration(cfg);
> > + config->addConfiguration(cfg);
> > }
> >
> > return config;
> > }
> >
> > -int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration &config)
> > +int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *config)
> > {
> > IPU3CameraData *data = cameraData(camera);
> > IPU3Stream *outStream = &data->outStream_;
> > @@ -316,7 +317,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration &config)
> >
> > outStream->active_ = false;
> > vfStream->active_ = false;
> > - for (StreamConfiguration &cfg : config) {
> > + for (StreamConfiguration &cfg : *config) {
> > /*
> > * Pick a stream for the configuration entry.
> > * \todo: This is a naive temporary implementation that will be
> > @@ -382,7 +383,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration &config)
> > return ret;
> >
> > /* Apply the format to the configured streams output devices. */
> > - for (StreamConfiguration &cfg : config) {
> > + for (StreamConfiguration &cfg : *config) {
> > IPU3Stream *stream = static_cast<IPU3Stream *>(cfg.stream());
> > ret = imgu->configureOutput(stream->device_, cfg);
> > if (ret)
> > @@ -395,13 +396,13 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration &config)
> > * be at least one active stream in the configuration request).
> > */
> > if (!outStream->active_) {
> > - ret = imgu->configureOutput(outStream->device_, config[0]);
> > + ret = imgu->configureOutput(outStream->device_, config->at(0));
> > if (ret)
> > return ret;
> > }
> >
> > if (!vfStream->active_) {
> > - ret = imgu->configureOutput(vfStream->device_, config[0]);
> > + ret = imgu->configureOutput(vfStream->device_, config->at(0));
> > if (ret)
> > return ret;
> > }
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index ec6980b0943a..01ba50f09db1 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -34,9 +34,9 @@ public:
> > PipelineHandlerRkISP1(CameraManager *manager);
> > ~PipelineHandlerRkISP1();
> >
> > - CameraConfiguration generateConfiguration(Camera *camera,
> > + CameraConfiguration *generateConfiguration(Camera *camera,
> > const StreamRoles &roles) override;
> > - int configure(Camera *camera, CameraConfiguration &config) override;
> > + int configure(Camera *camera, CameraConfiguration *config) override;
> >
> > int allocateBuffers(Camera *camera,
> > const std::set<Stream *> &streams) override;
> > @@ -105,26 +105,29 @@ PipelineHandlerRkISP1::~PipelineHandlerRkISP1()
> > * Pipeline Operations
> > */
> >
> > -CameraConfiguration PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
> > +CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
> > const StreamRoles &roles)
> > {
> > RkISP1CameraData *data = cameraData(camera);
> > - CameraConfiguration config;
> > - StreamConfiguration cfg{};
> > + CameraConfiguration *config = new CameraConfiguration();
> >
> > - cfg.pixelFormat = V4L2_PIX_FMT_NV12;
> > - cfg.size = data->sensor_->resolution();
> > - cfg.bufferCount = RKISP1_BUFFER_COUNT;
> > + if (!roles.empty()) {
> > + StreamConfiguration cfg{};
>
> Same comment as Niklas here, with the additional point of moving this
> check to Camera::generateConfiguration()
>
> > - config.addConfiguration(cfg);
> > + cfg.pixelFormat = V4L2_PIX_FMT_NV12;
> > + cfg.size = data->sensor_->resolution();
> > + cfg.bufferCount = RKISP1_BUFFER_COUNT;
> > +
> > + config->addConfiguration(cfg);
> > + }
> >
> > return config;
> > }
> >
> > -int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration &config)
> > +int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *config)
> > {
> > RkISP1CameraData *data = cameraData(camera);
> > - StreamConfiguration &cfg = config[0];
> > + StreamConfiguration &cfg = config->at(0);
> > CameraSensor *sensor = data->sensor_;
> > int ret;
> >
> > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> > index 5dcc868b2fc9..91b4065c250b 100644
> > --- a/src/libcamera/pipeline/uvcvideo.cpp
> > +++ b/src/libcamera/pipeline/uvcvideo.cpp
> > @@ -25,9 +25,9 @@ class PipelineHandlerUVC : public PipelineHandler
> > public:
> > PipelineHandlerUVC(CameraManager *manager);
> >
> > - CameraConfiguration
> > - generateConfiguration(Camera *camera, const StreamRoles &roles) override;
> > - int configure(Camera *camera, CameraConfiguration &config) override;
> > + CameraConfiguration *generateConfiguration(Camera *camera,
> > + const StreamRoles &roles) override;
> > + int configure(Camera *camera, CameraConfiguration *config) override;
> >
> > int allocateBuffers(Camera *camera,
> > const std::set<Stream *> &streams) override;
> > @@ -73,26 +73,28 @@ PipelineHandlerUVC::PipelineHandlerUVC(CameraManager *manager)
> > {
> > }
> >
> > -CameraConfiguration
> > -PipelineHandlerUVC::generateConfiguration(Camera *camera,
> > - const StreamRoles &roles)
> > +CameraConfiguration *PipelineHandlerUVC::generateConfiguration(Camera *camera,
> > + const StreamRoles &roles)
> > {
> > - CameraConfiguration config;
> > - StreamConfiguration cfg;
> > + CameraConfiguration *config = new CameraConfiguration();
> >
> > - cfg.pixelFormat = V4L2_PIX_FMT_YUYV;
> > - cfg.size = { 640, 480 };
> > - cfg.bufferCount = 4;
> > + if (!roles.empty()) {
> > + StreamConfiguration cfg;
> >
> > - config.addConfiguration(cfg);
> > + cfg.pixelFormat = V4L2_PIX_FMT_YUYV;
> > + cfg.size = { 640, 480 };
> > + cfg.bufferCount = 4;
> > +
> > + config->addConfiguration(cfg);
> > + }
> >
> > return config;
> > }
> >
> > -int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration &config)
> > +int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration *config)
> > {
> > UVCCameraData *data = cameraData(camera);
> > - StreamConfiguration &cfg = config[0];
> > + StreamConfiguration &cfg = config->at(0);
> > int ret;
> >
> > V4L2DeviceFormat format = {};
> > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> > index af6b6f21e3c5..510e6c082f13 100644
> > --- a/src/libcamera/pipeline/vimc.cpp
> > +++ b/src/libcamera/pipeline/vimc.cpp
> > @@ -25,9 +25,9 @@ class PipelineHandlerVimc : public PipelineHandler
> > public:
> > PipelineHandlerVimc(CameraManager *manager);
> >
> > - CameraConfiguration
> > - generateConfiguration(Camera *camera, const StreamRoles &roles) override;
> > - int configure(Camera *camera, CameraConfiguration &config) override;
> > + CameraConfiguration *generateConfiguration(Camera *camera,
> > + const StreamRoles &roles) override;
> > + int configure(Camera *camera, CameraConfiguration *config) override;
> >
> > int allocateBuffers(Camera *camera,
> > const std::set<Stream *> &streams) override;
> > @@ -73,26 +73,28 @@ PipelineHandlerVimc::PipelineHandlerVimc(CameraManager *manager)
> > {
> > }
> >
> > -CameraConfiguration
> > -PipelineHandlerVimc::generateConfiguration(Camera *camera,
> > - const StreamRoles &roles)
> > +CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera,
> > + const StreamRoles &roles)
> > {
> > - CameraConfiguration config;
> > - StreamConfiguration cfg;
> > + CameraConfiguration *config = new CameraConfiguration();
> >
> > - cfg.pixelFormat = V4L2_PIX_FMT_RGB24;
> > - cfg.size = { 640, 480 };
> > - cfg.bufferCount = 4;
> > + if (!roles.empty()) {
> > + StreamConfiguration cfg;
> >
> > - config.addConfiguration(cfg);
> > + cfg.pixelFormat = V4L2_PIX_FMT_RGB24;
> > + cfg.size = { 640, 480 };
> > + cfg.bufferCount = 4;
> > +
> > + config->addConfiguration(cfg);
> > + }
> >
> > return config;
> > }
> >
> > -int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration &config)
> > +int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config)
> > {
> > VimcCameraData *data = cameraData(camera);
> > - StreamConfiguration &cfg = config[0];
> > + StreamConfiguration &cfg = config->at(0);
> > int ret;
> >
> > V4L2DeviceFormat format = {};
> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > index 4185e3557dcb..de46e98880a2 100644
> > --- a/src/libcamera/pipeline_handler.cpp
> > +++ b/src/libcamera/pipeline_handler.cpp
> > @@ -233,7 +233,8 @@ void PipelineHandler::unlock()
> > * the group of streams parameters.
> > *
> > * \return A valid CameraConfiguration if the requested roles can be satisfied,
> > - * or a invalid configuration otherwise
> > + * or a null pointer otherwise. The ownership of the returned configuration is
> > + * passed to the caller.
> > */
> >
> > /**
> > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> > index 06ae2985f80d..3ae5aae9cc76 100644
> > --- a/src/qcam/main_window.cpp
> > +++ b/src/qcam/main_window.cpp
> > @@ -104,7 +104,7 @@ int MainWindow::startCapture()
> > return ret;
> > }
> >
> > - const StreamConfiguration &cfg = config_[0];
> > + const StreamConfiguration &cfg = config_->at(0);
> > Stream *stream = cfg.stream();
> > ret = viewfinder_->setFormat(cfg.pixelFormat, cfg.size.width,
> > cfg.size.height);
> > @@ -180,6 +180,9 @@ void MainWindow::stopCapture()
> >
> > camera_->freeBuffers();
> > isCapturing_ = false;
> > +
> > + delete config_;
> > + config_ = nullptr;
> > }
> >
> > void MainWindow::requestComplete(Request *request,
> > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> > index 143b5b08a5a0..866866e93d23 100644
> > --- a/src/qcam/main_window.h
> > +++ b/src/qcam/main_window.h
> > @@ -45,7 +45,7 @@ private:
> >
> > std::shared_ptr<Camera> camera_;
> > bool isCapturing_;
> > - CameraConfiguration config_;
> > + CameraConfiguration *config_;
> >
> > ViewFinder *viewfinder_;
> > };
> > diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp
> > index bfd11eefedcf..137aa649a505 100644
> > --- a/test/camera/capture.cpp
> > +++ b/test/camera/capture.cpp
> > @@ -40,13 +40,25 @@ protected:
> > camera_->queueRequest(request);
> > }
> >
> > - int run()
> > + int init() override
> > {
> > - CameraConfiguration config =
> > - camera_->generateConfiguration({ StreamRole::VideoRecording });
> > - StreamConfiguration *cfg = &config[0];
> > + CameraTest::init();
> >
> > - if (!config.isValid()) {
> > + config_ = camera_->generateConfiguration({ StreamRole::VideoRecording });
> > + if (!config_) {
> > + cout << "Failed to generate default configuration" << endl;
> > + CameraTest::cleanup();
> > + return TestFail;
> > + }
> > +
> > + return TestPass;
> > + }
> > +
> > + int run() override
> > + {
> > + StreamConfiguration &cfg = config_->at(0);
> > +
> > + if (!config_->isValid()) {
> > cout << "Failed to read default configuration" << endl;
> > return TestFail;
> > }
> > @@ -56,7 +68,7 @@ protected:
> > return TestFail;
> > }
> >
> > - if (camera_->configure(config)) {
> > + if (camera_->configure(config_)) {
> > cout << "Failed to set default configuration" << endl;
> > return TestFail;
> > }
> > @@ -66,7 +78,7 @@ protected:
> > return TestFail;
> > }
> >
> > - Stream *stream = cfg->stream();
> > + Stream *stream = cfg.stream();
> > BufferPool &pool = stream->bufferPool();
> > std::vector<Request *> requests;
> > for (Buffer &buffer : pool.buffers()) {
> > @@ -110,10 +122,10 @@ protected:
> > while (timer.isRunning())
> > dispatcher->processEvents();
> >
> > - if (completeRequestsCount_ <= cfg->bufferCount * 2) {
> > + if (completeRequestsCount_ <= cfg.bufferCount * 2) {
> > cout << "Failed to capture enough frames (got "
> > << completeRequestsCount_ << " expected at least "
> > - << cfg->bufferCount * 2 << ")" << endl;
> > + << cfg.bufferCount * 2 << ")" << endl;
> > return TestFail;
> > }
> >
> > @@ -134,6 +146,14 @@ protected:
> >
> > return TestPass;
> > }
> > +
> > + void cleanup() override
> > + {
> > + delete config_;
> > + CameraTest::cleanup();
> > + }
> > +
> > + CameraConfiguration *config_;
> > };
> >
> > } /* namespace */
> > diff --git a/test/camera/configuration_default.cpp b/test/camera/configuration_default.cpp
> > index 8c4a03db498a..d5cefc1127c9 100644
> > --- a/test/camera/configuration_default.cpp
> > +++ b/test/camera/configuration_default.cpp
> > @@ -18,26 +18,32 @@ class ConfigurationDefault : public CameraTest
> > protected:
> > int run()
> > {
> > - CameraConfiguration config;
> > + CameraConfiguration *config;
> >
> > /* Test asking for configuration for a video stream. */
> > config = camera_->generateConfiguration({ StreamRole::VideoRecording });
> > - if (!config.isValid()) {
> > + if (!config || !config->isValid()) {
> > cout << "Default configuration invalid" << endl;
> > + delete config;
> > return TestFail;
> > }
> >
> > + delete config;
> > +
> > /*
> > * Test that asking for configuration for an empty array of
> > - * stream roles returns an empty list of configurations.
> > + * stream roles returns an empty camera configuration.
> > */
> > config = camera_->generateConfiguration({});
> > - if (config.isValid()) {
> > + if (!config || config->isValid()) {
> > cout << "Failed to retrieve configuration for empty roles list"
> > << endl;
> > + delete config;
> > return TestFail;
> > }
> >
> > + delete config;
> > +
> > return TestPass;
> > }
> > };
> > diff --git a/test/camera/configuration_set.cpp b/test/camera/configuration_set.cpp
> > index 25b5db67103a..23c611a93355 100644
> > --- a/test/camera/configuration_set.cpp
> > +++ b/test/camera/configuration_set.cpp
> > @@ -16,13 +16,25 @@ namespace {
> > class ConfigurationSet : public CameraTest
> > {
> > protected:
> > - int run()
> > + int init() override
> > {
> > - CameraConfiguration config =
> > - camera_->generateConfiguration({ StreamRole::VideoRecording });
> > - StreamConfiguration *cfg = &config[0];
> > + CameraTest::init();
> >
> > - if (!config.isValid()) {
> > + config_ = camera_->generateConfiguration({ StreamRole::VideoRecording });
> > + if (!config_) {
> > + cout << "Failed to generate default configuration" << endl;
> > + CameraTest::cleanup();
> > + return TestFail;
> > + }
> > +
> > + return TestPass;
> > + }
> > +
> > + int run() override
> > + {
> > + StreamConfiguration &cfg = config_->at(0);
> > +
> > + if (!config_->isValid()) {
> > cout << "Failed to read default configuration" << endl;
> > return TestFail;
> > }
> > @@ -33,7 +45,7 @@ protected:
> > }
> >
> > /* Test that setting the default configuration works. */
> > - if (camera_->configure(config)) {
> > + if (camera_->configure(config_)) {
> > cout << "Failed to set default configuration" << endl;
> > return TestFail;
> > }
> > @@ -48,7 +60,7 @@ protected:
> > return TestFail;
> > }
> >
> > - if (!camera_->configure(config)) {
> > + if (!camera_->configure(config_)) {
> > cout << "Setting configuration on a camera not acquired succeeded when it should have failed"
> > << endl;
> > return TestFail;
> > @@ -64,9 +76,9 @@ protected:
> > * the default configuration of the VIMC camera is known to
> > * work.
> > */
> > - cfg->size.width *= 2;
> > - cfg->size.height *= 2;
> > - if (camera_->configure(config)) {
> > + cfg.size.width *= 2;
> > + cfg.size.height *= 2;
> > + if (camera_->configure(config_)) {
> > cout << "Failed to set modified configuration" << endl;
> > return TestFail;
> > }
> > @@ -74,14 +86,22 @@ protected:
> > /*
> > * Test that setting an invalid configuration fails.
> > */
> > - cfg->size = { 0, 0 };
> > - if (!camera_->configure(config)) {
> > + cfg.size = { 0, 0 };
> > + if (!camera_->configure(config_)) {
> > cout << "Invalid configuration incorrectly accepted" << endl;
> > return TestFail;
> > }
> >
> > return TestPass;
> > }
> > +
> > + void cleanup() override
> > + {
> > + delete config_;
> > + CameraTest::cleanup();
> > + }
> > +
> > + CameraConfiguration *config_;
> > };
> >
> > } /* namespace */
> > diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp
> > index 7a74cd85a37a..a662e869fadf 100644
> > --- a/test/camera/statemachine.cpp
> > +++ b/test/camera/statemachine.cpp
> > @@ -233,10 +233,22 @@ protected:
> > return TestPass;
> > }
> >
> > - int run()
> > + int init() override
> > {
> > + CameraTest::init();
> > +
> > defconf_ = camera_->generateConfiguration({ StreamRole::VideoRecording });
> > + if (!defconf_) {
> > + cout << "Failed to generate default configuration" << endl;
> > + CameraTest::cleanup();
> > + return TestFail;
> > + }
> >
> > + return TestPass;
> > + }
> > +
> > + int run() override
> > + {
> > if (testAvailable() != TestPass) {
> > cout << "State machine in Available state failed" << endl;
> > return TestFail;
> > @@ -265,7 +277,13 @@ protected:
> > return TestPass;
> > }
> >
> > - CameraConfiguration defconf_;
> > + void cleanup() override
> > + {
> > + delete defconf_;
> > + CameraTest::cleanup();
> > + }
> > +
> > + CameraConfiguration *defconf_;
> > };
> >
> > } /* namespace */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list