[{"id":1616,"web_url":"https://patchwork.libcamera.org/comment/1616/","msgid":"<20190518164127.GG25081@bigcity.dyn.berto.se>","date":"2019-05-18T16:41:27","subject":"Re: [libcamera-devel] [PATCH/RFC 05/12] libcamera: camera: Return a\n\tpointer from generateConfiguration()","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your work.\n\nOn 2019-05-18 02:06:14 +0300, Laurent Pinchart wrote:\n> To prepare for specialising the CameraConfiguration class in pipeline\n> handlers, return a pointer to a camera configuration instead of a\n> reference from Camera::generateConfiguration(). The camera configuration\n> always needs to be allocated from the pipeline handler, and its\n> ownership is passed to the application.\n\nI understand the need for pipeline handlers to be able to specialise the \nCameraConfiguration, at the same time I'm not super happy with how this \nchange effects the API towards applications. Specially as we model the \nCameraConfiguration around a vector.\n\n    CameraConfiguration *config = ... prepareCameraConfig();\n    StreamConfiguration &cfg = (*config)[index];\n\nIs not the most intuitive interface ;-)\n\nI wonder if we could improve here using a d-pointer design and a private \npointer with a reference count, or maybe even use a shared_ptr<> \ninternally?\n\n> \n> For symmetry, change Camera::configure() to take a CameraConfiguration\n> pointer instead of a reference. This aligns with our coding practice of\n> passing parameters that are modified by the callee by pointer.\n\nThis agree with and should be changed disregarding of the above, and \nmaybe even moved to a patch (or it's own patch) earlier in this series\nwhere 'const' is dropped from the argument.\n\n> \n> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> ---\n>  include/libcamera/camera.h               |  4 +--\n>  src/cam/main.cpp                         | 38 ++++++++++----------\n>  src/libcamera/camera.cpp                 | 31 +++++++++--------\n>  src/libcamera/include/pipeline_handler.h |  6 ++--\n>  src/libcamera/pipeline/ipu3/ipu3.cpp     | 31 +++++++++--------\n>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 25 ++++++++------\n>  src/libcamera/pipeline/uvcvideo.cpp      | 30 ++++++++--------\n>  src/libcamera/pipeline/vimc.cpp          | 30 ++++++++--------\n>  src/libcamera/pipeline_handler.cpp       |  3 +-\n>  src/qcam/main_window.cpp                 |  5 ++-\n>  src/qcam/main_window.h                   |  2 +-\n>  test/camera/capture.cpp                  | 38 +++++++++++++++-----\n>  test/camera/configuration_default.cpp    | 14 +++++---\n>  test/camera/configuration_set.cpp        | 44 +++++++++++++++++-------\n>  test/camera/statemachine.cpp             | 22 ++++++++++--\n>  15 files changed, 201 insertions(+), 122 deletions(-)\n> \n> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> index e2759405f497..841e8fc505b9 100644\n> --- a/include/libcamera/camera.h\n> +++ b/include/libcamera/camera.h\n> @@ -68,8 +68,8 @@ public:\n>  \tint release();\n>  \n>  \tconst std::set<Stream *> &streams() const;\n> -\tCameraConfiguration generateConfiguration(const StreamRoles &roles);\n> -\tint configure(CameraConfiguration &config);\n> +\tCameraConfiguration *generateConfiguration(const StreamRoles &roles);\n> +\tint configure(CameraConfiguration *config);\n>  \n>  \tint allocateBuffers();\n>  \tint freeBuffers();\n> diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> index cd165bea34cd..7a1b332f68c7 100644\n> --- a/src/cam/main.cpp\n> +++ b/src/cam/main.cpp\n> @@ -85,15 +85,13 @@ static int parseOptions(int argc, char *argv[])\n>  \treturn 0;\n>  }\n>  \n> -static int prepareCameraConfig(CameraConfiguration *config)\n> +static CameraConfiguration *prepareCameraConfig()\n>  {\n>  \tStreamRoles roles;\n>  \n>  \t/* If no configuration is provided assume a single video stream. */\n> -\tif (!options.isSet(OptStream)) {\n> -\t\t*config = camera->generateConfiguration({ StreamRole::VideoRecording });\n> -\t\treturn 0;\n> -\t}\n> +\tif (!options.isSet(OptStream))\n> +\t\treturn camera->generateConfiguration({ StreamRole::VideoRecording });\n>  \n>  \tconst std::vector<OptionValue> &streamOptions =\n>  \t\toptions[OptStream].toArray();\n> @@ -113,16 +111,16 @@ static int prepareCameraConfig(CameraConfiguration *config)\n>  \t\t} else {\n>  \t\t\tstd::cerr << \"Unknown stream role \"\n>  \t\t\t\t  << conf[\"role\"].toString() << std::endl;\n> -\t\t\treturn -EINVAL;\n> +\t\t\treturn nullptr;\n>  \t\t}\n>  \t}\n>  \n> -\t*config = camera->generateConfiguration(roles);\n> -\n> -\tif (!config->isValid()) {\n> +\tCameraConfiguration *config = camera->generateConfiguration(roles);\n> +\tif (!config || !config->isValid()) {\n>  \t\tstd::cerr << \"Failed to get default stream configuration\"\n>  \t\t\t  << std::endl;\n> -\t\treturn -EINVAL;\n> +\t\tdelete config;\n> +\t\treturn nullptr;\n>  \t}\n>  \n>  \t/* Apply configuration explicitly requested. */\n> @@ -142,7 +140,7 @@ static int prepareCameraConfig(CameraConfiguration *config)\n>  \t\t\tcfg.pixelFormat = conf[\"pixelformat\"];\n>  \t}\n>  \n> -\treturn 0;\n> +\treturn config;\n>  }\n>  \n>  static void requestComplete(Request *request, const std::map<Stream *, Buffer *> &buffers)\n> @@ -191,25 +189,25 @@ static void requestComplete(Request *request, const std::map<Stream *, Buffer *>\n>  \n>  static int capture()\n>  {\n> -\tCameraConfiguration config;\n>  \tint ret;\n>  \n> -\tret = prepareCameraConfig(&config);\n> -\tif (ret) {\n> +\tCameraConfiguration *config = prepareCameraConfig();\n> +\tif (!config) {\n>  \t\tstd::cout << \"Failed to prepare camera configuration\" << std::endl;\n> -\t\treturn ret;\n> +\t\treturn -EINVAL;\n>  \t}\n>  \n>  \tret = camera->configure(config);\n>  \tif (ret < 0) {\n>  \t\tstd::cout << \"Failed to configure camera\" << std::endl;\n> +\t\tdelete config;\n>  \t\treturn ret;\n>  \t}\n>  \n>  \tstreamInfo.clear();\n>  \n> -\tfor (unsigned int index = 0; index < config.size(); ++index) {\n> -\t\tStreamConfiguration &cfg = config[index];\n> +\tfor (unsigned int index = 0; index < config->size(); ++index) {\n> +\t\tStreamConfiguration &cfg = (*config)[index];\n>  \t\tstreamInfo[cfg.stream()] = \"stream\" + std::to_string(index);\n>  \t}\n>  \n> @@ -217,6 +215,7 @@ static int capture()\n>  \tif (ret) {\n>  \t\tstd::cerr << \"Failed to allocate buffers\"\n>  \t\t\t  << std::endl;\n> +\t\tdelete config;\n>  \t\treturn ret;\n>  \t}\n>  \n> @@ -224,7 +223,7 @@ static int capture()\n>  \n>  \t/* Identify the stream with the least number of buffers. */\n>  \tunsigned int nbuffers = UINT_MAX;\n> -\tfor (StreamConfiguration &cfg : config) {\n> +\tfor (StreamConfiguration &cfg : *config) {\n>  \t\tStream *stream = cfg.stream();\n>  \t\tnbuffers = std::min(nbuffers, stream->bufferPool().count());\n>  \t}\n> @@ -244,7 +243,7 @@ static int capture()\n>  \t\t}\n>  \n>  \t\tstd::map<Stream *, Buffer *> map;\n> -\t\tfor (StreamConfiguration &cfg : config) {\n> +\t\tfor (StreamConfiguration &cfg : *config) {\n>  \t\t\tStream *stream = cfg.stream();\n>  \t\t\tmap[stream] = &stream->bufferPool().buffers()[i];\n>  \t\t}\n> @@ -280,6 +279,7 @@ static int capture()\n>  \t\tstd::cout << \"Failed to stop capture\" << std::endl;\n>  out:\n>  \tcamera->freeBuffers();\n> +\tdelete config;\n>  \n>  \treturn ret;\n>  }\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index f8a4946b4a6a..115cdb1c024b 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -518,21 +518,24 @@ const std::set<Stream *> &Camera::streams() const\n>   * specifies a list of stream roles and the camera returns a configuration\n>   * containing suitable streams and their suggested default configurations.\n>   *\n> - * \\return A valid CameraConfiguration if the requested roles can be satisfied,\n> - * or a invalid one otherwise\n> + * \\return A CameraConfiguration if the requested roles can be satisfied, or a\n> + * null pointer otherwise. The ownership of the returned configuration is\n> + * passed to the caller.\n>   */\n> -CameraConfiguration\n> -Camera::generateConfiguration(const StreamRoles &roles)\n> +CameraConfiguration *Camera::generateConfiguration(const StreamRoles &roles)\n>  {\n> -\tif (disconnected_ || !roles.size() || roles.size() > streams_.size())\n> -\t\treturn CameraConfiguration();\n> +\tif (disconnected_ || roles.size() > streams_.size())\n> +\t\treturn nullptr;\n>  \n> -\tCameraConfiguration config = pipe_->generateConfiguration(this, roles);\n> +\tCameraConfiguration *config = pipe_->generateConfiguration(this, roles);\n>  \n>  \tstd::ostringstream msg(\"streams configuration:\", std::ios_base::ate);\n>  \n> -\tfor (unsigned int index = 0; index < config.size(); ++index)\n> -\t\tmsg << \" (\" << index << \") \" << config[index].toString();\n> +\tif (config->isEmpty())\n> +\t\tmsg << \" empty\";\n> +\n> +\tfor (unsigned int index = 0; index < config->size(); ++index)\n> +\t\tmsg << \" (\" << index << \") \" << (*config)[index].toString();\n>  \n>  \tLOG(Camera, Debug) << msg.str();\n>  \n> @@ -566,7 +569,7 @@ Camera::generateConfiguration(const StreamRoles &roles)\n>   * \\retval -EACCES The camera is not in a state where it can be configured\n>   * \\retval -EINVAL The configuration is not valid\n>   */\n> -int Camera::configure(CameraConfiguration &config)\n> +int Camera::configure(CameraConfiguration *config)\n>  {\n>  \tint ret;\n>  \n> @@ -576,7 +579,7 @@ int Camera::configure(CameraConfiguration &config)\n>  \tif (!stateBetween(CameraAcquired, CameraConfigured))\n>  \t\treturn -EACCES;\n>  \n> -\tif (!config.isValid()) {\n> +\tif (!config->isValid()) {\n>  \t\tLOG(Camera, Error)\n>  \t\t\t<< \"Can't configure camera with invalid configuration\";\n>  \t\treturn -EINVAL;\n> @@ -584,8 +587,8 @@ int Camera::configure(CameraConfiguration &config)\n>  \n>  \tstd::ostringstream msg(\"configuring streams:\", std::ios_base::ate);\n>  \n> -\tfor (unsigned int index = 0; index < config.size(); ++index) {\n> -\t\tStreamConfiguration &cfg = config[index];\n> +\tfor (unsigned int index = 0; index < config->size(); ++index) {\n> +\t\tStreamConfiguration &cfg = (*config)[index];\n>  \t\tcfg.setStream(nullptr);\n>  \t\tmsg << \" (\" << index << \") \" << cfg.toString();\n>  \t}\n> @@ -597,7 +600,7 @@ int Camera::configure(CameraConfiguration &config)\n>  \t\treturn ret;\n>  \n>  \tactiveStreams_.clear();\n> -\tfor (const StreamConfiguration &cfg : config) {\n> +\tfor (const StreamConfiguration &cfg : *config) {\n>  \t\tStream *stream = cfg.stream();\n>  \t\tif (!stream)\n>  \t\t\tLOG(Camera, Fatal)\n> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> index a025905ab68f..7da6df1ab2a0 100644\n> --- a/src/libcamera/include/pipeline_handler.h\n> +++ b/src/libcamera/include/pipeline_handler.h\n> @@ -60,9 +60,9 @@ public:\n>  \tbool lock();\n>  \tvoid unlock();\n>  \n> -\tvirtual CameraConfiguration\n> -\tgenerateConfiguration(Camera *camera, const StreamRoles &roles) = 0;\n> -\tvirtual int configure(Camera *camera, CameraConfiguration &config) = 0;\n> +\tvirtual CameraConfiguration *generateConfiguration(Camera *camera,\n> +\t\tconst StreamRoles &roles) = 0;\n> +\tvirtual int configure(Camera *camera, CameraConfiguration *config) = 0;\n>  \n>  \tvirtual int allocateBuffers(Camera *camera,\n>  \t\t\t\t    const std::set<Stream *> &streams) = 0;\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index ed0ef69de1d1..7c6f2d4a23be 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -150,9 +150,9 @@ class PipelineHandlerIPU3 : public PipelineHandler\n>  public:\n>  \tPipelineHandlerIPU3(CameraManager *manager);\n>  \n> -\tCameraConfiguration\n> -\tgenerateConfiguration(Camera *camera, const StreamRoles &roles) override;\n> -\tint configure(Camera *camera, CameraConfiguration &config) override;\n> +\tCameraConfiguration *generateConfiguration(Camera *camera,\n> +\t\tconst StreamRoles &roles) override;\n> +\tint configure(Camera *camera, CameraConfiguration *config) override;\n>  \n>  \tint allocateBuffers(Camera *camera,\n>  \t\t\t    const std::set<Stream *> &streams) override;\n> @@ -207,12 +207,11 @@ PipelineHandlerIPU3::PipelineHandlerIPU3(CameraManager *manager)\n>  {\n>  }\n>  \n> -CameraConfiguration\n> -PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n> -\t\t\t\t\t   const StreamRoles &roles)\n> +CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n> +\tconst StreamRoles &roles)\n>  {\n>  \tIPU3CameraData *data = cameraData(camera);\n> -\tCameraConfiguration config = {};\n> +\tCameraConfiguration *config = new CameraConfiguration();\n>  \tstd::set<IPU3Stream *> streams = {\n>  \t\t&data->outStream_,\n>  \t\t&data->vfStream_,\n> @@ -290,21 +289,23 @@ PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n>  \t\t\tbreak;\n>  \t\t}\n>  \n> -\t\tif (!stream)\n> -\t\t\treturn CameraConfiguration{};\n> +\t\tif (!stream) {\n> +\t\t\tdelete config;\n> +\t\t\treturn nullptr;\n> +\t\t}\n>  \n>  \t\tstreams.erase(stream);\n>  \n>  \t\tcfg.pixelFormat = V4L2_PIX_FMT_NV12;\n>  \t\tcfg.bufferCount = IPU3_BUFFER_COUNT;\n>  \n> -\t\tconfig.addConfiguration(cfg);\n> +\t\tconfig->addConfiguration(cfg);\n>  \t}\n>  \n>  \treturn config;\n>  }\n>  \n> -int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration &config)\n> +int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *config)\n>  {\n>  \tIPU3CameraData *data = cameraData(camera);\n>  \tIPU3Stream *outStream = &data->outStream_;\n> @@ -316,7 +317,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration &config)\n>  \n>  \toutStream->active_ = false;\n>  \tvfStream->active_ = false;\n> -\tfor (StreamConfiguration &cfg : config) {\n> +\tfor (StreamConfiguration &cfg : *config) {\n>  \t\t/*\n>  \t\t * Pick a stream for the configuration entry.\n>  \t\t * \\todo: This is a naive temporary implementation that will be\n> @@ -382,7 +383,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration &config)\n>  \t\treturn ret;\n>  \n>  \t/* Apply the format to the configured streams output devices. */\n> -\tfor (StreamConfiguration &cfg : config) {\n> +\tfor (StreamConfiguration &cfg : *config) {\n>  \t\tIPU3Stream *stream = static_cast<IPU3Stream *>(cfg.stream());\n>  \t\tret = imgu->configureOutput(stream->device_, cfg);\n>  \t\tif (ret)\n> @@ -395,13 +396,13 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration &config)\n>  \t * be at least one active stream in the configuration request).\n>  \t */\n>  \tif (!outStream->active_) {\n> -\t\tret = imgu->configureOutput(outStream->device_, config[0]);\n> +\t\tret = imgu->configureOutput(outStream->device_, (*config)[0]);\n>  \t\tif (ret)\n>  \t\t\treturn ret;\n>  \t}\n>  \n>  \tif (!vfStream->active_) {\n> -\t\tret = imgu->configureOutput(vfStream->device_, config[0]);\n> +\t\tret = imgu->configureOutput(vfStream->device_, (*config)[0]);\n>  \t\tif (ret)\n>  \t\t\treturn ret;\n>  \t}\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 6b35aa3fe7c3..c3b3912c96f3 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -34,9 +34,9 @@ public:\n>  \tPipelineHandlerRkISP1(CameraManager *manager);\n>  \t~PipelineHandlerRkISP1();\n>  \n> -\tCameraConfiguration generateConfiguration(Camera *camera,\n> +\tCameraConfiguration *generateConfiguration(Camera *camera,\n>  \t\tconst StreamRoles &roles) override;\n> -\tint configure(Camera *camera, CameraConfiguration &config) override;\n> +\tint configure(Camera *camera, CameraConfiguration *config) override;\n>  \n>  \tint allocateBuffers(Camera *camera,\n>  \t\tconst std::set<Stream *> &streams) override;\n> @@ -105,26 +105,29 @@ PipelineHandlerRkISP1::~PipelineHandlerRkISP1()\n>   * Pipeline Operations\n>   */\n>  \n> -CameraConfiguration PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n> +CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n>  \tconst StreamRoles &roles)\n>  {\n>  \tRkISP1CameraData *data = cameraData(camera);\n> -\tCameraConfiguration config;\n> -\tStreamConfiguration cfg{};\n> +\tCameraConfiguration *config = new CameraConfiguration();\n>  \n> -\tcfg.pixelFormat = V4L2_PIX_FMT_NV12;\n> -\tcfg.size = data->sensor_->resolution();\n> -\tcfg.bufferCount = RKISP1_BUFFER_COUNT;\n> +\tif (!roles.empty()) {\n> +\t\tStreamConfiguration cfg{};\n>  \n> -\tconfig.addConfiguration(cfg);\n> +\t\tcfg.pixelFormat = V4L2_PIX_FMT_NV12;\n> +\t\tcfg.size = data->sensor_->resolution();\n> +\t\tcfg.bufferCount = RKISP1_BUFFER_COUNT;\n> +\n> +\t\tconfig->addConfiguration(cfg);\n> +\t}\n>  \n>  \treturn config;\n>  }\n>  \n> -int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration &config)\n> +int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *config)\n>  {\n>  \tRkISP1CameraData *data = cameraData(camera);\n> -\tStreamConfiguration &cfg = config[0];\n> +\tStreamConfiguration &cfg = (*config)[0];\n>  \tCameraSensor *sensor = data->sensor_;\n>  \tint ret;\n>  \n> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> index 5dcc868b2fc9..5c061ca61016 100644\n> --- a/src/libcamera/pipeline/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> @@ -25,9 +25,9 @@ class PipelineHandlerUVC : public PipelineHandler\n>  public:\n>  \tPipelineHandlerUVC(CameraManager *manager);\n>  \n> -\tCameraConfiguration\n> -\tgenerateConfiguration(Camera *camera, const StreamRoles &roles) override;\n> -\tint configure(Camera *camera, CameraConfiguration &config) override;\n> +\tCameraConfiguration *generateConfiguration(Camera *camera,\n> +\t\tconst StreamRoles &roles) override;\n> +\tint configure(Camera *camera, CameraConfiguration *config) override;\n>  \n>  \tint allocateBuffers(Camera *camera,\n>  \t\t\t    const std::set<Stream *> &streams) override;\n> @@ -73,26 +73,28 @@ PipelineHandlerUVC::PipelineHandlerUVC(CameraManager *manager)\n>  {\n>  }\n>  \n> -CameraConfiguration\n> -PipelineHandlerUVC::generateConfiguration(Camera *camera,\n> -\t\t\t\t\t  const StreamRoles &roles)\n> +CameraConfiguration *PipelineHandlerUVC::generateConfiguration(Camera *camera,\n> +\tconst StreamRoles &roles)\n>  {\n> -\tCameraConfiguration config;\n> -\tStreamConfiguration cfg;\n> +\tCameraConfiguration *config = new CameraConfiguration();\n>  \n> -\tcfg.pixelFormat = V4L2_PIX_FMT_YUYV;\n> -\tcfg.size = { 640, 480 };\n> -\tcfg.bufferCount = 4;\n> +\tif (!roles.empty()) {\n> +\t\tStreamConfiguration cfg;\n>  \n> -\tconfig.addConfiguration(cfg);\n> +\t\tcfg.pixelFormat = V4L2_PIX_FMT_YUYV;\n> +\t\tcfg.size = { 640, 480 };\n> +\t\tcfg.bufferCount = 4;\n> +\n> +\t\tconfig->addConfiguration(cfg);\n> +\t}\n>  \n>  \treturn config;\n>  }\n>  \n> -int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration &config)\n> +int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration *config)\n>  {\n>  \tUVCCameraData *data = cameraData(camera);\n> -\tStreamConfiguration &cfg = config[0];\n> +\tStreamConfiguration &cfg = (*config)[0];\n>  \tint ret;\n>  \n>  \tV4L2DeviceFormat format = {};\n> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> index af6b6f21e3c5..0ece97f09e7e 100644\n> --- a/src/libcamera/pipeline/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc.cpp\n> @@ -25,9 +25,9 @@ class PipelineHandlerVimc : public PipelineHandler\n>  public:\n>  \tPipelineHandlerVimc(CameraManager *manager);\n>  \n> -\tCameraConfiguration\n> -\tgenerateConfiguration(Camera *camera, const StreamRoles &roles) override;\n> -\tint configure(Camera *camera, CameraConfiguration &config) override;\n> +\tCameraConfiguration *generateConfiguration(Camera *camera,\n> +\t\tconst StreamRoles &roles) override;\n> +\tint configure(Camera *camera, CameraConfiguration *config) override;\n>  \n>  \tint allocateBuffers(Camera *camera,\n>  \t\t\t    const std::set<Stream *> &streams) override;\n> @@ -73,26 +73,28 @@ PipelineHandlerVimc::PipelineHandlerVimc(CameraManager *manager)\n>  {\n>  }\n>  \n> -CameraConfiguration\n> -PipelineHandlerVimc::generateConfiguration(Camera *camera,\n> -\t\t\t\t\t   const StreamRoles &roles)\n> +CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera,\n> +\tconst StreamRoles &roles)\n>  {\n> -\tCameraConfiguration config;\n> -\tStreamConfiguration cfg;\n> +\tCameraConfiguration *config = new CameraConfiguration();\n>  \n> -\tcfg.pixelFormat = V4L2_PIX_FMT_RGB24;\n> -\tcfg.size = { 640, 480 };\n> -\tcfg.bufferCount = 4;\n> +\tif (!roles.empty()) {\n> +\t\tStreamConfiguration cfg;\n>  \n> -\tconfig.addConfiguration(cfg);\n> +\t\tcfg.pixelFormat = V4L2_PIX_FMT_RGB24;\n> +\t\tcfg.size = { 640, 480 };\n> +\t\tcfg.bufferCount = 4;\n> +\n> +\t\tconfig->addConfiguration(cfg);\n> +\t}\n>  \n>  \treturn config;\n>  }\n>  \n> -int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration &config)\n> +int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config)\n>  {\n>  \tVimcCameraData *data = cameraData(camera);\n> -\tStreamConfiguration &cfg = config[0];\n> +\tStreamConfiguration &cfg = (*config)[0];\n>  \tint ret;\n>  \n>  \tV4L2DeviceFormat format = {};\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index 4185e3557dcb..de46e98880a2 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -233,7 +233,8 @@ void PipelineHandler::unlock()\n>   * the group of streams parameters.\n>   *\n>   * \\return A valid CameraConfiguration if the requested roles can be satisfied,\n> - * or a invalid configuration otherwise\n> + * or a null pointer otherwise. The ownership of the returned configuration is\n> + * passed to the caller.\n>   */\n>  \n>  /**\n> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> index 06ae2985f80d..9f0454b7a5f3 100644\n> --- a/src/qcam/main_window.cpp\n> +++ b/src/qcam/main_window.cpp\n> @@ -104,7 +104,7 @@ int MainWindow::startCapture()\n>  \t\treturn ret;\n>  \t}\n>  \n> -\tconst StreamConfiguration &cfg = config_[0];\n> +\tconst StreamConfiguration &cfg = (*config_)[0];\n>  \tStream *stream = cfg.stream();\n>  \tret = viewfinder_->setFormat(cfg.pixelFormat, cfg.size.width,\n>  \t\t\t\t     cfg.size.height);\n> @@ -180,6 +180,9 @@ void MainWindow::stopCapture()\n>  \n>  \tcamera_->freeBuffers();\n>  \tisCapturing_ = false;\n> +\n> +\tdelete config_;\n> +\tconfig_ = nullptr;\n>  }\n>  \n>  void MainWindow::requestComplete(Request *request,\n> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h\n> index 143b5b08a5a0..866866e93d23 100644\n> --- a/src/qcam/main_window.h\n> +++ b/src/qcam/main_window.h\n> @@ -45,7 +45,7 @@ private:\n>  \n>  \tstd::shared_ptr<Camera> camera_;\n>  \tbool isCapturing_;\n> -\tCameraConfiguration config_;\n> +\tCameraConfiguration *config_;\n>  \n>  \tViewFinder *viewfinder_;\n>  };\n> diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp\n> index bfd11eefedcf..f640e80fbaf3 100644\n> --- a/test/camera/capture.cpp\n> +++ b/test/camera/capture.cpp\n> @@ -40,13 +40,25 @@ protected:\n>  \t\tcamera_->queueRequest(request);\n>  \t}\n>  \n> -\tint run()\n> +\tint init() override\n>  \t{\n> -\t\tCameraConfiguration config =\n> -\t\t\tcamera_->generateConfiguration({ StreamRole::VideoRecording });\n> -\t\tStreamConfiguration *cfg = &config[0];\n> +\t\tCameraTest::init();\n>  \n> -\t\tif (!config.isValid()) {\n> +\t\tconfig_ = camera_->generateConfiguration({ StreamRole::VideoRecording });\n> +\t\tif (!config_) {\n> +\t\t\tcout << \"Failed to generate default configuration\" << endl;\n> +\t\t\tCameraTest::cleanup();\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\treturn TestPass;\n> +\t}\n> +\n> +\tint run() override\n> +\t{\n> +\t\tStreamConfiguration &cfg = (*config_)[0];\n> +\n> +\t\tif (!config_->isValid()) {\n>  \t\t\tcout << \"Failed to read default configuration\" << endl;\n>  \t\t\treturn TestFail;\n>  \t\t}\n> @@ -56,7 +68,7 @@ protected:\n>  \t\t\treturn TestFail;\n>  \t\t}\n>  \n> -\t\tif (camera_->configure(config)) {\n> +\t\tif (camera_->configure(config_)) {\n>  \t\t\tcout << \"Failed to set default configuration\" << endl;\n>  \t\t\treturn TestFail;\n>  \t\t}\n> @@ -66,7 +78,7 @@ protected:\n>  \t\t\treturn TestFail;\n>  \t\t}\n>  \n> -\t\tStream *stream = cfg->stream();\n> +\t\tStream *stream = cfg.stream();\n>  \t\tBufferPool &pool = stream->bufferPool();\n>  \t\tstd::vector<Request *> requests;\n>  \t\tfor (Buffer &buffer : pool.buffers()) {\n> @@ -110,10 +122,10 @@ protected:\n>  \t\twhile (timer.isRunning())\n>  \t\t\tdispatcher->processEvents();\n>  \n> -\t\tif (completeRequestsCount_ <= cfg->bufferCount * 2) {\n> +\t\tif (completeRequestsCount_ <= cfg.bufferCount * 2) {\n>  \t\t\tcout << \"Failed to capture enough frames (got \"\n>  \t\t\t     << completeRequestsCount_ << \" expected at least \"\n> -\t\t\t     << cfg->bufferCount * 2 << \")\" << endl;\n> +\t\t\t     << cfg.bufferCount * 2 << \")\" << endl;\n>  \t\t\treturn TestFail;\n>  \t\t}\n>  \n> @@ -134,6 +146,14 @@ protected:\n>  \n>  \t\treturn TestPass;\n>  \t}\n> +\n> +\tvoid cleanup() override\n> +\t{\n> +\t\tdelete config_;\n> +\t\tCameraTest::cleanup();\n> +\t}\n> +\n> +\tCameraConfiguration *config_;\n>  };\n>  \n>  } /* namespace */\n> diff --git a/test/camera/configuration_default.cpp b/test/camera/configuration_default.cpp\n> index 8c4a03db498a..d5cefc1127c9 100644\n> --- a/test/camera/configuration_default.cpp\n> +++ b/test/camera/configuration_default.cpp\n> @@ -18,26 +18,32 @@ class ConfigurationDefault : public CameraTest\n>  protected:\n>  \tint run()\n>  \t{\n> -\t\tCameraConfiguration config;\n> +\t\tCameraConfiguration *config;\n>  \n>  \t\t/* Test asking for configuration for a video stream. */\n>  \t\tconfig = camera_->generateConfiguration({ StreamRole::VideoRecording });\n> -\t\tif (!config.isValid()) {\n> +\t\tif (!config || !config->isValid()) {\n>  \t\t\tcout << \"Default configuration invalid\" << endl;\n> +\t\t\tdelete config;\n>  \t\t\treturn TestFail;\n>  \t\t}\n>  \n> +\t\tdelete config;\n> +\n>  \t\t/*\n>  \t\t * Test that asking for configuration for an empty array of\n> -\t\t * stream roles returns an empty list of configurations.\n> +\t\t * stream roles returns an empty camera configuration.\n>  \t\t */\n>  \t\tconfig = camera_->generateConfiguration({});\n> -\t\tif (config.isValid()) {\n> +\t\tif (!config || config->isValid()) {\n>  \t\t\tcout << \"Failed to retrieve configuration for empty roles list\"\n>  \t\t\t     << endl;\n> +\t\t\tdelete config;\n>  \t\t\treturn TestFail;\n>  \t\t}\n>  \n> +\t\tdelete config;\n> +\n>  \t\treturn TestPass;\n>  \t}\n>  };\n> diff --git a/test/camera/configuration_set.cpp b/test/camera/configuration_set.cpp\n> index 25b5db67103a..ca911f459c32 100644\n> --- a/test/camera/configuration_set.cpp\n> +++ b/test/camera/configuration_set.cpp\n> @@ -16,13 +16,25 @@ namespace {\n>  class ConfigurationSet : public CameraTest\n>  {\n>  protected:\n> -\tint run()\n> +\tint init() override\n>  \t{\n> -\t\tCameraConfiguration config =\n> -\t\t\tcamera_->generateConfiguration({ StreamRole::VideoRecording });\n> -\t\tStreamConfiguration *cfg = &config[0];\n> +\t\tCameraTest::init();\n>  \n> -\t\tif (!config.isValid()) {\n> +\t\tconfig_ = camera_->generateConfiguration({ StreamRole::VideoRecording });\n> +\t\tif (!config_) {\n> +\t\t\tcout << \"Failed to generate default configuration\" << endl;\n> +\t\t\tCameraTest::cleanup();\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\treturn TestPass;\n> +\t}\n> +\n> +\tint run() override\n> +\t{\n> +\t\tStreamConfiguration &cfg = (*config_)[0];\n> +\n> +\t\tif (!config_->isValid()) {\n>  \t\t\tcout << \"Failed to read default configuration\" << endl;\n>  \t\t\treturn TestFail;\n>  \t\t}\n> @@ -33,7 +45,7 @@ protected:\n>  \t\t}\n>  \n>  \t\t/* Test that setting the default configuration works. */\n> -\t\tif (camera_->configure(config)) {\n> +\t\tif (camera_->configure(config_)) {\n>  \t\t\tcout << \"Failed to set default configuration\" << endl;\n>  \t\t\treturn TestFail;\n>  \t\t}\n> @@ -48,7 +60,7 @@ protected:\n>  \t\t\treturn TestFail;\n>  \t\t}\n>  \n> -\t\tif (!camera_->configure(config)) {\n> +\t\tif (!camera_->configure(config_)) {\n>  \t\t\tcout << \"Setting configuration on a camera not acquired succeeded when it should have failed\"\n>  \t\t\t     << endl;\n>  \t\t\treturn TestFail;\n> @@ -64,9 +76,9 @@ protected:\n>  \t\t * the default configuration of the VIMC camera is known to\n>  \t\t * work.\n>  \t\t */\n> -\t\tcfg->size.width *= 2;\n> -\t\tcfg->size.height *= 2;\n> -\t\tif (camera_->configure(config)) {\n> +\t\tcfg.size.width *= 2;\n> +\t\tcfg.size.height *= 2;\n> +\t\tif (camera_->configure(config_)) {\n>  \t\t\tcout << \"Failed to set modified configuration\" << endl;\n>  \t\t\treturn TestFail;\n>  \t\t}\n> @@ -74,14 +86,22 @@ protected:\n>  \t\t/*\n>  \t\t * Test that setting an invalid configuration fails.\n>  \t\t */\n> -\t\tcfg->size = { 0, 0 };\n> -\t\tif (!camera_->configure(config)) {\n> +\t\tcfg.size = { 0, 0 };\n> +\t\tif (!camera_->configure(config_)) {\n>  \t\t\tcout << \"Invalid configuration incorrectly accepted\" << endl;\n>  \t\t\treturn TestFail;\n>  \t\t}\n>  \n>  \t\treturn TestPass;\n>  \t}\n> +\n> +\tvoid cleanup() override\n> +\t{\n> +\t\tdelete config_;\n> +\t\tCameraTest::cleanup();\n> +\t}\n> +\n> +\tCameraConfiguration *config_;\n>  };\n>  \n>  } /* namespace */\n> diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp\n> index 7a74cd85a37a..a662e869fadf 100644\n> --- a/test/camera/statemachine.cpp\n> +++ b/test/camera/statemachine.cpp\n> @@ -233,10 +233,22 @@ protected:\n>  \t\treturn TestPass;\n>  \t}\n>  \n> -\tint run()\n> +\tint init() override\n>  \t{\n> +\t\tCameraTest::init();\n> +\n>  \t\tdefconf_ = camera_->generateConfiguration({ StreamRole::VideoRecording });\n> +\t\tif (!defconf_) {\n> +\t\t\tcout << \"Failed to generate default configuration\" << endl;\n> +\t\t\tCameraTest::cleanup();\n> +\t\t\treturn TestFail;\n> +\t\t}\n>  \n> +\t\treturn TestPass;\n> +\t}\n> +\n> +\tint run() override\n> +\t{\n>  \t\tif (testAvailable() != TestPass) {\n>  \t\t\tcout << \"State machine in Available state failed\" << endl;\n>  \t\t\treturn TestFail;\n> @@ -265,7 +277,13 @@ protected:\n>  \t\treturn TestPass;\n>  \t}\n>  \n> -\tCameraConfiguration defconf_;\n> +\tvoid cleanup() override\n> +\t{\n> +\t\tdelete defconf_;\n> +\t\tCameraTest::cleanup();\n> +\t}\n> +\n> +\tCameraConfiguration *defconf_;\n>  };\n>  \n>  } /* namespace */\n> -- \n> Regards,\n> \n> Laurent Pinchart\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<niklas.soderlund@ragnatech.se>","Received":["from mail-lj1-x242.google.com (mail-lj1-x242.google.com\n\t[IPv6:2a00:1450:4864:20::242])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 296A560C02\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat, 18 May 2019 18:41:30 +0200 (CEST)","by mail-lj1-x242.google.com with SMTP id e13so8846750ljl.11\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat, 18 May 2019 09:41:30 -0700 (PDT)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\ty19sm2632584lfl.40.2019.05.18.09.41.28\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tSat, 18 May 2019 09:41:28 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=aOD7ImKmbKOhL/+aNNLaVXHXofcCvHc1QYFZ1lJSPag=;\n\tb=mJlx6HOCtR4cZIZZDNhtSURBuayiNGfk+O8ll6+E4FqGFtVbP+T1716C8/Mclt7aKp\n\tMwpWIKdO+ogHUQcJxq3umSaRmhdEbanL7c1RxfKaDjvJ20fT4uRkGVO4YQNULKuHv064\n\tCNwwtOvUwj9vP/cOd7wBzw3rAXEc5kEUzKopOsKq8sgSM5XxQMP3PKT6WIK+IKkc5Hp+\n\t27G96AwxCe6fORH7Tg7NuT5fbV+plfRT1jj/4JQM25JLlRruSSnpmw8jgsv3xlZb0ynQ\n\tOLuN6OKvkvujtrTQ/zi8Ylpey0fEuELmBMS8s62Uiobv+5iIXcSiXQxVFeUOFalHvxNP\n\tyjWw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=aOD7ImKmbKOhL/+aNNLaVXHXofcCvHc1QYFZ1lJSPag=;\n\tb=Ux5VHF2Ao7Huo+ggYCLUut9Zq9uzpr5XikC6Lqz6HggTgvENhTzuRJYdetqP1X+xta\n\tiFPTZYOx/R3meaC5S2eMJrMpBZzDPz49wqKpsvCcNT58YJ/H7AYJR89D1Sb3aNnQTwGR\n\t9i8DaXWe2MUFdGqMDAJLmvl5YleqjxCrzs/mwC9Rhkln8AOT7ccpt9OE6FRvGQm9bjxH\n\tMwpkYrs2Yy29Yj/Kdcxhdp/z141zYB/yGzibNwWmnTGRcSNh5phZUssMcNu67FEiMBCo\n\tBuCIe13iA65weYBV40ff13gZui9zF+Lfo4ABBkIWLtApT+39fTR6cgN1XGnmPPO43YtG\n\tPDKA==","X-Gm-Message-State":"APjAAAVUoB3Gr4jd1d40kGM4f76UEJ52ZvOhAbgVYWvPw7OC+z5E6Q8+\n\tXJHhTQD07LIyoqYAhbX0AU8FJ/VhJWI=","X-Google-Smtp-Source":"APXvYqxCCaD6EAMZukh5rb0Gyl6E56JzkGAXCsK/AwGgor0wQaugKDiTTv4VWn6bnMdtZ0tEFhJrSw==","X-Received":"by 2002:a2e:4a09:: with SMTP id x9mr8310922lja.19.1558197689242; \n\tSat, 18 May 2019 09:41:29 -0700 (PDT)","Date":"Sat, 18 May 2019 18:41:27 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= <niklas.soderlund@ragnatech.se>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190518164127.GG25081@bigcity.dyn.berto.se>","References":"<20190517230621.24668-1-laurent.pinchart@ideasonboard.com>\n\t<20190517230621.24668-6-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190517230621.24668-6-laurent.pinchart@ideasonboard.com>","User-Agent":"Mutt/1.11.3 (2019-02-01)","Subject":"Re: [libcamera-devel] [PATCH/RFC 05/12] libcamera: camera: Return a\n\tpointer from generateConfiguration()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Sat, 18 May 2019 16:41:30 -0000"}},{"id":1619,"web_url":"https://patchwork.libcamera.org/comment/1619/","msgid":"<20190518172224.GB4995@pendragon.ideasonboard.com>","date":"2019-05-18T17:22:24","subject":"Re: [libcamera-devel] [PATCH/RFC 05/12] libcamera: camera: Return a\n\tpointer from generateConfiguration()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nOn Sat, May 18, 2019 at 06:41:27PM +0200, Niklas Söderlund wrote:\n> On 2019-05-18 02:06:14 +0300, Laurent Pinchart wrote:\n> > To prepare for specialising the CameraConfiguration class in pipeline\n> > handlers, return a pointer to a camera configuration instead of a\n> > reference from Camera::generateConfiguration(). The camera configuration\n> > always needs to be allocated from the pipeline handler, and its\n> > ownership is passed to the application.\n> \n> I understand the need for pipeline handlers to be able to specialise the \n> CameraConfiguration, at the same time I'm not super happy with how this \n> change effects the API towards applications. Specially as we model the \n> CameraConfiguration around a vector.\n> \n>     CameraConfiguration *config = ... prepareCameraConfig();\n>     StreamConfiguration &cfg = (*config)[index];\n> \n> Is not the most intuitive interface ;-)\n\nI've seen worse :-) A vector pointer isn't that bad.\n\n> I wonder if we could improve here using a d-pointer design and a private \n> pointer with a reference count, or maybe even use a shared_ptr<> \n> internally?\n\nIf we do this I fear it will make the interface towards pipeline\nhandlers much more complex. We will need to allow copies of\nCameraConfiguration, which internally will need to duplicate the\npipeline-handler specific data stored in the CameraConfiguration. I\ndon't really see how we could keep it clean and simple. Of course, feel\nfree to prove me wrong :-)\n\n> > For symmetry, change Camera::configure() to take a CameraConfiguration\n> > pointer instead of a reference. This aligns with our coding practice of\n> > passing parameters that are modified by the callee by pointer.\n> \n> This agree with and should be changed disregarding of the above, and \n> maybe even moved to a patch (or it's own patch) earlier in this series\n> where 'const' is dropped from the argument.\n\nLet's first see if we find a good way to return configurations by value\ninstead of pointer. If we don't I think we can bundle the two changes in\nthis patch.\n\n> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> > ---\n> >  include/libcamera/camera.h               |  4 +--\n> >  src/cam/main.cpp                         | 38 ++++++++++----------\n> >  src/libcamera/camera.cpp                 | 31 +++++++++--------\n> >  src/libcamera/include/pipeline_handler.h |  6 ++--\n> >  src/libcamera/pipeline/ipu3/ipu3.cpp     | 31 +++++++++--------\n> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 25 ++++++++------\n> >  src/libcamera/pipeline/uvcvideo.cpp      | 30 ++++++++--------\n> >  src/libcamera/pipeline/vimc.cpp          | 30 ++++++++--------\n> >  src/libcamera/pipeline_handler.cpp       |  3 +-\n> >  src/qcam/main_window.cpp                 |  5 ++-\n> >  src/qcam/main_window.h                   |  2 +-\n> >  test/camera/capture.cpp                  | 38 +++++++++++++++-----\n> >  test/camera/configuration_default.cpp    | 14 +++++---\n> >  test/camera/configuration_set.cpp        | 44 +++++++++++++++++-------\n> >  test/camera/statemachine.cpp             | 22 ++++++++++--\n> >  15 files changed, 201 insertions(+), 122 deletions(-)\n> > \n> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > index e2759405f497..841e8fc505b9 100644\n> > --- a/include/libcamera/camera.h\n> > +++ b/include/libcamera/camera.h\n> > @@ -68,8 +68,8 @@ public:\n> >  \tint release();\n> >  \n> >  \tconst std::set<Stream *> &streams() const;\n> > -\tCameraConfiguration generateConfiguration(const StreamRoles &roles);\n> > -\tint configure(CameraConfiguration &config);\n> > +\tCameraConfiguration *generateConfiguration(const StreamRoles &roles);\n> > +\tint configure(CameraConfiguration *config);\n> >  \n> >  \tint allocateBuffers();\n> >  \tint freeBuffers();\n> > diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> > index cd165bea34cd..7a1b332f68c7 100644\n> > --- a/src/cam/main.cpp\n> > +++ b/src/cam/main.cpp\n> > @@ -85,15 +85,13 @@ static int parseOptions(int argc, char *argv[])\n> >  \treturn 0;\n> >  }\n> >  \n> > -static int prepareCameraConfig(CameraConfiguration *config)\n> > +static CameraConfiguration *prepareCameraConfig()\n> >  {\n> >  \tStreamRoles roles;\n> >  \n> >  \t/* If no configuration is provided assume a single video stream. */\n> > -\tif (!options.isSet(OptStream)) {\n> > -\t\t*config = camera->generateConfiguration({ StreamRole::VideoRecording });\n> > -\t\treturn 0;\n> > -\t}\n> > +\tif (!options.isSet(OptStream))\n> > +\t\treturn camera->generateConfiguration({ StreamRole::VideoRecording });\n> >  \n> >  \tconst std::vector<OptionValue> &streamOptions =\n> >  \t\toptions[OptStream].toArray();\n> > @@ -113,16 +111,16 @@ static int prepareCameraConfig(CameraConfiguration *config)\n> >  \t\t} else {\n> >  \t\t\tstd::cerr << \"Unknown stream role \"\n> >  \t\t\t\t  << conf[\"role\"].toString() << std::endl;\n> > -\t\t\treturn -EINVAL;\n> > +\t\t\treturn nullptr;\n> >  \t\t}\n> >  \t}\n> >  \n> > -\t*config = camera->generateConfiguration(roles);\n> > -\n> > -\tif (!config->isValid()) {\n> > +\tCameraConfiguration *config = camera->generateConfiguration(roles);\n> > +\tif (!config || !config->isValid()) {\n> >  \t\tstd::cerr << \"Failed to get default stream configuration\"\n> >  \t\t\t  << std::endl;\n> > -\t\treturn -EINVAL;\n> > +\t\tdelete config;\n> > +\t\treturn nullptr;\n> >  \t}\n> >  \n> >  \t/* Apply configuration explicitly requested. */\n> > @@ -142,7 +140,7 @@ static int prepareCameraConfig(CameraConfiguration *config)\n> >  \t\t\tcfg.pixelFormat = conf[\"pixelformat\"];\n> >  \t}\n> >  \n> > -\treturn 0;\n> > +\treturn config;\n> >  }\n> >  \n> >  static void requestComplete(Request *request, const std::map<Stream *, Buffer *> &buffers)\n> > @@ -191,25 +189,25 @@ static void requestComplete(Request *request, const std::map<Stream *, Buffer *>\n> >  \n> >  static int capture()\n> >  {\n> > -\tCameraConfiguration config;\n> >  \tint ret;\n> >  \n> > -\tret = prepareCameraConfig(&config);\n> > -\tif (ret) {\n> > +\tCameraConfiguration *config = prepareCameraConfig();\n> > +\tif (!config) {\n> >  \t\tstd::cout << \"Failed to prepare camera configuration\" << std::endl;\n> > -\t\treturn ret;\n> > +\t\treturn -EINVAL;\n> >  \t}\n> >  \n> >  \tret = camera->configure(config);\n> >  \tif (ret < 0) {\n> >  \t\tstd::cout << \"Failed to configure camera\" << std::endl;\n> > +\t\tdelete config;\n> >  \t\treturn ret;\n> >  \t}\n> >  \n> >  \tstreamInfo.clear();\n> >  \n> > -\tfor (unsigned int index = 0; index < config.size(); ++index) {\n> > -\t\tStreamConfiguration &cfg = config[index];\n> > +\tfor (unsigned int index = 0; index < config->size(); ++index) {\n> > +\t\tStreamConfiguration &cfg = (*config)[index];\n> >  \t\tstreamInfo[cfg.stream()] = \"stream\" + std::to_string(index);\n> >  \t}\n> >  \n> > @@ -217,6 +215,7 @@ static int capture()\n> >  \tif (ret) {\n> >  \t\tstd::cerr << \"Failed to allocate buffers\"\n> >  \t\t\t  << std::endl;\n> > +\t\tdelete config;\n> >  \t\treturn ret;\n> >  \t}\n> >  \n> > @@ -224,7 +223,7 @@ static int capture()\n> >  \n> >  \t/* Identify the stream with the least number of buffers. */\n> >  \tunsigned int nbuffers = UINT_MAX;\n> > -\tfor (StreamConfiguration &cfg : config) {\n> > +\tfor (StreamConfiguration &cfg : *config) {\n> >  \t\tStream *stream = cfg.stream();\n> >  \t\tnbuffers = std::min(nbuffers, stream->bufferPool().count());\n> >  \t}\n> > @@ -244,7 +243,7 @@ static int capture()\n> >  \t\t}\n> >  \n> >  \t\tstd::map<Stream *, Buffer *> map;\n> > -\t\tfor (StreamConfiguration &cfg : config) {\n> > +\t\tfor (StreamConfiguration &cfg : *config) {\n> >  \t\t\tStream *stream = cfg.stream();\n> >  \t\t\tmap[stream] = &stream->bufferPool().buffers()[i];\n> >  \t\t}\n> > @@ -280,6 +279,7 @@ static int capture()\n> >  \t\tstd::cout << \"Failed to stop capture\" << std::endl;\n> >  out:\n> >  \tcamera->freeBuffers();\n> > +\tdelete config;\n> >  \n> >  \treturn ret;\n> >  }\n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index f8a4946b4a6a..115cdb1c024b 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -518,21 +518,24 @@ const std::set<Stream *> &Camera::streams() const\n> >   * specifies a list of stream roles and the camera returns a configuration\n> >   * containing suitable streams and their suggested default configurations.\n> >   *\n> > - * \\return A valid CameraConfiguration if the requested roles can be satisfied,\n> > - * or a invalid one otherwise\n> > + * \\return A CameraConfiguration if the requested roles can be satisfied, or a\n> > + * null pointer otherwise. The ownership of the returned configuration is\n> > + * passed to the caller.\n> >   */\n> > -CameraConfiguration\n> > -Camera::generateConfiguration(const StreamRoles &roles)\n> > +CameraConfiguration *Camera::generateConfiguration(const StreamRoles &roles)\n> >  {\n> > -\tif (disconnected_ || !roles.size() || roles.size() > streams_.size())\n> > -\t\treturn CameraConfiguration();\n> > +\tif (disconnected_ || roles.size() > streams_.size())\n> > +\t\treturn nullptr;\n> >  \n> > -\tCameraConfiguration config = pipe_->generateConfiguration(this, roles);\n> > +\tCameraConfiguration *config = pipe_->generateConfiguration(this, roles);\n> >  \n> >  \tstd::ostringstream msg(\"streams configuration:\", std::ios_base::ate);\n> >  \n> > -\tfor (unsigned int index = 0; index < config.size(); ++index)\n> > -\t\tmsg << \" (\" << index << \") \" << config[index].toString();\n> > +\tif (config->isEmpty())\n> > +\t\tmsg << \" empty\";\n> > +\n> > +\tfor (unsigned int index = 0; index < config->size(); ++index)\n> > +\t\tmsg << \" (\" << index << \") \" << (*config)[index].toString();\n> >  \n> >  \tLOG(Camera, Debug) << msg.str();\n> >  \n> > @@ -566,7 +569,7 @@ Camera::generateConfiguration(const StreamRoles &roles)\n> >   * \\retval -EACCES The camera is not in a state where it can be configured\n> >   * \\retval -EINVAL The configuration is not valid\n> >   */\n> > -int Camera::configure(CameraConfiguration &config)\n> > +int Camera::configure(CameraConfiguration *config)\n> >  {\n> >  \tint ret;\n> >  \n> > @@ -576,7 +579,7 @@ int Camera::configure(CameraConfiguration &config)\n> >  \tif (!stateBetween(CameraAcquired, CameraConfigured))\n> >  \t\treturn -EACCES;\n> >  \n> > -\tif (!config.isValid()) {\n> > +\tif (!config->isValid()) {\n> >  \t\tLOG(Camera, Error)\n> >  \t\t\t<< \"Can't configure camera with invalid configuration\";\n> >  \t\treturn -EINVAL;\n> > @@ -584,8 +587,8 @@ int Camera::configure(CameraConfiguration &config)\n> >  \n> >  \tstd::ostringstream msg(\"configuring streams:\", std::ios_base::ate);\n> >  \n> > -\tfor (unsigned int index = 0; index < config.size(); ++index) {\n> > -\t\tStreamConfiguration &cfg = config[index];\n> > +\tfor (unsigned int index = 0; index < config->size(); ++index) {\n> > +\t\tStreamConfiguration &cfg = (*config)[index];\n> >  \t\tcfg.setStream(nullptr);\n> >  \t\tmsg << \" (\" << index << \") \" << cfg.toString();\n> >  \t}\n> > @@ -597,7 +600,7 @@ int Camera::configure(CameraConfiguration &config)\n> >  \t\treturn ret;\n> >  \n> >  \tactiveStreams_.clear();\n> > -\tfor (const StreamConfiguration &cfg : config) {\n> > +\tfor (const StreamConfiguration &cfg : *config) {\n> >  \t\tStream *stream = cfg.stream();\n> >  \t\tif (!stream)\n> >  \t\t\tLOG(Camera, Fatal)\n> > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> > index a025905ab68f..7da6df1ab2a0 100644\n> > --- a/src/libcamera/include/pipeline_handler.h\n> > +++ b/src/libcamera/include/pipeline_handler.h\n> > @@ -60,9 +60,9 @@ public:\n> >  \tbool lock();\n> >  \tvoid unlock();\n> >  \n> > -\tvirtual CameraConfiguration\n> > -\tgenerateConfiguration(Camera *camera, const StreamRoles &roles) = 0;\n> > -\tvirtual int configure(Camera *camera, CameraConfiguration &config) = 0;\n> > +\tvirtual CameraConfiguration *generateConfiguration(Camera *camera,\n> > +\t\tconst StreamRoles &roles) = 0;\n> > +\tvirtual int configure(Camera *camera, CameraConfiguration *config) = 0;\n> >  \n> >  \tvirtual int allocateBuffers(Camera *camera,\n> >  \t\t\t\t    const std::set<Stream *> &streams) = 0;\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index ed0ef69de1d1..7c6f2d4a23be 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -150,9 +150,9 @@ class PipelineHandlerIPU3 : public PipelineHandler\n> >  public:\n> >  \tPipelineHandlerIPU3(CameraManager *manager);\n> >  \n> > -\tCameraConfiguration\n> > -\tgenerateConfiguration(Camera *camera, const StreamRoles &roles) override;\n> > -\tint configure(Camera *camera, CameraConfiguration &config) override;\n> > +\tCameraConfiguration *generateConfiguration(Camera *camera,\n> > +\t\tconst StreamRoles &roles) override;\n> > +\tint configure(Camera *camera, CameraConfiguration *config) override;\n> >  \n> >  \tint allocateBuffers(Camera *camera,\n> >  \t\t\t    const std::set<Stream *> &streams) override;\n> > @@ -207,12 +207,11 @@ PipelineHandlerIPU3::PipelineHandlerIPU3(CameraManager *manager)\n> >  {\n> >  }\n> >  \n> > -CameraConfiguration\n> > -PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n> > -\t\t\t\t\t   const StreamRoles &roles)\n> > +CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n> > +\tconst StreamRoles &roles)\n> >  {\n> >  \tIPU3CameraData *data = cameraData(camera);\n> > -\tCameraConfiguration config = {};\n> > +\tCameraConfiguration *config = new CameraConfiguration();\n> >  \tstd::set<IPU3Stream *> streams = {\n> >  \t\t&data->outStream_,\n> >  \t\t&data->vfStream_,\n> > @@ -290,21 +289,23 @@ PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n> >  \t\t\tbreak;\n> >  \t\t}\n> >  \n> > -\t\tif (!stream)\n> > -\t\t\treturn CameraConfiguration{};\n> > +\t\tif (!stream) {\n> > +\t\t\tdelete config;\n> > +\t\t\treturn nullptr;\n> > +\t\t}\n> >  \n> >  \t\tstreams.erase(stream);\n> >  \n> >  \t\tcfg.pixelFormat = V4L2_PIX_FMT_NV12;\n> >  \t\tcfg.bufferCount = IPU3_BUFFER_COUNT;\n> >  \n> > -\t\tconfig.addConfiguration(cfg);\n> > +\t\tconfig->addConfiguration(cfg);\n> >  \t}\n> >  \n> >  \treturn config;\n> >  }\n> >  \n> > -int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration &config)\n> > +int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *config)\n> >  {\n> >  \tIPU3CameraData *data = cameraData(camera);\n> >  \tIPU3Stream *outStream = &data->outStream_;\n> > @@ -316,7 +317,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration &config)\n> >  \n> >  \toutStream->active_ = false;\n> >  \tvfStream->active_ = false;\n> > -\tfor (StreamConfiguration &cfg : config) {\n> > +\tfor (StreamConfiguration &cfg : *config) {\n> >  \t\t/*\n> >  \t\t * Pick a stream for the configuration entry.\n> >  \t\t * \\todo: This is a naive temporary implementation that will be\n> > @@ -382,7 +383,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration &config)\n> >  \t\treturn ret;\n> >  \n> >  \t/* Apply the format to the configured streams output devices. */\n> > -\tfor (StreamConfiguration &cfg : config) {\n> > +\tfor (StreamConfiguration &cfg : *config) {\n> >  \t\tIPU3Stream *stream = static_cast<IPU3Stream *>(cfg.stream());\n> >  \t\tret = imgu->configureOutput(stream->device_, cfg);\n> >  \t\tif (ret)\n> > @@ -395,13 +396,13 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration &config)\n> >  \t * be at least one active stream in the configuration request).\n> >  \t */\n> >  \tif (!outStream->active_) {\n> > -\t\tret = imgu->configureOutput(outStream->device_, config[0]);\n> > +\t\tret = imgu->configureOutput(outStream->device_, (*config)[0]);\n> >  \t\tif (ret)\n> >  \t\t\treturn ret;\n> >  \t}\n> >  \n> >  \tif (!vfStream->active_) {\n> > -\t\tret = imgu->configureOutput(vfStream->device_, config[0]);\n> > +\t\tret = imgu->configureOutput(vfStream->device_, (*config)[0]);\n> >  \t\tif (ret)\n> >  \t\t\treturn ret;\n> >  \t}\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index 6b35aa3fe7c3..c3b3912c96f3 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -34,9 +34,9 @@ public:\n> >  \tPipelineHandlerRkISP1(CameraManager *manager);\n> >  \t~PipelineHandlerRkISP1();\n> >  \n> > -\tCameraConfiguration generateConfiguration(Camera *camera,\n> > +\tCameraConfiguration *generateConfiguration(Camera *camera,\n> >  \t\tconst StreamRoles &roles) override;\n> > -\tint configure(Camera *camera, CameraConfiguration &config) override;\n> > +\tint configure(Camera *camera, CameraConfiguration *config) override;\n> >  \n> >  \tint allocateBuffers(Camera *camera,\n> >  \t\tconst std::set<Stream *> &streams) override;\n> > @@ -105,26 +105,29 @@ PipelineHandlerRkISP1::~PipelineHandlerRkISP1()\n> >   * Pipeline Operations\n> >   */\n> >  \n> > -CameraConfiguration PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n> > +CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n> >  \tconst StreamRoles &roles)\n> >  {\n> >  \tRkISP1CameraData *data = cameraData(camera);\n> > -\tCameraConfiguration config;\n> > -\tStreamConfiguration cfg{};\n> > +\tCameraConfiguration *config = new CameraConfiguration();\n> >  \n> > -\tcfg.pixelFormat = V4L2_PIX_FMT_NV12;\n> > -\tcfg.size = data->sensor_->resolution();\n> > -\tcfg.bufferCount = RKISP1_BUFFER_COUNT;\n> > +\tif (!roles.empty()) {\n> > +\t\tStreamConfiguration cfg{};\n> >  \n> > -\tconfig.addConfiguration(cfg);\n> > +\t\tcfg.pixelFormat = V4L2_PIX_FMT_NV12;\n> > +\t\tcfg.size = data->sensor_->resolution();\n> > +\t\tcfg.bufferCount = RKISP1_BUFFER_COUNT;\n> > +\n> > +\t\tconfig->addConfiguration(cfg);\n> > +\t}\n> >  \n> >  \treturn config;\n> >  }\n> >  \n> > -int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration &config)\n> > +int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *config)\n> >  {\n> >  \tRkISP1CameraData *data = cameraData(camera);\n> > -\tStreamConfiguration &cfg = config[0];\n> > +\tStreamConfiguration &cfg = (*config)[0];\n> >  \tCameraSensor *sensor = data->sensor_;\n> >  \tint ret;\n> >  \n> > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> > index 5dcc868b2fc9..5c061ca61016 100644\n> > --- a/src/libcamera/pipeline/uvcvideo.cpp\n> > +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> > @@ -25,9 +25,9 @@ class PipelineHandlerUVC : public PipelineHandler\n> >  public:\n> >  \tPipelineHandlerUVC(CameraManager *manager);\n> >  \n> > -\tCameraConfiguration\n> > -\tgenerateConfiguration(Camera *camera, const StreamRoles &roles) override;\n> > -\tint configure(Camera *camera, CameraConfiguration &config) override;\n> > +\tCameraConfiguration *generateConfiguration(Camera *camera,\n> > +\t\tconst StreamRoles &roles) override;\n> > +\tint configure(Camera *camera, CameraConfiguration *config) override;\n> >  \n> >  \tint allocateBuffers(Camera *camera,\n> >  \t\t\t    const std::set<Stream *> &streams) override;\n> > @@ -73,26 +73,28 @@ PipelineHandlerUVC::PipelineHandlerUVC(CameraManager *manager)\n> >  {\n> >  }\n> >  \n> > -CameraConfiguration\n> > -PipelineHandlerUVC::generateConfiguration(Camera *camera,\n> > -\t\t\t\t\t  const StreamRoles &roles)\n> > +CameraConfiguration *PipelineHandlerUVC::generateConfiguration(Camera *camera,\n> > +\tconst StreamRoles &roles)\n> >  {\n> > -\tCameraConfiguration config;\n> > -\tStreamConfiguration cfg;\n> > +\tCameraConfiguration *config = new CameraConfiguration();\n> >  \n> > -\tcfg.pixelFormat = V4L2_PIX_FMT_YUYV;\n> > -\tcfg.size = { 640, 480 };\n> > -\tcfg.bufferCount = 4;\n> > +\tif (!roles.empty()) {\n> > +\t\tStreamConfiguration cfg;\n> >  \n> > -\tconfig.addConfiguration(cfg);\n> > +\t\tcfg.pixelFormat = V4L2_PIX_FMT_YUYV;\n> > +\t\tcfg.size = { 640, 480 };\n> > +\t\tcfg.bufferCount = 4;\n> > +\n> > +\t\tconfig->addConfiguration(cfg);\n> > +\t}\n> >  \n> >  \treturn config;\n> >  }\n> >  \n> > -int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration &config)\n> > +int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration *config)\n> >  {\n> >  \tUVCCameraData *data = cameraData(camera);\n> > -\tStreamConfiguration &cfg = config[0];\n> > +\tStreamConfiguration &cfg = (*config)[0];\n> >  \tint ret;\n> >  \n> >  \tV4L2DeviceFormat format = {};\n> > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> > index af6b6f21e3c5..0ece97f09e7e 100644\n> > --- a/src/libcamera/pipeline/vimc.cpp\n> > +++ b/src/libcamera/pipeline/vimc.cpp\n> > @@ -25,9 +25,9 @@ class PipelineHandlerVimc : public PipelineHandler\n> >  public:\n> >  \tPipelineHandlerVimc(CameraManager *manager);\n> >  \n> > -\tCameraConfiguration\n> > -\tgenerateConfiguration(Camera *camera, const StreamRoles &roles) override;\n> > -\tint configure(Camera *camera, CameraConfiguration &config) override;\n> > +\tCameraConfiguration *generateConfiguration(Camera *camera,\n> > +\t\tconst StreamRoles &roles) override;\n> > +\tint configure(Camera *camera, CameraConfiguration *config) override;\n> >  \n> >  \tint allocateBuffers(Camera *camera,\n> >  \t\t\t    const std::set<Stream *> &streams) override;\n> > @@ -73,26 +73,28 @@ PipelineHandlerVimc::PipelineHandlerVimc(CameraManager *manager)\n> >  {\n> >  }\n> >  \n> > -CameraConfiguration\n> > -PipelineHandlerVimc::generateConfiguration(Camera *camera,\n> > -\t\t\t\t\t   const StreamRoles &roles)\n> > +CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera,\n> > +\tconst StreamRoles &roles)\n> >  {\n> > -\tCameraConfiguration config;\n> > -\tStreamConfiguration cfg;\n> > +\tCameraConfiguration *config = new CameraConfiguration();\n> >  \n> > -\tcfg.pixelFormat = V4L2_PIX_FMT_RGB24;\n> > -\tcfg.size = { 640, 480 };\n> > -\tcfg.bufferCount = 4;\n> > +\tif (!roles.empty()) {\n> > +\t\tStreamConfiguration cfg;\n> >  \n> > -\tconfig.addConfiguration(cfg);\n> > +\t\tcfg.pixelFormat = V4L2_PIX_FMT_RGB24;\n> > +\t\tcfg.size = { 640, 480 };\n> > +\t\tcfg.bufferCount = 4;\n> > +\n> > +\t\tconfig->addConfiguration(cfg);\n> > +\t}\n> >  \n> >  \treturn config;\n> >  }\n> >  \n> > -int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration &config)\n> > +int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config)\n> >  {\n> >  \tVimcCameraData *data = cameraData(camera);\n> > -\tStreamConfiguration &cfg = config[0];\n> > +\tStreamConfiguration &cfg = (*config)[0];\n> >  \tint ret;\n> >  \n> >  \tV4L2DeviceFormat format = {};\n> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > index 4185e3557dcb..de46e98880a2 100644\n> > --- a/src/libcamera/pipeline_handler.cpp\n> > +++ b/src/libcamera/pipeline_handler.cpp\n> > @@ -233,7 +233,8 @@ void PipelineHandler::unlock()\n> >   * the group of streams parameters.\n> >   *\n> >   * \\return A valid CameraConfiguration if the requested roles can be satisfied,\n> > - * or a invalid configuration otherwise\n> > + * or a null pointer otherwise. The ownership of the returned configuration is\n> > + * passed to the caller.\n> >   */\n> >  \n> >  /**\n> > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> > index 06ae2985f80d..9f0454b7a5f3 100644\n> > --- a/src/qcam/main_window.cpp\n> > +++ b/src/qcam/main_window.cpp\n> > @@ -104,7 +104,7 @@ int MainWindow::startCapture()\n> >  \t\treturn ret;\n> >  \t}\n> >  \n> > -\tconst StreamConfiguration &cfg = config_[0];\n> > +\tconst StreamConfiguration &cfg = (*config_)[0];\n> >  \tStream *stream = cfg.stream();\n> >  \tret = viewfinder_->setFormat(cfg.pixelFormat, cfg.size.width,\n> >  \t\t\t\t     cfg.size.height);\n> > @@ -180,6 +180,9 @@ void MainWindow::stopCapture()\n> >  \n> >  \tcamera_->freeBuffers();\n> >  \tisCapturing_ = false;\n> > +\n> > +\tdelete config_;\n> > +\tconfig_ = nullptr;\n> >  }\n> >  \n> >  void MainWindow::requestComplete(Request *request,\n> > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h\n> > index 143b5b08a5a0..866866e93d23 100644\n> > --- a/src/qcam/main_window.h\n> > +++ b/src/qcam/main_window.h\n> > @@ -45,7 +45,7 @@ private:\n> >  \n> >  \tstd::shared_ptr<Camera> camera_;\n> >  \tbool isCapturing_;\n> > -\tCameraConfiguration config_;\n> > +\tCameraConfiguration *config_;\n> >  \n> >  \tViewFinder *viewfinder_;\n> >  };\n> > diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp\n> > index bfd11eefedcf..f640e80fbaf3 100644\n> > --- a/test/camera/capture.cpp\n> > +++ b/test/camera/capture.cpp\n> > @@ -40,13 +40,25 @@ protected:\n> >  \t\tcamera_->queueRequest(request);\n> >  \t}\n> >  \n> > -\tint run()\n> > +\tint init() override\n> >  \t{\n> > -\t\tCameraConfiguration config =\n> > -\t\t\tcamera_->generateConfiguration({ StreamRole::VideoRecording });\n> > -\t\tStreamConfiguration *cfg = &config[0];\n> > +\t\tCameraTest::init();\n> >  \n> > -\t\tif (!config.isValid()) {\n> > +\t\tconfig_ = camera_->generateConfiguration({ StreamRole::VideoRecording });\n> > +\t\tif (!config_) {\n> > +\t\t\tcout << \"Failed to generate default configuration\" << endl;\n> > +\t\t\tCameraTest::cleanup();\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\treturn TestPass;\n> > +\t}\n> > +\n> > +\tint run() override\n> > +\t{\n> > +\t\tStreamConfiguration &cfg = (*config_)[0];\n> > +\n> > +\t\tif (!config_->isValid()) {\n> >  \t\t\tcout << \"Failed to read default configuration\" << endl;\n> >  \t\t\treturn TestFail;\n> >  \t\t}\n> > @@ -56,7 +68,7 @@ protected:\n> >  \t\t\treturn TestFail;\n> >  \t\t}\n> >  \n> > -\t\tif (camera_->configure(config)) {\n> > +\t\tif (camera_->configure(config_)) {\n> >  \t\t\tcout << \"Failed to set default configuration\" << endl;\n> >  \t\t\treturn TestFail;\n> >  \t\t}\n> > @@ -66,7 +78,7 @@ protected:\n> >  \t\t\treturn TestFail;\n> >  \t\t}\n> >  \n> > -\t\tStream *stream = cfg->stream();\n> > +\t\tStream *stream = cfg.stream();\n> >  \t\tBufferPool &pool = stream->bufferPool();\n> >  \t\tstd::vector<Request *> requests;\n> >  \t\tfor (Buffer &buffer : pool.buffers()) {\n> > @@ -110,10 +122,10 @@ protected:\n> >  \t\twhile (timer.isRunning())\n> >  \t\t\tdispatcher->processEvents();\n> >  \n> > -\t\tif (completeRequestsCount_ <= cfg->bufferCount * 2) {\n> > +\t\tif (completeRequestsCount_ <= cfg.bufferCount * 2) {\n> >  \t\t\tcout << \"Failed to capture enough frames (got \"\n> >  \t\t\t     << completeRequestsCount_ << \" expected at least \"\n> > -\t\t\t     << cfg->bufferCount * 2 << \")\" << endl;\n> > +\t\t\t     << cfg.bufferCount * 2 << \")\" << endl;\n> >  \t\t\treturn TestFail;\n> >  \t\t}\n> >  \n> > @@ -134,6 +146,14 @@ protected:\n> >  \n> >  \t\treturn TestPass;\n> >  \t}\n> > +\n> > +\tvoid cleanup() override\n> > +\t{\n> > +\t\tdelete config_;\n> > +\t\tCameraTest::cleanup();\n> > +\t}\n> > +\n> > +\tCameraConfiguration *config_;\n> >  };\n> >  \n> >  } /* namespace */\n> > diff --git a/test/camera/configuration_default.cpp b/test/camera/configuration_default.cpp\n> > index 8c4a03db498a..d5cefc1127c9 100644\n> > --- a/test/camera/configuration_default.cpp\n> > +++ b/test/camera/configuration_default.cpp\n> > @@ -18,26 +18,32 @@ class ConfigurationDefault : public CameraTest\n> >  protected:\n> >  \tint run()\n> >  \t{\n> > -\t\tCameraConfiguration config;\n> > +\t\tCameraConfiguration *config;\n> >  \n> >  \t\t/* Test asking for configuration for a video stream. */\n> >  \t\tconfig = camera_->generateConfiguration({ StreamRole::VideoRecording });\n> > -\t\tif (!config.isValid()) {\n> > +\t\tif (!config || !config->isValid()) {\n> >  \t\t\tcout << \"Default configuration invalid\" << endl;\n> > +\t\t\tdelete config;\n> >  \t\t\treturn TestFail;\n> >  \t\t}\n> >  \n> > +\t\tdelete config;\n> > +\n> >  \t\t/*\n> >  \t\t * Test that asking for configuration for an empty array of\n> > -\t\t * stream roles returns an empty list of configurations.\n> > +\t\t * stream roles returns an empty camera configuration.\n> >  \t\t */\n> >  \t\tconfig = camera_->generateConfiguration({});\n> > -\t\tif (config.isValid()) {\n> > +\t\tif (!config || config->isValid()) {\n> >  \t\t\tcout << \"Failed to retrieve configuration for empty roles list\"\n> >  \t\t\t     << endl;\n> > +\t\t\tdelete config;\n> >  \t\t\treturn TestFail;\n> >  \t\t}\n> >  \n> > +\t\tdelete config;\n> > +\n> >  \t\treturn TestPass;\n> >  \t}\n> >  };\n> > diff --git a/test/camera/configuration_set.cpp b/test/camera/configuration_set.cpp\n> > index 25b5db67103a..ca911f459c32 100644\n> > --- a/test/camera/configuration_set.cpp\n> > +++ b/test/camera/configuration_set.cpp\n> > @@ -16,13 +16,25 @@ namespace {\n> >  class ConfigurationSet : public CameraTest\n> >  {\n> >  protected:\n> > -\tint run()\n> > +\tint init() override\n> >  \t{\n> > -\t\tCameraConfiguration config =\n> > -\t\t\tcamera_->generateConfiguration({ StreamRole::VideoRecording });\n> > -\t\tStreamConfiguration *cfg = &config[0];\n> > +\t\tCameraTest::init();\n> >  \n> > -\t\tif (!config.isValid()) {\n> > +\t\tconfig_ = camera_->generateConfiguration({ StreamRole::VideoRecording });\n> > +\t\tif (!config_) {\n> > +\t\t\tcout << \"Failed to generate default configuration\" << endl;\n> > +\t\t\tCameraTest::cleanup();\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\treturn TestPass;\n> > +\t}\n> > +\n> > +\tint run() override\n> > +\t{\n> > +\t\tStreamConfiguration &cfg = (*config_)[0];\n> > +\n> > +\t\tif (!config_->isValid()) {\n> >  \t\t\tcout << \"Failed to read default configuration\" << endl;\n> >  \t\t\treturn TestFail;\n> >  \t\t}\n> > @@ -33,7 +45,7 @@ protected:\n> >  \t\t}\n> >  \n> >  \t\t/* Test that setting the default configuration works. */\n> > -\t\tif (camera_->configure(config)) {\n> > +\t\tif (camera_->configure(config_)) {\n> >  \t\t\tcout << \"Failed to set default configuration\" << endl;\n> >  \t\t\treturn TestFail;\n> >  \t\t}\n> > @@ -48,7 +60,7 @@ protected:\n> >  \t\t\treturn TestFail;\n> >  \t\t}\n> >  \n> > -\t\tif (!camera_->configure(config)) {\n> > +\t\tif (!camera_->configure(config_)) {\n> >  \t\t\tcout << \"Setting configuration on a camera not acquired succeeded when it should have failed\"\n> >  \t\t\t     << endl;\n> >  \t\t\treturn TestFail;\n> > @@ -64,9 +76,9 @@ protected:\n> >  \t\t * the default configuration of the VIMC camera is known to\n> >  \t\t * work.\n> >  \t\t */\n> > -\t\tcfg->size.width *= 2;\n> > -\t\tcfg->size.height *= 2;\n> > -\t\tif (camera_->configure(config)) {\n> > +\t\tcfg.size.width *= 2;\n> > +\t\tcfg.size.height *= 2;\n> > +\t\tif (camera_->configure(config_)) {\n> >  \t\t\tcout << \"Failed to set modified configuration\" << endl;\n> >  \t\t\treturn TestFail;\n> >  \t\t}\n> > @@ -74,14 +86,22 @@ protected:\n> >  \t\t/*\n> >  \t\t * Test that setting an invalid configuration fails.\n> >  \t\t */\n> > -\t\tcfg->size = { 0, 0 };\n> > -\t\tif (!camera_->configure(config)) {\n> > +\t\tcfg.size = { 0, 0 };\n> > +\t\tif (!camera_->configure(config_)) {\n> >  \t\t\tcout << \"Invalid configuration incorrectly accepted\" << endl;\n> >  \t\t\treturn TestFail;\n> >  \t\t}\n> >  \n> >  \t\treturn TestPass;\n> >  \t}\n> > +\n> > +\tvoid cleanup() override\n> > +\t{\n> > +\t\tdelete config_;\n> > +\t\tCameraTest::cleanup();\n> > +\t}\n> > +\n> > +\tCameraConfiguration *config_;\n> >  };\n> >  \n> >  } /* namespace */\n> > diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp\n> > index 7a74cd85a37a..a662e869fadf 100644\n> > --- a/test/camera/statemachine.cpp\n> > +++ b/test/camera/statemachine.cpp\n> > @@ -233,10 +233,22 @@ protected:\n> >  \t\treturn TestPass;\n> >  \t}\n> >  \n> > -\tint run()\n> > +\tint init() override\n> >  \t{\n> > +\t\tCameraTest::init();\n> > +\n> >  \t\tdefconf_ = camera_->generateConfiguration({ StreamRole::VideoRecording });\n> > +\t\tif (!defconf_) {\n> > +\t\t\tcout << \"Failed to generate default configuration\" << endl;\n> > +\t\t\tCameraTest::cleanup();\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> >  \n> > +\t\treturn TestPass;\n> > +\t}\n> > +\n> > +\tint run() override\n> > +\t{\n> >  \t\tif (testAvailable() != TestPass) {\n> >  \t\t\tcout << \"State machine in Available state failed\" << endl;\n> >  \t\t\treturn TestFail;\n> > @@ -265,7 +277,13 @@ protected:\n> >  \t\treturn TestPass;\n> >  \t}\n> >  \n> > -\tCameraConfiguration defconf_;\n> > +\tvoid cleanup() override\n> > +\t{\n> > +\t\tdelete defconf_;\n> > +\t\tCameraTest::cleanup();\n> > +\t}\n> > +\n> > +\tCameraConfiguration *defconf_;\n> >  };\n> >  \n> >  } /* namespace */","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 79C7D60C02\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat, 18 May 2019 19:22:41 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BB340D5;\n\tSat, 18 May 2019 19:22:40 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1558200161;\n\tbh=nV9SVCeBy1XF59QrVwSs5X51os+ijcHlbZ7e4XbZEiA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LCHvfW81wr4+rVr07NrMJShsGrKfxJUGlKPQ9b2gB0Ee0PUw/NnW7mJn/FDOTfcxo\n\tPUAIKhNgnMHWD9Ua5IFRv8BL2jgSvknFt53AALtnRxHir/P1IughTexLbowplofZRs\n\tewXBLk11NJMdEZWCENd5VQy9+pd6wXhxkNv0QzdY=","Date":"Sat, 18 May 2019 20:22:24 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190518172224.GB4995@pendragon.ideasonboard.com>","References":"<20190517230621.24668-1-laurent.pinchart@ideasonboard.com>\n\t<20190517230621.24668-6-laurent.pinchart@ideasonboard.com>\n\t<20190518164127.GG25081@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190518164127.GG25081@bigcity.dyn.berto.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH/RFC 05/12] libcamera: camera: Return a\n\tpointer from generateConfiguration()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Sat, 18 May 2019 17:22:41 -0000"}}]