[PATCH v2 03/12] libcamera: lc-compliance: Add initial set of per-frame-control tests
Stefan Klug
stefan.klug at ideasonboard.com
Thu Mar 21 12:34:49 CET 2024
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.
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-
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 :-)
>
> > >
> > > > +
> > > > + /* 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