[{"id":35970,"web_url":"https://patchwork.libcamera.org/comment/35970/","msgid":"<67fb6172-5149-45f3-826f-0a05eea342d9@ideasonboard.com>","date":"2025-09-25T10:58:41","subject":"Re: [PATCH v3 1/6] 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. 19. 19:37 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 | 16 ++++++++++++++++\n> 2 files changed, 21 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 66f6038c1..bcc847ae6 100644\n> --- a/src/libcamera/software_isp/debayer_cpu.cpp\n> +++ b/src/libcamera/software_isp/debayer_cpu.cpp\n> @@ -541,7 +541,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 4b77b3600..72aa88b69 100644\n> --- a/src/libcamera/software_isp/swstats_cpu.cpp\n> +++ b/src/libcamera/software_isp/swstats_cpu.cpp\n> @@ -416,6 +416,22 @@ 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 method may adjust the window slightly if it is not aligned according to\n> + * the bayer pattern determined in \\a SwStatsCpu::configure(). Such an\n> + * adjustment is guaranteed to not exceed the bounds of\n> + * Rectangle(0, 0, window.width, window.height).\n\nI think that's not entirely true. If one wants to set the window to an empty\nwindow (i.e. `width == 0`) and the pattern is GBRG or RGGB with no packing, then\n`xShift_ == 1`, thus\n\n\t/* width_ - xShift_ to make sure the window fits */\n\twindow_.width -= xShift_;\n\twindow_.width &= ~(patternSize_.width - 1);\n\nwill result in `window_.width == -2u == 4294967294` as far as I can see.\n\nAlso, it says\n\n guaranteed to not exceed the bounds of Rectangle(0, 0, window.width, window.height).\n\nbut is that true if the top-left corner is not (0,0)? Shouldn't it say\n\n Rectangle(0, 0, window.x + window.width, window.y + window.height)\n\nor similar?\n\n\nRegards,\nBarnabás Pőcze\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 17449C328C\n\tfor ;\n\tThu, 25 Sep 2025 10:58:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D08A96B5F3;\n\tThu, 25 Sep 2025 12:58:46 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D6F2F69318\n\tfor ;\n\tThu, 25 Sep 2025 12:58:44 +0200 (CEST)","from [192.168.33.22] (185.221.140.70.nat.pool.zt.hu\n\t[185.221.140.70])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 043BD1419;\n\tThu, 25 Sep 2025 12:57:19 +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=\"Rk478Etw\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1758797840;\n\tbh=laNBMw+BYCUgEpbP/s5q8ZhcAWiB7yWgbXHLDROO1TU=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=Rk478EtwlYVAcfTxUomvVSkjlO0XJalU5vjiiMLUlZHdBy1Th0nnz8GMbeDekZ0gE\n\tadb6HVfwZshgb50NmJIIiVN7g4JCJPXixEla+7ImIO01aiOaz8eaJRWaL0VRdvUBFO\n\tTBjEZ50KcuWhS+LvnDAwGyhEXT34bKwaqZymBJLQ=","Message-ID":"<67fb6172-5149-45f3-826f-0a05eea342d9@ideasonboard.com>","Date":"Thu, 25 Sep 2025 12:58:41 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v3 1/6] libcamera: software_isp: Clarify\n\tSwStatsCpu::setWindow use","To":"Milan Zamazal , libcamera-devel@lists.libcamera.org","Cc":"mail@maciej.szmigiero.name","References":"<20250919173709.220881-1-mzamazal@redhat.com>\n\t<20250919173709.220881-2-mzamazal@redhat.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<20250919173709.220881-2-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":35973,"web_url":"https://patchwork.libcamera.org/comment/35973/","msgid":"<85348azse8.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","date":"2025-09-25T11:24:47","subject":"Re: [PATCH v3 1/6] 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":"Hi Barnabás,\n\nBarnabás Pőcze writes:\n\n> Hi\n>\n> 2025. 09. 19. 19:37 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 | 16 ++++++++++++++++\n>> 2 files changed, 21 insertions(+), 1 deletion(-)\n>> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n>> index 66f6038c1..bcc847ae6 100644\n>> --- a/src/libcamera/software_isp/debayer_cpu.cpp\n>> +++ b/src/libcamera/software_isp/debayer_cpu.cpp\n>> @@ -541,7 +541,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 4b77b3600..72aa88b69 100644\n>> --- a/src/libcamera/software_isp/swstats_cpu.cpp\n>> +++ b/src/libcamera/software_isp/swstats_cpu.cpp\n>> @@ -416,6 +416,22 @@ 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 method may adjust the window slightly if it is not aligned according to\n>> + * the bayer pattern determined in \\a SwStatsCpu::configure(). Such an\n>> + * adjustment is guaranteed to not exceed the bounds of\n>> + * Rectangle(0, 0, window.width, window.height).\n>\n> I think that's not entirely true. If one wants to set the window to an empty\n> window (i.e. `width == 0`) and the pattern is GBRG or RGGB with no packing, then\n> `xShift_ == 1`, thus\n>\n> \t/* width_ - xShift_ to make sure the window fits */\n> \twindow_.width -= xShift_;\n> \twindow_.width &= ~(patternSize_.width - 1);\n>\n> will result in `window_.width == -2u == 4294967294` as far as I can see.\n>\n> Also, it says\n>\n> guaranteed to not exceed the bounds of Rectangle(0, 0, window.width, window.height).\n>\n> but is that true if the top-left corner is not (0,0)? Shouldn't it say\n>\n> Rectangle(0, 0, window.x + window.width, window.y + window.height)\n>\n> or similar?\n\nYou're right. From my point of view, the documentation should stay as\nit is (I think those are meaningful requirements) and the implementation\nof the method should be changed to comply. Do you agree?\n\n> Regards,\n> Barnabás Pőcze\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 0B08DC328C\n\tfor ;\n\tThu, 25 Sep 2025 11:24:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 38CB56B5F3;\n\tThu, 25 Sep 2025 13:24:57 +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 5E18569318\n\tfor ;\n\tThu, 25 Sep 2025 13:24:53 +0200 (CEST)","from mail-wm1-f71.google.com (mail-wm1-f71.google.com\n\t[209.85.128.71]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-651-M2jdYN-qOhyamvNfgqYngg-1; Thu, 25 Sep 2025 07:24:51 -0400","by mail-wm1-f71.google.com with SMTP id\n\t5b1f17b1804b1-46e31191379so5549605e9.3\n\tfor ;\n\tThu, 25 Sep 2025 04:24:50 -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-40fb89fb19fsm2680153f8f.21.2025.09.25.04.24.48\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 25 Sep 2025 04:24:48 -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=\"R4fZPaR+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1758799492;\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=Vgnm5ENp/DH/xIwi6CraKJYHiQaSY5asDaV3dCR3q1g=;\n\tb=R4fZPaR+GR4QjD6OQHW7NlWfNkucBnIGxQX2KIc3gfRc7Hdk8ouZHSj00tRk1FJcS3gMaZ\n\th765xA+s4lSXOQnhQCZ4kZowv2/GmRSrYV9cb/iv9ekiYbSEsfc/lrw6XX5clR10I9DqCp\n\tjtkc5BIYprmqISNQpTJ8WSDgf95kIvs=","X-MC-Unique":"M2jdYN-qOhyamvNfgqYngg-1","X-Mimecast-MFC-AGG-ID":"M2jdYN-qOhyamvNfgqYngg_1758799490","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1758799490; x=1759404290;\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=U0ja15zKMqD+TKWpqYb7tB25EHjxcZx/4g5hjkt9bIc=;\n\tb=FyHrR6YMFUIRG4zv7xK9/3iZIPWDZNVRht5yCVQHQCL7nC+LKSlF/GZT9RovYUTyLp\n\tbd6HEBH2FUHAJafPF76ODJf8LAM93KmC4FkW2Uy4hb/2wc32JuncURPI5mNWKKjyXLyB\n\tEagEkgHS2VM/zb9khtzd9hzTBXsECTx0hPGfrv9b1Dzb6ZwZczaWINgFkGbjxCaAWtEm\n\tjNVZ3CmknrivFhY9ggGlSG5HSMH/PKPhyM37j4tjB7towo9fb2/56TvZJhNBft5DFZIK\n\tBKvnWwbzNsTs9m3TTmI06DOCcfLWEg29fnnUYKgH1N97SxPBTMScc3+JEU9apn+7vaFM\n\tWcCg==","X-Gm-Message-State":"AOJu0Yzn1XBr37yb/1eb6peih/iuXIXrpGrRmM2a6+AWczjubq66g0QC\n\tur9E5TdofZgNusneIk0z2wJgD/V0oVI5T4/AXG+ZKO5dAfz0N+YFbYYz2giSLwO50DSJIwXr/GJ\n\tMBV3ZaQ2N4oRR6fDxGXbSdeZcL9uDTHyo0oN3iR9Wb7y8Hf0uc0YIVyox83/Ilna5uhRwScEJmC\n\t0=","X-Gm-Gg":"ASbGncsRDRWb0NiNP81zkAPq3bJqHo6ud5w9sDApNqoitkzKX3uxaVBTRSauT4PhLZe\n\to8ojkJoBDJwaaMbpB5IrfUMNAGWt02TBVuzruHbJoZs79XE6l0tt1wEV5/5dbcRmvfaJfo3hr2x\n\tyiiS5J8wZt6qiwsuXEiGnEsWNf3Tzs3kbLQDE2U5T6ptZXQvB8aABD6VFaOSwhO9vMA9APzd/Uh\n\tNXCJ7HAPK79KnMUPJ2FDLeGkd10UOJCXKHwQN4wOzU3tDlpmc03DG440RSQtiIkCGWcivXAPAV0\n\tD9Qi16+INiDoX5SdLWxDzUi2Vww6ersjZaUHBMGTSdnblCF4sWUJFh595hew2pesoFKHP9OS22+\n\tTEm/YMUMnQ4xwmyrwaQ==","X-Received":["by 2002:a05:600c:1e85:b0:459:e398:ed89 with SMTP id\n\t5b1f17b1804b1-46e3299b8f3mr30493265e9.1.1758799489676; \n\tThu, 25 Sep 2025 04:24:49 -0700 (PDT)","by 2002:a05:600c:1e85:b0:459:e398:ed89 with SMTP id\n\t5b1f17b1804b1-46e3299b8f3mr30493035e9.1.1758799489308; \n\tThu, 25 Sep 2025 04:24:49 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IGaJ/hkcEPlTLTR/FkyxEPpMgGWhyCHGfree4i0khgK+ulcQfLLkhOtXPvFh3DuJD8r+L7OwQ==","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 v3 1/6] libcamera: software_isp: Clarify\n\tSwStatsCpu::setWindow use","In-Reply-To":"<67fb6172-5149-45f3-826f-0a05eea342d9@ideasonboard.com> (\n\t=?utf-8?b?IkJhcm5hYsOhcyBQxZFjemUiJ3M=?= message of \"Thu,\n\t25 Sep 2025 12:58:41 +0200\")","References":"<20250919173709.220881-1-mzamazal@redhat.com>\n\t<20250919173709.220881-2-mzamazal@redhat.com>\n\t<67fb6172-5149-45f3-826f-0a05eea342d9@ideasonboard.com>","Date":"Thu, 25 Sep 2025 13:24:47 +0200","Message-ID":"<85348azse8.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":"j4BNITXVe0fJ9ZcseuhtnKoH4M3XMwwZaFBwG2UY7ak_1758799490","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":35975,"web_url":"https://patchwork.libcamera.org/comment/35975/","msgid":"<98df44d4-8c23-44e8-8b08-396a8084767d@ideasonboard.com>","date":"2025-09-25T12:03:49","subject":"Re: [PATCH v3 1/6] 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. 25. 13:24 keltezéssel, Milan Zamazal írta:\n> Hi Barnabás,\n> \n> Barnabás Pőcze writes:\n> \n>> Hi\n>>\n>> 2025. 09. 19. 19:37 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 | 16 ++++++++++++++++\n>>> 2 files changed, 21 insertions(+), 1 deletion(-)\n>>> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n>>> index 66f6038c1..bcc847ae6 100644\n>>> --- a/src/libcamera/software_isp/debayer_cpu.cpp\n>>> +++ b/src/libcamera/software_isp/debayer_cpu.cpp\n>>> @@ -541,7 +541,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 4b77b3600..72aa88b69 100644\n>>> --- a/src/libcamera/software_isp/swstats_cpu.cpp\n>>> +++ b/src/libcamera/software_isp/swstats_cpu.cpp\n>>> @@ -416,6 +416,22 @@ 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 method may adjust the window slightly if it is not aligned according to\n>>> + * the bayer pattern determined in \\a SwStatsCpu::configure(). Such an\n>>> + * adjustment is guaranteed to not exceed the bounds of\n>>> + * Rectangle(0, 0, window.width, window.height).\n>>\n>> I think that's not entirely true. If one wants to set the window to an empty\n>> window (i.e. `width == 0`) and the pattern is GBRG or RGGB with no packing, then\n>> `xShift_ == 1`, thus\n>>\n>> \t/* width_ - xShift_ to make sure the window fits */\n>> \twindow_.width -= xShift_;\n>> \twindow_.width &= ~(patternSize_.width - 1);\n>>\n>> will result in `window_.width == -2u == 4294967294` as far as I can see.\n>>\n>> Also, it says\n>>\n>> guaranteed to not exceed the bounds of Rectangle(0, 0, window.width, window.height).\n>>\n>> but is that true if the top-left corner is not (0,0)? Shouldn't it say\n>>\n>> Rectangle(0, 0, window.x + window.width, window.y + window.height)\n>>\n>> or similar?\n> \n> You're right. From my point of view, the documentation should stay as\n> it is (I think those are meaningful requirements) and the implementation\n> of the method should be changed to comply. Do you agree?\n\nI think I misunderstand something. If the proposed description is kept, then if\none wants `(20, 20, 200, 200)`, then in my interpretation it couldn't be adjusted\nto something reasonable like `(22, 22, 188, 188)` because that is outside `(0, 0, 200, 200)`.\n\nI am also realizing now that the meaning of \"Such an adjustment is guaranteed to not exceed the bounds of [...]\"\nis not clear to me. I think it should be expressed in terms of the \"adjusted window\" or similar.\n\n\n> \n>> Regards,\n>> Barnabás Pőcze\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 8C709C328C\n\tfor ;\n\tThu, 25 Sep 2025 12:03:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3E8006B5F3;\n\tThu, 25 Sep 2025 14:03:55 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8725A69318\n\tfor ;\n\tThu, 25 Sep 2025 14:03:52 +0200 (CEST)","from [192.168.33.22] (185.221.140.70.nat.pool.zt.hu\n\t[185.221.140.70])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BF1D31419;\n\tThu, 25 Sep 2025 14:02:27 +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=\"qcGYg2Ov\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1758801747;\n\tbh=kgSwC/t/7RZkpAKGHKv1KlC6DqD58U9ncAjy6rxmRtU=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=qcGYg2OvSWCQtvIJDJL9crHIhNFw4/P7Gj2weUTuEf3VV8OEOcGIqu6hgcoPJhOHs\n\tK8IYSKWCDoZUvpuEWVuHiBoIwVRMfvAO5xcoGiQwAEjfq/wXyBGunULrbniaX6nsvV\n\tknkGCAZlKIMZ2nxUn1oqgKTtMWghvEzKL+7twJTc=","Message-ID":"<98df44d4-8c23-44e8-8b08-396a8084767d@ideasonboard.com>","Date":"Thu, 25 Sep 2025 14:03:49 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v3 1/6] libcamera: software_isp: Clarify\n\tSwStatsCpu::setWindow use","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org, mail@maciej.szmigiero.name","References":"<20250919173709.220881-1-mzamazal@redhat.com>\n\t<20250919173709.220881-2-mzamazal@redhat.com>\n\t<67fb6172-5149-45f3-826f-0a05eea342d9@ideasonboard.com>\n\t<85348azse8.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<85348azse8.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":35992,"web_url":"https://patchwork.libcamera.org/comment/35992/","msgid":"<85plbexyum.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","date":"2025-09-25T16:48:17","subject":"Re: [PATCH v3 1/6] 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. 25. 13:24 keltezéssel, Milan Zamazal írta:\n>> Hi Barnabás,\n>> Barnabás Pőcze writes:\n>> \n>>> Hi\n>>>\n>>> 2025. 09. 19. 19:37 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 | 16 ++++++++++++++++\n>>>> 2 files changed, 21 insertions(+), 1 deletion(-)\n>>>> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n>>>> index 66f6038c1..bcc847ae6 100644\n>>>> --- a/src/libcamera/software_isp/debayer_cpu.cpp\n>>>> +++ b/src/libcamera/software_isp/debayer_cpu.cpp\n>>>> @@ -541,7 +541,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 4b77b3600..72aa88b69 100644\n>>>> --- a/src/libcamera/software_isp/swstats_cpu.cpp\n>>>> +++ b/src/libcamera/software_isp/swstats_cpu.cpp\n>>>> @@ -416,6 +416,22 @@ 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 method may adjust the window slightly if it is not aligned according to\n>>>> + * the bayer pattern determined in \\a SwStatsCpu::configure(). Such an\n>>>> + * adjustment is guaranteed to not exceed the bounds of\n>>>> + * Rectangle(0, 0, window.width, window.height).\n>>>\n>>> I think that's not entirely true. If one wants to set the window to an empty\n>>> window (i.e. `width == 0`) and the pattern is GBRG or RGGB with no packing, then\n>>> `xShift_ == 1`, thus\n>>>\n>>> \t/* width_ - xShift_ to make sure the window fits */\n>>> \twindow_.width -= xShift_;\n>>> \twindow_.width &= ~(patternSize_.width - 1);\n>>>\n>>> will result in `window_.width == -2u == 4294967294` as far as I can see.\n>>>\n>>> Also, it says\n>>>\n>>> guaranteed to not exceed the bounds of Rectangle(0, 0, window.width, window.height).\n>>>\n>>> but is that true if the top-left corner is not (0,0)? Shouldn't it say\n>>>\n>>> Rectangle(0, 0, window.x + window.width, window.y + window.height)\n>>>\n>>> or similar?\n>> You're right. From my point of view, the documentation should stay as\n>> it is (I think those are meaningful requirements) and the implementation\n>> of the method should be changed to comply. Do you agree?\n>\n> I think I misunderstand something. If the proposed description is kept, then if\n> one wants `(20, 20, 200, 200)`, then in my interpretation it couldn't be adjusted\n> to something reasonable like `(22, 22, 188, 188)` because that is outside `(0, 0, 200, 200)`.\n\nAh, I understand now. What I wanted to say in the docstring was: If the\nrequested window is (x, y, w, h) and the adjusted window is\n(x', y', w', h') then x' >= 0, y' >= 0, w' <= w, h' <= h. Which is what\nthe implementation does (minus the zero-size corner case).\n\"Rectangle(0, 0, window.width, window.height)\" is indeed a wrong\nexpression of that,\n\"Rectangle(0, 0, window.x + window.width, window.y + window.height)\" is\nalso not completely accurate (it doesn't guarantee w' <= w).\n\n> I am also realizing now that the meaning of \"Such an adjustment is guaranteed to not exceed the bounds of\n> [...]\"\n> is not clear to me. I think it should be expressed in terms of the \"adjusted window\" or similar.\n\nYes, I'll try to find a better description (and fix the implementation).\n\n>> \n>>> Regards,\n>>> Barnabás Pőcze\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 7BC73BDB1C\n\tfor ;\n\tThu, 25 Sep 2025 16:48:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EFEEA6B5F3;\n\tThu, 25 Sep 2025 18:48:24 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4F34269318\n\tfor ;\n\tThu, 25 Sep 2025 18:48:23 +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-604-JiU7OsPRPgSTgYxT52Pn_Q-1; Thu, 25 Sep 2025 12:48:20 -0400","by mail-wm1-f69.google.com with SMTP id\n\t5b1f17b1804b1-46e2c11b94cso6924745e9.3\n\tfor ;\n\tThu, 25 Sep 2025 09:48:20 -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-46e33beebbasm41187365e9.19.2025.09.25.09.48.18\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 25 Sep 2025 09:48:18 -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=\"gnN0o2sS\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1758818902;\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=XjeMtyuLFJmbKhNXum2eeg+JlI5LUSo5Bq1qyO53o9w=;\n\tb=gnN0o2sSCt+bRYHJ796ynoS+WlVGOZMwAmEhZCt/5fMmepuHyMsTi3+Rn5L0VXpzahQsyF\n\tuZv9VTbZiOGBtr6SkU4KfT9XQZNhj9YROYhbTQpf9rZisP+4KkDjqCjBDSMFzeWGpOU/Lc\n\t+pjPDML23rT5/gmE/H4a4I+YSbc6HYk=","X-MC-Unique":"JiU7OsPRPgSTgYxT52Pn_Q-1","X-Mimecast-MFC-AGG-ID":"JiU7OsPRPgSTgYxT52Pn_Q_1758818899","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1758818899; x=1759423699;\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=vm8bW+bQVAzDOTVgtfvjJ7IWnKzHrakuG8aLNsh1KRc=;\n\tb=YT/r0Qo0hD+BOXIY0NdthB4zjCMAZP/Y4dFPh3XXiPFW+/hETK6o4YGyPpM3AP9EFq\n\tiix3iCxchmYIwRgdb6NK8BWniMEHvjTPjWQRgJ/gr9o14upA3fVc9OyI+X/+4d1TeuWS\n\t8h9dJ61ntFiz649LnzsO81+IjSagq8lnLL8/a5xIlanLDdyTRwIOJzpNDgtpEEazA7NK\n\tnkhfJkiFaqwyFPSeyn8+UXyMdGGyjwRWVEOkU9JFv9LAZARhNW+OVVJhUjA4cpERFRC9\n\tqz2sH2wamZfv2nj1GvRzW4OhsmES/SZf2KeiSIOvI+LSg2H5zDDDZGSP9Nr6Rxz8qmcd\n\tvsvQ==","X-Gm-Message-State":"AOJu0YwT54hy6ctCYqN+zRXiN8atrit4FzfjuKJ1NoHqxXyi2mc5WweI\n\tzN4g9Wou131axMM+MVL2HAWwYZPrefBDMmDwKUoSjuBLcWKMFvGvJwk2vpZ3yCtnBRFnAsrkTe+\n\tXdps+nycQ5OUEGE+WCjJoMS364GvwkNvJuLa0Z/fcXEWgFeVKh0uTmdv5Y39xnbFCXdk8ZkhHVm\n\tI=","X-Gm-Gg":"ASbGncu6Ifz7xCodvx567WvHE6yxOoWNv1TOdVdqXyILjpB/R3Zi1zaJpjPAntH6duL\n\tRegg1nXEDnXyIpYCjZoHk2rCkZPLHfkpdEvPIK1NxSMz1jFxqyiLU6U6kD4GYAVUBr1QfPYSqm9\n\tI8K+BRyYlOpjCIY6wNyGcFZ8SxsaDKGYSGHAF/WYxZMjSDMA5o3qkxOWmW+13KB1IWOa9bn55DS\n\t6HH6VHy3J+UJlmnAe4BffjhfTY0USaQPJxqtoNhIdPut54IqvnkEOeNBgHAY3YAKyw9gW6w0yTI\n\t6X31M+ASfiJ/uS5kx8oF1WGv4vCV77PKKLhwT3B00XyBGWOqQvn7PM/uODRJo8MSUrjjFTkNIK0\n\tmnI68zUxhWmLCEMOSJw==","X-Received":["by 2002:a05:600c:26d4:b0:45c:b601:660a with SMTP id\n\t5b1f17b1804b1-46e329faa9bmr27953085e9.23.1758818899416; \n\tThu, 25 Sep 2025 09:48:19 -0700 (PDT)","by 2002:a05:600c:26d4:b0:45c:b601:660a with SMTP id\n\t5b1f17b1804b1-46e329faa9bmr27952945e9.23.1758818898956; \n\tThu, 25 Sep 2025 09:48:18 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IHzO8MBb6xzEYOB0sveySZA4NSEaZtCP9MWN4dnCyFlCQXrl1pTpj9LNA2GY0ylTiXFlFmMRg==","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 v3 1/6] libcamera: software_isp: Clarify\n\tSwStatsCpu::setWindow use","In-Reply-To":"<98df44d4-8c23-44e8-8b08-396a8084767d@ideasonboard.com> (\n\t=?utf-8?b?IkJhcm5hYsOhcyBQxZFjemUiJ3M=?= message of \"Thu,\n\t25 Sep 2025 14:03:49 +0200\")","References":"<20250919173709.220881-1-mzamazal@redhat.com>\n\t<20250919173709.220881-2-mzamazal@redhat.com>\n\t<67fb6172-5149-45f3-826f-0a05eea342d9@ideasonboard.com>\n\t<85348azse8.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>\n\t<98df44d4-8c23-44e8-8b08-396a8084767d@ideasonboard.com>","Date":"Thu, 25 Sep 2025 18:48:17 +0200","Message-ID":"<85plbexyum.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":"mhaeg5mTOiAngsiIslQ-AIZglQwMdo7jtFHpd3JCzdI_1758818899","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\" "}}]