[{"id":1615,"web_url":"https://patchwork.libcamera.org/comment/1615/","msgid":"<20190518160501.GF25081@bigcity.dyn.berto.se>","date":"2019-05-18T16:05:01","subject":"Re: [libcamera-devel] [PATCH/RFC 04/12] libcamera: Refactor the\n\tcamera configuration storage and API","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 patch.\n\nOn 2019-05-18 02:06:13 +0300, Laurent Pinchart wrote:\n> Refactor the CameraConfiguration structure to not rely on Stream\n> instances. This is a step towards making the camera configuration object\n> more powerful with configuration validation using \"try\" semantics.\n> \n> As applications need access to the Stream instances associated with the\n> configuration entries in order to associate buffers with streams when\n> creating requests, expose the stream selected by the pipeline handler\n> through a new StreamConfiguration::stream().\n> \n> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\nReviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n\n> ---\n>  include/libcamera/camera.h               |  19 ++-\n>  include/libcamera/stream.h               |   7 +\n>  src/cam/main.cpp                         |  35 ++---\n>  src/libcamera/camera.cpp                 | 174 +++++++++--------------\n>  src/libcamera/include/pipeline_handler.h |   2 +-\n>  src/libcamera/pipeline/ipu3/ipu3.cpp     |  32 ++---\n>  src/libcamera/pipeline/rkisp1/rkisp1.cpp |  12 +-\n>  src/libcamera/pipeline/uvcvideo.cpp      |  23 ++-\n>  src/libcamera/pipeline/vimc.cpp          |  23 ++-\n>  src/libcamera/pipeline_handler.cpp       |   4 +\n>  src/libcamera/stream.cpp                 |  22 +++\n>  src/qcam/main_window.cpp                 |   4 +-\n>  test/camera/capture.cpp                  |   4 +-\n>  test/camera/configuration_set.cpp        |   2 +-\n>  14 files changed, 179 insertions(+), 184 deletions(-)\n> \n> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> index 42ba5201eabc..e2759405f497 100644\n> --- a/include/libcamera/camera.h\n> +++ b/include/libcamera/camera.h\n> @@ -25,11 +25,13 @@ class Request;\n>  class CameraConfiguration\n>  {\n>  public:\n> -\tusing iterator = std::vector<Stream *>::iterator;\n> -\tusing const_iterator = std::vector<Stream *>::const_iterator;\n> +\tusing iterator = std::vector<StreamConfiguration>::iterator;\n> +\tusing const_iterator = std::vector<StreamConfiguration>::const_iterator;\n>  \n>  \tCameraConfiguration();\n>  \n> +\tvoid addConfiguration(const StreamConfiguration &cfg);\n> +\n>  \titerator begin();\n>  \titerator end();\n>  \tconst_iterator begin() const;\n> @@ -39,16 +41,11 @@ public:\n>  \tbool isEmpty() const;\n>  \tstd::size_t size() const;\n>  \n> -\tStream *front();\n> -\tconst Stream *front() const;\n> -\n> -\tStream *operator[](unsigned int index) const;\n> -\tStreamConfiguration &operator[](Stream *stream);\n> -\tconst StreamConfiguration &operator[](Stream *stream) const;\n> +\tStreamConfiguration &operator[](unsigned int index);\n> +\tconst StreamConfiguration &operator[](unsigned int index) const;\n>  \n>  private:\n> -\tstd::vector<Stream *> order_;\n> -\tstd::map<Stream *, StreamConfiguration> config_;\n> +\tstd::vector<StreamConfiguration> config_;\n>  };\n>  \n>  class Camera final\n> @@ -72,7 +69,7 @@ public:\n>  \n>  \tconst std::set<Stream *> &streams() const;\n>  \tCameraConfiguration generateConfiguration(const StreamRoles &roles);\n> -\tint configure(const CameraConfiguration &config);\n> +\tint configure(CameraConfiguration &config);\n>  \n>  \tint allocateBuffers();\n>  \tint freeBuffers();\n> diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h\n> index 59bdf217eb31..47c007ed52e2 100644\n> --- a/include/libcamera/stream.h\n> +++ b/include/libcamera/stream.h\n> @@ -16,6 +16,7 @@\n>  namespace libcamera {\n>  \n>  class Camera;\n> +class Stream;\n>  \n>  struct StreamConfiguration {\n>  \tunsigned int pixelFormat;\n> @@ -23,7 +24,13 @@ struct StreamConfiguration {\n>  \n>  \tunsigned int bufferCount;\n>  \n> +\tStream *stream() const { return stream_; }\n> +\tvoid setStream(Stream *stream) { stream_ = stream; }\n> +\n>  \tstd::string toString() const;\n> +\n> +private:\n> +\tStream *stream_;\n>  };\n>  \n>  enum StreamRole {\n> diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> index d603228c0116..cd165bea34cd 100644\n> --- a/src/cam/main.cpp\n> +++ b/src/cam/main.cpp\n> @@ -89,12 +89,9 @@ static int prepareCameraConfig(CameraConfiguration *config)\n>  {\n>  \tStreamRoles roles;\n>  \n> -\tstreamInfo.clear();\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\tstreamInfo[config->front()] = \"stream0\";\n>  \t\treturn 0;\n>  \t}\n>  \n> @@ -129,27 +126,20 @@ static int prepareCameraConfig(CameraConfiguration *config)\n>  \t}\n>  \n>  \t/* Apply configuration explicitly requested. */\n> -\tCameraConfiguration::iterator it = config->begin();\n> +\tunsigned int i = 0;\n>  \tfor (auto const &value : streamOptions) {\n>  \t\tKeyValueParser::Options conf = value.toKeyValues();\n> -\t\tStream *stream = *it;\n> -\t\tit++;\n> +\t\tStreamConfiguration &cfg = (*config)[i++];\n>  \n>  \t\tif (conf.isSet(\"width\"))\n> -\t\t\t(*config)[stream].size.width = conf[\"width\"];\n> +\t\t\tcfg.size.width = conf[\"width\"];\n>  \n>  \t\tif (conf.isSet(\"height\"))\n> -\t\t\t(*config)[stream].size.height = conf[\"height\"];\n> +\t\t\tcfg.size.height = conf[\"height\"];\n>  \n>  \t\t/* TODO: Translate 4CC string to ID. */\n>  \t\tif (conf.isSet(\"pixelformat\"))\n> -\t\t\t(*config)[stream].pixelFormat = conf[\"pixelformat\"];\n> -\t}\n> -\n> -\tunsigned int index = 0;\n> -\tfor (Stream *stream : *config) {\n> -\t\tstreamInfo[stream] = \"stream\" + std::to_string(index);\n> -\t\tindex++;\n> +\t\t\tcfg.pixelFormat = conf[\"pixelformat\"];\n>  \t}\n>  \n>  \treturn 0;\n> @@ -216,6 +206,13 @@ static int capture()\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> +\t\tstreamInfo[cfg.stream()] = \"stream\" + std::to_string(index);\n> +\t}\n> +\n>  \tret = camera->allocateBuffers();\n>  \tif (ret) {\n>  \t\tstd::cerr << \"Failed to allocate buffers\"\n> @@ -227,8 +224,10 @@ static int capture()\n>  \n>  \t/* Identify the stream with the least number of buffers. */\n>  \tunsigned int nbuffers = UINT_MAX;\n> -\tfor (Stream *stream : config)\n> +\tfor (StreamConfiguration &cfg : config) {\n> +\t\tStream *stream = cfg.stream();\n>  \t\tnbuffers = std::min(nbuffers, stream->bufferPool().count());\n> +\t}\n>  \n>  \t/*\n>  \t * TODO: make cam tool smarter to support still capture by for\n> @@ -245,8 +244,10 @@ static int capture()\n>  \t\t}\n>  \n>  \t\tstd::map<Stream *, Buffer *> map;\n> -\t\tfor (Stream *stream : 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>  \n>  \t\tret = request->setBuffers(map);\n>  \t\tif (ret < 0) {\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index a3921f91f1c9..f8a4946b4a6a 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -46,72 +46,81 @@ LOG_DECLARE_CATEGORY(Camera)\n>   * \\class CameraConfiguration\n>   * \\brief Hold configuration for streams of the camera\n>  \n> - * The CameraConfiguration holds an ordered list of streams and their associated\n> - * StreamConfiguration. From a data storage point of view, the class operates as\n> - * a map of Stream pointers to StreamConfiguration, with entries accessed with\n> - * operator[](Stream *). Accessing an entry for a Stream pointer not yet stored\n> - * in the configuration inserts a new empty entry.\n> - *\n> - * The class also suppors iterators, and from that point of view operates as a\n> - * vector of Stream pointers. The streams are iterated in insertion order, and\n> - * the operator[](int) returns the Stream pointer based on its insertion index.\n> - * Accessing a stream with an invalid index returns a null pointer.\n> + * The CameraConfiguration holds an ordered list of stream configurations. It\n> + * supports iterators and operates as a vector of StreamConfiguration instances.\n> + * The stream configurations are inserted by addConfiguration(), and the\n> + * operator[](int) returns a reference to the StreamConfiguration based on its\n> + * insertion index. Accessing a stream configuration with an invalid index\n> + * results in undefined behaviour.\n>   */\n>  \n>  /**\n>   * \\typedef CameraConfiguration::iterator\n> - * \\brief Iterator for the streams in the configuration\n> + * \\brief Iterator for the stream configurations in the camera configuration\n>   */\n>  \n>  /**\n>   * \\typedef CameraConfiguration::const_iterator\n> - * \\brief Const iterator for the streams in the configuration\n> + * \\brief Const iterator for the stream configuration in the camera\n> + * configuration\n>   */\n>  \n>  /**\n>   * \\brief Create an empty camera configuration\n>   */\n>  CameraConfiguration::CameraConfiguration()\n> -\t: order_({}), config_({})\n> +\t: config_({})\n>  {\n>  }\n>  \n>  /**\n> - * \\brief Retrieve an iterator to the first stream in the sequence\n> - * \\return An iterator to the first stream\n> + * \\brief Add a stream configuration to the camera configuration\n> + * \\param[in] cfg The stream configuration\n>   */\n> -std::vector<Stream *>::iterator CameraConfiguration::begin()\n> +void CameraConfiguration::addConfiguration(const StreamConfiguration &cfg)\n>  {\n> -\treturn order_.begin();\n> +\tconfig_.push_back(cfg);\n>  }\n>  \n>  /**\n> - * \\brief Retrieve an iterator pointing to the past-the-end stream in the\n> + * \\brief Retrieve an iterator to the first stream configuration in the\n>   * sequence\n> - * \\return An iterator to the element following the last stream\n> + * \\return An iterator to the first stream configuration\n>   */\n> -std::vector<Stream *>::iterator CameraConfiguration::end()\n> +CameraConfiguration::iterator CameraConfiguration::begin()\n>  {\n> -\treturn order_.end();\n> +\treturn config_.begin();\n>  }\n>  \n>  /**\n> - * \\brief Retrieve a const iterator to the first element of the streams\n> - * \\return A const iterator to the first stream\n> + * \\brief Retrieve an iterator pointing to the past-the-end stream\n> + * configuration in the sequence\n> + * \\return An iterator to the element following the last stream configuration\n>   */\n> -std::vector<Stream *>::const_iterator CameraConfiguration::begin() const\n> +CameraConfiguration::iterator CameraConfiguration::end()\n>  {\n> -\treturn order_.begin();\n> +\treturn config_.end();\n>  }\n>  \n>  /**\n> - * \\brief Retrieve a const iterator pointing to the past-the-end stream in the\n> - * sequence\n> + * \\brief Retrieve a const iterator to the first element of the stream\n> + * configurations\n> + * \\return A const iterator to the first stream configuration\n> + */\n> +CameraConfiguration::const_iterator CameraConfiguration::begin() const\n> +{\n> +\treturn config_.begin();\n> +}\n> +\n> +/**\n> + * \\brief Retrieve a const iterator pointing to the past-the-end stream\n> + * configuration in the sequence\n>   * \\return A const iterator to the element following the last stream\n> + * configuration\n>   */\n> -std::vector<Stream *>::const_iterator CameraConfiguration::end() const\n> +CameraConfiguration::const_iterator CameraConfiguration::end() const\n>  {\n> -\treturn order_.end();\n> +\treturn config_.end();\n>  }\n>  \n>  /**\n> @@ -128,9 +137,7 @@ bool CameraConfiguration::isValid() const\n>  \tif (isEmpty())\n>  \t\treturn false;\n>  \n> -\tfor (auto const &it : config_) {\n> -\t\tconst StreamConfiguration &cfg = it.second;\n> -\n> +\tfor (const StreamConfiguration &cfg : config_) {\n>  \t\tif (cfg.size.width == 0 || cfg.size.height == 0 ||\n>  \t\t    cfg.pixelFormat == 0 || cfg.bufferCount == 0)\n>  \t\t\treturn false;\n> @@ -145,7 +152,7 @@ bool CameraConfiguration::isValid() const\n>   */\n>  bool CameraConfiguration::isEmpty() const\n>  {\n> -\treturn order_.empty();\n> +\treturn config_.empty();\n>  }\n>  \n>  /**\n> @@ -154,75 +161,37 @@ bool CameraConfiguration::isEmpty() const\n>   */\n>  std::size_t CameraConfiguration::size() const\n>  {\n> -\treturn order_.size();\n> -}\n> -\n> -/**\n> - * \\brief Access the first stream in the configuration\n> - * \\return The first stream in the configuration\n> - */\n> -Stream *CameraConfiguration::front()\n> -{\n> -\treturn order_.front();\n> -}\n> -\n> -/**\n> - * \\brief Access the first stream in the configuration\n> - * \\return The first const stream pointer in the configuration\n> - */\n> -const Stream *CameraConfiguration::front() const\n> -{\n> -\treturn order_.front();\n> -}\n> -\n> -/**\n> - * \\brief Retrieve a stream pointer from index\n> - * \\param[in] index Numerical index\n> - *\n> - * The \\a index represents the zero based insertion order of stream and stream\n> - * configuration into the camera configuration.\n> - *\n> - * \\return The stream pointer at index, or a nullptr if the index is out of\n> - * bounds\n> - */\n> -Stream *CameraConfiguration::operator[](unsigned int index) const\n> -{\n> -\tif (index >= order_.size())\n> -\t\treturn nullptr;\n> -\n> -\treturn order_.at(index);\n> +\treturn config_.size();\n>  }\n>  \n>  /**\n>   * \\brief Retrieve a reference to a stream configuration\n> - * \\param[in] stream Stream to retrieve configuration for\n> + * \\param[in] index Numerical index\n>   *\n> - * If the camera configuration does not yet contain a configuration for\n> - * the requested stream, create and return an empty stream configuration.\n> + * The \\a index represents the zero based insertion order of stream\n> + * configuration into the camera configuration with addConfiguration(). Calling\n> + * this method with an invalid index results in undefined behaviour.\n>   *\n> - * \\return The configuration for the stream\n> + * \\return The stream configuration\n>   */\n> -StreamConfiguration &CameraConfiguration::operator[](Stream *stream)\n> +StreamConfiguration &CameraConfiguration::operator[](unsigned int index)\n>  {\n> -\tif (config_.find(stream) == config_.end())\n> -\t\torder_.push_back(stream);\n> -\n> -\treturn config_[stream];\n> +\treturn config_[index];\n>  }\n>  \n>  /**\n>   * \\brief Retrieve a const reference to a stream configuration\n> - * \\param[in] stream Stream to retrieve configuration for\n> + * \\param[in] index Numerical index\n>   *\n> - * No new stream configuration is created if called with \\a stream that is not\n> - * already part of the camera configuration, doing so is an invalid operation\n> - * and results in undefined behaviour.\n> + * The \\a index represents the zero based insertion order of stream\n> + * configuration into the camera configuration with addConfiguration(). Calling\n> + * this method with an invalid index results in undefined behaviour.\n>   *\n> - * \\return The configuration for the stream\n> + * \\return The stream configuration\n>   */\n> -const StreamConfiguration &CameraConfiguration::operator[](Stream *stream) const\n> +const StreamConfiguration &CameraConfiguration::operator[](unsigned int index) const\n>  {\n> -\treturn config_.at(stream);\n> +\treturn config_[index];\n>  }\n>  \n>  /**\n> @@ -561,13 +530,9 @@ Camera::generateConfiguration(const StreamRoles &roles)\n>  \tCameraConfiguration config = pipe_->generateConfiguration(this, roles);\n>  \n>  \tstd::ostringstream msg(\"streams configuration:\", std::ios_base::ate);\n> -\tunsigned int index = 0;\n>  \n> -\tfor (Stream *stream : config) {\n> -\t\tconst StreamConfiguration &cfg = config[stream];\n> -\t\tmsg << \" (\" << index << \") \" << cfg.toString();\n> -\t\tindex++;\n> -\t}\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> @@ -593,12 +558,15 @@ Camera::generateConfiguration(const StreamRoles &roles)\n>   *\n>   * This function affects the state of the camera, see \\ref camera_operation.\n>   *\n> + * Upon return the StreamConfiguration entries in \\a config are associated with\n> + * Stream instances which can be retrieved with StreamConfiguration::stream().\n> + *\n>   * \\return 0 on success or a negative error code otherwise\n>   * \\retval -ENODEV The camera has been disconnected from the system\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(const CameraConfiguration &config)\n> +int Camera::configure(CameraConfiguration &config)\n>  {\n>  \tint ret;\n>  \n> @@ -615,16 +583,11 @@ int Camera::configure(const CameraConfiguration &config)\n>  \t}\n>  \n>  \tstd::ostringstream msg(\"configuring streams:\", std::ios_base::ate);\n> -\tunsigned int index = 0;\n>  \n> -\tfor (Stream *stream : config) {\n> -\t\tif (streams_.find(stream) == streams_.end())\n> -\t\t\treturn -EINVAL;\n> -\n> -\t\tconst StreamConfiguration &cfg = config[stream];\n> -\t\tmsg << std::dec << \" (\" << index << \") \" << cfg.toString();\n> -\n> -\t\tindex++;\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>  \n>  \tLOG(Camera, Info) << msg.str();\n> @@ -634,8 +597,11 @@ int Camera::configure(const CameraConfiguration &config)\n>  \t\treturn ret;\n>  \n>  \tactiveStreams_.clear();\n> -\tfor (Stream *stream : config) {\n> -\t\tconst StreamConfiguration &cfg = config[stream];\n> +\tfor (const StreamConfiguration &cfg : config) {\n> +\t\tStream *stream = cfg.stream();\n> +\t\tif (!stream)\n> +\t\t\tLOG(Camera, Fatal)\n> +\t\t\t\t<< \"Pipeline handler failed to update stream configuration\";\n>  \n>  \t\tstream->configuration_ = cfg;\n>  \t\tactiveStreams_.insert(stream);\n> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> index 3352cb0e5bc9..a025905ab68f 100644\n> --- a/src/libcamera/include/pipeline_handler.h\n> +++ b/src/libcamera/include/pipeline_handler.h\n> @@ -62,7 +62,7 @@ public:\n>  \n>  \tvirtual CameraConfiguration\n>  \tgenerateConfiguration(Camera *camera, const StreamRoles &roles) = 0;\n> -\tvirtual int configure(Camera *camera, const CameraConfiguration &config) = 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 d234a8ac5289..ed0ef69de1d1 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -152,8 +152,7 @@ public:\n>  \n>  \tCameraConfiguration\n>  \tgenerateConfiguration(Camera *camera, const StreamRoles &roles) override;\n> -\tint configure(Camera *camera,\n> -\t\t      const CameraConfiguration &config) 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> @@ -299,14 +298,13 @@ PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n>  \t\tcfg.pixelFormat = V4L2_PIX_FMT_NV12;\n>  \t\tcfg.bufferCount = IPU3_BUFFER_COUNT;\n>  \n> -\t\tconfig[stream] = cfg;\n> +\t\tconfig.addConfiguration(cfg);\n>  \t}\n>  \n>  \treturn config;\n>  }\n>  \n> -int PipelineHandlerIPU3::configure(Camera *camera,\n> -\t\t\t\t   const CameraConfiguration &config)\n> +int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration &config)\n>  {\n>  \tIPU3CameraData *data = cameraData(camera);\n>  \tIPU3Stream *outStream = &data->outStream_;\n> @@ -318,9 +316,13 @@ int PipelineHandlerIPU3::configure(Camera *camera,\n>  \n>  \toutStream->active_ = false;\n>  \tvfStream->active_ = false;\n> -\tfor (Stream *s : config) {\n> -\t\tIPU3Stream *stream = static_cast<IPU3Stream *>(s);\n> -\t\tconst StreamConfiguration &cfg = config[stream];\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> +\t\t * reworked when implementing camera configuration validation.\n> +\t\t */\n> +\t\tIPU3Stream *stream = vfStream->active_ ? outStream : vfStream;\n>  \n>  \t\t/*\n>  \t\t * Verify that the requested size respects the IPU3 alignment\n> @@ -355,6 +357,7 @@ int PipelineHandlerIPU3::configure(Camera *camera,\n>  \t\t\tsensorSize.height = cfg.size.height;\n>  \n>  \t\tstream->active_ = true;\n> +\t\tcfg.setStream(stream);\n>  \t}\n>  \n>  \t/*\n> @@ -379,10 +382,9 @@ int PipelineHandlerIPU3::configure(Camera *camera,\n>  \t\treturn ret;\n>  \n>  \t/* Apply the format to the configured streams output devices. */\n> -\tfor (Stream *s : config) {\n> -\t\tIPU3Stream *stream = static_cast<IPU3Stream *>(s);\n> -\n> -\t\tret = imgu->configureOutput(stream->device_, config[stream]);\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>  \t\t\treturn ret;\n>  \t}\n> @@ -393,15 +395,13 @@ int PipelineHandlerIPU3::configure(Camera *camera,\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_,\n> -\t\t\t\t\t    config[vfStream]);\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_,\n> -\t\t\t\t\t    config[outStream]);\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 05dd517846d6..6b35aa3fe7c3 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -36,8 +36,7 @@ public:\n>  \n>  \tCameraConfiguration generateConfiguration(Camera *camera,\n>  \t\tconst StreamRoles &roles) override;\n> -\tint configure(Camera *camera,\n> -\t\tconst 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> @@ -117,16 +116,15 @@ CameraConfiguration PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n>  \tcfg.size = data->sensor_->resolution();\n>  \tcfg.bufferCount = RKISP1_BUFFER_COUNT;\n>  \n> -\tconfig[&data->stream_] = cfg;\n> +\tconfig.addConfiguration(cfg);\n>  \n>  \treturn config;\n>  }\n>  \n> -int PipelineHandlerRkISP1::configure(Camera *camera,\n> -\t\t\t\t     const CameraConfiguration &config)\n> +int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration &config)\n>  {\n>  \tRkISP1CameraData *data = cameraData(camera);\n> -\tconst StreamConfiguration &cfg = config[&data->stream_];\n> +\tStreamConfiguration &cfg = config[0];\n>  \tCameraSensor *sensor = data->sensor_;\n>  \tint ret;\n>  \n> @@ -220,6 +218,8 @@ int PipelineHandlerRkISP1::configure(Camera *camera,\n>  \t\treturn -EINVAL;\n>  \t}\n>  \n> +\tcfg.setStream(&data->stream_);\n> +\n>  \treturn 0;\n>  }\n>  \n> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> index d2e1f7d4e5b2..5dcc868b2fc9 100644\n> --- a/src/libcamera/pipeline/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> @@ -27,8 +27,7 @@ public:\n>  \n>  \tCameraConfiguration\n>  \tgenerateConfiguration(Camera *camera, const StreamRoles &roles) override;\n> -\tint configure(Camera *camera,\n> -\t\t      const CameraConfiguration &config) 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> @@ -78,38 +77,38 @@ CameraConfiguration\n>  PipelineHandlerUVC::generateConfiguration(Camera *camera,\n>  \t\t\t\t\t  const StreamRoles &roles)\n>  {\n> -\tUVCCameraData *data = cameraData(camera);\n>  \tCameraConfiguration config;\n> -\tStreamConfiguration cfg{};\n> +\tStreamConfiguration cfg;\n>  \n>  \tcfg.pixelFormat = V4L2_PIX_FMT_YUYV;\n>  \tcfg.size = { 640, 480 };\n>  \tcfg.bufferCount = 4;\n>  \n> -\tconfig[&data->stream_] = cfg;\n> +\tconfig.addConfiguration(cfg);\n>  \n>  \treturn config;\n>  }\n>  \n> -int PipelineHandlerUVC::configure(Camera *camera,\n> -\t\t\t\t  const CameraConfiguration &config)\n> +int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration &config)\n>  {\n>  \tUVCCameraData *data = cameraData(camera);\n> -\tconst StreamConfiguration *cfg = &config[&data->stream_];\n> +\tStreamConfiguration &cfg = config[0];\n>  \tint ret;\n>  \n>  \tV4L2DeviceFormat format = {};\n> -\tformat.fourcc = cfg->pixelFormat;\n> -\tformat.size = cfg->size;\n> +\tformat.fourcc = cfg.pixelFormat;\n> +\tformat.size = cfg.size;\n>  \n>  \tret = data->video_->setFormat(&format);\n>  \tif (ret)\n>  \t\treturn ret;\n>  \n> -\tif (format.size != cfg->size ||\n> -\t    format.fourcc != cfg->pixelFormat)\n> +\tif (format.size != cfg.size ||\n> +\t    format.fourcc != cfg.pixelFormat)\n>  \t\treturn -EINVAL;\n>  \n> +\tcfg.setStream(&data->stream_);\n> +\n>  \treturn 0;\n>  }\n>  \n> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> index 17e2491e5c27..af6b6f21e3c5 100644\n> --- a/src/libcamera/pipeline/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc.cpp\n> @@ -27,8 +27,7 @@ public:\n>  \n>  \tCameraConfiguration\n>  \tgenerateConfiguration(Camera *camera, const StreamRoles &roles) override;\n> -\tint configure(Camera *camera,\n> -\t\t      const CameraConfiguration &config) 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> @@ -78,38 +77,38 @@ CameraConfiguration\n>  PipelineHandlerVimc::generateConfiguration(Camera *camera,\n>  \t\t\t\t\t   const StreamRoles &roles)\n>  {\n> -\tVimcCameraData *data = cameraData(camera);\n>  \tCameraConfiguration config;\n> -\tStreamConfiguration cfg{};\n> +\tStreamConfiguration cfg;\n>  \n>  \tcfg.pixelFormat = V4L2_PIX_FMT_RGB24;\n>  \tcfg.size = { 640, 480 };\n>  \tcfg.bufferCount = 4;\n>  \n> -\tconfig[&data->stream_] = cfg;\n> +\tconfig.addConfiguration(cfg);\n>  \n>  \treturn config;\n>  }\n>  \n> -int PipelineHandlerVimc::configure(Camera *camera,\n> -\t\t\t\t   const CameraConfiguration &config)\n> +int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration &config)\n>  {\n>  \tVimcCameraData *data = cameraData(camera);\n> -\tconst StreamConfiguration *cfg = &config[&data->stream_];\n> +\tStreamConfiguration &cfg = config[0];\n>  \tint ret;\n>  \n>  \tV4L2DeviceFormat format = {};\n> -\tformat.fourcc = cfg->pixelFormat;\n> -\tformat.size = cfg->size;\n> +\tformat.fourcc = cfg.pixelFormat;\n> +\tformat.size = cfg.size;\n>  \n>  \tret = data->video_->setFormat(&format);\n>  \tif (ret)\n>  \t\treturn ret;\n>  \n> -\tif (format.size != cfg->size ||\n> -\t    format.fourcc != cfg->pixelFormat)\n> +\tif (format.size != cfg.size ||\n> +\t    format.fourcc != cfg.pixelFormat)\n>  \t\treturn -EINVAL;\n>  \n> +\tcfg.setStream(&data->stream_);\n> +\n>  \treturn 0;\n>  }\n>  \n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index 81c11149c9fe..4185e3557dcb 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -255,6 +255,10 @@ void PipelineHandler::unlock()\n>   * configuration of a subset of the streams can't be satisfied, the\n>   * whole configuration is considered invalid.\n>   *\n> + * Once the configuration is validated and the camera configured, the pipeline\n> + * handler shall associate a Stream instance to each StreamConfiguration entry\n> + * in the CameraConfiguration with the StreamConfiguration::setStream() method.\n> + *\n>   * \\return 0 on success or a negative error code otherwise\n>   */\n>  \n> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp\n> index fe4c4ecf4150..0c59a31a3a05 100644\n> --- a/src/libcamera/stream.cpp\n> +++ b/src/libcamera/stream.cpp\n> @@ -58,6 +58,28 @@ namespace libcamera {\n>   * \\brief Requested number of buffers to allocate for the stream\n>   */\n>  \n> +/**\n> + * \\fn StreamConfiguration::stream()\n> + * \\brief Retrieve the stream associated with the configuration\n> + *\n> + * When a camera is configured with Camera::configure() Stream instances are\n> + * associated with each stream configuration entry. This method retrieves the\n> + * associated Stream, which remains valid until the next call to\n> + * Camera::configure() or Camera::release().\n> + *\n> + * \\return The stream associated with the configuration\n> + */\n> +\n> +/**\n> + * \\fn StreamConfiguration::setStream()\n> + * \\brief Associate a stream with a configuration\n> + *\n> + * This method is meant for the PipelineHandler::configure() method and shall\n> + * not be called by applications.\n> + *\n> + * \\param[in] stream The stream\n> + */\n> +\n>  /**\n>   * \\brief Assemble and return a string describing the configuration\n>   *\n> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> index a984aaca764f..06ae2985f80d 100644\n> --- a/src/qcam/main_window.cpp\n> +++ b/src/qcam/main_window.cpp\n> @@ -98,14 +98,14 @@ int MainWindow::startCapture()\n>  \tint ret;\n>  \n>  \tconfig_ = camera_->generateConfiguration({ StreamRole::VideoRecording });\n> -\tStream *stream = config_.front();\n>  \tret = camera_->configure(config_);\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_[stream];\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>  \tif (ret < 0) {\n> diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp\n> index e7e6438203b9..bfd11eefedcf 100644\n> --- a/test/camera/capture.cpp\n> +++ b/test/camera/capture.cpp\n> @@ -44,8 +44,7 @@ protected:\n>  \t{\n>  \t\tCameraConfiguration config =\n>  \t\t\tcamera_->generateConfiguration({ StreamRole::VideoRecording });\n> -\t\tStream *stream = config.front();\n> -\t\tStreamConfiguration *cfg = &config[stream];\n> +\t\tStreamConfiguration *cfg = &config[0];\n>  \n>  \t\tif (!config.isValid()) {\n>  \t\t\tcout << \"Failed to read default configuration\" << endl;\n> @@ -67,6 +66,7 @@ protected:\n>  \t\t\treturn TestFail;\n>  \t\t}\n>  \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> diff --git a/test/camera/configuration_set.cpp b/test/camera/configuration_set.cpp\n> index 76d8bc3e40a4..25b5db67103a 100644\n> --- a/test/camera/configuration_set.cpp\n> +++ b/test/camera/configuration_set.cpp\n> @@ -20,7 +20,7 @@ protected:\n>  \t{\n>  \t\tCameraConfiguration config =\n>  \t\t\tcamera_->generateConfiguration({ StreamRole::VideoRecording });\n> -\t\tStreamConfiguration *cfg = &config[config.front()];\n> +\t\tStreamConfiguration *cfg = &config[0];\n>  \n>  \t\tif (!config.isValid()) {\n>  \t\t\tcout << \"Failed to read default configuration\" << endl;\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-lf1-x12f.google.com (mail-lf1-x12f.google.com\n\t[IPv6:2a00:1450:4864:20::12f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CAED660C02\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat, 18 May 2019 18:05:03 +0200 (CEST)","by mail-lf1-x12f.google.com with SMTP id y13so7379067lfh.9\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat, 18 May 2019 09:05:03 -0700 (PDT)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tr11sm2444293ljd.91.2019.05.18.09.05.01\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tSat, 18 May 2019 09:05:01 -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=4q7FFMT21O57se79N40eKDSXap1utK+ZTKL3SOv7khU=;\n\tb=1Kr1RvYTGXRH4fFprPepaUDoQkW+rSZ93ArhIkER5Z3Gi4R/UHNaikO/i5BRYM6goT\n\tWCfvo1O3H/kDkaM1GJ6tjJX0c3nuVxZfjdxtkVatT4mTC0FswQ0/KmAz0G+dfaxYWZmy\n\tAIbU6Zw2fSN/3fog5MamNc4JZTaIuolVHGVITK0TK/Rn8DJxsP0TGmseg5Z5CeYOCCiI\n\t3hdLZdq8nuV1QCdYeLeGUhIHRJyfAf6F2d4X+yY3vLRwa6jYjgBvAfHbSlpePq2Gc4Qu\n\t66JZwVHIh3j2ey6jnmetASO0ZxcbZH81kEGuVib60zxP0A+6bPZ2qBkyB36mY3DqX7G3\n\tCzUQ==","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=4q7FFMT21O57se79N40eKDSXap1utK+ZTKL3SOv7khU=;\n\tb=C7fdp0ggr8j5qUC2Ezjgebigy5POq/JkEBaTpIjJQGOLFfin8xTCqMnP8GC5rTO1cV\n\tJ1HmDcRN+hFQnhU/4n4KO2jRqT/UIFlikKeuqJp/UFCkaqGUWkQRmy2sxQd3D6h1MFwU\n\tMZZ4nIdC5VJxeSIpauaJ+DKBL6jmr6r7WNmMwscfV3a+Upm5HR2ftA1lRbhXhf7CFkre\n\t10gq2dqjWNuUWjRKvQSo4J7Rr+OtFURm8gLynjMBkiqHudfw8STPSKCsyXn/IPRbDplY\n\tkCODw+wW8JtcNVeZ/j+WqmFAllF9Q8PVbcBwY6u5Si6G49c4nQkJEbUWwDfo+gTigjgv\n\tvYlg==","X-Gm-Message-State":"APjAAAUljMTNrMuK3hpLj/UcqRGLkl4QCIa9mbGuBsngQxCmxDe/FBvE\n\t4UKwDuuVF3+e3C1cXGjktjptJB+3I88=","X-Google-Smtp-Source":"APXvYqy0yi2VQlPwXiFrXbse3MfbxauiVHdwPW+MOdd58k//n88x3pA2DAngnCm0DM56bxNLIOlCkA==","X-Received":"by 2002:ac2:5474:: with SMTP id\n\te20mr30365031lfn.59.1558195502987; \n\tSat, 18 May 2019 09:05:02 -0700 (PDT)","Date":"Sat, 18 May 2019 18:05:01 +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":"<20190518160501.GF25081@bigcity.dyn.berto.se>","References":"<20190517230621.24668-1-laurent.pinchart@ideasonboard.com>\n\t<20190517230621.24668-5-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-5-laurent.pinchart@ideasonboard.com>","User-Agent":"Mutt/1.11.3 (2019-02-01)","Subject":"Re: [libcamera-devel] [PATCH/RFC 04/12] libcamera: Refactor the\n\tcamera configuration storage and API","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:05:04 -0000"}}]