[{"id":22601,"web_url":"https://patchwork.libcamera.org/comment/22601/","msgid":"","date":"2022-04-06T01:47:27","subject":"Re: [libcamera-devel] [PATCH 7/9] libcamera: ipu3: Add helper class\n\tPipeConfigPreference","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Han-Lin,\n\nThank you for the patch.\n\nOn Wed, Feb 09, 2022 at 03:19:15PM +0800, Han-Lin Chen wrote:\n> Add helper class PipeConfigPreference to load the caliberated ipu3\n> pipeline configuration files, and provides query interface for the\n> pipeline handler.\n> \n> Signed-off-by: Han-Lin Chen \n> ---\n> include/libcamera/geometry.h | 4 +\n> src/libcamera/geometry.cpp | 20 ++\n\nCould you please split the changes to geometry.h and geometry.cpp to a\nseparate patch ?\n\n> src/libcamera/pipeline/ipu3/meson.build | 1 +\n> .../pipeline/ipu3/pipe_config_pref.cpp | 285 ++++++++++++++++++\n> .../pipeline/ipu3/pipe_config_pref.h | 80 +++++\n> 5 files changed, 390 insertions(+)\n> create mode 100644 src/libcamera/pipeline/ipu3/pipe_config_pref.cpp\n> create mode 100644 src/libcamera/pipeline/ipu3/pipe_config_pref.h\n> \n> diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h\n> index 7838b679..ede54981 100644\n> --- a/include/libcamera/geometry.h\n> +++ b/include/libcamera/geometry.h\n> @@ -46,6 +46,8 @@ static inline bool operator!=(const Point &lhs, const Point &rhs)\n> \treturn !(lhs == rhs);\n> }\n> \n> +std::ostream &operator<<(std::ostream &out, const Point &d);\n> +\n> class Size\n> {\n> public:\n> @@ -192,6 +194,8 @@ static inline bool operator>=(const Size &lhs, const Size &rhs)\n> \treturn !(lhs < rhs);\n> }\n> \n> +std::ostream &operator<<(std::ostream &out, const Size &s);\n> +\n> class SizeRange\n> {\n> public:\n> diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp\n> index cb3c2de1..a65f9f2f 100644\n> --- a/src/libcamera/geometry.cpp\n> +++ b/src/libcamera/geometry.cpp\n> @@ -83,6 +83,16 @@ bool operator==(const Point &lhs, const Point &rhs)\n> * \\return True if the two points are not equal, false otherwise\n> */\n> \n> +/**\n> + * \\brief Insert operation for Point with ostream\n> + * \\return The input std::ostream\n> + */\n> +std::ostream &operator<<(std::ostream &out, const Point &p)\n> +{\n> +\tout << \"(\" << p.x << \", \" << p.y << \")\";\n\nThis doesn't match the format used by Point::toString(), which can be\nconfusing.\n\nI'm actually wondering if we could use toString() instead of adding\noperator<<() overloads. I half-recall having the same discussion a long\ntime ago and advocating against operator<<(), but I can't recall why. Or\nmaybe I don't remember correctly, and operator<<() is fine :-) Does\nanyone have an opinion on this ?\n\n> +\treturn out;\n> +}\n> +\n> /**\n> * \\struct Size\n> * \\brief Describe a two-dimensional size\n> @@ -428,6 +438,16 @@ bool operator<(const Size &lhs, const Size &rhs)\n> * \\sa bool operator<(const Size &lhs, const Size &rhs)\n> */\n> \n> +/**\n> + * \\brief Insert operation for Size with ostream\n> + * \\return The input std::ostream\n> + */\n> +std::ostream &operator<<(std::ostream &out, const Size &s)\n> +{\n> +\tout << s.width << \"x\" << s.height;\n> +\treturn out;\n> +}\n> +\n\nFor completeness, can you add the operators for SizeRange and Rectangle\ntoo ?\n\nI'll review the IPU3 part separately, possibly after updating it to the\nnext version of the YAML parser class as it will result in some changes.\n\n> /**\n> * \\struct SizeRange\n> * \\brief Describe a range of sizes\n> diff --git a/src/libcamera/pipeline/ipu3/meson.build b/src/libcamera/pipeline/ipu3/meson.build\n> index a1b0b31a..dcc450f0 100644\n> --- a/src/libcamera/pipeline/ipu3/meson.build\n> +++ b/src/libcamera/pipeline/ipu3/meson.build\n> @@ -5,4 +5,5 @@ libcamera_sources += files([\n> 'frames.cpp',\n> 'imgu.cpp',\n> 'ipu3.cpp',\n> + 'pipe_config_pref.cpp',\n> ])\n> diff --git a/src/libcamera/pipeline/ipu3/pipe_config_pref.cpp b/src/libcamera/pipeline/ipu3/pipe_config_pref.cpp\n> new file mode 100644\n> index 00000000..5b4a17c9\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/ipu3/pipe_config_pref.cpp\n> @@ -0,0 +1,285 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2022, Google Inc.\n> + *\n> + * pipe_config_pref.cpp - Helper class for IPU3 pipeline config preference\n> + */\n> +\n> +#include \"pipe_config_pref.h\"\n> +\n> +#include \n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +LOG_DECLARE_CATEGORY(IPU3)\n> +\n> +namespace {\n> +\n> +std::ostream &operator<<(std::ostream &out,\n> +\t\t\t const PipeConfigPreference::PipeConfig &d)\n> +{\n> +\tout << \"cio2: \" << d.cio2 << \" bds: \" << d.bds\n> +\t << \" gdc: \" << d.gdc << \" iff: \" << d.iff\n> +\t << \" main: \" << d.main << \" viewfinder: \" << d.viewfinder;\n> +\treturn out;\n> +}\n> +\n> +int loadPipeConfig(const YamlObject &yamlObject,\n> +\t\t PipeConfigPreference::PipeConfig &pipeConfig)\n> +{\n> +\tif (!yamlObject.isMember(\"bds\") ||\n> +\t !yamlObject.isMember(\"gdc\") ||\n> +\t !yamlObject.isMember(\"iff\") ||\n> +\t !yamlObject.isMember(\"cio2\") ||\n> +\t !yamlObject.isMember(\"main\") ||\n> +\t !yamlObject.isMember(\"viewfinder\")) {\n> +\t\tLOG(IPU3, Error) << \"Missing mandatory attributes in a config\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tpipeConfig.bds = yamlObject.get(\"bds\").asSize();\n> +\tpipeConfig.gdc = yamlObject.get(\"gdc\").asSize();\n> +\tpipeConfig.iff = yamlObject.get(\"iff\").asSize();\n> +\tpipeConfig.cio2 = yamlObject.get(\"cio2\").asSize();\n> +\tpipeConfig.main = yamlObject.get(\"main\").asSize();\n> +\tpipeConfig.viewfinder = yamlObject.get(\"viewfinder\").asSize();\n> +\n> +\treturn 0;\n> +}\n> +\n> +int loadPipeConfigs(const YamlObject &yamlPipeConfigs,\n> +\t\t std::vector &pipeConfigs,\n> +\t\t Size &maxResolution, Size &minResolution)\n> +{\n> +\tfor (int i = 0; i < yamlPipeConfigs.length(); i++) {\n> +\t\tconst YamlObject &yamlConfig = yamlPipeConfigs[i];\n> +\t\tpipeConfigs.emplace_back();\n> +\t\tif (loadPipeConfig(yamlConfig, pipeConfigs.back()))\n> +\t\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tif (pipeConfigs.size() == 0)\n> +\t\treturn -EINVAL;\n> +\n> +\tmaxResolution = minResolution = pipeConfigs[0].main;\n> +\n> +\tfor (const PipeConfigPreference::PipeConfig &config : pipeConfigs) {\n> +\t\tmaxResolution.expandTo(config.main);\n> +\t\tminResolution.boundTo(config.main);\n> +\t}\n> +\n> +\t/* Sort configs by the size of the cio2 */\n> +\tsort(pipeConfigs.begin(), pipeConfigs.end(),\n> +\t [](const auto &a, const auto &b) -> bool {\n> +\t\t return a.cio2 < b.cio2;\n> +\t });\n> +\n> +\treturn 0;\n> +}\n> +\n> +} /* namespace */\n> +\n> +PipeConfigPreference::PipeConfigPreference()\n> +\t: valid_(false)\n> +{\n> +}\n> +\n> +/**\n> + * \\struct PipeConfig\n> + * \\brief Describe a valid ImgU configuration\n> + *\n> + * The ImgU unit processes images through several components, which have\n> + * to be properly configured inspecting the input image size and the desired\n> + * output sizes. This structure collects the ImgU configuration for IF, BDS\n> + * and GDC, and the requested main output, viewfinder and the input (CIO2)\n> + * resolutions.\n> + */\n> +\n> +/**\n> + * \\brief Parse the configuration file from a path\n> + * \\param[in] path The path to the configuration file\n> + *\n> + * Parse the configuration file from a path and set isValid() to true if\n> + * success.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int PipeConfigPreference::parsePreferenceFile(const std::string &path)\n> +{\n> +\tvalid_ = false;\n> +\n> +\tFILE *fh = fopen(path.c_str(), \"r\");\n> +\tif (!fh) {\n> +\t\tLOG(IPU3, Error) << \"Fail to open file: \" << path;\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tYamlParser yamlParser;\n> +\tYamlObject yamlObjectPreference;\n> +\n> +\tint ret = yamlParser.ParseAsYamlObject(fh, yamlObjectPreference);\n> +\tfclose(fh);\n> +\n> +\tif (ret)\n> +\t\treturn -EINVAL;\n> +\n> +\tret = load(yamlObjectPreference);\n> +\n> +\tif (ret)\n> +\t\treturn -EINVAL;\n> +\n> +\tvalid_ = true;\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\brief Query the valid pipeline configuration for video and still pipe\n> + * \\param[in] videoMain The size of main output from video pipe\n> + * \\param[in] videoViewfinder The size of viewfinder output from video pipe\n> + * \\param[in] stillMain The size of main output from still pipe\n> + * \\param[in] stillViewfinder The size of viewfinder output from still pipe\n> + * \\param[out] videoResult The ImgU setting for video pipe\n> + * \\param[out] stillResult The ImgU setting for still pipe\n> + *\n> + * Helper function to query valid settings for ImgU with the desired\n> + * output resolutions. The query interface is based on the assumption\n> + * that both video and still ImgU might be used together.\n> + * An output can be set disabled if not required. If both main and viewfinder\n> + * are set disabled for a ImgU, video or still, the corresponding pipeConfig\n> + * undefined. For example, a typical usage is to only one video output is\n> + * required, the user may set:\n> + *\n> + * videoMain = [width, height]\n> + * videoViewfinder = Disabled\n> + * stillMain = Disabled\n> + * stillViewfinder = Disabled\n> + *\n> + * In the case, only the videoResult would be valid, since still pipe is not\n> + * used. When both video and still ImgU are used, their cio2 will have the\n> + * same resolution, since they should use the same raw capture.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int PipeConfigPreference::queryPipeConfig(\n> +\tconst Size &videoMain, const Size &videoViewfinder,\n> +\tconst Size &stillMain, const Size &stillViewfinder,\n> +\tPipeConfig &videoResult, PipeConfig &stillResult) const\n> +{\n> +\tbool hasVideo = (videoMain != Disabled) && (videoMain >= videoViewfinder);\n> +\tbool hasStill = (stillMain != Disabled) && (stillMain >= stillViewfinder);\n> +\n> +\tif (!hasStill && !hasVideo)\n> +\t\treturn -EINVAL;\n> +\n> +\tstd::vector validVideoConfigs;\n> +\tstd::vector validStillConfigs;\n> +\n> +\tfor (auto &config : videoPipeConfigs_) {\n> +\t\tif (config.main == videoMain && config.viewfinder == videoViewfinder)\n> +\t\t\tvalidVideoConfigs.emplace_back(config);\n> +\t}\n> +\n> +\tfor (auto &config : stillPipeConfigs_) {\n> +\t\tif (config.main == stillMain && config.viewfinder == stillViewfinder)\n> +\t\t\tvalidStillConfigs.emplace_back(config);\n> +\t}\n> +\n> +\t/*\n> +\t * Since the configurations are sorted by the size of CIO2, pick\n> +\t * the first valid resolution for lower bandwith.\n> +\t */\n> +\tif (hasVideo && !hasStill) {\n> +\t\tif (validVideoConfigs.empty())\n> +\t\t\treturn -EINVAL;\n> +\t\tvideoResult = validVideoConfigs[0];\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tif (hasStill && !hasVideo) {\n> +\t\tif (validStillConfigs.empty())\n> +\t\t\treturn -EINVAL;\n> +\t\tstillResult = validStillConfigs[0];\n> +\t\treturn 0;\n> +\t}\n> +\n> +\t/* (hasVideo && hasStill) */\n> +\tbool found = false;\n> +\tfor (const PipeConfig &videoConfig : validVideoConfigs) {\n> +\t\tfor (const PipeConfig &stillConfig : validVideoConfigs) {\n> +\t\t\tif (videoConfig.cio2 == stillConfig.cio2) {\n> +\t\t\t\tfound = true;\n> +\t\t\t\tvideoResult = videoConfig;\n> +\t\t\t\tstillResult = stillConfig;\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t}\n> +\t}\n> +\n> +\treturn (found) ? 0 : -EINVAL;\n> +}\n> +\n> +void PipeConfigPreference::dump()\n> +{\n> +\tLOG(IPU3, Debug) << \"Video Pipe configs: \";\n> +\tfor (auto &configs : videoPipeConfigs_) {\n> +\t\tLOG(IPU3, Debug) << configs;\n> +\t}\n> +\n> +\tLOG(IPU3, Debug) << \"Still Pipe configs: \";\n> +\tfor (auto &configs : stillPipeConfigs_) {\n> +\t\tLOG(IPU3, Debug) << configs;\n> +\t}\n> +}\n> +\n> +int PipeConfigPreference::load(const YamlObject &configs)\n> +{\n> +\t/*\n> +\t * Load the pipeline configure file properties.\n> +\t *\n> +\t * Each valid configuration is a list of properties associated\n> +\t * with the corresponding IMGU settings and grouped into still\n> +\t * and video modes. For each configuration, the main output should\n> +\t * be valid, and the viewfinder is optional. If the viewfinder is\n> +\t * disabled, its width and height should be set to [0, 0];\n> +\t *\n> +\t * still_mode:\n> +\t * - bds: [width, height]\n> +\t * cio2: [width, height]\n> +\t * gdc: [width, height]\n> +\t * iff: [width, height]\n> +\t * main: [width, height]\n> +\t * viewfinder: [0, 0]\n> +\t * ...\n> +\t *\n> +\t * video_mode:\n> +\t * - bds: [width, height]\n> +\t * cio2: [width, height]\n> +\t * gdc: [width, height]\n> +\t * iff: [width, height]\n> +\t * main: [width, height]\n> +\t * viewfinder: [0, 0]\n> +\t * ...\n> +\t */\n> +\n> +\tif (!configs.isMember(\"video_mode\") || !configs.isMember(\"still_mode\"))\n> +\t\treturn -EINVAL;\n> +\n> +\tvideoPipeConfigs_.clear();\n> +\tstillPipeConfigs_.clear();\n> +\n> +\tint ret = loadPipeConfigs(configs.get(\"video_mode\"), videoPipeConfigs_,\n> +\t\t\t\t maxVideoResolution_, minVideoResolution_);\n> +\tif (ret)\n> +\t\treturn -EINVAL;\n> +\n> +\tret = loadPipeConfigs(configs.get(\"still_mode\"), stillPipeConfigs_,\n> +\t\t\t maxStillResolution_, minStillResolution_);\n> +\tif (ret)\n> +\t\treturn -EINVAL;\n> +\n> +\treturn 0;\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/ipu3/pipe_config_pref.h b/src/libcamera/pipeline/ipu3/pipe_config_pref.h\n> new file mode 100644\n> index 00000000..08626d4e\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/ipu3/pipe_config_pref.h\n> @@ -0,0 +1,80 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2022, Google Inc.\n> + *\n> + * pipe_config_pref.h - Helper class for IPU3 pipeline config preference\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +class YamlObject;\n> +\n> +class PipeConfigPreference final\n> +{\n> +public:\n> +\tconstexpr static Size Disabled = Size(0, 0);\n> +\n> +\tstruct PipeConfig {\n> +\t\tSize cio2;\n> +\t\tSize bds;\n> +\t\tSize gdc;\n> +\t\tSize iff;\n> +\t\tSize main;\n> +\t\tSize viewfinder;\n> +\t};\n> +\n> +\tPipeConfigPreference();\n> +\n> +\tint parsePreferenceFile(const std::string &path);\n> +\tbool isValid() const\n> +\t{\n> +\t\treturn valid_;\n> +\t}\n> +\tbool invalid()\n> +\t{\n> +\t\treturn valid_ = false;\n> +\t}\n> +\tSize maxVideoResolution()\n> +\t{\n> +\t\treturn maxVideoResolution_;\n> +\t}\n> +\tSize maxStillResolution()\n> +\t{\n> +\t\treturn maxStillResolution_;\n> +\t}\n> +\tSize minVideoResolution()\n> +\t{\n> +\t\treturn minVideoResolution_;\n> +\t}\n> +\tSize minStillResolution()\n> +\t{\n> +\t\treturn minStillResolution_;\n> +\t}\n> +\n> +\tint queryPipeConfig(const Size &videoMain, const Size &videoViewfinder,\n> +\t\t\t const Size &stillMain, const Size &stillViewfinder,\n> +\t\t\t PipeConfig &videoPipeConfig,\n> +\t\t\t PipeConfig &stillPipeConfig) const;\n> +\tvoid dump();\n> +\n> +private:\n> +\tint load(const YamlObject &object);\n> +\tbool valid_;\n> +\n> +\tstd::vector videoPipeConfigs_;\n> +\tstd::vector stillPipeConfigs_;\n> +\n> +\tSize maxVideoResolution_;\n> +\tSize maxStillResolution_;\n> +\n> +\tSize minVideoResolution_;\n> +\tSize minStillResolution_;\n> +};\n> +\n> +} /* namespace libcamera */","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id EE0EDC0F1B\n\tfor ;\n\tWed, 6 Apr 2022 01:47:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4CF3C65642;\n\tWed, 6 Apr 2022 03:47:32 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 59D80604B6\n\tfor ;\n\tWed, 6 Apr 2022 03:47:31 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E33FF482;\n\tWed, 6 Apr 2022 03:47:30 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1649209652;\n\tbh=mH15T8U1BStutZpb0gYOv6LKkMlkUf8LxcPh0aFV5cM=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=Rjp0BXcL304X2gCwCC6RDNCJ3+vTR1MRxF77PrRUlOWJMrs9l5qOOQuPzHMhU+uAb\n\tDANeIjcWx0dsrO34iDHJMNUVH6IvE/Ye+cmWpqG5ym3aYYh+z3dTQgJ+GqgbjtZDh5\n\tuePye+6Nxa76nykHlnCZEAkvrBrL0rvPhHt2V4dpEPcO85mlhIujJ8WjY5T8vCVHCY\n\tMuOhM8tonlLlT3RqY45TpqKZ7xAZXmGOuEAjSCwj5F1srA/7WWT8c90TDUSA7fGQEV\n\t37Ex726en1Vdf90vIpOS9m9rm7f6Xjp5Tv0tmWmwicLThXLVAXFzdqMUnrRO/SAQU7\n\t1cUhQVWtQ6HJQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1649209651;\n\tbh=mH15T8U1BStutZpb0gYOv6LKkMlkUf8LxcPh0aFV5cM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=dkLWq4zozGlXBuO6KCnAwif/TCqsZk+PiVIh1D+oIdpgN5Ybo3f6wDQKLyJIxIAvX\n\tAkK9iAbZ/7TikCZqhPC4Bv7TnipipzqmbmiTLfPcN3VTOlbLUMC0NnNstSlJSxEalp\n\ta58hTLZ9oMw/7lIOYeMKp0AvNeuf30FGYTR0xVJ0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"dkLWq4zo\"; dkim-atps=neutral","Date":"Wed, 6 Apr 2022 04:47:27 +0300","To":"Han-Lin Chen ","Message-ID":"","References":"<20220209071917.559993-1-hanlinchen@chromium.org>\n\t<20220209071917.559993-8-hanlinchen@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220209071917.559993-8-hanlinchen@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH 7/9] libcamera: ipu3: Add helper class\n\tPipeConfigPreference","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22633,"web_url":"https://patchwork.libcamera.org/comment/22633/","msgid":"<164927087099.4051137.11389690945396795355@Monstersaurus>","date":"2022-04-06T18:47:50","subject":"Re: [libcamera-devel] [PATCH 7/9] libcamera: ipu3: Add helper class\n\tPipeConfigPreference","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart via libcamera-devel (2022-04-06 02:47:27)\n> Hi Han-Lin,\n> \n> Thank you for the patch.\n> \n> On Wed, Feb 09, 2022 at 03:19:15PM +0800, Han-Lin Chen wrote:\n> > Add helper class PipeConfigPreference to load the caliberated ipu3\n> > pipeline configuration files, and provides query interface for the\n> > pipeline handler.\n> > \n> > Signed-off-by: Han-Lin Chen \n> > ---\n> > include/libcamera/geometry.h | 4 +\n> > src/libcamera/geometry.cpp | 20 ++\n> \n> Could you please split the changes to geometry.h and geometry.cpp to a\n> separate patch ?\n> \n\n\n\n> > +/**\n> > + * \\brief Insert operation for Point with ostream\n> > + * \\return The input std::ostream\n> > + */\n> > +std::ostream &operator<<(std::ostream &out, const Point &p)\n> > +{\n> > + out << \"(\" << p.x << \", \" << p.y << \")\";\n> \n> This doesn't match the format used by Point::toString(), which can be\n> confusing.\n> \n> I'm actually wondering if we could use toString() instead of adding\n> operator<<() overloads. I half-recall having the same discussion a long\n> time ago and advocating against operator<<(), but I can't recall why. Or\n> maybe I don't remember correctly, and operator<<() is fine :-) Does\n> anyone have an opinion on this ?\n\nPersonally, I like having operator<< implementations, but I think they\nshould be of the form:\n\n\nstd::ostream &operator<<(std::ostream &out, const Point &p)\n{\n\treturn out << p.toString();\n}\n\nor such to ensure the implementation is consistent.\n\nIf we can have that, then I'd like to add lots more around for other\nobjects ;-)\n\n\n\n> \n> > + return out;\n> > +}\n> > +\n> > /**\n> > * \\struct Size\n> > * \\brief Describe a two-dimensional size\n> > @@ -428,6 +438,16 @@ bool operator<(const Size &lhs, const Size &rhs)\n> > * \\sa bool operator<(const Size &lhs, const Size &rhs)\n> > */\n> > \n> > +/**\n> > + * \\brief Insert operation for Size with ostream\n> > + * \\return The input std::ostream\n> > + */\n> > +std::ostream &operator<<(std::ostream &out, const Size &s)\n> > +{\n> > + out << s.width << \"x\" << s.height;\n> > + return out;\n> > +}\n> > +\n> \n> For completeness, can you add the operators for SizeRange and Rectangle\n> too ?\n\nYes, I would say a single patch should update all objects in geometry to\nuse the same consistent addition. We can tackle other classes/files\nlater.\n\n\n\nKieran","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 420F1C0F1B\n\tfor ;\n\tWed, 6 Apr 2022 18:47:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8B23F65642;\n\tWed, 6 Apr 2022 20:47:55 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2BED1604B8\n\tfor ;\n\tWed, 6 Apr 2022 20:47:54 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 99BC4482;\n\tWed, 6 Apr 2022 20:47:53 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1649270875;\n\tbh=Ergx/iZOeAXkK1Ui8+VEWPz3aLKzKjFJ9t1g9mEdzAA=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=fKvBnpb//uHW11u9SRBdGka+k9Ansyz9DtsEudCEKU+IBtVxLjXYQHiesYJ7Quhto\n\tSI7uMdaHGzi0OW8jCL9tFBTfrGrStt3QA16fTJ5nqTqdxGwQWRos1BCFfG+RcOwbtm\n\t370MBh2PfO/JiK0p1LEVgdYQ3A5ZFNUKkKukr8KH22u/AyZ261csc9P/QvKf+Jj6hp\n\tPJI5gSiMT8RvbZNzawr88fV/7ptKflBXRnEn8DMsLKtpp59Q7j570dy9eT2688aHH2\n\tHMamFb5PuIFAFsnCsVqZvmJe4jDa1SAiKVmBN9x8oL39zGfuXmN45evm+SAgD/6YDo\n\tCyOBbGHNwfQIA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1649270873;\n\tbh=Ergx/iZOeAXkK1Ui8+VEWPz3aLKzKjFJ9t1g9mEdzAA=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=LUwpxiG911wHTmuNCHhiyNzQQlaw/vD2NK/WJaHWZfCYZx5Rq8j9x2ZcgstMKn3Ze\n\tlx+Ynu7QuHjoO0/lNuwrA21GIxSpVZreexAOghwLHkwNt2aR3U1R+b+GQncAFrMD5S\n\t70DPTAcVYv3q2aS9qRJEP3V6vGiU1rCeEcAkWOYQ="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"LUwpxiG9\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20220209071917.559993-1-hanlinchen@chromium.org>\n\t<20220209071917.559993-8-hanlinchen@chromium.org>\n\t","To":"Han-Lin Chen ,\n\tLaurent Pinchart ,\n\tLaurent Pinchart via libcamera-devel\n\t","Date":"Wed, 06 Apr 2022 19:47:50 +0100","Message-ID":"<164927087099.4051137.11389690945396795355@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH 7/9] libcamera: ipu3: Add helper class\n\tPipeConfigPreference","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22638,"web_url":"https://patchwork.libcamera.org/comment/22638/","msgid":"","date":"2022-04-07T07:26:48","subject":"Re: [libcamera-devel] [PATCH 7/9] libcamera: ipu3: Add helper class\n\tPipeConfigPreference","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Apr 06, 2022 at 07:47:50PM +0100, Kieran Bingham wrote:\n> Quoting Laurent Pinchart via libcamera-devel (2022-04-06 02:47:27)\n> > Hi Han-Lin,\n> > \n> > Thank you for the patch.\n> > \n> > On Wed, Feb 09, 2022 at 03:19:15PM +0800, Han-Lin Chen wrote:\n> > > Add helper class PipeConfigPreference to load the caliberated ipu3\n> > > pipeline configuration files, and provides query interface for the\n> > > pipeline handler.\n> > > \n> > > Signed-off-by: Han-Lin Chen \n> > > ---\n> > > include/libcamera/geometry.h | 4 +\n> > > src/libcamera/geometry.cpp | 20 ++\n> > \n> > Could you please split the changes to geometry.h and geometry.cpp to a\n> > separate patch ?\n> \n> \n> \n> > > +/**\n> > > + * \\brief Insert operation for Point with ostream\n> > > + * \\return The input std::ostream\n> > > + */\n> > > +std::ostream &operator<<(std::ostream &out, const Point &p)\n> > > +{\n> > > + out << \"(\" << p.x << \", \" << p.y << \")\";\n> > \n> > This doesn't match the format used by Point::toString(), which can be\n> > confusing.\n> > \n> > I'm actually wondering if we could use toString() instead of adding\n> > operator<<() overloads. I half-recall having the same discussion a long\n> > time ago and advocating against operator<<(), but I can't recall why. Or\n> > maybe I don't remember correctly, and operator<<() is fine :-) Does\n> > anyone have an opinion on this ?\n> \n> Personally, I like having operator<< implementations, but I think they\n> should be of the form:\n> \n> \n> std::ostream &operator<<(std::ostream &out, const Point &p)\n> {\n> \treturn out << p.toString();\n> }\n> \n> or such to ensure the implementation is consistent.\n\nI was thinking about that, and then wondered about the efficiency\ncompared to open-coding it. Maybe we could do it the other way around,\nimplement toString() based on operator<<() ? Or maybe it doesn't matter\n?\n\n> If we can have that, then I'd like to add lots more around for other\n> objects ;-)\n> \n> > > + return out;\n> > > +}\n> > > +\n> > > /**\n> > > * \\struct Size\n> > > * \\brief Describe a two-dimensional size\n> > > @@ -428,6 +438,16 @@ bool operator<(const Size &lhs, const Size &rhs)\n> > > * \\sa bool operator<(const Size &lhs, const Size &rhs)\n> > > */\n> > > \n> > > +/**\n> > > + * \\brief Insert operation for Size with ostream\n> > > + * \\return The input std::ostream\n> > > + */\n> > > +std::ostream &operator<<(std::ostream &out, const Size &s)\n> > > +{\n> > > + out << s.width << \"x\" << s.height;\n> > > + return out;\n> > > +}\n> > > +\n> > \n> > For completeness, can you add the operators for SizeRange and Rectangle\n> > too ?\n> \n> Yes, I would say a single patch should update all objects in geometry to\n> use the same consistent addition. We can tackle other classes/files\n> later.\n> \n> ","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 6DDAEC3256\n\tfor ;\n\tThu, 7 Apr 2022 07:26:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C225E65642;\n\tThu, 7 Apr 2022 09:26:53 +0200 (CEST)","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 D4BA561FBB\n\tfor ;\n\tThu, 7 Apr 2022 09:26:52 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2B174499;\n\tThu, 7 Apr 2022 09:26:52 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1649316413;\n\tbh=h2RQOATAOtxLQDmja0N8ABWRsl+1/6zMZHwzjXJH6FI=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=tvZkkKIq1marJrLFwGGh4g32eKj4yHwPyle/lF2mJVcRIvY5GZHpNBTY9MX9dAoyG\n\tHqy4htE8LpMYrrKOyt7Aoq7LhRbGGUShx5ELLib6Vab+TrLBJoEB0INvCzsee7AB7m\n\tdouB9+jTJiu9kUR+eIvKX0M1KLUGnLffQW6DtddWFK7wR51WwJmL1r4wAju6Gn7VfB\n\tDOpRi3hczuiFvLfrUeyIENo8Y19MZeM6UQpWC6uN21eHeWdlqYsHPZAMBmQ518jbb3\n\tmDRcFmwq8NF2ZQRd6oYGu0ek+oRamDLMVTuVnwlrmIH+RMfrrE5ABTMtuVc07jcVP0\n\tGqTeWkG9YRlpg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1649316412;\n\tbh=h2RQOATAOtxLQDmja0N8ABWRsl+1/6zMZHwzjXJH6FI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=HHMH3cIXENzBKrKsx208estX/iWLa0VoWnlhZRyk/0Nr9fZzquqIVIvkIWQsG7bJK\n\t3B2FTdvNGNS5rFNFoQjchFMm4+CWSXnmZsdML4XFCXPvXb89fZ84cJNiU+Y6eDd4+d\n\tWqB6QRlz1V+baEElCeW3JAZadSDRjaZT0pq11s7k="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"HHMH3cIX\"; dkim-atps=neutral","Date":"Thu, 7 Apr 2022 10:26:48 +0300","To":"Kieran Bingham ","Message-ID":"","References":"<20220209071917.559993-1-hanlinchen@chromium.org>\n\t<20220209071917.559993-8-hanlinchen@chromium.org>\n\t\n\t<164927087099.4051137.11389690945396795355@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<164927087099.4051137.11389690945396795355@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH 7/9] libcamera: ipu3: Add helper class\n\tPipeConfigPreference","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"Laurent Pinchart via libcamera-devel\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22639,"web_url":"https://patchwork.libcamera.org/comment/22639/","msgid":"<164932215409.4051137.6299442474146850362@Monstersaurus>","date":"2022-04-07T09:02:34","subject":"Re: [libcamera-devel] [PATCH 7/9] libcamera: ipu3: Add helper class\n\tPipeConfigPreference","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart (2022-04-07 08:26:48)\n> On Wed, Apr 06, 2022 at 07:47:50PM +0100, Kieran Bingham wrote:\n> > Quoting Laurent Pinchart via libcamera-devel (2022-04-06 02:47:27)\n> > > Hi Han-Lin,\n> > > \n> > > Thank you for the patch.\n> > > \n> > > On Wed, Feb 09, 2022 at 03:19:15PM +0800, Han-Lin Chen wrote:\n> > > > Add helper class PipeConfigPreference to load the caliberated ipu3\n> > > > pipeline configuration files, and provides query interface for the\n> > > > pipeline handler.\n> > > > \n> > > > Signed-off-by: Han-Lin Chen \n> > > > ---\n> > > > include/libcamera/geometry.h | 4 +\n> > > > src/libcamera/geometry.cpp | 20 ++\n> > > \n> > > Could you please split the changes to geometry.h and geometry.cpp to a\n> > > separate patch ?\n> > \n> > \n> > \n> > > > +/**\n> > > > + * \\brief Insert operation for Point with ostream\n> > > > + * \\return The input std::ostream\n> > > > + */\n> > > > +std::ostream &operator<<(std::ostream &out, const Point &p)\n> > > > +{\n> > > > + out << \"(\" << p.x << \", \" << p.y << \")\";\n> > > \n> > > This doesn't match the format used by Point::toString(), which can be\n> > > confusing.\n> > > \n> > > I'm actually wondering if we could use toString() instead of adding\n> > > operator<<() overloads. I half-recall having the same discussion a long\n> > > time ago and advocating against operator<<(), but I can't recall why. Or\n> > > maybe I don't remember correctly, and operator<<() is fine :-) Does\n> > > anyone have an opinion on this ?\n> > \n> > Personally, I like having operator<< implementations, but I think they\n> > should be of the form:\n> > \n> > \n> > std::ostream &operator<<(std::ostream &out, const Point &p)\n> > {\n> > return out << p.toString();\n> > }\n> > \n> > or such to ensure the implementation is consistent.\n> \n> I was thinking about that, and then wondered about the efficiency\n> compared to open-coding it. Maybe we could do it the other way around,\n> implement toString() based on operator<<() ? Or maybe it doesn't matter\n> ?\n\nI think open coding and duplicating the prints leads to a strong chance\nof inconsistent prints. I would be surprised if either the compiler can\nnot detect that this could be inlined, or if the extra function call\noverhead here is troublesome compared to the actual generation of the\nstring.\n\n\n> \n> > If we can have that, then I'd like to add lots more around for other\n> > objects ;-)\n> > \n> > > > + return out;\n> > > > +}\n> > > > +\n> > > > /**\n> > > > * \\struct Size\n> > > > * \\brief Describe a two-dimensional size\n> > > > @@ -428,6 +438,16 @@ bool operator<(const Size &lhs, const Size &rhs)\n> > > > * \\sa bool operator<(const Size &lhs, const Size &rhs)\n> > > > */\n> > > > \n> > > > +/**\n> > > > + * \\brief Insert operation for Size with ostream\n> > > > + * \\return The input std::ostream\n> > > > + */\n> > > > +std::ostream &operator<<(std::ostream &out, const Size &s)\n> > > > +{\n> > > > + out << s.width << \"x\" << s.height;\n> > > > + return out;\n> > > > +}\n> > > > +\n> > > \n> > > For completeness, can you add the operators for SizeRange and Rectangle\n> > > too ?\n> > \n> > Yes, I would say a single patch should update all objects in geometry to\n> > use the same consistent addition. We can tackle other classes/files\n> > later.\n> > \n> > \n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 4B026C3256\n\tfor ;\n\tThu, 7 Apr 2022 09:02:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A602B65645;\n\tThu, 7 Apr 2022 11:02:37 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0CA4261FBB\n\tfor ;\n\tThu, 7 Apr 2022 11:02:37 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8E280499;\n\tThu, 7 Apr 2022 11:02:36 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1649322157;\n\tbh=3gpUw3ns+yF/vAvYH3/LImaJ6Er8zYR2FmxNzUz7AEc=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=NZd7rFgHwoTX1z4P6+nh9DOGRfySRp70crRq15EQwjvfN4Q5NV1SGgTpy+O2glc42\n\tQEj6lzAIW2cInpoGYMG+7a57BhcG2o4sSFDpGEeBCK8uhACGiOAwscndRK6zx2Kiye\n\tzmNlmCkG44FV9guPCBiv3fWmirD2fbv+Rn1dmh3VQ/VWPpe2cLsQ2r3MJZgcX2x01S\n\ttcwSzMIDW0ZVagsI8Umt4bVHfTQ0Sv4ZEmq1BnLeYBs1g62pQfNJ4qq/N6aVUhVnve\n\tGuf07kmPVATfkBJWS5GrqCN73OpTmQRzpRf8maDOTtfB4mFS5WHsuxdbE9N7X3cNvP\n\tdRbIcjlriCRXQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1649322156;\n\tbh=3gpUw3ns+yF/vAvYH3/LImaJ6Er8zYR2FmxNzUz7AEc=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=Q4SSCEi4uY4mxPbdiFDH6RebmDDfCFhpSpJR7DqHGS0Ix2e5U0GrFKjC4XdpVUCm0\n\tukoP6R/ZQIFdSwSLr9+xVIErn4lhDCPeHnZtclk97qQeg6F0ju8dxkJROBIWp9qWxH\n\tQz1DsZ9fLmZM8/0omoryaZ800Yi/aU9cbNmo5INE="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Q4SSCEi4\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20220209071917.559993-1-hanlinchen@chromium.org>\n\t<20220209071917.559993-8-hanlinchen@chromium.org>\n\t\n\t<164927087099.4051137.11389690945396795355@Monstersaurus>\n\t","To":"Laurent Pinchart ","Date":"Thu, 07 Apr 2022 10:02:34 +0100","Message-ID":"<164932215409.4051137.6299442474146850362@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH 7/9] libcamera: ipu3: Add helper class\n\tPipeConfigPreference","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"Laurent Pinchart via libcamera-devel\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22640,"web_url":"https://patchwork.libcamera.org/comment/22640/","msgid":"","date":"2022-04-07T09:04:18","subject":"Re: [libcamera-devel] [PATCH 7/9] libcamera: ipu3: Add helper class\n\tPipeConfigPreference","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Thu, Apr 07, 2022 at 10:02:34AM +0100, Kieran Bingham wrote:\n> Quoting Laurent Pinchart (2022-04-07 08:26:48)\n> > On Wed, Apr 06, 2022 at 07:47:50PM +0100, Kieran Bingham wrote:\n> > > Quoting Laurent Pinchart via libcamera-devel (2022-04-06 02:47:27)\n> > > > Hi Han-Lin,\n> > > > \n> > > > Thank you for the patch.\n> > > > \n> > > > On Wed, Feb 09, 2022 at 03:19:15PM +0800, Han-Lin Chen wrote:\n> > > > > Add helper class PipeConfigPreference to load the caliberated ipu3\n> > > > > pipeline configuration files, and provides query interface for the\n> > > > > pipeline handler.\n> > > > > \n> > > > > Signed-off-by: Han-Lin Chen \n> > > > > ---\n> > > > > include/libcamera/geometry.h | 4 +\n> > > > > src/libcamera/geometry.cpp | 20 ++\n> > > > \n> > > > Could you please split the changes to geometry.h and geometry.cpp to a\n> > > > separate patch ?\n> > > \n> > > \n> > > \n> > > > > +/**\n> > > > > + * \\brief Insert operation for Point with ostream\n> > > > > + * \\return The input std::ostream\n> > > > > + */\n> > > > > +std::ostream &operator<<(std::ostream &out, const Point &p)\n> > > > > +{\n> > > > > + out << \"(\" << p.x << \", \" << p.y << \")\";\n> > > > \n> > > > This doesn't match the format used by Point::toString(), which can be\n> > > > confusing.\n> > > > \n> > > > I'm actually wondering if we could use toString() instead of adding\n> > > > operator<<() overloads. I half-recall having the same discussion a long\n> > > > time ago and advocating against operator<<(), but I can't recall why. Or\n> > > > maybe I don't remember correctly, and operator<<() is fine :-) Does\n> > > > anyone have an opinion on this ?\n> > > \n> > > Personally, I like having operator<< implementations, but I think they\n> > > should be of the form:\n> > > \n> > > \n> > > std::ostream &operator<<(std::ostream &out, const Point &p)\n> > > {\n> > > return out << p.toString();\n> > > }\n> > > \n> > > or such to ensure the implementation is consistent.\n> > \n> > I was thinking about that, and then wondered about the efficiency\n> > compared to open-coding it. Maybe we could do it the other way around,\n> > implement toString() based on operator<<() ? Or maybe it doesn't matter\n> > ?\n> \n> I think open coding and duplicating the prints leads to a strong chance\n> of inconsistent prints. I would be surprised if either the compiler can\n> not detect that this could be inlined, or if the extra function call\n> overhead here is troublesome compared to the actual generation of the\n> string.\n\nThe reason why I thought we could implement toString() based on\noperator<<() instead of the other way around is that toString() already\nuses a stringstream in some classes, so it could be more efficient.\n\n> > > If we can have that, then I'd like to add lots more around for other\n> > > objects ;-)\n> > > \n> > > > > + return out;\n> > > > > +}\n> > > > > +\n> > > > > /**\n> > > > > * \\struct Size\n> > > > > * \\brief Describe a two-dimensional size\n> > > > > @@ -428,6 +438,16 @@ bool operator<(const Size &lhs, const Size &rhs)\n> > > > > * \\sa bool operator<(const Size &lhs, const Size &rhs)\n> > > > > */\n> > > > > \n> > > > > +/**\n> > > > > + * \\brief Insert operation for Size with ostream\n> > > > > + * \\return The input std::ostream\n> > > > > + */\n> > > > > +std::ostream &operator<<(std::ostream &out, const Size &s)\n> > > > > +{\n> > > > > + out << s.width << \"x\" << s.height;\n> > > > > + return out;\n> > > > > +}\n> > > > > +\n> > > > \n> > > > For completeness, can you add the operators for SizeRange and Rectangle\n> > > > too ?\n> > > \n> > > Yes, I would say a single patch should update all objects in geometry to\n> > > use the same consistent addition. We can tackle other classes/files\n> > > later.\n> > > \n> > > ","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 31C0BC3256\n\tfor ;\n\tThu, 7 Apr 2022 09:04:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CDA2B65642;\n\tThu, 7 Apr 2022 11:04:22 +0200 (CEST)","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 4FEA061FBB\n\tfor ;\n\tThu, 7 Apr 2022 11:04:22 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CB682499;\n\tThu, 7 Apr 2022 11:04:21 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1649322262;\n\tbh=Ooks9LR5bn0Iqk13caH7EmxOi4UFUpM/kFC6V6ZoMvM=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=YMS0bAHCYFNAwEN0/QtX688TVbpgJsSVodUP3KC06rpbst4uDPWEW/rxOnHHDdLew\n\tM84EVVidFi0f9skIUupkN49JObViEkHyhh6IFln3ImYebz9q8/G1Na4N8SPhvqV3qJ\n\tXxqyV1j1DnWc6ZGNH6ViMGJgrgzAonrkG53on8WYji8qlHmRaRAg4zobBZD2e0jRUj\n\trbieQ367i+JfLlQ6Pr0yReeH47/Y7kQ9fiAvH40P/0FPmUZv1ReZmjdAWS64a60rV4\n\tsSrpWPMkQ5saHpmcf1DdCEESuqHsW2QJ1jkjFrELw2TeHTV5LXQyqPRJkG4MhYfQ5r\n\t6Xzef1DxWI3xw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1649322261;\n\tbh=Ooks9LR5bn0Iqk13caH7EmxOi4UFUpM/kFC6V6ZoMvM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Py2BZSt0owjurS9h+uWIn7v4yAHJjtIRNvyr0sxoCjPKqEk+4BGRMHRPUbEkN2IzZ\n\tr/DDn5zLy2zcfp6uczODb5n15UCXBscvxc0RcqFismtDTMvYl0UqGj4LAsJ0gvxI2H\n\tvVajbcjgTtX0onFgVDJTx/iaJk1GRAp2z+CrgQus="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Py2BZSt0\"; dkim-atps=neutral","Date":"Thu, 7 Apr 2022 12:04:18 +0300","To":"Kieran Bingham ","Message-ID":"","References":"<20220209071917.559993-1-hanlinchen@chromium.org>\n\t<20220209071917.559993-8-hanlinchen@chromium.org>\n\t\n\t<164927087099.4051137.11389690945396795355@Monstersaurus>\n\t\n\t<164932215409.4051137.6299442474146850362@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<164932215409.4051137.6299442474146850362@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH 7/9] libcamera: ipu3: Add helper class\n\tPipeConfigPreference","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"Laurent Pinchart via libcamera-devel\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22656,"web_url":"https://patchwork.libcamera.org/comment/22656/","msgid":"<164937081814.127674.17532636160670939738@Monstersaurus>","date":"2022-04-07T22:33:38","subject":"Re: [libcamera-devel] [PATCH 7/9] libcamera: ipu3: Add helper class\n\tPipeConfigPreference","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart (2022-04-07 10:04:18)\n> On Thu, Apr 07, 2022 at 10:02:34AM +0100, Kieran Bingham wrote:\n> > Quoting Laurent Pinchart (2022-04-07 08:26:48)\n> > > On Wed, Apr 06, 2022 at 07:47:50PM +0100, Kieran Bingham wrote:\n> > > > Quoting Laurent Pinchart via libcamera-devel (2022-04-06 02:47:27)\n> > > > > Hi Han-Lin,\n> > > > > \n> > > > > Thank you for the patch.\n> > > > > \n> > > > > On Wed, Feb 09, 2022 at 03:19:15PM +0800, Han-Lin Chen wrote:\n> > > > > > Add helper class PipeConfigPreference to load the caliberated ipu3\n> > > > > > pipeline configuration files, and provides query interface for the\n> > > > > > pipeline handler.\n> > > > > > \n> > > > > > Signed-off-by: Han-Lin Chen \n> > > > > > ---\n> > > > > > include/libcamera/geometry.h | 4 +\n> > > > > > src/libcamera/geometry.cpp | 20 ++\n> > > > > \n> > > > > Could you please split the changes to geometry.h and geometry.cpp to a\n> > > > > separate patch ?\n> > > > \n> > > > \n> > > > \n> > > > > > +/**\n> > > > > > + * \\brief Insert operation for Point with ostream\n> > > > > > + * \\return The input std::ostream\n> > > > > > + */\n> > > > > > +std::ostream &operator<<(std::ostream &out, const Point &p)\n> > > > > > +{\n> > > > > > + out << \"(\" << p.x << \", \" << p.y << \")\";\n> > > > > \n> > > > > This doesn't match the format used by Point::toString(), which can be\n> > > > > confusing.\n> > > > > \n> > > > > I'm actually wondering if we could use toString() instead of adding\n> > > > > operator<<() overloads. I half-recall having the same discussion a long\n> > > > > time ago and advocating against operator<<(), but I can't recall why. Or\n> > > > > maybe I don't remember correctly, and operator<<() is fine :-) Does\n> > > > > anyone have an opinion on this ?\n> > > > \n> > > > Personally, I like having operator<< implementations, but I think they\n> > > > should be of the form:\n> > > > \n> > > > \n> > > > std::ostream &operator<<(std::ostream &out, const Point &p)\n> > > > {\n> > > > return out << p.toString();\n> > > > }\n> > > > \n> > > > or such to ensure the implementation is consistent.\n> > > \n> > > I was thinking about that, and then wondered about the efficiency\n> > > compared to open-coding it. Maybe we could do it the other way around,\n> > > implement toString() based on operator<<() ? Or maybe it doesn't matter\n> > > ?\n> > \n> > I think open coding and duplicating the prints leads to a strong chance\n> > of inconsistent prints. I would be surprised if either the compiler can\n> > not detect that this could be inlined, or if the extra function call\n> > overhead here is troublesome compared to the actual generation of the\n> > string.\n> \n> The reason why I thought we could implement toString() based on\n> operator<<() instead of the other way around is that toString() already\n> uses a stringstream in some classes, so it could be more efficient.\n> \n\nOk - so sounds like it could be an implementation detail when it's used\nanyway... unless you want to mandate that they are always coded a set\ndirection?\n\n\n> > > > If we can have that, then I'd like to add lots more around for other\n> > > > objects ;-)\n> > > > \n> > > > > > + return out;\n> > > > > > +}\n> > > > > > +\n> > > > > > /**\n> > > > > > * \\struct Size\n> > > > > > * \\brief Describe a two-dimensional size\n> > > > > > @@ -428,6 +438,16 @@ bool operator<(const Size &lhs, const Size &rhs)\n> > > > > > * \\sa bool operator<(const Size &lhs, const Size &rhs)\n> > > > > > */\n> > > > > > \n> > > > > > +/**\n> > > > > > + * \\brief Insert operation for Size with ostream\n> > > > > > + * \\return The input std::ostream\n> > > > > > + */\n> > > > > > +std::ostream &operator<<(std::ostream &out, const Size &s)\n> > > > > > +{\n> > > > > > + out << s.width << \"x\" << s.height;\n> > > > > > + return out;\n> > > > > > +}\n> > > > > > +\n> > > > > \n> > > > > For completeness, can you add the operators for SizeRange and Rectangle\n> > > > > too ?\n> > > > \n> > > > Yes, I would say a single patch should update all objects in geometry to\n> > > > use the same consistent addition. We can tackle other classes/files\n> > > > later.\n> > > > \n> > > > \n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 3E432C3256\n\tfor ;\n\tThu, 7 Apr 2022 22:33:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 924BE65640;\n\tFri, 8 Apr 2022 00:33:42 +0200 (CEST)","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 7CD94604B8\n\tfor ;\n\tFri, 8 Apr 2022 00:33:41 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id ECF84880;\n\tFri, 8 Apr 2022 00:33:40 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1649370822;\n\tbh=mioV1AQSPVWCWQfaC/PRLatP76Waw3gHo1yYEd5L1bo=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=mI15sI2NVtrGPfY/oOngN5iEU5D7rBFu8igIm3TJvh+elsID4Zd8e7yr9ZO6FVKMV\n\t1/meePYhL01a61hPhDOrGrOuvWVtcAxPpNA85plBpmrIaEDAvhhGutsGU/8SAwcY1g\n\tee3g6rdxEEMb9V7jIJN4wcJjEaJ9LYp3NYaybqiCo/DnxaKBTpQllguZFZ4YVcVled\n\t/ehOJrJg+NExG8xQQY0Sp3JPH7vK1BWZCWY8SOdPvhU6//i01ea9c3XHkVJuNG22iM\n\t7n1p/LWT0U8+GWD8CGNK7Ak8jjzpuAoVE5VcvSUWVUrtFYjuU+g/h06Liy7i8JIy24\n\t1NaUJoPguTrVA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1649370821;\n\tbh=mioV1AQSPVWCWQfaC/PRLatP76Waw3gHo1yYEd5L1bo=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=WM9fqV99Fu90TxyTU7xsT5pAF3WNiUNnbAA+Hyg2GEdE5xPRmijf+Sx0Ilp+v8c8c\n\tBygkr4pYvB1XyD6QkmCQrFIrtnWNmkOPip88HSIef73yEGhXvXq+YO+gAmybwVLtag\n\tHWZeUHWznNdrZ3+4Zd8C8kPVpcVyGgoOMlho8weE="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"WM9fqV99\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20220209071917.559993-1-hanlinchen@chromium.org>\n\t<20220209071917.559993-8-hanlinchen@chromium.org>\n\t\n\t<164927087099.4051137.11389690945396795355@Monstersaurus>\n\t\n\t<164932215409.4051137.6299442474146850362@Monstersaurus>\n\t","To":"Laurent Pinchart ","Date":"Thu, 07 Apr 2022 23:33:38 +0100","Message-ID":"<164937081814.127674.17532636160670939738@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH 7/9] libcamera: ipu3: Add helper class\n\tPipeConfigPreference","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"Laurent Pinchart via libcamera-devel\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]