[libcamera-devel] [PATCH v4 6/7] test: delayed_controls: Fixup tests after recent DelayedControls changes

[Naushir Patuck]

Hi Kieran,


On Fri, 12 Mar 2021 at 13:38, Kieran Bingham <
kieran.bingham at ideasonboard.com> wrote:

> Hi Naush,
>
>
> On 04/03/2021 08:17, Naushir Patuck wrote:
> > The recent fixes applied to DelayedControls change the behavior of the
> > library. As such, the tests ended up failing as they relied on the old
> > behavior of the helper. Update the tests to account for the new behavior
> > and get the tests passing again.
> >
> > Specifically, the following changes have been made for each test:
> >
> > singleControlNoDelay():
> > - Add a call to reset() to initialise internal DelayedControls state
> > only after setting the control on the device.
> > - Trigger a first frame start by calling applyControls() as a pipeline
> > handler would.
> > - Test frames from 1 onwards, as we will never have a queue item at
> > frame 0, since the reset() handles that.
> >
> > singleControlWithDelay():
> > - Trigger a first frame start by calling applyControls() as a pipeline
> > handler would.
> > - Test frames from 1 onwards, as we will never have a queue item at
> > frame 0, since the reset() handles that.
> >
> > dualControlsWithDelay() and dualControlsMultiQueue():
> > - Trigger a first frame start by calling applyControls() as a pipeline
> > handler would.
> > - Test frames from (startOffset + 1) onwards, as we will never have a
> > queue item at frame startOffset.
> > - Use the max delay (2 in this case) to determine the expected result
> > value.
> >
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
>
> I wondered if these should be squashed into the commits that change it -
> but I don't think it's a problem.
>

I did consider that before submitting this version.  However, it does get a
bit
unwieldy.  No one previous commit fixes the overall behaviour of the helper,
and as such, there would be churn in the test code for each commit, and not
just incremental fixes, if you get what I mean?

Regards,
Naush


> Hard to track all these offsets :-( I'm glad you've created a tool to
> help debugging!
>

Yes, the parsing script has kept me sane when trying to debug these
oscillations :)

Regards,
Naush



>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> > ---
> >  test/delayed_contols.cpp | 35 ++++++++++++++++++++++++++---------
> >  1 file changed, 26 insertions(+), 9 deletions(-)
> >
> > diff --git a/test/delayed_contols.cpp b/test/delayed_contols.cpp
> > index 3855eb18ecd4..c6f195b7bc7f 100644
> > --- a/test/delayed_contols.cpp
> > +++ b/test/delayed_contols.cpp
> > @@ -82,9 +82,13 @@ protected:
> >               /* Reset control to value not used in test. */
> >               ctrls.set(V4L2_CID_BRIGHTNESS, 1);
> >               dev_->setControls(&ctrls);
> > +             delayed->reset();
> > +
> > +             /* Trigger the first frame start event */
> > +             delayed->applyControls(0);
> >
> >               /* Test control without delay are set at once. */
> > -             for (unsigned int i = 0; i < 100; i++) {
> > +             for (unsigned int i = 1; i < 100; i++) {
> >                       int32_t value = 100 + i;
> >
> >                       ctrls.set(V4L2_CID_BRIGHTNESS, value);
> > @@ -122,8 +126,11 @@ protected:
> >               dev_->setControls(&ctrls);
> >               delayed->reset();
> >
> > +             /* Trigger the first frame start event */
> > +             delayed->applyControls(0);
> > +
> >               /* Test single control with delay. */
> > -             for (unsigned int i = 0; i < 100; i++) {
> > +             for (unsigned int i = 1; i < 100; i++) {
> >                       int32_t value = 10 + i;
> >
> >                       ctrls.set(V4L2_CID_BRIGHTNESS, value);
> > @@ -150,9 +157,11 @@ protected:
> >
> >       int dualControlsWithDelay(uint32_t startOffset)
> >       {
> > +             static const unsigned int maxDelay = 2;
> > +
> >               std::unordered_map<uint32_t,
> DelayedControls::ControlParams> delays = {
> >                       { V4L2_CID_BRIGHTNESS, { 1, false } },
> > -                     { V4L2_CID_CONTRAST, { 2, false } },
> > +                     { V4L2_CID_CONTRAST, { maxDelay, false } },
> >               };
> >               std::unique_ptr<DelayedControls> delayed =
> >                       std::make_unique<DelayedControls>(dev_.get(),
> delays);
> > @@ -165,8 +174,11 @@ protected:
> >               dev_->setControls(&ctrls);
> >               delayed->reset();
> >
> > +             /* Trigger the first frame start event */
> > +             delayed->applyControls(startOffset);
> > +
> >               /* Test dual control with delay. */
> > -             for (unsigned int i = 0; i < 100; i++) {
> > +             for (unsigned int i = 1; i < 100; i++) {
> >                       uint32_t frame = startOffset + i;
> >                       int32_t value = 10 + i;
> >
> > @@ -189,7 +201,7 @@ protected:
> >                               return TestFail;
> >                       }
> >
> > -                     expected = i < 1 ? expected : value - 1;
> > +                     expected = i < maxDelay ? expected : value - 1;
> >               }
> >
> >               return TestPass;
> > @@ -197,9 +209,11 @@ protected:
> >
> >       int dualControlsMultiQueue()
> >       {
> > +             static const unsigned int maxDelay = 2;
> > +
> >               std::unordered_map<uint32_t,
> DelayedControls::ControlParams> delays = {
> >                       { V4L2_CID_BRIGHTNESS, { 1, false } },
> > -                     { V4L2_CID_CONTRAST, { 2, false } }
> > +                     { V4L2_CID_CONTRAST, { maxDelay, false } }
> >               };
> >               std::unique_ptr<DelayedControls> delayed =
> >                       std::make_unique<DelayedControls>(dev_.get(),
> delays);
> > @@ -212,6 +226,9 @@ protected:
> >               dev_->setControls(&ctrls);
> >               delayed->reset();
> >
> > +             /* Trigger the first frame start event */
> > +             delayed->applyControls(0);
> > +
> >               /*
> >                * Queue all controls before any fake frame start. Note we
> >                * can't queue up more then the delayed controls history
> size
> > @@ -226,8 +243,8 @@ protected:
> >               }
> >
> >               /* Process all queued controls. */
> > -             for (unsigned int i = 0; i < 16; i++) {
> > -                     int32_t value = 10 + i;
> > +             for (unsigned int i = 1; i < 16; i++) {
> > +                     int32_t value = 10 + i - 1;
> >
> >                       delayed->applyControls(i);
> >
> > @@ -245,7 +262,7 @@ protected:
> >                               return TestFail;
> >                       }
> >
> > -                     expected = i < 1 ? expected : value - 1;
> > +                     expected = i < maxDelay ? expected : value - 1;
> >               }
> >
> >               return TestPass;
> >
>
> --
> Regards
> --
> Kieran
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210312/e8e861f6/attachment-0001.htm>