<div dir="ltr"><div dir="ltr">Hi Kieran,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 3 Mar 2021 at 11:00, Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com">kieran.bingham@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Naush,<br>
<br>
On 01/03/2021 13:31, Naushir Patuck wrote:<br>
> There was an off-by-one error in DelayedControls::get() when picking<br>
> controls from the queue to return back to the pipeline handler.<br>
> This is only noticeable as small oscillations in brightness when closely<br>
> viewing frame while AGC is running. The old StaggeredCtrl did not show<br>
> this error as the startup queuing mechanism has changed in<br>
> DelayedControls.<br>
> <br>
> Fix this by indexing to the correct position in the queue.<br>
<br>
It seems this behaviour is currently expected by the test framework.<br>
<br>
Running the tests (whole suite with `ninja test`) identifies a failure on:<br>
<br>
<br>
> 44/61 libcamera / delayed_contols                                        FAIL            0.12s   (exit status 255 or signal 127 SIGinvalid)<br>
<br>
<br>
Running this test alone on each patch in the series:<br>
<br>
> git rebase -i <a href="http://libcamera.org/master" rel="noreferrer" target="_blank">libcamera.org/master</a> -x "ninja -C build && ./build/test/delayed_contols"<br>
<br>
stops at this patch.<br>
<br>
Could you also investigate the test, and update it accordingly in this<br>
patch to make sure that it is correct and doesn't fail please?<br></blockquote><div><br></div><div>Sorry, my bad.  I had run the tests, and did not see a failure.  However, on closer</div><div>inspection, the test was marked as SKIPPED, as I do not have the "vivid video"</div><div>device driver available.</div><div><br></div><div>Let me see what's involved in getting this device available to use on our</div><div>kernel, and I will update the tests to reflect the new state of the world.</div><div><br></div><div>Regards,</div><div>Naush</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
I anticipate that this is just a case of needing to update the tests to<br>
match, if we assume it is the test that is wrong in this instance, but<br>
we should check that too, and make sure we're not breaking some other<br>
assumptions elsewhere.<br>
<br>
--<br>
Kieran<br>
<br>
<br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> Reported-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
> Fixes: 3d4b7b005911 ("libcamera: delayed_controls: Add helper for controls that apply with a delay")<br>
> Tested-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
> Reviewed-by: Paul Elder <<a href="mailto:paul.elder@ideasonboard.com" target="_blank">paul.elder@ideasonboard.com</a>><br>
> ---<br>
>  src/libcamera/delayed_controls.cpp | 2 +-<br>
>  1 file changed, 1 insertion(+), 1 deletion(-)<br>
> <br>
> diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp<br>
> index 5a05741c0285..138761c9852e 100644<br>
> --- a/src/libcamera/delayed_controls.cpp<br>
> +++ b/src/libcamera/delayed_controls.cpp<br>
> @@ -184,7 +184,7 @@ bool DelayedControls::push(const ControlList &controls)<br>
>   */<br>
>  ControlList DelayedControls::get(uint32_t sequence)<br>
>  {<br>
> -     uint32_t adjustedSeq = sequence - firstSequence_ + 1;<br>
> +     uint32_t adjustedSeq = sequence - firstSequence_;<br>
>       unsigned int index = std::max<int>(0, adjustedSeq - maxDelay_);<br>
>  <br>
>       ControlList out(device_->controls());<br>
> <br>
<br>
-- <br>
Regards<br>
--<br>
Kieran<br>
</blockquote></div></div>