[{"id":1636,"web_url":"https://patchwork.libcamera.org/comment/1636/","msgid":"<20190521091941.xocjngsp3yobosie@uno.localdomain>","date":"2019-05-21T09:19:41","subject":"Re: [libcamera-devel] [PATCH v2 3/6] libcamera: Refactor the camera\n\tconfiguration storage and API","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Sun, May 19, 2019 at 06:00:44PM +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> The CameraConfiguration now exposes a simple vector-like API to access\n> the contained stream configurations. Both operator[]() and at() are\n> provided to access elements. The isEmpty() method is renamed to empty()\n> and the methods reordered to match the std::vector class.\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 \n> Reviewed-by: Niklas Söderlund \n> ---\n> include/libcamera/camera.h | 36 +--\n> include/libcamera/stream.h | 7 +\n> src/cam/main.cpp | 35 +--\n> src/libcamera/camera.cpp | 268 +++++++++++------------\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, 251 insertions(+), 223 deletions(-)\n>\n> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> index 42ba5201eabc..284e5276a055 100644\n> --- a/include/libcamera/camera.h\n> +++ b/include/libcamera/camera.h\n> @@ -25,30 +25,36 @@ class Request;\n> class CameraConfiguration\n> {\n> public:\n> -\tusing iterator = std::vector::iterator;\n> -\tusing const_iterator = std::vector::const_iterator;\n> +\tusing iterator = std::vector::iterator;\n> +\tusing const_iterator = std::vector::const_iterator;\n>\n> \tCameraConfiguration();\n>\n> +\tvoid addConfiguration(const StreamConfiguration &cfg);\n\nI would use push_back() to match the std::vector API\n\n> +\n> +\tbool isValid() const;\n\nAs you s/isEmpty()/empty()/ should we s/isValid()/valid()/ ?\n\nAh, don't worry, this will become validate(), just noticed\n\n> +\n> +\tStreamConfiguration &at(unsigned int index);\n> +\tconst StreamConfiguration &at(unsigned int index) const;\n> +\tStreamConfiguration &operator[](unsigned int index)\n> +\t{\n> +\t\treturn at(index);\n> +\t}\n> +\tconst StreamConfiguration &operator[](unsigned int index) const\n> +\t{\n> +\t\treturn at(index);\n> +\t}\n\nOk, so we have [] and at() like std::vector, but it seems to me their\nbehaviour is inverted.\n\nstd::vector::at() performs bound checking, and\nCameraConfiguration::at() is implemented with std::vector::operator[],\nwhich does not perform bounds checking\n\nstd::vector::operator[] does not perform bounds checking, but\nCameraConfiguration::operator[] is implemented with std::vector::at()\nwhich performs bound checking.\n\nIs this intentional ?\nI would also question the need to have both operator[] and at()\naccessors, I know std::vector does, but do we need that or is it just\nan API expansion we could save?\n\n> +\n> \titerator begin();\n> -\titerator end();\n> \tconst_iterator begin() const;\n> +\titerator end();\n> \tconst_iterator end() const;\n>\n> -\tbool isValid() const;\n> -\tbool isEmpty() const;\n> +\tbool empty() 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> -\n> private:\n> -\tstd::vector order_;\n> -\tstd::map config_;\n> +\tstd::vector config_;\n> };\n>\n> class Camera final\n> @@ -72,7 +78,7 @@ public:\n>\n> \tconst std::set &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\nShould we protect access to unitialized streams_ ?\nIf StreamConfiguration is not inialized to {} what's the value of\n*stream_ ?\n\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\nnit and not related to this patch: s/conf/opt ? otherwise we have\n*config and cfg in this loop, which is quite confusing.\n\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 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..5848330f639a 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -46,72 +46,40 @@ 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\nAs operator[] is implemented with std::vector::at() accessing with an\ninvalid index, an exception is thrown (even if we don't use them in\nthe library).\n\nI would document CameraConfiguration::at() as well, or just provide one of\nthe two only.\n\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::iterator CameraConfiguration::begin()\n> +void CameraConfiguration::addConfiguration(const StreamConfiguration &cfg)\n> {\n> -\treturn order_.begin();\n> -}\n> -\n> -/**\n> - * \\brief Retrieve an iterator pointing to the past-the-end stream in the\n> - * sequence\n> - * \\return An iterator to the element following the last stream\n> - */\n> -std::vector::iterator CameraConfiguration::end()\n> -{\n> -\treturn order_.end();\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> - */\n> -std::vector::const_iterator CameraConfiguration::begin() const\n> -{\n> -\treturn order_.begin();\n> -}\n> -\n> -/**\n> - * \\brief Retrieve a const iterator pointing to the past-the-end stream in the\n> - * sequence\n> - * \\return A const iterator to the element following the last stream\n> - */\n> -std::vector::const_iterator CameraConfiguration::end() const\n> -{\n> -\treturn order_.end();\n> +\tconfig_.push_back(cfg);\n> }\n>\n> /**\n> @@ -125,12 +93,10 @@ std::vector::const_iterator CameraConfiguration::end() const\n> */\n> bool CameraConfiguration::isValid() const\n> {\n> -\tif (isEmpty())\n> +\tif (empty())\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> @@ -139,13 +105,108 @@ bool CameraConfiguration::isValid() const\n> \treturn true;\n> }\n>\n> +/**\n> + * \\brief Retrieve a reference to a stream configuration\n> + * \\param[in] index Numerical index\n\nSTL uses pos in place of index. Not sure we care, though.\n\n> + *\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 stream configuration\n\n> + */\n> +StreamConfiguration &CameraConfiguration::at(unsigned int index)\n> +{\n> +\treturn config_[index];\n> +}\n> +\n> +/**\n> + * \\brief Retrieve a const reference to a stream configuration\n> + * \\param[in] index Numerical index\n> + *\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 stream configuration\n> + */\n> +const StreamConfiguration &CameraConfiguration::at(unsigned int index) const\n> +{\n> +\treturn config_[index];\n> +}\n> +\n> +/**\n> + * \\fn StreamConfiguration &CameraConfiguration::operator[](unsigned int)\n> + * \\brief Retrieve a reference to a stream configuration\n> + * \\param[in] index Numerical index\n> + *\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 stream configuration\n> + */\n> +\n> +/**\n> + * \\fn const StreamConfiguration &CameraConfiguration::operator[](unsigned int) const\n> + * \\brief Retrieve a const reference to a stream configuration\n> + * \\param[in] index Numerical index\n> + *\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 stream configuration\n> + */\n> +\n> +/**\n> + * \\brief Retrieve an iterator to the first stream configuration in the\n> + * sequence\n> + * \\return An iterator to the first stream configuration\n> + */\n> +CameraConfiguration::iterator CameraConfiguration::begin()\n> +{\n> +\treturn config_.begin();\n> +}\n> +\n> +/**\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 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> +CameraConfiguration::iterator CameraConfiguration::end()\n\nAren't these StreamConfiguration::iterators ?\n\n> +{\n> +\treturn config_.end();\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> +CameraConfiguration::const_iterator CameraConfiguration::end() const\n> +{\n> +\treturn config_.end();\n> +}\n> +\n> /**\n> * \\brief Check if the camera configuration is empty\n> * \\return True if the configuration is empty\n> */\n> -bool CameraConfiguration::isEmpty() const\n> +bool CameraConfiguration::empty() const\n> {\n> -\treturn order_.empty();\n> +\treturn config_.empty();\n> }\n>\n> /**\n> @@ -154,75 +215,7 @@ 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> -}\n> -\n> -/**\n> - * \\brief Retrieve a reference to a stream configuration\n> - * \\param[in] stream Stream to retrieve configuration for\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> - *\n> - * \\return The configuration for the stream\n> - */\n> -StreamConfiguration &CameraConfiguration::operator[](Stream *stream)\n> -{\n> -\tif (config_.find(stream) == config_.end())\n> -\t\torder_.push_back(stream);\n> -\n> -\treturn config_[stream];\n> -}\n> -\n> -/**\n> - * \\brief Retrieve a const reference to a stream configuration\n> - * \\param[in] stream Stream to retrieve configuration for\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> - *\n> - * \\return The configuration for the stream\n> - */\n> -const StreamConfiguration &CameraConfiguration::operator[](Stream *stream) const\n> -{\n> -\treturn config_.at(stream);\n> +\treturn config_.size();\n> }\n>\n> /**\n> @@ -561,13 +554,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 +582,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 +607,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\nIsn't this better printed after pipe->configure(), once we know all\nstreams configuration have a stream assigned ?\n\nOverall this is very good. The fact StreamConfiguration instances are\nassociated to Stream just after configure() might require clearly\npreventing applications to try access it, but I think the\ndocumentation is quite clear on that. The biggest part is discussing\nthe CameraConfiguration API I guess, which we might want to make more\nsimilar to std::vector.\n\nThanks\n j\n\n> \t}\n>\n> \tLOG(Camera, Info) << msg.str();\n> @@ -634,8 +621,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 &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 &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(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(s);\n> -\n> -\t\tret = imgu->configureOutput(stream->device_, config[stream]);\n> +\tfor (StreamConfiguration &cfg : config) {\n> +\t\tIPU3Stream *stream = static_cast(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 4bd8c5101a96..ec6980b0943a 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 &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> @@ -217,6 +215,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 &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 &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 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":"","Received":["from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1ACC960C1D\n\tfor ;\n\tTue, 21 May 2019 11:18:36 +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 relay4-d.mail.gandi.net (Postfix) with ESMTPSA id 4A5DAE0019;\n\tTue, 21 May 2019 09:18:34 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Tue, 21 May 2019 11:19:41 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190521091941.xocjngsp3yobosie@uno.localdomain>","References":"<20190519150047.12444-1-laurent.pinchart@ideasonboard.com>\n\t<20190519150047.12444-4-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"oogfvzxqk6wrfvd5\"","Content-Disposition":"inline","In-Reply-To":"<20190519150047.12444-4-laurent.pinchart@ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v2 3/6] libcamera: Refactor the camera\n\tconfiguration storage and API","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 21 May 2019 09:18:36 -0000"}},{"id":1642,"web_url":"https://patchwork.libcamera.org/comment/1642/","msgid":"<20190521131436.GE5674@pendragon.ideasonboard.com>","date":"2019-05-21T13:14:36","subject":"Re: [libcamera-devel] [PATCH v2 3/6] libcamera: Refactor the camera\n\tconfiguration storage and API","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Tue, May 21, 2019 at 11:19:41AM +0200, Jacopo Mondi wrote:\n> On Sun, May 19, 2019 at 06:00:44PM +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> > The CameraConfiguration now exposes a simple vector-like API to access\n> > the contained stream configurations. Both operator[]() and at() are\n> > provided to access elements. The isEmpty() method is renamed to empty()\n> > and the methods reordered to match the std::vector class.\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 \n> > Reviewed-by: Niklas Söderlund \n> > ---\n> > include/libcamera/camera.h | 36 +--\n> > include/libcamera/stream.h | 7 +\n> > src/cam/main.cpp | 35 +--\n> > src/libcamera/camera.cpp | 268 +++++++++++------------\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, 251 insertions(+), 223 deletions(-)\n> >\n> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > index 42ba5201eabc..284e5276a055 100644\n> > --- a/include/libcamera/camera.h\n> > +++ b/include/libcamera/camera.h\n> > @@ -25,30 +25,36 @@ class Request;\n> > class CameraConfiguration\n> > {\n> > public:\n> > -\tusing iterator = std::vector::iterator;\n> > -\tusing const_iterator = std::vector::const_iterator;\n> > +\tusing iterator = std::vector::iterator;\n> > +\tusing const_iterator = std::vector::const_iterator;\n> >\n> > \tCameraConfiguration();\n> >\n> > +\tvoid addConfiguration(const StreamConfiguration &cfg);\n> \n> I would use push_back() to match the std::vector API\n\nI've thought about it, but the goal here isn't to mimick the vector API\ncompletely. For an interation point of view I think it makes sense to\noperate as a vector, but when it comes to building the configuration I\nthink we need explicit calls (same for validation). addConfiguration()\nmay not be the best name either, maybe addStreamConfiguration() ?\nThere's also a high chance we'll refactor this some more.\n\n> > +\n> > +\tbool isValid() const;\n> \n> As you s/isEmpty()/empty()/ should we s/isValid()/valid()/ ?\n\nDon't make me go back to isEmpty() ;-)\n\n> Ah, don't worry, this will become validate(), just noticed\n> \n> > +\n> > +\tStreamConfiguration &at(unsigned int index);\n> > +\tconst StreamConfiguration &at(unsigned int index) const;\n> > +\tStreamConfiguration &operator[](unsigned int index)\n> > +\t{\n> > +\t\treturn at(index);\n> > +\t}\n> > +\tconst StreamConfiguration &operator[](unsigned int index) const\n> > +\t{\n> > +\t\treturn at(index);\n> > +\t}\n> \n> Ok, so we have [] and at() like std::vector, but it seems to me their\n> behaviour is inverted.\n> \n> std::vector::at() performs bound checking, and\n> CameraConfiguration::at() is implemented with std::vector::operator[],\n> which does not perform bounds checking\n> \n> std::vector::operator[] does not perform bounds checking, but\n> CameraConfiguration::operator[] is implemented with std::vector::at()\n> which performs bound checking.\n\nNo, CameraConfiguration::operator[] is implemented with\nCameraConfiguration::at(), which, as you noted above, doesn't perform\nbound checking as it's implemented with std::vector::operator[].\n\n> \n> Is this intentional ?\n\nAs we don't support exceptions, it makes no real difference, and as a\nresult I went for the operator that won't throw an exception.\n\n> I would also question the need to have both operator[] and at()\n> accessors, I know std::vector does, but do we need that or is it just\n> an API expansion we could save?\n\nIt was requested by Niklas to replace (*config)[] with config->at() in\nthe callers. I have no personal problem with (*config)[], and if that's\ndesired, I'm fine dropping at().\n\n> > +\n> > \titerator begin();\n> > -\titerator end();\n> > \tconst_iterator begin() const;\n> > +\titerator end();\n> > \tconst_iterator end() const;\n> >\n> > -\tbool isValid() const;\n> > -\tbool isEmpty() const;\n> > +\tbool empty() 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> > -\n> > private:\n> > -\tstd::vector order_;\n> > -\tstd::map config_;\n> > +\tstd::vector config_;\n> > };\n> >\n> > class Camera final\n> > @@ -72,7 +78,7 @@ public:\n> >\n> > \tconst std::set &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> Should we protect access to unitialized streams_ ?\n> If StreamConfiguration is not inialized to {} what's the value of\n> *stream_ ?\n\nIt's indeed not initialised, but applications are not allowed to use it\nbefore the stream has been set by Camera::configure(). I can add a\ndefault constructor, but I really hope to remove Stream from the public\nAPI. I suppose it then won't hurt if I add the constructor just to be\nsafe in the meantime.\n\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> \n> nit and not related to this patch: s/conf/opt ? otherwise we have\n> *config and cfg in this loop, which is quite confusing.\n\nI agree. I'll add a patch.\n\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 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..5848330f639a 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -46,72 +46,40 @@ 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> As operator[] is implemented with std::vector::at() accessing with an\n> invalid index, an exception is thrown (even if we don't use them in\n> the library).\n\nIt's actually implemented with std::evctor::operator[] as explained\nabove :-)\n\n> I would document CameraConfiguration::at() as well, or just provide one of\n> the two only.\n\nThe at() functions are documented ;-)\n\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::iterator CameraConfiguration::begin()\n> > +void CameraConfiguration::addConfiguration(const StreamConfiguration &cfg)\n> > {\n> > -\treturn order_.begin();\n> > -}\n> > -\n> > -/**\n> > - * \\brief Retrieve an iterator pointing to the past-the-end stream in the\n> > - * sequence\n> > - * \\return An iterator to the element following the last stream\n> > - */\n> > -std::vector::iterator CameraConfiguration::end()\n> > -{\n> > -\treturn order_.end();\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> > - */\n> > -std::vector::const_iterator CameraConfiguration::begin() const\n> > -{\n> > -\treturn order_.begin();\n> > -}\n> > -\n> > -/**\n> > - * \\brief Retrieve a const iterator pointing to the past-the-end stream in the\n> > - * sequence\n> > - * \\return A const iterator to the element following the last stream\n> > - */\n> > -std::vector::const_iterator CameraConfiguration::end() const\n> > -{\n> > -\treturn order_.end();\n> > +\tconfig_.push_back(cfg);\n> > }\n> >\n> > /**\n> > @@ -125,12 +93,10 @@ std::vector::const_iterator CameraConfiguration::end() const\n> > */\n> > bool CameraConfiguration::isValid() const\n> > {\n> > -\tif (isEmpty())\n> > +\tif (empty())\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> > @@ -139,13 +105,108 @@ bool CameraConfiguration::isValid() const\n> > \treturn true;\n> > }\n> >\n> > +/**\n> > + * \\brief Retrieve a reference to a stream configuration\n> > + * \\param[in] index Numerical index\n> \n> STL uses pos in place of index. Not sure we care, though.\n\nI had considered this too :-) In the end I went for index to be\nconsistent with what we usually use (and I think it's also more explicit\nthan pos in this particular case).\n\n> > + *\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 stream configuration\n> \n> > + */\n> > +StreamConfiguration &CameraConfiguration::at(unsigned int index)\n> > +{\n> > +\treturn config_[index];\n> > +}\n> > +\n> > +/**\n> > + * \\brief Retrieve a const reference to a stream configuration\n> > + * \\param[in] index Numerical index\n> > + *\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 stream configuration\n> > + */\n> > +const StreamConfiguration &CameraConfiguration::at(unsigned int index) const\n> > +{\n> > +\treturn config_[index];\n> > +}\n> > +\n> > +/**\n> > + * \\fn StreamConfiguration &CameraConfiguration::operator[](unsigned int)\n> > + * \\brief Retrieve a reference to a stream configuration\n> > + * \\param[in] index Numerical index\n> > + *\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 stream configuration\n> > + */\n> > +\n> > +/**\n> > + * \\fn const StreamConfiguration &CameraConfiguration::operator[](unsigned int) const\n> > + * \\brief Retrieve a const reference to a stream configuration\n> > + * \\param[in] index Numerical index\n> > + *\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 stream configuration\n> > + */\n> > +\n> > +/**\n> > + * \\brief Retrieve an iterator to the first stream configuration in the\n> > + * sequence\n> > + * \\return An iterator to the first stream configuration\n> > + */\n> > +CameraConfiguration::iterator CameraConfiguration::begin()\n> > +{\n> > +\treturn config_.begin();\n> > +}\n> > +\n> > +/**\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 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> > +CameraConfiguration::iterator CameraConfiguration::end()\n> \n> Aren't these StreamConfiguration::iterators ?\n\nThere's no StreamConfiguration::iterator. CameraConfiguration::iterator\nis the iterator that iterates over the contents of CameraConfiguration.\nThis is similar to std::vector::iterator iterating over the contents of\nthe vector, we don't use int::iterator to iterate over a\nstd::vector. The iterator is implemented by the container, not the\ncontained data.\n\n> > +{\n> > +\treturn config_.end();\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> > +CameraConfiguration::const_iterator CameraConfiguration::end() const\n> > +{\n> > +\treturn config_.end();\n> > +}\n> > +\n> > /**\n> > * \\brief Check if the camera configuration is empty\n> > * \\return True if the configuration is empty\n> > */\n> > -bool CameraConfiguration::isEmpty() const\n> > +bool CameraConfiguration::empty() const\n> > {\n> > -\treturn order_.empty();\n> > +\treturn config_.empty();\n> > }\n> >\n> > /**\n> > @@ -154,75 +215,7 @@ 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> > -}\n> > -\n> > -/**\n> > - * \\brief Retrieve a reference to a stream configuration\n> > - * \\param[in] stream Stream to retrieve configuration for\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> > - *\n> > - * \\return The configuration for the stream\n> > - */\n> > -StreamConfiguration &CameraConfiguration::operator[](Stream *stream)\n> > -{\n> > -\tif (config_.find(stream) == config_.end())\n> > -\t\torder_.push_back(stream);\n> > -\n> > -\treturn config_[stream];\n> > -}\n> > -\n> > -/**\n> > - * \\brief Retrieve a const reference to a stream configuration\n> > - * \\param[in] stream Stream to retrieve configuration for\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> > - *\n> > - * \\return The configuration for the stream\n> > - */\n> > -const StreamConfiguration &CameraConfiguration::operator[](Stream *stream) const\n> > -{\n> > -\treturn config_.at(stream);\n> > +\treturn config_.size();\n> > }\n> >\n> > /**\n> > @@ -561,13 +554,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 +582,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 +607,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> \n> Isn't this better printed after pipe->configure(), once we know all\n> streams configuration have a stream assigned ?\n\nIt shouldn't make a difference as the stream is not printed, and I think\nit's useful to print it before in case pipe->configure() fails, to help\ndebugging the issue.\n\n> Overall this is very good. The fact StreamConfiguration instances are\n> associated to Stream just after configure() might require clearly\n> preventing applications to try access it, but I think the\n> documentation is quite clear on that.\n\nI don't like that part too much, but I didn't go through great lengths\nto fix it as I plan to remove the Stream anyway.\n\n> The biggest part is discussing the CameraConfiguration API I guess,\n> which we might want to make more similar to std::vector.\n> \n> > \t}\n> >\n> > \tLOG(Camera, Info) << msg.str();\n> > @@ -634,8 +621,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 &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 &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(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(s);\n> > -\n> > -\t\tret = imgu->configureOutput(stream->device_, config[stream]);\n> > +\tfor (StreamConfiguration &cfg : config) {\n> > +\t\tIPU3Stream *stream = static_cast(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 4bd8c5101a96..ec6980b0943a 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 &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> > @@ -217,6 +215,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 &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 &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 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;","headers":{"Return-Path":"","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 E682F60C27\n\tfor ;\n\tTue, 21 May 2019 15:14:55 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4EC7252C;\n\tTue, 21 May 2019 15:14:53 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1558444493;\n\tbh=wnFKoZFOfiertbU1SV8OV75G/0+QPZ8KAKbvxV7eXgk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=eP3Kv0Q0ie9GbJmFFlVH4KmNTXDiFRZ1XiSAOSJ5OemHAn67bVgxlT1YfulwCX7d/\n\tH3Q6WQsGKyJeJho5vjKd59xo4KeXalEYfVqx+Wdl1H8fxJx1N4D6z7ATTSgNjgwIho\n\tRS9k+NZWH6bgKyANV4flz6CUHpcBM99wnVsbArwo=","Date":"Tue, 21 May 2019 16:14:36 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190521131436.GE5674@pendragon.ideasonboard.com>","References":"<20190519150047.12444-1-laurent.pinchart@ideasonboard.com>\n\t<20190519150047.12444-4-laurent.pinchart@ideasonboard.com>\n\t<20190521091941.xocjngsp3yobosie@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190521091941.xocjngsp3yobosie@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 3/6] libcamera: Refactor the camera\n\tconfiguration storage and API","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 21 May 2019 13:14:56 -0000"}}]