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

[Naushir Patuck]

Hi Kieran,

On Wed, 3 Mar 2021 at 11:00, Kieran Bingham <kieran.bingham at ideasonboard.com>
wrote:

> 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?
>

Sorry, my bad.  I had run the tests, and did not see a failure.  However,
on closer
inspection, the test was marked as SKIPPED, as I do not have the "vivid
video"
device driver available.

Let me see what's involved in getting this device available to use on our
kernel, and I will update the tests to reflect the new state of the world.

Regards,
Naush


>
> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210303/a68432ae/attachment.htm>