[{"id":1300,"web_url":"https://patchwork.libcamera.org/comment/1300/","msgid":"<20190406170100.GC4817@pendragon.ideasonboard.com>","date":"2019-04-06T17:01:00","subject":"Re: [libcamera-devel] [PATCH v3 7/8] libcamera: camera: Add\n\tCameraConfiguration","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Sat, Apr 06, 2019 at 01:58:41AM +0200, Niklas Söderlund wrote:\n> To properly support both multiple streams and stream usages the library\n> must provide a method to map the stream usages to the returned streams\n> configurations. Add a camera configuration object to handle this\n> mapping.\n> \n> Applications can iterate over the returned camera configuration to\n> retrieve the streams selected by the library in the same order as the\n> usages it provided to the library.\n> \n> Application can use the operator[] to retrieve the stream pointer and\n> the stream configuration. Using a numerical index retrieves the stream\n> pointer, the numerical indexes corresponds to the insertion order of\n> usages by the application, using the stream pointer retrieves the\n> stream's configuration.\n> \n> Signed-off-by: Niklas Söderlund \n> ---\n> include/libcamera/camera.h | 28 ++++++\n> src/libcamera/camera.cpp | 173 +++++++++++++++++++++++++++++++++++++\n> 2 files changed, 201 insertions(+)\n> \n> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> index 0386671c902e55e8..8455049151d5c5a2 100644\n> --- a/include/libcamera/camera.h\n> +++ b/include/libcamera/camera.h\n> @@ -24,6 +24,34 @@ class Stream;\n> class StreamConfiguration;\n> class StreamUsage;\n> \n> +class CameraConfiguration\n> +{\n> +public:\n> +\tusing iterator = std::vector::iterator;\n> +\tusing const_iterator = std::vector::const_iterator;\n> +\n> +\tCameraConfiguration();\n> +\n> +\titerator begin();\n> +\titerator end();\n> +\tconst_iterator begin() const;\n> +\tconst_iterator end() const;\n> +\n> +\tbool valid() const;\n> +\tbool empty() const;\n> +\tstd::size_t size() const;\n> +\n> +\tStream *front();\n\nFor completeness, should you have a const Stream *front() const ?\n\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> +};\n> +\n> class Camera final\n> {\n> public:\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 63fde0ffc3d02d6c..98145edea1ac9c91 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -39,6 +39,179 @@ namespace libcamera {\n> \n> LOG_DECLARE_CATEGORY(Camera)\n> \n> +/**\n> + * \\class CameraConfiguration\n> + * \\brief Hold configuration for streams of the camera\n> + *\n> + * The CameraConfiguration is filled with information by the application either\n> + * manually or with the streamConfiguration() helper. The helper takes a list\n\ns/streamConfiguration()/Camera::streamConfigure()/ (to get doxygen to\ngenerate a link)\n\n> + * of usages describing how the application intends to use streams of the\n> + * camera, the application in return are provided with a default camera\n\ns/are provided/is provided/\n\n> + * configuration it can tune.\n\nMaybe s/tune/fine-tune/ ?\n\n> + *\n> + * Applications iterate over the CameraConfiguration to discover which streams\n> + * the camera has associated to the usages, and can inspect the configuration\n> + * of individual streams using the operator[].\n\nThis all explains how to use CameraConfiguration with the\nstreamConfiguration() helper, which I think belongs more to the helper's\ndocumentation. Here I would explain what the CameraConfiguration is and\nhow it is used. How about the following ?\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\nThe text you wrote should be reused to document Camera::streamConfiguration().\n\n> + */\n> +\n> +/**\n> + * \\typedef CameraConfiguration::iterator\n> + * \\brief Iterator for the streams in the configuration\n> + */\n> +\n> +/**\n> + * \\typedef CameraConfiguration::const_iterator\n> + * \\brief Const iterator for the streams in the configuration\n> + */\n> +\n> +/**\n> + * \\brief Create an empty camera configuration\n> + */\n> +CameraConfiguration::CameraConfiguration()\n> +\t: order_({}), config_({})\n> +{\n> +}\n> +\n> +/**\n> + * \\brief Retrieve an iterator to the first stream in the sequence\n> + *\n> + * \\return An iterator to the first stream\n> + */\n> +std::vector::iterator CameraConfiguration::begin()\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> + *\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 an iterator to the first element of the streams\n\nMaybe \"a const iterator\" ?\n\n> + *\n> + * \\return An iterator to the first stream\n\nAnd here too, and for the const version of end() as well.\n\n> + */\n> +std::vector::const_iterator CameraConfiguration::begin() const\n> +{\n> +\treturn order_.begin();\n> +}\n> +\n> +/**\n> + * \\brief Retrieve an iterator to the end of the streams\n\nThe non-const version mentions past-the-end.\n\n> + *\n> + * \\return An iterator to the element following the last stream\n> + */\n> +std::vector::const_iterator CameraConfiguration::end() const\n> +{\n> +\treturn order_.end();\n> +}\n> +\n> +/**\n> + * \\brief Check if the camera configuration is valid\n\nI think you should explain what an invalid configuration is.\n\n> + *\n> + * \\return True if the configuration is valid\n> + */\n> +bool CameraConfiguration::valid() const\n> +{\n> +\tif (empty())\n> +\t\treturn false;\n> +\n> +\tfor (auto const &it : config_) {\n> +\t\tconst StreamConfiguration &conf = it.second;\n> +\n> +\t\tif (conf.width == 0 || conf.height == 0 ||\n> +\t\t conf.pixelFormat == 0 || conf.bufferCount == 0)\n> +\t\t\treturn false;\n\nDo we have use cases for a non-empty CameraConfiguration with invalid\nStreamConfiguration ? Or is it just for completeness ?\n\n> +\t}\n> +\n> +\treturn true;\n> +}\n> +\n> +/**\n> + * \\brief Check if the camera configuration is empty\n> + *\n> + * \\return True if the configuration is empty\n> + */\n> +bool CameraConfiguration::empty() const\n> +{\n> +\treturn order_.empty();\n> +}\n> +\n> +/**\n> + * \\brief Retrieve the number of stream configurations\n> + *\n> + * \\return Number of stream configurations\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> + *\n> + * \\return The first stream in the configuration\n> + */\n> +Stream *CameraConfiguration::front()\n> +{\n> +\treturn order_.front();\n> +}\n> +/**\n> + * \\brief Retrieve a stream pointer from index\n> + * \\param[in] index Numerical index\n> + *\n> + * The \\a index represents the zero bases insertion order of stream and stream\n\ns/bases/based/\n\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 illegal operation.\n\ns/illegal/invalid/ ?\n\nI would also say \"and results in undefined behaviour\" as the std::map\nwill throw an exception.\n\n> + *\n> + * \\return The configuration for the stream\n> + */\n> +const StreamConfiguration &CameraConfiguration::operator[](Stream *stream) const\n> +{\n> +\treturn config_.at(stream);\n> +}\n> +\n> /**\n> * \\class Camera\n> * \\brief Camera device","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 596F360B1B\n\tfor ;\n\tSat, 6 Apr 2019 19:01:10 +0200 (CEST)","from pendragon.ideasonboard.com (unknown [91.183.39.81])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C514399F;\n\tSat, 6 Apr 2019 19:01:09 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1554570070;\n\tbh=AYd+W6dn4iTGxxo2b5NfpgL503mK1PXAgI7s/ehqamQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=XjtIvtQHoVCYrwY5bua0vbX5Egds7pLsTDrKC9K67IWm3FQZLdndkg35AiRm7av5K\n\tismzBSTXoYZtBOkQ50GLNypOpWMNdy9NMNdeuH/7pHBCQXd9Iddde52esk5mr5O1/a\n\tm6FTvRV1mjmO1Am8NYTb92JpWrLo/hH4SXlje1ZM=","Date":"Sat, 6 Apr 2019 20:01:00 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190406170100.GC4817@pendragon.ideasonboard.com>","References":"<20190405235842.27884-1-niklas.soderlund@ragnatech.se>\n\t<20190405235842.27884-8-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190405235842.27884-8-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v3 7/8] libcamera: camera: Add\n\tCameraConfiguration","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":"Sat, 06 Apr 2019 17:01:10 -0000"}},{"id":1302,"web_url":"https://patchwork.libcamera.org/comment/1302/","msgid":"<20190406171032.GE4817@pendragon.ideasonboard.com>","date":"2019-04-06T17:10:32","subject":"Re: [libcamera-devel] [PATCH v3 7/8] libcamera: camera: Add\n\tCameraConfiguration","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nAnother comment.\n\nOn Sat, Apr 06, 2019 at 08:01:00PM +0300, Laurent Pinchart wrote:\n> On Sat, Apr 06, 2019 at 01:58:41AM +0200, Niklas Söderlund wrote:\n> > To properly support both multiple streams and stream usages the library\n> > must provide a method to map the stream usages to the returned streams\n> > configurations. Add a camera configuration object to handle this\n> > mapping.\n> > \n> > Applications can iterate over the returned camera configuration to\n> > retrieve the streams selected by the library in the same order as the\n> > usages it provided to the library.\n> > \n> > Application can use the operator[] to retrieve the stream pointer and\n> > the stream configuration. Using a numerical index retrieves the stream\n> > pointer, the numerical indexes corresponds to the insertion order of\n> > usages by the application, using the stream pointer retrieves the\n> > stream's configuration.\n> > \n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > include/libcamera/camera.h | 28 ++++++\n> > src/libcamera/camera.cpp | 173 +++++++++++++++++++++++++++++++++++++\n> > 2 files changed, 201 insertions(+)\n> > \n> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > index 0386671c902e55e8..8455049151d5c5a2 100644\n> > --- a/include/libcamera/camera.h\n> > +++ b/include/libcamera/camera.h\n> > @@ -24,6 +24,34 @@ class Stream;\n> > class StreamConfiguration;\n> > class StreamUsage;\n> > \n> > +class CameraConfiguration\n> > +{\n> > +public:\n> > +\tusing iterator = std::vector::iterator;\n> > +\tusing const_iterator = std::vector::const_iterator;\n> > +\n> > +\tCameraConfiguration();\n> > +\n> > +\titerator begin();\n> > +\titerator end();\n> > +\tconst_iterator begin() const;\n> > +\tconst_iterator end() const;\n> > +\n> > +\tbool valid() const;\n> > +\tbool empty() const;\n\nvalid() and empty() vs. isValid() and isEmpty() ? I have a preference\nfor the latter due to my Qt bias, but I'm open to discuss this.\n\n> > +\tstd::size_t size() const;\n> > +\n> > +\tStream *front();\n> \n> For completeness, should you have a const Stream *front() const ?\n> \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> > +};\n> > +\n> > class Camera final\n> > {\n> > public:\n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index 63fde0ffc3d02d6c..98145edea1ac9c91 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -39,6 +39,179 @@ namespace libcamera {\n> > \n> > LOG_DECLARE_CATEGORY(Camera)\n> > \n> > +/**\n> > + * \\class CameraConfiguration\n> > + * \\brief Hold configuration for streams of the camera\n> > + *\n> > + * The CameraConfiguration is filled with information by the application either\n> > + * manually or with the streamConfiguration() helper. The helper takes a list\n> \n> s/streamConfiguration()/Camera::streamConfigure()/ (to get doxygen to\n> generate a link)\n> \n> > + * of usages describing how the application intends to use streams of the\n> > + * camera, the application in return are provided with a default camera\n> \n> s/are provided/is provided/\n> \n> > + * configuration it can tune.\n> \n> Maybe s/tune/fine-tune/ ?\n> \n> > + *\n> > + * Applications iterate over the CameraConfiguration to discover which streams\n> > + * the camera has associated to the usages, and can inspect the configuration\n> > + * of individual streams using the operator[].\n> \n> This all explains how to use CameraConfiguration with the\n> streamConfiguration() helper, which I think belongs more to the helper's\n> documentation. Here I would explain what the CameraConfiguration is and\n> how it is used. How about the following ?\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> \n> The text you wrote should be reused to document Camera::streamConfiguration().\n> \n> > + */\n> > +\n> > +/**\n> > + * \\typedef CameraConfiguration::iterator\n> > + * \\brief Iterator for the streams in the configuration\n> > + */\n> > +\n> > +/**\n> > + * \\typedef CameraConfiguration::const_iterator\n> > + * \\brief Const iterator for the streams in the configuration\n> > + */\n> > +\n> > +/**\n> > + * \\brief Create an empty camera configuration\n> > + */\n> > +CameraConfiguration::CameraConfiguration()\n> > +\t: order_({}), config_({})\n> > +{\n> > +}\n> > +\n> > +/**\n> > + * \\brief Retrieve an iterator to the first stream in the sequence\n> > + *\n> > + * \\return An iterator to the first stream\n> > + */\n> > +std::vector::iterator CameraConfiguration::begin()\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> > + *\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 an iterator to the first element of the streams\n> \n> Maybe \"a const iterator\" ?\n> \n> > + *\n> > + * \\return An iterator to the first stream\n> \n> And here too, and for the const version of end() as well.\n> \n> > + */\n> > +std::vector::const_iterator CameraConfiguration::begin() const\n> > +{\n> > +\treturn order_.begin();\n> > +}\n> > +\n> > +/**\n> > + * \\brief Retrieve an iterator to the end of the streams\n> \n> The non-const version mentions past-the-end.\n> \n> > + *\n> > + * \\return An iterator to the element following the last stream\n> > + */\n> > +std::vector::const_iterator CameraConfiguration::end() const\n> > +{\n> > +\treturn order_.end();\n> > +}\n> > +\n> > +/**\n> > + * \\brief Check if the camera configuration is valid\n> \n> I think you should explain what an invalid configuration is.\n> \n> > + *\n> > + * \\return True if the configuration is valid\n> > + */\n> > +bool CameraConfiguration::valid() const\n> > +{\n> > +\tif (empty())\n> > +\t\treturn false;\n> > +\n> > +\tfor (auto const &it : config_) {\n> > +\t\tconst StreamConfiguration &conf = it.second;\n> > +\n> > +\t\tif (conf.width == 0 || conf.height == 0 ||\n> > +\t\t conf.pixelFormat == 0 || conf.bufferCount == 0)\n> > +\t\t\treturn false;\n> \n> Do we have use cases for a non-empty CameraConfiguration with invalid\n> StreamConfiguration ? Or is it just for completeness ?\n> \n> > +\t}\n> > +\n> > +\treturn true;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Check if the camera configuration is empty\n> > + *\n> > + * \\return True if the configuration is empty\n> > + */\n> > +bool CameraConfiguration::empty() const\n> > +{\n> > +\treturn order_.empty();\n> > +}\n> > +\n> > +/**\n> > + * \\brief Retrieve the number of stream configurations\n> > + *\n> > + * \\return Number of stream configurations\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> > + *\n> > + * \\return The first stream in the configuration\n> > + */\n> > +Stream *CameraConfiguration::front()\n> > +{\n> > +\treturn order_.front();\n> > +}\n> > +/**\n> > + * \\brief Retrieve a stream pointer from index\n> > + * \\param[in] index Numerical index\n> > + *\n> > + * The \\a index represents the zero bases insertion order of stream and stream\n> \n> s/bases/based/\n> \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 illegal operation.\n> \n> s/illegal/invalid/ ?\n> \n> I would also say \"and results in undefined behaviour\" as the std::map\n> will throw an exception.\n> \n> > + *\n> > + * \\return The configuration for the stream\n> > + */\n> > +const StreamConfiguration &CameraConfiguration::operator[](Stream *stream) const\n> > +{\n> > +\treturn config_.at(stream);\n> > +}\n> > +\n> > /**\n> > * \\class Camera\n> > * \\brief Camera device","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1891760B1B\n\tfor ;\n\tSat, 6 Apr 2019 19:10:43 +0200 (CEST)","from pendragon.ideasonboard.com (unknown [91.183.39.81])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 98D0199F;\n\tSat, 6 Apr 2019 19:10:42 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1554570642;\n\tbh=W/rt15pjQTPshlrr3jGa3Nn0ZFz4DdbFSL/c0lAUozo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=VVZALa5dUnokKdaEpy4pWDuoQb1wgnYkcJUM1+pPrIMi66OGRVEUYFEIeNaYbRXzr\n\t9WMw+GFzU84VHtWxO92pBQ4cdMKFjJJHhPcpOlaxU/oRHxdKjpXaE+b/swC2sdvRkq\n\tC5RYP4nhQjM+5gsN6iqOeHu6FuOYFhT+b4aa32BY=","Date":"Sat, 6 Apr 2019 20:10:32 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190406171032.GE4817@pendragon.ideasonboard.com>","References":"<20190405235842.27884-1-niklas.soderlund@ragnatech.se>\n\t<20190405235842.27884-8-niklas.soderlund@ragnatech.se>\n\t<20190406170100.GC4817@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190406170100.GC4817@pendragon.ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v3 7/8] libcamera: camera: Add\n\tCameraConfiguration","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":"Sat, 06 Apr 2019 17:10:43 -0000"}},{"id":1309,"web_url":"https://patchwork.libcamera.org/comment/1309/","msgid":"<20190408113222.GG15350@bigcity.dyn.berto.se>","date":"2019-04-08T11:32:22","subject":"Re: [libcamera-devel] [PATCH v3 7/8] libcamera: camera: Add\n\tCameraConfiguration","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your feedback.\n\nOn 2019-04-06 20:01:00 +0300, Laurent Pinchart wrote:\n> Hi Niklas,\n> \n> Thank you for the patch.\n> \n> On Sat, Apr 06, 2019 at 01:58:41AM +0200, Niklas Söderlund wrote:\n> > To properly support both multiple streams and stream usages the library\n> > must provide a method to map the stream usages to the returned streams\n> > configurations. Add a camera configuration object to handle this\n> > mapping.\n> > \n> > Applications can iterate over the returned camera configuration to\n> > retrieve the streams selected by the library in the same order as the\n> > usages it provided to the library.\n> > \n> > Application can use the operator[] to retrieve the stream pointer and\n> > the stream configuration. Using a numerical index retrieves the stream\n> > pointer, the numerical indexes corresponds to the insertion order of\n> > usages by the application, using the stream pointer retrieves the\n> > stream's configuration.\n> > \n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > include/libcamera/camera.h | 28 ++++++\n> > src/libcamera/camera.cpp | 173 +++++++++++++++++++++++++++++++++++++\n> > 2 files changed, 201 insertions(+)\n> > \n> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > index 0386671c902e55e8..8455049151d5c5a2 100644\n> > --- a/include/libcamera/camera.h\n> > +++ b/include/libcamera/camera.h\n> > @@ -24,6 +24,34 @@ class Stream;\n> > class StreamConfiguration;\n> > class StreamUsage;\n> > \n> > +class CameraConfiguration\n> > +{\n> > +public:\n> > +\tusing iterator = std::vector::iterator;\n> > +\tusing const_iterator = std::vector::const_iterator;\n> > +\n> > +\tCameraConfiguration();\n> > +\n> > +\titerator begin();\n> > +\titerator end();\n> > +\tconst_iterator begin() const;\n> > +\tconst_iterator end() const;\n> > +\n> > +\tbool valid() const;\n> > +\tbool empty() const;\n> > +\tstd::size_t size() const;\n> > +\n> > +\tStream *front();\n> \n> For completeness, should you have a const Stream *front() const ?\n\nGood point, I will add this for v4.\n\n> \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> > +};\n> > +\n> > class Camera final\n> > {\n> > public:\n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index 63fde0ffc3d02d6c..98145edea1ac9c91 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -39,6 +39,179 @@ namespace libcamera {\n> > \n> > LOG_DECLARE_CATEGORY(Camera)\n> > \n> > +/**\n> > + * \\class CameraConfiguration\n> > + * \\brief Hold configuration for streams of the camera\n> > + *\n> > + * The CameraConfiguration is filled with information by the application either\n> > + * manually or with the streamConfiguration() helper. The helper takes a list\n> \n> s/streamConfiguration()/Camera::streamConfigure()/ (to get doxygen to\n> generate a link)\n> \n> > + * of usages describing how the application intends to use streams of the\n> > + * camera, the application in return are provided with a default camera\n> \n> s/are provided/is provided/\n> \n> > + * configuration it can tune.\n> \n> Maybe s/tune/fine-tune/ ?\n> \n> > + *\n> > + * Applications iterate over the CameraConfiguration to discover which streams\n> > + * the camera has associated to the usages, and can inspect the configuration\n> > + * of individual streams using the operator[].\n> \n> This all explains how to use CameraConfiguration with the\n> streamConfiguration() helper, which I think belongs more to the helper's\n> documentation. Here I would explain what the CameraConfiguration is and\n> how it is used. How about the following ?\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\nThanks this sounds much better!\n> \n> The text you wrote should be reused to document Camera::streamConfiguration().\n\nI will add this to my list of things to do. Once this series is merged I \nhope to find time to improve StreamConfiguration and will then try to \nlook over the full documentation of that code path.\n\n> \n> > + */\n> > +\n> > +/**\n> > + * \\typedef CameraConfiguration::iterator\n> > + * \\brief Iterator for the streams in the configuration\n> > + */\n> > +\n> > +/**\n> > + * \\typedef CameraConfiguration::const_iterator\n> > + * \\brief Const iterator for the streams in the configuration\n> > + */\n> > +\n> > +/**\n> > + * \\brief Create an empty camera configuration\n> > + */\n> > +CameraConfiguration::CameraConfiguration()\n> > +\t: order_({}), config_({})\n> > +{\n> > +}\n> > +\n> > +/**\n> > + * \\brief Retrieve an iterator to the first stream in the sequence\n> > + *\n> > + * \\return An iterator to the first stream\n> > + */\n> > +std::vector::iterator CameraConfiguration::begin()\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> > + *\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 an iterator to the first element of the streams\n> \n> Maybe \"a const iterator\" ?\n> \n> > + *\n> > + * \\return An iterator to the first stream\n> \n> And here too, and for the const version of end() as well.\n> \n> > + */\n> > +std::vector::const_iterator CameraConfiguration::begin() const\n> > +{\n> > +\treturn order_.begin();\n> > +}\n> > +\n> > +/**\n> > + * \\brief Retrieve an iterator to the end of the streams\n> \n> The non-const version mentions past-the-end.\n> \n> > + *\n> > + * \\return An iterator to the element following the last stream\n> > + */\n> > +std::vector::const_iterator CameraConfiguration::end() const\n> > +{\n> > +\treturn order_.end();\n> > +}\n> > +\n> > +/**\n> > + * \\brief Check if the camera configuration is valid\n> \n> I think you should explain what an invalid configuration is.\n\nWill do so for v4.\n\n> \n> > + *\n> > + * \\return True if the configuration is valid\n> > + */\n> > +bool CameraConfiguration::valid() const\n> > +{\n> > +\tif (empty())\n> > +\t\treturn false;\n> > +\n> > +\tfor (auto const &it : config_) {\n> > +\t\tconst StreamConfiguration &conf = it.second;\n> > +\n> > +\t\tif (conf.width == 0 || conf.height == 0 ||\n> > +\t\t conf.pixelFormat == 0 || conf.bufferCount == 0)\n> > +\t\t\treturn false;\n> \n> Do we have use cases for a non-empty CameraConfiguration with invalid\n> StreamConfiguration ? Or is it just for completeness ?\n\nAt the top of my head I can't think of a use-case for this. I do however \nthink we should turn StreamConfiguration into a class and move this \ncheck into a StreamConfiguration::valid(). I plan to do so ontop of this \nseries if there are no objections.\n\n> \n> > +\t}\n> > +\n> > +\treturn true;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Check if the camera configuration is empty\n> > + *\n> > + * \\return True if the configuration is empty\n> > + */\n> > +bool CameraConfiguration::empty() const\n> > +{\n> > +\treturn order_.empty();\n> > +}\n> > +\n> > +/**\n> > + * \\brief Retrieve the number of stream configurations\n> > + *\n> > + * \\return Number of stream configurations\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> > + *\n> > + * \\return The first stream in the configuration\n> > + */\n> > +Stream *CameraConfiguration::front()\n> > +{\n> > +\treturn order_.front();\n> > +}\n> > +/**\n> > + * \\brief Retrieve a stream pointer from index\n> > + * \\param[in] index Numerical index\n> > + *\n> > + * The \\a index represents the zero bases insertion order of stream and stream\n> \n> s/bases/based/\n> \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 illegal operation.\n> \n> s/illegal/invalid/ ?\n> \n> I would also say \"and results in undefined behaviour\" as the std::map\n> will throw an exception.\n\nThanks.\n\n> \n> > + *\n> > + * \\return The configuration for the stream\n> > + */\n> > +const StreamConfiguration &CameraConfiguration::operator[](Stream *stream) const\n> > +{\n> > +\treturn config_.at(stream);\n> > +}\n> > +\n> > /**\n> > * \\class Camera\n> > * \\brief Camera device\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from mail-lf1-x144.google.com (mail-lf1-x144.google.com\n\t[IPv6:2a00:1450:4864:20::144])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CC05960004\n\tfor ;\n\tMon, 8 Apr 2019 13:32:24 +0200 (CEST)","by mail-lf1-x144.google.com with SMTP id d18so9241899lfn.3\n\tfor ;\n\tMon, 08 Apr 2019 04:32:24 -0700 (PDT)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\ts67sm6148320lja.57.2019.04.08.04.32.22\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tMon, 08 Apr 2019 04:32:22 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=5O+uqr/tq+sNNfkc+S4L2caVCk1cqpW7o2BRdctnsQ0=;\n\tb=dKMBDxeLmE6W/SLt9VUOH87g1Wsy9sS+Rp/iS6tDm3k9wVO/8MU6M2EhqjGWZM4I4r\n\tWz0rebeAVhYgm+d+sLkR3f0Ebi0P7hYCSUNFthw9qOhY9kaVJIPYnrfnaSxXUyTkbhb2\n\t4OmIkAkI3ed8mtXVXtRCEnYYogDn7q20C8MwE67XGou1mFAnAXB6kKVmpBcuGeQiiJ1k\n\tOrWW7t/xixg7jHj1HZ/0LAaKRoMQHKv/bSUiqs4AkeOYw74p7+hfq1g/PIPJWl5ZG+ap\n\tvX/aSJnRF7wk0wda7im7x7K+OG48zCnNNrphNq8QTLPVkyBpktau/C2byIegjkX8D+JH\n\tsSXw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=5O+uqr/tq+sNNfkc+S4L2caVCk1cqpW7o2BRdctnsQ0=;\n\tb=ghAyOI266XKNn79AbY2uO5VgoMnS6ehOMKj41FXF5Tm/ZIThurT67OW8y/b/6cY0vb\n\ttz8Hxdc5e10hZQyHPzqPl9WTe2XsNZBbXGI+096iQy6gpH8nqFVy4DOmMUMZ2+P0YmdB\n\t9iTTma89WmB6FhPh5Rg8ckHUAhn5cSw+auA6wmOgZNWdQvLR+BeSg8QyTAhV26zL3FLD\n\tR0To4JPxVVwcK1Vi+XJ5kGeQTKD4OOwO0q8jVF74j5m6pIXHfSsCpIyKOsTu1nBulK9X\n\t/DPxALLh0LiXb5YfKsXdYbbCb9ldL85v8dFrc4LC8CX0Q7Kq7RrX7rNGnte5x755CaUX\n\tCN6A==","X-Gm-Message-State":"APjAAAUt+cVxFhO9i6bVvvSnkNlMdBHYay7g1NEVPixeVcGp+lZo1PFl\n\thsIhsAq2xRi6SWpiqEUn3pfIdQ==","X-Google-Smtp-Source":"APXvYqwmZ2z64ieP/7Eg5em+EFdL5KFlhreS8hYlfAQPgRAGEGUi1n1+uDrRyPrU6frd1XaHBsWr5w==","X-Received":"by 2002:ac2:4285:: with SMTP id\n\tm5mr15286832lfh.103.1554723143977; \n\tMon, 08 Apr 2019 04:32:23 -0700 (PDT)","Date":"Mon, 8 Apr 2019 13:32:22 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190408113222.GG15350@bigcity.dyn.berto.se>","References":"<20190405235842.27884-1-niklas.soderlund@ragnatech.se>\n\t<20190405235842.27884-8-niklas.soderlund@ragnatech.se>\n\t<20190406170100.GC4817@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190406170100.GC4817@pendragon.ideasonboard.com>","User-Agent":"Mutt/1.11.3 (2019-02-01)","Subject":"Re: [libcamera-devel] [PATCH v3 7/8] libcamera: camera: Add\n\tCameraConfiguration","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":"Mon, 08 Apr 2019 11:32:25 -0000"}},{"id":1310,"web_url":"https://patchwork.libcamera.org/comment/1310/","msgid":"<20190408113431.GH15350@bigcity.dyn.berto.se>","date":"2019-04-08T11:34:31","subject":"Re: [libcamera-devel] [PATCH v3 7/8] libcamera: camera: Add\n\tCameraConfiguration","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nOn 2019-04-06 20:10:32 +0300, Laurent Pinchart wrote:\n> Hi Niklas,\n> \n> Another comment.\n> \n> On Sat, Apr 06, 2019 at 08:01:00PM +0300, Laurent Pinchart wrote:\n> > On Sat, Apr 06, 2019 at 01:58:41AM +0200, Niklas Söderlund wrote:\n> > > To properly support both multiple streams and stream usages the library\n> > > must provide a method to map the stream usages to the returned streams\n> > > configurations. Add a camera configuration object to handle this\n> > > mapping.\n> > > \n> > > Applications can iterate over the returned camera configuration to\n> > > retrieve the streams selected by the library in the same order as the\n> > > usages it provided to the library.\n> > > \n> > > Application can use the operator[] to retrieve the stream pointer and\n> > > the stream configuration. Using a numerical index retrieves the stream\n> > > pointer, the numerical indexes corresponds to the insertion order of\n> > > usages by the application, using the stream pointer retrieves the\n> > > stream's configuration.\n> > > \n> > > Signed-off-by: Niklas Söderlund \n> > > ---\n> > > include/libcamera/camera.h | 28 ++++++\n> > > src/libcamera/camera.cpp | 173 +++++++++++++++++++++++++++++++++++++\n> > > 2 files changed, 201 insertions(+)\n> > > \n> > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > > index 0386671c902e55e8..8455049151d5c5a2 100644\n> > > --- a/include/libcamera/camera.h\n> > > +++ b/include/libcamera/camera.h\n> > > @@ -24,6 +24,34 @@ class Stream;\n> > > class StreamConfiguration;\n> > > class StreamUsage;\n> > > \n> > > +class CameraConfiguration\n> > > +{\n> > > +public:\n> > > +\tusing iterator = std::vector::iterator;\n> > > +\tusing const_iterator = std::vector::const_iterator;\n> > > +\n> > > +\tCameraConfiguration();\n> > > +\n> > > +\titerator begin();\n> > > +\titerator end();\n> > > +\tconst_iterator begin() const;\n> > > +\tconst_iterator end() const;\n> > > +\n> > > +\tbool valid() const;\n> > > +\tbool empty() const;\n> \n> valid() and empty() vs. isValid() and isEmpty() ? I have a preference\n> for the latter due to my Qt bias, but I'm open to discuss this.\n\nI have no strong opinion on valid() vs isValid() so will bend to popular \nopinion. For empty() I think we should keep it as is as this class to \nsome degree tries to mimic the std::{vector,map} interface where empty() \nis used.\n\n> \n> > > +\tstd::size_t size() const;\n> > > +\n> > > +\tStream *front();\n> > \n> > For completeness, should you have a const Stream *front() const ?\n> > \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> > > +};\n> > > +\n> > > class Camera final\n> > > {\n> > > public:\n> > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > index 63fde0ffc3d02d6c..98145edea1ac9c91 100644\n> > > --- a/src/libcamera/camera.cpp\n> > > +++ b/src/libcamera/camera.cpp\n> > > @@ -39,6 +39,179 @@ namespace libcamera {\n> > > \n> > > LOG_DECLARE_CATEGORY(Camera)\n> > > \n> > > +/**\n> > > + * \\class CameraConfiguration\n> > > + * \\brief Hold configuration for streams of the camera\n> > > + *\n> > > + * The CameraConfiguration is filled with information by the application either\n> > > + * manually or with the streamConfiguration() helper. The helper takes a list\n> > \n> > s/streamConfiguration()/Camera::streamConfigure()/ (to get doxygen to\n> > generate a link)\n> > \n> > > + * of usages describing how the application intends to use streams of the\n> > > + * camera, the application in return are provided with a default camera\n> > \n> > s/are provided/is provided/\n> > \n> > > + * configuration it can tune.\n> > \n> > Maybe s/tune/fine-tune/ ?\n> > \n> > > + *\n> > > + * Applications iterate over the CameraConfiguration to discover which streams\n> > > + * the camera has associated to the usages, and can inspect the configuration\n> > > + * of individual streams using the operator[].\n> > \n> > This all explains how to use CameraConfiguration with the\n> > streamConfiguration() helper, which I think belongs more to the helper's\n> > documentation. Here I would explain what the CameraConfiguration is and\n> > how it is used. How about the following ?\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> > \n> > The text you wrote should be reused to document Camera::streamConfiguration().\n> > \n> > > + */\n> > > +\n> > > +/**\n> > > + * \\typedef CameraConfiguration::iterator\n> > > + * \\brief Iterator for the streams in the configuration\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\typedef CameraConfiguration::const_iterator\n> > > + * \\brief Const iterator for the streams in the configuration\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\brief Create an empty camera configuration\n> > > + */\n> > > +CameraConfiguration::CameraConfiguration()\n> > > +\t: order_({}), config_({})\n> > > +{\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Retrieve an iterator to the first stream in the sequence\n> > > + *\n> > > + * \\return An iterator to the first stream\n> > > + */\n> > > +std::vector::iterator CameraConfiguration::begin()\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> > > + *\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 an iterator to the first element of the streams\n> > \n> > Maybe \"a const iterator\" ?\n> > \n> > > + *\n> > > + * \\return An iterator to the first stream\n> > \n> > And here too, and for the const version of end() as well.\n> > \n> > > + */\n> > > +std::vector::const_iterator CameraConfiguration::begin() const\n> > > +{\n> > > +\treturn order_.begin();\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Retrieve an iterator to the end of the streams\n> > \n> > The non-const version mentions past-the-end.\n> > \n> > > + *\n> > > + * \\return An iterator to the element following the last stream\n> > > + */\n> > > +std::vector::const_iterator CameraConfiguration::end() const\n> > > +{\n> > > +\treturn order_.end();\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Check if the camera configuration is valid\n> > \n> > I think you should explain what an invalid configuration is.\n> > \n> > > + *\n> > > + * \\return True if the configuration is valid\n> > > + */\n> > > +bool CameraConfiguration::valid() const\n> > > +{\n> > > +\tif (empty())\n> > > +\t\treturn false;\n> > > +\n> > > +\tfor (auto const &it : config_) {\n> > > +\t\tconst StreamConfiguration &conf = it.second;\n> > > +\n> > > +\t\tif (conf.width == 0 || conf.height == 0 ||\n> > > +\t\t conf.pixelFormat == 0 || conf.bufferCount == 0)\n> > > +\t\t\treturn false;\n> > \n> > Do we have use cases for a non-empty CameraConfiguration with invalid\n> > StreamConfiguration ? Or is it just for completeness ?\n> > \n> > > +\t}\n> > > +\n> > > +\treturn true;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Check if the camera configuration is empty\n> > > + *\n> > > + * \\return True if the configuration is empty\n> > > + */\n> > > +bool CameraConfiguration::empty() const\n> > > +{\n> > > +\treturn order_.empty();\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Retrieve the number of stream configurations\n> > > + *\n> > > + * \\return Number of stream configurations\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> > > + *\n> > > + * \\return The first stream in the configuration\n> > > + */\n> > > +Stream *CameraConfiguration::front()\n> > > +{\n> > > +\treturn order_.front();\n> > > +}\n> > > +/**\n> > > + * \\brief Retrieve a stream pointer from index\n> > > + * \\param[in] index Numerical index\n> > > + *\n> > > + * The \\a index represents the zero bases insertion order of stream and stream\n> > \n> > s/bases/based/\n> > \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 illegal operation.\n> > \n> > s/illegal/invalid/ ?\n> > \n> > I would also say \"and results in undefined behaviour\" as the std::map\n> > will throw an exception.\n> > \n> > > + *\n> > > + * \\return The configuration for the stream\n> > > + */\n> > > +const StreamConfiguration &CameraConfiguration::operator[](Stream *stream) const\n> > > +{\n> > > +\treturn config_.at(stream);\n> > > +}\n> > > +\n> > > /**\n> > > * \\class Camera\n> > > * \\brief Camera device\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from mail-lj1-x244.google.com (mail-lj1-x244.google.com\n\t[IPv6:2a00:1450:4864:20::244])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8A22060004\n\tfor ;\n\tMon, 8 Apr 2019 13:34:33 +0200 (CEST)","by mail-lj1-x244.google.com with SMTP id f18so10873382lja.10\n\tfor ;\n\tMon, 08 Apr 2019 04:34:33 -0700 (PDT)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tz8sm6312435ljc.50.2019.04.08.04.34.32\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tMon, 08 Apr 2019 04:34:32 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=PMXvRTOIaQv7EDgpRaB5ZGzkiLbH4waqkjtuaNRz2lc=;\n\tb=ew/GIhijAdxvIIi/1bnipbi3eqf2UQhu7mU1kIG/NtFfYT0JxMi0O82Ag80EwJazRF\n\tuT3WYBX2QsGB+Q1kWt0BgeI6ZSrrKYiPflzzWc4kDeVNimpzMQwSc1yaNuqYy/tNfNq1\n\tAz7mlqweltIuLT5/OwSt7zDPXLlkKzVjmqW2r1DSC+ZWv51FiSFMyRMkVu1FNaz+IkqQ\n\to5UbsJxg7AH5/XBpkw7X3ipL00U2Y/FvneCF292o6gWmUyWbC1R5wB9ELvsFHCy0OINj\n\tQL3MmWd9J0FFfK6BHIwyDR/eMy81o+TDK5VfZZ1LEYghEeRAooTLMG9EiTHklltJRbKT\n\tE6/w==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=PMXvRTOIaQv7EDgpRaB5ZGzkiLbH4waqkjtuaNRz2lc=;\n\tb=PuouM/vYizaKN9vuNyd9bGfP2XxgHgUXIPDda+YptuvSqfx6fqTl1NTPhpPqsov9in\n\tIt+cBq/LukMSUQF9l+gHYgkcrWISOXgfF/dTovY3tP1XmEWJfOMgvu+sqbsNDE9AAn7d\n\tLRRjmyuOdQe7HqbNYDwqIfS63lPn/aSJY0dj0aq+cCaKs3sfKaTF2yA3yZnvKqgZIu8M\n\ty4tJRMQJE5V5nShL5zWL0GTaffkTx8Uke/wMdh3a8pjhHSR5r6U86AQctKgBTV7jCPtg\n\tN0f6Cj2EVzu/VDxzsxTTop5T0ydt0+DR3/bLyWGjVNZY7UYy98mHP3fjm56thNj5uh2F\n\tJT4Q==","X-Gm-Message-State":"APjAAAVrJ7Jt4rytWpprmXsKaAJndD/jRb1HVQWmEtdS3taxk/QWlmgk\n\t6E3cP75N5xUEZL1YfNn3mnP1Hg==","X-Google-Smtp-Source":"APXvYqzAQLbMMKpo4X7Y1cReoPu79XpzlEoKtKUFeJSa4yv09pyPnaKf0GdXVPHiuYGsSjbzPumdFg==","X-Received":"by 2002:a2e:8192:: with SMTP id e18mr15468406ljg.6.1554723272898;\n\tMon, 08 Apr 2019 04:34:32 -0700 (PDT)","Date":"Mon, 8 Apr 2019 13:34:31 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190408113431.GH15350@bigcity.dyn.berto.se>","References":"<20190405235842.27884-1-niklas.soderlund@ragnatech.se>\n\t<20190405235842.27884-8-niklas.soderlund@ragnatech.se>\n\t<20190406170100.GC4817@pendragon.ideasonboard.com>\n\t<20190406171032.GE4817@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190406171032.GE4817@pendragon.ideasonboard.com>","User-Agent":"Mutt/1.11.3 (2019-02-01)","Subject":"Re: [libcamera-devel] [PATCH v3 7/8] libcamera: camera: Add\n\tCameraConfiguration","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":"Mon, 08 Apr 2019 11:34:33 -0000"}},{"id":1311,"web_url":"https://patchwork.libcamera.org/comment/1311/","msgid":"<20190408122325.GA4888@pendragon.ideasonboard.com>","date":"2019-04-08T12:23:25","subject":"Re: [libcamera-devel] [PATCH v3 7/8] libcamera: camera: Add\n\tCameraConfiguration","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nOn Mon, Apr 08, 2019 at 01:34:31PM +0200, Niklas Söderlund wrote:\n> On 2019-04-06 20:10:32 +0300, Laurent Pinchart wrote:\n> > On Sat, Apr 06, 2019 at 08:01:00PM +0300, Laurent Pinchart wrote:\n> >> On Sat, Apr 06, 2019 at 01:58:41AM +0200, Niklas Söderlund wrote:\n> >>> To properly support both multiple streams and stream usages the library\n> >>> must provide a method to map the stream usages to the returned streams\n> >>> configurations. Add a camera configuration object to handle this\n> >>> mapping.\n> >>> \n> >>> Applications can iterate over the returned camera configuration to\n> >>> retrieve the streams selected by the library in the same order as the\n> >>> usages it provided to the library.\n> >>> \n> >>> Application can use the operator[] to retrieve the stream pointer and\n> >>> the stream configuration. Using a numerical index retrieves the stream\n> >>> pointer, the numerical indexes corresponds to the insertion order of\n> >>> usages by the application, using the stream pointer retrieves the\n> >>> stream's configuration.\n> >>> \n> >>> Signed-off-by: Niklas Söderlund \n> >>> ---\n> >>> include/libcamera/camera.h | 28 ++++++\n> >>> src/libcamera/camera.cpp | 173 +++++++++++++++++++++++++++++++++++++\n> >>> 2 files changed, 201 insertions(+)\n> >>> \n> >>> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> >>> index 0386671c902e55e8..8455049151d5c5a2 100644\n> >>> --- a/include/libcamera/camera.h\n> >>> +++ b/include/libcamera/camera.h\n> >>> @@ -24,6 +24,34 @@ class Stream;\n> >>> class StreamConfiguration;\n> >>> class StreamUsage;\n> >>> \n> >>> +class CameraConfiguration\n> >>> +{\n> >>> +public:\n> >>> +\tusing iterator = std::vector::iterator;\n> >>> +\tusing const_iterator = std::vector::const_iterator;\n> >>> +\n> >>> +\tCameraConfiguration();\n> >>> +\n> >>> +\titerator begin();\n> >>> +\titerator end();\n> >>> +\tconst_iterator begin() const;\n> >>> +\tconst_iterator end() const;\n> >>> +\n> >>> +\tbool valid() const;\n> >>> +\tbool empty() const;\n> > \n> > valid() and empty() vs. isValid() and isEmpty() ? I have a preference\n> > for the latter due to my Qt bias, but I'm open to discuss this.\n> \n> I have no strong opinion on valid() vs isValid() so will bend to popular \n> opinion. For empty() I think we should keep it as is as this class to \n> some degree tries to mimic the std::{vector,map} interface where empty() \n> is used.\n\nI think we should remain consistent and use either empty(), valid() or\nisEmpty(), isValid(). STL doesn't use an \"is\" prefix, Qt does. We have a\nfew functions with the \"is\" prefix already, and that's my preference,\nbut as I aid I'm open to discuss this.\n\n> >>> +\tstd::size_t size() const;\n> >>> +\n> >>> +\tStream *front();\n> >> \n> >> For completeness, should you have a const Stream *front() const ?\n> >> \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> >>> +};\n> >>> +\n> >>> class Camera final\n> >>> {\n> >>> public:\n> >>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> >>> index 63fde0ffc3d02d6c..98145edea1ac9c91 100644\n> >>> --- a/src/libcamera/camera.cpp\n> >>> +++ b/src/libcamera/camera.cpp\n> >>> @@ -39,6 +39,179 @@ namespace libcamera {\n> >>> \n> >>> LOG_DECLARE_CATEGORY(Camera)\n> >>> \n> >>> +/**\n> >>> + * \\class CameraConfiguration\n> >>> + * \\brief Hold configuration for streams of the camera\n> >>> + *\n> >>> + * The CameraConfiguration is filled with information by the application either\n> >>> + * manually or with the streamConfiguration() helper. The helper takes a list\n> >> \n> >> s/streamConfiguration()/Camera::streamConfigure()/ (to get doxygen to\n> >> generate a link)\n> >> \n> >>> + * of usages describing how the application intends to use streams of the\n> >>> + * camera, the application in return are provided with a default camera\n> >> \n> >> s/are provided/is provided/\n> >> \n> >>> + * configuration it can tune.\n> >> \n> >> Maybe s/tune/fine-tune/ ?\n> >> \n> >>> + *\n> >>> + * Applications iterate over the CameraConfiguration to discover which streams\n> >>> + * the camera has associated to the usages, and can inspect the configuration\n> >>> + * of individual streams using the operator[].\n> >> \n> >> This all explains how to use CameraConfiguration with the\n> >> streamConfiguration() helper, which I think belongs more to the helper's\n> >> documentation. Here I would explain what the CameraConfiguration is and\n> >> how it is used. How about the following ?\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> >> \n> >> The text you wrote should be reused to document Camera::streamConfiguration().\n> >> \n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\typedef CameraConfiguration::iterator\n> >>> + * \\brief Iterator for the streams in the configuration\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\typedef CameraConfiguration::const_iterator\n> >>> + * \\brief Const iterator for the streams in the configuration\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\brief Create an empty camera configuration\n> >>> + */\n> >>> +CameraConfiguration::CameraConfiguration()\n> >>> +\t: order_({}), config_({})\n> >>> +{\n> >>> +}\n> >>> +\n> >>> +/**\n> >>> + * \\brief Retrieve an iterator to the first stream in the sequence\n> >>> + *\n> >>> + * \\return An iterator to the first stream\n> >>> + */\n> >>> +std::vector::iterator CameraConfiguration::begin()\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> >>> + *\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 an iterator to the first element of the streams\n> >> \n> >> Maybe \"a const iterator\" ?\n> >> \n> >>> + *\n> >>> + * \\return An iterator to the first stream\n> >> \n> >> And here too, and for the const version of end() as well.\n> >> \n> >>> + */\n> >>> +std::vector::const_iterator CameraConfiguration::begin() const\n> >>> +{\n> >>> +\treturn order_.begin();\n> >>> +}\n> >>> +\n> >>> +/**\n> >>> + * \\brief Retrieve an iterator to the end of the streams\n> >> \n> >> The non-const version mentions past-the-end.\n> >> \n> >>> + *\n> >>> + * \\return An iterator to the element following the last stream\n> >>> + */\n> >>> +std::vector::const_iterator CameraConfiguration::end() const\n> >>> +{\n> >>> +\treturn order_.end();\n> >>> +}\n> >>> +\n> >>> +/**\n> >>> + * \\brief Check if the camera configuration is valid\n> >> \n> >> I think you should explain what an invalid configuration is.\n> >> \n> >>> + *\n> >>> + * \\return True if the configuration is valid\n> >>> + */\n> >>> +bool CameraConfiguration::valid() const\n> >>> +{\n> >>> +\tif (empty())\n> >>> +\t\treturn false;\n> >>> +\n> >>> +\tfor (auto const &it : config_) {\n> >>> +\t\tconst StreamConfiguration &conf = it.second;\n> >>> +\n> >>> +\t\tif (conf.width == 0 || conf.height == 0 ||\n> >>> +\t\t conf.pixelFormat == 0 || conf.bufferCount == 0)\n> >>> +\t\t\treturn false;\n> >> \n> >> Do we have use cases for a non-empty CameraConfiguration with invalid\n> >> StreamConfiguration ? Or is it just for completeness ?\n> >> \n> >>> +\t}\n> >>> +\n> >>> +\treturn true;\n> >>> +}\n> >>> +\n> >>> +/**\n> >>> + * \\brief Check if the camera configuration is empty\n> >>> + *\n> >>> + * \\return True if the configuration is empty\n> >>> + */\n> >>> +bool CameraConfiguration::empty() const\n> >>> +{\n> >>> +\treturn order_.empty();\n> >>> +}\n> >>> +\n> >>> +/**\n> >>> + * \\brief Retrieve the number of stream configurations\n> >>> + *\n> >>> + * \\return Number of stream configurations\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> >>> + *\n> >>> + * \\return The first stream in the configuration\n> >>> + */\n> >>> +Stream *CameraConfiguration::front()\n> >>> +{\n> >>> +\treturn order_.front();\n> >>> +}\n> >>> +/**\n> >>> + * \\brief Retrieve a stream pointer from index\n> >>> + * \\param[in] index Numerical index\n> >>> + *\n> >>> + * The \\a index represents the zero bases insertion order of stream and stream\n> >> \n> >> s/bases/based/\n> >> \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 illegal operation.\n> >> \n> >> s/illegal/invalid/ ?\n> >> \n> >> I would also say \"and results in undefined behaviour\" as the std::map\n> >> will throw an exception.\n> >> \n> >>> + *\n> >>> + * \\return The configuration for the stream\n> >>> + */\n> >>> +const StreamConfiguration &CameraConfiguration::operator[](Stream *stream) const\n> >>> +{\n> >>> +\treturn config_.at(stream);\n> >>> +}\n> >>> +\n> >>> /**\n> >>> * \\class Camera\n> >>> * \\brief Camera device","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2F12E60004\n\tfor ;\n\tMon, 8 Apr 2019 14:23:38 +0200 (CEST)","from pendragon.ideasonboard.com (85-76-86-94-nat.elisa-mobile.fi\n\t[85.76.86.94])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 39B4F2C6;\n\tMon, 8 Apr 2019 14:23:37 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1554726217;\n\tbh=uscaJJLYIgnQS/Xadnm4Owabt9te0T1+bRmDxzD7V6A=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=AcqQv0JRZ/PyV57iqAZw/J8rJ8seVQTIIvc4ReRRp94RXAEGCw44+Y/T0CPSdQmQ0\n\ty5xIZkAO8Ee0kHW4BrXSGHlbPCZnQO7+Oef03sKlfvYKAcepBlgWYc4yeYn4nn3uFF\n\tvx6pr+Z3V8rdrgPub3btLd4GYghGbi6JLh5SW1/4=","Date":"Mon, 8 Apr 2019 15:23:25 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190408122325.GA4888@pendragon.ideasonboard.com>","References":"<20190405235842.27884-1-niklas.soderlund@ragnatech.se>\n\t<20190405235842.27884-8-niklas.soderlund@ragnatech.se>\n\t<20190406170100.GC4817@pendragon.ideasonboard.com>\n\t<20190406171032.GE4817@pendragon.ideasonboard.com>\n\t<20190408113431.GH15350@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190408113431.GH15350@bigcity.dyn.berto.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v3 7/8] libcamera: camera: Add\n\tCameraConfiguration","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":"Mon, 08 Apr 2019 12:23:38 -0000"}},{"id":1318,"web_url":"https://patchwork.libcamera.org/comment/1318/","msgid":"<20190408133546.GJ15350@bigcity.dyn.berto.se>","date":"2019-04-08T13:35:47","subject":"Re: [libcamera-devel] [PATCH v3 7/8] libcamera: camera: Add\n\tCameraConfiguration","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nOn 2019-04-08 15:23:25 +0300, Laurent Pinchart wrote:\n> Hi Niklas,\n> \n> On Mon, Apr 08, 2019 at 01:34:31PM +0200, Niklas Söderlund wrote:\n> > On 2019-04-06 20:10:32 +0300, Laurent Pinchart wrote:\n> > > On Sat, Apr 06, 2019 at 08:01:00PM +0300, Laurent Pinchart wrote:\n> > >> On Sat, Apr 06, 2019 at 01:58:41AM +0200, Niklas Söderlund wrote:\n> > >>> To properly support both multiple streams and stream usages the library\n> > >>> must provide a method to map the stream usages to the returned streams\n> > >>> configurations. Add a camera configuration object to handle this\n> > >>> mapping.\n> > >>> \n> > >>> Applications can iterate over the returned camera configuration to\n> > >>> retrieve the streams selected by the library in the same order as the\n> > >>> usages it provided to the library.\n> > >>> \n> > >>> Application can use the operator[] to retrieve the stream pointer and\n> > >>> the stream configuration. Using a numerical index retrieves the stream\n> > >>> pointer, the numerical indexes corresponds to the insertion order of\n> > >>> usages by the application, using the stream pointer retrieves the\n> > >>> stream's configuration.\n> > >>> \n> > >>> Signed-off-by: Niklas Söderlund \n> > >>> ---\n> > >>> include/libcamera/camera.h | 28 ++++++\n> > >>> src/libcamera/camera.cpp | 173 +++++++++++++++++++++++++++++++++++++\n> > >>> 2 files changed, 201 insertions(+)\n> > >>> \n> > >>> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > >>> index 0386671c902e55e8..8455049151d5c5a2 100644\n> > >>> --- a/include/libcamera/camera.h\n> > >>> +++ b/include/libcamera/camera.h\n> > >>> @@ -24,6 +24,34 @@ class Stream;\n> > >>> class StreamConfiguration;\n> > >>> class StreamUsage;\n> > >>> \n> > >>> +class CameraConfiguration\n> > >>> +{\n> > >>> +public:\n> > >>> +\tusing iterator = std::vector::iterator;\n> > >>> +\tusing const_iterator = std::vector::const_iterator;\n> > >>> +\n> > >>> +\tCameraConfiguration();\n> > >>> +\n> > >>> +\titerator begin();\n> > >>> +\titerator end();\n> > >>> +\tconst_iterator begin() const;\n> > >>> +\tconst_iterator end() const;\n> > >>> +\n> > >>> +\tbool valid() const;\n> > >>> +\tbool empty() const;\n> > > \n> > > valid() and empty() vs. isValid() and isEmpty() ? I have a preference\n> > > for the latter due to my Qt bias, but I'm open to discuss this.\n> > \n> > I have no strong opinion on valid() vs isValid() so will bend to popular \n> > opinion. For empty() I think we should keep it as is as this class to \n> > some degree tries to mimic the std::{vector,map} interface where empty() \n> > is used.\n> \n> I think we should remain consistent and use either empty(), valid() or\n> isEmpty(), isValid(). STL doesn't use an \"is\" prefix, Qt does. We have a\n> few functions with the \"is\" prefix already, and that's my preference,\n> but as I aid I'm open to discuss this.\n\nI agree consistency is key, I have added the is prefix to both \nfunctions.\n\n> \n> > >>> +\tstd::size_t size() const;\n> > >>> +\n> > >>> +\tStream *front();\n> > >> \n> > >> For completeness, should you have a const Stream *front() const ?\n> > >> \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> > >>> +};\n> > >>> +\n> > >>> class Camera final\n> > >>> {\n> > >>> public:\n> > >>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > >>> index 63fde0ffc3d02d6c..98145edea1ac9c91 100644\n> > >>> --- a/src/libcamera/camera.cpp\n> > >>> +++ b/src/libcamera/camera.cpp\n> > >>> @@ -39,6 +39,179 @@ namespace libcamera {\n> > >>> \n> > >>> LOG_DECLARE_CATEGORY(Camera)\n> > >>> \n> > >>> +/**\n> > >>> + * \\class CameraConfiguration\n> > >>> + * \\brief Hold configuration for streams of the camera\n> > >>> + *\n> > >>> + * The CameraConfiguration is filled with information by the application either\n> > >>> + * manually or with the streamConfiguration() helper. The helper takes a list\n> > >> \n> > >> s/streamConfiguration()/Camera::streamConfigure()/ (to get doxygen to\n> > >> generate a link)\n> > >> \n> > >>> + * of usages describing how the application intends to use streams of the\n> > >>> + * camera, the application in return are provided with a default camera\n> > >> \n> > >> s/are provided/is provided/\n> > >> \n> > >>> + * configuration it can tune.\n> > >> \n> > >> Maybe s/tune/fine-tune/ ?\n> > >> \n> > >>> + *\n> > >>> + * Applications iterate over the CameraConfiguration to discover which streams\n> > >>> + * the camera has associated to the usages, and can inspect the configuration\n> > >>> + * of individual streams using the operator[].\n> > >> \n> > >> This all explains how to use CameraConfiguration with the\n> > >> streamConfiguration() helper, which I think belongs more to the helper's\n> > >> documentation. Here I would explain what the CameraConfiguration is and\n> > >> how it is used. How about the following ?\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> > >> \n> > >> The text you wrote should be reused to document Camera::streamConfiguration().\n> > >> \n> > >>> + */\n> > >>> +\n> > >>> +/**\n> > >>> + * \\typedef CameraConfiguration::iterator\n> > >>> + * \\brief Iterator for the streams in the configuration\n> > >>> + */\n> > >>> +\n> > >>> +/**\n> > >>> + * \\typedef CameraConfiguration::const_iterator\n> > >>> + * \\brief Const iterator for the streams in the configuration\n> > >>> + */\n> > >>> +\n> > >>> +/**\n> > >>> + * \\brief Create an empty camera configuration\n> > >>> + */\n> > >>> +CameraConfiguration::CameraConfiguration()\n> > >>> +\t: order_({}), config_({})\n> > >>> +{\n> > >>> +}\n> > >>> +\n> > >>> +/**\n> > >>> + * \\brief Retrieve an iterator to the first stream in the sequence\n> > >>> + *\n> > >>> + * \\return An iterator to the first stream\n> > >>> + */\n> > >>> +std::vector::iterator CameraConfiguration::begin()\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> > >>> + *\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 an iterator to the first element of the streams\n> > >> \n> > >> Maybe \"a const iterator\" ?\n> > >> \n> > >>> + *\n> > >>> + * \\return An iterator to the first stream\n> > >> \n> > >> And here too, and for the const version of end() as well.\n> > >> \n> > >>> + */\n> > >>> +std::vector::const_iterator CameraConfiguration::begin() const\n> > >>> +{\n> > >>> +\treturn order_.begin();\n> > >>> +}\n> > >>> +\n> > >>> +/**\n> > >>> + * \\brief Retrieve an iterator to the end of the streams\n> > >> \n> > >> The non-const version mentions past-the-end.\n> > >> \n> > >>> + *\n> > >>> + * \\return An iterator to the element following the last stream\n> > >>> + */\n> > >>> +std::vector::const_iterator CameraConfiguration::end() const\n> > >>> +{\n> > >>> +\treturn order_.end();\n> > >>> +}\n> > >>> +\n> > >>> +/**\n> > >>> + * \\brief Check if the camera configuration is valid\n> > >> \n> > >> I think you should explain what an invalid configuration is.\n> > >> \n> > >>> + *\n> > >>> + * \\return True if the configuration is valid\n> > >>> + */\n> > >>> +bool CameraConfiguration::valid() const\n> > >>> +{\n> > >>> +\tif (empty())\n> > >>> +\t\treturn false;\n> > >>> +\n> > >>> +\tfor (auto const &it : config_) {\n> > >>> +\t\tconst StreamConfiguration &conf = it.second;\n> > >>> +\n> > >>> +\t\tif (conf.width == 0 || conf.height == 0 ||\n> > >>> +\t\t conf.pixelFormat == 0 || conf.bufferCount == 0)\n> > >>> +\t\t\treturn false;\n> > >> \n> > >> Do we have use cases for a non-empty CameraConfiguration with invalid\n> > >> StreamConfiguration ? Or is it just for completeness ?\n> > >> \n> > >>> +\t}\n> > >>> +\n> > >>> +\treturn true;\n> > >>> +}\n> > >>> +\n> > >>> +/**\n> > >>> + * \\brief Check if the camera configuration is empty\n> > >>> + *\n> > >>> + * \\return True if the configuration is empty\n> > >>> + */\n> > >>> +bool CameraConfiguration::empty() const\n> > >>> +{\n> > >>> +\treturn order_.empty();\n> > >>> +}\n> > >>> +\n> > >>> +/**\n> > >>> + * \\brief Retrieve the number of stream configurations\n> > >>> + *\n> > >>> + * \\return Number of stream configurations\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> > >>> + *\n> > >>> + * \\return The first stream in the configuration\n> > >>> + */\n> > >>> +Stream *CameraConfiguration::front()\n> > >>> +{\n> > >>> +\treturn order_.front();\n> > >>> +}\n> > >>> +/**\n> > >>> + * \\brief Retrieve a stream pointer from index\n> > >>> + * \\param[in] index Numerical index\n> > >>> + *\n> > >>> + * The \\a index represents the zero bases insertion order of stream and stream\n> > >> \n> > >> s/bases/based/\n> > >> \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 illegal operation.\n> > >> \n> > >> s/illegal/invalid/ ?\n> > >> \n> > >> I would also say \"and results in undefined behaviour\" as the std::map\n> > >> will throw an exception.\n> > >> \n> > >>> + *\n> > >>> + * \\return The configuration for the stream\n> > >>> + */\n> > >>> +const StreamConfiguration &CameraConfiguration::operator[](Stream *stream) const\n> > >>> +{\n> > >>> +\treturn config_.at(stream);\n> > >>> +}\n> > >>> +\n> > >>> /**\n> > >>> * \\class Camera\n> > >>> * \\brief Camera device\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from mail-lj1-x242.google.com (mail-lj1-x242.google.com\n\t[IPv6:2a00:1450:4864:20::242])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8673360004\n\tfor ;\n\tMon, 8 Apr 2019 15:35:49 +0200 (CEST)","by mail-lj1-x242.google.com with SMTP id p14so11270371ljg.5\n\tfor ;\n\tMon, 08 Apr 2019 06:35:49 -0700 (PDT)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\te18sm809547lfj.39.2019.04.08.06.35.47\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tMon, 08 Apr 2019 06:35:47 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=1SOLbm1lbLfy4bXdOqSUdiMHPhOgAOzDHFA74kywIKE=;\n\tb=WoHfOmx/y7V5LawvOze2TYa5ToCbaVHDpAZse3MVjzD0ubuJkWcOhQuaJIHc03g65d\n\tbzntg8GHt4CeIZT0PeDzBGaDFa3itmEBYaFcilkDBahuZSz2OiCVYz/CNSPowBrwfzmb\n\tX8w3b/ETBVHaJCE6eCm80dqQm7vZ3xTJItIaJ0pbmejp1XG7NK0xhjd3t3rtDn+fXmJv\n\tul5Is63VUOMVTxS82UiqvX3fYxjWWrhtoQZLpo/2bf7nzQ2GapRmQQ6wfgiWW+yt4Apq\n\thCx9PRb5Lol/B/nJhWYq/tcen1n1WyQkZk4o5m9shbn6xNmEF4yL+7hx5ZMlnPmz214y\n\t5pLg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=1SOLbm1lbLfy4bXdOqSUdiMHPhOgAOzDHFA74kywIKE=;\n\tb=Hc99I3k4v1gvfLZMsGEOoATzifbXpj+8mj1WMFe+vOtv7Qy1XPJjvJHuXiQBgyoi14\n\ti27aptoKm9lAe7A99jaOu2ULPRbI+5ZDjzNE7RIA88YW+qXhXzuKWFgRRRARv22lawpA\n\tFzh9W4ojoLQugTzywuuLsBkX9O6mQMFVu7j02Nuwb3Js/b66Nl22NAnDTm7gUsBRIDt0\n\tS7gCfmkgoEvJ3PkdKgrC/IC5rdzq+E/mHHhEs+VLDrvqN8fBSRtRf1ierChzVN9r4XxG\n\tOZL9Dg+vWwiSzmLs/1NrsK+uiRfhtnlkjh2UmGOcv7OOIt9Foe28CXJPVbN2JAxCZmgj\n\tIgMA==","X-Gm-Message-State":"APjAAAVed6T+PTDkxvMa92F0Xa+qDIF+8jWeQGcY5GL9wSvCOqTvC/m6\n\tV9YAM7p1iVuU4FJZlvRrDWwkDr4zYdw=","X-Google-Smtp-Source":"APXvYqyEZS6UWuXO/hXY9nrs5VVUScRJJZ2pKuPk4pVhWXT7fWkbEhKJfrUEiccTV6VXI6KxaC1AQg==","X-Received":"by 2002:a2e:85d2:: with SMTP id\n\th18mr16097054ljj.128.1554730548795; \n\tMon, 08 Apr 2019 06:35:48 -0700 (PDT)","Date":"Mon, 8 Apr 2019 15:35:47 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190408133546.GJ15350@bigcity.dyn.berto.se>","References":"<20190405235842.27884-1-niklas.soderlund@ragnatech.se>\n\t<20190405235842.27884-8-niklas.soderlund@ragnatech.se>\n\t<20190406170100.GC4817@pendragon.ideasonboard.com>\n\t<20190406171032.GE4817@pendragon.ideasonboard.com>\n\t<20190408113431.GH15350@bigcity.dyn.berto.se>\n\t<20190408122325.GA4888@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190408122325.GA4888@pendragon.ideasonboard.com>","User-Agent":"Mutt/1.11.3 (2019-02-01)","Subject":"Re: [libcamera-devel] [PATCH v3 7/8] libcamera: camera: Add\n\tCameraConfiguration","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":"Mon, 08 Apr 2019 13:35:49 -0000"}}]