[{"id":33556,"web_url":"https://patchwork.libcamera.org/comment/33556/","msgid":"<20250303211533.GC30583@pendragon.ideasonboard.com>","date":"2025-03-03T21:15:33","subject":"Re: [PATCH v1 3/3] libcamera: base: log: Do not instantiate disabled\n\t`LogMessage`s","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Barnabás,\n\nThank you for the patch.\n\nOn Mon, Mar 03, 2025 at 04:48:44PM +0100, Barnabás Pőcze wrote:\n> At the moment every `LOG()` macro invocation results in a `LogMessage`\n> being created, the message serialized into an `std::stringstream`.\n> Only in the destructor is is actually checked whether the corresponding\n> `LogCategory` enables the given log level.\n> \n> This is not too efficient, it would be better to skip the log message\n> construction and all the `operator<<()` invocations if the message will\n> just be discarded.\n\nThis means that the behaviour of a LOG() statement where operands have\nside effects would differ depending on the log level. We should really\nnot use any expression with side effects when logging, but if we happen\nto do so, the resulting issue would be fairly hard to debug.\n\nWould it be possible for the LOG() macro to return a \"blackhole\" object\nwhen the severity would result in the message being discarded ?\n\n> This could be easily if the `LOG()` macro accepted its arguments\n> like a traditional function as in that case an appropriate `if`\n> statement can be injected in a do-while loop. However, that is\n> not the case, the `LOG()` macro should effectively \"return\"\n> a stream.\n> \n> It is not possible inject an `if` statement directly as that would\n> lead to issues:\n> \n> if (...)\n> LOG(...)\n> else\n> ...\n> \n> The `else` would bind the to the `if` in the `LOG()` macro. This is\n> diagnosed by `-Wdangling-else`.\n> \n> An alternative approach would be to use a `for` loop and force\n> a single iteration using a boolean flag or similar. This is entirely\n> doable but I think the implemented approach is easier to understand.\n> \n> This change implements the early log level checking using a `switch`\n> statement as this avoids the dangling else related issues. One small\n> issue arises because having a boolean controlling expression is diagnosed\n> by clang (`-Wswitch-bool`); the result is cast to `int` to avoid the warning.\n> \n> Signed-off-by: Barnabás Pőcze \n> ---\n> \n> One remaining questions is the handling of \"Fatal\" log messages. I think\n> it would make sense to handle them separately because that way the compiler\n> could be told that it is actually a `[[noreturn]]` function call.\n> \n> ---\n> include/libcamera/base/log.h | 9 +++++++--\n> 1 file changed, 7 insertions(+), 2 deletions(-)\n> \n> diff --git a/include/libcamera/base/log.h b/include/libcamera/base/log.h\n> index 958cb488d..1a14b8dbc 100644\n> --- a/include/libcamera/base/log.h\n> +++ b/include/libcamera/base/log.h\n> @@ -108,10 +108,15 @@ LogMessage _log(const LogCategory &category, LogSeverity severity,\n> #ifndef __DOXYGEN__\n> #define _LOG_CATEGORY(name) logCategory##name\n> \n> +#define _LOG(cat, sev) \\\n> +\tswitch (const auto &_logCategory = (cat); int(_logCategory.severity() <= Log##sev)) \\\n> +\tcase 1: \\\n> +\t\t_log(_logCategory, Log##sev).stream()\n> +\n> #define _LOG1(severity) \\\n> -\t_log(LogCategory::defaultCategory(), Log##severity).stream()\n> +\t_LOG(LogCategory::defaultCategory(), severity)\n> #define _LOG2(category, severity) \\\n> -\t_log(_LOG_CATEGORY(category)(), Log##severity).stream()\n> +\t_LOG(_LOG_CATEGORY(category)(), severity)\n> \n> /*\n> * Expand the LOG() macro to _LOG1() or _LOG2() based on the number of","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 10E65BD808\n\tfor ;\n\tMon, 3 Mar 2025 21:15:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 22E69687EB;\n\tMon, 3 Mar 2025 22:15:57 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 83DD668772\n\tfor ;\n\tMon, 3 Mar 2025 22:15:55 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A0391169;\n\tMon, 3 Mar 2025 22:14:23 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"IDtGBzHx\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1741036463;\n\tbh=p41p08W5l214bX6qAQ4SV6iEMLF5n0HBME4wi4n6JmU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=IDtGBzHx0dR20QoWpgnF7mI7xuOZu21/hC5m6zIezqCXsxz8eWVFf7KIcvtGrMc1D\n\tk3YsxlQNF20cfc5cyyksrB3ZUVJHDzsL3p26ivjrvHqMA2QBUB8D2H/w/bOK5sTNnX\n\t/VW/5VOUA8no5vjWKjpFq47941f2P4JDVEldRufE=","Date":"Mon, 3 Mar 2025 23:15:33 +0200","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1 3/3] libcamera: base: log: Do not instantiate disabled\n\t`LogMessage`s","Message-ID":"<20250303211533.GC30583@pendragon.ideasonboard.com>","References":"<20250303154844.745574-1-barnabas.pocze@ideasonboard.com>\n\t<20250303154844.745574-4-barnabas.pocze@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20250303154844.745574-4-barnabas.pocze@ideasonboard.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":33557,"web_url":"https://patchwork.libcamera.org/comment/33557/","msgid":"<7a171842-623a-4df3-9e3a-0e3cf320d657@ideasonboard.com>","date":"2025-03-03T21:46:47","subject":"Re: [PATCH v1 3/3] libcamera: base: log: Do not instantiate disabled\n\t`LogMessage`s","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n\n2025. 03. 03. 22:15 keltezéssel, Laurent Pinchart írta:\n> Hi Barnabás,\n> \n> Thank you for the patch.\n> \n> On Mon, Mar 03, 2025 at 04:48:44PM +0100, Barnabás Pőcze wrote:\n>> At the moment every `LOG()` macro invocation results in a `LogMessage`\n>> being created, the message serialized into an `std::stringstream`.\n>> Only in the destructor is is actually checked whether the corresponding\n>> `LogCategory` enables the given log level.\n>>\n>> This is not too efficient, it would be better to skip the log message\n>> construction and all the `operator<<()` invocations if the message will\n>> just be discarded.\n> \n> This means that the behaviour of a LOG() statement where operands have\n> side effects would differ depending on the log level. We should really\n> not use any expression with side effects when logging, but if we happen\n> to do so, the resulting issue would be fairly hard to debug.\n\nThat is true, but how likely is it? Do you think the likelihood is big\nenough to warrant this? I would assume it is unlikely.\n\n\n> \n> Would it be possible for the LOG() macro to return a \"blackhole\" object\n> when the severity would result in the message being discarded ?\n\nIf we had `LOG(X, Y, args...)`, then it would be more or less trivial.\nBut with the current status quo I think something like this is needed:\n\n struct LogMessageStream {\n std::ostringstream *const s = nullptr;\n\n template())>>\n LogMessageStream operator<<(const T& x) const\n {\n if (s) *s << x;\n return *this;\n }\n };\n\nI am not sure how well the optimizer can handle this (probably not too bad).\nAlso there are still many `x.toString()` calls, and especially in log messages,\nwhich would unfortunately still be evaluated unnecessarily.\n\n\nRegards,\nBarnabás Pőcze\n\n\n> \n>> This could be easily if the `LOG()` macro accepted its arguments\n>> like a traditional function as in that case an appropriate `if`\n>> statement can be injected in a do-while loop. However, that is\n>> not the case, the `LOG()` macro should effectively \"return\"\n>> a stream.\n>>\n>> It is not possible inject an `if` statement directly as that would\n>> lead to issues:\n>>\n>> if (...)\n>> LOG(...)\n>> else\n>> ...\n>>\n>> The `else` would bind the to the `if` in the `LOG()` macro. This is\n>> diagnosed by `-Wdangling-else`.\n>>\n>> An alternative approach would be to use a `for` loop and force\n>> a single iteration using a boolean flag or similar. This is entirely\n>> doable but I think the implemented approach is easier to understand.\n>>\n>> This change implements the early log level checking using a `switch`\n>> statement as this avoids the dangling else related issues. One small\n>> issue arises because having a boolean controlling expression is diagnosed\n>> by clang (`-Wswitch-bool`); the result is cast to `int` to avoid the warning.\n>>\n>> Signed-off-by: Barnabás Pőcze \n>> ---\n>>\n>> One remaining questions is the handling of \"Fatal\" log messages. I think\n>> it would make sense to handle them separately because that way the compiler\n>> could be told that it is actually a `[[noreturn]]` function call.\n>>\n>> ---\n>> include/libcamera/base/log.h | 9 +++++++--\n>> 1 file changed, 7 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/include/libcamera/base/log.h b/include/libcamera/base/log.h\n>> index 958cb488d..1a14b8dbc 100644\n>> --- a/include/libcamera/base/log.h\n>> +++ b/include/libcamera/base/log.h\n>> @@ -108,10 +108,15 @@ LogMessage _log(const LogCategory &category, LogSeverity severity,\n>> #ifndef __DOXYGEN__\n>> #define _LOG_CATEGORY(name) logCategory##name\n>>\n>> +#define _LOG(cat, sev) \\\n>> +\tswitch (const auto &_logCategory = (cat); int(_logCategory.severity() <= Log##sev)) \\\n>> +\tcase 1: \\\n>> +\t\t_log(_logCategory, Log##sev).stream()\n>> +\n>> #define _LOG1(severity) \\\n>> -\t_log(LogCategory::defaultCategory(), Log##severity).stream()\n>> +\t_LOG(LogCategory::defaultCategory(), severity)\n>> #define _LOG2(category, severity) \\\n>> -\t_log(_LOG_CATEGORY(category)(), Log##severity).stream()\n>> +\t_LOG(_LOG_CATEGORY(category)(), severity)\n>>\n>> /*\n>> * Expand the LOG() macro to _LOG1() or _LOG2() based on the number of\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 AFC3CC3257\n\tfor ;\n\tMon, 3 Mar 2025 21:46:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CD2A2687EB;\n\tMon, 3 Mar 2025 22:46:52 +0100 (CET)","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 12F8C68772\n\tfor ;\n\tMon, 3 Mar 2025 22:46:52 +0100 (CET)","from [192.168.33.24] (185.221.143.4.nat.pool.zt.hu [185.221.143.4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 071A5169;\n\tMon, 3 Mar 2025 22:45:19 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"vPjddxNc\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1741038320;\n\tbh=pIwO6BoF7ombXePoGlUA/l7vqagDVzZlx5YFwc+geI8=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=vPjddxNcmwF+Hz7Ajzg0VfMUUqadX/UOAijNE7sJkvqhgrCHfWNei5xsW+FbnGCLG\n\txV5yUMBS5cgyN+5YCVCTIrNQxRwpzO6gaTj/z0Yth/VouE+h4aPVXdFI/seImfazMj\n\t0iBjC+LHt8PK10Yt10qApbLwGQdc6apgAz1TW6Jo=","Message-ID":"<7a171842-623a-4df3-9e3a-0e3cf320d657@ideasonboard.com>","Date":"Mon, 3 Mar 2025 22:46:47 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v1 3/3] libcamera: base: log: Do not instantiate disabled\n\t`LogMessage`s","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20250303154844.745574-1-barnabas.pocze@ideasonboard.com>\n\t<20250303154844.745574-4-barnabas.pocze@ideasonboard.com>\n\t<20250303211533.GC30583@pendragon.ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<20250303211533.GC30583@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":34987,"web_url":"https://patchwork.libcamera.org/comment/34987/","msgid":"<175312350282.50296.8744883867566854160@ping.linuxembedded.co.uk>","date":"2025-07-21T18:45:02","subject":"Re: [PATCH v1 3/3] libcamera: base: log: Do not instantiate disabled\n\t`LogMessage`s","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Barnabás Pőcze (2025-03-03 21:46:47)\n> Hi\n> \n> \n> 2025. 03. 03. 22:15 keltezéssel, Laurent Pinchart írta:\n> > Hi Barnabás,\n> > \n> > Thank you for the patch.\n> > \n> > On Mon, Mar 03, 2025 at 04:48:44PM +0100, Barnabás Pőcze wrote:\n> >> At the moment every `LOG()` macro invocation results in a `LogMessage`\n> >> being created, the message serialized into an `std::stringstream`.\n> >> Only in the destructor is is actually checked whether the corresponding\n> >> `LogCategory` enables the given log level.\n> >>\n> >> This is not too efficient, it would be better to skip the log message\n> >> construction and all the `operator<<()` invocations if the message will\n> >> just be discarded.\n> > \n> > This means that the behaviour of a LOG() statement where operands have\n> > side effects would differ depending on the log level. We should really\n> > not use any expression with side effects when logging, but if we happen\n> > to do so, the resulting issue would be fairly hard to debug.\n> \n> That is true, but how likely is it? Do you think the likelihood is big\n> enough to warrant this? I would assume it is unlikely.\n> \n> \n> > \n> > Would it be possible for the LOG() macro to return a \"blackhole\" object\n> > when the severity would result in the message being discarded ?\n> \n> If we had `LOG(X, Y, args...)`, then it would be more or less trivial.\n> But with the current status quo I think something like this is needed:\n> \n> struct LogMessageStream {\n> std::ostringstream *const s = nullptr;\n> \n> template())>>\n> LogMessageStream operator<<(const T& x) const\n> {\n> if (s) *s << x;\n> return *this;\n> }\n> };\n> \n> I am not sure how well the optimizer can handle this (probably not too bad).\n> Also there are still many `x.toString()` calls, and especially in log messages,\n> which would unfortunately still be evaluated unnecessarily.\n> \n> \n> Regards,\n> Barnabás Pőcze\n> \n> \n> > \n> >> This could be easily if the `LOG()` macro accepted its arguments\n> >> like a traditional function as in that case an appropriate `if`\n> >> statement can be injected in a do-while loop. However, that is\n> >> not the case, the `LOG()` macro should effectively \"return\"\n> >> a stream.\n> >>\n> >> It is not possible inject an `if` statement directly as that would\n> >> lead to issues:\n> >>\n> >> if (...)\n> >> LOG(...)\n> >> else\n> >> ...\n> >>\n> >> The `else` would bind the to the `if` in the `LOG()` macro. This is\n> >> diagnosed by `-Wdangling-else`.\n> >>\n> >> An alternative approach would be to use a `for` loop and force\n> >> a single iteration using a boolean flag or similar. This is entirely\n> >> doable but I think the implemented approach is easier to understand.\n> >>\n> >> This change implements the early log level checking using a `switch`\n> >> statement as this avoids the dangling else related issues. One small\n> >> issue arises because having a boolean controlling expression is diagnosed\n> >> by clang (`-Wswitch-bool`); the result is cast to `int` to avoid the warning.\n> >>\n> >> Signed-off-by: Barnabás Pőcze \n> >> ---\n> >>\n> >> One remaining questions is the handling of \"Fatal\" log messages. I think\n> >> it would make sense to handle them separately because that way the compiler\n> >> could be told that it is actually a `[[noreturn]]` function call.\n> >>\n> >> ---\n> >> include/libcamera/base/log.h | 9 +++++++--\n> >> 1 file changed, 7 insertions(+), 2 deletions(-)\n> >>\n> >> diff --git a/include/libcamera/base/log.h b/include/libcamera/base/log.h\n> >> index 958cb488d..1a14b8dbc 100644\n> >> --- a/include/libcamera/base/log.h\n> >> +++ b/include/libcamera/base/log.h\n> >> @@ -108,10 +108,15 @@ LogMessage _log(const LogCategory &category, LogSeverity severity,\n> >> #ifndef __DOXYGEN__\n> >> #define _LOG_CATEGORY(name) logCategory##name\n> >>\n> >> +#define _LOG(cat, sev) \\\n> >> + switch (const auto &_logCategory = (cat); int(_logCategory.severity() <= Log##sev)) \\\n> >> + case 1: \\\n> >> + _log(_logCategory, Log##sev).stream()\n> >> +\n\nthis looks like ... magic ?\n\nWith the above discussion from Laurent on this 3/3 - perhaps 1/3 and 2/3\ncould be merged and this one dealt with separately.\n\n--\nKieran\n\n\n> >> #define _LOG1(severity) \\\n> >> - _log(LogCategory::defaultCategory(), Log##severity).stream()\n> >> + _LOG(LogCategory::defaultCategory(), severity)\n> >> #define _LOG2(category, severity) \\\n> >> - _log(_LOG_CATEGORY(category)(), Log##severity).stream()\n> >> + _LOG(_LOG_CATEGORY(category)(), severity)\n> >>\n> >> /*\n> >> * Expand the LOG() macro to _LOG1() or _LOG2() based on the number of\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 F0F16BDCC1\n\tfor ;\n\tMon, 21 Jul 2025 18:45:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DC30669006;\n\tMon, 21 Jul 2025 20:45:07 +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 CE8C268FB1\n\tfor ;\n\tMon, 21 Jul 2025 20:45:05 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9DF437917;\n\tMon, 21 Jul 2025 20:44:28 +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=\"DPGXzJap\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1753123468;\n\tbh=u9Vni0CjFgoMB9w/f1QSZkdzyXvqoqmPw0fgFIONSvw=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=DPGXzJapP3yx627m3atoRaSMUw+en3DpTxwRShLbprTALB/hu30tNdBAKFwlKPL2i\n\tcBwhSOMaYVEiSFPA6dpunj7rVS9KCG/hqAaJszTHKn51YHXkbpskXw4bqSS+WqEZkC\n\tt+/M62N/BLg7T/RTsSZMRIFzBNDZrnseM0iRzdBQ=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<7a171842-623a-4df3-9e3a-0e3cf320d657@ideasonboard.com>","References":"<20250303154844.745574-1-barnabas.pocze@ideasonboard.com>\n\t<20250303154844.745574-4-barnabas.pocze@ideasonboard.com>\n\t<20250303211533.GC30583@pendragon.ideasonboard.com>\n\t<7a171842-623a-4df3-9e3a-0e3cf320d657@ideasonboard.com>","Subject":"Re: [PATCH v1 3/3] libcamera: base: log: Do not instantiate disabled\n\t`LogMessage`s","From":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tLaurent Pinchart ","Date":"Mon, 21 Jul 2025 19:45:02 +0100","Message-ID":"<175312350282.50296.8744883867566854160@ping.linuxembedded.co.uk>","User-Agent":"alot/0.9.1","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":35001,"web_url":"https://patchwork.libcamera.org/comment/35001/","msgid":"<20250721193059.GC14718@pendragon.ideasonboard.com>","date":"2025-07-21T19:30:59","subject":"Re: [PATCH v1 3/3] libcamera: base: log: Do not instantiate disabled\n\t`LogMessage`s","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, Mar 03, 2025 at 10:46:47PM +0100, Barnabás Pőcze wrote:\n> 2025. 03. 03. 22:15 keltezéssel, Laurent Pinchart írta:\n> > On Mon, Mar 03, 2025 at 04:48:44PM +0100, Barnabás Pőcze wrote:\n> >> At the moment every `LOG()` macro invocation results in a `LogMessage`\n> >> being created, the message serialized into an `std::stringstream`.\n> >> Only in the destructor is is actually checked whether the corresponding\n> >> `LogCategory` enables the given log level.\n> >>\n> >> This is not too efficient, it would be better to skip the log message\n> >> construction and all the `operator<<()` invocations if the message will\n> >> just be discarded.\n> > \n> > This means that the behaviour of a LOG() statement where operands have\n> > side effects would differ depending on the log level. We should really\n> > not use any expression with side effects when logging, but if we happen\n> > to do so, the resulting issue would be fairly hard to debug.\n> \n> That is true, but how likely is it? Do you think the likelihood is big\n> enough to warrant this? I would assume it is unlikely.\n\nIt's not very likely, but it would be very hard to debug. Probability\ntimes damage is hard to evaluate, but it worries me a bit.\n\n> > Would it be possible for the LOG() macro to return a \"blackhole\" object\n> > when the severity would result in the message being discarded ?\n> \n> If we had `LOG(X, Y, args...)`, then it would be more or less trivial.\n> But with the current status quo I think something like this is needed:\n> \n> struct LogMessageStream {\n> std::ostringstream *const s = nullptr;\n> \n> template())>>\n> LogMessageStream operator<<(const T& x) const\n> {\n> if (s) *s << x;\n> return *this;\n> }\n> };\n> \n> I am not sure how well the optimizer can handle this (probably not too bad).\n> Also there are still many `x.toString()` calls, and especially in log messages,\n> which would unfortunately still be evaluated unnecessarily.\n\nThat's certainly right.\n\nWould you recommend handling the issue through reviews to make sure log\nstatements have no side effect ?\n\n> >> This could be easily if the `LOG()` macro accepted its arguments\n> >> like a traditional function as in that case an appropriate `if`\n> >> statement can be injected in a do-while loop. However, that is\n> >> not the case, the `LOG()` macro should effectively \"return\"\n> >> a stream.\n> >>\n> >> It is not possible inject an `if` statement directly as that would\n> >> lead to issues:\n> >>\n> >> if (...)\n> >> LOG(...)\n> >> else\n> >> ...\n> >>\n> >> The `else` would bind the to the `if` in the `LOG()` macro. This is\n> >> diagnosed by `-Wdangling-else`.\n> >>\n> >> An alternative approach would be to use a `for` loop and force\n> >> a single iteration using a boolean flag or similar. This is entirely\n> >> doable but I think the implemented approach is easier to understand.\n> >>\n> >> This change implements the early log level checking using a `switch`\n> >> statement as this avoids the dangling else related issues. One small\n> >> issue arises because having a boolean controlling expression is diagnosed\n> >> by clang (`-Wswitch-bool`); the result is cast to `int` to avoid the warning.\n> >>\n> >> Signed-off-by: Barnabás Pőcze \n> >> ---\n> >>\n> >> One remaining questions is the handling of \"Fatal\" log messages. I think\n> >> it would make sense to handle them separately because that way the compiler\n> >> could be told that it is actually a `[[noreturn]]` function call.\n> >>\n> >> ---\n> >> include/libcamera/base/log.h | 9 +++++++--\n> >> 1 file changed, 7 insertions(+), 2 deletions(-)\n> >>\n> >> diff --git a/include/libcamera/base/log.h b/include/libcamera/base/log.h\n> >> index 958cb488d..1a14b8dbc 100644\n> >> --- a/include/libcamera/base/log.h\n> >> +++ b/include/libcamera/base/log.h\n> >> @@ -108,10 +108,15 @@ LogMessage _log(const LogCategory &category, LogSeverity severity,\n> >> #ifndef __DOXYGEN__\n> >> #define _LOG_CATEGORY(name) logCategory##name\n> >>\n> >> +#define _LOG(cat, sev) \\\n> >> +\tswitch (const auto &_logCategory = (cat); int(_logCategory.severity() <= Log##sev)) \\\n> >> +\tcase 1: \\\n> >> +\t\t_log(_logCategory, Log##sev).stream()\n> >> +\n> >> #define _LOG1(severity) \\\n> >> -\t_log(LogCategory::defaultCategory(), Log##severity).stream()\n> >> +\t_LOG(LogCategory::defaultCategory(), severity)\n> >> #define _LOG2(category, severity) \\\n> >> -\t_log(_LOG_CATEGORY(category)(), Log##severity).stream()\n> >> +\t_LOG(_LOG_CATEGORY(category)(), severity)\n> >>\n> >> /*\n> >> * Expand the LOG() macro to _LOG1() or _LOG2() based on the number of","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 28FA4C3237\n\tfor ;\n\tMon, 21 Jul 2025 19:31:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DBC2069018;\n\tMon, 21 Jul 2025 21:31:04 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0C55368FB1\n\tfor ;\n\tMon, 21 Jul 2025 21:31:03 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id CF28A7950; \n\tMon, 21 Jul 2025 21:30:25 +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=\"NRyQRiiq\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1753126226;\n\tbh=gf8XP71OMaxvx7E5gzOR5TrIYDXb35QIU3p/VLhubL8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=NRyQRiiqMvD/WNmO9Q4yhnxxD6P/aqCWJaYFBjpSD8oBOnHn8PJEqJFc+wEIqdHWX\n\tK1+i+SonakWlTpvZ7gE5534WzzU14Vn+Eh29mxIdVYgTnu60L4FV1+V9Ke58OGwH6z\n\tDMNcA3P6uTcdOAa5silt9a6kztTF8Df8dzsnUXrQ=","Date":"Mon, 21 Jul 2025 22:30:59 +0300","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1 3/3] libcamera: base: log: Do not instantiate disabled\n\t`LogMessage`s","Message-ID":"<20250721193059.GC14718@pendragon.ideasonboard.com>","References":"<20250303154844.745574-1-barnabas.pocze@ideasonboard.com>\n\t<20250303154844.745574-4-barnabas.pocze@ideasonboard.com>\n\t<20250303211533.GC30583@pendragon.ideasonboard.com>\n\t<7a171842-623a-4df3-9e3a-0e3cf320d657@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<7a171842-623a-4df3-9e3a-0e3cf320d657@ideasonboard.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":35045,"web_url":"https://patchwork.libcamera.org/comment/35045/","msgid":"<719c9351-ba8b-427a-9987-afef44b0ed6c@ideasonboard.com>","date":"2025-07-23T15:50:51","subject":"Re: [PATCH v1 3/3] libcamera: base: log: Do not instantiate disabled\n\t`LogMessage`s","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n2025. 07. 21. 20:45 keltezéssel, Kieran Bingham írta:\n> Quoting Barnabás Pőcze (2025-03-03 21:46:47)\n>> Hi\n>>\n>>\n>> 2025. 03. 03. 22:15 keltezéssel, Laurent Pinchart írta:\n>>> Hi Barnabás,\n>>>\n>>> Thank you for the patch.\n>>>\n>>> On Mon, Mar 03, 2025 at 04:48:44PM +0100, Barnabás Pőcze wrote:\n>>>> At the moment every `LOG()` macro invocation results in a `LogMessage`\n>>>> being created, the message serialized into an `std::stringstream`.\n>>>> Only in the destructor is is actually checked whether the corresponding\n>>>> `LogCategory` enables the given log level.\n>>>>\n>>>> This is not too efficient, it would be better to skip the log message\n>>>> construction and all the `operator<<()` invocations if the message will\n>>>> just be discarded.\n>>>\n>>> This means that the behaviour of a LOG() statement where operands have\n>>> side effects would differ depending on the log level. We should really\n>>> not use any expression with side effects when logging, but if we happen\n>>> to do so, the resulting issue would be fairly hard to debug.\n>>\n>> That is true, but how likely is it? Do you think the likelihood is big\n>> enough to warrant this? I would assume it is unlikely.\n>>\n>>\n>>>\n>>> Would it be possible for the LOG() macro to return a \"blackhole\" object\n>>> when the severity would result in the message being discarded ?\n>>\n>> If we had `LOG(X, Y, args...)`, then it would be more or less trivial.\n>> But with the current status quo I think something like this is needed:\n>>\n>> struct LogMessageStream {\n>> std::ostringstream *const s = nullptr;\n>>\n>> template())>>\n>> LogMessageStream operator<<(const T& x) const\n>> {\n>> if (s) *s << x;\n>> return *this;\n>> }\n>> };\n>>\n>> I am not sure how well the optimizer can handle this (probably not too bad).\n>> Also there are still many `x.toString()` calls, and especially in log messages,\n>> which would unfortunately still be evaluated unnecessarily.\n>>\n>>\n>> Regards,\n>> Barnabás Pőcze\n>>\n>>\n>>>\n>>>> This could be easily if the `LOG()` macro accepted its arguments\n>>>> like a traditional function as in that case an appropriate `if`\n>>>> statement can be injected in a do-while loop. However, that is\n>>>> not the case, the `LOG()` macro should effectively \"return\"\n>>>> a stream.\n>>>>\n>>>> It is not possible inject an `if` statement directly as that would\n>>>> lead to issues:\n>>>>\n>>>> if (...)\n>>>> LOG(...)\n>>>> else\n>>>> ...\n>>>>\n>>>> The `else` would bind the to the `if` in the `LOG()` macro. This is\n>>>> diagnosed by `-Wdangling-else`.\n>>>>\n>>>> An alternative approach would be to use a `for` loop and force\n>>>> a single iteration using a boolean flag or similar. This is entirely\n>>>> doable but I think the implemented approach is easier to understand.\n>>>>\n>>>> This change implements the early log level checking using a `switch`\n>>>> statement as this avoids the dangling else related issues. One small\n>>>> issue arises because having a boolean controlling expression is diagnosed\n>>>> by clang (`-Wswitch-bool`); the result is cast to `int` to avoid the warning.\n>>>>\n>>>> Signed-off-by: Barnabás Pőcze \n>>>> ---\n>>>>\n>>>> One remaining questions is the handling of \"Fatal\" log messages. I think\n>>>> it would make sense to handle them separately because that way the compiler\n>>>> could be told that it is actually a `[[noreturn]]` function call.\n>>>>\n>>>> ---\n>>>> include/libcamera/base/log.h | 9 +++++++--\n>>>> 1 file changed, 7 insertions(+), 2 deletions(-)\n>>>>\n>>>> diff --git a/include/libcamera/base/log.h b/include/libcamera/base/log.h\n>>>> index 958cb488d..1a14b8dbc 100644\n>>>> --- a/include/libcamera/base/log.h\n>>>> +++ b/include/libcamera/base/log.h\n>>>> @@ -108,10 +108,15 @@ LogMessage _log(const LogCategory &category, LogSeverity severity,\n>>>> #ifndef __DOXYGEN__\n>>>> #define _LOG_CATEGORY(name) logCategory##name\n>>>>\n>>>> +#define _LOG(cat, sev) \\\n>>>> + switch (const auto &_logCategory = (cat); int(_logCategory.severity() <= Log##sev)) \\\n>>>> + case 1: \\\n>>>> + _log(_logCategory, Log##sev).stream()\n>>>> +\n> \n> this looks like ... magic ?\n> \n> With the above discussion from Laurent on this 3/3 - perhaps 1/3 and 2/3\n> could be merged and this one dealt with separately.\n\nPatch 2 is really only needed because of patch 3, so I will merge just patch 1.\n\nRegards,\nBarnabás Pőcze\n\n> \n> --\n> Kieran\n> \n> \n>>>> #define _LOG1(severity) \\\n>>>> - _log(LogCategory::defaultCategory(), Log##severity).stream()\n>>>> + _LOG(LogCategory::defaultCategory(), severity)\n>>>> #define _LOG2(category, severity) \\\n>>>> - _log(_LOG_CATEGORY(category)(), Log##severity).stream()\n>>>> + _LOG(_LOG_CATEGORY(category)(), severity)\n>>>>\n>>>> /*\n>>>> * Expand the LOG() macro to _LOG1() or _LOG2() based on the number of\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 37194BDCC1\n\tfor ;\n\tWed, 23 Jul 2025 15:51:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DF04769056;\n\tWed, 23 Jul 2025 17:51:00 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D8C1F614ED\n\tfor ;\n\tWed, 23 Jul 2025 17:50:59 +0200 (CEST)","from [192.168.33.15] (185.221.140.39.nat.pool.zt.hu\n\t[185.221.140.39])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 545CDE92;\n\tWed, 23 Jul 2025 17:50:21 +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=\"L73LSDl4\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1753285821;\n\tbh=b2VomVgPVqOmMpHvOTIefWTyffpQAMPqq9PHOJQ3330=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=L73LSDl4yBH4ik1KXZOtFB2+YMf41HkPvEIAY7obkeYk7nfM/HlxTD/epizK06vLM\n\tURdO6LAVAJ8mvc3imj87JD88O32wa/RrsYOPdfrbL9hJUaS8NTabgT8ORPobM1OGYs\n\t8km0LkPwrVb7Udjp395Fp0Frxvr5S1uP5AsjC7Os=","Message-ID":"<719c9351-ba8b-427a-9987-afef44b0ed6c@ideasonboard.com>","Date":"Wed, 23 Jul 2025 17:50:51 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v1 3/3] libcamera: base: log: Do not instantiate disabled\n\t`LogMessage`s","To":"Kieran Bingham ,\n\tLaurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20250303154844.745574-1-barnabas.pocze@ideasonboard.com>\n\t<20250303154844.745574-4-barnabas.pocze@ideasonboard.com>\n\t<20250303211533.GC30583@pendragon.ideasonboard.com>\n\t<7a171842-623a-4df3-9e3a-0e3cf320d657@ideasonboard.com>\n\t<175312350282.50296.8744883867566854160@ping.linuxembedded.co.uk>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<175312350282.50296.8744883867566854160@ping.linuxembedded.co.uk>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":35047,"web_url":"https://patchwork.libcamera.org/comment/35047/","msgid":"<20250723155750.GB14576@pendragon.ideasonboard.com>","date":"2025-07-23T15:57:50","subject":"Re: [PATCH v1 3/3] libcamera: base: log: Do not instantiate disabled\n\t`LogMessage`s","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Barnabás,\n\nOn Wed, Jul 23, 2025 at 05:50:51PM +0200, Barnabás Pőcze wrote:\n> 2025. 07. 21. 20:45 keltezéssel, Kieran Bingham írta:\n> > Quoting Barnabás Pőcze (2025-03-03 21:46:47)\n> >> 2025. 03. 03. 22:15 keltezéssel, Laurent Pinchart írta:\n> >>> On Mon, Mar 03, 2025 at 04:48:44PM +0100, Barnabás Pőcze wrote:\n> >>>> At the moment every `LOG()` macro invocation results in a `LogMessage`\n> >>>> being created, the message serialized into an `std::stringstream`.\n> >>>> Only in the destructor is is actually checked whether the corresponding\n> >>>> `LogCategory` enables the given log level.\n> >>>>\n> >>>> This is not too efficient, it would be better to skip the log message\n> >>>> construction and all the `operator<<()` invocations if the message will\n> >>>> just be discarded.\n> >>>\n> >>> This means that the behaviour of a LOG() statement where operands have\n> >>> side effects would differ depending on the log level. We should really\n> >>> not use any expression with side effects when logging, but if we happen\n> >>> to do so, the resulting issue would be fairly hard to debug.\n> >>\n> >> That is true, but how likely is it? Do you think the likelihood is big\n> >> enough to warrant this? I would assume it is unlikely.\n> >>\n> >>\n> >>>\n> >>> Would it be possible for the LOG() macro to return a \"blackhole\" object\n> >>> when the severity would result in the message being discarded ?\n> >>\n> >> If we had `LOG(X, Y, args...)`, then it would be more or less trivial.\n> >> But with the current status quo I think something like this is needed:\n> >>\n> >> struct LogMessageStream {\n> >> std::ostringstream *const s = nullptr;\n> >>\n> >> template())>>\n> >> LogMessageStream operator<<(const T& x) const\n> >> {\n> >> if (s) *s << x;\n> >> return *this;\n> >> }\n> >> };\n> >>\n> >> I am not sure how well the optimizer can handle this (probably not too bad).\n> >> Also there are still many `x.toString()` calls, and especially in log messages,\n> >> which would unfortunately still be evaluated unnecessarily.\n> >>\n> >>\n> >> Regards,\n> >> Barnabás Pőcze\n> >>\n> >>\n> >>>\n> >>>> This could be easily if the `LOG()` macro accepted its arguments\n> >>>> like a traditional function as in that case an appropriate `if`\n> >>>> statement can be injected in a do-while loop. However, that is\n> >>>> not the case, the `LOG()` macro should effectively \"return\"\n> >>>> a stream.\n> >>>>\n> >>>> It is not possible inject an `if` statement directly as that would\n> >>>> lead to issues:\n> >>>>\n> >>>> if (...)\n> >>>> LOG(...)\n> >>>> else\n> >>>> ...\n> >>>>\n> >>>> The `else` would bind the to the `if` in the `LOG()` macro. This is\n> >>>> diagnosed by `-Wdangling-else`.\n> >>>>\n> >>>> An alternative approach would be to use a `for` loop and force\n> >>>> a single iteration using a boolean flag or similar. This is entirely\n> >>>> doable but I think the implemented approach is easier to understand.\n> >>>>\n> >>>> This change implements the early log level checking using a `switch`\n> >>>> statement as this avoids the dangling else related issues. One small\n> >>>> issue arises because having a boolean controlling expression is diagnosed\n> >>>> by clang (`-Wswitch-bool`); the result is cast to `int` to avoid the warning.\n> >>>>\n> >>>> Signed-off-by: Barnabás Pőcze \n> >>>> ---\n> >>>>\n> >>>> One remaining questions is the handling of \"Fatal\" log messages. I think\n> >>>> it would make sense to handle them separately because that way the compiler\n> >>>> could be told that it is actually a `[[noreturn]]` function call.\n> >>>>\n> >>>> ---\n> >>>> include/libcamera/base/log.h | 9 +++++++--\n> >>>> 1 file changed, 7 insertions(+), 2 deletions(-)\n> >>>>\n> >>>> diff --git a/include/libcamera/base/log.h b/include/libcamera/base/log.h\n> >>>> index 958cb488d..1a14b8dbc 100644\n> >>>> --- a/include/libcamera/base/log.h\n> >>>> +++ b/include/libcamera/base/log.h\n> >>>> @@ -108,10 +108,15 @@ LogMessage _log(const LogCategory &category, LogSeverity severity,\n> >>>> #ifndef __DOXYGEN__\n> >>>> #define _LOG_CATEGORY(name) logCategory##name\n> >>>>\n> >>>> +#define _LOG(cat, sev) \\\n> >>>> + switch (const auto &_logCategory = (cat); int(_logCategory.severity() <= Log##sev)) \\\n> >>>> + case 1: \\\n> >>>> + _log(_logCategory, Log##sev).stream()\n> >>>> +\n> > \n> > this looks like ... magic ?\n> > \n> > With the above discussion from Laurent on this 3/3 - perhaps 1/3 and 2/3\n> > could be merged and this one dealt with separately.\n> \n> Patch 2 is really only needed because of patch 3, so I will merge just patch 1.\n\nLet's finish the discussion on this mail thread and try to get this one\nmerged too.\n\n> >>>> #define _LOG1(severity) \\\n> >>>> - _log(LogCategory::defaultCategory(), Log##severity).stream()\n> >>>> + _LOG(LogCategory::defaultCategory(), severity)\n> >>>> #define _LOG2(category, severity) \\\n> >>>> - _log(_LOG_CATEGORY(category)(), Log##severity).stream()\n> >>>> + _LOG(_LOG_CATEGORY(category)(), severity)\n> >>>>\n> >>>> /*\n> >>>> * Expand the LOG() macro to _LOG1() or _LOG2() based on the number of","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 0086CBDCC1\n\tfor ;\n\tWed, 23 Jul 2025 15:57:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 28A476905D;\n\tWed, 23 Jul 2025 17:57:55 +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 B7AC569054\n\tfor ;\n\tWed, 23 Jul 2025 17:57:53 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id EFC13E92;\n\tWed, 23 Jul 2025 17:57:14 +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=\"cUzWat0J\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1753286235;\n\tbh=EmV0NagoYI42XuqiQr4MOxC1qcAWko3fDL77yufjK7s=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=cUzWat0Jp4wG1uKJqEHMS6G/axUayZOcC1qfLiwsPF6tXkoIWz9ZANJ6APEB100b9\n\tqDC5izQPzu5B/MRC4OJZJJCy/MPimRSTcjR1Q9eVv7a/ilmtjJRK6BNLLlyjlmfIxl\n\tNeWT/M1BzbMO7RFxxo934Bqu2bzM8Y9ECS0H+bSo=","Date":"Wed, 23 Jul 2025 18:57:50 +0300","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1 3/3] libcamera: base: log: Do not instantiate disabled\n\t`LogMessage`s","Message-ID":"<20250723155750.GB14576@pendragon.ideasonboard.com>","References":"<20250303154844.745574-1-barnabas.pocze@ideasonboard.com>\n\t<20250303154844.745574-4-barnabas.pocze@ideasonboard.com>\n\t<20250303211533.GC30583@pendragon.ideasonboard.com>\n\t<7a171842-623a-4df3-9e3a-0e3cf320d657@ideasonboard.com>\n\t<175312350282.50296.8744883867566854160@ping.linuxembedded.co.uk>\n\t<719c9351-ba8b-427a-9987-afef44b0ed6c@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<719c9351-ba8b-427a-9987-afef44b0ed6c@ideasonboard.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":35048,"web_url":"https://patchwork.libcamera.org/comment/35048/","msgid":"<125aa931-abf3-4b7f-a95d-e66dbd99025c@ideasonboard.com>","date":"2025-07-23T16:06:01","subject":"Re: [PATCH v1 3/3] libcamera: base: log: Do not instantiate disabled\n\t`LogMessage`s","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n2025. 07. 21. 21:30 keltezéssel, Laurent Pinchart írta:\n> On Mon, Mar 03, 2025 at 10:46:47PM +0100, Barnabás Pőcze wrote:\n>> 2025. 03. 03. 22:15 keltezéssel, Laurent Pinchart írta:\n>>> On Mon, Mar 03, 2025 at 04:48:44PM +0100, Barnabás Pőcze wrote:\n>>>> At the moment every `LOG()` macro invocation results in a `LogMessage`\n>>>> being created, the message serialized into an `std::stringstream`.\n>>>> Only in the destructor is is actually checked whether the corresponding\n>>>> `LogCategory` enables the given log level.\n>>>>\n>>>> This is not too efficient, it would be better to skip the log message\n>>>> construction and all the `operator<<()` invocations if the message will\n>>>> just be discarded.\n>>>\n>>> This means that the behaviour of a LOG() statement where operands have\n>>> side effects would differ depending on the log level. We should really\n>>> not use any expression with side effects when logging, but if we happen\n>>> to do so, the resulting issue would be fairly hard to debug.\n>>\n>> That is true, but how likely is it? Do you think the likelihood is big\n>> enough to warrant this? I would assume it is unlikely.\n> \n> It's not very likely, but it would be very hard to debug. Probability\n> times damage is hard to evaluate, but it worries me a bit.\n> \n>>> Would it be possible for the LOG() macro to return a \"blackhole\" object\n>>> when the severity would result in the message being discarded ?\n>>\n>> If we had `LOG(X, Y, args...)`, then it would be more or less trivial.\n>> But with the current status quo I think something like this is needed:\n>>\n>> struct LogMessageStream {\n>> std::ostringstream *const s = nullptr;\n>>\n>> template())>>\n>> LogMessageStream operator<<(const T& x) const\n>> {\n>> if (s) *s << x;\n>> return *this;\n>> }\n>> };\n>>\n>> I am not sure how well the optimizer can handle this (probably not too bad).\n>> Also there are still many `x.toString()` calls, and especially in log messages,\n>> which would unfortunately still be evaluated unnecessarily.\n> \n> That's certainly right.\n> \n> Would you recommend handling the issue through reviews to make sure log\n> statements have no side effect ?\n\nI have given it some thought, and sadly I have no better idea. I still believe that\nthe advantages outweigh the potential issues. I have one anecdotal evidence: pipewire\nhas the same behaviour and I have never seen it cause any issues (although it's probably\nharder to do that in C in this case); it even has a log level that can be removed from\nthe compilation altogether (which has lead to multiple build failures over the years,\nbut that's all).\n\n\nRegards,\nBarnabás Pőcze\n\n\n> \n>>>> This could be easily if the `LOG()` macro accepted its arguments\n>>>> like a traditional function as in that case an appropriate `if`\n>>>> statement can be injected in a do-while loop. However, that is\n>>>> not the case, the `LOG()` macro should effectively \"return\"\n>>>> a stream.\n>>>>\n>>>> It is not possible inject an `if` statement directly as that would\n>>>> lead to issues:\n>>>>\n>>>> if (...)\n>>>> LOG(...)\n>>>> else\n>>>> ...\n>>>>\n>>>> The `else` would bind the to the `if` in the `LOG()` macro. This is\n>>>> diagnosed by `-Wdangling-else`.\n>>>>\n>>>> An alternative approach would be to use a `for` loop and force\n>>>> a single iteration using a boolean flag or similar. This is entirely\n>>>> doable but I think the implemented approach is easier to understand.\n>>>>\n>>>> This change implements the early log level checking using a `switch`\n>>>> statement as this avoids the dangling else related issues. One small\n>>>> issue arises because having a boolean controlling expression is diagnosed\n>>>> by clang (`-Wswitch-bool`); the result is cast to `int` to avoid the warning.\n>>>>\n>>>> Signed-off-by: Barnabás Pőcze \n>>>> ---\n>>>>\n>>>> One remaining questions is the handling of \"Fatal\" log messages. I think\n>>>> it would make sense to handle them separately because that way the compiler\n>>>> could be told that it is actually a `[[noreturn]]` function call.\n>>>>\n>>>> ---\n>>>> include/libcamera/base/log.h | 9 +++++++--\n>>>> 1 file changed, 7 insertions(+), 2 deletions(-)\n>>>>\n>>>> diff --git a/include/libcamera/base/log.h b/include/libcamera/base/log.h\n>>>> index 958cb488d..1a14b8dbc 100644\n>>>> --- a/include/libcamera/base/log.h\n>>>> +++ b/include/libcamera/base/log.h\n>>>> @@ -108,10 +108,15 @@ LogMessage _log(const LogCategory &category, LogSeverity severity,\n>>>> #ifndef __DOXYGEN__\n>>>> #define _LOG_CATEGORY(name) logCategory##name\n>>>>\n>>>> +#define _LOG(cat, sev) \\\n>>>> +\tswitch (const auto &_logCategory = (cat); int(_logCategory.severity() <= Log##sev)) \\\n>>>> +\tcase 1: \\\n>>>> +\t\t_log(_logCategory, Log##sev).stream()\n>>>> +\n>>>> #define _LOG1(severity) \\\n>>>> -\t_log(LogCategory::defaultCategory(), Log##severity).stream()\n>>>> +\t_LOG(LogCategory::defaultCategory(), severity)\n>>>> #define _LOG2(category, severity) \\\n>>>> -\t_log(_LOG_CATEGORY(category)(), Log##severity).stream()\n>>>> +\t_LOG(_LOG_CATEGORY(category)(), severity)\n>>>>\n>>>> /*\n>>>> * Expand the LOG() macro to _LOG1() or _LOG2() based on the number of\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 5D4F3C3237\n\tfor ;\n\tWed, 23 Jul 2025 16:06:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 451766905F;\n\tWed, 23 Jul 2025 18:06:07 +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 6AE8069054\n\tfor ;\n\tWed, 23 Jul 2025 18:06:05 +0200 (CEST)","from [192.168.33.15] (185.221.140.39.nat.pool.zt.hu\n\t[185.221.140.39])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 81162EA0;\n\tWed, 23 Jul 2025 18:05:26 +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=\"cnG3JLHi\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1753286727;\n\tbh=ZxoI/92XcNos1dkpHm9LRy76z27LRxYZyaxuCWtXxmc=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=cnG3JLHiiHUZjwZbsjL6gNrrdPKMYp8MJ88ILFefL+rP6mXdbEZyRewZtff8KZ0Ex\n\t0uYaSNVOMCWIN4/e8GsQ0vyN26PL/xDIu0Q7arZw+HpxpwOODTg9erhH4pDWBCqowv\n\t4ZJA0/VA18FBMvS1AW98h2VdLdTKjWRl+vGdto2U=","Message-ID":"<125aa931-abf3-4b7f-a95d-e66dbd99025c@ideasonboard.com>","Date":"Wed, 23 Jul 2025 18:06:01 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v1 3/3] libcamera: base: log: Do not instantiate disabled\n\t`LogMessage`s","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20250303154844.745574-1-barnabas.pocze@ideasonboard.com>\n\t<20250303154844.745574-4-barnabas.pocze@ideasonboard.com>\n\t<20250303211533.GC30583@pendragon.ideasonboard.com>\n\t<7a171842-623a-4df3-9e3a-0e3cf320d657@ideasonboard.com>\n\t<20250721193059.GC14718@pendragon.ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<20250721193059.GC14718@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":35051,"web_url":"https://patchwork.libcamera.org/comment/35051/","msgid":"<20250723164335.GA11202@pendragon.ideasonboard.com>","date":"2025-07-23T16:43:35","subject":"Re: [PATCH v1 3/3] libcamera: base: log: Do not instantiate disabled\n\t`LogMessage`s","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Jul 23, 2025 at 06:06:01PM +0200, Barnabás Pőcze wrote:\n> 2025. 07. 21. 21:30 keltezéssel, Laurent Pinchart írta:\n> > On Mon, Mar 03, 2025 at 10:46:47PM +0100, Barnabás Pőcze wrote:\n> >> 2025. 03. 03. 22:15 keltezéssel, Laurent Pinchart írta:\n> >>> On Mon, Mar 03, 2025 at 04:48:44PM +0100, Barnabás Pőcze wrote:\n> >>>> At the moment every `LOG()` macro invocation results in a `LogMessage`\n> >>>> being created, the message serialized into an `std::stringstream`.\n> >>>> Only in the destructor is is actually checked whether the corresponding\n> >>>> `LogCategory` enables the given log level.\n> >>>>\n> >>>> This is not too efficient, it would be better to skip the log message\n> >>>> construction and all the `operator<<()` invocations if the message will\n> >>>> just be discarded.\n> >>>\n> >>> This means that the behaviour of a LOG() statement where operands have\n> >>> side effects would differ depending on the log level. We should really\n> >>> not use any expression with side effects when logging, but if we happen\n> >>> to do so, the resulting issue would be fairly hard to debug.\n> >>\n> >> That is true, but how likely is it? Do you think the likelihood is big\n> >> enough to warrant this? I would assume it is unlikely.\n> > \n> > It's not very likely, but it would be very hard to debug. Probability\n> > times damage is hard to evaluate, but it worries me a bit.\n> > \n> >>> Would it be possible for the LOG() macro to return a \"blackhole\" object\n> >>> when the severity would result in the message being discarded ?\n> >>\n> >> If we had `LOG(X, Y, args...)`, then it would be more or less trivial.\n> >> But with the current status quo I think something like this is needed:\n> >>\n> >> struct LogMessageStream {\n> >> std::ostringstream *const s = nullptr;\n> >>\n> >> template())>>\n> >> LogMessageStream operator<<(const T& x) const\n> >> {\n> >> if (s) *s << x;\n> >> return *this;\n> >> }\n> >> };\n> >>\n> >> I am not sure how well the optimizer can handle this (probably not too bad).\n> >> Also there are still many `x.toString()` calls, and especially in log messages,\n> >> which would unfortunately still be evaluated unnecessarily.\n> > \n> > That's certainly right.\n> > \n> > Would you recommend handling the issue through reviews to make sure log\n> > statements have no side effect ?\n> \n> I have given it some thought, and sadly I have no better idea. I still believe that\n> the advantages outweigh the potential issues. I have one anecdotal evidence: pipewire\n> has the same behaviour and I have never seen it cause any issues (although it's probably\n> harder to do that in C in this case); it even has a log level that can be removed from\n> the compilation altogether (which has lead to multiple build failures over the years,\n> but that's all).\n\nOK, let's focus on the advantages then, and cross our fingers :-)\n\n> >>>> This could be easily if the `LOG()` macro accepted its arguments\n> >>>> like a traditional function as in that case an appropriate `if`\n> >>>> statement can be injected in a do-while loop. However, that is\n> >>>> not the case, the `LOG()` macro should effectively \"return\"\n> >>>> a stream.\n> >>>>\n> >>>> It is not possible inject an `if` statement directly as that would\n> >>>> lead to issues:\n> >>>>\n> >>>> if (...)\n> >>>> LOG(...)\n> >>>> else\n> >>>> ...\n> >>>>\n> >>>> The `else` would bind the to the `if` in the `LOG()` macro. This is\n> >>>> diagnosed by `-Wdangling-else`.\n> >>>>\n> >>>> An alternative approach would be to use a `for` loop and force\n> >>>> a single iteration using a boolean flag or similar. This is entirely\n> >>>> doable but I think the implemented approach is easier to understand.\n> >>>>\n> >>>> This change implements the early log level checking using a `switch`\n> >>>> statement as this avoids the dangling else related issues. One small\n> >>>> issue arises because having a boolean controlling expression is diagnosed\n> >>>> by clang (`-Wswitch-bool`); the result is cast to `int` to avoid the warning.\n> >>>>\n> >>>> Signed-off-by: Barnabás Pőcze \n> >>>> ---\n> >>>>\n> >>>> One remaining questions is the handling of \"Fatal\" log messages. I think\n> >>>> it would make sense to handle them separately because that way the compiler\n> >>>> could be told that it is actually a `[[noreturn]]` function call.\n> >>>>\n> >>>> ---\n> >>>> include/libcamera/base/log.h | 9 +++++++--\n> >>>> 1 file changed, 7 insertions(+), 2 deletions(-)\n> >>>>\n> >>>> diff --git a/include/libcamera/base/log.h b/include/libcamera/base/log.h\n> >>>> index 958cb488d..1a14b8dbc 100644\n> >>>> --- a/include/libcamera/base/log.h\n> >>>> +++ b/include/libcamera/base/log.h\n> >>>> @@ -108,10 +108,15 @@ LogMessage _log(const LogCategory &category, LogSeverity severity,\n> >>>> #ifndef __DOXYGEN__\n> >>>> #define _LOG_CATEGORY(name) logCategory##name\n> >>>>\n> >>>> +#define _LOG(cat, sev) \\\n> >>>> +\tswitch (const auto &_logCategory = (cat); int(_logCategory.severity() <= Log##sev)) \\\n> >>>> +\tcase 1: \\\n> >>>> +\t\t_log(_logCategory, Log##sev).stream()\n> >>>> +\n> >>>> #define _LOG1(severity) \\\n> >>>> -\t_log(LogCategory::defaultCategory(), Log##severity).stream()\n> >>>> +\t_LOG(LogCategory::defaultCategory(), severity)\n> >>>> #define _LOG2(category, severity) \\\n> >>>> -\t_log(_LOG_CATEGORY(category)(), Log##severity).stream()\n> >>>> +\t_LOG(_LOG_CATEGORY(category)(), severity)\n> >>>>\n> >>>> /*\n> >>>> * Expand the LOG() macro to _LOG1() or _LOG2() based on the number of","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 934DFC3237\n\tfor ;\n\tWed, 23 Jul 2025 16:43:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EBB9469061;\n\tWed, 23 Jul 2025 18:43:39 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B626469057\n\tfor ;\n\tWed, 23 Jul 2025 18:43:37 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 29733C78;\n\tWed, 23 Jul 2025 18:42:59 +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=\"p5mUWus+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1753288979;\n\tbh=ubq/9PH2r+jzoSTpaSJI3DvfGlEn72XyTuwD2ORaZPI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=p5mUWus+BkxHpO+nCeVZDozFLOq+q2YtyLcxj8sFEI+2dYOSwqX3q2XsgGQGTLp/x\n\t/wm/aQYUXfo2y2dzOCbkmoZFyRmU4NWU4CGbZ0vDx54Nz39wnIr1U5qWnX8u+C6Isd\n\tci/VKa5saIZoJtUwwcN1LAIrwH96N2+yYemAAg4Q=","Date":"Wed, 23 Jul 2025 19:43:35 +0300","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1 3/3] libcamera: base: log: Do not instantiate disabled\n\t`LogMessage`s","Message-ID":"<20250723164335.GA11202@pendragon.ideasonboard.com>","References":"<20250303154844.745574-1-barnabas.pocze@ideasonboard.com>\n\t<20250303154844.745574-4-barnabas.pocze@ideasonboard.com>\n\t<20250303211533.GC30583@pendragon.ideasonboard.com>\n\t<7a171842-623a-4df3-9e3a-0e3cf320d657@ideasonboard.com>\n\t<20250721193059.GC14718@pendragon.ideasonboard.com>\n\t<125aa931-abf3-4b7f-a95d-e66dbd99025c@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<125aa931-abf3-4b7f-a95d-e66dbd99025c@ideasonboard.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":36051,"web_url":"https://patchwork.libcamera.org/comment/36051/","msgid":"<175922862995.4000262.14562182531096651793@ping.linuxembedded.co.uk>","date":"2025-09-30T10:37:09","subject":"Re: [PATCH v1 3/3] libcamera: base: log: Do not instantiate disabled\n\t`LogMessage`s","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart (2025-07-23 17:43:35)\n> On Wed, Jul 23, 2025 at 06:06:01PM +0200, Barnabás Pőcze wrote:\n> > 2025. 07. 21. 21:30 keltezéssel, Laurent Pinchart írta:\n> > > On Mon, Mar 03, 2025 at 10:46:47PM +0100, Barnabás Pőcze wrote:\n> > >> 2025. 03. 03. 22:15 keltezéssel, Laurent Pinchart írta:\n> > >>> On Mon, Mar 03, 2025 at 04:48:44PM +0100, Barnabás Pőcze wrote:\n> > >>>> At the moment every `LOG()` macro invocation results in a `LogMessage`\n> > >>>> being created, the message serialized into an `std::stringstream`.\n> > >>>> Only in the destructor is is actually checked whether the corresponding\n> > >>>> `LogCategory` enables the given log level.\n> > >>>>\n> > >>>> This is not too efficient, it would be better to skip the log message\n> > >>>> construction and all the `operator<<()` invocations if the message will\n> > >>>> just be discarded.\n> > >>>\n> > >>> This means that the behaviour of a LOG() statement where operands have\n> > >>> side effects would differ depending on the log level. We should really\n> > >>> not use any expression with side effects when logging, but if we happen\n> > >>> to do so, the resulting issue would be fairly hard to debug.\n> > >>\n> > >> That is true, but how likely is it? Do you think the likelihood is big\n> > >> enough to warrant this? I would assume it is unlikely.\n> > > \n> > > It's not very likely, but it would be very hard to debug. Probability\n> > > times damage is hard to evaluate, but it worries me a bit.\n> > > \n> > >>> Would it be possible for the LOG() macro to return a \"blackhole\" object\n> > >>> when the severity would result in the message being discarded ?\n> > >>\n> > >> If we had `LOG(X, Y, args...)`, then it would be more or less trivial.\n> > >> But with the current status quo I think something like this is needed:\n> > >>\n> > >> struct LogMessageStream {\n> > >> std::ostringstream *const s = nullptr;\n> > >>\n> > >> template())>>\n> > >> LogMessageStream operator<<(const T& x) const\n> > >> {\n> > >> if (s) *s << x;\n> > >> return *this;\n> > >> }\n> > >> };\n> > >>\n> > >> I am not sure how well the optimizer can handle this (probably not too bad).\n> > >> Also there are still many `x.toString()` calls, and especially in log messages,\n> > >> which would unfortunately still be evaluated unnecessarily.\n> > > \n> > > That's certainly right.\n> > > \n> > > Would you recommend handling the issue through reviews to make sure log\n> > > statements have no side effect ?\n> > \n> > I have given it some thought, and sadly I have no better idea. I still believe that\n> > the advantages outweigh the potential issues. I have one anecdotal evidence: pipewire\n> > has the same behaviour and I have never seen it cause any issues (although it's probably\n> > harder to do that in C in this case); it even has a log level that can be removed from\n> > the compilation altogether (which has lead to multiple build failures over the years,\n> > but that's all).\n> \n> OK, let's focus on the advantages then, and cross our fingers :-)\n\nI have one project who has concerns over the time it takes to construct\nobjects which are then not used. They in particular have an important\nrequirement to get the camera started as fast as possible.\n\nPersonally I can't imagine that constructing objects in C++ takes a lot\nof time - but if we construct a lot of them and then don't use them -\nperhaps it could add up. But are we talking sub-milliseconds *total*? \n\nHard to know without some sort of measurement and profiling (which isn't\nconstructing a class itself) ... but for that project there is indeed a\ndesire to disable all logging too if it helps so I think this patch\nwould help.\n\n--\nRegards\n\nKieran\n\n\n\n> > >>>> This could be easily if the `LOG()` macro accepted its arguments\n> > >>>> like a traditional function as in that case an appropriate `if`\n> > >>>> statement can be injected in a do-while loop. However, that is\n> > >>>> not the case, the `LOG()` macro should effectively \"return\"\n> > >>>> a stream.\n> > >>>>\n> > >>>> It is not possible inject an `if` statement directly as that would\n> > >>>> lead to issues:\n> > >>>>\n> > >>>> if (...)\n> > >>>> LOG(...)\n> > >>>> else\n> > >>>> ...\n> > >>>>\n> > >>>> The `else` would bind the to the `if` in the `LOG()` macro. This is\n> > >>>> diagnosed by `-Wdangling-else`.\n> > >>>>\n> > >>>> An alternative approach would be to use a `for` loop and force\n> > >>>> a single iteration using a boolean flag or similar. This is entirely\n> > >>>> doable but I think the implemented approach is easier to understand.\n> > >>>>\n> > >>>> This change implements the early log level checking using a `switch`\n> > >>>> statement as this avoids the dangling else related issues. One small\n> > >>>> issue arises because having a boolean controlling expression is diagnosed\n> > >>>> by clang (`-Wswitch-bool`); the result is cast to `int` to avoid the warning.\n> > >>>>\n> > >>>> Signed-off-by: Barnabás Pőcze \n> > >>>> ---\n> > >>>>\n> > >>>> One remaining questions is the handling of \"Fatal\" log messages. I think\n> > >>>> it would make sense to handle them separately because that way the compiler\n> > >>>> could be told that it is actually a `[[noreturn]]` function call.\n> > >>>>\n> > >>>> ---\n> > >>>> include/libcamera/base/log.h | 9 +++++++--\n> > >>>> 1 file changed, 7 insertions(+), 2 deletions(-)\n> > >>>>\n> > >>>> diff --git a/include/libcamera/base/log.h b/include/libcamera/base/log.h\n> > >>>> index 958cb488d..1a14b8dbc 100644\n> > >>>> --- a/include/libcamera/base/log.h\n> > >>>> +++ b/include/libcamera/base/log.h\n> > >>>> @@ -108,10 +108,15 @@ LogMessage _log(const LogCategory &category, LogSeverity severity,\n> > >>>> #ifndef __DOXYGEN__\n> > >>>> #define _LOG_CATEGORY(name) logCategory##name\n> > >>>>\n> > >>>> +#define _LOG(cat, sev) \\\n> > >>>> + switch (const auto &_logCategory = (cat); int(_logCategory.severity() <= Log##sev)) \\\n> > >>>> + case 1: \\\n> > >>>> + _log(_logCategory, Log##sev).stream()\n> > >>>> +\n> > >>>> #define _LOG1(severity) \\\n> > >>>> - _log(LogCategory::defaultCategory(), Log##severity).stream()\n> > >>>> + _LOG(LogCategory::defaultCategory(), severity)\n> > >>>> #define _LOG2(category, severity) \\\n> > >>>> - _log(_LOG_CATEGORY(category)(), Log##severity).stream()\n> > >>>> + _LOG(_LOG_CATEGORY(category)(), severity)\n> > >>>>\n> > >>>> /*\n> > >>>> * Expand the LOG() macro to _LOG1() or _LOG2() based on the number of\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 9A17CC324C\n\tfor ;\n\tTue, 30 Sep 2025 10:37:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A1E1E6B622;\n\tTue, 30 Sep 2025 12:37:15 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 44EC56B5F0\n\tfor ;\n\tTue, 30 Sep 2025 12:37:13 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EF9F08D4;\n\tTue, 30 Sep 2025 12:35:44 +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=\"Uaihx/Hl\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1759228545;\n\tbh=T5XAkP35Bs9+gyIpcEmofKnK+DLpG4C66hRgCSuS7kY=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=Uaihx/Hlt2VTVyMHTTRCKsvg+bDX5Cb6SncGaTdJ9jIykfgiPuydchUC+Mx3/NZga\n\t/0uREaAsFRx/uIs+OwraOf3vXjU9k/qybbPF9JPjPY6e7b+xW1+D7lSsrbHI9yeFOA\n\t80vPbb8HipPcLGtbE3L0W8srzqn4m9UBfFH4bAdE=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250723164335.GA11202@pendragon.ideasonboard.com>","References":"<20250303154844.745574-1-barnabas.pocze@ideasonboard.com>\n\t<20250303154844.745574-4-barnabas.pocze@ideasonboard.com>\n\t<20250303211533.GC30583@pendragon.ideasonboard.com>\n\t<7a171842-623a-4df3-9e3a-0e3cf320d657@ideasonboard.com>\n\t<20250721193059.GC14718@pendragon.ideasonboard.com>\n\t<125aa931-abf3-4b7f-a95d-e66dbd99025c@ideasonboard.com>\n\t<20250723164335.GA11202@pendragon.ideasonboard.com>","Subject":"Re: [PATCH v1 3/3] libcamera: base: log: Do not instantiate disabled\n\t`LogMessage`s","From":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tLaurent Pinchart ","Date":"Tue, 30 Sep 2025 11:37:09 +0100","Message-ID":"<175922862995.4000262.14562182531096651793@ping.linuxembedded.co.uk>","User-Agent":"alot/0.9.1","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]