[libcamera-devel,v4,5/7] libcamera: delayed_controls: Fix off-by-one error in get()
diff mbox series

Message ID 20210304081728.1058394-6-naush@raspberrypi.com
State Accepted
Headers show
Series
  • DelayedControls updates and fixes
Related show

Commit Message

Naushir Patuck March 4, 2021, 8:17 a.m. UTC 
There was an off-by-one error in DelayedControls::get() when picking
controls from the queue to return back to the pipeline handler.
This is only noticeable as small oscillations in brightness when closely
viewing frame while AGC is running. The old StaggeredCtrl did not show
this error as the startup queuing mechanism has changed in
DelayedControls.

Fix this by indexing to the correct position in the queue.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
Reported-by: David Plowman <david.plowman@raspberrypi.com>
Fixes: 3d4b7b005911 ("libcamera: delayed_controls: Add helper for controls that apply with a delay")
Tested-by: David Plowman <david.plowman@raspberrypi.com>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
---
 src/libcamera/delayed_controls.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Jean-Michel Hautbois March 9, 2021, 9:56 a.m. UTC | #1 
On 04/03/2021 09:17, Naushir Patuck wrote:
> There was an off-by-one error in DelayedControls::get() when picking
> controls from the queue to return back to the pipeline handler.
> This is only noticeable as small oscillations in brightness when closely
> viewing frame while AGC is running. The old StaggeredCtrl did not show
> this error as the startup queuing mechanism has changed in
> DelayedControls.
> 
> Fix this by indexing to the correct position in the queue.
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Reported-by: David Plowman <david.plowman@raspberrypi.com>
> Fixes: 3d4b7b005911 ("libcamera: delayed_controls: Add helper for controls that apply with a delay")
> Tested-by: David Plowman <david.plowman@raspberrypi.com>
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
Tested-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> ---
>  src/libcamera/delayed_controls.cpp | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp
> index 5a05741c0285..138761c9852e 100644
> --- a/src/libcamera/delayed_controls.cpp
> +++ b/src/libcamera/delayed_controls.cpp
> @@ -184,7 +184,7 @@ bool DelayedControls::push(const ControlList &controls)
>   */
>  ControlList DelayedControls::get(uint32_t sequence)
>  {
> -	uint32_t adjustedSeq = sequence - firstSequence_ + 1;
> +	uint32_t adjustedSeq = sequence - firstSequence_;
>  	unsigned int index = std::max<int>(0, adjustedSeq - maxDelay_);
>  
>  	ControlList out(device_->controls());
>
Kieran Bingham March 12, 2021, 1:34 p.m. UTC | #2 
On 04/03/2021 08:17, Naushir Patuck wrote:
> There was an off-by-one error in DelayedControls::get() when picking
> controls from the queue to return back to the pipeline handler.
> This is only noticeable as small oscillations in brightness when closely
> viewing frame while AGC is running. The old StaggeredCtrl did not show
> this error as the startup queuing mechanism has changed in
> DelayedControls.
> 
> Fix this by indexing to the correct position in the queue.
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Reported-by: David Plowman <david.plowman@raspberrypi.com>
> Fixes: 3d4b7b005911 ("libcamera: delayed_controls: Add helper for controls that apply with a delay")
> Tested-by: David Plowman <david.plowman@raspberrypi.com>
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> ---
>  src/libcamera/delayed_controls.cpp | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp
> index 5a05741c0285..138761c9852e 100644
> --- a/src/libcamera/delayed_controls.cpp
> +++ b/src/libcamera/delayed_controls.cpp
> @@ -184,7 +184,7 @@ bool DelayedControls::push(const ControlList &controls)
>   */
>  ControlList DelayedControls::get(uint32_t sequence)
>  {
> -	uint32_t adjustedSeq = sequence - firstSequence_ + 1;
> +	uint32_t adjustedSeq = sequence - firstSequence_;
>  	unsigned int index = std::max<int>(0, adjustedSeq - maxDelay_);
>  
>  	ControlList out(device_->controls());
>

Patch
diff mbox series 

diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp
index 5a05741c0285..138761c9852e 100644
--- a/src/libcamera/delayed_controls.cpp
+++ b/src/libcamera/delayed_controls.cpp
@@ -184,7 +184,7 @@  bool DelayedControls::push(const ControlList &controls)
  */
 ControlList DelayedControls::get(uint32_t sequence)
 {
-	uint32_t adjustedSeq = sequence - firstSequence_ + 1;
+	uint32_t adjustedSeq = sequence - firstSequence_;
 	unsigned int index = std::max<int>(0, adjustedSeq - maxDelay_);
 
 	ControlList out(device_->controls());