[{"id":16041,"web_url":"https://patchwork.libcamera.org/comment/16041/","msgid":"","date":"2021-03-30T03:17:18","subject":"Re: [libcamera-devel] [PATCH v3 2/5] ipa: ipu3: Add an histogram\n\tclass","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jean-Michel,\n\nThank you for the patch.\n\nOn Mon, Mar 29, 2021 at 09:18:23PM +0200, Jean-Michel Hautbois wrote:\n> This class will be used at least by AGC algorithm when quantiles are\n> needed for example.\n> \n> Signed-off-by: Jean-Michel Hautbois \n> ---\n> src/ipa/libipa/histogram.cpp | 102 +++++++++++++++++++++++++++++++++++\n> src/ipa/libipa/histogram.h | 62 +++++++++++++++++++++\n> src/ipa/libipa/meson.build | 2 +\n> 3 files changed, 166 insertions(+)\n> create mode 100644 src/ipa/libipa/histogram.cpp\n> create mode 100644 src/ipa/libipa/histogram.h\n> \n> diff --git a/src/ipa/libipa/histogram.cpp b/src/ipa/libipa/histogram.cpp\n> new file mode 100644\n> index 00000000..bea52687\n> --- /dev/null\n> +++ b/src/ipa/libipa/histogram.cpp\n> @@ -0,0 +1,102 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited\n> + *\n> + * histogram.cpp - histogram calculations\n> + */\n> +#include \n\n (and std:: functions below) as explained in\nDocumentation/coding-style.rst.\n\n> +\n> +#include \"histogram.h\"\n\nDocumentation/coding-style.rst:\n\n\"For .cpp files, if the file implements an API declared in a header\nfile, that header file shall be included first in order to ensure it is\nself-contained.\"\n\nhistogram.h should thus go first.\n\n> +\n> +/**\n> + * \\file histogram.h\n> + * \\brief Class to represent Histograms and manipulate them\n> + */\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa {\n> +\n> +/**\n> + * \\class Histogram\n> + * \\brief The base class for creating histograms\n> + *\n> + * The Histogram class defines a standard interface for IPA algorithms. By\n> + * abstracting histograms, it makes possible the implementation of generic code\n\ns/makes possible the implementation of/makes it possible to implement/\n\n> + * to manage algorithms regardless of their specific type.\n\nDid you mean histograms instead of algorithms here ?\n\nMore information is needed here, to explain what the class does. In\nparticular, despite its name, this class doesn't store a histogram, so\nthat can be confusing.\n\n> + */\n> +\n> +/**\n> + * \\brief Cumulative frequency up to a (fractional) point in a bin.\n> + * \\param[in] bin the number of bins\n\nI don't think this parameter stores the number of bins.\n\n> + * \\return The number of bins cumulated\n\nThe functions also need better documentation. The reader needs to be\nable to understand what the function does and how to use it by reading\nthe documentation, without reading the code.\n\nOne important piece of information here is how the function assumes a\nuniform distribution of values within a bin. I think that's an\nacceptable compromise, but it should be documented.\n\nThe functions below also need better documentation, as they're more\ndifficult to understand.\n\n> + */\n> +uint64_t Histogram::cumulativeFreq(double bin) const\n> +{\n> +\tif (bin <= 0)\n> +\t\treturn 0;\n> +\telse if (bin >= bins())\n> +\t\treturn total();\n> +\tint b = (int)bin;\n\nPlease use C++-style casts (static_cast in this case).\n\n> +\treturn cumulative_[b] +\n> +\t (bin - b) * (cumulative_[b + 1] - cumulative_[b]);\n> +}\n> +\n> +/**\n> + * \\brief Return the (fractional) bin of the point through the histogram\n> + * \\param[in] q the desired point (0 <= q <= 1)\n> + * \\param[in] first low limit (optionnal if -1)\n\ns/optionnal/optional/\n\n> + * \\param[in] last high limit (optionnal if -1)\n> + * \\return The fractionnal bin of the point\n\ns/fractionnal/fractional/\n\n> + */\n> +double Histogram::quantile(double q, int first, int last) const\n> +{\n> +\tif (first == -1)\n> +\t\tfirst = 0;\n\nWhy can't we then use 0 as the default value for first ? We could then\nmake it an unsigned int.\n\n> +\tif (last == -1)\n> +\t\tlast = cumulative_.size() - 2;\n\nAnd you can use UINT_MAX as the default value for max, which will also\nallow making it an unsigned int. That way we won't risk the values being\nnegative.\n\n> +\tassert(first <= last);\n> +\tuint64_t items = q * total();\n\nitems or item ?\n\n> +\t/* Binary search to find the right bin */\n> +\twhile (first < last) {\n> +\t\tint middle = (first + last) / 2;\n> +\t\t/* Is it between first and middle ? */\n> +\t\tif (cumulative_[middle + 1] > items)\n> +\t\t\tlast = middle;\n> +\t\telse\n> +\t\t\tfirst = middle + 1;\n> +\t}\n> +\tassert(items >= cumulative_[first] && items <= cumulative_[last + 1]);\n\nYou should use ASSERT(), defined in internal/log.h, it will print a\nbacktrace.\n\n> +\tdouble frac = cumulative_[first + 1] == cumulative_[first] ? 0\n> +\t\t\t\t\t\t\t\t : (double)(items - cumulative_[first]) /\n> +\t\t\t\t\t\t\t\t\t (cumulative_[first + 1] - cumulative_[first]);\n\nPlease rework the code to avoid long lines. You can align the ? or :\nwith the = if it helps.\n\n> +\treturn first + frac;\n> +}\n> +\n> +/**\n> + * \\brief Calculate the mean between two quantiles\n> + * \\param[in] lowQuantile low Quantile\n> + * \\param[in] highQuantile high Quantile\n> + * \\return The average histogram bin value between the two quantiles\n> + */\n> +double Histogram::interQuantileMean(double lowQuantile, double highQuantile) const\n> +{\n> +\tassert(highQuantile > lowQuantile);\n> +\tdouble lowPoint = quantile(lowQuantile);\n> +\tdouble highPoint = quantile(highQuantile, (int)lowPoint);\n> +\tdouble sumBinFreq = 0, cumulFreq = 0;\n> +\tfor (double p_next = floor(lowPoint) + 1.0; p_next <= ceil(highPoint);\n> +\t lowPoint = p_next, p_next += 1.0) {\n> +\t\tint bin = floor(lowPoint);\n> +\t\tdouble freq = (cumulative_[bin + 1] - cumulative_[bin]) *\n> +\t\t\t (std::min(p_next, highPoint) - lowPoint);\n\nYou can align the * with the =\n\n> +\t\tsumBinFreq += bin * freq;\n> +\t\tcumulFreq += freq;\n> +\t}\n> +\t/* add 0.5 to give an average for bin mid-points */\n> +\treturn sumBinFreq / cumulFreq + 0.5;\n\nThe implementation needs more comments too, it's not trivial.\n\n> +}\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/libipa/histogram.h b/src/ipa/libipa/histogram.h\n> new file mode 100644\n> index 00000000..a610f675\n> --- /dev/null\n> +++ b/src/ipa/libipa/histogram.h\n> @@ -0,0 +1,62 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited\n> + *\n> + * histogram.h - histogram calculation interface\n> + */\n> +#ifndef __LIBCAMERA_IPA_LIBIPA_HISTOGRAM_H__\n> +#define __LIBCAMERA_IPA_LIBIPA_HISTOGRAM_H__\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +// A simple histogram class, for use in particular to find \"quantiles\" and\n> +// averages between \"quantiles\".\n\nWe use C-style comments, and avoid comments in header files. You can\ndrop this one.\n\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa {\n> +\n> +class Histogram\n> +{\n> +public:\n> +\ttemplate\n> +\t/**\n> +\t * \\brief Create a cumulative histogram with a bin number of intervals\n> +\t * \\param[in] histogram a reference to the histogram\n> +\t * \\param[in] num the number of bins\n> + \t*/\n\nThis belongs to the .cpp file, even for inline functions. There are\nplenty of examples in libcamera.\n\n> +\tHistogram(T *histogram, int num)\n\nI'd rename histogram to data.\n\nInstead of passing a pointer and a number of elements separately, you\nshould use a Span. And as the data shouldn't be modified, it should\nbe a Span.\n\n> +\t{\n> +\t\tassert(num);\n> +\t\tcumulative_.reserve(num + 1);\n> +\t\tcumulative_.push_back(0);\n> +\t\tfor (int i = 0; i < num; i++)\n> +\t\t\tcumulative_.push_back(cumulative_.back() +\n> +\t\t\t\t\t histogram[i]);\n\n\t\tfor (const T &value : data)\n\t\t\tcumulative_.push_back(cumulative_.back() + value);\n\n> +\t}\n\nAnd the constructor shouldn't be inline anyway, as it's not trivial.\n\n> +\n> +\t/**\n> +\t * \\brief getter for number of bins\n> +\t * \\return number of bins\n> +\t */\n\nThis comment, as well as the next one, also belong to the .cpp file.\n\n> +\tuint32_t bins() const { return cumulative_.size() - 1; }\n\nA size is better represented by a size_t than a uin32_t.\n\n> +\t/**\n> +\t * \\brief getter for number of values\n> +\t * \\return number of values\n> +\t */\n> +\tuint64_t total() const { return cumulative_[cumulative_.size() - 1]; }\n> +\tuint64_t cumulativeFreq(double bin) const;\n> +\tdouble quantile(double q, int first = -1, int last = -1) const;\n> +\tdouble interQuantileMean(double lowQuantile, double hiQuantile) const;\n> +\n> +private:\n> +\tstd::vector cumulative_;\n> +};\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_IPA_LIBIPA_HISTOGRAM_H__ */\n> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> index 585d02a3..53edeef9 100644\n> --- a/src/ipa/libipa/meson.build\n> +++ b/src/ipa/libipa/meson.build\n> @@ -2,10 +2,12 @@\n> \n> libipa_headers = files([\n> 'algorithm.h',\n> + 'histogram.h'\n\n4 spaces.\n\n> ])\n> \n> libipa_sources = files([\n> 'algorithm.cpp',\n> + 'histogram.cpp'\n> ])\n> \n> libipa_includes = include_directories('..')","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 E731DC32F0\n\tfor ;\n\tTue, 30 Mar 2021 03:18:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 172A568782;\n\tTue, 30 Mar 2021 05:18:06 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 74482602D7\n\tfor ;\n\tTue, 30 Mar 2021 05:18:04 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 626428B7;\n\tTue, 30 Mar 2021 05:18:03 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"hQzYlSVN\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1617074283;\n\tbh=I7XIJ/IOur+jo6RP5ZSQhKxTCi20vOPXhQYNE3mQs10=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=hQzYlSVNabzivTV+HuTdYcvj7TYa8xVgoLFE1cOuiMmziemH/nLOIg3DAORspefk7\n\tru8mmRw6uy36b6cHzrqNLb6pomFinOia+yJ7CvbYS9hEHnB1H+XTvYIiFVyQuHVEgW\n\tAttqHixuLWenL+U4AWuXUFPCH17lU3sl/prOhz9I=","Date":"Tue, 30 Mar 2021 06:17:18 +0300","From":"Laurent Pinchart ","To":"Jean-Michel Hautbois ","Message-ID":"","References":"<20210329191826.77817-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210329191826.77817-3-jeanmichel.hautbois@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210329191826.77817-3-jeanmichel.hautbois@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 2/5] ipa: ipu3: Add an histogram\n\tclass","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]