[{"id":1649,"web_url":"https://patchwork.libcamera.org/comment/1649/","msgid":"<20190521200042.rduaoapvpxthvvoj@uno.localdomain>","date":"2019-05-21T20:00:42","subject":"Re: [libcamera-devel] [PATCH v3 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 Tue, May 21, 2019 at 10:27:37PM +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\nI'm fine with having both (*config)[] and config->at() for the moment\n\nReviewed-by: Jacopo Mondi \nThanks\n j\n\n> Signed-off-by: Laurent Pinchart \n> Reviewed-by: Niklas Söderlund \n> ---\n> Changes since v2:\n>\n> - Add a constructor to StreamConfiguration to initialise the private\n> stream_ pointer to nullptr\n> ---\n> include/libcamera/camera.h | 36 +--\n> include/libcamera/stream.h | 12 +\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, 256 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> +\tbool isValid() const;\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> \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..e38c0e7e827d 100644\n> --- a/include/libcamera/stream.h\n> +++ b/include/libcamera/stream.h\n> @@ -16,14 +16,26 @@\n> namespace libcamera {\n>\n> class Camera;\n> +class Stream;\n>\n> struct StreamConfiguration {\n> +\tStreamConfiguration()\n> +\t\t: stream_(nullptr)\n> +\t{\n> +\t}\n> +\n> \tunsigned int pixelFormat;\n> \tSize size;\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 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>\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> + * 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> +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> +\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> \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 relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 18C7060C40\n\tfor ;\n\tTue, 21 May 2019 21:59:37 +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 relay11.mail.gandi.net (Postfix) with ESMTPSA id E751A100004;\n\tTue, 21 May 2019 19:59:35 +0000 (UTC)"],"Date":"Tue, 21 May 2019 22:00:42 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190521200042.rduaoapvpxthvvoj@uno.localdomain>","References":"<20190521192740.28112-1-laurent.pinchart@ideasonboard.com>\n\t<20190521192740.28112-4-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"76f7uzx2rg473jn4\"","Content-Disposition":"inline","In-Reply-To":"<20190521192740.28112-4-laurent.pinchart@ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v3 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 19:59:37 -0000"}}]