[PATCH v2 03/12] libcamera: lc-compliance: Add initial set of per-frame-control tests
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Thu Mar 21 12:56:16 CET 2024
Hi Stefan
On Thu, Mar 21, 2024 at 12:34:49PM +0100, Stefan Klug wrote:
> Hi Jacopo,
>
> On Thu, Mar 21, 2024 at 09:47:53AM +0100, Jacopo Mondi wrote:
> > Hi Stefan
> >
> > On Fri, Mar 15, 2024 at 04:53:03PM +0100, Stefan Klug wrote:
> > > Hi Jacopo,
> > >
> > > thanks for the review.
> > >
> > > On Fri, Mar 15, 2024 at 03:42:39PM +0100, Jacopo Mondi wrote:
> > > > Hi Stefan
> > > >
> > > > On Wed, Mar 13, 2024 at 01:12:14PM +0100, Stefan Klug wrote:
> > > > > These tests check if controls (only exposure time and analogue gain at
> > > > > the moment) get applied on the frame they were requested for.
> > > > >
> > > > > This is tested by looking at the metadata and the mean brightness
> > > > > of the image center.
> > > > >
> > > > > At the moment these tests fail. Fixes for the pipelines will be delivered
> > > > > in later patches.
> > > >
> > > > Nice!
> > > >
> > > > >
> > > > > To run only the teste, one can run:
> > > >
> > > > s/teste/test
> > > >
> > > > > lc-compliance -c <cam> -f "SingleStream.*"
> > > > >
> > > > > Note that the current implementation is a bit picky on what the camera
> > > > > actually sees. If it is too dark (or too bright), the tests will fail.
> > > > > Looking at a white wall in a normally lit office usually works.
> > > >
> > > > Mmm, is this ok for a compliance suite ? Is this the reason the image
> > > > tests are 'optional' ?
> > >
> > > Yes, in the beginning I wasn't sure how far we should go in first place.
> > > You basically have two options here:
> > > - Dynamically finding a "base" exposure time that works (e.g. bright
> > > enough that you see a difference when the values increase and dim
> > > enough to not saturate any pixel). This might work but has a large
> > > dependency on the efficiency of the sensor and might still be flaky.
> > > - Building and distributing a physical testrig with defined brightness.
> > > This is cool, but a larger task.
> > >
> > > So I started small added this 'optional' term so that we could collect
> > > some practical experience on how stable these tests are on different
> > > devices.
> > >
> > > >
> > > > >
> > > > > Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > > > > ---
> > > > > src/apps/lc-compliance/capture_test.cpp | 46 +++
> > > > > src/apps/lc-compliance/meson.build | 1 +
> > > > > src/apps/lc-compliance/per_frame_controls.cpp | 316 ++++++++++++++++++
> > > > > src/apps/lc-compliance/per_frame_controls.h | 43 +++
> > > > > 4 files changed, 406 insertions(+)
> > > > > create mode 100644 src/apps/lc-compliance/per_frame_controls.cpp
> > > > > create mode 100644 src/apps/lc-compliance/per_frame_controls.h
> > > > >
> > > > > diff --git a/src/apps/lc-compliance/capture_test.cpp b/src/apps/lc-compliance/capture_test.cpp
> > > > > index 1dcfcf92..b19e8936 100644
> > > > > --- a/src/apps/lc-compliance/capture_test.cpp
> > > > > +++ b/src/apps/lc-compliance/capture_test.cpp
> > > > > @@ -11,6 +11,7 @@
> > > > > #include <gtest/gtest.h>
> > > > >
> > > > > #include "environment.h"
> > > > > +#include "per_frame_controls.h"
> > > > > #include "simple_capture.h"
> > > > >
> > > > > using namespace libcamera;
> > > > > @@ -133,3 +134,48 @@ INSTANTIATE_TEST_SUITE_P(CaptureTests,
> > > > > testing::Combine(testing::ValuesIn(ROLES),
> > > > > testing::ValuesIn(NUMREQUESTS)),
> > > > > SingleStream::nameParameters);
> > > > > +
> > > > > +/*
> > > > > + * Test Per frame controls
> > > > > + */
> > > > > +TEST_F(SingleStream, testExposureGainChangeOnSameFrame)
> > > > > +{
> > > > > + PerFrameControls capture(camera_);
> > > > > + capture.configure(StreamRole::VideoRecording);
> > > > > + capture.testExposureGainChangeOnSameFrame();
> > > > > +}
> > > > > +
> > > > > +TEST_F(SingleStream, testFramePreciseExposureChange)
> > > > > +{
> > > > > + PerFrameControls capture(camera_);
> > > > > + capture.configure(StreamRole::VideoRecording);
> > > > > + capture.testFramePreciseExposureChange();
> > > > > +}
> > > > > +
> > > > > +TEST_F(SingleStream, testFramePreciseGainChange)
> > > > > +{
> > > > > + PerFrameControls capture(camera_);
> > > > > + capture.configure(StreamRole::VideoRecording);
> > > > > + capture.testFramePreciseGainChange();
> > > > > +}
> > > > > +
> > > > > +TEST_F(SingleStream, testExposureGainIsAppliedOnFirstFrame)
> > > > > +{
> > > > > + PerFrameControls capture(camera_);
> > > > > + capture.configure(StreamRole::VideoRecording);
> > > > > + capture.testExposureGainIsAppliedOnFirstFrame();
> > > > > +}
> > > > > +
> > > > > +TEST_F(SingleStream, testExposureGainFromFirstRequestGetsApplied)
> > > > > +{
> > > > > + PerFrameControls capture(camera_);
> > > > > + capture.configure(StreamRole::VideoRecording);
> > > > > + capture.testExposureGainFromFirstRequestGetsApplied();
> > > > > +}
> > > > > +
> > > > > +TEST_F(SingleStream, testExposureGainFromFirstAndSecondRequestGetsApplied)
> > > > > +{
> > > > > + PerFrameControls capture(camera_);
> > > > > + capture.configure(StreamRole::VideoRecording);
> > > > > + capture.testExposureGainFromFirstAndSecondRequestGetsApplied();
> > > > > +}
> > > >
> > > > This now shows up as
> > > >
> > > > SingleStream.
> > > > testExposureGainChangeOnSameFrame
> > > > testFramePreciseExposureChange
> > > > testFramePreciseGainChange
> > > > testExposureGainIsAppliedOnFirstFrame
> > > > testExposureGainFromFirstRequestGetsApplied
> > > > testExposureGainFromFirstAndSecondRequestGetsApplied
> > > >
> > > > And we already have
> > > >
> > > > CaptureTests/SingleStream.
> > > > Capture/Raw_1 # GetParam() = (Raw, 1)
> > > > Capture/Raw_2 # GetParam() = (Raw, 2)
> > > > Capture/Raw_3 # GetParam() = (Raw, 3)
> > > > Capture/Raw_5 # GetParam() = (Raw, 5)
> > > > Capture/Raw_8 # GetParam() = (Raw, 8)
> > > > Capture/Raw_13 # GetParam() = (Raw, 13)
> > > > Capture/Raw_21 # GetParam() = (Raw, 21)
> > > > Capture/Raw_34 # GetParam() = (Raw, 34)
> > > > Capture/Raw_55 # GetParam() = (Raw, 55)
> > > > Capture/Raw_89 # GetParam() = (Raw, 89)
> > > >
> > > > I would have not instantiated these tests in capture_test.cpp but
> > > > directly in per_frame_control.cpp and I would have named them
> > > > "PerFrameControl". To do so you need to define a test class that
> > > > derives from testing::Test in per_frame_control.cpp
> > > >
> > > >
> > > > +/*
> > > > + * Test Per frame controls
> > > > + */
> > > > +
> > > > +class PerFrameControlTest : public testing::Test
> > > > +{
> > > > +protected:
> > > > + void SetUp() override;
> > > > + void TearDown() override;
> > > > +
> > > > + std::shared_ptr<Camera> camera_;
> > > > +};
> > > > +
> > > > +void PerFrameControlTest::SetUp()
> > > > +{
> > > > + Environment *env = Environment::get();
> > > > +
> > > > + camera_ = env->cm()->get(env->cameraId());
> > > > +
> > > > + ASSERT_EQ(camera_->acquire(), 0);
> > > > +}
> > > > +
> > > > +void PerFrameControlTest::TearDown()
> > > > +{
> > > > + if (!camera_)
> > > > + return;
> > > > +
> > > > + camera_->release();
> > > > + camera_.reset();
> > > > +}
> > > > +
> > > > +TEST_F(PerFrameControlTest, testExposureGainChangeOnSameFrame)
> > > > .....
> > > >
> > > > With this you get a dedicated test suite
> > > >
> > > > PerFrameControlTest.
> > > > testExposureGainChangeOnSameFrame
> > > > testFramePreciseExposureChange
> > > > testFramePreciseGainChange
> > > > testExposureGainIsAppliedOnFirstFrame
> > > > testExposureGainFromFirstRequestGetsApplied
> > > > testExposureGainFromFirstAndSecondRequestGetsApplied
> > > >
> > > > Also, you now can drop the
> > > >
> > > > void testExposureGainChangeOnSameFrame();
> > > > void testFramePreciseExposureChange();
> > > > void testFramePreciseGainChange();
> > > > void testExposureGainIsAppliedOnFirstFrame();
> > > > void testExposureGainFromFirstRequestGetsApplied();
> > > > void testExposureGainFromFirstAndSecondRequestGetsApplied();
> > > >
> > > > functions from the PerFrameControl class, and implement the tests in
> > > > the test definition instead of having them as wrappers that call the
> > > > PerFrameControl class' functions
> > > >
> > > > TL;DR do this:
> > > >
> > > > TEST_F(PerFrameControlTest, testExposureGainChangeOnSameFrame)
> > > > {
> > > > PerFrameControls capture(camera_);
> > > > capture.configure(StreamRole::VideoRecording);
> > > >
> > > > ControlList startValues;
> > > > startValues.set(controls::ExposureTime, 5000);
> > > > startValues.set(controls::AnalogueGain, 1.0);
> > > >
> > > > auto timeSheet = capture.startCaptureWithTimeSheet(10, &startValues);
> > > > auto &ts = *timeSheet;
> > > >
> > > > /* wait a few frames to settle */
> > > > ts[7].controls().set(controls::ExposureTime, 10000);
> > > > ts[7].controls().set(controls::AnalogueGain, 4.0);
> > > >
> > > > ...
> > > > }
> > > >
> > > >
> > > > in place of:
> > > >
> > > > TEST_F(PerFrameControlTest, testFramePreciseExposureChange)
> > > > {
> > > > PerFrameControls capture(camera_);
> > > > capture.configure(StreamRole::VideoRecording);
> > > > capture.testFramePreciseExposureChange();
> > > > }
> > > >
> > >
> > > Great. Thanks for that. I didn't want to spend too much time inside
> > > google test before getting feedback on the overall direction. Your
> > > proposal is perfect. I'll do that.
> > >
> > > >
> > > >
> > > > > diff --git a/src/apps/lc-compliance/meson.build b/src/apps/lc-compliance/meson.build
> > > > > index eb7b2d71..2a6f52af 100644
> > > > > --- a/src/apps/lc-compliance/meson.build
> > > > > +++ b/src/apps/lc-compliance/meson.build
> > > > > @@ -15,6 +15,7 @@ lc_compliance_sources = files([
> > > > > 'capture_test.cpp',
> > > > > 'environment.cpp',
> > > > > 'main.cpp',
> > > > > + 'per_frame_controls.cpp',
> > > > > 'simple_capture.cpp',
> > > > > 'time_sheet.cpp',
> > > > > ])
> > > > > diff --git a/src/apps/lc-compliance/per_frame_controls.cpp b/src/apps/lc-compliance/per_frame_controls.cpp
> > > > > new file mode 100644
> > > > > index 00000000..eb7164e0
> > > > > --- /dev/null
> > > > > +++ b/src/apps/lc-compliance/per_frame_controls.cpp
> > > > > @@ -0,0 +1,316 @@
> > > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > > > +/*
> > > > > + * Copyright (C) 2024, Ideas on Board Oy
> > > > > + *
> > > > > + * per_frame_controls.cpp - Tests for per frame controls
> > > > > + */
> > > > > +#include "per_frame_controls.h"
> > > > > +
> > > > > +#include <gtest/gtest.h>
> > > > > +
> > > > > +#include "time_sheet.h"
> > > > > +
> > > > > +using namespace libcamera;
> > > > > +
> > > > > +static const bool doImageTests = true;
> > > > > +
> > > > > +PerFrameControls::PerFrameControls(std::shared_ptr<Camera> camera)
> > > > > + : SimpleCapture(camera)
> > > > > +{
> > > > > +}
> > > > > +
> > > > > +std::shared_ptr<TimeSheet>
> > > > > +PerFrameControls::startCaptureWithTimeSheet(unsigned int framesToCapture, const ControlList *controls)
> > > > > +{
> > > > > + ControlList ctrls(camera_->controls().idmap());
> > > >
> > > > Empty line please
> > > >
> > > > > + /* Ensure defined default values */
> > > > > + ctrls.set(controls::AeEnable, false);
> > > > > + ctrls.set(controls::AeExposureMode, controls::ExposureCustom);
> > > > > + ctrls.set(controls::ExposureTime, 10000);
> > > > > + ctrls.set(controls::AnalogueGain, 1.0);
> > > > > +
> > > > > + if (controls)
> > > > > + ctrls.merge(*controls, ControlList::MergePolicy::OverwriteExisting);
> > > > > +
> > > > > + start(&ctrls);
> > > > > +
> > > > > + queueCount_ = 0;
> > > > > + captureCount_ = 0;
> > > > > + captureLimit_ = framesToCapture;
> > > > > +
> > > > > + auto timeSheet = std::make_shared<TimeSheet>(captureLimit_, camera_->controls().idmap());
> > > > > + timeSheet_ = timeSheet;
> > > >
> > > > I'm sorry but I don't see why you would use a shared_ptr<> and a weak
> > > > reference when the timeSheet_ ownership is not shared with any other
> > > > component outside of this class
> > >
> > > This class is still based on the Capture class. So you are free to write
> > > tests and capture without a timesheet. In such tests the weak ptr will
> > > automatically be empty and the queueRequest and requestComplete
> > > functions still work properly. The lifetime of the timesheet is bound to
> > > the scope of the test function. (You could even use the
> > > startCaptureWithTimeSheet() and ignore the result, in wich case the
> > > timesheet would get destroyed immediately).
> > >
> >
> > Let me re-cap (I'm looking at v3)
> >
> > The PerFrameControlsCapture class:
> >
> > - has a member std::weak_ptr<TimeSheet> timeSheet_
> > - has a
> > std::shared_ptr<TimeSheet>
> > startCaptureWithTimeSheet(unsigned int framesToCapture,
> > const libcamera::ControlList *controls = nullptr);
> >
> > function that
> >
> > * creates a TimeSheet as shared_ptr<> and returns it to the caller
> > * Initialize the weak_ref<> class member with the shared_ptr<>
> >
> > auto timeSheet = std::make_shared<TimeSheet>(captureLimit_,
> > camera_->controls().idmap());
> > timeSheet_ = timeSheet;
> > return timeSheet;
> >
> > so, if I read it right, you create a shared_ptr<> (refcount = 1),
> > initialize a weak_ptr<> (no reference count increment) return a
> > shared_ptr<> by copy (refcount = 2) and the end of the function scope
> > the local shared_ptr<> is destroyed (refcount = 1). Now the pointer
> > ownership is on the caller only, so you basically trasferred the
> > ownership outside of the class and refers to that within the class
> > with a weak_ptr<>.
>
> That's was exactly the plan. The only reason for the weak_ptr to exist
> at all, is to be able to access a potential timesheet inside requestComplete
> and queueRequest.
>
> >
> > In facts
> >
> > --- a/src/apps/lc-compliance/per_frame_controls_test.cpp
> > +++ b/src/apps/lc-compliance/per_frame_controls_test.cpp
> > @@ -157,6 +157,8 @@ TEST_F(PerFrameControlTests, testExposureGainChangeOnSameFrame)
> > startValues.set(controls::AnalogueGain, 1.0);
> >
> > auto timeSheet = capture.startCaptureWithTimeSheet(10, &startValues);
> > + std::cerr << " TIMESHEET USE COUNT: " << timeSheet.use_count();
> > +
> > auto &ts = *timeSheet;
> >
> > Gives me
> > TIMESHEET USE COUNT: 1
> >
> > if ever the caller does
> >
> > {
> > ts = capture.startCaptureWithTimeSheet(10, &startValues);
> > }
> >
> > The instance of the TimeSheet class the timeSheet_ member refers to gets
> > destroyed. It's 'safe' as it's a weak_ptr<> but I don't think that's what you
> > want.
>
> No, that was intended. As the lifetime of the timesheet shall be bound
> to the test function and not to the lifetime of the
> PerFrameControlsCapture instance.
>
Ah ok, it's weird than that the timesheet is created by the
PerFrameControlsCapture class then.
> But I think I understand what bothers you. And thinking about that I
> came to a solution that might be easier to follow. What about the
> following proposal (not compiled though):
>
> - startCaptureWithTimsheet becomes a simple startCapture
> - timesheet in instanciated in the test and passed as pointer to
> runCaptureSession.
> - that also removes the nasty "auto &ts = *timesheet" inside the tests-
>
I like it ! Let's make it clear what owns what to avoid passing
ownerships around!
>
> diff --git a/src/apps/lc-compliance/per_frame_controls_test.cpp b/src/apps/lc-compliance/per_frame_controls_test.cpp
> index 589ef517..d98ab3a7 100644
> --- a/src/apps/lc-compliance/per_frame_controls_test.cpp
> +++ b/src/apps/lc-compliance/per_frame_controls_test.cpp
> @@ -48,11 +48,10 @@ class PerFrameControlsCapture : public SimpleCapture
> public:
> PerFrameControlsCapture(std::shared_ptr<libcamera::Camera> camera);
>
> - std::shared_ptr<TimeSheet>
> - startCaptureWithTimeSheet(unsigned int framesToCapture,
> + void startCapture(unsigned int framesToCapture,
> const libcamera::ControlList *controls = nullptr);
>
> - void runCaptureSession();
> + void runCaptureSession(TimeSheet* ts);
> int queueRequest(libcamera::Request *request);
> void requestComplete(libcamera::Request *request) override;
>
> @@ -60,7 +59,7 @@ public:
> unsigned int captureCount_;
> unsigned int captureLimit_;
>
> - std::weak_ptr<TimeSheet> timeSheet_;
> + TimeSheet* sessionTimeSheet_;
> };
>
> static const bool doImageTests = true;
> @@ -70,8 +69,7 @@ PerFrameControlsCapture::PerFrameControlsCapture(std::shared_ptr<Camera> camera)
> {
> }
>
> -std::shared_ptr<TimeSheet>
> -PerFrameControlsCapture::startCaptureWithTimeSheet(unsigned int framesToCapture,
> +void PerFrameControlsCapture::startCapture(unsigned int framesToCapture,
> const ControlList *controls)
> {
> ControlList ctrls(camera_->controls().idmap());
> @@ -90,10 +88,6 @@ PerFrameControlsCapture::startCaptureWithTimeSheet(unsigned int framesToCapture,
> queueCount_ = 0;
> captureCount_ = 0;
> captureLimit_ = framesToCapture;
> -
> - auto timeSheet = std::make_shared<TimeSheet>(captureLimit_, camera_);
> - timeSheet_ = timeSheet;
> - return timeSheet;
> }
>
> int PerFrameControlsCapture::queueRequest(Request *request)
> @@ -102,18 +96,16 @@ int PerFrameControlsCapture::queueRequest(Request *request)
> if (queueCount_ > captureLimit_)
> return 0;
>
> - auto ts = timeSheet_.lock();
> - if (ts)
> - ts->prepareForQueue(request, queueCount_ - 1);
> + if (sessionTimeSheet_)
> + sessionTimeSheet_->prepareForQueue(request, queueCount_ - 1);
>
> return camera_->queueRequest(request);
> }
>
> void PerFrameControlsCapture::requestComplete(Request *request)
> {
> - auto ts = timeSheet_.lock();
> - if (ts)
> - ts->handleCompleteRequest(request);
> + if (sessionTimeSheet_)
> + sessionTimeSheet_->handleCompleteRequest(request);
>
> captureCount_++;
> if (captureCount_ >= captureLimit_) {
> @@ -126,11 +118,13 @@ void PerFrameControlsCapture::requestComplete(Request *request)
> loop_->exit(-EINVAL);
> }
>
> -void PerFrameControlsCapture::runCaptureSession()
> +void PerFrameControlsCapture::runCaptureSession(TimeSheet* ts)
> {
> Stream *stream = config_->at(0).stream();
> const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_->buffers(stream);
>
> + sessionTimeSheet_ = ts;
> +
> /* Queue the recommended number of requests. */
> for (const std::unique_ptr<FrameBuffer> &buffer : buffers) {
> std::unique_ptr<Request> request = camera_->createRequest();
> @@ -144,6 +138,7 @@ void PerFrameControlsCapture::runCaptureSession()
> loop_->exec();
> stop();
> delete loop_;
> + sessionTimeSheet_ = nullptr;
> }
>
> TEST_F(PerFrameControlTests, testExposureGainChangeOnSameFrame)
> @@ -155,14 +150,14 @@ TEST_F(PerFrameControlTests, testExposureGainChangeOnSameFrame)
> startValues.set(controls::ExposureTime, 5000);
> startValues.set(controls::AnalogueGain, 1.0);
>
> - auto timeSheet = capture.startCaptureWithTimeSheet(10, &startValues);
> - auto &ts = *timeSheet;
> + capture.startCaptureWithTimeSheet(10, &startValues);
>
> + TimeSheet ts(10);
> /* wait a few frames to settle */
> ts[7].controls().set(controls::ExposureTime, 10000);
> ts[7].controls().set(controls::AnalogueGain, 4.0);
>
> - capture.runCaptureSession();
> + capture.runCaptureSession(&ts);
>
> ASSERT_TRUE(ts[5].metadata().contains(controls::ExposureTime.id()))
> << "Required metadata entry is missing";
>
>
> >
> > If you want to share ownership of the timesheet between the
> > PerFrameControlsCapture class and the caller, so that it gets
> > destroyed when the the last one of the two owners gets destroyed,
> > just use a shared_ptr<>
> >
> > --- a/src/apps/lc-compliance/per_frame_controls_test.cpp
> > +++ b/src/apps/lc-compliance/per_frame_controls_test.cpp
> > @@ -60,7 +60,7 @@ public:
> > unsigned int captureCount_;
> > unsigned int captureLimit_;
> >
> > - std::weak_ptr<TimeSheet> timeSheet_;
> > + std::shared_ptr<TimeSheet> timeSheet_;
> > };
> >
> > static const bool doImageTests = true;
> > @@ -91,10 +91,9 @@ PerFrameControlsCapture::startCaptureWithTimeSheet(unsigned int framesToCapture,
> > captureCount_ = 0;
> > captureLimit_ = framesToCapture;
> >
> > - auto timeSheet = std::make_shared<TimeSheet>(captureLimit_,
> > + timeSheet_ = std::make_shared<TimeSheet>(captureLimit_,
> > camera_->controls().idmap());
> > - timeSheet_ = timeSheet;
> > - return timeSheet;
> > + return timeSheet_;
> > }
> >
> > int PerFrameControlsCapture::queueRequest(Request *request)
> > @@ -103,18 +102,16 @@ int PerFrameControlsCapture::queueRequest(Request *request)
> > if (queueCount_ > captureLimit_)
> > return 0;
> >
> > - auto ts = timeSheet_.lock();
> > - if (ts)
> > - ts->prepareForQueue(request, queueCount_ - 1);
> > + if (timeSheet_)
> > + timeSheet_->prepareForQueue(request, queueCount_ - 1);
> >
> > return camera_->queueRequest(request);
> > }
> >
> > void PerFrameControlsCapture::requestComplete(Request *request)
> > {
> > - auto ts = timeSheet_.lock();
> > - if (ts)
> > - ts->handleCompleteRequest(request);
> > + if (timeSheet_)
> > + timeSheet_->handleCompleteRequest(request);
> >
> > captureCount_++;
> > if (captureCount_ >= captureLimit_) {
> >
> > So that now
> >
> > auto timeSheet = capture.startCaptureWithTimeSheet(10, &startValues);
> > std::cerr << " TIMESHEET USE COUNT: " << timeSheet.use_count();
> >
> > Reads as
> > TIMESHEET USE COUNT: 2
> >
> > > >
> > > > > + return timeSheet;
> > > > > +}
> > > > > +
> > > > > +int PerFrameControls::queueRequest(Request *request)
> > > > > +{
> > > > > + queueCount_++;
> > > > > + if (queueCount_ > captureLimit_)
> > > > > + return 0;
> > > > > +
> > > > > + auto ts = timeSheet_.lock();
> > > > > + if (ts)
> > > > > + ts->prepareForQueue(request, queueCount_ - 1);
> > > > > +
> > > > > + return camera_->queueRequest(request);
> > > > > +}
> > > > > +
> > > > > +void PerFrameControls::requestComplete(Request *request)
> > > > > +{
> > > > > + auto ts = timeSheet_.lock();
> > > > > + if (ts)
> > > > > + ts->handleCompleteRequest(request);
> > > > > +
> > > > > + captureCount_++;
> > > > > + if (captureCount_ >= captureLimit_) {
> > > > > + loop_->exit(0);
> > > > > + return;
> > > > > + }
> > > > > +
> > > > > + request->reuse(Request::ReuseBuffers);
> > > > > + if (queueRequest(request))
> > > > > + loop_->exit(-EINVAL);
> > > > > +}
> > > > > +
> > > > > +void PerFrameControls::runCaptureSession()
> > > > > +{
> > > > > + Stream *stream = config_->at(0).stream();
> > > > > + const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_->buffers(stream);
> > > > > +
> > > > > + /* Queue the recommended number of reqeuests. */
> > > >
> > > > s/reqeuests/requests/
> > > >
> > > > > + for (const std::unique_ptr<FrameBuffer> &buffer : buffers) {
> > > > > + std::unique_ptr<Request> request = camera_->createRequest();
> > > > > + request->addBuffer(stream, buffer.get());
> > > > > + queueRequest(request.get());
> > > > > + requests_.push_back(std::move(request));
> > > > > + }
> > > > > +
> > > > > + /* Run capture session. */
> > > > > + loop_ = new EventLoop();
> > > > > + loop_->exec();
> > > > > + stop();
> > > > > + delete loop_;
> > > > > +}
> > > > > +
> > > > > +void PerFrameControls::testExposureGainChangeOnSameFrame()
> > > > > +{
> > > > > + ControlList startValues;
> > > > > + startValues.set(controls::ExposureTime, 5000);
> > > > > + startValues.set(controls::AnalogueGain, 1.0);
> > > > > +
> > > > > + auto timeSheet = startCaptureWithTimeSheet(10, &startValues);
> > > > > + auto &ts = *timeSheet;
> > > >
> > > > Why a temporary reference ?
> > >
> > > The shared_ptr keeps the timesheet alive. The reference is just
> >
> > It's a reference, so it doesn't increment the usage count
> >
> > > syntactic shugar to be able to write ts[x]. I could replace these with
> > > (*ts)[x] or ts->get(x) if you like that better.
> > >
> >
> > Ah ok, seeing a reference just to be able to "ts[]" brings more
> > questions on why the reference is there than clarity imho. Up to you.
>
> Thats now solved by my proposal above :-)
>
Even better, thanks for coming up with a nice solution :)
> >
> > > >
> > > > > +
> > > > > + /* wait a few frames to settle */
> > > > > + ts[7].controls().set(controls::ExposureTime, 10000);
> > > > > + ts[7].controls().set(controls::AnalogueGain, 4.0);
> > > > > +
> > > > > + runCaptureSession();
> > > > > +
> > > > > + /* Uncomment this to debug the test */
> > > > > + /* ts.printAllInfos(); */
> > > >
> > > > Please drop
> > > >
> > > > > +
> > > > > + ASSERT_TRUE(ts[5].metadata().contains(controls::ExposureTime.id())) << "Required metadata entry is missing";
> > > > > + ASSERT_TRUE(ts[5].metadata().contains(controls::AnalogueGain.id())) << "Required metadata entry is missing";
> > > >
> > > > Break long lines when possible
> > > >
> > > > > +
> > > > > + EXPECT_NEAR(ts[3].metadata().get(controls::ExposureTime).value(), 5000, 20);
> > > > > + EXPECT_NEAR(ts[3].metadata().get(controls::AnalogueGain).value(), 1.0, 0.05);
> > > > > +
> > > > > + //find the frame with the changes
> > > >
> > > > No C++ comments please
> > > >
> > > > > + int exposureChangeIndex = 0;
> > > > > + for (unsigned i = 3; i < ts.size(); i++) {
> > > > > + if (ts[i].metadata().get(controls::ExposureTime).value() > 7500) {
> > > > > + exposureChangeIndex = i;
> > > > > + break;
> > > > > + }
> > > > > + }
> > > > > +
> > > > > + int gainChangeIndex = 0;
> > > > > + for (unsigned i = 3; i < ts.size(); i++) {
> > > > > + if (ts[i].metadata().get(controls::AnalogueGain).value() > 2.0) {
> > > > > + gainChangeIndex = i;
> > > > > + break;
> > > > > + }
> > > > > + }
> > > > > +
> > > > > + EXPECT_NE(exposureChangeIndex, 0) << "Exposure change not found in metadata";
> > > > > + EXPECT_NE(gainChangeIndex, 0) << "Gain change not found in metadata";
> > > > > + EXPECT_EQ(exposureChangeIndex, gainChangeIndex)
> > > > > + << "Metadata contained gain and exposure changes on different frames";
> > > > > +
> > > > > + if (doImageTests) {
> > > >
> > > > Why do you think it should be optional ? If it has to be made optional
> > > > it should be done in a way that doesn't depend on a compile time
> > > > constant defined in the source code. Either make a series of separate
> > > > image tests or add an option to lc-compliance.
> > >
> > > The tests still have a value without the content based tests, so yes
> > > this should propably be a option to lc-compliance. We could default that
> > > to false to express the 'optional' aspect.
> > >
> > > >
> > > > > + int brightnessChangeIndex = 0;
> > > > > + for (unsigned i = 3; i < ts.size(); i++) {
> > > >
> > > > The usage of '3' seems to be there to ignore the first three frames,
> > > > right ? If so, what about defining a constant and add a comment ?
> > > >
> > > > > + if (ts[i].getBrightnessChange() > 1.3) {
> > > > > + EXPECT_EQ(brightnessChangeIndex, 0)
> > > > > + << "Detected multiple frames with brightness increase (Wrong control delays?)";
> > > > > +
> > > > > + if (!brightnessChangeIndex)
> > > > > + brightnessChangeIndex = i;
> > > > > + }
> > > > > + }
> > > > > +
> > > > > + EXPECT_EQ(exposureChangeIndex, brightnessChangeIndex)
> > > > > + << "Exposure change and mesaured brightness change were not on same frame. "
> > > > > + << "(Wrong control delay?, Start frame event too late?)";
> > > > > + EXPECT_EQ(exposureChangeIndex, gainChangeIndex)
> > > > > + << "Gain change and mesaured brightness change were not on same frame. "
> > > > > + << "(Wrong control delay?, Start frame event too late?)";
> > > > > + }
> > > > > +}
> > > > > +
> > > > > +void PerFrameControls::testFramePreciseExposureChange()
> > > > > +{
> > > > > + auto timeSheet = startCaptureWithTimeSheet(10);
> > > > > + auto &ts = *timeSheet;
> > > > > +
> > > > > + ts[3].controls().set(controls::ExposureTime, 5000);
> > > > > + /* wait a few frames to settle */
> > > > > + ts[6].controls().set(controls::ExposureTime, 20000);
> > > > > +
> > > > > + runCaptureSession();
> > > > > +
> > > > > + /* Uncomment this to debug the test */
> > > > > + /* ts.printAllInfos(); */
> > > >
> > > > ditto
> > > >
> > > > > +
> > > > > + ASSERT_TRUE(ts[5].metadata().contains(controls::ExposureTime.id())) << "Required metadata entry is missing";
> > > >
> > > > break long lines
> > > >
> > > > > +
> > > > > + EXPECT_NEAR(ts[5].metadata().get(controls::ExposureTime).value(), 5000, 20);
> > > > > + EXPECT_NEAR(ts[6].metadata().get(controls::ExposureTime).value(), 20000, 20);
> > > > > +
> > > > > + if (doImageTests) {
> > > > > + /* No increase just before setting exposure */
> > > > > + EXPECT_NEAR(ts[5].getBrightnessChange(), 1.0, 0.05)
> > > > > + << "Brightness changed too much before the expected time of change (control delay too high?).";
> > > > > + /*
> > > > > + * Todo: The change is brightness was a bit low
> > > >
> > > > Wrong alignment
> > > >
> > > > We don't Doxygen lc-compliance but try to use \todo for consistency
> > > > with the rest of the code base
> > > >
> > > > > + * (Exposure time increase by 4x resulted in a brightness increase of < 2).
> > > > > + * This should be investigated.
> > > >
> > > > Might be platform specific issue ?
> > > >
> > > > > + */
> > > > > + EXPECT_GT(ts[6].getBrightnessChange(), 1.3)
> > > > > + << "Brightness in frame " << 6 << " did not increase as expected (reference: "
> > > > > + << ts[3].getSpotBrightness() << " current: " << ts[6].getSpotBrightness() << " )" << std::endl;
> > > > > +
> > > > > + /* No increase just after setting exposure */
> > > > > + EXPECT_NEAR(ts[7].getBrightnessChange(), 1.0, 0.05)
> > > > > + << "Brightness changed too much after the expected time of change (control delay too low?).";
> > > > > +
> > > > > + /* No increase just after setting exposure */
> > > > > + EXPECT_NEAR(ts[8].getBrightnessChange(), 1.0, 0.05)
> > > > > + << "Brightness changed too much 2 frames after the expected time of change (control delay too low?).";
> > > > > + }
> > > > > +}
> > > > > +
> > > > > +void PerFrameControls::testFramePreciseGainChange()
> > > > > +{
> > > > > + auto timeSheet = startCaptureWithTimeSheet(10);
> > > > > + auto &ts = *timeSheet;
> > > > > +
> > > > > + ts[3].controls().set(controls::AnalogueGain, 1.0);
> > > > > + /* wait a few frames to settle */
> > > > > + ts[6].controls().set(controls::AnalogueGain, 4.0);
> > > > > +
> > > > > + runCaptureSession();
> > > > > +
> > > > > + /* Uncomment this, to debug the test */
> > > > > + /* ts.printAllInfos(); */
> > > > > +
> > > > > + ASSERT_TRUE(ts[5].metadata().contains(controls::AnalogueGain.id())) << "Required metadata entry is missing";
> > > >
> > > > Break this long line
> > > >
> > > > > +
> > > > > + EXPECT_NEAR(ts[5].metadata().get(controls::AnalogueGain).value(), 1.0, 0.1);
> > > > > + EXPECT_NEAR(ts[6].metadata().get(controls::AnalogueGain).value(), 4.0, 0.1);
> > > > > +
> > > > > + if (doImageTests) {
> > > > > + /* No increase just before setting gain */
> > > > > + EXPECT_NEAR(ts[5].getBrightnessChange(), 1.0, 0.05)
> > > > > + << "Brightness changed too much before the expected time of change (control delay too high?).";
> > > > > + /*
> > > > > + * Todo: I see a brightness change of roughly half the expected one.
> > > >
> > > > Wrong alignment here too, also \todo
> > > >
> > > > > + * This is not yet understood and needs investigation
> > > >
> > > > Defintely some platform specific thing to investigate then ?
> > > >
> > > > > + */
> > > > > + EXPECT_GT(ts[6].getBrightnessChange(), 1.7)
> > > > > + << "Brightness in frame " << 6 << " did not increase as expected (reference: "
> > > > > + << ts[5].getSpotBrightness() << " current: " << ts[6].getSpotBrightness() << " )" << std::endl;
> > > > > +
> > > > > + /* No increase just after setting gain */
> > > > > + EXPECT_NEAR(ts[7].getBrightnessChange(), 1.0, 0.05)
> > > > > + << "Brightness changed too much after the expected time of change (control delay too low?).";
> > > > > +
> > > > > + /* No increase just after setting gain */
> > > > > + EXPECT_NEAR(ts[8].getBrightnessChange(), 1.0, 0.05)
> > > > > + << "Brightness changed too much after the expected time of change (control delay too low?).";
> > > > > + }
> > > > > +}
> > > > > +
> > > > > +void PerFrameControls::testExposureGainFromFirstRequestGetsApplied()
> > > > > +{
> > > > > + auto timeSheet = startCaptureWithTimeSheet(5);
> > > > > + auto &ts = *timeSheet;
> > > > > +
> > > > > + ts[0].controls().set(controls::ExposureTime, 10000);
> > > > > + ts[0].controls().set(controls::AnalogueGain, 4.0);
> > > > > +
> > > > > + runCaptureSession();
> > > > > +
> > > > > + ASSERT_TRUE(ts[4].metadata().contains(controls::ExposureTime.id())) << "Required metadata entry is missing";
> > > > > + ASSERT_TRUE(ts[4].metadata().contains(controls::AnalogueGain.id())) << "Required metadata entry is missing";
> > > >
> > > > You can easily break these lines
> > > >
> > > > > +
> > > > > + /* We expect it to be applied after 3 frames, the latest*/
> > > >
> > > > What if a sensor takes a longer time to apply exposure and gain ?
> > >
> > > Then we should adjust the test :-)
> > >
> > > >
> > > > > + EXPECT_NEAR(ts[4].metadata().get(controls::ExposureTime).value(), 10000, 20);
> > > > > + EXPECT_NEAR(ts[4].metadata().get(controls::AnalogueGain).value(), 4.0, 0.1);
> > > > > +}
> > > > > +
> > > > > +void PerFrameControls::testExposureGainFromFirstAndSecondRequestGetsApplied()
> > > > > +{
> > > > > + auto timeSheet = startCaptureWithTimeSheet(5);
> > > > > + auto &ts = *timeSheet;
> > > > > +
> > > > > + ts[0].controls().set(controls::ExposureTime, 8000);
> > > > > + ts[0].controls().set(controls::AnalogueGain, 2.0);
> > > > > + ts[1].controls().set(controls::ExposureTime, 10000);
> > > > > + ts[1].controls().set(controls::AnalogueGain, 4.0);
> > > >
> > > > As a general question, how do we guarantee the values you use to set
> > > > exposure and gains are valid for all possible sensors ?
> > > >
> > > > Shouldn't you inspect inspect the ControlInfo limit from
> > > > Camera::controls() and clamp the values in the min/max range ?
> > >
> > > I thought about that too. I started with values that most if not all
> > > sensor we care atm should support. I'm weary if it is worth the effort
> > > and that it would make the tests way less readable. But you are right,
> > > that is an unsolved issue.
> >
> > Recording it with a \todo comment is fine for now
> >
> > >
> > > >
> > > > > +
> > > > > + runCaptureSession();
> > > > > +
> > > > > + ASSERT_TRUE(ts[4].metadata().contains(controls::ExposureTime.id())) << "Required metadata entry is missing";
> > > > > + ASSERT_TRUE(ts[4].metadata().contains(controls::AnalogueGain.id())) << "Required metadata entry is missing";
> > > >
> > > > Ditto
> > > >
> > > > > +
> > > > > + /* We expect it to be applied after 3 frames, the latest */
> > > > > + EXPECT_NEAR(ts[4].metadata().get(controls::ExposureTime).value(), 10000, 20);
> > > > > + EXPECT_NEAR(ts[4].metadata().get(controls::AnalogueGain).value(), 4.0, 0.1);
> > > > > +}
> > > > > +
> > > > > +void PerFrameControls::testExposureGainIsAppliedOnFirstFrame()
> > > > > +{
> > > > > + ControlList startValues;
> > > > > + startValues.set(controls::ExposureTime, 5000);
> > > > > + startValues.set(controls::AnalogueGain, 1.0);
> > > > > +
> > > > > + auto ts1 = startCaptureWithTimeSheet(3, &startValues);
> > > > > +
> > > > > + runCaptureSession();
> > > > > +
> > > > > + ASSERT_TRUE((*ts1)[0].metadata().contains(controls::ExposureTime.id())) << "Required metadata entry is missing";
> > > > > + ASSERT_TRUE((*ts1)[0].metadata().contains(controls::AnalogueGain.id())) << "Required metadata entry is missing";
> > > > > +
> > > > > + EXPECT_NEAR((*ts1)[0].metadata().get(controls::ExposureTime).value(), 5000, 20);
> > > > > + EXPECT_NEAR((*ts1)[0].metadata().get(controls::AnalogueGain).value(), 1.0, 0.02);
> > > > > +
> > > > > + /* Second capture with different values to ensure we don't hit default/old values */
> > > > > + startValues.set(controls::ExposureTime, 15000);
> > > > > + startValues.set(controls::AnalogueGain, 4.0);
> > > > > +
> > > > > + auto ts2 = startCaptureWithTimeSheet(3, &startValues);
> > > > > +
> > > > > + runCaptureSession();
> > > > > +
> > > > > + EXPECT_NEAR((*ts2)[0].metadata().get(controls::ExposureTime).value(), 15000, 20);
> > > > > + EXPECT_NEAR((*ts2)[0].metadata().get(controls::AnalogueGain).value(), 4.0, 0.02);
> > > > > +
> > > > > + if (doImageTests) {
> > > > > + /* With 3x exposure and 4x gain we could expect a brightness increase of 2x */
> > > > > + double brightnessChange = ts2->get(1).getSpotBrightness() / ts1->get(1).getSpotBrightness();
> > > > > + EXPECT_GT(brightnessChange, 2.0);
> > > > > + }
> > > > > +}
> > > > > diff --git a/src/apps/lc-compliance/per_frame_controls.h b/src/apps/lc-compliance/per_frame_controls.h
> > > > > new file mode 100644
> > > > > index 00000000..a341c61f
> > > > > --- /dev/null
> > > > > +++ b/src/apps/lc-compliance/per_frame_controls.h
> > > > > @@ -0,0 +1,43 @@
> > > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > > > +/*
> > > > > + * Copyright (C) 2024, Ideas on Board Oy
> > > > > + *
> > > > > + * per_frame_controls.h - Tests for per frame controls
> > > > > + */
> > > > > +
> > > > > +#pragma once
> > > > > +
> > > > > +#include <memory>
> > > > > +
> > > > > +#include <libcamera/libcamera.h>
> > > > > +
> > > > > +#include "../common/event_loop.h"
> > > > > +
> > > > > +#include "simple_capture.h"
> > > > > +#include "time_sheet.h"
> > > > > +
> > > > > +class PerFrameControls : public SimpleCapture
> > > > > +{
> > > > > +public:
> > > > > + PerFrameControls(std::shared_ptr<libcamera::Camera> camera);
> > > > > +
> > > > > + std::shared_ptr<TimeSheet>
> > > > > + startCaptureWithTimeSheet(unsigned int framesToCapture, const libcamera::ControlList *controls = nullptr);
> > > > > + void runCaptureSession();
> > > > > +
> > > > > + void testExposureGainChangeOnSameFrame();
> > > > > + void testFramePreciseExposureChange();
> > > > > + void testFramePreciseGainChange();
> > > > > + void testExposureGainIsAppliedOnFirstFrame();
> > > > > + void testExposureGainFromFirstRequestGetsApplied();
> > > > > + void testExposureGainFromFirstAndSecondRequestGetsApplied();
> > > > > +
> > > > > + int queueRequest(libcamera::Request *request);
> > > > > + void requestComplete(libcamera::Request *request) override;
> > > > > +
> > > > > + unsigned int queueCount_;
> > > > > + unsigned int captureCount_;
> > > > > + unsigned int captureLimit_;
> > > > > +
> > > > > + std::weak_ptr<TimeSheet> timeSheet_;
> > > > > +};
> > > > > --
> > > > > 2.40.1
> > > > >
> > >
> > > Thanks for all the input :-)
> > >
> > > Cheers,
> > > Stefan
More information about the libcamera-devel
mailing list