[libcamera-devel] [PATCH v3 5/5] libcamera: delayed_controls: Fix off-by-one error in get()

[Kieran Bingham]

Hi Naush,

On 01/03/2021 13:31, 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.

It seems this behaviour is currently expected by the test framework.

Running the tests (whole suite with `ninja test`) identifies a failure on:


> 44/61 libcamera / delayed_contols                                        FAIL            0.12s   (exit status 255 or signal 127 SIGinvalid)


Running this test alone on each patch in the series:

> git rebase -i libcamera.org/master -x "ninja -C build && ./build/test/delayed_contols"

stops at this patch.

Could you also investigate the test, and update it accordingly in this
patch to make sure that it is correct and doesn't fail please?

I anticipate that this is just a case of needing to update the tests to
match, if we assume it is the test that is wrong in this instance, but
we should check that too, and make sure we're not breaking some other
assumptions elsewhere.

--
Kieran


> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> Reported-by: David Plowman <david.plowman at raspberrypi.com>
> Fixes: 3d4b7b005911 ("libcamera: delayed_controls: Add helper for controls that apply with a delay")
> Tested-by: David Plowman <david.plowman at raspberrypi.com>
> Reviewed-by: Paul Elder <paul.elder at 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());
> 

-- 
Regards
--
Kieran