[{"id":17080,"web_url":"https://patchwork.libcamera.org/comment/17080/","msgid":"","date":"2021-05-21T10:20:31","subject":"Re: [libcamera-devel] [PATCH v3 1/4] libcamera: utils: Add helper\n\tclass for std::chrono::duration","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Naush\n\nThanks for the updated patch!\n\nOn Fri, 21 May 2021 at 09:05, Naushir Patuck wrote:\n>\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>\n> An operator << overload is define to help with displaying\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> ---\n> include/libcamera/internal/utils.h | 42 ++++++++++++++++++++++++++++++\n> src/libcamera/utils.cpp | 36 +++++++++++++++++++++++++\n> 2 files changed, 78 insertions(+)\n>\n> diff --git a/include/libcamera/internal/utils.h b/include/libcamera/internal/utils.h\n> index 83dada7cc16c..a377d4ea07ab 100644\n> --- a/include/libcamera/internal/utils.h\n> +++ b/include/libcamera/internal/utils.h\n> @@ -316,6 +316,48 @@ auto enumerate(T (&iterable)[N]) -> details::enumerate_adapter\n> }\n> #endif\n>\n> +using BaseDuration = std::chrono::duration;\n> +\n> +class Duration : public BaseDuration\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> +#ifndef __DOXYGEN__\n> +template\n> +static inline std::basic_ostream &operator<<(std::basic_ostream &os,\n> + const 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> +#endif\n\nOne small thing I wonder about is that many classes have a toString\nmethod. Do we want one here? It could even take an optional argument\nto allow you to format the output differently... but I think that's\noverkill and best left for now!\n\n> +\n> } /* namespace utils */\n>\n> } /* namespace libcamera */\n> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp\n> index 826144d3c837..3131aa2d6666 100644\n> --- a/src/libcamera/utils.cpp\n> +++ b/src/libcamera/utils.cpp\n> @@ -506,6 +506,42 @@ std::string libcameraSourcePath()\n> * loop, iterates over an indexed view of the \\a iterable\n> */\n>\n> +/**\n> + * \\class Duration\n> + * \\brief Helper for std::chrono::duration types. Derived from \\a BaseDuration\n> + */\n> +\n> +/**\n> + * \\fn Duration::Duration(const std::chrono::duration &d)\n> + * \\brief Copy constructor from a \\a std::chrono::duration type.\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> + * converting the representation to \\a BaseDuration type.\n> + */\n> +\n> +/**\n> + * \\fn Duration::get()\n> + * \\brief Retrieve the tick count, coverted to the timebase provided by the\n\ns/coverted/converted/\n\nWith this tiny fix:\n\nReviewed-by: David Plowman \n\nThanks!\nDavid\n\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 Tick count\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 */\n> --\n> 2.25.1\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 6FCACC31FF\n\tfor ;\n\tFri, 21 May 2021 10:20:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D47C46891D;\n\tFri, 21 May 2021 12:20:44 +0200 (CEST)","from mail-wr1-x42f.google.com (mail-wr1-x42f.google.com\n\t[IPv6:2a00:1450:4864:20::42f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 780AB68911\n\tfor ;\n\tFri, 21 May 2021 12:20:43 +0200 (CEST)","by mail-wr1-x42f.google.com with SMTP id x8so20509732wrq.9\n\tfor ;\n\tFri, 21 May 2021 03:20:43 -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=\"gtvDQB4M\"; 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=Optp6ATmXv0dFvMuenSwjPacBbmk6RWFz5qaNbjBdfM=;\n\tb=gtvDQB4MNlcWtwnEMgSgOFnpBF0upIRVbkyqqhLlHA5yZAVs5noMyf+kOwxsmGYCNA\n\tdMgzcr4Z0BTGwLT9K2SYu2f9rTsJMbnA6g58VaVbNEC9OQFDU5G8ANWVg2FtdCT6WrwB\n\twNadJPBP9P1hb+NFtBGn+svfa9cHA68O3znh+Kz52frwf8g0dhDC2mwR2Rj4Ev9NZbdJ\n\t2+qyQm2Zc2HgOtDlbkrtLu1QibF8Ov54kBhBIu7Mf3Z6xPtIjhVoCciJxwy1ttJL4JcS\n\t0hxVYX1YxM+NBIBLcEgzXxIdgP8AgIDqsnDzN5wL7MtnsKekq1Q6GV3PE8ZuD+h0jZ+b\n\tuVLw==","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=Optp6ATmXv0dFvMuenSwjPacBbmk6RWFz5qaNbjBdfM=;\n\tb=aFkFPDhYM6IcwsTe4oE3kriSCkkduVGT06Y5cxo9mwKAdDe/RSSTuAKYTkrsEO1mCU\n\tPCY9bBmtLIfnq0LZeS82gMHgCpRhBG+4KdaKbdbgnQenpULMLvEw703kEtqUN3JepTWc\n\tdbHcL6h9ZdiFOnca2oASH5iNO4UMTmuB1jhrh/XbvZ7lzU7yN5tkAPEIk0RFN1kJNMT3\n\tunEEMIc3b7UAisvmZGguaEeehp5ADLRs6bHvPlJZc1POkJx3I9s4NdtgJhpFXY7G+4zD\n\ty/31XR97Enp9Dp6FEGs7mEdQQhHqj+U55jx9fQ51cC8KkR75+YmANL2IVv9BRT/iamih\n\tjSTw==","X-Gm-Message-State":"AOAM532Gcx1l+SpVVplRgINfLAONw+YwcvThfrMmwGpFIj0Yndu3NGGq\n\tg0/EUxQW6vFQcRbGtDazRnAxdXM489OPdADWXu2HBQ==","X-Google-Smtp-Source":"ABdhPJxiRPThzO1YPu+7jYh+nrp1gkOsy+KGJZZfRfkh1tZdFgcByHMgpfsvxK8R1qHPcJCtyF4VjkRbaD9NIJpHur0=","X-Received":"by 2002:a5d:408f:: with SMTP id o15mr8464506wrp.89.1621592443152;\n\tFri, 21 May 2021 03:20:43 -0700 (PDT)","MIME-Version":"1.0","References":"<20210521080530.37591-1-naush@raspberrypi.com>\n\t<20210521080530.37591-2-naush@raspberrypi.com>","In-Reply-To":"<20210521080530.37591-2-naush@raspberrypi.com>","From":"David Plowman ","Date":"Fri, 21 May 2021 11:20:31 +0100","Message-ID":"","To":"Naushir Patuck ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v3 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":17083,"web_url":"https://patchwork.libcamera.org/comment/17083/","msgid":"","date":"2021-05-21T10:53:50","subject":"Re: [libcamera-devel] [PATCH v3 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 David,\n\nThank you for your feedback.\n\nOn Fri, 21 May 2021 at 11:20, David Plowman \nwrote:\n\n> Hi Naush\n>\n> Thanks for the updated patch!\n>\n> On Fri, 21 May 2021 at 09:05, Naushir Patuck \n> wrote:\n> >\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> > An operator << overload is define to help with displaying\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> > ---\n> > include/libcamera/internal/utils.h | 42 ++++++++++++++++++++++++++++++\n> > src/libcamera/utils.cpp | 36 +++++++++++++++++++++++++\n> > 2 files changed, 78 insertions(+)\n> >\n> > diff --git a/include/libcamera/internal/utils.h\n> b/include/libcamera/internal/utils.h\n> > index 83dada7cc16c..a377d4ea07ab 100644\n> > --- a/include/libcamera/internal/utils.h\n> > +++ b/include/libcamera/internal/utils.h\n> > @@ -316,6 +316,48 @@ auto enumerate(T (&iterable)[N]) ->\n> details::enumerate_adapter\n> > }\n> > #endif\n> >\n> > +using BaseDuration = std::chrono::duration;\n> > +\n> > +class Duration : public BaseDuration\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> > +#ifndef __DOXYGEN__\n> > +template\n> > +static inline std::basic_ostream\n> &operator<<(std::basic_ostream &os,\n> > + const\n> 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> > +#endif\n>\n> One small thing I wonder about is that many classes have a toString\n> method. Do we want one here? It could even take an optional argument\n> to allow you to format the output differently... but I think that's\n> overkill and best left for now!\n>\n\nDid you mean that we should do something like:\n\nLOG(IPA, Debug) << \"Exposure time is \" << exposure.toString();\n\nover\n\nLOG(IPA, Debug) << \"Exposure time is \" << exposure;\n\nI chose the later way with the overload <<() operator as that is more\nsimilar\nto what C++20 will eventually allow with std::chrono::duration formatting.\nWhat do others think?\n\nRegards,\nNaush\n\n\n\n>\n> > +\n> > } /* namespace utils */\n> >\n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp\n> > index 826144d3c837..3131aa2d6666 100644\n> > --- a/src/libcamera/utils.cpp\n> > +++ b/src/libcamera/utils.cpp\n> > @@ -506,6 +506,42 @@ std::string libcameraSourcePath()\n> > * loop, iterates over an indexed view of the \\a iterable\n> > */\n> >\n> > +/**\n> > + * \\class Duration\n> > + * \\brief Helper for std::chrono::duration types. Derived from \\a\n> BaseDuration\n> > + */\n> > +\n> > +/**\n> > + * \\fn Duration::Duration(const std::chrono::duration &d)\n> > + * \\brief Copy constructor from a \\a std::chrono::duration type.\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> > + * converting the representation to \\a BaseDuration type.\n> > + */\n> > +\n> > +/**\n> > + * \\fn Duration::get()\n> > + * \\brief Retrieve the tick count, coverted to the timebase provided by\n> the\n>\n> s/coverted/converted/\n>\n> With this tiny fix:\n>\n> Reviewed-by: David Plowman \n>\n> Thanks!\n> David\n>\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 Tick count\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\n> otherwise.\n> > + */\n> > +\n> > } /* namespace utils */\n> >\n> > } /* namespace libcamera */\n> > --\n> > 2.25.1\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 00B42C3200\n\tfor ;\n\tFri, 21 May 2021 10:54:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 707B36891D;\n\tFri, 21 May 2021 12:54:08 +0200 (CEST)","from mail-lj1-x236.google.com (mail-lj1-x236.google.com\n\t[IPv6:2a00:1450:4864:20::236])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C940E68911\n\tfor ;\n\tFri, 21 May 2021 12:54:06 +0200 (CEST)","by mail-lj1-x236.google.com with SMTP id p20so23423674ljj.8\n\tfor ;\n\tFri, 21 May 2021 03:54:06 -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=\"Bn4Qsroq\"; 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=gRW6prWtoSQ/icc3PP2EW5JdCqM6U5csUu8SlhtVjRs=;\n\tb=Bn4QsroqrrEgE4ghPuR93yDypSZsuaAFXVVE8D41prT3Jcy4v4TjbcCjWSiGOsKrwz\n\t3vM701R+QRYaJB4bqe6kSIT8LlnTLCWhROa5Vm6AcpP2ANd61CtqWX5WCLjIkMZpZpFH\n\tQFy07CCkLyssHeHETToS8P+9Tn2FVv7XmFZEr0W1pH/KF4EDF2G3ETc7rcU+dFDWY6t/\n\t0i/3WoqYOQ5ur94k6FwEAUe5IOsFnwQyZCqcumZ2NyAgbm5bW3HZQ5p0cjJeh94Pe5Us\n\tSXekCCSaA+3NVzy/2aKbFeMiRyfV5avpMMlSjX5ZiLd/nfS9yZpXn98s/E1IFrIAdupp\n\tBQdg==","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=gRW6prWtoSQ/icc3PP2EW5JdCqM6U5csUu8SlhtVjRs=;\n\tb=j7rFYAwmk8NUbxeqpsxbSMvpZG9Ie3tmFb7AO96MnlIu1Um8zNjvDziZS+jPPdYmeg\n\t8uj+liCFP0Fb1nCiVi9no0WdSZ0akfLBU/FnO5z+PkaQ7ErnJCGWl24zLQUAQ7hvfe7A\n\tVzcVvbO6a5GlMDjswpg6BhbiZR0RnOM9sV0jbtJ28Ao+xcpdj4I6uAx5EbyM6IHZ24yH\n\tsP3Zbf+VVuPFSjVKsE8/1ss6tR0ohgNTOjz7hT7dcqP9vmAFimyNUvy9VfcuV/3cz3h8\n\tHWPziEAxopjrnqy70FLLj+dlsSepKXk/NRKXzBJ9/YCWiUGu/zukfTxAn+L72DGrbK9y\n\tW+Ng==","X-Gm-Message-State":"AOAM5330DQrIWmmMKim4eQnpUdk7tXAleKNnxNnJL72XHR3d8ug/3V7p\n\tTI+sP+qYX/tlNYpxIXEYnyGBBsNz3E2CKFMcQleA1Q==","X-Google-Smtp-Source":"ABdhPJyguqfPrmEYZEiQnE+xXNRGjUdc5DYnFHbQ26Ls484VGEl/K/YI/F/AjiKQ07OFsI7n3HRxqdOK9uNM6ePCvEQ=","X-Received":"by 2002:a2e:9896:: with SMTP id\n\tb22mr6401721ljj.329.1621594446251; \n\tFri, 21 May 2021 03:54:06 -0700 (PDT)","MIME-Version":"1.0","References":"<20210521080530.37591-1-naush@raspberrypi.com>\n\t<20210521080530.37591-2-naush@raspberrypi.com>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Fri, 21 May 2021 11:53:50 +0100","Message-ID":"","To":"David Plowman ","Content-Type":"multipart/alternative; boundary=\"000000000000f0526f05c2d4e07f\"","Subject":"Re: [libcamera-devel] [PATCH v3 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":17084,"web_url":"https://patchwork.libcamera.org/comment/17084/","msgid":"<646e8fb1-9dcf-f352-7a99-3303fb8816e0@ideasonboard.com>","date":"2021-05-21T11:14:35","subject":"Re: [libcamera-devel] [PATCH v3 1/4] libcamera: utils: Add helper\n\tclass for std::chrono::duration","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Naush, David,\n\nOn 21/05/2021 11:53, Naushir Patuck wrote:\n> Hi David,\n> \n> Thank you for your feedback.\n> \n> On Fri, 21 May 2021 at 11:20, David Plowman\n> >\n> wrote:\n> \n> Hi Naush\n> \n> Thanks for the updated patch!\n> \n> On Fri, 21 May 2021 at 09:05, Naushir Patuck > wrote:\n> >\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\n> timebases.\n> > This helper class may be used by IPAs to represent variables such\n> as frame\n> > durations and exposure times.\n> >\n> > An operator << overload is define to help with displaying\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> > ---\n> >  include/libcamera/internal/utils.h | 42\n> ++++++++++++++++++++++++++++++\n> >  src/libcamera/utils.cpp            | 36 +++++++++++++++++++++++++\n> >  2 files changed, 78 insertions(+)\n> >\n> > diff --git a/include/libcamera/internal/utils.h\n> b/include/libcamera/internal/utils.h\n> > index 83dada7cc16c..a377d4ea07ab 100644\n> > --- a/include/libcamera/internal/utils.h\n> > +++ b/include/libcamera/internal/utils.h\n> > @@ -316,6 +316,48 @@ auto enumerate(T (&iterable)[N]) ->\n> details::enumerate_adapter\n> >  }\n> >  #endif\n> >\n> > +using BaseDuration = std::chrono::duration;\n> > +\n> > +class Duration : public BaseDuration\n> > +{\n> > +public:\n> > +       Duration() = default;\n> > +\n> > +       template\n> > +       constexpr Duration(const std::chrono::duration Period> &d)\n> > +               : BaseDuration(d)\n> > +       {\n> > +       }\n> > +\n> > +       template\n> > +       double get() const\n> > +       {\n> > +               auto const c =\n> std::chrono::duration_cast Period>>(*this);\n> > +               return c.count();\n> > +       }\n> > +\n> > +       explicit constexpr operator bool() const\n> > +       {\n> > +               return *this != BaseDuration::zero();\n> > +       }\n> > +};\n> > +\n> > +#ifndef __DOXYGEN__\n> > +template\n> > +static inline std::basic_ostream\n> &operator<<(std::basic_ostream &os,\n> > +                                                           const\n> 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> > +#endif\n> \n> One small thing I wonder about is that many classes have a toString\n> method. Do we want one here? It could even take an optional argument\n> to allow you to format the output differently... but I think that's\n> overkill and best left for now!\n> \n> \n> Did you mean that we should do something like:\n> \n> LOG(IPA, Debug) << \"Exposure time is \" << exposure.toString();\n> \n> over\n> \n> LOG(IPA, Debug) << \"Exposure time is \" << exposure;\n> \n> I chose the later way with the overload <<() operator as that is more\n> similar\n> to what C++20 will eventually allow with std::chrono::duration formatting.\n> What do others think?\n\nI think the main reason we've used toString() is so that we don't have\nto override operator<< outside of the libcamera namespace, so we've been\nmore explicit with the toString().\n\nI don't mind the override or the explicit, but I think that's the reasoning.\n\nWould it be cleaner to implement a toString() which doesn't have to\ntrack the outputstream state, using a local stringstream perhaps and as\nthat returns a string, the override operator<< can simply return that\npre-prepared string?\n\n\n\n> \n> Regards,\n> Naush\n> \n>  \n> \n> \n> > +\n> >  } /* namespace utils */\n> >\n> >  } /* namespace libcamera */\n> > diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp\n> > index 826144d3c837..3131aa2d6666 100644\n> > --- a/src/libcamera/utils.cpp\n> > +++ b/src/libcamera/utils.cpp\n> > @@ -506,6 +506,42 @@ std::string libcameraSourcePath()\n> >   * loop, iterates over an indexed view of the \\a iterable\n> >   */\n> >\n> > +/**\n> > + * \\class Duration\n> > + * \\brief Helper for std::chrono::duration types. Derived from \\a\n> BaseDuration\n> > + */\n> > +\n> > +/**\n> > + * \\fn Duration::Duration(const std::chrono::duration Period> &d)\n> > + * \\brief Copy constructor from a \\a std::chrono::duration type.\n> > + * \\param[in] d The std::chrono::duration object to convert from.\n> > + *\n> > + * Constructs a \\a Duration object from a \\a\n> std::chrono::duration object,\n> > + * converting the representation to \\a BaseDuration type.\n> > + */\n> > +\n> > +/**\n> > + * \\fn Duration::get()\n> > + * \\brief Retrieve the tick count, coverted to the timebase\n> provided by the\n> \n> s/coverted/converted/\n> \n> With this tiny fix:\n> \n> Reviewed-by: David Plowman >\n> \n> Thanks!\n> David\n> \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 Tick count\n> > + */\n> > +\n> > +/**\n> > + * \\fn Duration::operator bool()\n> > + * \\brief Boolean operator to test if a \\a Duration holds a\n> non-zero time value.\n> > + *\n> > + * \\return True if \\a Duration is a non-zero time value, False\n> otherwise.\n> > + */\n> > +\n> >  } /* namespace utils */\n> >\n> >  } /* namespace libcamera */\n> > --\n> > 2.25.1\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 119FCC31FF\n\tfor ;\n\tFri, 21 May 2021 11:14:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 49AC56891A;\n\tFri, 21 May 2021 13:14:39 +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 05DF168911\n\tfor ;\n\tFri, 21 May 2021 13:14:38 +0200 (CEST)","from [192.168.0.20]\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 6F9A48D8;\n\tFri, 21 May 2021 13:14:37 +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=\"YaQiXLC6\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1621595677;\n\tbh=G/qgXVh3w8fviW+KvbzF5rJU5epfYBcv1eG9z+OmO3Y=;\n\th=Reply-To:To:Cc:References:From:Subject:Date:In-Reply-To:From;\n\tb=YaQiXLC6PwZ1byKtYQlNsGDoqjTvxYNokY2UT95CoUJPYnahelH576MkeXxJ7KOtQ\n\tOidXDo1stLbltqD00o4L9tJs1CfnGkYFmyA6TfGS86IidxcaqAxyPbtMwMGTTqc391\n\t7CSkNwhnSzqtGTqveEURvziWe5i0mSD/8nNXVYlc=","To":"Naushir Patuck ,\n\tDavid Plowman ","References":"<20210521080530.37591-1-naush@raspberrypi.com>\n\t<20210521080530.37591-2-naush@raspberrypi.com>\n\t\n\t","From":"Kieran Bingham ","Organization":"Ideas on Board","Message-ID":"<646e8fb1-9dcf-f352-7a99-3303fb8816e0@ideasonboard.com>","Date":"Fri, 21 May 2021 12:14:35 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.8.1","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v3 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","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17091,"web_url":"https://patchwork.libcamera.org/comment/17091/","msgid":"","date":"2021-05-21T11:44:26","subject":"Re: [libcamera-devel] [PATCH v3 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 Kieran,\n\nOn Fri, 21 May 2021 at 12:14, Kieran Bingham <\nkieran.bingham@ideasonboard.com> wrote:\n\n> Hi Naush, David,\n>\n> On 21/05/2021 11:53, Naushir Patuck wrote:\n> > Hi David,\n> >\n> > Thank you for your feedback.\n> >\n> > On Fri, 21 May 2021 at 11:20, David Plowman\n> > >\n> > wrote:\n> >\n> > Hi Naush\n> >\n> > Thanks for the updated patch!\n> >\n> > On Fri, 21 May 2021 at 09:05, Naushir Patuck > > wrote:\n> > >\n> > > A new utils::Duration class is defined to represent a\n> > > std::chrono::duration type with double precision nanosecond\n> timebase.\n> > > Using a double minimises the loss of precision when converting\n> > timebases.\n> > > This helper class may be used by IPAs to represent variables such\n> > as frame\n> > > durations and exposure times.\n> > >\n> > > An operator << overload is define to help with displaying\n> > > utils::Duration value in stream objects. Currently, this will\n> display\n> > > the duration value in microseconds.\n> > >\n> > > Signed-off-by: Naushir Patuck > >\n> > > ---\n> > > include/libcamera/internal/utils.h | 42\n> > ++++++++++++++++++++++++++++++\n> > > src/libcamera/utils.cpp | 36 +++++++++++++++++++++++++\n> > > 2 files changed, 78 insertions(+)\n> > >\n> > > diff --git a/include/libcamera/internal/utils.h\n> > b/include/libcamera/internal/utils.h\n> > > index 83dada7cc16c..a377d4ea07ab 100644\n> > > --- a/include/libcamera/internal/utils.h\n> > > +++ b/include/libcamera/internal/utils.h\n> > > @@ -316,6 +316,48 @@ auto enumerate(T (&iterable)[N]) ->\n> > details::enumerate_adapter\n> > > }\n> > > #endif\n> > >\n> > > +using BaseDuration = std::chrono::duration;\n> > > +\n> > > +class Duration : public BaseDuration\n> > > +{\n> > > +public:\n> > > + Duration() = default;\n> > > +\n> > > + template\n> > > + constexpr Duration(const std::chrono::duration > Period> &d)\n> > > + : BaseDuration(d)\n> > > + {\n> > > + }\n> > > +\n> > > + template\n> > > + double get() const\n> > > + {\n> > > + auto const c =\n> > std::chrono::duration_cast > Period>>(*this);\n> > > + return c.count();\n> > > + }\n> > > +\n> > > + explicit constexpr operator bool() const\n> > > + {\n> > > + return *this != BaseDuration::zero();\n> > > + }\n> > > +};\n> > > +\n> > > +#ifndef __DOXYGEN__\n> > > +template\n> > > +static inline std::basic_ostream\n> > &operator<<(std::basic_ostream &os,\n> > > + const\n> > 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> > > +#endif\n> >\n> > One small thing I wonder about is that many classes have a toString\n> > method. Do we want one here? It could even take an optional argument\n> > to allow you to format the output differently... but I think that's\n> > overkill and best left for now!\n> >\n> >\n> > Did you mean that we should do something like:\n> >\n> > LOG(IPA, Debug) << \"Exposure time is \" << exposure.toString();\n> >\n> > over\n> >\n> > LOG(IPA, Debug) << \"Exposure time is \" << exposure;\n> >\n> > I chose the later way with the overload <<() operator as that is more\n> > similar\n> > to what C++20 will eventually allow with std::chrono::duration\n> formatting.\n> > What do others think?\n>\n> I think the main reason we've used toString() is so that we don't have\n> to override operator<< outside of the libcamera namespace, so we've been\n> more explicit with the toString().\n>\n\n> I don't mind the override or the explicit, but I think that's the\n> reasoning.\n>\n> Would it be cleaner to implement a toString() which doesn't have to\n> track the outputstream state, using a local stringstream perhaps and as\n> that returns a string, the override operator<< can simply return that\n> pre-prepared string?\n>\n\nI don't think it makes much of a difference in the implementation, as this\nis exactly\nwhat the operator<< overload does right now. There is an awkwardness in\nhaving\nto remember to do a \"using libcamera::utils::operator<<\" in every source\nfile\nthat wants to use the overload. Te toString() usage could eliminate that,\nbut then\nit means typing more characters in your logging statement :-)\n\nI do like the overload usage (apart from the using.. directive) in that it\nsort of mirrors\nwhat C++20 will allow.\n\nNaush\n\n\n>\n>\n> >\n> > Regards,\n> > Naush\n> >\n> >\n> >\n> >\n> > > +\n> > > } /* namespace utils */\n> > >\n> > > } /* namespace libcamera */\n> > > diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp\n> > > index 826144d3c837..3131aa2d6666 100644\n> > > --- a/src/libcamera/utils.cpp\n> > > +++ b/src/libcamera/utils.cpp\n> > > @@ -506,6 +506,42 @@ std::string libcameraSourcePath()\n> > > * loop, iterates over an indexed view of the \\a iterable\n> > > */\n> > >\n> > > +/**\n> > > + * \\class Duration\n> > > + * \\brief Helper for std::chrono::duration types. Derived from \\a\n> > BaseDuration\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn Duration::Duration(const std::chrono::duration > Period> &d)\n> > > + * \\brief Copy constructor from a \\a std::chrono::duration type.\n> > > + * \\param[in] d The std::chrono::duration object to convert from.\n> > > + *\n> > > + * Constructs a \\a Duration object from a \\a\n> > std::chrono::duration object,\n> > > + * converting the representation to \\a BaseDuration type.\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn Duration::get()\n> > > + * \\brief Retrieve the tick count, coverted to the timebase\n> > provided by the\n> >\n> > s/coverted/converted/\n> >\n> > With this tiny fix:\n> >\n> > Reviewed-by: David Plowman > >\n> >\n> > Thanks!\n> > David\n> >\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 Tick count\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn Duration::operator bool()\n> > > + * \\brief Boolean operator to test if a \\a Duration holds a\n> > non-zero time value.\n> > > + *\n> > > + * \\return True if \\a Duration is a non-zero time value, False\n> > otherwise.\n> > > + */\n> > > +\n> > > } /* namespace utils */\n> > >\n> > > } /* namespace libcamera */\n> > > --\n> > > 2.25.1\n> > >\n> >\n>\n> --\n> Regards\n> --\n> Kieran\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 578D8C3200\n\tfor ;\n\tFri, 21 May 2021 11:44:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CB41768921;\n\tFri, 21 May 2021 13:44:44 +0200 (CEST)","from mail-lf1-x135.google.com (mail-lf1-x135.google.com\n\t[IPv6:2a00:1450:4864:20::135])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2DC1568911\n\tfor ;\n\tFri, 21 May 2021 13:44:43 +0200 (CEST)","by mail-lf1-x135.google.com with SMTP id r5so29208020lfr.5\n\tfor ;\n\tFri, 21 May 2021 04:44:43 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"oua+6LFO\"; 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=nCLl291IWFlLQIVkPUcCz/ML1f/ozxevXOZFM8e8yy4=;\n\tb=oua+6LFOb4Oh6TTBWxnHad8IzH/yfNB0NRyexohyOjoJ9nFZvbKM6+ShkJ8pkXc3cZ\n\twQuxKqDl3Udo3sKC9qnoPzyGwYrp5x+b+Glcb7ZkiK/Kj9n0+d/u1hOOamKSaQERnYKw\n\ttIeQHLVGdJ6mxNeEgSaLyB7V/Hl7JL8db7OChZXVDXlkg9s4neWaJ3m0hbN+f7NaqmT8\n\tPJlWu70f04n46X3Iy9OfpjAnsSHhNlA8wAEKWquKiP1VoU374nzHkBNFFUe6njnewe7H\n\tXAZHRlKCk1ffMDbPGBEIKq807kqxYrWauwK8Af0aa3i3+g6BE37g9iVrkyqbtz93R2BE\n\tHKVg==","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=nCLl291IWFlLQIVkPUcCz/ML1f/ozxevXOZFM8e8yy4=;\n\tb=X/D0YhDhggTgC668mnor7oLOa750hQSgzfEFd5O60URUQyPL1AwIYQawdv2xzGcfNi\n\tgoXHA8voS4oaQVZ7wj0l/ti3nOEyM2TbquTjIm47Ng13B4Hl8g2qp7N77eoUQb+LU7V+\n\tKUSvTOi5nJbn6RXS81hstl1nsveBQa9DLWnXrwcrB4atxOyxgw0S2JZgrw/hFW6wVzwH\n\tHW3aq0BE2BS8pf4oMDyQ9w6wQRIp5Ny2mfJAktl/cALMdv60heU1m2j+k43QGLqrBTnZ\n\txZS4ZxHRwTZBV/qVnNwNgbaVXLf5iq1PJHBi4rkq586b3M37V/+A/sbJDqU5XB534aTr\n\tAzzg==","X-Gm-Message-State":"AOAM533rpIh0ZCtaRrdX7lzLwLb2sLNBeaxvzUmjKKIVDjci5kK4BMUA\n\tmIGqS5+edOM/+QxIxYb1bWObM+AY94oKdD6Hmvzk4Dmw84U=","X-Google-Smtp-Source":"ABdhPJyccw2yXsMBP8mZv5KyGzeCprYG5BWpBrhdgMZwVfw0/5bTSiWstNXU2MXY81HIwBbQVn5irnt7eH5PgnWFgUw=","X-Received":"by 2002:a05:6512:10d4:: with SMTP id\n\tk20mr1900742lfg.210.1621597482566; \n\tFri, 21 May 2021 04:44:42 -0700 (PDT)","MIME-Version":"1.0","References":"<20210521080530.37591-1-naush@raspberrypi.com>\n\t<20210521080530.37591-2-naush@raspberrypi.com>\n\t\n\t\n\t<646e8fb1-9dcf-f352-7a99-3303fb8816e0@ideasonboard.com>","In-Reply-To":"<646e8fb1-9dcf-f352-7a99-3303fb8816e0@ideasonboard.com>","From":"Naushir Patuck ","Date":"Fri, 21 May 2021 12:44:26 +0100","Message-ID":"","To":"Kieran Bingham ","Content-Type":"multipart/alternative; boundary=\"000000000000eade0305c2d595b1\"","Subject":"Re: [libcamera-devel] [PATCH v3 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":17097,"web_url":"https://patchwork.libcamera.org/comment/17097/","msgid":"<20210521122748.uxiqsqouexdvdpqw@uno.localdomain>","date":"2021-05-21T12:27:48","subject":"Re: [libcamera-devel] [PATCH v3 1/4] libcamera: utils: Add helper\n\tclass for std::chrono::duration","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Naush,\n\nOn Fri, May 21, 2021 at 09:05:27AM +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>\n> An operator << overload is define to help with displaying\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> ---\n> include/libcamera/internal/utils.h | 42 ++++++++++++++++++++++++++++++\n> src/libcamera/utils.cpp | 36 +++++++++++++++++++++++++\n> 2 files changed, 78 insertions(+)\n>\n> diff --git a/include/libcamera/internal/utils.h b/include/libcamera/internal/utils.h\n> index 83dada7cc16c..a377d4ea07ab 100644\n> --- a/include/libcamera/internal/utils.h\n> +++ b/include/libcamera/internal/utils.h\n> @@ -316,6 +316,48 @@ auto enumerate(T (&iterable)[N]) -> details::enumerate_adapter\n> }\n> #endif\n>\n> +using BaseDuration = std::chrono::duration;\n\nI was a bit puzzled by the use of double, but reading the standard I\nsee constructor 4)\n\ntemplate< class Rep2, class Period2 >\n constexpr duration( const duration& d );\n\nwhich partecipates in overload resolution if\n computation of the conversion factor (by\n std::ratio_divide) does not overflow and:\n - std::chrono::treat_as_floating_point::value == true\n or both:\n - std::ratio_divide::den == 1, and\n - std::chrono::treat_as_floating_point::value == false.\n\n (that is, either the duration uses floating-point ticks, or\n Period2 is exactly divisible by period)\n\nSo I guess we need double to maximize the number of template\nspecializations that can be used to construct a Duration ?\n\nCould we use std::chrono:nanoseconds ? Or it gets complicated to\nspecify the additional template argument ? Just out of\ncuriosity...\n\nAlso, I would not expose BaseDuration outside. Can this be done as\n\n class Duration : public std::chrono::duration:...\n {\n using BaseDuration = ....\n\n };\n\nOr do you plan to expose BaseDuration to the rest of the library ?\n\n> +\n> +class Duration : public BaseDuration\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> +#ifndef __DOXYGEN__\n> +template\n> +static inline std::basic_ostream &operator<<(std::basic_ostream &os,\n> +\t\t\t\t\t\t\t const Duration &d)\n\nNit: alignement to (\n\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\nWhy micro and not nano ?\n\n> +\ts << d.get() << \"us\";\n> +\treturn os << s.str();\n> +}\n> +#endif\n> +\n> } /* namespace utils */\n>\n> } /* namespace libcamera */\n> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp\n> index 826144d3c837..3131aa2d6666 100644\n> --- a/src/libcamera/utils.cpp\n> +++ b/src/libcamera/utils.cpp\n> @@ -506,6 +506,42 @@ std::string libcameraSourcePath()\n> * loop, iterates over an indexed view of the \\a iterable\n> */\n>\n> +/**\n> + * \\class Duration\n> + * \\brief Helper for std::chrono::duration types. Derived from \\a BaseDuration\n\nWhere's BaseDuration documented ?\n\n> + */\n> +\n> +/**\n> + * \\fn Duration::Duration(const std::chrono::duration &d)\n> + * \\brief Copy constructor from a \\a std::chrono::duration type.\n> + * \\param[in] d The std::chrono::duration object to convert from.\n\nwe don't usually end with a . in documentation single sentences\n\n> + *\n> + * Constructs a \\a Duration object from a \\a std::chrono::duration object,\n> + * converting the representation to \\a BaseDuration type.\n> + */\n> +\n> +/**\n> + * \\fn Duration::get()\n> + * \\brief Retrieve the tick count, coverted 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\nNice!\n\n> + * \\endcode\n> + *\n> + * \\return Tick count\n\nThe 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\nI'm a bit skeptical of operator overloading like this one, even if\nit's understandable that\n Duration d = ...\n if (!d) {\n\n }\nchecks for d being 0 or not.\n\nNot sure Duration::zero() is not better, nor is Duration::expired()...\n\nAll minors, the patch looks mostly good, maybe with BaseDuration made\nprivate...\n\nThanks\n j\n\n\n> +\n> } /* namespace utils */\n>\n> } /* namespace libcamera */\n> --\n> 2.25.1\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 616D4C31FF\n\tfor ;\n\tFri, 21 May 2021 12:27:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C4F836891A;\n\tFri, 21 May 2021 14:27:04 +0200 (CEST)","from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[217.70.178.232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5D93768911\n\tfor ;\n\tFri, 21 May 2021 14:27:03 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay12.mail.gandi.net (Postfix) with ESMTPSA id D1002200010;\n\tFri, 21 May 2021 12:27:02 +0000 (UTC)"],"Date":"Fri, 21 May 2021 14:27:48 +0200","From":"Jacopo Mondi ","To":"Naushir Patuck ","Message-ID":"<20210521122748.uxiqsqouexdvdpqw@uno.localdomain>","References":"<20210521080530.37591-1-naush@raspberrypi.com>\n\t<20210521080530.37591-2-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210521080530.37591-2-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v3 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":17099,"web_url":"https://patchwork.libcamera.org/comment/17099/","msgid":"","date":"2021-05-21T12:42:43","subject":"Re: [libcamera-devel] [PATCH v3 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 Jacopo,\n\nThank you for your review feedback.\n\nOn Fri, 21 May 2021 at 13:27, Jacopo Mondi wrote:\n\n> Hi Naush,\n>\n> On Fri, May 21, 2021 at 09:05:27AM +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> > An operator << overload is define to help with displaying\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> > ---\n> > include/libcamera/internal/utils.h | 42 ++++++++++++++++++++++++++++++\n> > src/libcamera/utils.cpp | 36 +++++++++++++++++++++++++\n> > 2 files changed, 78 insertions(+)\n> >\n> > diff --git a/include/libcamera/internal/utils.h\n> b/include/libcamera/internal/utils.h\n> > index 83dada7cc16c..a377d4ea07ab 100644\n> > --- a/include/libcamera/internal/utils.h\n> > +++ b/include/libcamera/internal/utils.h\n> > @@ -316,6 +316,48 @@ auto enumerate(T (&iterable)[N]) ->\n> details::enumerate_adapter\n> > }\n> > #endif\n> >\n> > +using BaseDuration = std::chrono::duration;\n>\n> I was a bit puzzled by the use of double, but reading the standard I\n> see constructor 4)\n>\n> template< class Rep2, class Period2 >\n> constexpr duration( const duration& d );\n>\n> which partecipates in overload resolution if\n> computation of the conversion factor (by\n> std::ratio_divide) does not overflow and:\n> - std::chrono::treat_as_floating_point::value == true\n> or both:\n> - std::ratio_divide::den == 1, and\n> - std::chrono::treat_as_floating_point::value ==\n> false.\n>\n> (that is, either the duration uses floating-point ticks, or\n> Period2 is exactly divisible by period)\n>\n> So I guess we need double to maximize the number of template\n> specializations that can be used to construct a Duration ?\n>\n\nThat's correct! Also I used double to preserve precision during possible\nbase conversions.\n\n\n>\n> Could we use std::chrono:nanoseconds ? Or it gets complicated to\n> specify the additional template argument ? Just out of\n> curiosity...\n>\n\nI don't think we can, std::chrono::nanoseconds is a typedef for\nstd::chrono::duration, so we would\nlose the double.\n\n\n>\n> Also, I would not expose BaseDuration outside. Can this be done as\n>\n> class Duration : public std::chrono::duration:...\n> {\n> using BaseDuration = ....\n>\n> };\n>\n> Or do you plan to expose BaseDuration to the rest of the library ?\n>\n\nI don't see BaseDuration being used outside this library, so I will put it\ninto the class definition as you suggested.\n\n\n>\n> > +\n> > +class Duration : public BaseDuration\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> > +#ifndef __DOXYGEN__\n> > +template\n> > +static inline std::basic_ostream\n> &operator<<(std::basic_ostream &os,\n> > + const Duration\n> &d)\n>\n> Nit: alignement to (\n>\n\nAck.\n\n\n>\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>\n> Why micro and not nano ?\n>\n\nPurely personal preference :-)\nLaurent did also suggest using seconds. I don't have strong opinions on\nthis one,\nso I will go with what the majority would like.\n\n\n> > + s << d.get() << \"us\";\n> > + return os << s.str();\n> > +}\n> > +#endif\n> > +\n> > } /* namespace utils */\n> >\n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp\n> > index 826144d3c837..3131aa2d6666 100644\n> > --- a/src/libcamera/utils.cpp\n> > +++ b/src/libcamera/utils.cpp\n> > @@ -506,6 +506,42 @@ std::string libcameraSourcePath()\n> > * loop, iterates over an indexed view of the \\a iterable\n> > */\n> >\n> > +/**\n> > + * \\class Duration\n> > + * \\brief Helper for std::chrono::duration types. Derived from \\a\n> BaseDuration\n>\n> Where's BaseDuration documented ?\n>\n\nSorry, this is my bad. It somehow ended up in patch 4/4, don't ask how :-)\nI'll move it back to this patch in the next revision.\n\n\n>\n> > + */\n> > +\n> > +/**\n> > + * \\fn Duration::Duration(const std::chrono::duration &d)\n> > + * \\brief Copy constructor from a \\a std::chrono::duration type.\n> > + * \\param[in] d The std::chrono::duration object to convert from.\n>\n> we don't usually end with a . in documentation single sentences\n>\n\nAck.\n\n\n>\n> > + *\n> > + * Constructs a \\a Duration object from a \\a std::chrono::duration\n> object,\n> > + * converting the representation to \\a BaseDuration type.\n> > + */\n> > +\n> > +/**\n> > + * \\fn Duration::get()\n> > + * \\brief Retrieve the tick count, coverted to the timebase provided by\n> 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>\n> Nice!\n>\n> > + * \\endcode\n> > + *\n> > + * \\return Tick count\n>\n> The tick count of the Duration expressed in \\a Period ?\n>\n\nAck.\n\n\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\n> otherwise.\n> > + */\n>\n> I'm a bit skeptical of operator overloading like this one, even if\n> it's understandable that\n> Duration d = ...\n> if (!d) {\n>\n> }\n> checks for d being 0 or not.\n>\n> Not sure Duration::zero() is not better, nor is Duration::expired()...\n>\n\nI do like that we can use Duration as a scaler equivalent with the overload,\ne.g.\n\nDuration manualShutter = 5ms;\n\nIf (manualShutter) {\n /* do something */\n}\n\nBut again, I am happy to create a Duration::zero() if you prefer.\n\nRegards,\nNaush\n\n\n>\n> All minors, the patch looks mostly good, maybe with BaseDuration made\n> private...\n>\n> Thanks\n> j\n>\n>\n> > +\n> > } /* namespace utils */\n> >\n> > } /* namespace libcamera */\n> > --\n> > 2.25.1\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 6CC1BC31FF\n\tfor ;\n\tFri, 21 May 2021 12:43:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D39296891D;\n\tFri, 21 May 2021 14:43:01 +0200 (CEST)","from mail-lj1-x236.google.com (mail-lj1-x236.google.com\n\t[IPv6:2a00:1450:4864:20::236])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 818CA68911\n\tfor ;\n\tFri, 21 May 2021 14:42:59 +0200 (CEST)","by mail-lj1-x236.google.com with SMTP id b12so16555368ljp.1\n\tfor ;\n\tFri, 21 May 2021 05:42: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=\"o8ITc/oU\"; 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=smkdmM+/TNBxxSZOkw3fFF+GgjLpKlQJRhzxYRQ6LPc=;\n\tb=o8ITc/oUcRB4mNYv+tvmMFDoUKskQJvoZxuvah5QYikKvaEP4S6TDJXdE2Yf01+dfa\n\tdZ7Xhfd+1rdPKs1P67VVvmkwO77godpIKU1iZ9Gf5RRxdRONagYQRxsZdjbQ+SksqvmK\n\tSlL+HSBA2viekOPsgWXZGe8+kPJolHz3xprvDz3pQlyRNZS6tPKhoceJhZjIRWluVpe0\n\t+Lv8uGfI7w5WxTN5L7cjfGzvR3WV4UHpj6Sm2+Azb8bv7YHrIdtu4w9xLlKOkGH8Y9v8\n\t2g7wKizNOmM3vhc+5KjjTYHjMbt1iwLPMJglVnTXd/+Rf0oe3q7fLOXO6DhWNouF4Tr9\n\tZE3Q==","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=smkdmM+/TNBxxSZOkw3fFF+GgjLpKlQJRhzxYRQ6LPc=;\n\tb=iVGOZ+9Q40H/ENEiQYqm7TxvoeUmb7TtlWZlH7WGATD6wljSHYcu+gNXz7nmoCNVF3\n\tlACdvCHkR8iQ5kLg/MLDopDcfvruVnfxczVkLBOAokhxcN+nEIzlkSIQK0/ZyUWetOO7\n\tGkDo9VQmmI9C69fN0ScHOj27TEfIyMBfQyxuqyZsvZZS0O66jy2F9d7OIPbpb8Cpf4kC\n\t8X7rAxrrXpJ31ldYMl2Xoji8bQsG2KKB7sPPiKSS2t2y+aHw9pia5nYuMyEwbBH39Q8E\n\tpMxcJMFTmBD4YNsngb/NCv3AwjAuirglnxnGrtMgVzCGx+KtzmDQcC8rjB4TcLJkq6oX\n\tQkdg==","X-Gm-Message-State":"AOAM530Kuhk5b3ozJBArJde57kjTiWcCnpvQ17gkFt8oMr3u+tL/es/s\n\tcF1FU/eQJjwKyRW4hwnNi7fIScSwPfrZ52hRbTmBE6qE0nd74A==","X-Google-Smtp-Source":"ABdhPJyj6LZiHHX+yJCQiaAdc/A3z0As41hpXPghq94RvuJEHCr7oaajuxsx9D6hH2tDCc8dxvm4FzoOqVAY6M37b9E=","X-Received":"by 2002:a2e:9145:: with SMTP id q5mr6639353ljg.400.1621600978923;\n\tFri, 21 May 2021 05:42:58 -0700 (PDT)","MIME-Version":"1.0","References":"<20210521080530.37591-1-naush@raspberrypi.com>\n\t<20210521080530.37591-2-naush@raspberrypi.com>\n\t<20210521122748.uxiqsqouexdvdpqw@uno.localdomain>","In-Reply-To":"<20210521122748.uxiqsqouexdvdpqw@uno.localdomain>","From":"Naushir Patuck ","Date":"Fri, 21 May 2021 13:42:43 +0100","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"multipart/alternative; boundary=\"00000000000051026905c2d66685\"","Subject":"Re: [libcamera-devel] [PATCH v3 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":17102,"web_url":"https://patchwork.libcamera.org/comment/17102/","msgid":"<20210521130525.k7llrbfrh6bplee2@uno.localdomain>","date":"2021-05-21T13:05:25","subject":"Re: [libcamera-devel] [PATCH v3 1/4] libcamera: utils: Add helper\n\tclass for std::chrono::duration","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Naush,\n\nOn Fri, May 21, 2021 at 01:42:43PM +0100, Naushir Patuck wrote:\n> Hi Jacopo,\n>\n> Thank you for your review feedback.\n>\n> On Fri, 21 May 2021 at 13:27, Jacopo Mondi wrote:\n>\n> > Hi Naush,\n> >\n> > On Fri, May 21, 2021 at 09:05:27AM +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> > > An operator << overload is define to help with displaying\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> > > ---\n> > > include/libcamera/internal/utils.h | 42 ++++++++++++++++++++++++++++++\n> > > src/libcamera/utils.cpp | 36 +++++++++++++++++++++++++\n> > > 2 files changed, 78 insertions(+)\n> > >\n> > > diff --git a/include/libcamera/internal/utils.h\n> > b/include/libcamera/internal/utils.h\n> > > index 83dada7cc16c..a377d4ea07ab 100644\n> > > --- a/include/libcamera/internal/utils.h\n> > > +++ b/include/libcamera/internal/utils.h\n> > > @@ -316,6 +316,48 @@ auto enumerate(T (&iterable)[N]) ->\n> > details::enumerate_adapter\n> > > }\n> > > #endif\n> > >\n> > > +using BaseDuration = std::chrono::duration;\n> >\n> > I was a bit puzzled by the use of double, but reading the standard I\n> > see constructor 4)\n> >\n> > template< class Rep2, class Period2 >\n> > constexpr duration( const duration& d );\n> >\n> > which partecipates in overload resolution if\n> > computation of the conversion factor (by\n> > std::ratio_divide) does not overflow and:\n> > - std::chrono::treat_as_floating_point::value == true\n> > or both:\n> > - std::ratio_divide::den == 1, and\n> > - std::chrono::treat_as_floating_point::value ==\n> > false.\n> >\n> > (that is, either the duration uses floating-point ticks, or\n> > Period2 is exactly divisible by period)\n> >\n> > So I guess we need double to maximize the number of template\n> > specializations that can be used to construct a Duration ?\n> >\n>\n> That's correct! Also I used double to preserve precision during possible\n> base conversions.\n>\n>\n> >\n> > Could we use std::chrono:nanoseconds ? Or it gets complicated to\n> > specify the additional template argument ? Just out of\n> > curiosity...\n> >\n>\n> I don't think we can, std::chrono::nanoseconds is a typedef for\n> std::chrono::duration, so we would\n> lose the double.\n>\n>\n> >\n> > Also, I would not expose BaseDuration outside. Can this be done as\n> >\n> > class Duration : public std::chrono::duration:...\n> > {\n> > using BaseDuration = ....\n> >\n> > };\n> >\n> > Or do you plan to expose BaseDuration to the rest of the library ?\n> >\n>\n> I don't see BaseDuration being used outside this library, so I will put it\n> into the class definition as you suggested.\n>\n\nThanks!\n\n>\n> >\n> > > +\n> > > +class Duration : public BaseDuration\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> > > +#ifndef __DOXYGEN__\n> > > +template\n> > > +static inline std::basic_ostream\n> > &operator<<(std::basic_ostream &os,\n> > > + const Duration\n> > &d)\n> >\n> > Nit: alignement to (\n> >\n>\n> Ack.\n>\n>\n> >\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> >\n> > Why micro and not nano ?\n> >\n>\n> Purely personal preference :-)\n> Laurent did also suggest using seconds. I don't have strong opinions on\n> this one,\n> so I will go with what the majority would like.\n\nI see. Seconds for the kind of durations we deal with it probably too\nmuch ? We have failed to standardize on a time measurement unit\nunforuntately. We have a mjority of controls that work in microseconds\nand one nanosecond (SensorTimestamp, my bad). Keep micro might be\nbetter to align with most of the time measurement units. Although\nstabilizing on micro might require some rational values to represent\nshorter events such as a line duration. Anyway, not strictly related\nto this one, feel free to keep micro if you like it most.\n\n>\n>\n> > > + s << d.get() << \"us\";\n> > > + return os << s.str();\n> > > +}\n> > > +#endif\n> > > +\n> > > } /* namespace utils */\n> > >\n> > > } /* namespace libcamera */\n> > > diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp\n> > > index 826144d3c837..3131aa2d6666 100644\n> > > --- a/src/libcamera/utils.cpp\n> > > +++ b/src/libcamera/utils.cpp\n> > > @@ -506,6 +506,42 @@ std::string libcameraSourcePath()\n> > > * loop, iterates over an indexed view of the \\a iterable\n> > > */\n> > >\n> > > +/**\n> > > + * \\class Duration\n> > > + * \\brief Helper for std::chrono::duration types. Derived from \\a\n> > BaseDuration\n> >\n> > Where's BaseDuration documented ?\n> >\n>\n> Sorry, this is my bad. It somehow ended up in patch 4/4, don't ask how :-)\n> I'll move it back to this patch in the next revision.\n>\n>\n> >\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn Duration::Duration(const std::chrono::duration &d)\n> > > + * \\brief Copy constructor from a \\a std::chrono::duration type.\n> > > + * \\param[in] d The std::chrono::duration object to convert from.\n> >\n> > we don't usually end with a . in documentation single sentences\n> >\n>\n> Ack.\n>\n>\n> >\n> > > + *\n> > > + * Constructs a \\a Duration object from a \\a std::chrono::duration\n> > object,\n> > > + * converting the representation to \\a BaseDuration type.\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn Duration::get()\n> > > + * \\brief Retrieve the tick count, coverted to the timebase provided by\n> > 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> >\n> > Nice!\n> >\n> > > + * \\endcode\n> > > + *\n> > > + * \\return Tick count\n> >\n> > The tick count of the Duration expressed in \\a Period ?\n> >\n>\n> Ack.\n>\n>\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\n> > otherwise.\n> > > + */\n> >\n> > I'm a bit skeptical of operator overloading like this one, even if\n> > it's understandable that\n> > Duration d = ...\n> > if (!d) {\n> >\n> > }\n> > checks for d being 0 or not.\n> >\n> > Not sure Duration::zero() is not better, nor is Duration::expired()...\n> >\n>\n> I do like that we can use Duration as a scaler equivalent with the overload,\n> e.g.\n>\n> Duration manualShutter = 5ms;\n>\n> If (manualShutter) {\n> /* do something */\n> }\n>\n> But again, I am happy to create a Duration::zero() if you prefer.\n\nI'm not pushing for it, as I think the behaviour of this bool operator\nis predicatable enough, so feel free to keep it!\n\nThanks\n j\n>\n> Regards,\n> Naush\n>\n>\n> >\n> > All minors, the patch looks mostly good, maybe with BaseDuration made\n> > private...\n> >\n> > Thanks\n> > j\n> >\n> >\n> > > +\n> > > } /* namespace utils */\n> > >\n> > > } /* namespace libcamera */\n> > > --\n> > > 2.25.1\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 72BADC31FF\n\tfor ;\n\tFri, 21 May 2021 13:04:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DDF0E6891D;\n\tFri, 21 May 2021 15:04:40 +0200 (CEST)","from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 25BC368911\n\tfor ;\n\tFri, 21 May 2021 15:04:40 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay4-d.mail.gandi.net (Postfix) with ESMTPSA id 95017E0008;\n\tFri, 21 May 2021 13:04:39 +0000 (UTC)"],"Date":"Fri, 21 May 2021 15:05:25 +0200","From":"Jacopo Mondi ","To":"Naushir Patuck ","Message-ID":"<20210521130525.k7llrbfrh6bplee2@uno.localdomain>","References":"<20210521080530.37591-1-naush@raspberrypi.com>\n\t<20210521080530.37591-2-naush@raspberrypi.com>\n\t<20210521122748.uxiqsqouexdvdpqw@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 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\" "}}]