[v4,2/7] libcamera: software_isp: Clarify SwStatsCpu::setWindow use

24456 diff mbox series

The window coordinates passed to SwStatsCpu::setWindow are confusing.
Let's clarify what the coordinates should be.

A source of confusion is that the specified window is relative to the
processed area.  Debayering adjusts line pointers for its processed area
and this is what's also passed to stats processing.  The window passed
to SwStatsCpu::setWindow should either specify the size of the whole
processed (not image) area, or its cropping in case the stats shouldn't
be gathered over the whole processed area.  This patch should clarify
this in the code.

Reviewed-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
---
 src/libcamera/software_isp/debayer_cpu.cpp |  6 +++++-
 src/libcamera/software_isp/swstats_cpu.cpp | 19 +++++++++++++++++++
 2 files changed, 24 insertions(+), 1 deletion(-)

Hi

2025. 09. 25. 21:28 keltezéssel, Milan Zamazal írta:
> The window coordinates passed to SwStatsCpu::setWindow are confusing.
> Let's clarify what the coordinates should be.
> 
> A source of confusion is that the specified window is relative to the
> processed area.  Debayering adjusts line pointers for its processed area
> and this is what's also passed to stats processing.  The window passed
> to SwStatsCpu::setWindow should either specify the size of the whole
> processed (not image) area, or its cropping in case the stats shouldn't
> be gathered over the whole processed area.  This patch should clarify
> this in the code.
> 
> Reviewed-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> ---
>   src/libcamera/software_isp/debayer_cpu.cpp |  6 +++++-
>   src/libcamera/software_isp/swstats_cpu.cpp | 19 +++++++++++++++++++
>   2 files changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
> index 2dc85e5e0..bcaaa5dee 100644
> --- a/src/libcamera/software_isp/debayer_cpu.cpp
> +++ b/src/libcamera/software_isp/debayer_cpu.cpp
> @@ -554,7 +554,11 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg,
>   	window_.width = outputCfg.size.width;
>   	window_.height = outputCfg.size.height;
>   
> -	/* Don't pass x,y since process() already adjusts src before passing it */
> +	/*
> +	 * Set the stats window to the whole processed window. Its coordinates are
> +	 * relative to the debayered area since debayering passes only the part of
> +	 * data to be processed to the stats; see SwStatsCpu::setWindow.
> +	 */
>   	stats_->setWindow(Rectangle(window_.size()));
>   
>   	/* pad with patternSize.Width on both left and right side */
> diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp
> index e8a1d52f2..c936ef1dc 100644
> --- a/src/libcamera/software_isp/swstats_cpu.cpp
> +++ b/src/libcamera/software_isp/swstats_cpu.cpp
> @@ -416,6 +416,25 @@ int SwStatsCpu::configure(const StreamConfiguration &inputCfg)
>   /**
>    * \brief Specify window coordinates over which to gather statistics
>    * \param[in] window The window object.
> + *
> + * This method specifies the image area over which to gather the statistics.
> + * It must be called to set the area, otherwise the default zero-sized
> + * \a Rectangle is used and no statistics is gathered.
> + *
> + * The specified \a window is relative to what is passed to processLine*
> + * methods. Typically, this means processLine* methods get only data from the
> + * processed area and \a window is \a Rectangle with (0, 0) top-left point and
> + * of the same size as the processed area. But if statistics is gathered only
> + * from some part of the image, e.g. its centre, \a window should specify such a
> + * restriction accordingly.
> + *
> + * It is the responsibility of the callers to provide sensible \a window values,
> + * most notably not exceeding the original image boundaries.
> + *
> + * The method may adjust the window slightly if it is not aligned according to
> + * the bayer pattern determined in \a SwStatsCpu::configure(). If that happens,
> + * it is guaranteed that non-negative top-left point coordinates remain
> + * non-negative and that the window width and height don't get enlarged.

Why mention "non-negative"? I feel like, if anything, we should disallow negative
coordinates for the top left corner altogether.


Regards,
Barnabás Pőcze


>    */
>   void SwStatsCpu::setWindow(const Rectangle &window)
>   {

Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes:

> Hi
>
> 2025. 09. 25. 21:28 keltezéssel, Milan Zamazal írta:
>> The window coordinates passed to SwStatsCpu::setWindow are confusing.
>> Let's clarify what the coordinates should be.
>> A source of confusion is that the specified window is relative to the
>> processed area.  Debayering adjusts line pointers for its processed area
>> and this is what's also passed to stats processing.  The window passed
>> to SwStatsCpu::setWindow should either specify the size of the whole
>> processed (not image) area, or its cropping in case the stats shouldn't
>> be gathered over the whole processed area.  This patch should clarify
>> this in the code.
>> Reviewed-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
>> ---
>>   src/libcamera/software_isp/debayer_cpu.cpp |  6 +++++-
>>   src/libcamera/software_isp/swstats_cpu.cpp | 19 +++++++++++++++++++
>>   2 files changed, 24 insertions(+), 1 deletion(-)
>> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
>> index 2dc85e5e0..bcaaa5dee 100644
>> --- a/src/libcamera/software_isp/debayer_cpu.cpp
>> +++ b/src/libcamera/software_isp/debayer_cpu.cpp
>> @@ -554,7 +554,11 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg,
>>   	window_.width = outputCfg.size.width;
>>   	window_.height = outputCfg.size.height;
>>   -	/* Don't pass x,y since process() already adjusts src before passing it */
>> +	/*
>> +	 * Set the stats window to the whole processed window. Its coordinates are
>> +	 * relative to the debayered area since debayering passes only the part of
>> +	 * data to be processed to the stats; see SwStatsCpu::setWindow.
>> +	 */
>>   	stats_->setWindow(Rectangle(window_.size()));
>>     	/* pad with patternSize.Width on both left and right side */
>> diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp
>> index e8a1d52f2..c936ef1dc 100644
>> --- a/src/libcamera/software_isp/swstats_cpu.cpp
>> +++ b/src/libcamera/software_isp/swstats_cpu.cpp
>> @@ -416,6 +416,25 @@ int SwStatsCpu::configure(const StreamConfiguration &inputCfg)
>>   /**
>>    * \brief Specify window coordinates over which to gather statistics
>>    * \param[in] window The window object.
>> + *
>> + * This method specifies the image area over which to gather the statistics.
>> + * It must be called to set the area, otherwise the default zero-sized
>> + * \a Rectangle is used and no statistics is gathered.
>> + *
>> + * The specified \a window is relative to what is passed to processLine*
>> + * methods. Typically, this means processLine* methods get only data from the
>> + * processed area and \a window is \a Rectangle with (0, 0) top-left point and
>> + * of the same size as the processed area. But if statistics is gathered only
>> + * from some part of the image, e.g. its centre, \a window should specify such a
>> + * restriction accordingly.
>> + *
>> + * It is the responsibility of the callers to provide sensible \a window values,
>> + * most notably not exceeding the original image boundaries.
>> + *
>> + * The method may adjust the window slightly if it is not aligned according to
>> + * the bayer pattern determined in \a SwStatsCpu::configure(). If that happens,
>> + * it is guaranteed that non-negative top-left point coordinates remain
>> + * non-negative and that the window width and height don't get enlarged.
>
> Why mention "non-negative"? 

It describes the property of the implementation.  Without it, the
statement would be false.

> I feel like, if anything, we should disallow negative coordinates for
> the top left corner altogether.

The method doesn't provide a mechanism to disallow insane `window'
values (other than to change the values completely) and I don't think
it's needed for such a very internal call.  I think the preceding
paragraph about the caller responsibility is enough; the implementation
is sufficient for its purpose and the primary intended purpose of the
docstring is to make the life of readers of the code easier
(relative/absolute confusion etc.) rather than to define a robust API.

> Regards,
> Barnabás Pőcze
>
>
>>    */
>>   void SwStatsCpu::setWindow(const Rectangle &window)
>>   {

2025. 09. 26. 17:35 keltezéssel, Milan Zamazal írta:
> Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes:
> 
>> Hi
>>
>> 2025. 09. 25. 21:28 keltezéssel, Milan Zamazal írta:
>>> The window coordinates passed to SwStatsCpu::setWindow are confusing.
>>> Let's clarify what the coordinates should be.
>>> A source of confusion is that the specified window is relative to the
>>> processed area.  Debayering adjusts line pointers for its processed area
>>> and this is what's also passed to stats processing.  The window passed
>>> to SwStatsCpu::setWindow should either specify the size of the whole
>>> processed (not image) area, or its cropping in case the stats shouldn't
>>> be gathered over the whole processed area.  This patch should clarify
>>> this in the code.
>>> Reviewed-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
>>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
>>> ---
>>>    src/libcamera/software_isp/debayer_cpu.cpp |  6 +++++-
>>>    src/libcamera/software_isp/swstats_cpu.cpp | 19 +++++++++++++++++++
>>>    2 files changed, 24 insertions(+), 1 deletion(-)
>>> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
>>> index 2dc85e5e0..bcaaa5dee 100644
>>> --- a/src/libcamera/software_isp/debayer_cpu.cpp
>>> +++ b/src/libcamera/software_isp/debayer_cpu.cpp
>>> @@ -554,7 +554,11 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg,
>>>    	window_.width = outputCfg.size.width;
>>>    	window_.height = outputCfg.size.height;
>>>    -	/* Don't pass x,y since process() already adjusts src before passing it */
>>> +	/*
>>> +	 * Set the stats window to the whole processed window. Its coordinates are
>>> +	 * relative to the debayered area since debayering passes only the part of
>>> +	 * data to be processed to the stats; see SwStatsCpu::setWindow.
>>> +	 */
>>>    	stats_->setWindow(Rectangle(window_.size()));
>>>      	/* pad with patternSize.Width on both left and right side */
>>> diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp
>>> index e8a1d52f2..c936ef1dc 100644
>>> --- a/src/libcamera/software_isp/swstats_cpu.cpp
>>> +++ b/src/libcamera/software_isp/swstats_cpu.cpp
>>> @@ -416,6 +416,25 @@ int SwStatsCpu::configure(const StreamConfiguration &inputCfg)
>>>    /**
>>>     * \brief Specify window coordinates over which to gather statistics
>>>     * \param[in] window The window object.
>>> + *
>>> + * This method specifies the image area over which to gather the statistics.
>>> + * It must be called to set the area, otherwise the default zero-sized
>>> + * \a Rectangle is used and no statistics is gathered.
>>> + *
>>> + * The specified \a window is relative to what is passed to processLine*
>>> + * methods. Typically, this means processLine* methods get only data from the
>>> + * processed area and \a window is \a Rectangle with (0, 0) top-left point and
>>> + * of the same size as the processed area. But if statistics is gathered only
>>> + * from some part of the image, e.g. its centre, \a window should specify such a
>>> + * restriction accordingly.

The second sentence is a bit hard to parse for me, or rather, I think it would be
clearer to mention the typical case as an explicit example:

   The specified \a window is relative to what is passed to the processLine* methods. For example,
   if statistics are to be gathered from the entire processed area, then \a window should be a
   rectangle with the top-left corner of (0,0) and the same size as the processed area. If only
   a part of the processed area (e.g. its centre) is to be considered for statistics, then \a window
   should specify such a restriction accordingly.

Thoughts?


>>> + *
>>> + * It is the responsibility of the callers to provide sensible \a window values,
>>> + * most notably not exceeding the original image boundaries.
>>> + *
>>> + * The method may adjust the window slightly if it is not aligned according to
>>> + * the bayer pattern determined in \a SwStatsCpu::configure(). If that happens,
>>> + * it is guaranteed that non-negative top-left point coordinates remain
>>> + * non-negative and that the window width and height don't get enlarged.
>>
>> Why mention "non-negative"?
> 
> It describes the property of the implementation.  Without it, the
> statement would be false.

Negative y coordinates will stay negative, negative x coordinates will
stay negative (assuming patternSize_.{width,height} > 1). In any case,
I just feel like mentioning "non-negative" will immediately make the
reader wonder "but what about negative numbers?", at least it did to me.


> 
>> I feel like, if anything, we should disallow negative coordinates for
>> the top left corner altogether.
> 
> The method doesn't provide a mechanism to disallow insane `window'
> values (other than to change the values completely) and I don't think
> it's needed for such a very internal call.  I think the preceding

I was thinking of something like an ASSERT().


> paragraph about the caller responsibility is enough; the implementation
> is sufficient for its purpose and the primary intended purpose of the
> docstring is to make the life of readers of the code easier
> (relative/absolute confusion etc.) rather than to define a robust API.

Okay, let's not complicate things.


Regards,
Barnabás Pőcze


> 
>> Regards,
>> Barnabás Pőcze
>>
>>
>>>     */
>>>    void SwStatsCpu::setWindow(const Rectangle &window)
>>>    {
>

Hi,

On 25-Sep-25 21:28, Milan Zamazal wrote:
> The window coordinates passed to SwStatsCpu::setWindow are confusing.
> Let's clarify what the coordinates should be.
> 
> A source of confusion is that the specified window is relative to the
> processed area.  Debayering adjusts line pointers for its processed area
> and this is what's also passed to stats processing.  The window passed
> to SwStatsCpu::setWindow should either specify the size of the whole
> processed (not image) area, or its cropping in case the stats shouldn't
> be gathered over the whole processed area.  This patch should clarify
> this in the code.
> 
> Reviewed-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hansg@kernel.org>

Regards,

Hans


> ---
>  src/libcamera/software_isp/debayer_cpu.cpp |  6 +++++-
>  src/libcamera/software_isp/swstats_cpu.cpp | 19 +++++++++++++++++++
>  2 files changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
> index 2dc85e5e0..bcaaa5dee 100644
> --- a/src/libcamera/software_isp/debayer_cpu.cpp
> +++ b/src/libcamera/software_isp/debayer_cpu.cpp
> @@ -554,7 +554,11 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg,
>  	window_.width = outputCfg.size.width;
>  	window_.height = outputCfg.size.height;
>  
> -	/* Don't pass x,y since process() already adjusts src before passing it */
> +	/*
> +	 * Set the stats window to the whole processed window. Its coordinates are
> +	 * relative to the debayered area since debayering passes only the part of
> +	 * data to be processed to the stats; see SwStatsCpu::setWindow.
> +	 */
>  	stats_->setWindow(Rectangle(window_.size()));
>  
>  	/* pad with patternSize.Width on both left and right side */
> diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp
> index e8a1d52f2..c936ef1dc 100644
> --- a/src/libcamera/software_isp/swstats_cpu.cpp
> +++ b/src/libcamera/software_isp/swstats_cpu.cpp
> @@ -416,6 +416,25 @@ int SwStatsCpu::configure(const StreamConfiguration &inputCfg)
>  /**
>   * \brief Specify window coordinates over which to gather statistics
>   * \param[in] window The window object.
> + *
> + * This method specifies the image area over which to gather the statistics.
> + * It must be called to set the area, otherwise the default zero-sized
> + * \a Rectangle is used and no statistics is gathered.
> + *
> + * The specified \a window is relative to what is passed to processLine*
> + * methods. Typically, this means processLine* methods get only data from the
> + * processed area and \a window is \a Rectangle with (0, 0) top-left point and
> + * of the same size as the processed area. But if statistics is gathered only
> + * from some part of the image, e.g. its centre, \a window should specify such a
> + * restriction accordingly.
> + *
> + * It is the responsibility of the callers to provide sensible \a window values,
> + * most notably not exceeding the original image boundaries.
> + *
> + * The method may adjust the window slightly if it is not aligned according to
> + * the bayer pattern determined in \a SwStatsCpu::configure(). If that happens,
> + * it is guaranteed that non-negative top-left point coordinates remain
> + * non-negative and that the window width and height don't get enlarged.
>   */
>  void SwStatsCpu::setWindow(const Rectangle &window)
>  {

Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes:

> 2025. 09. 26. 17:35 keltezéssel, Milan Zamazal írta:
>> Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes:
>> 
>>> Hi
>>>
>>> 2025. 09. 25. 21:28 keltezéssel, Milan Zamazal írta:
>>>> The window coordinates passed to SwStatsCpu::setWindow are confusing.
>>>> Let's clarify what the coordinates should be.
>>>> A source of confusion is that the specified window is relative to the
>>>> processed area.  Debayering adjusts line pointers for its processed area
>>>> and this is what's also passed to stats processing.  The window passed
>>>> to SwStatsCpu::setWindow should either specify the size of the whole
>>>> processed (not image) area, or its cropping in case the stats shouldn't
>>>> be gathered over the whole processed area.  This patch should clarify
>>>> this in the code.
>>>> Reviewed-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
>>>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
>>>> ---
>>>>    src/libcamera/software_isp/debayer_cpu.cpp |  6 +++++-
>>>>    src/libcamera/software_isp/swstats_cpu.cpp | 19 +++++++++++++++++++
>>>>    2 files changed, 24 insertions(+), 1 deletion(-)
>>>> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
>>>> index 2dc85e5e0..bcaaa5dee 100644
>>>> --- a/src/libcamera/software_isp/debayer_cpu.cpp
>>>> +++ b/src/libcamera/software_isp/debayer_cpu.cpp
>>>> @@ -554,7 +554,11 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg,
>>>>    	window_.width = outputCfg.size.width;
>>>>    	window_.height = outputCfg.size.height;
>>>>    -	/* Don't pass x,y since process() already adjusts src before passing it */
>>>> +	/*
>>>> +	 * Set the stats window to the whole processed window. Its coordinates are
>>>> +	 * relative to the debayered area since debayering passes only the part of
>>>> +	 * data to be processed to the stats; see SwStatsCpu::setWindow.
>>>> +	 */
>>>>    	stats_->setWindow(Rectangle(window_.size()));
>>>>      	/* pad with patternSize.Width on both left and right side */
>>>> diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp
>>>> index e8a1d52f2..c936ef1dc 100644
>>>> --- a/src/libcamera/software_isp/swstats_cpu.cpp
>>>> +++ b/src/libcamera/software_isp/swstats_cpu.cpp
>>>> @@ -416,6 +416,25 @@ int SwStatsCpu::configure(const StreamConfiguration &inputCfg)
>>>>    /**
>>>>     * \brief Specify window coordinates over which to gather statistics
>>>>     * \param[in] window The window object.
>>>> + *
>>>> + * This method specifies the image area over which to gather the statistics.
>>>> + * It must be called to set the area, otherwise the default zero-sized
>>>> + * \a Rectangle is used and no statistics is gathered.
>>>> + *
>>>> + * The specified \a window is relative to what is passed to processLine*
>>>> + * methods. Typically, this means processLine* methods get only data from the
>>>> + * processed area and \a window is \a Rectangle with (0, 0) top-left point and
>>>> + * of the same size as the processed area. But if statistics is gathered only
>>>> + * from some part of the image, e.g. its centre, \a window should specify such a
>>>> + * restriction accordingly.
>
> The second sentence is a bit hard to parse for me, or rather, I think it would be
> clearer to mention the typical case as an explicit example:
>
>   The specified \a window is relative to what is passed to the processLine* methods. For example,
>   if statistics are to be gathered from the entire processed area, then \a window should be a
>   rectangle with the top-left corner of (0,0) and the same size as the processed area. If only
>   a part of the processed area (e.g. its centre) is to be considered for statistics, then \a window
>   should specify such a restriction accordingly.
>
> Thoughts?

OK for me.

>>>> + *
>>>> + * It is the responsibility of the callers to provide sensible \a window values,
>>>> + * most notably not exceeding the original image boundaries.
>>>> + *
>>>> + * The method may adjust the window slightly if it is not aligned according to
>>>> + * the bayer pattern determined in \a SwStatsCpu::configure(). If that happens,
>>>> + * it is guaranteed that non-negative top-left point coordinates remain
>>>> + * non-negative and that the window width and height don't get enlarged.
>>>
>>> Why mention "non-negative"?
>> It describes the property of the implementation.  Without it, the
>> statement would be false.
>
> Negative y coordinates will stay negative, negative x coordinates will
> stay negative (assuming patternSize_.{width,height} > 1). In any case,
> I just feel like mentioning "non-negative" will immediately make the
> reader wonder "but what about negative numbers?", at least it did to me.

Not mentioned => undefined.  Can you perhaps propose a different text?
 
>>> I feel like, if anything, we should disallow negative coordinates for
>>> the top left corner altogether.
>> The method doesn't provide a mechanism to disallow insane `window'
>> values (other than to change the values completely) and I don't think
>> it's needed for such a very internal call.  I think the preceding
>
> I was thinking of something like an ASSERT().

I see.  It can be added for negative values.  We don't know the image
size here, so we can't check for the maximum values.

>> paragraph about the caller responsibility is enough; the implementation
>> is sufficient for its purpose and the primary intended purpose of the
>> docstring is to make the life of readers of the code easier
>> (relative/absolute confusion etc.) rather than to define a robust API.
>
> Okay, let's not complicate things.
>
>
> Regards,
> Barnabás Pőcze
>
>
>> 
>>> Regards,
>>> Barnabás Pőcze
>>>
>>>
>>>>     */
>>>>    void SwStatsCpu::setWindow(const Rectangle &window)
>>>>    {
>>

Hi

2025. 09. 29. 15:06 keltezéssel, Milan Zamazal írta:
> Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes:
> 
>> 2025. 09. 26. 17:35 keltezéssel, Milan Zamazal írta:
>>> Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes:
>>>
>>>> Hi
>>>>
>>>> 2025. 09. 25. 21:28 keltezéssel, Milan Zamazal írta:
>>>>> The window coordinates passed to SwStatsCpu::setWindow are confusing.
>>>>> Let's clarify what the coordinates should be.
>>>>> A source of confusion is that the specified window is relative to the
>>>>> processed area.  Debayering adjusts line pointers for its processed area
>>>>> and this is what's also passed to stats processing.  The window passed
>>>>> to SwStatsCpu::setWindow should either specify the size of the whole
>>>>> processed (not image) area, or its cropping in case the stats shouldn't
>>>>> be gathered over the whole processed area.  This patch should clarify
>>>>> this in the code.
>>>>> Reviewed-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
>>>>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
>>>>> ---
>>>>>     src/libcamera/software_isp/debayer_cpu.cpp |  6 +++++-
>>>>>     src/libcamera/software_isp/swstats_cpu.cpp | 19 +++++++++++++++++++
>>>>>     2 files changed, 24 insertions(+), 1 deletion(-)
>>>>> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
>>>>> index 2dc85e5e0..bcaaa5dee 100644
>>>>> --- a/src/libcamera/software_isp/debayer_cpu.cpp
>>>>> +++ b/src/libcamera/software_isp/debayer_cpu.cpp
>>>>> @@ -554,7 +554,11 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg,
>>>>>     	window_.width = outputCfg.size.width;
>>>>>     	window_.height = outputCfg.size.height;
>>>>>     -	/* Don't pass x,y since process() already adjusts src before passing it */
>>>>> +	/*
>>>>> +	 * Set the stats window to the whole processed window. Its coordinates are
>>>>> +	 * relative to the debayered area since debayering passes only the part of
>>>>> +	 * data to be processed to the stats; see SwStatsCpu::setWindow.
>>>>> +	 */
>>>>>     	stats_->setWindow(Rectangle(window_.size()));
>>>>>       	/* pad with patternSize.Width on both left and right side */
>>>>> diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp
>>>>> index e8a1d52f2..c936ef1dc 100644
>>>>> --- a/src/libcamera/software_isp/swstats_cpu.cpp
>>>>> +++ b/src/libcamera/software_isp/swstats_cpu.cpp
>>>>> @@ -416,6 +416,25 @@ int SwStatsCpu::configure(const StreamConfiguration &inputCfg)
>>>>>     /**
>>>>>      * \brief Specify window coordinates over which to gather statistics
>>>>>      * \param[in] window The window object.
>>>>> + *
>>>>> + * This method specifies the image area over which to gather the statistics.
>>>>> + * It must be called to set the area, otherwise the default zero-sized
>>>>> + * \a Rectangle is used and no statistics is gathered.
>>>>> + *
>>>>> + * The specified \a window is relative to what is passed to processLine*
>>>>> + * methods. Typically, this means processLine* methods get only data from the
>>>>> + * processed area and \a window is \a Rectangle with (0, 0) top-left point and
>>>>> + * of the same size as the processed area. But if statistics is gathered only
>>>>> + * from some part of the image, e.g. its centre, \a window should specify such a
>>>>> + * restriction accordingly.
>>
>> The second sentence is a bit hard to parse for me, or rather, I think it would be
>> clearer to mention the typical case as an explicit example:
>>
>>    The specified \a window is relative to what is passed to the processLine* methods. For example,
>>    if statistics are to be gathered from the entire processed area, then \a window should be a
>>    rectangle with the top-left corner of (0,0) and the same size as the processed area. If only
>>    a part of the processed area (e.g. its centre) is to be considered for statistics, then \a window
>>    should specify such a restriction accordingly.
>>
>> Thoughts?
> 
> OK for me.
> 
>>>>> + *
>>>>> + * It is the responsibility of the callers to provide sensible \a window values,
>>>>> + * most notably not exceeding the original image boundaries.

Could you add something like "Neither coordinate of the top-left corner shall be negative."
here somewhere in some way?


>>>>> + *
>>>>> + * The method may adjust the window slightly if it is not aligned according to
>>>>> + * the bayer pattern determined in \a SwStatsCpu::configure(). If that happens,
>>>>> + * it is guaranteed that non-negative top-left point coordinates remain
>>>>> + * non-negative and that the window width and height don't get enlarged.
>>>>
>>>> Why mention "non-negative"?
>>> It describes the property of the implementation.  Without it, the
>>> statement would be false.
>>
>> Negative y coordinates will stay negative, negative x coordinates will
>> stay negative (assuming patternSize_.{width,height} > 1). In any case,
>> I just feel like mentioning "non-negative" will immediately make the
>> reader wonder "but what about negative numbers?", at least it did to me.
> 
> Not mentioned => undefined.  Can you perhaps propose a different text?

Fair enough. I don't have a good suggestion. If the adjusted window remained
inside the original window, that would be easier to express succinctly; but
since the x,y coordinates are rounded towards -infinity, that is not true...

Maybe the following:

   Due to limitations of the implementation, the method may adjust the window slightly
   if it is not aligned according to the bayer pattern determined in \a SwStatsCpu::configure().
   In that case the window will be modified such that the sides are no larger than the original,
   and that the new bottom-left corner will be no further from (0,0) (along either axis) than the
   original was.

But let's not dwell on it too much, I have already wasted enough of our time with this, sorry.


>   
>>>> I feel like, if anything, we should disallow negative coordinates for
>>>> the top left corner altogether.
>>> The method doesn't provide a mechanism to disallow insane `window'
>>> values (other than to change the values completely) and I don't think
>>> it's needed for such a very internal call.  I think the preceding
>>
>> I was thinking of something like an ASSERT().
> 
> I see.  It can be added for negative values.  We don't know the image
> size here, so we can't check for the maximum values.

Feel free to decide if you want to add the `ASSERT(... >= 0)`.


Regards,
Barnabás Pőcze


> 
>>> paragraph about the caller responsibility is enough; the implementation
>>> is sufficient for its purpose and the primary intended purpose of the
>>> docstring is to make the life of readers of the code easier
>>> (relative/absolute confusion etc.) rather than to define a robust API.
>>
>> Okay, let's not complicate things.
>>
>>
>> Regards,
>> Barnabás Pőcze
>>
>>
>>>
>>>> Regards,
>>>> Barnabás Pőcze
>>>>
>>>>
>>>>>      */
>>>>>     void SwStatsCpu::setWindow(const Rectangle &window)
>>>>>     {
>>>
>