[libcamera-devel] [PATCH/RFC 05/12] libcamera: camera: Return a pointer from generateConfiguration()
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat May 18 19:22:24 CEST 2019
Hi Niklas,
On Sat, May 18, 2019 at 06:41:27PM +0200, Niklas Söderlund wrote:
> On 2019-05-18 02:06:14 +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.
>
> I understand the need for pipeline handlers to be able to specialise the
> CameraConfiguration, at the same time I'm not super happy with how this
> change effects the API towards applications. Specially as we model the
> CameraConfiguration around a vector.
>
> CameraConfiguration *config = ... prepareCameraConfig();
> StreamConfiguration &cfg = (*config)[index];
>
> Is not the most intuitive interface ;-)
I've seen worse :-) A vector pointer isn't that bad.
> I wonder if we could improve here using a d-pointer design and a private
> pointer with a reference count, or maybe even use a shared_ptr<>
> internally?
If we do this I fear it will make the interface towards pipeline
handlers much more complex. We will need to allow copies of
CameraConfiguration, which internally will need to duplicate the
pipeline-handler specific data stored in the CameraConfiguration. I
don't really see how we could keep it clean and simple. Of course, feel
free to prove me wrong :-)
> > 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.
>
> This agree with and should be changed disregarding of the above, and
> maybe even moved to a patch (or it's own patch) earlier in this series
> where 'const' is dropped from the argument.
Let's first see if we find a good way to return configurations by value
instead of pointer. If we don't I think we can bundle the two changes in
this patch.
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > include/libcamera/camera.h | 4 +--
> > src/cam/main.cpp | 38 ++++++++++----------
> > 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, 201 insertions(+), 122 deletions(-)
> >
> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > index e2759405f497..841e8fc505b9 100644
> > --- a/include/libcamera/camera.h
> > +++ b/include/libcamera/camera.h
> > @@ -68,8 +68,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..7a1b332f68c7 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,16 +111,16 @@ 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. */
> > @@ -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)[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;
> > 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 f8a4946b4a6a..115cdb1c024b 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -518,21 +518,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())
> > + 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->isEmpty())
> > + msg << " empty";
> > +
> > + for (unsigned int index = 0; index < config->size(); ++index)
> > + msg << " (" << index << ") " << (*config)[index].toString();
> >
> > LOG(Camera, Debug) << msg.str();
> >
> > @@ -566,7 +569,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;
> >
> > @@ -576,7 +579,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;
> > @@ -584,8 +587,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)[index];
> > cfg.setStream(nullptr);
> > msg << " (" << index << ") " << cfg.toString();
> > }
> > @@ -597,7 +600,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..7c6f2d4a23be 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)
> > {
> > 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)[0]);
> > if (ret)
> > return ret;
> > }
> >
> > if (!vfStream->active_) {
> > - ret = imgu->configureOutput(vfStream->device_, config[0]);
> > + ret = imgu->configureOutput(vfStream->device_, (*config)[0]);
> > if (ret)
> > return ret;
> > }
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 6b35aa3fe7c3..c3b3912c96f3 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{};
> >
> > - 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)[0];
> > CameraSensor *sensor = data->sensor_;
> > int ret;
> >
> > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> > index 5dcc868b2fc9..5c061ca61016 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)[0];
> > int ret;
> >
> > V4L2DeviceFormat format = {};
> > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> > index af6b6f21e3c5..0ece97f09e7e 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)[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..9f0454b7a5f3 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_)[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..f640e80fbaf3 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_)[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..ca911f459c32 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_)[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