[{"id":36593,"web_url":"https://patchwork.libcamera.org/comment/36593/","msgid":"<zlc75y2j4xa5kb55awuesilazrgm3buyw252g7mmcjvunzvjnf@eltjdlxevfld>","date":"2025-11-01T16:36:20","subject":"Re: [RFC PATCH v1 1/2] apps: lc-compliance: Commit camera\n\tconfiguration last","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Barnabás\n\nOn Tue, Jun 24, 2025 at 02:28:29PM +0200, Barnabás Pőcze wrote:\n> Save the result of `Camera::generateConfiguration()` in a local variable,\n> and only commit it to the member variable `config_` if it passes all checks.\n> This removes the need for explicit `config_.reset()` calls in error paths,\n> hence making things simpler.\n>\n> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>\n\nReviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>\n\nI think that you can push this patch\n\n> ---\n>  src/apps/lc-compliance/helpers/capture.cpp | 23 +++++++++-------------\n>  1 file changed, 9 insertions(+), 14 deletions(-)\n>\n> diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp\n> index 2a3fa3b68..d076e964a 100644\n> --- a/src/apps/lc-compliance/helpers/capture.cpp\n> +++ b/src/apps/lc-compliance/helpers/capture.cpp\n> @@ -27,35 +27,30 @@ void Capture::configure(libcamera::Span<const libcamera::StreamRole> roles)\n>  {\n>  \tassert(!roles.empty());\n>\n> -\tconfig_ = camera_->generateConfiguration(roles);\n> -\tif (!config_)\n> +\tauto config = camera_->generateConfiguration(roles);\n> +\tif (!config)\n>  \t\tGTEST_SKIP() << \"Roles not supported by camera\";\n>\n> -\tASSERT_EQ(config_->size(), roles.size()) << \"Unexpected number of streams in configuration\";\n> +\tASSERT_EQ(config->size(), roles.size()) << \"Unexpected number of streams in configuration\";\n>\n>  \t/*\n>  \t * Set the buffers count to the largest value across all streams.\n>  \t * \\todo: Should all streams from a Camera have the same buffer count ?\n>  \t */\n>  \tauto largest =\n> -\t\tstd::max_element(config_->begin(), config_->end(),\n> +\t\tstd::max_element(config->begin(), config->end(),\n>  \t\t\t\t [](const StreamConfiguration &l, const StreamConfiguration &r)\n>  \t\t\t\t { return l.bufferCount < r.bufferCount; });\n>\n> -\tassert(largest != config_->end());\n> +\tassert(largest != config->end());\n>\n> -\tfor (auto &cfg : *config_)\n> +\tfor (auto &cfg : *config)\n>  \t\tcfg.bufferCount = largest->bufferCount;\n>\n> -\tif (config_->validate() != CameraConfiguration::Valid) {\n> -\t\tconfig_.reset();\n> -\t\tFAIL() << \"Configuration not valid\";\n> -\t}\n> +\tASSERT_EQ(config->validate(), CameraConfiguration::Valid);\n> +\tASSERT_EQ(camera_->configure(config.get()), 0);\n>\n> -\tif (camera_->configure(config_.get())) {\n> -\t\tconfig_.reset();\n> -\t\tFAIL() << \"Failed to configure camera\";\n> -\t}\n> +\tconfig_ = std::move(config);\n>  }\n>\n>  void Capture::run(unsigned int captureLimit, std::optional<unsigned int> queueLimit)\n> --\n> 2.50.0\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 C0298C3241\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSat,  1 Nov 2025 16:36:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0E2AA60A85;\n\tSat,  1 Nov 2025 17:36:25 +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 440FB606D5\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat,  1 Nov 2025 17:36:23 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2001:b07:6462:5de2:153:f9b8:5024:faa2])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 99AF4BB;\n\tSat,  1 Nov 2025 17:34:31 +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=\"VXIH/Krr\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1762014871;\n\tbh=O1EOCSqLiguxOVmYz2qRdJmjniIoKxoyja/abg2N+wk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=VXIH/Krr7uzi1kEHs81YjJ5jmb6IRNERuOseLnGj/8BMShkTHSimXvqPp2LZc8Omi\n\twlzJeg1vMth9LWOM6SyTaYgGFNPvEan8jpvgZEXFpBbUOjBeJ5cxv+8qBVEzUZjH/n\n\tLktrCsOLIaOOP3miqXDQQX9mI5AqmB+mil6uGuL4=","Date":"Sat, 1 Nov 2025 17:36:20 +0100","From":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= <barnabas.pocze@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v1 1/2] apps: lc-compliance: Commit camera\n\tconfiguration last","Message-ID":"<zlc75y2j4xa5kb55awuesilazrgm3buyw252g7mmcjvunzvjnf@eltjdlxevfld>","References":"<20250624122830.726987-1-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":"<20250624122830.726987-1-barnabas.pocze@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":36654,"web_url":"https://patchwork.libcamera.org/comment/36654/","msgid":"<176217440594.39745.9718670581870803138@localhost>","date":"2025-11-03T12:53:25","subject":"Re: [RFC PATCH v1 1/2] apps: lc-compliance: Commit camera\n\tconfiguration last","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Barnabás,\n\nThank you for the patch.\n\nQuoting Barnabás Pőcze (2025-06-24 14:28:29)\n> Save the result of `Camera::generateConfiguration()` in a local variable,\n> and only commit it to the member variable `config_` if it passes all checks.\n> This removes the need for explicit `config_.reset()` calls in error paths,\n> hence making things simpler.\n> \n> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>\n> ---\n>  src/apps/lc-compliance/helpers/capture.cpp | 23 +++++++++-------------\n>  1 file changed, 9 insertions(+), 14 deletions(-)\n> \n> diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp\n> index 2a3fa3b68..d076e964a 100644\n> --- a/src/apps/lc-compliance/helpers/capture.cpp\n> +++ b/src/apps/lc-compliance/helpers/capture.cpp\n> @@ -27,35 +27,30 @@ void Capture::configure(libcamera::Span<const libcamera::StreamRole> roles)\n>  {\n>         assert(!roles.empty());\n>  \n> -       config_ = camera_->generateConfiguration(roles);\n> -       if (!config_)\n> +       auto config = camera_->generateConfiguration(roles);\n> +       if (!config)\n>                 GTEST_SKIP() << \"Roles not supported by camera\";\n>  \n> -       ASSERT_EQ(config_->size(), roles.size()) << \"Unexpected number of streams in configuration\";\n> +       ASSERT_EQ(config->size(), roles.size()) << \"Unexpected number of streams in configuration\";\n>  \n>         /*\n>          * Set the buffers count to the largest value across all streams.\n>          * \\todo: Should all streams from a Camera have the same buffer count ?\n>          */\n>         auto largest =\n> -               std::max_element(config_->begin(), config_->end(),\n> +               std::max_element(config->begin(), config->end(),\n>                                  [](const StreamConfiguration &l, const StreamConfiguration &r)\n>                                  { return l.bufferCount < r.bufferCount; });\n>  \n> -       assert(largest != config_->end());\n> +       assert(largest != config->end());\n>  \n> -       for (auto &cfg : *config_)\n> +       for (auto &cfg : *config)\n>                 cfg.bufferCount = largest->bufferCount;\n>  \n> -       if (config_->validate() != CameraConfiguration::Valid) {\n> -               config_.reset();\n> -               FAIL() << \"Configuration not valid\";\n> -       }\n> +       ASSERT_EQ(config->validate(), CameraConfiguration::Valid);\n> +       ASSERT_EQ(camera_->configure(config.get()), 0);\n\nActually I don't like this as it moves logic into an ASSERT. My\nassumptions was (maybe others see that differently) that ASSERTs could\n(in theory) be defined as noop for release builds and therefore must not\ncontain anything that needs to be called. Is my assumption wrong?\n\nBest regards,\nStefan\n\n>  \n> -       if (camera_->configure(config_.get())) {\n> -               config_.reset();\n> -               FAIL() << \"Failed to configure camera\";\n> -       }\n> +       config_ = std::move(config);\n>  }\n>  \n>  void Capture::run(unsigned int captureLimit, std::optional<unsigned int> queueLimit)\n> -- \n> 2.50.0\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 36009BDE4C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon,  3 Nov 2025 12:53:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1878E60A80;\n\tMon,  3 Nov 2025 13:53:31 +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 31C81606A0\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon,  3 Nov 2025 13:53:29 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:1b71:26a2:a362:3fd7])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 38E4799F;\n\tMon,  3 Nov 2025 13:51:36 +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=\"O6Gdr2qU\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1762174296;\n\tbh=RlEVWHiYZoJpQwuY/8RF8C33PIeEOF3riLx5RGO4PL0=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=O6Gdr2qUlduuOOkpWcvoZLu6ouWyT0S7dseyKYGz1YjiaLkL/QpPYeGQMflz5IqZg\n\tK/5BCCxbvngvygKzlwM4g6M1XYgxdGrtbgtA+mznDGuF6Dz3mwpNC2UwMH579RdKek\n\txI2zL9BFCJDp+2wnnpQwxAnIf67vI8hJZFU44+iM=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250624122830.726987-1-barnabas.pocze@ideasonboard.com>","References":"<20250624122830.726987-1-barnabas.pocze@ideasonboard.com>","Subject":"Re: [RFC PATCH v1 1/2] apps: lc-compliance: Commit camera\n\tconfiguration last","From":"Stefan Klug <stefan.klug@ideasonboard.com>","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= <barnabas.pocze@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 03 Nov 2025 13:53:25 +0100","Message-ID":"<176217440594.39745.9718670581870803138@localhost>","User-Agent":"alot/0.12.dev8+g2c003385c862.d20250602","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":36655,"web_url":"https://patchwork.libcamera.org/comment/36655/","msgid":"<f3582e33-1298-4279-b242-920b4846753b@ideasonboard.com>","date":"2025-11-03T13:00:57","subject":"Re: [RFC PATCH v1 1/2] apps: lc-compliance: Commit camera\n\tconfiguration last","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. 11. 03. 13:53 keltezéssel, Stefan Klug írta:\n> Hi Barnabás,\n> \n> Thank you for the patch.\n> \n> Quoting Barnabás Pőcze (2025-06-24 14:28:29)\n>> Save the result of `Camera::generateConfiguration()` in a local variable,\n>> and only commit it to the member variable `config_` if it passes all checks.\n>> This removes the need for explicit `config_.reset()` calls in error paths,\n>> hence making things simpler.\n>>\n>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>\n>> ---\n>>   src/apps/lc-compliance/helpers/capture.cpp | 23 +++++++++-------------\n>>   1 file changed, 9 insertions(+), 14 deletions(-)\n>>\n>> diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp\n>> index 2a3fa3b68..d076e964a 100644\n>> --- a/src/apps/lc-compliance/helpers/capture.cpp\n>> +++ b/src/apps/lc-compliance/helpers/capture.cpp\n>> @@ -27,35 +27,30 @@ void Capture::configure(libcamera::Span<const libcamera::StreamRole> roles)\n>>   {\n>>          assert(!roles.empty());\n>>   \n>> -       config_ = camera_->generateConfiguration(roles);\n>> -       if (!config_)\n>> +       auto config = camera_->generateConfiguration(roles);\n>> +       if (!config)\n>>                  GTEST_SKIP() << \"Roles not supported by camera\";\n>>   \n>> -       ASSERT_EQ(config_->size(), roles.size()) << \"Unexpected number of streams in configuration\";\n>> +       ASSERT_EQ(config->size(), roles.size()) << \"Unexpected number of streams in configuration\";\n>>   \n>>          /*\n>>           * Set the buffers count to the largest value across all streams.\n>>           * \\todo: Should all streams from a Camera have the same buffer count ?\n>>           */\n>>          auto largest =\n>> -               std::max_element(config_->begin(), config_->end(),\n>> +               std::max_element(config->begin(), config->end(),\n>>                                   [](const StreamConfiguration &l, const StreamConfiguration &r)\n>>                                   { return l.bufferCount < r.bufferCount; });\n>>   \n>> -       assert(largest != config_->end());\n>> +       assert(largest != config->end());\n>>   \n>> -       for (auto &cfg : *config_)\n>> +       for (auto &cfg : *config)\n>>                  cfg.bufferCount = largest->bufferCount;\n>>   \n>> -       if (config_->validate() != CameraConfiguration::Valid) {\n>> -               config_.reset();\n>> -               FAIL() << \"Configuration not valid\";\n>> -       }\n>> +       ASSERT_EQ(config->validate(), CameraConfiguration::Valid);\n>> +       ASSERT_EQ(camera_->configure(config.get()), 0);\n> \n> Actually I don't like this as it moves logic into an ASSERT. My\n> assumptions was (maybe others see that differently) that ASSERTs could\n> (in theory) be defined as noop for release builds and therefore must not\n> contain anything that needs to be called. Is my assumption wrong?\n\nAs far as I am aware gtest never defines its ASSERT/EXPECT macros as no-ops.\nIn fact, I would be very surprised if it did.\n\nFor `assert()` from `<cassert>`, etc. I agree completely.\n\n\n> \n> Best regards,\n> Stefan\n> \n>>   \n>> -       if (camera_->configure(config_.get())) {\n>> -               config_.reset();\n>> -               FAIL() << \"Failed to configure camera\";\n>> -       }\n>> +       config_ = std::move(config);\n>>   }\n>>   \n>>   void Capture::run(unsigned int captureLimit, std::optional<unsigned int> queueLimit)\n>> -- \n>> 2.50.0\n>>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 582EFC3241\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon,  3 Nov 2025 13:01:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 79D1860A80;\n\tMon,  3 Nov 2025 14:01:02 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 62270606A0\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon,  3 Nov 2025 14:01:01 +0100 (CET)","from [192.168.33.39] (185.221.140.239.nat.pool.zt.hu\n\t[185.221.140.239])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 55A5411D5;\n\tMon,  3 Nov 2025 13:59:08 +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=\"mP7WJ1on\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1762174748;\n\tbh=xGIsK5oFoGrQJZWkuUap1t/3egYdni3ZelTJ2d/F3rc=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=mP7WJ1onoooUPZen3nYSiT6VatIpiR0xnSZdQD7ESlq9E35YQsr88OYXrcHa2bWM0\n\tTLG3KXfbMdH5ost3dnxauW+7rX+ckAHnwluSBWkNQCIpitl9JBaeth2vx647QY5WtN\n\t02RH0cG7ZHZB5dIAWLoUz0QfUzlU2DrQHMWDC0g8=","Message-ID":"<f3582e33-1298-4279-b242-920b4846753b@ideasonboard.com>","Date":"Mon, 3 Nov 2025 14:00:57 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [RFC PATCH v1 1/2] apps: lc-compliance: Commit camera\n\tconfiguration last","To":"Stefan Klug <stefan.klug@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","References":"<20250624122830.726987-1-barnabas.pocze@ideasonboard.com>\n\t<176217440594.39745.9718670581870803138@localhost>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= <barnabas.pocze@ideasonboard.com>","Content-Language":"en-US, hu-HU","In-Reply-To":"<176217440594.39745.9718670581870803138@localhost>","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":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":36656,"web_url":"https://patchwork.libcamera.org/comment/36656/","msgid":"<176217570169.39745.13935461164547514927@localhost>","date":"2025-11-03T13:15:01","subject":"Re: [RFC PATCH v1 1/2] apps: lc-compliance: Commit camera\n\tconfiguration last","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi,\n\nQuoting Barnabás Pőcze (2025-11-03 14:00:57)\n> Hi\n> \n> 2025. 11. 03. 13:53 keltezéssel, Stefan Klug írta:\n> > Hi Barnabás,\n> > \n> > Thank you for the patch.\n> > \n> > Quoting Barnabás Pőcze (2025-06-24 14:28:29)\n> >> Save the result of `Camera::generateConfiguration()` in a local variable,\n> >> and only commit it to the member variable `config_` if it passes all checks.\n> >> This removes the need for explicit `config_.reset()` calls in error paths,\n> >> hence making things simpler.\n> >>\n> >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>\n> >> ---\n> >>   src/apps/lc-compliance/helpers/capture.cpp | 23 +++++++++-------------\n> >>   1 file changed, 9 insertions(+), 14 deletions(-)\n> >>\n> >> diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp\n> >> index 2a3fa3b68..d076e964a 100644\n> >> --- a/src/apps/lc-compliance/helpers/capture.cpp\n> >> +++ b/src/apps/lc-compliance/helpers/capture.cpp\n> >> @@ -27,35 +27,30 @@ void Capture::configure(libcamera::Span<const libcamera::StreamRole> roles)\n> >>   {\n> >>          assert(!roles.empty());\n> >>   \n> >> -       config_ = camera_->generateConfiguration(roles);\n> >> -       if (!config_)\n> >> +       auto config = camera_->generateConfiguration(roles);\n> >> +       if (!config)\n> >>                  GTEST_SKIP() << \"Roles not supported by camera\";\n> >>   \n> >> -       ASSERT_EQ(config_->size(), roles.size()) << \"Unexpected number of streams in configuration\";\n> >> +       ASSERT_EQ(config->size(), roles.size()) << \"Unexpected number of streams in configuration\";\n> >>   \n> >>          /*\n> >>           * Set the buffers count to the largest value across all streams.\n> >>           * \\todo: Should all streams from a Camera have the same buffer count ?\n> >>           */\n> >>          auto largest =\n> >> -               std::max_element(config_->begin(), config_->end(),\n> >> +               std::max_element(config->begin(), config->end(),\n> >>                                   [](const StreamConfiguration &l, const StreamConfiguration &r)\n> >>                                   { return l.bufferCount < r.bufferCount; });\n> >>   \n> >> -       assert(largest != config_->end());\n> >> +       assert(largest != config->end());\n> >>   \n> >> -       for (auto &cfg : *config_)\n> >> +       for (auto &cfg : *config)\n> >>                  cfg.bufferCount = largest->bufferCount;\n> >>   \n> >> -       if (config_->validate() != CameraConfiguration::Valid) {\n> >> -               config_.reset();\n> >> -               FAIL() << \"Configuration not valid\";\n> >> -       }\n> >> +       ASSERT_EQ(config->validate(), CameraConfiguration::Valid);\n> >> +       ASSERT_EQ(camera_->configure(config.get()), 0);\n> > \n> > Actually I don't like this as it moves logic into an ASSERT. My\n> > assumptions was (maybe others see that differently) that ASSERTs could\n> > (in theory) be defined as noop for release builds and therefore must not\n> > contain anything that needs to be called. Is my assumption wrong?\n> \n> As far as I am aware gtest never defines its ASSERT/EXPECT macros as no-ops.\n> In fact, I would be very surprised if it did.\n> \n> For `assert()` from `<cassert>`, etc. I agree completely.\n\nOops, I completely missed that this is gtest and my brain just jumped on\nupper case letters. In that case\n\nReviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>\n\nCheers,\nStefan\n\n> \n> \n> > \n> > Best regards,\n> > Stefan\n> > \n> >>   \n> >> -       if (camera_->configure(config_.get())) {\n> >> -               config_.reset();\n> >> -               FAIL() << \"Failed to configure camera\";\n> >> -       }\n> >> +       config_ = std::move(config);\n> >>   }\n> >>   \n> >>   void Capture::run(unsigned int captureLimit, std::optional<unsigned int> queueLimit)\n> >> -- \n> >> 2.50.0\n> >>\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 00DA7BDE4C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon,  3 Nov 2025 13:15:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3373E60A80;\n\tMon,  3 Nov 2025 14:15:06 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8C694606A0\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon,  3 Nov 2025 14:15:04 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:1b71:26a2:a362:3fd7])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 8C74F8BE;\n\tMon,  3 Nov 2025 14:13:11 +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=\"J94fkxRU\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1762175591;\n\tbh=B0D0JLXUHU9gmkyuhrRpdRhc5BcyicL7Tvyp7ikZ85w=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=J94fkxRUqGw122XucanEKCBTu9eAHTpjtwY6ixVLCNsVqFcWdWL3Gy+Svoi4jq2a5\n\tDm5WFwFcXgkq22WX6+8HVCKLuteLqEXluz7Uu00306tyVTH0RvxhALwvpxYTJsAqRP\n\tX17ClGyHSRcA7UD/OKAYQfKnCHIKOaRHT+hgvwBU=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<f3582e33-1298-4279-b242-920b4846753b@ideasonboard.com>","References":"<20250624122830.726987-1-barnabas.pocze@ideasonboard.com>\n\t<176217440594.39745.9718670581870803138@localhost>\n\t<f3582e33-1298-4279-b242-920b4846753b@ideasonboard.com>","Subject":"Re: [RFC PATCH v1 1/2] apps: lc-compliance: Commit camera\n\tconfiguration last","From":"Stefan Klug <stefan.klug@ideasonboard.com>","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= <barnabas.pocze@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 03 Nov 2025 14:15:01 +0100","Message-ID":"<176217570169.39745.13935461164547514927@localhost>","User-Agent":"alot/0.12.dev8+g2c003385c862.d20250602","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]