[{"id":17458,"web_url":"https://patchwork.libcamera.org/comment/17458/","msgid":"","date":"2021-06-08T01:09:03","subject":"Re: [libcamera-devel] [PATCH v5 1/4] libcamera: utils: Add helper\n\tclass for std::chrono::duration","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nThank you for the patch.\n\nOn Tue, May 25, 2021 at 12:42:38PM +0100, Naushir Patuck wrote:\n> A new utils::Duration class is defined to represent a\n> std::chrono::duration type with double precision nanosecond timebase.\n> Using a double minimises the loss of precision when converting timebases.\n> This helper class may be used by IPAs to represent variables such as frame\n> durations and exposure times.\n\nDo we really need double precision ? Have you noticed loss of precision\nin calculations that would be caused by usage of a 64-bit integer ? I'm\nfine keeping the double for now, but moving forward towards more\nwidespread usage of std::chrono in libcamera, and especially in its\npublic API, I'd prefer using an integer if possible.\n\n> An operator << overload is define to help with displaying\n\ns/define/defined/\n\n> utils::Duration value in stream objects. Currently, this will display\n> the duration value in microseconds.\n> \n> Signed-off-by: Naushir Patuck \n> Reviewed-by: David Plowman \n> Reviewed-by: Jacopo Mondi \n> ---\n> include/libcamera/internal/utils.h | 42 ++++++++++++++++++++++++++++++\n> src/libcamera/utils.cpp | 37 ++++++++++++++++++++++++++\n> 2 files changed, 79 insertions(+)\n> \n> diff --git a/include/libcamera/internal/utils.h b/include/libcamera/internal/utils.h\n> index 83dada7cc16c..f536f2267d70 100644\n> --- a/include/libcamera/internal/utils.h\n> +++ b/include/libcamera/internal/utils.h\n> @@ -316,8 +316,50 @@ auto enumerate(T (&iterable)[N]) -> details::enumerate_adapter\n> }\n> #endif\n> \n> +class Duration : public std::chrono::duration\n> +{\n> +\tusing BaseDuration = std::chrono::duration;\n> +\n> +public:\n> +\tDuration() = default;\n> +\n> +\ttemplate\n> +\tconstexpr Duration(const std::chrono::duration &d)\n> +\t\t: BaseDuration(d)\n> +\t{\n> +\t}\n> +\n> +\ttemplate\n> +\tdouble get() const\n> +\t{\n> +\t\tauto const c = std::chrono::duration_cast>(*this);\n> +\t\treturn c.count();\n> +\t}\n> +\n> +\texplicit constexpr operator bool() const\n> +\t{\n> +\t\treturn *this != BaseDuration::zero();\n> +\t}\n> +};\n> +\n> } /* namespace utils */\n> \n> +#ifndef __DOXYGEN__\n> +template\n> +static inline std::basic_ostream &operator<<(std::basic_ostream &os,\n> +\t\t\t\t\t\t\t const utils::Duration &d)\n> +{\n> +\tstd::basic_ostringstream s;\n> +\n> +\ts.flags(os.flags());\n> +\ts.imbue(os.getloc());\n> +\ts.setf(std::ios_base::fixed, std::ios_base::floatfield);\n> +\ts.precision(2);\n> +\ts << d.get() << \"us\";\n> +\treturn os << s.str();\n> +}\n\nIt would be nice to avoid inlining this. What do you think of the\nfollowing ?\n\ndiff --git a/include/libcamera/internal/utils.h b/include/libcamera/internal/utils.h\nindex f536f2267d70..15beb0f44172 100644\n--- a/include/libcamera/internal/utils.h\n+++ b/include/libcamera/internal/utils.h\n@@ -346,18 +346,8 @@ public:\n\n #ifndef __DOXYGEN__\n template\n-static inline std::basic_ostream &operator<<(std::basic_ostream &os,\n-\t\t\t\t\t\t\t const utils::Duration &d)\n-{\n-\tstd::basic_ostringstream s;\n-\n-\ts.flags(os.flags());\n-\ts.imbue(os.getloc());\n-\ts.setf(std::ios_base::fixed, std::ios_base::floatfield);\n-\ts.precision(2);\n-\ts << d.get() << \"us\";\n-\treturn os << s.str();\n-}\n+std::basic_ostream &operator<<(std::basic_ostream &os,\n+\t\t\t\t\t const utils::Duration &d);\n #endif\n\n } /* namespace libcamera */\ndiff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp\nindex 6624ff74cdc7..cdfc6e104b52 100644\n--- a/src/libcamera/utils.cpp\n+++ b/src/libcamera/utils.cpp\n@@ -545,4 +545,26 @@ std::string libcameraSourcePath()\n\n } /* namespace utils */\n\n+#ifndef __DOXYGEN__\n+template\n+std::basic_ostream &operator<<(std::basic_ostream &os,\n+\t\t\t\t\t const utils::Duration &d)\n+{\n+\tstd::basic_ostringstream s;\n+\n+\ts.flags(os.flags());\n+\ts.imbue(os.getloc());\n+\ts.setf(std::ios_base::fixed, std::ios_base::floatfield);\n+\ts.precision(2);\n+\ts << d.get() << \"us\";\n+\treturn os << s.str();\n+}\n+\n+template\n+std::basic_ostream> &\n+operator<< >(\n+\tstd::basic_ostream> &os,\n+\tconst utils::Duration &d);\n+#endif /* __DOXYGEN__ */\n+\n } /* namespace libcamera */\n\n\nOn a side node, it took me quite some time to debug linker errors. Turns\nout that\n\ntemplate<>\nstd::basic_ostream> &\noperator<< >(\n\tstd::basic_ostream> &os,\n\tconst utils::Duration &d);\n\nin utils.cpp doesn't instantiate the template specialization, while\n\ntemplate\nstd::basic_ostream> &\noperator<< >(\n\tstd::basic_ostream> &os,\n\tconst utils::Duration &d);\n\ndoes.\n\n> +#endif\n> +\n> } /* namespace libcamera */\n> \n> #endif /* __LIBCAMERA_INTERNAL_UTILS_H__ */\n> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp\n> index 826144d3c837..6624ff74cdc7 100644\n> --- a/src/libcamera/utils.cpp\n> +++ b/src/libcamera/utils.cpp\n> @@ -506,6 +506,43 @@ std::string libcameraSourcePath()\n> * loop, iterates over an indexed view of the \\a iterable\n> */\n> \n> +/**\n> + * \\class Duration\n> + * \\brief Helper class from std::chrono::duration that represents a time\n> + * duration in nanoseconds with double precision\n> + */\n> +\n> +/**\n> + * \\fn Duration::Duration(const std::chrono::duration &d)\n> + * \\brief Copy constructor from a \\a std::chrono::duration type\n\nIt's not a copy constructor, as d can be of a different type than\nDuration. How about\n\n * \\brief Construct a Duration by converting an arbitrary std::chrono::duration\n\n> + * \\param[in] d The std::chrono::duration object to convert from\n> + *\n> + * Constructs a \\a Duration object from a \\a std::chrono::duration object,\n> + * internally represented in double precision with nanoseconds ticks\n\nThis should probably updated accordingly, I find the the second part a\nbit misleading, it seems to imply that d is of double precision with\nnanoseconds ticks.\n\nReviewed-by: Laurent Pinchart \n\n> + */\n> +\n> +/**\n> + * \\fn Duration::get()\n> + * \\brief Retrieve the tick count, converted to the timebase provided by the\n> + * template argument Period of type \\a std::ratio\n> + *\n> + * A typical usage example is given below:\n> + *\n> + * \\code{.cpp}\n> + * utils::Duration d = 5s;\n> + * double d_in_ms = d.get();\n> + * \\endcode\n> + *\n> + * \\return The tick count of the Duration expressed in \\a Period\n> + */\n> +\n> +/**\n> + * \\fn Duration::operator bool()\n> + * \\brief Boolean operator to test if a \\a Duration holds a non-zero time value\n> + *\n> + * \\return True if \\a Duration is a non-zero time value, False otherwise\n> + */\n> +\n> } /* namespace utils */\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 9C8AFBD22E\n\tfor ;\n\tTue, 8 Jun 2021 01:09:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E1F316892D;\n\tTue, 8 Jun 2021 03:09:19 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 86A346891C\n\tfor ;\n\tTue, 8 Jun 2021 03:09:18 +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 01C6B4AD;\n\tTue, 8 Jun 2021 03:09:17 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"bzCDG82v\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1623114558;\n\tbh=KY8L/69ls1OdRBrRnxTwBwvB1g7XDB4dWtV5Fj5wO6k=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=bzCDG82vPOJi9YZ9mcyi8187L3SeXz0xa6YNzWniRJGOazsQaEZHzX2i/9uNUXH10\n\t5TSLoa2H5n4WFZkrFIHLXvOnrKKSTkRmGTR9hPTlEIoDb7GA5614Hibem5PMRuE9aS\n\t3cDSTW93uui1sdIyJ/v6DheLu9bJ0bDcT5fk4IUg=","Date":"Tue, 8 Jun 2021 04:09:03 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20210525114241.906280-1-naush@raspberrypi.com>\n\t<20210525114241.906280-2-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210525114241.906280-2-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v5 1/4] libcamera: utils: Add helper\n\tclass for std::chrono::duration","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17473,"web_url":"https://patchwork.libcamera.org/comment/17473/","msgid":"","date":"2021-06-08T09:33:42","subject":"Re: [libcamera-devel] [PATCH v5 1/4] libcamera: utils: Add helper\n\tclass for std::chrono::duration","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nThank you for your feedback.\n\nOn Tue, 8 Jun 2021 at 02:09, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> On Tue, May 25, 2021 at 12:42:38PM +0100, Naushir Patuck wrote:\n> > A new utils::Duration class is defined to represent a\n> > std::chrono::duration type with double precision nanosecond timebase.\n> > Using a double minimises the loss of precision when converting timebases.\n> > This helper class may be used by IPAs to represent variables such as\n> frame\n> > durations and exposure times.\n>\n> Do we really need double precision ? Have you noticed loss of precision\n> in calculations that would be caused by usage of a 64-bit integer ? I'm\n> fine keeping the double for now, but moving forward towards more\n> widespread usage of std::chrono in libcamera, and especially in its\n> public API, I'd prefer using an integer if possible.\n>\n\nI've used double precision as a catch all more than anything else here.\nI don't expect a precision problem with replacing it with int64 in the\nfuture.\n\n\n>\n> > An operator << overload is define to help with displaying\n>\n> s/define/defined/\n>\n\nAck\n\n\n\n> > utils::Duration value in stream objects. Currently, this will display\n> > the duration value in microseconds.\n> >\n> > Signed-off-by: Naushir Patuck \n> > Reviewed-by: David Plowman \n> > Reviewed-by: Jacopo Mondi \n> > ---\n> > include/libcamera/internal/utils.h | 42 ++++++++++++++++++++++++++++++\n> > src/libcamera/utils.cpp | 37 ++++++++++++++++++++++++++\n> > 2 files changed, 79 insertions(+)\n> >\n> > diff --git a/include/libcamera/internal/utils.h\n> b/include/libcamera/internal/utils.h\n> > index 83dada7cc16c..f536f2267d70 100644\n> > --- a/include/libcamera/internal/utils.h\n> > +++ b/include/libcamera/internal/utils.h\n> > @@ -316,8 +316,50 @@ auto enumerate(T (&iterable)[N]) ->\n> details::enumerate_adapter\n> > }\n> > #endif\n> >\n> > +class Duration : public std::chrono::duration\n> > +{\n> > + using BaseDuration = std::chrono::duration;\n> > +\n> > +public:\n> > + Duration() = default;\n> > +\n> > + template\n> > + constexpr Duration(const std::chrono::duration &d)\n> > + : BaseDuration(d)\n> > + {\n> > + }\n> > +\n> > + template\n> > + double get() const\n> > + {\n> > + auto const c =\n> std::chrono::duration_cast>(*this);\n> > + return c.count();\n> > + }\n> > +\n> > + explicit constexpr operator bool() const\n> > + {\n> > + return *this != BaseDuration::zero();\n> > + }\n> > +};\n> > +\n> > } /* namespace utils */\n> >\n> > +#ifndef __DOXYGEN__\n> > +template\n> > +static inline std::basic_ostream\n> &operator<<(std::basic_ostream &os,\n> > + const\n> utils::Duration &d)\n> > +{\n> > + std::basic_ostringstream s;\n> > +\n> > + s.flags(os.flags());\n> > + s.imbue(os.getloc());\n> > + s.setf(std::ios_base::fixed, std::ios_base::floatfield);\n> > + s.precision(2);\n> > + s << d.get() << \"us\";\n> > + return os << s.str();\n> > +}\n>\n> It would be nice to avoid inlining this. What do you think of the\n> following ?\n>\n> diff --git a/include/libcamera/internal/utils.h\n> b/include/libcamera/internal/utils.h\n> index f536f2267d70..15beb0f44172 100644\n> --- a/include/libcamera/internal/utils.h\n> +++ b/include/libcamera/internal/utils.h\n> @@ -346,18 +346,8 @@ public:\n>\n> #ifndef __DOXYGEN__\n> template\n> -static inline std::basic_ostream\n> &operator<<(std::basic_ostream &os,\n> - const\n> utils::Duration &d)\n> -{\n> - std::basic_ostringstream s;\n> -\n> - s.flags(os.flags());\n> - s.imbue(os.getloc());\n> - s.setf(std::ios_base::fixed, std::ios_base::floatfield);\n> - s.precision(2);\n> - s << d.get() << \"us\";\n> - return os << s.str();\n> -}\n> +std::basic_ostream &operator<<(std::basic_ostream Traits> &os,\n> + const utils::Duration &d);\n> #endif\n>\n> } /* namespace libcamera */\n> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp\n> index 6624ff74cdc7..cdfc6e104b52 100644\n> --- a/src/libcamera/utils.cpp\n> +++ b/src/libcamera/utils.cpp\n> @@ -545,4 +545,26 @@ std::string libcameraSourcePath()\n>\n> } /* namespace utils */\n>\n> +#ifndef __DOXYGEN__\n> +template\n> +std::basic_ostream &operator<<(std::basic_ostream Traits> &os,\n> + const utils::Duration &d)\n> +{\n> + std::basic_ostringstream s;\n> +\n> + s.flags(os.flags());\n> + s.imbue(os.getloc());\n> + s.setf(std::ios_base::fixed, std::ios_base::floatfield);\n> + s.precision(2);\n> + s << d.get() << \"us\";\n> + return os << s.str();\n> +}\n> +\n> +template\n> +std::basic_ostream> &\n> +operator<< >(\n> + std::basic_ostream> &os,\n> + const utils::Duration &d);\n> +#endif /* __DOXYGEN__ */\n> +\n> } /* namespace libcamera */\n>\n\nYes, I can replace the inline function with the above implementation in the\nnext revision.\n\n\n>\n>\n> On a side node, it took me quite some time to debug linker errors. Turns\n> out that\n>\n> template<>\n> std::basic_ostream> &\n> operator<< >(\n> std::basic_ostream> &os,\n> const utils::Duration &d);\n>\n> in utils.cpp doesn't instantiate the template specialization, while\n>\n> template\n> std::basic_ostream> &\n> operator<< >(\n> std::basic_ostream> &os,\n> const utils::Duration &d);\n>\n> does.\n>\n\nInterestingly, both the above forms work on my builds with gcc 9.3.0.\nI didn't even know that \"template\" without arrow brackets was valid!\n\n > +#endif\n\n> > +\n> > } /* namespace libcamera */\n> >\n> > #endif /* __LIBCAMERA_INTERNAL_UTILS_H__ */\n> > diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp\n> > index 826144d3c837..6624ff74cdc7 100644\n> > --- a/src/libcamera/utils.cpp\n> > +++ b/src/libcamera/utils.cpp\n> > @@ -506,6 +506,43 @@ std::string libcameraSourcePath()\n> > * loop, iterates over an indexed view of the \\a iterable\n> > */\n> >\n> > +/**\n> > + * \\class Duration\n> > + * \\brief Helper class from std::chrono::duration that represents a time\n> > + * duration in nanoseconds with double precision\n> > + */\n> > +\n> > +/**\n> > + * \\fn Duration::Duration(const std::chrono::duration &d)\n> > + * \\brief Copy constructor from a \\a std::chrono::duration type\n>\n> It's not a copy constructor, as d can be of a different type than\n> Duration. How about\n>\n> * \\brief Construct a Duration by converting an arbitrary\n> std::chrono::duration\n>\n\nAck\n\n\n>\n> > + * \\param[in] d The std::chrono::duration object to convert from\n> > + *\n> > + * Constructs a \\a Duration object from a \\a std::chrono::duration\n> object,\n> > + * internally represented in double precision with nanoseconds ticks\n>\n> This should probably updated accordingly, I find the the second part a\n> bit misleading, it seems to imply that d is of double precision with\n> nanoseconds ticks.\n>\n\nAck\n\n\n>\n> Reviewed-by: Laurent Pinchart \n>\n> > + */\n> > +\n> > +/**\n> > + * \\fn Duration::get()\n> > + * \\brief Retrieve the tick count, converted to the timebase provided\n> by the\n> > + * template argument Period of type \\a std::ratio\n> > + *\n> > + * A typical usage example is given below:\n> > + *\n> > + * \\code{.cpp}\n> > + * utils::Duration d = 5s;\n> > + * double d_in_ms = d.get();\n> > + * \\endcode\n> > + *\n> > + * \\return The tick count of the Duration expressed in \\a Period\n> > + */\n> > +\n> > +/**\n> > + * \\fn Duration::operator bool()\n> > + * \\brief Boolean operator to test if a \\a Duration holds a non-zero\n> time value\n> > + *\n> > + * \\return True if \\a Duration is a non-zero time value, False otherwise\n> > + */\n> > +\n> > } /* namespace utils */\n> >\n> > } /* namespace libcamera */\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 1E3D1BD22E\n\tfor ;\n\tTue, 8 Jun 2021 09:34:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6CF3B6892C;\n\tTue, 8 Jun 2021 11:34:00 +0200 (CEST)","from mail-lj1-x22d.google.com (mail-lj1-x22d.google.com\n\t[IPv6:2a00:1450:4864:20::22d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 24AED6029E\n\tfor ;\n\tTue, 8 Jun 2021 11:33:59 +0200 (CEST)","by mail-lj1-x22d.google.com with SMTP id e11so26092692ljn.13\n\tfor ;\n\tTue, 08 Jun 2021 02:33:59 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"jhIHOYP5\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=hMDtUpNDz83273b+ItSKuyQ22144eTHJeR452Um2xqA=;\n\tb=jhIHOYP5k0h6oXyuCYEurdZ7py+kmUkCRHYkZ3/NgfZgrnj8zkkDqpyu2vTlk5oCUf\n\tzM/A6En5PaNNsNXa7MiM6C2tt3Hmqnf+gYaoTXin7fgL/kssC881FrF58D+IrM+VcmiK\n\tu/q4vdiJEQFoev1h36P8fk63ZEiQ2xUid49jmhbhROIeCEQtM9zb/KjmeNaqJgWwX0kx\n\tlJZYXrC1O5yXVIbWYjocGMDO97c9CcYTQKYkQPaDMAl5sl4SssoDomMPe6no0XBvEPhg\n\tvCdF2yjDu33/ZUsRhzeZU3Tmj+0KcCVdxm5P5Asie1Oh8Xc+yweIh0h1ZmvKvPQY2vrI\n\twF5Q==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=hMDtUpNDz83273b+ItSKuyQ22144eTHJeR452Um2xqA=;\n\tb=M74vAFW5wgl2xOw06pjCtJo1P/hY11M9Nab3FQayFnwDxiogmsataIs+9gIx/H3alf\n\ty7SpAdfM44V9yenk/gsL3cLU80/9thDAb+G6SVKBP101Zj1K/XbyEhjDlymh3uiAGI8T\n\tQVV2KGk+p3YzVf18Q8mmjqpTq7VV6FWCzUQzIfXWyDmlIRFcvt4tVSYQVFMkGWC+XZHU\n\tm9ClCukIVde3hiXtX+Lk+xq0Uv4/pQ5dIVkwWkh4WoDJfsnx7FTchfeywX8Uj73jwvXu\n\tV6UwWWsfvYnaFzgFDD8bPV5XrGi2o5yGOUMtssGjkL1nBruPeRtnXEtSuzFR9In2bx6h\n\tWY9Q==","X-Gm-Message-State":"AOAM5332tCDcB+nqwqPEgk9Ps5owTAo/+i6UB5Hem0mjK088ArFpIQ9Q\n\tNVAIh4A3s/Oz6C6rZtw3jyQjBxJgnU1hKkZ+gYSIIBvypwU=","X-Google-Smtp-Source":"ABdhPJzLX6+2/5DDgB3bul2DI1LBvtwfb9xmasrQmrRdPSICqCqu1/dpURz+EKGHFiBWGqPs1H1e6J50p6rx7SI7078=","X-Received":"by 2002:a2e:9a4f:: with SMTP id\n\tk15mr9508584ljj.400.1623144838333; \n\tTue, 08 Jun 2021 02:33:58 -0700 (PDT)","MIME-Version":"1.0","References":"<20210525114241.906280-1-naush@raspberrypi.com>\n\t<20210525114241.906280-2-naush@raspberrypi.com>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Tue, 8 Jun 2021 10:33:42 +0100","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"000000000000821b3e05c43ddb50\"","Subject":"Re: [libcamera-devel] [PATCH v5 1/4] libcamera: utils: Add helper\n\tclass for std::chrono::duration","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17474,"web_url":"https://patchwork.libcamera.org/comment/17474/","msgid":"","date":"2021-06-08T09:42:38","subject":"Re: [libcamera-devel] [PATCH v5 1/4] libcamera: utils: Add helper\n\tclass for std::chrono::duration","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Tue, Jun 08, 2021 at 10:33:42AM +0100, Naushir Patuck wrote:\n> On Tue, 8 Jun 2021 at 02:09, Laurent Pinchart wrote:\n> > On Tue, May 25, 2021 at 12:42:38PM +0100, Naushir Patuck wrote:\n> > > A new utils::Duration class is defined to represent a\n> > > std::chrono::duration type with double precision nanosecond timebase.\n> > > Using a double minimises the loss of precision when converting timebases.\n> > > This helper class may be used by IPAs to represent variables such as\n> > frame\n> > > durations and exposure times.\n> >\n> > Do we really need double precision ? Have you noticed loss of precision\n> > in calculations that would be caused by usage of a 64-bit integer ? I'm\n> > fine keeping the double for now, but moving forward towards more\n> > widespread usage of std::chrono in libcamera, and especially in its\n> > public API, I'd prefer using an integer if possible.\n> \n> I've used double precision as a catch all more than anything else here.\n> I don't expect a precision problem with replacing it with int64 in the\n> future.\n> \n> > > An operator << overload is define to help with displaying\n> >\n> > s/define/defined/\n> \n> Ack\n> \n> > > the duration value in microseconds.\n> > >\n> > > Signed-off-by: Naushir Patuck \n> > > Reviewed-by: David Plowman \n> > > Reviewed-by: Jacopo Mondi \n> > > ---\n> > > include/libcamera/internal/utils.h | 42 ++++++++++++++++++++++++++++++\n> > > src/libcamera/utils.cpp | 37 ++++++++++++++++++++++++++\n> > > 2 files changed, 79 insertions(+)\n> > >\n> > > diff --git a/include/libcamera/internal/utils.h b/include/libcamera/internal/utils.h\n> > > index 83dada7cc16c..f536f2267d70 100644\n> > > --- a/include/libcamera/internal/utils.h\n> > > +++ b/include/libcamera/internal/utils.h\n> > > @@ -316,8 +316,50 @@ auto enumerate(T (&iterable)[N]) -> details::enumerate_adapter\n> > > }\n> > > #endif\n> > >\n> > > +class Duration : public std::chrono::duration\n> > > +{\n> > > + using BaseDuration = std::chrono::duration;\n> > > +\n> > > +public:\n> > > + Duration() = default;\n> > > +\n> > > + template\n> > > + constexpr Duration(const std::chrono::duration &d)\n> > > + : BaseDuration(d)\n> > > + {\n> > > + }\n> > > +\n> > > + template\n> > > + double get() const\n> > > + {\n> > > + auto const c = std::chrono::duration_cast>(*this);\n> > > + return c.count();\n> > > + }\n> > > +\n> > > + explicit constexpr operator bool() const\n> > > + {\n> > > + return *this != BaseDuration::zero();\n> > > + }\n> > > +};\n> > > +\n> > > } /* namespace utils */\n> > >\n> > > +#ifndef __DOXYGEN__\n> > > +template\n> > > +static inline std::basic_ostream &operator<<(std::basic_ostream &os,\n> > > + const utils::Duration &d)\n> > > +{\n> > > + std::basic_ostringstream s;\n> > > +\n> > > + s.flags(os.flags());\n> > > + s.imbue(os.getloc());\n> > > + s.setf(std::ios_base::fixed, std::ios_base::floatfield);\n> > > + s.precision(2);\n> > > + s << d.get() << \"us\";\n> > > + return os << s.str();\n> > > +}\n> >\n> > It would be nice to avoid inlining this. What do you think of the\n> > following ?\n> >\n> > diff --git a/include/libcamera/internal/utils.h\n> > b/include/libcamera/internal/utils.h\n> > index f536f2267d70..15beb0f44172 100644\n> > --- a/include/libcamera/internal/utils.h\n> > +++ b/include/libcamera/internal/utils.h\n> > @@ -346,18 +346,8 @@ public:\n> >\n> > #ifndef __DOXYGEN__\n> > template\n> > -static inline std::basic_ostream\n> > &operator<<(std::basic_ostream &os,\n> > - const\n> > utils::Duration &d)\n> > -{\n> > - std::basic_ostringstream s;\n> > -\n> > - s.flags(os.flags());\n> > - s.imbue(os.getloc());\n> > - s.setf(std::ios_base::fixed, std::ios_base::floatfield);\n> > - s.precision(2);\n> > - s << d.get() << \"us\";\n> > - return os << s.str();\n> > -}\n> > +std::basic_ostream &operator<<(std::basic_ostream > Traits> &os,\n> > + const utils::Duration &d);\n> > #endif\n> >\n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp\n> > index 6624ff74cdc7..cdfc6e104b52 100644\n> > --- a/src/libcamera/utils.cpp\n> > +++ b/src/libcamera/utils.cpp\n> > @@ -545,4 +545,26 @@ std::string libcameraSourcePath()\n> >\n> > } /* namespace utils */\n> >\n> > +#ifndef __DOXYGEN__\n> > +template\n> > +std::basic_ostream &operator<<(std::basic_ostream > Traits> &os,\n> > + const utils::Duration &d)\n> > +{\n> > + std::basic_ostringstream s;\n> > +\n> > + s.flags(os.flags());\n> > + s.imbue(os.getloc());\n> > + s.setf(std::ios_base::fixed, std::ios_base::floatfield);\n> > + s.precision(2);\n> > + s << d.get() << \"us\";\n> > + return os << s.str();\n> > +}\n> > +\n> > +template\n> > +std::basic_ostream> &\n> > +operator<< >(\n> > + std::basic_ostream> &os,\n> > + const utils::Duration &d);\n> > +#endif /* __DOXYGEN__ */\n> > +\n> > } /* namespace libcamera */\n> >\n> \n> Yes, I can replace the inline function with the above implementation in the\n> next revision.\n\nThanks. I'll then merge it right away.\n\n> > On a side node, it took me quite some time to debug linker errors. Turns\n> > out that\n> >\n> > template<>\n> > std::basic_ostream> &\n> > operator<< >(\n> > std::basic_ostream> &os,\n> > const utils::Duration &d);\n> >\n> > in utils.cpp doesn't instantiate the template specialization, while\n> >\n> > template\n> > std::basic_ostream> &\n> > operator<< >(\n> > std::basic_ostream> &os,\n> > const utils::Duration &d);\n> >\n> > does.\n> \n> Interestingly, both the above forms work on my builds with gcc 9.3.0.\n> I didn't even know that \"template\" without arrow brackets was valid!\n\nNeither did I, and I've refrained from researching this :-) I was\ntesting with gcc 11 and clang 11.\n\n> > +#endif\n> \n> > > +\n> > > } /* namespace libcamera */\n> > >\n> > > #endif /* __LIBCAMERA_INTERNAL_UTILS_H__ */\n> > > diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp\n> > > index 826144d3c837..6624ff74cdc7 100644\n> > > --- a/src/libcamera/utils.cpp\n> > > +++ b/src/libcamera/utils.cpp\n> > > @@ -506,6 +506,43 @@ std::string libcameraSourcePath()\n> > > * loop, iterates over an indexed view of the \\a iterable\n> > > */\n> > >\n> > > +/**\n> > > + * \\class Duration\n> > > + * \\brief Helper class from std::chrono::duration that represents a time\n> > > + * duration in nanoseconds with double precision\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn Duration::Duration(const std::chrono::duration &d)\n> > > + * \\brief Copy constructor from a \\a std::chrono::duration type\n> >\n> > It's not a copy constructor, as d can be of a different type than\n> > Duration. How about\n> >\n> > * \\brief Construct a Duration by converting an arbitrary\n> > std::chrono::duration\n> \n> Ack\n> \n> > > + * \\param[in] d The std::chrono::duration object to convert from\n> > > + *\n> > > + * Constructs a \\a Duration object from a \\a std::chrono::duration object,\n> > > + * internally represented in double precision with nanoseconds ticks\n> >\n> > This should probably updated accordingly, I find the the second part a\n> > bit misleading, it seems to imply that d is of double precision with\n> > nanoseconds ticks.\n> \n> Ack\n> \n> > Reviewed-by: Laurent Pinchart \n> >\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn Duration::get()\n> > > + * \\brief Retrieve the tick count, converted to the timebase provided by the\n> > > + * template argument Period of type \\a std::ratio\n> > > + *\n> > > + * A typical usage example is given below:\n> > > + *\n> > > + * \\code{.cpp}\n> > > + * utils::Duration d = 5s;\n> > > + * double d_in_ms = d.get();\n> > > + * \\endcode\n> > > + *\n> > > + * \\return The tick count of the Duration expressed in \\a Period\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn Duration::operator bool()\n> > > + * \\brief Boolean operator to test if a \\a Duration holds a non-zero time value\n> > > + *\n> > > + * \\return True if \\a Duration is a non-zero time value, False otherwise\n> > > + */\n> > > +\n> > > } /* namespace utils */\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 93AA8C3206\n\tfor ;\n\tTue, 8 Jun 2021 09:42:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EE8CA6892F;\n\tTue, 8 Jun 2021 11:42:54 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C9FD76029F\n\tfor ;\n\tTue, 8 Jun 2021 11:42:53 +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 411153E6;\n\tTue, 8 Jun 2021 11:42:53 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"DXe/QPAk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1623145373;\n\tbh=mZTODatqQ9xaE8fTNZYK7a7Jf3bQ75bUMnfj+8Yf0fU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=DXe/QPAkvcCxMwsUiHsMpNJJKxZsNV23VG3NonFjPvHVRfAnyH7EOqlnISdWPl4FW\n\tY5kfmoQwh45vZ6rAuL70NhmCrj7EUQj4Gx5c8yX9GK1HX3ZSpOeAD3Mj90+ZdGRduk\n\tegFT1/3s0Y30Tyz7yUkOTD+EbiNCAZpNMaDU9HjU=","Date":"Tue, 8 Jun 2021 12:42:38 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20210525114241.906280-1-naush@raspberrypi.com>\n\t<20210525114241.906280-2-naush@raspberrypi.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v5 1/4] libcamera: utils: Add helper\n\tclass for std::chrono::duration","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]