| Message ID | 20220705184350.31369-1-jacopo@jmondi.org |
|---|---|
| State | Accepted |
| Commit | 2778c3298ca421e2ec670c10d8bab7beaa78f5e9 |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Quoting Jacopo Mondi via libcamera-devel (2022-07-05 19:43:50) > Commit 6f539a6d2fa9 ("delayed_controls: Remove reduandant firstSequence_") > removed support for frame number start offset from the DelayedControls > class, as it is now guaranteed that the first sequence number as it comes > from the V4L2VideoDevice will always be 0. > > However the delayed_controls.cpp unit still has two tests that passes > a non-zero first sequence number to the DelayedControl class, causing > the test to spin forever and consequentially fail. > Ayeee, I can't believe I missed this. I must have glanced too casually at the test results and placed more attention on the fact that CTS had succeed! > Remove the two tests from the unit to fix this. > > The first removed test was testing the class against frame start > sequence numbers greater than zero and can safely be removed. > > The second test was instead validating the class against sequence number > overflow, which is now not possible to test anymore as the DelayedControls > class now assumes 0 as first frame sequence number. Hrm ... that's a bit of a shame, but I don't think it's too troublesome just yet. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Fixes: 6f539a6d2fa9 ("delayed_controls: Remove reduandant firstSequence_") > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > test/delayed_controls.cpp | 23 ++++++----------------- > 1 file changed, 6 insertions(+), 17 deletions(-) > > diff --git a/test/delayed_controls.cpp b/test/delayed_controls.cpp > index c6f195b7bc7f..a8ce9828d73d 100644 > --- a/test/delayed_controls.cpp > +++ b/test/delayed_controls.cpp > @@ -155,7 +155,7 @@ protected: > return TestPass; > } > > - int dualControlsWithDelay(uint32_t startOffset) > + int dualControlsWithDelay() > { > static const unsigned int maxDelay = 2; > > @@ -175,25 +175,24 @@ protected: > delayed->reset(); > > /* Trigger the first frame start event */ > - delayed->applyControls(startOffset); > + delayed->applyControls(0); > > /* Test dual control with delay. */ > for (unsigned int i = 1; i < 100; i++) { > - uint32_t frame = startOffset + i; > int32_t value = 10 + i; > > ctrls.set(V4L2_CID_BRIGHTNESS, value); > ctrls.set(V4L2_CID_CONTRAST, value + 1); > delayed->push(ctrls); > > - delayed->applyControls(frame); > + delayed->applyControls(i); > > - ControlList result = delayed->get(frame); > + ControlList result = delayed->get(i); > int32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get<int32_t>(); > int32_t contrast = result.get(V4L2_CID_CONTRAST).get<int32_t>(); > if (brightness != expected || contrast != expected + 1) { > cerr << "Failed dual controls" > - << " frame " << frame > + << " frame " << i > << " brightness " << brightness > << " contrast " << contrast > << " expected " << expected > @@ -283,17 +282,7 @@ protected: > return ret; > > /* Test dual controls with different delays. */ > - ret = dualControlsWithDelay(0); > - if (ret) > - return ret; > - > - /* Test dual controls with non-zero sequence start. */ > - ret = dualControlsWithDelay(10000); > - if (ret) > - return ret; > - > - /* Test dual controls with sequence number wraparound. */ > - ret = dualControlsWithDelay(UINT32_MAX - 50); > + ret = dualControlsWithDelay(); > if (ret) > return ret; > > -- > 2.36.1 >
Hi Jacopo, Thank you for the patch. On Tue, Jul 05, 2022 at 08:43:50PM +0200, Jacopo Mondi via libcamera-devel wrote: > Commit 6f539a6d2fa9 ("delayed_controls: Remove reduandant firstSequence_") > removed support for frame number start offset from the DelayedControls > class, as it is now guaranteed that the first sequence number as it comes > from the V4L2VideoDevice will always be 0. > > However the delayed_controls.cpp unit still has two tests that passes > a non-zero first sequence number to the DelayedControl class, causing > the test to spin forever and consequentially fail. > > Remove the two tests from the unit to fix this. > > The first removed test was testing the class against frame start > sequence numbers greater than zero and can safely be removed. > > The second test was instead validating the class against sequence number > overflow, which is now not possible to test anymore as the DelayedControls > class now assumes 0 as first frame sequence number. > > Fixes: 6f539a6d2fa9 ("delayed_controls: Remove reduandant firstSequence_") > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > test/delayed_controls.cpp | 23 ++++++----------------- > 1 file changed, 6 insertions(+), 17 deletions(-) > > diff --git a/test/delayed_controls.cpp b/test/delayed_controls.cpp > index c6f195b7bc7f..a8ce9828d73d 100644 > --- a/test/delayed_controls.cpp > +++ b/test/delayed_controls.cpp > @@ -155,7 +155,7 @@ protected: > return TestPass; > } > > - int dualControlsWithDelay(uint32_t startOffset) > + int dualControlsWithDelay() > { > static const unsigned int maxDelay = 2; > > @@ -175,25 +175,24 @@ protected: > delayed->reset(); > > /* Trigger the first frame start event */ > - delayed->applyControls(startOffset); > + delayed->applyControls(0); > > /* Test dual control with delay. */ > for (unsigned int i = 1; i < 100; i++) { > - uint32_t frame = startOffset + i; > int32_t value = 10 + i; > > ctrls.set(V4L2_CID_BRIGHTNESS, value); > ctrls.set(V4L2_CID_CONTRAST, value + 1); > delayed->push(ctrls); > > - delayed->applyControls(frame); > + delayed->applyControls(i); > > - ControlList result = delayed->get(frame); > + ControlList result = delayed->get(i); > int32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get<int32_t>(); > int32_t contrast = result.get(V4L2_CID_CONTRAST).get<int32_t>(); > if (brightness != expected || contrast != expected + 1) { > cerr << "Failed dual controls" > - << " frame " << frame > + << " frame " << i > << " brightness " << brightness > << " contrast " << contrast > << " expected " << expected > @@ -283,17 +282,7 @@ protected: > return ret; > > /* Test dual controls with different delays. */ > - ret = dualControlsWithDelay(0); > - if (ret) > - return ret; > - > - /* Test dual controls with non-zero sequence start. */ > - ret = dualControlsWithDelay(10000); > - if (ret) > - return ret; > - > - /* Test dual controls with sequence number wraparound. */ > - ret = dualControlsWithDelay(UINT32_MAX - 50); > + ret = dualControlsWithDelay(); > if (ret) > return ret; >
Hi Kieran, On Tue, Jul 05, 2022 at 08:13:40PM +0100, Kieran Bingham via libcamera-devel wrote: > Hi Jacopo, > > Quoting Jacopo Mondi via libcamera-devel (2022-07-05 19:43:50) > > Commit 6f539a6d2fa9 ("delayed_controls: Remove reduandant firstSequence_") > > removed support for frame number start offset from the DelayedControls > > class, as it is now guaranteed that the first sequence number as it comes > > from the V4L2VideoDevice will always be 0. > > > > However the delayed_controls.cpp unit still has two tests that passes > > a non-zero first sequence number to the DelayedControl class, causing > > the test to spin forever and consequentially fail. > > Ayeee, I can't believe I missed this. I must have glanced too casually > at the test results and placed more attention on the fact that CTS had > succeed! Are there unit tests that normally fail for you ? > > Remove the two tests from the unit to fix this. > > > > The first removed test was testing the class against frame start > > sequence numbers greater than zero and can safely be removed. > > > > The second test was instead validating the class against sequence number > > overflow, which is now not possible to test anymore as the DelayedControls > > class now assumes 0 as first frame sequence number. > > Hrm ... that's a bit of a shame, but I don't think it's too troublesome > just yet. > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > Fixes: 6f539a6d2fa9 ("delayed_controls: Remove reduandant firstSequence_") > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > test/delayed_controls.cpp | 23 ++++++----------------- > > 1 file changed, 6 insertions(+), 17 deletions(-) > > > > diff --git a/test/delayed_controls.cpp b/test/delayed_controls.cpp > > index c6f195b7bc7f..a8ce9828d73d 100644 > > --- a/test/delayed_controls.cpp > > +++ b/test/delayed_controls.cpp > > @@ -155,7 +155,7 @@ protected: > > return TestPass; > > } > > > > - int dualControlsWithDelay(uint32_t startOffset) > > + int dualControlsWithDelay() > > { > > static const unsigned int maxDelay = 2; > > > > @@ -175,25 +175,24 @@ protected: > > delayed->reset(); > > > > /* Trigger the first frame start event */ > > - delayed->applyControls(startOffset); > > + delayed->applyControls(0); > > > > /* Test dual control with delay. */ > > for (unsigned int i = 1; i < 100; i++) { > > - uint32_t frame = startOffset + i; > > int32_t value = 10 + i; > > > > ctrls.set(V4L2_CID_BRIGHTNESS, value); > > ctrls.set(V4L2_CID_CONTRAST, value + 1); > > delayed->push(ctrls); > > > > - delayed->applyControls(frame); > > + delayed->applyControls(i); > > > > - ControlList result = delayed->get(frame); > > + ControlList result = delayed->get(i); > > int32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get<int32_t>(); > > int32_t contrast = result.get(V4L2_CID_CONTRAST).get<int32_t>(); > > if (brightness != expected || contrast != expected + 1) { > > cerr << "Failed dual controls" > > - << " frame " << frame > > + << " frame " << i > > << " brightness " << brightness > > << " contrast " << contrast > > << " expected " << expected > > @@ -283,17 +282,7 @@ protected: > > return ret; > > > > /* Test dual controls with different delays. */ > > - ret = dualControlsWithDelay(0); > > - if (ret) > > - return ret; > > - > > - /* Test dual controls with non-zero sequence start. */ > > - ret = dualControlsWithDelay(10000); > > - if (ret) > > - return ret; > > - > > - /* Test dual controls with sequence number wraparound. */ > > - ret = dualControlsWithDelay(UINT32_MAX - 50); > > + ret = dualControlsWithDelay(); > > if (ret) > > return ret; > >
diff --git a/test/delayed_controls.cpp b/test/delayed_controls.cpp index c6f195b7bc7f..a8ce9828d73d 100644 --- a/test/delayed_controls.cpp +++ b/test/delayed_controls.cpp @@ -155,7 +155,7 @@ protected: return TestPass; } - int dualControlsWithDelay(uint32_t startOffset) + int dualControlsWithDelay() { static const unsigned int maxDelay = 2; @@ -175,25 +175,24 @@ protected: delayed->reset(); /* Trigger the first frame start event */ - delayed->applyControls(startOffset); + delayed->applyControls(0); /* Test dual control with delay. */ for (unsigned int i = 1; i < 100; i++) { - uint32_t frame = startOffset + i; int32_t value = 10 + i; ctrls.set(V4L2_CID_BRIGHTNESS, value); ctrls.set(V4L2_CID_CONTRAST, value + 1); delayed->push(ctrls); - delayed->applyControls(frame); + delayed->applyControls(i); - ControlList result = delayed->get(frame); + ControlList result = delayed->get(i); int32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get<int32_t>(); int32_t contrast = result.get(V4L2_CID_CONTRAST).get<int32_t>(); if (brightness != expected || contrast != expected + 1) { cerr << "Failed dual controls" - << " frame " << frame + << " frame " << i << " brightness " << brightness << " contrast " << contrast << " expected " << expected @@ -283,17 +282,7 @@ protected: return ret; /* Test dual controls with different delays. */ - ret = dualControlsWithDelay(0); - if (ret) - return ret; - - /* Test dual controls with non-zero sequence start. */ - ret = dualControlsWithDelay(10000); - if (ret) - return ret; - - /* Test dual controls with sequence number wraparound. */ - ret = dualControlsWithDelay(UINT32_MAX - 50); + ret = dualControlsWithDelay(); if (ret) return ret;
Commit 6f539a6d2fa9 ("delayed_controls: Remove reduandant firstSequence_") removed support for frame number start offset from the DelayedControls class, as it is now guaranteed that the first sequence number as it comes from the V4L2VideoDevice will always be 0. However the delayed_controls.cpp unit still has two tests that passes a non-zero first sequence number to the DelayedControl class, causing the test to spin forever and consequentially fail. Remove the two tests from the unit to fix this. The first removed test was testing the class against frame start sequence numbers greater than zero and can safely be removed. The second test was instead validating the class against sequence number overflow, which is now not possible to test anymore as the DelayedControls class now assumes 0 as first frame sequence number. Fixes: 6f539a6d2fa9 ("delayed_controls: Remove reduandant firstSequence_") Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- test/delayed_controls.cpp | 23 ++++++----------------- 1 file changed, 6 insertions(+), 17 deletions(-)