[{"id":1650,"web_url":"https://patchwork.libcamera.org/comment/1650/","msgid":"<20190521203828.nlrl4eqzassag3wa@uno.localdomain>","date":"2019-05-21T20:38:28","subject":"Re: [libcamera-devel] [PATCH v3 4/6] libcamera: camera: Return a\n\tpointer from generateConfiguration()","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Tue, May 21, 2019 at 10:27:38PM +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> 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> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n\nReviewed-by: Jacopo Mondi <jacopo@jmondi.org>\n\nThanks\n  j\n\n> ---\n> Changes since v2:\n>\n> - Return an std::unique_ptr<> from Camera::generateConfiguration()\n> - Document that Camera::generateConfiguration() can take an empty list\n>   of roles\n> ---\n>  include/libcamera/camera.h               |  4 +--\n>  src/cam/main.cpp                         | 38 +++++++++++-------------\n>  src/libcamera/camera.cpp                 | 37 +++++++++++++----------\n>  src/libcamera/include/pipeline_handler.h |  6 ++--\n>  src/libcamera/pipeline/ipu3/ipu3.cpp     | 31 +++++++++----------\n>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 19 +++++++-----\n>  src/libcamera/pipeline/uvcvideo.cpp      | 24 ++++++++-------\n>  src/libcamera/pipeline/vimc.cpp          | 24 ++++++++-------\n>  src/libcamera/pipeline_handler.cpp       |  3 +-\n>  src/qcam/main_window.cpp                 |  6 ++--\n>  src/qcam/main_window.h                   |  3 +-\n>  test/camera/capture.cpp                  | 32 ++++++++++++++------\n>  test/camera/configuration_default.cpp    |  8 ++---\n>  test/camera/configuration_set.cpp        | 38 ++++++++++++++++--------\n>  test/camera/statemachine.cpp             | 30 +++++++++++++------\n>  15 files changed, 178 insertions(+), 125 deletions(-)\n>\n> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> index 284e5276a055..a3a7289a7aa7 100644\n> --- a/include/libcamera/camera.h\n> +++ b/include/libcamera/camera.h\n> @@ -77,8 +77,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> +\tstd::unique_ptr<CameraConfiguration> 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..535c2420893e 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 std::unique_ptr<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,23 +111,22 @@ 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> +\tstd::unique_ptr<CameraConfiguration> 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\treturn nullptr;\n>  \t}\n>\n>  \t/* Apply configuration explicitly requested. */\n>  \tunsigned int i = 0;\n>  \tfor (auto const &value : streamOptions) {\n>  \t\tKeyValueParser::Options conf = value.toKeyValues();\n> -\t\tStreamConfiguration &cfg = (*config)[i++];\n> +\t\tStreamConfiguration &cfg = config->at(i++);\n>\n>  \t\tif (conf.isSet(\"width\"))\n>  \t\t\tcfg.size.width = conf[\"width\"];\n> @@ -142,7 +139,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,16 +188,15 @@ 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> +\tstd::unique_ptr<CameraConfiguration> 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> +\tret = camera->configure(config.get());\n>  \tif (ret < 0) {\n>  \t\tstd::cout << \"Failed to configure camera\" << std::endl;\n>  \t\treturn ret;\n> @@ -208,8 +204,8 @@ static int capture()\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->at(index);\n>  \t\tstreamInfo[cfg.stream()] = \"stream\" + std::to_string(index);\n>  \t}\n>\n> @@ -224,7 +220,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 +240,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> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 5848330f639a..0e5fd7f57271 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -540,27 +540,32 @@ const std::set<Stream *> &Camera::streams() const\n>   *\n>   * Generate a camera configuration for a set of desired stream roles. The caller\n>   * specifies a list of stream roles and the camera returns a configuration\n> - * containing suitable streams and their suggested default configurations.\n> + * containing suitable streams and their suggested default configurations. An\n> + * empty list of roles is valid, and will generate an empty configuration that\n> + * can be filled by the caller.\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> +std::unique_ptr<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->empty())\n> +\t\tmsg << \" empty\";\n> +\n> +\tfor (unsigned int index = 0; index < config->size(); ++index)\n> +\t\tmsg << \" (\" << index << \") \" << config->at(index).toString();\n>\n>  \tLOG(Camera, Debug) << msg.str();\n>\n> -\treturn config;\n> +\treturn std::unique_ptr<CameraConfiguration>(config);\n>  }\n>\n>  /**\n> @@ -590,7 +595,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> @@ -600,7 +605,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> @@ -608,8 +613,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->at(index);\n>  \t\tcfg.setStream(nullptr);\n>  \t\tmsg << \" (\" << index << \") \" << cfg.toString();\n>  \t}\n> @@ -621,7 +626,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..3acf82ff4665 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->at(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->at(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 ec6980b0943a..c8d217abdca8 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> +\tCameraConfiguration *config = new CameraConfiguration();\n> +\n> +\tif (roles.empty())\n> +\t\treturn config;\n> +\n>  \tStreamConfiguration cfg{};\n> -\n>  \tcfg.pixelFormat = V4L2_PIX_FMT_NV12;\n>  \tcfg.size = data->sensor_->resolution();\n>  \tcfg.bufferCount = RKISP1_BUFFER_COUNT;\n>\n> -\tconfig.addConfiguration(cfg);\n> +\tconfig->addConfiguration(cfg);\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->at(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..120d8d3a1522 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> +\tif (roles.empty())\n> +\t\treturn config;\n> +\n> +\tStreamConfiguration cfg{};\n>  \tcfg.pixelFormat = V4L2_PIX_FMT_YUYV;\n>  \tcfg.size = { 640, 480 };\n>  \tcfg.bufferCount = 4;\n>\n> -\tconfig.addConfiguration(cfg);\n> +\tconfig->addConfiguration(cfg);\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->at(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..f6aa32683e5e 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> +\tif (roles.empty())\n> +\t\treturn config;\n> +\n> +\tStreamConfiguration cfg{};\n>  \tcfg.pixelFormat = V4L2_PIX_FMT_RGB24;\n>  \tcfg.size = { 640, 480 };\n>  \tcfg.bufferCount = 4;\n>\n> -\tconfig.addConfiguration(cfg);\n> +\tconfig->addConfiguration(cfg);\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->at(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..16b123132dd9 100644\n> --- a/src/qcam/main_window.cpp\n> +++ b/src/qcam/main_window.cpp\n> @@ -98,13 +98,13 @@ int MainWindow::startCapture()\n>  \tint ret;\n>\n>  \tconfig_ = camera_->generateConfiguration({ StreamRole::VideoRecording });\n> -\tret = camera_->configure(config_);\n> +\tret = camera_->configure(config_.get());\n>  \tif (ret < 0) {\n>  \t\tstd::cout << \"Failed to configure camera\" << std::endl;\n>  \t\treturn ret;\n>  \t}\n>\n> -\tconst StreamConfiguration &cfg = config_[0];\n> +\tconst StreamConfiguration &cfg = config_->at(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,8 @@ void MainWindow::stopCapture()\n>\n>  \tcamera_->freeBuffers();\n>  \tisCapturing_ = false;\n> +\n> +\tconfig_.reset();\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..fe565cbcb460 100644\n> --- a/src/qcam/main_window.h\n> +++ b/src/qcam/main_window.h\n> @@ -8,6 +8,7 @@\n>  #define __QCAM_MAIN_WINDOW_H__\n>\n>  #include <map>\n> +#include <memory>\n>\n>  #include <QMainWindow>\n>\n> @@ -45,7 +46,7 @@ private:\n>\n>  \tstd::shared_ptr<Camera> camera_;\n>  \tbool isCapturing_;\n> -\tCameraConfiguration config_;\n> +\tstd::unique_ptr<CameraConfiguration> config_;\n>\n>  \tViewFinder *viewfinder_;\n>  };\n> diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp\n> index bfd11eefedcf..bb7d380cdc1a 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_->at(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_.get())) {\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,8 @@ protected:\n>\n>  \t\treturn TestPass;\n>  \t}\n> +\n> +\tstd::unique_ptr<CameraConfiguration> config_;\n>  };\n>\n>  } /* namespace */\n> diff --git a/test/camera/configuration_default.cpp b/test/camera/configuration_default.cpp\n> index 8c4a03db498a..8a767d2321e0 100644\n> --- a/test/camera/configuration_default.cpp\n> +++ b/test/camera/configuration_default.cpp\n> @@ -18,21 +18,21 @@ class ConfigurationDefault : public CameraTest\n>  protected:\n>  \tint run()\n>  \t{\n> -\t\tCameraConfiguration config;\n> +\t\tstd::unique_ptr<CameraConfiguration> 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\treturn TestFail;\n>  \t\t}\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\treturn TestFail;\n> diff --git a/test/camera/configuration_set.cpp b/test/camera/configuration_set.cpp\n> index 25b5db67103a..ece987c7752a 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_->at(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_.get())) {\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_.get())) {\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_.get())) {\n>  \t\t\tcout << \"Failed to set modified configuration\" << endl;\n>  \t\t\treturn TestFail;\n>  \t\t}\n> @@ -74,14 +86,16 @@ 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_.get())) {\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> +\tstd::unique_ptr<CameraConfiguration> config_;\n>  };\n>\n>  } /* namespace */\n> diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp\n> index 7a74cd85a37a..d489f197e402 100644\n> --- a/test/camera/statemachine.cpp\n> +++ b/test/camera/statemachine.cpp\n> @@ -19,7 +19,7 @@ protected:\n>  \tint testAvailable()\n>  \t{\n>  \t\t/* Test operations which should fail. */\n> -\t\tif (camera_->configure(defconf_) != -EACCES)\n> +\t\tif (camera_->configure(defconf_.get()) != -EACCES)\n>  \t\t\treturn TestFail;\n>\n>  \t\tif (camera_->allocateBuffers() != -EACCES)\n> @@ -84,7 +84,7 @@ protected:\n>  \t\tif (camera_->acquire())\n>  \t\t\treturn TestFail;\n>\n> -\t\tif (camera_->configure(defconf_))\n> +\t\tif (camera_->configure(defconf_.get()))\n>  \t\t\treturn TestFail;\n>\n>  \t\treturn TestPass;\n> @@ -113,7 +113,7 @@ protected:\n>  \t\t\treturn TestFail;\n>\n>  \t\t/* Test operations which should pass. */\n> -\t\tif (camera_->configure(defconf_))\n> +\t\tif (camera_->configure(defconf_.get()))\n>  \t\t\treturn TestFail;\n>\n>  \t\t/* Test valid state transitions, end in Prepared state. */\n> @@ -123,7 +123,7 @@ protected:\n>  \t\tif (camera_->acquire())\n>  \t\t\treturn TestFail;\n>\n> -\t\tif (camera_->configure(defconf_))\n> +\t\tif (camera_->configure(defconf_.get()))\n>  \t\t\treturn TestFail;\n>\n>  \t\tif (camera_->allocateBuffers())\n> @@ -141,7 +141,7 @@ protected:\n>  \t\tif (camera_->release() != -EBUSY)\n>  \t\t\treturn TestFail;\n>\n> -\t\tif (camera_->configure(defconf_) != -EACCES)\n> +\t\tif (camera_->configure(defconf_.get()) != -EACCES)\n>  \t\t\treturn TestFail;\n>\n>  \t\tif (camera_->allocateBuffers() != -EACCES)\n> @@ -172,7 +172,7 @@ protected:\n>  \t\tif (camera_->acquire())\n>  \t\t\treturn TestFail;\n>\n> -\t\tif (camera_->configure(defconf_))\n> +\t\tif (camera_->configure(defconf_.get()))\n>  \t\t\treturn TestFail;\n>\n>  \t\tif (camera_->allocateBuffers())\n> @@ -193,7 +193,7 @@ protected:\n>  \t\tif (camera_->release() != -EBUSY)\n>  \t\t\treturn TestFail;\n>\n> -\t\tif (camera_->configure(defconf_) != -EACCES)\n> +\t\tif (camera_->configure(defconf_.get()) != -EACCES)\n>  \t\t\treturn TestFail;\n>\n>  \t\tif (camera_->allocateBuffers() != -EACCES)\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,7 @@ protected:\n>  \t\treturn TestPass;\n>  \t}\n>\n> -\tCameraConfiguration defconf_;\n> +\tstd::unique_ptr<CameraConfiguration> 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":"<jacopo@jmondi.org>","Received":["from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7DF3160C40\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 21 May 2019 22:37:22 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id C04B8C0003;\n\tTue, 21 May 2019 20:37:21 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Tue, 21 May 2019 22:38:28 +0200","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190521203828.nlrl4eqzassag3wa@uno.localdomain>","References":"<20190521192740.28112-1-laurent.pinchart@ideasonboard.com>\n\t<20190521192740.28112-5-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"5uxt4757tncfmfuv\"","Content-Disposition":"inline","In-Reply-To":"<20190521192740.28112-5-laurent.pinchart@ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v3 4/6] 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":"Tue, 21 May 2019 20:37:22 -0000"}}]