[v3,1/6] libcamera: software_isp: Clarify SwStatsCpu::setWindow use
diff mbox series

Message ID 20250919173709.220881-2-mzamazal@redhat.com
State Superseded
Headers show
Series
  • Fix stats related problems in software ISP
Related show

Commit Message

Milan Zamazal Sept. 19, 2025, 5:37 p.m. UTC
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 | 16 ++++++++++++++++
 2 files changed, 21 insertions(+), 1 deletion(-)

Comments

Barnabás Pőcze Sept. 25, 2025, 10:58 a.m. UTC | #1
Hi

2025. 09. 19. 19:37 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 | 16 ++++++++++++++++
>   2 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
> index 66f6038c1..bcc847ae6 100644
> --- a/src/libcamera/software_isp/debayer_cpu.cpp
> +++ b/src/libcamera/software_isp/debayer_cpu.cpp
> @@ -541,7 +541,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 4b77b3600..72aa88b69 100644
> --- a/src/libcamera/software_isp/swstats_cpu.cpp
> +++ b/src/libcamera/software_isp/swstats_cpu.cpp
> @@ -416,6 +416,22 @@ 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 method may adjust the window slightly if it is not aligned according to
> + * the bayer pattern determined in \a SwStatsCpu::configure(). Such an
> + * adjustment is guaranteed to not exceed the bounds of
> + * Rectangle(0, 0, window.width, window.height).

I think that's not entirely true. If one wants to set the window to an empty
window (i.e. `width == 0`) and the pattern is GBRG or RGGB with no packing, then
`xShift_ == 1`, thus

	/* width_ - xShift_ to make sure the window fits */
	window_.width -= xShift_;
	window_.width &= ~(patternSize_.width - 1);

will result in `window_.width == -2u == 4294967294` as far as I can see.

Also, it says

   guaranteed to not exceed the bounds of Rectangle(0, 0, window.width, window.height).

but is that true if the top-left corner is not (0,0)? Shouldn't it say

    Rectangle(0, 0, window.x + window.width, window.y + window.height)

or similar?


Regards,
Barnabás Pőcze

