[{"id":36001,"web_url":"https://patchwork.libcamera.org/comment/36001/","msgid":"<72d0879e-6b58-476c-a38c-5ebed85c85e3@ideasonboard.com>","date":"2025-09-26T14:59:04","subject":"Re: [PATCH v4 2/7] libcamera: software_isp: Clarify\n\tSwStatsCpu::setWindow use","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. 09. 25. 21:28 keltezéssel, Milan Zamazal írta:\n> The window coordinates passed to SwStatsCpu::setWindow are confusing.\n> Let's clarify what the coordinates should be.\n> \n> A source of confusion is that the specified window is relative to the\n> processed area. Debayering adjusts line pointers for its processed area\n> and this is what's also passed to stats processing. The window passed\n> to SwStatsCpu::setWindow should either specify the size of the whole\n> processed (not image) area, or its cropping in case the stats shouldn't\n> be gathered over the whole processed area. This patch should clarify\n> this in the code.\n> \n> Reviewed-by: Maciej S. Szmigiero \n> Signed-off-by: Milan Zamazal \n> ---\n> src/libcamera/software_isp/debayer_cpu.cpp | 6 +++++-\n> src/libcamera/software_isp/swstats_cpu.cpp | 19 +++++++++++++++++++\n> 2 files changed, 24 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n> index 2dc85e5e0..bcaaa5dee 100644\n> --- a/src/libcamera/software_isp/debayer_cpu.cpp\n> +++ b/src/libcamera/software_isp/debayer_cpu.cpp\n> @@ -554,7 +554,11 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg,\n> \twindow_.width = outputCfg.size.width;\n> \twindow_.height = outputCfg.size.height;\n> \n> -\t/* Don't pass x,y since process() already adjusts src before passing it */\n> +\t/*\n> +\t * Set the stats window to the whole processed window. Its coordinates are\n> +\t * relative to the debayered area since debayering passes only the part of\n> +\t * data to be processed to the stats; see SwStatsCpu::setWindow.\n> +\t */\n> \tstats_->setWindow(Rectangle(window_.size()));\n> \n> \t/* pad with patternSize.Width on both left and right side */\n> diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp\n> index e8a1d52f2..c936ef1dc 100644\n> --- a/src/libcamera/software_isp/swstats_cpu.cpp\n> +++ b/src/libcamera/software_isp/swstats_cpu.cpp\n> @@ -416,6 +416,25 @@ int SwStatsCpu::configure(const StreamConfiguration &inputCfg)\n> /**\n> * \\brief Specify window coordinates over which to gather statistics\n> * \\param[in] window The window object.\n> + *\n> + * This method specifies the image area over which to gather the statistics.\n> + * It must be called to set the area, otherwise the default zero-sized\n> + * \\a Rectangle is used and no statistics is gathered.\n> + *\n> + * The specified \\a window is relative to what is passed to processLine*\n> + * methods. Typically, this means processLine* methods get only data from the\n> + * processed area and \\a window is \\a Rectangle with (0, 0) top-left point and\n> + * of the same size as the processed area. But if statistics is gathered only\n> + * from some part of the image, e.g. its centre, \\a window should specify such a\n> + * restriction accordingly.\n> + *\n> + * It is the responsibility of the callers to provide sensible \\a window values,\n> + * most notably not exceeding the original image boundaries.\n> + *\n> + * The method may adjust the window slightly if it is not aligned according to\n> + * the bayer pattern determined in \\a SwStatsCpu::configure(). If that happens,\n> + * it is guaranteed that non-negative top-left point coordinates remain\n> + * non-negative and that the window width and height don't get enlarged.\n\nWhy mention \"non-negative\"? I feel like, if anything, we should disallow negative\ncoordinates for the top left corner altogether.\n\n\nRegards,\nBarnabás Pőcze\n\n\n> */\n> void SwStatsCpu::setWindow(const Rectangle &window)\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 272F1C328C\n\tfor ;\n\tFri, 26 Sep 2025 14:59:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 21B3C6B5F3;\n\tFri, 26 Sep 2025 16:59:10 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0F342613AB\n\tfor ;\n\tFri, 26 Sep 2025 16:59:08 +0200 (CEST)","from [192.168.33.19] (185.221.140.70.nat.pool.zt.hu\n\t[185.221.140.70])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7DC5114B0;\n\tFri, 26 Sep 2025 16:57:42 +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=\"ozapZzSS\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1758898662;\n\tbh=6UPnIr/FeXuKyghru06iqVAWwb5Hk+KbAiqLQSZR81Q=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=ozapZzSSW2AJyfqByFoVSlIA82B0K8IE5emgifP8c80KBkgqqzEEctoaPSciEaJbI\n\tx6KNyLMvz/G+1SHDYODxKK0cJ/Wz+HQkRwKhEYX0P3Poa4dZ81a8OdgNlXZ2NtzEWG\n\t8GkyMT59bdepTSwWTuEokou+PfQ4bNh+VoEQi+4w=","Message-ID":"<72d0879e-6b58-476c-a38c-5ebed85c85e3@ideasonboard.com>","Date":"Fri, 26 Sep 2025 16:59:04 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v4 2/7] libcamera: software_isp: Clarify\n\tSwStatsCpu::setWindow use","To":"Milan Zamazal , libcamera-devel@lists.libcamera.org","Cc":"mail@maciej.szmigiero.name","References":"<20250925192856.77881-1-mzamazal@redhat.com>\n\t<20250925192856.77881-3-mzamazal@redhat.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<20250925192856.77881-3-mzamazal@redhat.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":36002,"web_url":"https://patchwork.libcamera.org/comment/36002/","msgid":"<85qzvtkz17.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","date":"2025-09-26T15:35:00","subject":"Re: [PATCH v4 2/7] libcamera: software_isp: Clarify\n\tSwStatsCpu::setWindow use","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Barnabás Pőcze writes:\n\n> Hi\n>\n> 2025. 09. 25. 21:28 keltezéssel, Milan Zamazal írta:\n>> The window coordinates passed to SwStatsCpu::setWindow are confusing.\n>> Let's clarify what the coordinates should be.\n>> A source of confusion is that the specified window is relative to the\n>> processed area. Debayering adjusts line pointers for its processed area\n>> and this is what's also passed to stats processing. The window passed\n>> to SwStatsCpu::setWindow should either specify the size of the whole\n>> processed (not image) area, or its cropping in case the stats shouldn't\n>> be gathered over the whole processed area. This patch should clarify\n>> this in the code.\n>> Reviewed-by: Maciej S. Szmigiero \n>> Signed-off-by: Milan Zamazal \n>> ---\n>> src/libcamera/software_isp/debayer_cpu.cpp | 6 +++++-\n>> src/libcamera/software_isp/swstats_cpu.cpp | 19 +++++++++++++++++++\n>> 2 files changed, 24 insertions(+), 1 deletion(-)\n>> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n>> index 2dc85e5e0..bcaaa5dee 100644\n>> --- a/src/libcamera/software_isp/debayer_cpu.cpp\n>> +++ b/src/libcamera/software_isp/debayer_cpu.cpp\n>> @@ -554,7 +554,11 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg,\n>> \twindow_.width = outputCfg.size.width;\n>> \twindow_.height = outputCfg.size.height;\n>> -\t/* Don't pass x,y since process() already adjusts src before passing it */\n>> +\t/*\n>> +\t * Set the stats window to the whole processed window. Its coordinates are\n>> +\t * relative to the debayered area since debayering passes only the part of\n>> +\t * data to be processed to the stats; see SwStatsCpu::setWindow.\n>> +\t */\n>> \tstats_->setWindow(Rectangle(window_.size()));\n>> \t/* pad with patternSize.Width on both left and right side */\n>> diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp\n>> index e8a1d52f2..c936ef1dc 100644\n>> --- a/src/libcamera/software_isp/swstats_cpu.cpp\n>> +++ b/src/libcamera/software_isp/swstats_cpu.cpp\n>> @@ -416,6 +416,25 @@ int SwStatsCpu::configure(const StreamConfiguration &inputCfg)\n>> /**\n>> * \\brief Specify window coordinates over which to gather statistics\n>> * \\param[in] window The window object.\n>> + *\n>> + * This method specifies the image area over which to gather the statistics.\n>> + * It must be called to set the area, otherwise the default zero-sized\n>> + * \\a Rectangle is used and no statistics is gathered.\n>> + *\n>> + * The specified \\a window is relative to what is passed to processLine*\n>> + * methods. Typically, this means processLine* methods get only data from the\n>> + * processed area and \\a window is \\a Rectangle with (0, 0) top-left point and\n>> + * of the same size as the processed area. But if statistics is gathered only\n>> + * from some part of the image, e.g. its centre, \\a window should specify such a\n>> + * restriction accordingly.\n>> + *\n>> + * It is the responsibility of the callers to provide sensible \\a window values,\n>> + * most notably not exceeding the original image boundaries.\n>> + *\n>> + * The method may adjust the window slightly if it is not aligned according to\n>> + * the bayer pattern determined in \\a SwStatsCpu::configure(). If that happens,\n>> + * it is guaranteed that non-negative top-left point coordinates remain\n>> + * non-negative and that the window width and height don't get enlarged.\n>\n> Why mention \"non-negative\"? \n\nIt describes the property of the implementation. Without it, the\nstatement would be false.\n\n> I feel like, if anything, we should disallow negative coordinates for\n> the top left corner altogether.\n\nThe method doesn't provide a mechanism to disallow insane `window'\nvalues (other than to change the values completely) and I don't think\nit's needed for such a very internal call. I think the preceding\nparagraph about the caller responsibility is enough; the implementation\nis sufficient for its purpose and the primary intended purpose of the\ndocstring is to make the life of readers of the code easier\n(relative/absolute confusion etc.) rather than to define a robust API.\n\n> Regards,\n> Barnabás Pőcze\n>\n>\n>> */\n>> void SwStatsCpu::setWindow(const Rectangle &window)\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 02F68BDB1C\n\tfor ;\n\tFri, 26 Sep 2025 15:35:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5F2D76B5F3;\n\tFri, 26 Sep 2025 17:35:09 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 64B32613AB\n\tfor ;\n\tFri, 26 Sep 2025 17:35:07 +0200 (CEST)","from mail-wm1-f72.google.com (mail-wm1-f72.google.com\n\t[209.85.128.72]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-325-_Sr8DuDqNqmNvwolbrwa1w-1; Fri, 26 Sep 2025 11:35:04 -0400","by mail-wm1-f72.google.com with SMTP id\n\t5b1f17b1804b1-45e05ff0b36so14828635e9.0\n\tfor ;\n\tFri, 26 Sep 2025 08:35:04 -0700 (PDT)","from mzamazal-thinkpadp1gen7.tpbc.csb\n\t(ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\tffacd0b85a97d-40fb72fb71esm7385013f8f.1.2025.09.26.08.35.01\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tFri, 26 Sep 2025 08:35:01 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"YxDO3CQf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1758900906;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tcontent-transfer-encoding:content-transfer-encoding:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=py4f7RrM3ok61yV7YUCvGxhMJOf6H5lzT41NHgswIiE=;\n\tb=YxDO3CQfE5eor2JurZ6+f8LkXucDDHAZa0WjO6aAkrR6ZPjfgt6X1jXw2PGvE9Ofmfm19Y\n\tbthlGUk0rr3t7QUDDc5cd6pICwnlLo/3w1VA6WZnwHUGJfPnP1szCX/KeTA+AFkstz2UKc\n\thGXuVn0SUASQ3bw5q4fdIEmZF4vIQ34=","X-MC-Unique":"_Sr8DuDqNqmNvwolbrwa1w-1","X-Mimecast-MFC-AGG-ID":"_Sr8DuDqNqmNvwolbrwa1w_1758900903","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1758900903; x=1759505703;\n\th=content-transfer-encoding:mime-version:user-agent:message-id:date\n\t:references:in-reply-to:subject:cc:to:from:x-gm-message-state:from\n\t:to:cc:subject:date:message-id:reply-to;\n\tbh=+GTrzkb7CtbKWQnqHvFsgp061QkFo/NWuG9Dpx3X7lY=;\n\tb=wmb8D4y4FUfzOEAKhiEoAegGZHlBNV3jPJzNkCk8pmkUwt8YNvmHTX3pC1zuWNge3A\n\tx+hk2IYqb8ZZ2lq7PjRxbi2h7SgSHPX7MC91pME3ULlMdCT4rs61dlzEhWeNG+RPFKtQ\n\taFGx2iXCfig7wxcd49qz+NioOlK08+2iy4F/kxYcRxuamWzzXs3MU0gaftT3CuOqFIf5\n\tScgNC6bSUgifTp6/RfWu7IheSt8ED525jNAV+Dodz2ZvmPGu00aa1yuJsxYXBluKd6iu\n\tM99HKOoPoIry9OLpXtzTNrsb38ianH5dFIGDh4WzJJmpo+XMlgU1yhBy0Bu/H0qgDRgt\n\tnUsA==","X-Gm-Message-State":"AOJu0Yyv6qt5Vf+UxOek5ebraxGZGhY9obLgvr/o9g8udZ4WOIDuBDjf\n\t018yOuOLgnqTSzAFT25lhS6SdkdsWlkzda3/x+cfMgRa5W2x1ddGWK5bh3SkPN25CheOppwB2gJ\n\tQtAPoQbTQXFkOd4RnTs4RBVRKZbInf6ohvwf7BIsojI+0F5BeLIPOxcpKP/Lc7MiIxOtpeIYm8G\n\t/DfRSUBO4=","X-Gm-Gg":"ASbGncsx/fHt3cG3vywbcsFpVGhUqStMVV3mG0978/QI+DgxOtCkvwuvgRNp3cPKpZo\n\txk0JU03JjX0QD6JAwQselhXx1XjgtPHlwHdJTFf6J/8lByvDj3p5pe8xePdiuz148ExzTq4LSqm\n\t/sCah/DZstxhuBlIL4LlpRAfpeRwaaHFB8EYTSkYkLNJl2uV0F+ZpI2fmTY9kqOSeBW+m/3/3mn\n\tyS5Bprih1OEvSpbkql79Km+GsYB4utrE2l8wBdFeSatCefGoZGpb7xlXOiRml1xPMfPq5+0fJVQ\n\tigzmT/eC7CMud8aa47ql4xsngD2smQlPReEZT007wF/NRn603bBIdFkIjjsOFrfPApanyEd3I8q\n\t0N95PE/oxmFdR6qNGRg==","X-Received":["by 2002:a05:600c:4754:b0:45b:8ac2:9761 with SMTP id\n\t5b1f17b1804b1-46e33c8d50bmr76770965e9.13.1758900902639; \n\tFri, 26 Sep 2025 08:35:02 -0700 (PDT)","by 2002:a05:600c:4754:b0:45b:8ac2:9761 with SMTP id\n\t5b1f17b1804b1-46e33c8d50bmr76770545e9.13.1758900902094; \n\tFri, 26 Sep 2025 08:35:02 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IHbr+MbOv7AEj2yz6lQkajSFN0pDvhsQxaaW++wlUbr9kJIXBuhTx2NPwwrs05vp58V3JoquQ==","From":"Milan Zamazal ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org, mail@maciej.szmigiero.name","Subject":"Re: [PATCH v4 2/7] libcamera: software_isp: Clarify\n\tSwStatsCpu::setWindow use","In-Reply-To":"<72d0879e-6b58-476c-a38c-5ebed85c85e3@ideasonboard.com> (\n\t=?utf-8?b?IkJhcm5hYsOhcyBQxZFjemUiJ3M=?= message of \"Fri,\n\t26 Sep 2025 16:59:04 +0200\")","References":"<20250925192856.77881-1-mzamazal@redhat.com>\n\t<20250925192856.77881-3-mzamazal@redhat.com>\n\t<72d0879e-6b58-476c-a38c-5ebed85c85e3@ideasonboard.com>","Date":"Fri, 26 Sep 2025 17:35:00 +0200","Message-ID":"<85qzvtkz17.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"cbb34HIaFWXZLTx4AjnZbVOPicnR4aqDcqhYTVD39i0_1758900903","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","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":36023,"web_url":"https://patchwork.libcamera.org/comment/36023/","msgid":"<8428ce59-13bf-48f0-8589-0bd5f3b1bbc3@ideasonboard.com>","date":"2025-09-29T10:36:22","subject":"Re: [PATCH v4 2/7] libcamera: software_isp: Clarify\n\tSwStatsCpu::setWindow use","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"2025. 09. 26. 17:35 keltezéssel, Milan Zamazal írta:\n> Barnabás Pőcze writes:\n> \n>> Hi\n>>\n>> 2025. 09. 25. 21:28 keltezéssel, Milan Zamazal írta:\n>>> The window coordinates passed to SwStatsCpu::setWindow are confusing.\n>>> Let's clarify what the coordinates should be.\n>>> A source of confusion is that the specified window is relative to the\n>>> processed area. Debayering adjusts line pointers for its processed area\n>>> and this is what's also passed to stats processing. The window passed\n>>> to SwStatsCpu::setWindow should either specify the size of the whole\n>>> processed (not image) area, or its cropping in case the stats shouldn't\n>>> be gathered over the whole processed area. This patch should clarify\n>>> this in the code.\n>>> Reviewed-by: Maciej S. Szmigiero \n>>> Signed-off-by: Milan Zamazal \n>>> ---\n>>> src/libcamera/software_isp/debayer_cpu.cpp | 6 +++++-\n>>> src/libcamera/software_isp/swstats_cpu.cpp | 19 +++++++++++++++++++\n>>> 2 files changed, 24 insertions(+), 1 deletion(-)\n>>> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n>>> index 2dc85e5e0..bcaaa5dee 100644\n>>> --- a/src/libcamera/software_isp/debayer_cpu.cpp\n>>> +++ b/src/libcamera/software_isp/debayer_cpu.cpp\n>>> @@ -554,7 +554,11 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg,\n>>> \twindow_.width = outputCfg.size.width;\n>>> \twindow_.height = outputCfg.size.height;\n>>> -\t/* Don't pass x,y since process() already adjusts src before passing it */\n>>> +\t/*\n>>> +\t * Set the stats window to the whole processed window. Its coordinates are\n>>> +\t * relative to the debayered area since debayering passes only the part of\n>>> +\t * data to be processed to the stats; see SwStatsCpu::setWindow.\n>>> +\t */\n>>> \tstats_->setWindow(Rectangle(window_.size()));\n>>> \t/* pad with patternSize.Width on both left and right side */\n>>> diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp\n>>> index e8a1d52f2..c936ef1dc 100644\n>>> --- a/src/libcamera/software_isp/swstats_cpu.cpp\n>>> +++ b/src/libcamera/software_isp/swstats_cpu.cpp\n>>> @@ -416,6 +416,25 @@ int SwStatsCpu::configure(const StreamConfiguration &inputCfg)\n>>> /**\n>>> * \\brief Specify window coordinates over which to gather statistics\n>>> * \\param[in] window The window object.\n>>> + *\n>>> + * This method specifies the image area over which to gather the statistics.\n>>> + * It must be called to set the area, otherwise the default zero-sized\n>>> + * \\a Rectangle is used and no statistics is gathered.\n>>> + *\n>>> + * The specified \\a window is relative to what is passed to processLine*\n>>> + * methods. Typically, this means processLine* methods get only data from the\n>>> + * processed area and \\a window is \\a Rectangle with (0, 0) top-left point and\n>>> + * of the same size as the processed area. But if statistics is gathered only\n>>> + * from some part of the image, e.g. its centre, \\a window should specify such a\n>>> + * restriction accordingly.\n\nThe second sentence is a bit hard to parse for me, or rather, I think it would be\nclearer to mention the typical case as an explicit example:\n\n The specified \\a window is relative to what is passed to the processLine* methods. For example,\n if statistics are to be gathered from the entire processed area, then \\a window should be a\n rectangle with the top-left corner of (0,0) and the same size as the processed area. If only\n a part of the processed area (e.g. its centre) is to be considered for statistics, then \\a window\n should specify such a restriction accordingly.\n\nThoughts?\n\n\n>>> + *\n>>> + * It is the responsibility of the callers to provide sensible \\a window values,\n>>> + * most notably not exceeding the original image boundaries.\n>>> + *\n>>> + * The method may adjust the window slightly if it is not aligned according to\n>>> + * the bayer pattern determined in \\a SwStatsCpu::configure(). If that happens,\n>>> + * it is guaranteed that non-negative top-left point coordinates remain\n>>> + * non-negative and that the window width and height don't get enlarged.\n>>\n>> Why mention \"non-negative\"?\n> \n> It describes the property of the implementation. Without it, the\n> statement would be false.\n\nNegative y coordinates will stay negative, negative x coordinates will\nstay negative (assuming patternSize_.{width,height} > 1). In any case,\nI just feel like mentioning \"non-negative\" will immediately make the\nreader wonder \"but what about negative numbers?\", at least it did to me.\n\n\n> \n>> I feel like, if anything, we should disallow negative coordinates for\n>> the top left corner altogether.\n> \n> The method doesn't provide a mechanism to disallow insane `window'\n> values (other than to change the values completely) and I don't think\n> it's needed for such a very internal call. I think the preceding\n\nI was thinking of something like an ASSERT().\n\n\n> paragraph about the caller responsibility is enough; the implementation\n> is sufficient for its purpose and the primary intended purpose of the\n> docstring is to make the life of readers of the code easier\n> (relative/absolute confusion etc.) rather than to define a robust API.\n\nOkay, let's not complicate things.\n\n\nRegards,\nBarnabás Pőcze\n\n\n> \n>> Regards,\n>> Barnabás Pőcze\n>>\n>>\n>>> */\n>>> void SwStatsCpu::setWindow(const Rectangle &window)\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 5BE97C328C\n\tfor ;\n\tMon, 29 Sep 2025 10:36:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 699CC6B5F3;\n\tMon, 29 Sep 2025 12:36:27 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4F8946B599\n\tfor ;\n\tMon, 29 Sep 2025 12:36:25 +0200 (CEST)","from [192.168.33.13] (185.221.142.146.nat.pool.zt.hu\n\t[185.221.142.146])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A5A656BE;\n\tMon, 29 Sep 2025 12:34:57 +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=\"XngMabgH\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1759142097;\n\tbh=X1VEJ/zFQUfLzxmpeEQ2Wt1CiuSiMiJym6tqBI8hHbY=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=XngMabgH9xDSVyUbGYH5LaqYLtvMqmvPtkxvttOuOPOqWkebFIbJEeKaCedYj50l2\n\t9hRJp6JZMlxtIjkpLWSWAyii5mcqSvrZ6Y7UP1HKOmvYmeg5tiK69z//C3n34tJ0ea\n\tkjGeZuFMDK4GJ8NbxIKm0ujIHQ0lbAkc6/KcYnz0=","Message-ID":"<8428ce59-13bf-48f0-8589-0bd5f3b1bbc3@ideasonboard.com>","Date":"Mon, 29 Sep 2025 12:36:22 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v4 2/7] libcamera: software_isp: Clarify\n\tSwStatsCpu::setWindow use","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org, mail@maciej.szmigiero.name","References":"<20250925192856.77881-1-mzamazal@redhat.com>\n\t<20250925192856.77881-3-mzamazal@redhat.com>\n\t<72d0879e-6b58-476c-a38c-5ebed85c85e3@ideasonboard.com>\n\t<85qzvtkz17.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<85qzvtkz17.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","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":36025,"web_url":"https://patchwork.libcamera.org/comment/36025/","msgid":"","date":"2025-09-29T11:12:38","subject":"Re: [PATCH v4 2/7] libcamera: software_isp: Clarify\n\tSwStatsCpu::setWindow use","submitter":{"id":239,"url":"https://patchwork.libcamera.org/api/people/239/","name":"Hans de Goede","email":"hansg@kernel.org"},"content":"Hi,\n\nOn 25-Sep-25 21:28, Milan Zamazal wrote:\n> The window coordinates passed to SwStatsCpu::setWindow are confusing.\n> Let's clarify what the coordinates should be.\n> \n> A source of confusion is that the specified window is relative to the\n> processed area. Debayering adjusts line pointers for its processed area\n> and this is what's also passed to stats processing. The window passed\n> to SwStatsCpu::setWindow should either specify the size of the whole\n> processed (not image) area, or its cropping in case the stats shouldn't\n> be gathered over the whole processed area. This patch should clarify\n> this in the code.\n> \n> Reviewed-by: Maciej S. Szmigiero \n> Signed-off-by: Milan Zamazal \n\nThanks, patch looks good to me:\n\nReviewed-by: Hans de Goede \n\nRegards,\n\nHans\n\n\n> ---\n> src/libcamera/software_isp/debayer_cpu.cpp | 6 +++++-\n> src/libcamera/software_isp/swstats_cpu.cpp | 19 +++++++++++++++++++\n> 2 files changed, 24 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n> index 2dc85e5e0..bcaaa5dee 100644\n> --- a/src/libcamera/software_isp/debayer_cpu.cpp\n> +++ b/src/libcamera/software_isp/debayer_cpu.cpp\n> @@ -554,7 +554,11 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg,\n> \twindow_.width = outputCfg.size.width;\n> \twindow_.height = outputCfg.size.height;\n> \n> -\t/* Don't pass x,y since process() already adjusts src before passing it */\n> +\t/*\n> +\t * Set the stats window to the whole processed window. Its coordinates are\n> +\t * relative to the debayered area since debayering passes only the part of\n> +\t * data to be processed to the stats; see SwStatsCpu::setWindow.\n> +\t */\n> \tstats_->setWindow(Rectangle(window_.size()));\n> \n> \t/* pad with patternSize.Width on both left and right side */\n> diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp\n> index e8a1d52f2..c936ef1dc 100644\n> --- a/src/libcamera/software_isp/swstats_cpu.cpp\n> +++ b/src/libcamera/software_isp/swstats_cpu.cpp\n> @@ -416,6 +416,25 @@ int SwStatsCpu::configure(const StreamConfiguration &inputCfg)\n> /**\n> * \\brief Specify window coordinates over which to gather statistics\n> * \\param[in] window The window object.\n> + *\n> + * This method specifies the image area over which to gather the statistics.\n> + * It must be called to set the area, otherwise the default zero-sized\n> + * \\a Rectangle is used and no statistics is gathered.\n> + *\n> + * The specified \\a window is relative to what is passed to processLine*\n> + * methods. Typically, this means processLine* methods get only data from the\n> + * processed area and \\a window is \\a Rectangle with (0, 0) top-left point and\n> + * of the same size as the processed area. But if statistics is gathered only\n> + * from some part of the image, e.g. its centre, \\a window should specify such a\n> + * restriction accordingly.\n> + *\n> + * It is the responsibility of the callers to provide sensible \\a window values,\n> + * most notably not exceeding the original image boundaries.\n> + *\n> + * The method may adjust the window slightly if it is not aligned according to\n> + * the bayer pattern determined in \\a SwStatsCpu::configure(). If that happens,\n> + * it is guaranteed that non-negative top-left point coordinates remain\n> + * non-negative and that the window width and height don't get enlarged.\n> */\n> void SwStatsCpu::setWindow(const Rectangle &window)\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 8EA27C328C\n\tfor ;\n\tMon, 29 Sep 2025 11:12:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 868F16B5F8;\n\tMon, 29 Sep 2025 13:12:45 +0200 (CEST)","from sea.source.kernel.org (sea.source.kernel.org [172.234.252.31])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8972B6B599\n\tfor ;\n\tMon, 29 Sep 2025 13:12:43 +0200 (CEST)","from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58])\n\tby sea.source.kernel.org (Postfix) with ESMTP id 8B8C941789;\n\tMon, 29 Sep 2025 11:12:41 +0000 (UTC)","by smtp.kernel.org (Postfix) with ESMTPSA id 6F1E7C4CEF4;\n\tMon, 29 Sep 2025 11:12:40 +0000 (UTC)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=kernel.org header.i=@kernel.org\n\theader.b=\"R+wAz8E3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org;\n\ts=k20201202; t=1759144361;\n\tbh=FwSExbzdmb4aBSpwCUj1kK0kdlEw3zMLi+La3BQdbxY=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=R+wAz8E3sGqaY1vysROsf5NYcsvVnCE6skAPzcYnYtULaFKj0v5l5zv1DxybQdrHU\n\tukUkH12ofK3qrbG7z9iV2n76+IbxEKCWBt8fixp1YV1tMiQfSjISrcwYUYzW4K67wA\n\tLbEbUU5ZGswkbfjht69vBu6u5YKU9T0pFbsXPCWUbO+xh0kG949oms97C6m9pAKhFD\n\tA8N/cWcoGSajzcxTOGdRpeDLc7ENDJw19CLISA74jBd2izmi/LVESr6dsDJb/pahcp\n\tM6GnG+rlahKxsDvU0ur38G73fjX4l7sIvUR6Iam3o4PJRuXAWj1YQeQtFD364riTww\n\tYdV3izRjFM+rQ==","Message-ID":"","Date":"Mon, 29 Sep 2025 13:12:38 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v4 2/7] libcamera: software_isp: Clarify\n\tSwStatsCpu::setWindow use","To":"Milan Zamazal , libcamera-devel@lists.libcamera.org","Cc":"pobrn@protonmail.com, mail@maciej.szmigiero.name","References":"<20250925192856.77881-1-mzamazal@redhat.com>\n\t<20250925192856.77881-3-mzamazal@redhat.com>","From":"Hans de Goede ","Content-Language":"en-US, nl","In-Reply-To":"<20250925192856.77881-3-mzamazal@redhat.com>","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"7bit","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":36038,"web_url":"https://patchwork.libcamera.org/comment/36038/","msgid":"<85y0pxpfwk.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","date":"2025-09-29T13:06:03","subject":"Re: [PATCH v4 2/7] libcamera: software_isp: Clarify\n\tSwStatsCpu::setWindow use","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Barnabás Pőcze writes:\n\n> 2025. 09. 26. 17:35 keltezéssel, Milan Zamazal írta:\n>> Barnabás Pőcze writes:\n>> \n>>> Hi\n>>>\n>>> 2025. 09. 25. 21:28 keltezéssel, Milan Zamazal írta:\n>>>> The window coordinates passed to SwStatsCpu::setWindow are confusing.\n>>>> Let's clarify what the coordinates should be.\n>>>> A source of confusion is that the specified window is relative to the\n>>>> processed area. Debayering adjusts line pointers for its processed area\n>>>> and this is what's also passed to stats processing. The window passed\n>>>> to SwStatsCpu::setWindow should either specify the size of the whole\n>>>> processed (not image) area, or its cropping in case the stats shouldn't\n>>>> be gathered over the whole processed area. This patch should clarify\n>>>> this in the code.\n>>>> Reviewed-by: Maciej S. Szmigiero \n>>>> Signed-off-by: Milan Zamazal \n>>>> ---\n>>>> src/libcamera/software_isp/debayer_cpu.cpp | 6 +++++-\n>>>> src/libcamera/software_isp/swstats_cpu.cpp | 19 +++++++++++++++++++\n>>>> 2 files changed, 24 insertions(+), 1 deletion(-)\n>>>> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n>>>> index 2dc85e5e0..bcaaa5dee 100644\n>>>> --- a/src/libcamera/software_isp/debayer_cpu.cpp\n>>>> +++ b/src/libcamera/software_isp/debayer_cpu.cpp\n>>>> @@ -554,7 +554,11 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg,\n>>>> \twindow_.width = outputCfg.size.width;\n>>>> \twindow_.height = outputCfg.size.height;\n>>>> -\t/* Don't pass x,y since process() already adjusts src before passing it */\n>>>> +\t/*\n>>>> +\t * Set the stats window to the whole processed window. Its coordinates are\n>>>> +\t * relative to the debayered area since debayering passes only the part of\n>>>> +\t * data to be processed to the stats; see SwStatsCpu::setWindow.\n>>>> +\t */\n>>>> \tstats_->setWindow(Rectangle(window_.size()));\n>>>> \t/* pad with patternSize.Width on both left and right side */\n>>>> diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp\n>>>> index e8a1d52f2..c936ef1dc 100644\n>>>> --- a/src/libcamera/software_isp/swstats_cpu.cpp\n>>>> +++ b/src/libcamera/software_isp/swstats_cpu.cpp\n>>>> @@ -416,6 +416,25 @@ int SwStatsCpu::configure(const StreamConfiguration &inputCfg)\n>>>> /**\n>>>> * \\brief Specify window coordinates over which to gather statistics\n>>>> * \\param[in] window The window object.\n>>>> + *\n>>>> + * This method specifies the image area over which to gather the statistics.\n>>>> + * It must be called to set the area, otherwise the default zero-sized\n>>>> + * \\a Rectangle is used and no statistics is gathered.\n>>>> + *\n>>>> + * The specified \\a window is relative to what is passed to processLine*\n>>>> + * methods. Typically, this means processLine* methods get only data from the\n>>>> + * processed area and \\a window is \\a Rectangle with (0, 0) top-left point and\n>>>> + * of the same size as the processed area. But if statistics is gathered only\n>>>> + * from some part of the image, e.g. its centre, \\a window should specify such a\n>>>> + * restriction accordingly.\n>\n> The second sentence is a bit hard to parse for me, or rather, I think it would be\n> clearer to mention the typical case as an explicit example:\n>\n> The specified \\a window is relative to what is passed to the processLine* methods. For example,\n> if statistics are to be gathered from the entire processed area, then \\a window should be a\n> rectangle with the top-left corner of (0,0) and the same size as the processed area. If only\n> a part of the processed area (e.g. its centre) is to be considered for statistics, then \\a window\n> should specify such a restriction accordingly.\n>\n> Thoughts?\n\nOK for me.\n\n>>>> + *\n>>>> + * It is the responsibility of the callers to provide sensible \\a window values,\n>>>> + * most notably not exceeding the original image boundaries.\n>>>> + *\n>>>> + * The method may adjust the window slightly if it is not aligned according to\n>>>> + * the bayer pattern determined in \\a SwStatsCpu::configure(). If that happens,\n>>>> + * it is guaranteed that non-negative top-left point coordinates remain\n>>>> + * non-negative and that the window width and height don't get enlarged.\n>>>\n>>> Why mention \"non-negative\"?\n>> It describes the property of the implementation. Without it, the\n>> statement would be false.\n>\n> Negative y coordinates will stay negative, negative x coordinates will\n> stay negative (assuming patternSize_.{width,height} > 1). In any case,\n> I just feel like mentioning \"non-negative\" will immediately make the\n> reader wonder \"but what about negative numbers?\", at least it did to me.\n\nNot mentioned => undefined. Can you perhaps propose a different text?\n \n>>> I feel like, if anything, we should disallow negative coordinates for\n>>> the top left corner altogether.\n>> The method doesn't provide a mechanism to disallow insane `window'\n>> values (other than to change the values completely) and I don't think\n>> it's needed for such a very internal call. I think the preceding\n>\n> I was thinking of something like an ASSERT().\n\nI see. It can be added for negative values. We don't know the image\nsize here, so we can't check for the maximum values.\n\n>> paragraph about the caller responsibility is enough; the implementation\n>> is sufficient for its purpose and the primary intended purpose of the\n>> docstring is to make the life of readers of the code easier\n>> (relative/absolute confusion etc.) rather than to define a robust API.\n>\n> Okay, let's not complicate things.\n>\n>\n> Regards,\n> Barnabás Pőcze\n>\n>\n>> \n>>> Regards,\n>>> Barnabás Pőcze\n>>>\n>>>\n>>>> */\n>>>> void SwStatsCpu::setWindow(const Rectangle &window)\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 85F0BC324C\n\tfor ;\n\tMon, 29 Sep 2025 13:06:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0937F6B5F3;\n\tMon, 29 Sep 2025 15:06:12 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1CD796B599\n\tfor ;\n\tMon, 29 Sep 2025 15:06:09 +0200 (CEST)","from mail-wm1-f69.google.com (mail-wm1-f69.google.com\n\t[209.85.128.69]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-657-zYVuMu-ENaqBWExHvwBuug-1; Mon, 29 Sep 2025 09:06:06 -0400","by mail-wm1-f69.google.com with SMTP id\n\t5b1f17b1804b1-46e4943d713so11343595e9.1\n\tfor ;\n\tMon, 29 Sep 2025 06:06:06 -0700 (PDT)","from mzamazal-thinkpadp1gen7.tpbc.csb\n\t(ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\t5b1f17b1804b1-46e32bcd016sm96885045e9.1.2025.09.29.06.06.03\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 29 Sep 2025 06:06:04 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"OilU3tHO\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1759151167;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tcontent-transfer-encoding:content-transfer-encoding:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=fDypJkKsgqXIPoNrNvLiiEz7pvMEcsgNWECha+eVWGc=;\n\tb=OilU3tHObHPywrsllkGMDalAgh0fPDKEJ3d/jk7pmM04WdCU9jf+aXekVfPu7IOlm/cu+d\n\tBEOeU6vRrFwT0JujpkDQIrLDCu0LfoXz6coYYa5oy+55QcMrprXpUxRjhLbheW4633D3yN\n\twb5BdeprpwxxChW/asjjsquSp85yKdc=","X-MC-Unique":"zYVuMu-ENaqBWExHvwBuug-1","X-Mimecast-MFC-AGG-ID":"zYVuMu-ENaqBWExHvwBuug_1759151165","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1759151165; x=1759755965;\n\th=content-transfer-encoding:mime-version:user-agent:message-id:date\n\t:references:in-reply-to:subject:cc:to:from:x-gm-message-state:from\n\t:to:cc:subject:date:message-id:reply-to;\n\tbh=1D8Ech89MJFfu5RAc95gk+ACR58pKv2lLLaZcLJRNiU=;\n\tb=IZtThPewk4hqN6jmcVZ4aywjegNWaOxKWOajgSkeEDcTY1JWtC7ppVmi04I4x3Uqph\n\to/w5em7HWguXXhFIMezBp71JKTVgyblvYNqtNendWkuX6LJgYRiWHuiLrmTS0naR4G6H\n\tvhRTuW0q2fMa7CGuAL4i0L1j2NzGVrO4NTL0x+/oJJoOV24COZVbA3KWC+sgq7urnqjS\n\tQLTLNQQnW2F49J+v7Nmz2wmVqKWFroikzsPFRM+3KiO3wNvxrKd+KvfU51ZZJiaOFCSa\n\tbQ8g67nW0LTGeEKppaPM/V7GmXuj/PLMLX2p53WaanAbbu/jtZjsaN/BkpLu1V0SpS5m\n\tIC7w==","X-Gm-Message-State":"AOJu0YzE9NQv3Nc8oX3wlvZufGLIOHzsPcxUL9zLZnPpM1Kutm+zYa2u\n\tbS5o7xStk3lRXuncOxnf7FZ8+k9Zkqj/XfBIUaIKbxUkVor1KsgiW0ZmAJ+yIlvPUv8I5+MMYRx\n\tkTjg66kl4d5jAcKhOwbdxC4ZF+2DjfaIct1ZsTj3eKMsUPAKAVFoTnIK86H1ltEtlLFCBaC3hHP\n\tw=","X-Gm-Gg":"ASbGnctMPIMwvzJAXZDkVRe9LosUFV5K0tqEAGLd9ovsi5ZmhJAEWla3/Mh/RDIOqIJ\n\t61DWDycd8O+QgIq8dapADxF/q2poITRWwxm11djg8rjQUjuOD6sHaNMH2KD3T3YKF7ltH7TKySt\n\tNzoDVhbC5XPREth7FCMxCtKcBy1CmFxRZAb6kqyviM3lYFgqYhIwrtwuuDmFAtQhJ4Yie99PjVD\n\tp17EYnFGf671u7/elx1ZjUMh7oeGQyx6G1Yg6f8cVGsfpy5JRV9Wml8ml/AP3rmsujjDyHLFPwu\n\t8lhBCwpw9POLWd2LC7kSbSebqlrsBACHXKo8EvFtqKabr82Ij0w+RgzmrylN/rrsiQOcFL73xLo\n\tfRt93jkRv+b24v16Pxw==","X-Received":["by 2002:a05:600c:4511:b0:46e:19f8:88d8 with SMTP id\n\t5b1f17b1804b1-46e32a54d21mr150484155e9.34.1759151165059; \n\tMon, 29 Sep 2025 06:06:05 -0700 (PDT)","by 2002:a05:600c:4511:b0:46e:19f8:88d8 with SMTP id\n\t5b1f17b1804b1-46e32a54d21mr150483775e9.34.1759151164552; \n\tMon, 29 Sep 2025 06:06:04 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IERI4GD1H1fE9ivdEdHULHc/sc4TDdTJBtLr3NJWjuNOcCti63F5Rm+OvoxX5eUikZFGNP0pw==","From":"Milan Zamazal ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org, mail@maciej.szmigiero.name","Subject":"Re: [PATCH v4 2/7] libcamera: software_isp: Clarify\n\tSwStatsCpu::setWindow use","In-Reply-To":"<8428ce59-13bf-48f0-8589-0bd5f3b1bbc3@ideasonboard.com> (\n\t=?utf-8?b?IkJhcm5hYsOhcyBQxZFjemUiJ3M=?= message of \"Mon,\n\t29 Sep 2025 12:36:22 +0200\")","References":"<20250925192856.77881-1-mzamazal@redhat.com>\n\t<20250925192856.77881-3-mzamazal@redhat.com>\n\t<72d0879e-6b58-476c-a38c-5ebed85c85e3@ideasonboard.com>\n\t<85qzvtkz17.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>\n\t<8428ce59-13bf-48f0-8589-0bd5f3b1bbc3@ideasonboard.com>","Date":"Mon, 29 Sep 2025 15:06:03 +0200","Message-ID":"<85y0pxpfwk.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"rCMUeJfYQ9u5xJHinrPCGWUK_U-96ih-l0V-gLb3Y5I_1759151165","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","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":36040,"web_url":"https://patchwork.libcamera.org/comment/36040/","msgid":"<67da400b-3ae7-4fe5-809e-9a5bd3b826b9@ideasonboard.com>","date":"2025-09-29T15:24:01","subject":"Re: [PATCH v4 2/7] libcamera: software_isp: Clarify\n\tSwStatsCpu::setWindow use","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. 09. 29. 15:06 keltezéssel, Milan Zamazal írta:\n> Barnabás Pőcze writes:\n> \n>> 2025. 09. 26. 17:35 keltezéssel, Milan Zamazal írta:\n>>> Barnabás Pőcze writes:\n>>>\n>>>> Hi\n>>>>\n>>>> 2025. 09. 25. 21:28 keltezéssel, Milan Zamazal írta:\n>>>>> The window coordinates passed to SwStatsCpu::setWindow are confusing.\n>>>>> Let's clarify what the coordinates should be.\n>>>>> A source of confusion is that the specified window is relative to the\n>>>>> processed area. Debayering adjusts line pointers for its processed area\n>>>>> and this is what's also passed to stats processing. The window passed\n>>>>> to SwStatsCpu::setWindow should either specify the size of the whole\n>>>>> processed (not image) area, or its cropping in case the stats shouldn't\n>>>>> be gathered over the whole processed area. This patch should clarify\n>>>>> this in the code.\n>>>>> Reviewed-by: Maciej S. Szmigiero \n>>>>> Signed-off-by: Milan Zamazal \n>>>>> ---\n>>>>> src/libcamera/software_isp/debayer_cpu.cpp | 6 +++++-\n>>>>> src/libcamera/software_isp/swstats_cpu.cpp | 19 +++++++++++++++++++\n>>>>> 2 files changed, 24 insertions(+), 1 deletion(-)\n>>>>> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n>>>>> index 2dc85e5e0..bcaaa5dee 100644\n>>>>> --- a/src/libcamera/software_isp/debayer_cpu.cpp\n>>>>> +++ b/src/libcamera/software_isp/debayer_cpu.cpp\n>>>>> @@ -554,7 +554,11 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg,\n>>>>> \twindow_.width = outputCfg.size.width;\n>>>>> \twindow_.height = outputCfg.size.height;\n>>>>> -\t/* Don't pass x,y since process() already adjusts src before passing it */\n>>>>> +\t/*\n>>>>> +\t * Set the stats window to the whole processed window. Its coordinates are\n>>>>> +\t * relative to the debayered area since debayering passes only the part of\n>>>>> +\t * data to be processed to the stats; see SwStatsCpu::setWindow.\n>>>>> +\t */\n>>>>> \tstats_->setWindow(Rectangle(window_.size()));\n>>>>> \t/* pad with patternSize.Width on both left and right side */\n>>>>> diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp\n>>>>> index e8a1d52f2..c936ef1dc 100644\n>>>>> --- a/src/libcamera/software_isp/swstats_cpu.cpp\n>>>>> +++ b/src/libcamera/software_isp/swstats_cpu.cpp\n>>>>> @@ -416,6 +416,25 @@ int SwStatsCpu::configure(const StreamConfiguration &inputCfg)\n>>>>> /**\n>>>>> * \\brief Specify window coordinates over which to gather statistics\n>>>>> * \\param[in] window The window object.\n>>>>> + *\n>>>>> + * This method specifies the image area over which to gather the statistics.\n>>>>> + * It must be called to set the area, otherwise the default zero-sized\n>>>>> + * \\a Rectangle is used and no statistics is gathered.\n>>>>> + *\n>>>>> + * The specified \\a window is relative to what is passed to processLine*\n>>>>> + * methods. Typically, this means processLine* methods get only data from the\n>>>>> + * processed area and \\a window is \\a Rectangle with (0, 0) top-left point and\n>>>>> + * of the same size as the processed area. But if statistics is gathered only\n>>>>> + * from some part of the image, e.g. its centre, \\a window should specify such a\n>>>>> + * restriction accordingly.\n>>\n>> The second sentence is a bit hard to parse for me, or rather, I think it would be\n>> clearer to mention the typical case as an explicit example:\n>>\n>> The specified \\a window is relative to what is passed to the processLine* methods. For example,\n>> if statistics are to be gathered from the entire processed area, then \\a window should be a\n>> rectangle with the top-left corner of (0,0) and the same size as the processed area. If only\n>> a part of the processed area (e.g. its centre) is to be considered for statistics, then \\a window\n>> should specify such a restriction accordingly.\n>>\n>> Thoughts?\n> \n> OK for me.\n> \n>>>>> + *\n>>>>> + * It is the responsibility of the callers to provide sensible \\a window values,\n>>>>> + * most notably not exceeding the original image boundaries.\n\nCould you add something like \"Neither coordinate of the top-left corner shall be negative.\"\nhere somewhere in some way?\n\n\n>>>>> + *\n>>>>> + * The method may adjust the window slightly if it is not aligned according to\n>>>>> + * the bayer pattern determined in \\a SwStatsCpu::configure(). If that happens,\n>>>>> + * it is guaranteed that non-negative top-left point coordinates remain\n>>>>> + * non-negative and that the window width and height don't get enlarged.\n>>>>\n>>>> Why mention \"non-negative\"?\n>>> It describes the property of the implementation. Without it, the\n>>> statement would be false.\n>>\n>> Negative y coordinates will stay negative, negative x coordinates will\n>> stay negative (assuming patternSize_.{width,height} > 1). In any case,\n>> I just feel like mentioning \"non-negative\" will immediately make the\n>> reader wonder \"but what about negative numbers?\", at least it did to me.\n> \n> Not mentioned => undefined. Can you perhaps propose a different text?\n\nFair enough. I don't have a good suggestion. If the adjusted window remained\ninside the original window, that would be easier to express succinctly; but\nsince the x,y coordinates are rounded towards -infinity, that is not true...\n\nMaybe the following:\n\n Due to limitations of the implementation, the method may adjust the window slightly\n if it is not aligned according to the bayer pattern determined in \\a SwStatsCpu::configure().\n In that case the window will be modified such that the sides are no larger than the original,\n and that the new bottom-left corner will be no further from (0,0) (along either axis) than the\n original was.\n\nBut let's not dwell on it too much, I have already wasted enough of our time with this, sorry.\n\n\n> \n>>>> I feel like, if anything, we should disallow negative coordinates for\n>>>> the top left corner altogether.\n>>> The method doesn't provide a mechanism to disallow insane `window'\n>>> values (other than to change the values completely) and I don't think\n>>> it's needed for such a very internal call. I think the preceding\n>>\n>> I was thinking of something like an ASSERT().\n> \n> I see. It can be added for negative values. We don't know the image\n> size here, so we can't check for the maximum values.\n\nFeel free to decide if you want to add the `ASSERT(... >= 0)`.\n\n\nRegards,\nBarnabás Pőcze\n\n\n> \n>>> paragraph about the caller responsibility is enough; the implementation\n>>> is sufficient for its purpose and the primary intended purpose of the\n>>> docstring is to make the life of readers of the code easier\n>>> (relative/absolute confusion etc.) rather than to define a robust API.\n>>\n>> Okay, let's not complicate things.\n>>\n>>\n>> Regards,\n>> Barnabás Pőcze\n>>\n>>\n>>>\n>>>> Regards,\n>>>> Barnabás Pőcze\n>>>>\n>>>>\n>>>>> */\n>>>>> void SwStatsCpu::setWindow(const Rectangle &window)\n>>>>> {\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 70648C324C\n\tfor ;\n\tMon, 29 Sep 2025 15:24:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 59E2D6B5F3;\n\tMon, 29 Sep 2025 17:24:06 +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 12A8C6B599\n\tfor ;\n\tMon, 29 Sep 2025 17:24:05 +0200 (CEST)","from [192.168.33.13] (185.221.142.146.nat.pool.zt.hu\n\t[185.221.142.146])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3144E4C7;\n\tMon, 29 Sep 2025 17:22:37 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"MPYGDA2z\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1759159357;\n\tbh=KRhLFH6bTsvrxH0THbWKL8tDIuAhtVPc+/kUqT1MODE=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=MPYGDA2zsH3+vjaisDApT9Cxo74weZzmWVtdQLRur2KrJ7EdwJETOdTFA0lYuBeBb\n\t1k77ynTJ7Zc/8lgFASIpiKCMq1QDNRaFDuk5EffDEqZbcwNXfb+LzvwGV/oKuq20lm\n\tTLEfYHIdej3Zc24iSglg6B/wrxzo12QrkBwDrnUM=","Message-ID":"<67da400b-3ae7-4fe5-809e-9a5bd3b826b9@ideasonboard.com>","Date":"Mon, 29 Sep 2025 17:24:01 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v4 2/7] libcamera: software_isp: Clarify\n\tSwStatsCpu::setWindow use","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org, mail@maciej.szmigiero.name","References":"<20250925192856.77881-1-mzamazal@redhat.com>\n\t<20250925192856.77881-3-mzamazal@redhat.com>\n\t<72d0879e-6b58-476c-a38c-5ebed85c85e3@ideasonboard.com>\n\t<85qzvtkz17.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>\n\t<8428ce59-13bf-48f0-8589-0bd5f3b1bbc3@ideasonboard.com>\n\t<85y0pxpfwk.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<85y0pxpfwk.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","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\" "}}]