>    */
>   void SwStatsCpu::setWindow(const Rectangle &window)
>   {
Milan Zamazal Sept. 25, 2025, 11:24 a.m. UTC | #2
Hi Barnabás,

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

> Hi
>
> 2025. 09. 19. 19:37 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 | 16 ++++++++++++++++
>>   2 files changed, 21 insertions(+), 1 deletion(-)
>> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
>> index 66f6038c1..bcc847ae6 100644
>> --- a/src/libcamera/software_isp/debayer_cpu.cpp
>> +++ b/src/libcamera/software_isp/debayer_cpu.cpp
>> @@ -541,7 +541,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 4b77b3600..72aa88b69 100644
>> --- a/src/libcamera/software_isp/swstats_cpu.cpp
>> +++ b/src/libcamera/software_isp/swstats_cpu.cpp
>> @@ -416,6 +416,22 @@ 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 method may adjust the window slightly if it is not aligned according to
>> + * the bayer pattern determined in \a SwStatsCpu::configure(). Such an
>> + * adjustment is guaranteed to not exceed the bounds of
>> + * Rectangle(0, 0, window.width, window.height).
>
> I think that's not entirely true. If one wants to set the window to an empty
> window (i.e. `width == 0`) and the pattern is GBRG or RGGB with no packing, then
> `xShift_ == 1`, thus
>
> 	/* width_ - xShift_ to make sure the window fits */
> 	window_.width -= xShift_;
> 	window_.width &= ~(patternSize_.width - 1);
>
> will result in `window_.width == -2u == 4294967294` as far as I can see.
>
> Also, it says
>
>   guaranteed to not exceed the bounds of Rectangle(0, 0, window.width, window.height).
>
> but is that true if the top-left corner is not (0,0)? Shouldn't it say
>
>    Rectangle(0, 0, window.x + window.width, window.y + window.height)
>
> or similar?

You're right.  From my point of view, the documentation should stay as
it is (I think those are meaningful requirements) and the implementation
of the method should be changed to comply.  Do you agree?

> Regards,
> Barnabás Pőcze
>
>>    */
>>   void SwStatsCpu::setWindow(const Rectangle &window)
>>   {
Barnabás Pőcze Sept. 25, 2025, 12:03 p.m. UTC | #3
2025. 09. 25. 13:24 keltezéssel, Milan Zamazal írta:
> Hi Barnabás,
> 
> Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes:
> 
>> Hi
>>
>> 2025. 09. 19. 19:37 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 | 16 ++++++++++++++++
>>>    2 files changed, 21 insertions(+), 1 deletion(-)
>>> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
>>> index 66f6038c1..bcc847ae6 100644
>>> --- a/src/libcamera/software_isp/debayer_cpu.cpp
>>> +++ b/src/libcamera/software_isp/debayer_cpu.cpp
>>> @@ -541,7 +541,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 4b77b3600..72aa88b69 100644
>>> --- a/src/libcamera/software_isp/swstats_cpu.cpp
>>> +++ b/src/libcamera/software_isp/swstats_cpu.cpp
>>> @@ -416,6 +416,22 @@ 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 method may adjust the window slightly if it is not aligned according to
>>> + * the bayer pattern determined in \a SwStatsCpu::configure(). Such an
>>> + * adjustment is guaranteed to not exceed the bounds of
>>> + * Rectangle(0, 0, window.width, window.height).
>>
>> I think that's not entirely true. If one wants to set the window to an empty
>> window (i.e. `width == 0`) and the pattern is GBRG or RGGB with no packing, then
>> `xShift_ == 1`, thus
>>
>> 	/* width_ - xShift_ to make sure the window fits */
>> 	window_.width -= xShift_;
>> 	window_.width &= ~(patternSize_.width - 1);
>>
>> will result in `window_.width == -2u == 4294967294` as far as I can see.
>>
>> Also, it says
>>
>>    guaranteed to not exceed the bounds of Rectangle(0, 0, window.width, window.height).
>>
>> but is that true if the top-left corner is not (0,0)? Shouldn't it say
>>
>>     Rectangle(0, 0, window.x + window.width, window.y + window.height)
>>
>> or similar?
> 
> You're right.  From my point of view, the documentation should stay as
> it is (I think those are meaningful requirements) and the implementation
> of the method should be changed to comply.  Do you agree?

I think I misunderstand something. If the proposed description is kept, then if
one wants `(20, 20, 200, 200)`, then in my interpretation it couldn't be adjusted
to something reasonable like `(22, 22, 188, 188)` because that is outside `(0, 0, 200, 200)`.

I am also realizing now that the meaning of "Such an adjustment is guaranteed to not exceed the bounds of [...]"
is not clear to me. I think it should be expressed in terms of the "adjusted window" or similar.


> 
>> Regards,
>> Barnabás Pőcze
>>
>>>     */
>>>    void SwStatsCpu::setWindow(const Rectangle &window)
>>>    {
>
Milan Zamazal Sept. 25, 2025, 4:48 p.m. UTC | #4
Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes:

> 2025. 09. 25. 13:24 keltezéssel, Milan Zamazal írta:
>> Hi Barnabás,
>> Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes:
>> 
>>> Hi
>>>
>>> 2025. 09. 19. 19:37 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 | 16 ++++++++++++++++
>>>>    2 files changed, 21 insertions(+), 1 deletion(-)
>>>> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
>>>> index 66f6038c1..bcc847ae6 100644
>>>> --- a/src/libcamera/software_isp/debayer_cpu.cpp
>>>> +++ b/src/libcamera/software_isp/debayer_cpu.cpp
>>>> @@ -541,7 +541,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 4b77b3600..72aa88b69 100644
>>>> --- a/src/libcamera/software_isp/swstats_cpu.cpp
>>>> +++ b/src/libcamera/software_isp/swstats_cpu.cpp
>>>> @@ -416,6 +416,22 @@ 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 method may adjust the window slightly if it is not aligned according to
>>>> + * the bayer pattern determined in \a SwStatsCpu::configure(). Such an
>>>> + * adjustment is guaranteed to not exceed the bounds of
>>>> + * Rectangle(0, 0, window.width, window.height).
>>>
>>> I think that's not entirely true. If one wants to set the window to an empty
>>> window (i.e. `width == 0`) and the pattern is GBRG or RGGB with no packing, then
>>> `xShift_ == 1`, thus
>>>
>>> 	/* width_ - xShift_ to make sure the window fits */
>>> 	window_.width -= xShift_;
>>> 	window_.width &= ~(patternSize_.width - 1);
>>>
>>> will result in `window_.width == -2u == 4294967294` as far as I can see.
>>>
>>> Also, it says
>>>
>>>    guaranteed to not exceed the bounds of Rectangle(0, 0, window.width, window.height).
>>>
>>> but is that true if the top-left corner is not (0,0)? Shouldn't it say
>>>
>>>     Rectangle(0, 0, window.x + window.width, window.y + window.height)
>>>
>>> or similar?
>> You're right.  From my point of view, the documentation should stay as
>> it is (I think those are meaningful requirements) and the implementation
>> of the method should be changed to comply.  Do you agree?
>
> I think I misunderstand something. If the proposed description is kept, then if
> one wants `(20, 20, 200, 200)`, then in my interpretation it couldn't be adjusted
> to something reasonable like `(22, 22, 188, 188)` because that is outside `(0, 0, 200, 200)`.

Ah, I understand now.  What I wanted to say in the docstring was: If the
requested window is (x, y, w, h) and the adjusted window is
(x', y', w', h') then x' >= 0, y' >= 0, w' <= w, h' <= h.  Which is what
the implementation does (minus the zero-size corner case).
"Rectangle(0, 0, window.width, window.height)" is indeed a wrong
expression of that,
"Rectangle(0, 0, window.x + window.width, window.y + window.height)" is
also not completely accurate (it doesn't guarantee w' <= w).

> I am also realizing now that the meaning of "Such an adjustment is guaranteed to not exceed the bounds of
> [...]"
> is not clear to me. I think it should be expressed in terms of the "adjusted window" or similar.

Yes, I'll try to find a better description (and fix the implementation).

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

Patch
diff mbox series

diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
index 66f6038c1..bcc847ae6 100644
--- a/src/libcamera/software_isp/debayer_cpu.cpp
+++ b/src/libcamera/software_isp/debayer_cpu.cpp
@@ -541,7 +541,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 4b77b3600..72aa88b69 100644
--- a/src/libcamera/software_isp/swstats_cpu.cpp
+++ b/src/libcamera/software_isp/swstats_cpu.cpp
@@ -416,6 +416,22 @@  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 method may adjust the window slightly if it is not aligned according to
+ * the bayer pattern determined in \a SwStatsCpu::configure(). Such an
+ * adjustment is guaranteed to not exceed the bounds of
+ * Rectangle(0, 0, window.width, window.height).
  */
 void SwStatsCpu::setWindow(const Rectangle &window)
 {