[{"id":28987,"web_url":"https://patchwork.libcamera.org/comment/28987/","msgid":"","date":"2024-03-15T14:42:39","subject":"Re: [PATCH v2 03/12] libcamera: lc-compliance: Add initial set of\n\tper-frame-control tests","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Stefan\n\nOn Wed, Mar 13, 2024 at 01:12:14PM +0100, Stefan Klug wrote:\n> These tests check if controls (only exposure time and analogue gain at\n> the moment) get applied on the frame they were requested for.\n>\n> This is tested by looking at the metadata and the mean brightness\n> of the image center.\n>\n> At the moment these tests fail. Fixes for the pipelines will be delivered\n> in later patches.\n\nNice!\n\n>\n> To run only the teste, one can run:\n\ns/teste/test\n\n> lc-compliance -c -f \"SingleStream.*\"\n>\n> Note that the current implementation is a bit picky on what the camera\n> actually sees. If it is too dark (or too bright), the tests will fail.\n> Looking at a white wall in a normally lit office usually works.\n\nMmm, is this ok for a compliance suite ? Is this the reason the image\ntests are 'optional' ?\n\n>\n> Signed-off-by: Stefan Klug \n> ---\n> src/apps/lc-compliance/capture_test.cpp | 46 +++\n> src/apps/lc-compliance/meson.build | 1 +\n> src/apps/lc-compliance/per_frame_controls.cpp | 316 ++++++++++++++++++\n> src/apps/lc-compliance/per_frame_controls.h | 43 +++\n> 4 files changed, 406 insertions(+)\n> create mode 100644 src/apps/lc-compliance/per_frame_controls.cpp\n> create mode 100644 src/apps/lc-compliance/per_frame_controls.h\n>\n> diff --git a/src/apps/lc-compliance/capture_test.cpp b/src/apps/lc-compliance/capture_test.cpp\n> index 1dcfcf92..b19e8936 100644\n> --- a/src/apps/lc-compliance/capture_test.cpp\n> +++ b/src/apps/lc-compliance/capture_test.cpp\n> @@ -11,6 +11,7 @@\n> #include \n>\n> #include \"environment.h\"\n> +#include \"per_frame_controls.h\"\n> #include \"simple_capture.h\"\n>\n> using namespace libcamera;\n> @@ -133,3 +134,48 @@ INSTANTIATE_TEST_SUITE_P(CaptureTests,\n> \t\t\t testing::Combine(testing::ValuesIn(ROLES),\n> \t\t\t\t\t testing::ValuesIn(NUMREQUESTS)),\n> \t\t\t SingleStream::nameParameters);\n> +\n> +/*\n> + * Test Per frame controls\n> + */\n> +TEST_F(SingleStream, testExposureGainChangeOnSameFrame)\n> +{\n> +\tPerFrameControls capture(camera_);\n> +\tcapture.configure(StreamRole::VideoRecording);\n> +\tcapture.testExposureGainChangeOnSameFrame();\n> +}\n> +\n> +TEST_F(SingleStream, testFramePreciseExposureChange)\n> +{\n> +\tPerFrameControls capture(camera_);\n> +\tcapture.configure(StreamRole::VideoRecording);\n> +\tcapture.testFramePreciseExposureChange();\n> +}\n> +\n> +TEST_F(SingleStream, testFramePreciseGainChange)\n> +{\n> +\tPerFrameControls capture(camera_);\n> +\tcapture.configure(StreamRole::VideoRecording);\n> +\tcapture.testFramePreciseGainChange();\n> +}\n> +\n> +TEST_F(SingleStream, testExposureGainIsAppliedOnFirstFrame)\n> +{\n> +\tPerFrameControls capture(camera_);\n> +\tcapture.configure(StreamRole::VideoRecording);\n> +\tcapture.testExposureGainIsAppliedOnFirstFrame();\n> +}\n> +\n> +TEST_F(SingleStream, testExposureGainFromFirstRequestGetsApplied)\n> +{\n> +\tPerFrameControls capture(camera_);\n> +\tcapture.configure(StreamRole::VideoRecording);\n> +\tcapture.testExposureGainFromFirstRequestGetsApplied();\n> +}\n> +\n> +TEST_F(SingleStream, testExposureGainFromFirstAndSecondRequestGetsApplied)\n> +{\n> +\tPerFrameControls capture(camera_);\n> +\tcapture.configure(StreamRole::VideoRecording);\n> +\tcapture.testExposureGainFromFirstAndSecondRequestGetsApplied();\n> +}\n\nThis now shows up as\n\nSingleStream.\n testExposureGainChangeOnSameFrame\n testFramePreciseExposureChange\n testFramePreciseGainChange\n testExposureGainIsAppliedOnFirstFrame\n testExposureGainFromFirstRequestGetsApplied\n testExposureGainFromFirstAndSecondRequestGetsApplied\n\nAnd we already have\n\nCaptureTests/SingleStream.\n Capture/Raw_1 # GetParam() = (Raw, 1)\n Capture/Raw_2 # GetParam() = (Raw, 2)\n Capture/Raw_3 # GetParam() = (Raw, 3)\n Capture/Raw_5 # GetParam() = (Raw, 5)\n Capture/Raw_8 # GetParam() = (Raw, 8)\n Capture/Raw_13 # GetParam() = (Raw, 13)\n Capture/Raw_21 # GetParam() = (Raw, 21)\n Capture/Raw_34 # GetParam() = (Raw, 34)\n Capture/Raw_55 # GetParam() = (Raw, 55)\n Capture/Raw_89 # GetParam() = (Raw, 89)\n\nI would have not instantiated these tests in capture_test.cpp but\ndirectly in per_frame_control.cpp and I would have named them\n\"PerFrameControl\". To do so you need to define a test class that\nderives from testing::Test in per_frame_control.cpp\n\n\n+/*\n+ * Test Per frame controls\n+ */\n+\n+class PerFrameControlTest : public testing::Test\n+{\n+protected:\n+ void SetUp() override;\n+ void TearDown() override;\n+\n+ std::shared_ptr camera_;\n+};\n+\n+void PerFrameControlTest::SetUp()\n+{\n+ Environment *env = Environment::get();\n+\n+ camera_ = env->cm()->get(env->cameraId());\n+\n+ ASSERT_EQ(camera_->acquire(), 0);\n+}\n+\n+void PerFrameControlTest::TearDown()\n+{\n+ if (!camera_)\n+ return;\n+\n+ camera_->release();\n+ camera_.reset();\n+}\n+\n+TEST_F(PerFrameControlTest, testExposureGainChangeOnSameFrame)\n.....\n\nWith this you get a dedicated test suite\n\nPerFrameControlTest.\n testExposureGainChangeOnSameFrame\n testFramePreciseExposureChange\n testFramePreciseGainChange\n testExposureGainIsAppliedOnFirstFrame\n testExposureGainFromFirstRequestGetsApplied\n testExposureGainFromFirstAndSecondRequestGetsApplied\n\nAlso, you now can drop the\n\n\tvoid testExposureGainChangeOnSameFrame();\n\tvoid testFramePreciseExposureChange();\n\tvoid testFramePreciseGainChange();\n\tvoid testExposureGainIsAppliedOnFirstFrame();\n\tvoid testExposureGainFromFirstRequestGetsApplied();\n\tvoid testExposureGainFromFirstAndSecondRequestGetsApplied();\n\nfunctions from the PerFrameControl class, and implement the tests in\nthe test definition instead of having them as wrappers that call the\nPerFrameControl class' functions\n\nTL;DR do this:\n\nTEST_F(PerFrameControlTest, testExposureGainChangeOnSameFrame)\n{\n\tPerFrameControls capture(camera_);\n\tcapture.configure(StreamRole::VideoRecording);\n\n\tControlList startValues;\n\tstartValues.set(controls::ExposureTime, 5000);\n\tstartValues.set(controls::AnalogueGain, 1.0);\n\n\tauto timeSheet = capture.startCaptureWithTimeSheet(10, &startValues);\n\tauto &ts = *timeSheet;\n\n\t/* wait a few frames to settle */\n\tts[7].controls().set(controls::ExposureTime, 10000);\n\tts[7].controls().set(controls::AnalogueGain, 4.0);\n\n ...\n}\n\n\nin place of:\n\nTEST_F(PerFrameControlTest, testFramePreciseExposureChange)\n{\n\tPerFrameControls capture(camera_);\n\tcapture.configure(StreamRole::VideoRecording);\n\tcapture.testFramePreciseExposureChange();\n}\n\n\n\n> diff --git a/src/apps/lc-compliance/meson.build b/src/apps/lc-compliance/meson.build\n> index eb7b2d71..2a6f52af 100644\n> --- a/src/apps/lc-compliance/meson.build\n> +++ b/src/apps/lc-compliance/meson.build\n> @@ -15,6 +15,7 @@ lc_compliance_sources = files([\n> 'capture_test.cpp',\n> 'environment.cpp',\n> 'main.cpp',\n> + 'per_frame_controls.cpp',\n> 'simple_capture.cpp',\n> 'time_sheet.cpp',\n> ])\n> diff --git a/src/apps/lc-compliance/per_frame_controls.cpp b/src/apps/lc-compliance/per_frame_controls.cpp\n> new file mode 100644\n> index 00000000..eb7164e0\n> --- /dev/null\n> +++ b/src/apps/lc-compliance/per_frame_controls.cpp\n> @@ -0,0 +1,316 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2024, Ideas on Board Oy\n> + *\n> + * per_frame_controls.cpp - Tests for per frame controls\n> + */\n> +#include \"per_frame_controls.h\"\n> +\n> +#include \n> +\n> +#include \"time_sheet.h\"\n> +\n> +using namespace libcamera;\n> +\n> +static const bool doImageTests = true;\n> +\n> +PerFrameControls::PerFrameControls(std::shared_ptr camera)\n> +\t: SimpleCapture(camera)\n> +{\n> +}\n> +\n> +std::shared_ptr\n> +PerFrameControls::startCaptureWithTimeSheet(unsigned int framesToCapture, const ControlList *controls)\n> +{\n> +\tControlList ctrls(camera_->controls().idmap());\n\nEmpty line please\n\n> +\t/* Ensure defined default values */\n> +\tctrls.set(controls::AeEnable, false);\n> +\tctrls.set(controls::AeExposureMode, controls::ExposureCustom);\n> +\tctrls.set(controls::ExposureTime, 10000);\n> +\tctrls.set(controls::AnalogueGain, 1.0);\n> +\n> +\tif (controls)\n> +\t\tctrls.merge(*controls, ControlList::MergePolicy::OverwriteExisting);\n> +\n> +\tstart(&ctrls);\n> +\n> +\tqueueCount_ = 0;\n> +\tcaptureCount_ = 0;\n> +\tcaptureLimit_ = framesToCapture;\n> +\n> +\tauto timeSheet = std::make_shared(captureLimit_, camera_->controls().idmap());\n> +\ttimeSheet_ = timeSheet;\n\nI'm sorry but I don't see why you would use a shared_ptr<> and a weak\nreference when the timeSheet_ ownership is not shared with any other\ncomponent outside of this class\n\n> +\treturn timeSheet;\n> +}\n> +\n> +int PerFrameControls::queueRequest(Request *request)\n> +{\n> +\tqueueCount_++;\n> +\tif (queueCount_ > captureLimit_)\n> +\t\treturn 0;\n> +\n> +\tauto ts = timeSheet_.lock();\n> +\tif (ts)\n> +\t\tts->prepareForQueue(request, queueCount_ - 1);\n> +\n> +\treturn camera_->queueRequest(request);\n> +}\n> +\n> +void PerFrameControls::requestComplete(Request *request)\n> +{\n> +\tauto ts = timeSheet_.lock();\n> +\tif (ts)\n> +\t\tts->handleCompleteRequest(request);\n> +\n> +\tcaptureCount_++;\n> +\tif (captureCount_ >= captureLimit_) {\n> +\t\tloop_->exit(0);\n> +\t\treturn;\n> +\t}\n> +\n> +\trequest->reuse(Request::ReuseBuffers);\n> +\tif (queueRequest(request))\n> +\t\tloop_->exit(-EINVAL);\n> +}\n> +\n> +void PerFrameControls::runCaptureSession()\n> +{\n> +\tStream *stream = config_->at(0).stream();\n> +\tconst std::vector> &buffers = allocator_->buffers(stream);\n> +\n> +\t/* Queue the recommended number of reqeuests. */\n\ns/reqeuests/requests/\n\n> +\tfor (const std::unique_ptr &buffer : buffers) {\n> +\t\tstd::unique_ptr request = camera_->createRequest();\n> +\t\trequest->addBuffer(stream, buffer.get());\n> +\t\tqueueRequest(request.get());\n> +\t\trequests_.push_back(std::move(request));\n> +\t}\n> +\n> +\t/* Run capture session. */\n> +\tloop_ = new EventLoop();\n> +\tloop_->exec();\n> +\tstop();\n> +\tdelete loop_;\n> +}\n> +\n> +void PerFrameControls::testExposureGainChangeOnSameFrame()\n> +{\n> +\tControlList startValues;\n> +\tstartValues.set(controls::ExposureTime, 5000);\n> +\tstartValues.set(controls::AnalogueGain, 1.0);\n> +\n> +\tauto timeSheet = startCaptureWithTimeSheet(10, &startValues);\n> +\tauto &ts = *timeSheet;\n\nWhy a temporary reference ?\n\n> +\n> +\t/* wait a few frames to settle */\n> +\tts[7].controls().set(controls::ExposureTime, 10000);\n> +\tts[7].controls().set(controls::AnalogueGain, 4.0);\n> +\n> +\trunCaptureSession();\n> +\n> +\t/* Uncomment this to debug the test */\n> +\t/* ts.printAllInfos(); */\n\nPlease drop\n\n> +\n> +\tASSERT_TRUE(ts[5].metadata().contains(controls::ExposureTime.id())) << \"Required metadata entry is missing\";\n> +\tASSERT_TRUE(ts[5].metadata().contains(controls::AnalogueGain.id())) << \"Required metadata entry is missing\";\n\nBreak long lines when possible\n\n> +\n> +\tEXPECT_NEAR(ts[3].metadata().get(controls::ExposureTime).value(), 5000, 20);\n> +\tEXPECT_NEAR(ts[3].metadata().get(controls::AnalogueGain).value(), 1.0, 0.05);\n> +\n> +\t//find the frame with the changes\n\nNo C++ comments please\n\n> +\tint exposureChangeIndex = 0;\n> +\tfor (unsigned i = 3; i < ts.size(); i++) {\n> +\t\tif (ts[i].metadata().get(controls::ExposureTime).value() > 7500) {\n> +\t\t\texposureChangeIndex = i;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\n> +\tint gainChangeIndex = 0;\n> +\tfor (unsigned i = 3; i < ts.size(); i++) {\n> +\t\tif (ts[i].metadata().get(controls::AnalogueGain).value() > 2.0) {\n> +\t\t\tgainChangeIndex = i;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\n> +\tEXPECT_NE(exposureChangeIndex, 0) << \"Exposure change not found in metadata\";\n> +\tEXPECT_NE(gainChangeIndex, 0) << \"Gain change not found in metadata\";\n> +\tEXPECT_EQ(exposureChangeIndex, gainChangeIndex)\n> +\t\t<< \"Metadata contained gain and exposure changes on different frames\";\n> +\n> +\tif (doImageTests) {\n\nWhy do you think it should be optional ? If it has to be made optional\nit should be done in a way that doesn't depend on a compile time\nconstant defined in the source code. Either make a series of separate\nimage tests or add an option to lc-compliance.\n\n> +\t\tint brightnessChangeIndex = 0;\n> +\t\tfor (unsigned i = 3; i < ts.size(); i++) {\n\nThe usage of '3' seems to be there to ignore the first three frames,\nright ? If so, what about defining a constant and add a comment ?\n\n> +\t\t\tif (ts[i].getBrightnessChange() > 1.3) {\n> +\t\t\t\tEXPECT_EQ(brightnessChangeIndex, 0)\n> +\t\t\t\t\t<< \"Detected multiple frames with brightness increase (Wrong control delays?)\";\n> +\n> +\t\t\t\tif (!brightnessChangeIndex)\n> +\t\t\t\t\tbrightnessChangeIndex = i;\n> +\t\t\t}\n> +\t\t}\n> +\n> +\t\tEXPECT_EQ(exposureChangeIndex, brightnessChangeIndex)\n> +\t\t\t<< \"Exposure change and mesaured brightness change were not on same frame. \"\n> +\t\t\t<< \"(Wrong control delay?, Start frame event too late?)\";\n> +\t\tEXPECT_EQ(exposureChangeIndex, gainChangeIndex)\n> +\t\t\t<< \"Gain change and mesaured brightness change were not on same frame. \"\n> +\t\t\t<< \"(Wrong control delay?, Start frame event too late?)\";\n> +\t}\n> +}\n> +\n> +void PerFrameControls::testFramePreciseExposureChange()\n> +{\n> +\tauto timeSheet = startCaptureWithTimeSheet(10);\n> +\tauto &ts = *timeSheet;\n> +\n> +\tts[3].controls().set(controls::ExposureTime, 5000);\n> +\t/* wait a few frames to settle */\n> +\tts[6].controls().set(controls::ExposureTime, 20000);\n> +\n> +\trunCaptureSession();\n> +\n> +\t/* Uncomment this to debug the test */\n> +\t/* ts.printAllInfos(); */\n\nditto\n\n> +\n> +\tASSERT_TRUE(ts[5].metadata().contains(controls::ExposureTime.id())) << \"Required metadata entry is missing\";\n\nbreak long lines\n\n> +\n> +\tEXPECT_NEAR(ts[5].metadata().get(controls::ExposureTime).value(), 5000, 20);\n> +\tEXPECT_NEAR(ts[6].metadata().get(controls::ExposureTime).value(), 20000, 20);\n> +\n> +\tif (doImageTests) {\n> +\t\t/* No increase just before setting exposure */\n> +\t\tEXPECT_NEAR(ts[5].getBrightnessChange(), 1.0, 0.05)\n> +\t\t\t<< \"Brightness changed too much before the expected time of change (control delay too high?).\";\n> +\t\t/*\n> +\t\t* Todo: The change is brightness was a bit low\n\nWrong alignment\n\nWe don't Doxygen lc-compliance but try to use \\todo for consistency\nwith the rest of the code base\n\n> +\t\t* (Exposure time increase by 4x resulted in a brightness increase of < 2).\n> +\t\t* This should be investigated.\n\nMight be platform specific issue ?\n\n> +\t\t*/\n> +\t\tEXPECT_GT(ts[6].getBrightnessChange(), 1.3)\n> +\t\t\t<< \"Brightness in frame \" << 6 << \" did not increase as expected (reference: \"\n> +\t\t\t<< ts[3].getSpotBrightness() << \" current: \" << ts[6].getSpotBrightness() << \" )\" << std::endl;\n> +\n> +\t\t/* No increase just after setting exposure */\n> +\t\tEXPECT_NEAR(ts[7].getBrightnessChange(), 1.0, 0.05)\n> +\t\t\t<< \"Brightness changed too much after the expected time of change (control delay too low?).\";\n> +\n> +\t\t/* No increase just after setting exposure */\n> +\t\tEXPECT_NEAR(ts[8].getBrightnessChange(), 1.0, 0.05)\n> +\t\t\t<< \"Brightness changed too much 2 frames after the expected time of change (control delay too low?).\";\n> +\t}\n> +}\n> +\n> +void PerFrameControls::testFramePreciseGainChange()\n> +{\n> +\tauto timeSheet = startCaptureWithTimeSheet(10);\n> +\tauto &ts = *timeSheet;\n> +\n> +\tts[3].controls().set(controls::AnalogueGain, 1.0);\n> +\t/* wait a few frames to settle */\n> +\tts[6].controls().set(controls::AnalogueGain, 4.0);\n> +\n> +\trunCaptureSession();\n> +\n> +\t/* Uncomment this, to debug the test */\n> +\t/* ts.printAllInfos(); */\n> +\n> +\tASSERT_TRUE(ts[5].metadata().contains(controls::AnalogueGain.id())) << \"Required metadata entry is missing\";\n\nBreak this long line\n\n> +\n> +\tEXPECT_NEAR(ts[5].metadata().get(controls::AnalogueGain).value(), 1.0, 0.1);\n> +\tEXPECT_NEAR(ts[6].metadata().get(controls::AnalogueGain).value(), 4.0, 0.1);\n> +\n> +\tif (doImageTests) {\n> +\t\t/* No increase just before setting gain */\n> +\t\tEXPECT_NEAR(ts[5].getBrightnessChange(), 1.0, 0.05)\n> +\t\t\t<< \"Brightness changed too much before the expected time of change (control delay too high?).\";\n> +\t\t/*\n> +\t\t* Todo: I see a brightness change of roughly half the expected one.\n\nWrong alignment here too, also \\todo\n\n> +\t\t* This is not yet understood and needs investigation\n\nDefintely some platform specific thing to investigate then ?\n\n> +\t\t*/\n> +\t\tEXPECT_GT(ts[6].getBrightnessChange(), 1.7)\n> +\t\t\t<< \"Brightness in frame \" << 6 << \" did not increase as expected (reference: \"\n> +\t\t\t<< ts[5].getSpotBrightness() << \" current: \" << ts[6].getSpotBrightness() << \" )\" << std::endl;\n> +\n> +\t\t/* No increase just after setting gain */\n> +\t\tEXPECT_NEAR(ts[7].getBrightnessChange(), 1.0, 0.05)\n> +\t\t\t<< \"Brightness changed too much after the expected time of change (control delay too low?).\";\n> +\n> +\t\t/* No increase just after setting gain */\n> +\t\tEXPECT_NEAR(ts[8].getBrightnessChange(), 1.0, 0.05)\n> +\t\t\t<< \"Brightness changed too much after the expected time of change (control delay too low?).\";\n> +\t}\n> +}\n> +\n> +void PerFrameControls::testExposureGainFromFirstRequestGetsApplied()\n> +{\n> +\tauto timeSheet = startCaptureWithTimeSheet(5);\n> +\tauto &ts = *timeSheet;\n> +\n> +\tts[0].controls().set(controls::ExposureTime, 10000);\n> +\tts[0].controls().set(controls::AnalogueGain, 4.0);\n> +\n> +\trunCaptureSession();\n> +\n> +\tASSERT_TRUE(ts[4].metadata().contains(controls::ExposureTime.id())) << \"Required metadata entry is missing\";\n> +\tASSERT_TRUE(ts[4].metadata().contains(controls::AnalogueGain.id())) << \"Required metadata entry is missing\";\n\nYou can easily break these lines\n\n> +\n> +\t/* We expect it to be applied after 3 frames, the latest*/\n\nWhat if a sensor takes a longer time to apply exposure and gain ?\n\n> +\tEXPECT_NEAR(ts[4].metadata().get(controls::ExposureTime).value(), 10000, 20);\n> +\tEXPECT_NEAR(ts[4].metadata().get(controls::AnalogueGain).value(), 4.0, 0.1);\n> +}\n> +\n> +void PerFrameControls::testExposureGainFromFirstAndSecondRequestGetsApplied()\n> +{\n> +\tauto timeSheet = startCaptureWithTimeSheet(5);\n> +\tauto &ts = *timeSheet;\n> +\n> +\tts[0].controls().set(controls::ExposureTime, 8000);\n> +\tts[0].controls().set(controls::AnalogueGain, 2.0);\n> +\tts[1].controls().set(controls::ExposureTime, 10000);\n> +\tts[1].controls().set(controls::AnalogueGain, 4.0);\n\nAs a general question, how do we guarantee the values you use to set\nexposure and gains are valid for all possible sensors ?\n\nShouldn't you inspect inspect the ControlInfo limit from\nCamera::controls() and clamp the values in the min/max range ?\n\n> +\n> +\trunCaptureSession();\n> +\n> +\tASSERT_TRUE(ts[4].metadata().contains(controls::ExposureTime.id())) << \"Required metadata entry is missing\";\n> +\tASSERT_TRUE(ts[4].metadata().contains(controls::AnalogueGain.id())) << \"Required metadata entry is missing\";\n\nDitto\n\n> +\n> +\t/* We expect it to be applied after 3 frames, the latest */\n> +\tEXPECT_NEAR(ts[4].metadata().get(controls::ExposureTime).value(), 10000, 20);\n> +\tEXPECT_NEAR(ts[4].metadata().get(controls::AnalogueGain).value(), 4.0, 0.1);\n> +}\n> +\n> +void PerFrameControls::testExposureGainIsAppliedOnFirstFrame()\n> +{\n> +\tControlList startValues;\n> +\tstartValues.set(controls::ExposureTime, 5000);\n> +\tstartValues.set(controls::AnalogueGain, 1.0);\n> +\n> +\tauto ts1 = startCaptureWithTimeSheet(3, &startValues);\n> +\n> +\trunCaptureSession();\n> +\n> +\tASSERT_TRUE((*ts1)[0].metadata().contains(controls::ExposureTime.id())) << \"Required metadata entry is missing\";\n> +\tASSERT_TRUE((*ts1)[0].metadata().contains(controls::AnalogueGain.id())) << \"Required metadata entry is missing\";\n> +\n> +\tEXPECT_NEAR((*ts1)[0].metadata().get(controls::ExposureTime).value(), 5000, 20);\n> +\tEXPECT_NEAR((*ts1)[0].metadata().get(controls::AnalogueGain).value(), 1.0, 0.02);\n> +\n> +\t/* Second capture with different values to ensure we don't hit default/old values */\n> +\tstartValues.set(controls::ExposureTime, 15000);\n> +\tstartValues.set(controls::AnalogueGain, 4.0);\n> +\n> +\tauto ts2 = startCaptureWithTimeSheet(3, &startValues);\n> +\n> +\trunCaptureSession();\n> +\n> +\tEXPECT_NEAR((*ts2)[0].metadata().get(controls::ExposureTime).value(), 15000, 20);\n> +\tEXPECT_NEAR((*ts2)[0].metadata().get(controls::AnalogueGain).value(), 4.0, 0.02);\n> +\n> +\tif (doImageTests) {\n> +\t\t/* With 3x exposure and 4x gain we could expect a brightness increase of 2x */\n> +\t\tdouble brightnessChange = ts2->get(1).getSpotBrightness() / ts1->get(1).getSpotBrightness();\n> +\t\tEXPECT_GT(brightnessChange, 2.0);\n> +\t}\n> +}\n> diff --git a/src/apps/lc-compliance/per_frame_controls.h b/src/apps/lc-compliance/per_frame_controls.h\n> new file mode 100644\n> index 00000000..a341c61f\n> --- /dev/null\n> +++ b/src/apps/lc-compliance/per_frame_controls.h\n> @@ -0,0 +1,43 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2024, Ideas on Board Oy\n> + *\n> + * per_frame_controls.h - Tests for per frame controls\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +\n> +#include \n> +\n> +#include \"../common/event_loop.h\"\n> +\n> +#include \"simple_capture.h\"\n> +#include \"time_sheet.h\"\n> +\n> +class PerFrameControls : public SimpleCapture\n> +{\n> +public:\n> +\tPerFrameControls(std::shared_ptr camera);\n> +\n> +\tstd::shared_ptr\n> +\tstartCaptureWithTimeSheet(unsigned int framesToCapture, const libcamera::ControlList *controls = nullptr);\n> +\tvoid runCaptureSession();\n> +\n> +\tvoid testExposureGainChangeOnSameFrame();\n> +\tvoid testFramePreciseExposureChange();\n> +\tvoid testFramePreciseGainChange();\n> +\tvoid testExposureGainIsAppliedOnFirstFrame();\n> +\tvoid testExposureGainFromFirstRequestGetsApplied();\n> +\tvoid testExposureGainFromFirstAndSecondRequestGetsApplied();\n> +\n> +\tint queueRequest(libcamera::Request *request);\n> +\tvoid requestComplete(libcamera::Request *request) override;\n> +\n> +\tunsigned int queueCount_;\n> +\tunsigned int captureCount_;\n> +\tunsigned int captureLimit_;\n> +\n> +\tstd::weak_ptr timeSheet_;\n> +};\n> --\n> 2.40.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 9857DBD808\n\tfor ;\n\tFri, 15 Mar 2024 14:42:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D0E2A62C8C;\n\tFri, 15 Mar 2024 15:42:43 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C5CFD61C65\n\tfor ;\n\tFri, 15 Mar 2024 15:42:42 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2001:b07:5d2e:52c9:cc1e:e404:491f:e6ea])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9033AE4;\n\tFri, 15 Mar 2024 15:42:18 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Jg1qsBSz\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1710513738;\n\tbh=6adhMTg+vUCpFkTpopLcT0KmhEcHbFvnz+pTDx7bCYo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Jg1qsBSzL7tewtAftfoARMBleXF2cEdPoB/uAFLQ//SUCS4eEFJhguy/ueH2ogRAq\n\tSQkjdG173KR0ws0NEQ87Q9bsZfGS8BKJfUTsKos4KjR2iuX3Z+yIpOWnlK4uITsKn/\n\tRvzbQBwxXKyhllezEOs1keWC1225wJB42Y3Nph+U=","Date":"Fri, 15 Mar 2024 15:42:39 +0100","From":"Jacopo Mondi ","To":"Stefan Klug ","Subject":"Re: [PATCH v2 03/12] libcamera: lc-compliance: Add initial set of\n\tper-frame-control tests","Message-ID":"","References":"<20240313121223.138150-1-stefan.klug@ideasonboard.com>\n\t<20240313121223.138150-4-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240313121223.138150-4-stefan.klug@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28989,"web_url":"https://patchwork.libcamera.org/comment/28989/","msgid":"<20240315155303.m2yfff7w2fcfydtw@jasper>","date":"2024-03-15T15:53:03","subject":"Re: [PATCH v2 03/12] libcamera: lc-compliance: Add initial set of\n\tper-frame-control tests","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Jacopo,\n\nthanks for the review.\n\nOn Fri, Mar 15, 2024 at 03:42:39PM +0100, Jacopo Mondi wrote:\n> Hi Stefan\n> \n> On Wed, Mar 13, 2024 at 01:12:14PM +0100, Stefan Klug wrote:\n> > These tests check if controls (only exposure time and analogue gain at\n> > the moment) get applied on the frame they were requested for.\n> >\n> > This is tested by looking at the metadata and the mean brightness\n> > of the image center.\n> >\n> > At the moment these tests fail. Fixes for the pipelines will be delivered\n> > in later patches.\n> \n> Nice!\n> \n> >\n> > To run only the teste, one can run:\n> \n> s/teste/test\n> \n> > lc-compliance -c -f \"SingleStream.*\"\n> >\n> > Note that the current implementation is a bit picky on what the camera\n> > actually sees. If it is too dark (or too bright), the tests will fail.\n> > Looking at a white wall in a normally lit office usually works.\n> \n> Mmm, is this ok for a compliance suite ? Is this the reason the image\n> tests are 'optional' ?\n\nYes, in the beginning I wasn't sure how far we should go in first place.\nYou basically have two options here:\n- Dynamically finding a \"base\" exposure time that works (e.g. bright\n enough that you see a difference when the values increase and dim\n enough to not saturate any pixel). This might work but has a large\n dependency on the efficiency of the sensor and might still be flaky.\n- Building and distributing a physical testrig with defined brightness.\n This is cool, but a larger task.\n\nSo I started small added this 'optional' term so that we could collect\nsome practical experience on how stable these tests are on different\ndevices.\n\n> \n> >\n> > Signed-off-by: Stefan Klug \n> > ---\n> > src/apps/lc-compliance/capture_test.cpp | 46 +++\n> > src/apps/lc-compliance/meson.build | 1 +\n> > src/apps/lc-compliance/per_frame_controls.cpp | 316 ++++++++++++++++++\n> > src/apps/lc-compliance/per_frame_controls.h | 43 +++\n> > 4 files changed, 406 insertions(+)\n> > create mode 100644 src/apps/lc-compliance/per_frame_controls.cpp\n> > create mode 100644 src/apps/lc-compliance/per_frame_controls.h\n> >\n> > diff --git a/src/apps/lc-compliance/capture_test.cpp b/src/apps/lc-compliance/capture_test.cpp\n> > index 1dcfcf92..b19e8936 100644\n> > --- a/src/apps/lc-compliance/capture_test.cpp\n> > +++ b/src/apps/lc-compliance/capture_test.cpp\n> > @@ -11,6 +11,7 @@\n> > #include \n> >\n> > #include \"environment.h\"\n> > +#include \"per_frame_controls.h\"\n> > #include \"simple_capture.h\"\n> >\n> > using namespace libcamera;\n> > @@ -133,3 +134,48 @@ INSTANTIATE_TEST_SUITE_P(CaptureTests,\n> > \t\t\t testing::Combine(testing::ValuesIn(ROLES),\n> > \t\t\t\t\t testing::ValuesIn(NUMREQUESTS)),\n> > \t\t\t SingleStream::nameParameters);\n> > +\n> > +/*\n> > + * Test Per frame controls\n> > + */\n> > +TEST_F(SingleStream, testExposureGainChangeOnSameFrame)\n> > +{\n> > +\tPerFrameControls capture(camera_);\n> > +\tcapture.configure(StreamRole::VideoRecording);\n> > +\tcapture.testExposureGainChangeOnSameFrame();\n> > +}\n> > +\n> > +TEST_F(SingleStream, testFramePreciseExposureChange)\n> > +{\n> > +\tPerFrameControls capture(camera_);\n> > +\tcapture.configure(StreamRole::VideoRecording);\n> > +\tcapture.testFramePreciseExposureChange();\n> > +}\n> > +\n> > +TEST_F(SingleStream, testFramePreciseGainChange)\n> > +{\n> > +\tPerFrameControls capture(camera_);\n> > +\tcapture.configure(StreamRole::VideoRecording);\n> > +\tcapture.testFramePreciseGainChange();\n> > +}\n> > +\n> > +TEST_F(SingleStream, testExposureGainIsAppliedOnFirstFrame)\n> > +{\n> > +\tPerFrameControls capture(camera_);\n> > +\tcapture.configure(StreamRole::VideoRecording);\n> > +\tcapture.testExposureGainIsAppliedOnFirstFrame();\n> > +}\n> > +\n> > +TEST_F(SingleStream, testExposureGainFromFirstRequestGetsApplied)\n> > +{\n> > +\tPerFrameControls capture(camera_);\n> > +\tcapture.configure(StreamRole::VideoRecording);\n> > +\tcapture.testExposureGainFromFirstRequestGetsApplied();\n> > +}\n> > +\n> > +TEST_F(SingleStream, testExposureGainFromFirstAndSecondRequestGetsApplied)\n> > +{\n> > +\tPerFrameControls capture(camera_);\n> > +\tcapture.configure(StreamRole::VideoRecording);\n> > +\tcapture.testExposureGainFromFirstAndSecondRequestGetsApplied();\n> > +}\n> \n> This now shows up as\n> \n> SingleStream.\n> testExposureGainChangeOnSameFrame\n> testFramePreciseExposureChange\n> testFramePreciseGainChange\n> testExposureGainIsAppliedOnFirstFrame\n> testExposureGainFromFirstRequestGetsApplied\n> testExposureGainFromFirstAndSecondRequestGetsApplied\n> \n> And we already have\n> \n> CaptureTests/SingleStream.\n> Capture/Raw_1 # GetParam() = (Raw, 1)\n> Capture/Raw_2 # GetParam() = (Raw, 2)\n> Capture/Raw_3 # GetParam() = (Raw, 3)\n> Capture/Raw_5 # GetParam() = (Raw, 5)\n> Capture/Raw_8 # GetParam() = (Raw, 8)\n> Capture/Raw_13 # GetParam() = (Raw, 13)\n> Capture/Raw_21 # GetParam() = (Raw, 21)\n> Capture/Raw_34 # GetParam() = (Raw, 34)\n> Capture/Raw_55 # GetParam() = (Raw, 55)\n> Capture/Raw_89 # GetParam() = (Raw, 89)\n> \n> I would have not instantiated these tests in capture_test.cpp but\n> directly in per_frame_control.cpp and I would have named them\n> \"PerFrameControl\". To do so you need to define a test class that\n> derives from testing::Test in per_frame_control.cpp\n> \n> \n> +/*\n> + * Test Per frame controls\n> + */\n> +\n> +class PerFrameControlTest : public testing::Test\n> +{\n> +protected:\n> + void SetUp() override;\n> + void TearDown() override;\n> +\n> + std::shared_ptr camera_;\n> +};\n> +\n> +void PerFrameControlTest::SetUp()\n> +{\n> + Environment *env = Environment::get();\n> +\n> + camera_ = env->cm()->get(env->cameraId());\n> +\n> + ASSERT_EQ(camera_->acquire(), 0);\n> +}\n> +\n> +void PerFrameControlTest::TearDown()\n> +{\n> + if (!camera_)\n> + return;\n> +\n> + camera_->release();\n> + camera_.reset();\n> +}\n> +\n> +TEST_F(PerFrameControlTest, testExposureGainChangeOnSameFrame)\n> .....\n> \n> With this you get a dedicated test suite\n> \n> PerFrameControlTest.\n> testExposureGainChangeOnSameFrame\n> testFramePreciseExposureChange\n> testFramePreciseGainChange\n> testExposureGainIsAppliedOnFirstFrame\n> testExposureGainFromFirstRequestGetsApplied\n> testExposureGainFromFirstAndSecondRequestGetsApplied\n> \n> Also, you now can drop the\n> \n> \tvoid testExposureGainChangeOnSameFrame();\n> \tvoid testFramePreciseExposureChange();\n> \tvoid testFramePreciseGainChange();\n> \tvoid testExposureGainIsAppliedOnFirstFrame();\n> \tvoid testExposureGainFromFirstRequestGetsApplied();\n> \tvoid testExposureGainFromFirstAndSecondRequestGetsApplied();\n> \n> functions from the PerFrameControl class, and implement the tests in\n> the test definition instead of having them as wrappers that call the\n> PerFrameControl class' functions\n> \n> TL;DR do this:\n> \n> TEST_F(PerFrameControlTest, testExposureGainChangeOnSameFrame)\n> {\n> \tPerFrameControls capture(camera_);\n> \tcapture.configure(StreamRole::VideoRecording);\n> \n> \tControlList startValues;\n> \tstartValues.set(controls::ExposureTime, 5000);\n> \tstartValues.set(controls::AnalogueGain, 1.0);\n> \n> \tauto timeSheet = capture.startCaptureWithTimeSheet(10, &startValues);\n> \tauto &ts = *timeSheet;\n> \n> \t/* wait a few frames to settle */\n> \tts[7].controls().set(controls::ExposureTime, 10000);\n> \tts[7].controls().set(controls::AnalogueGain, 4.0);\n> \n> ...\n> }\n> \n> \n> in place of:\n> \n> TEST_F(PerFrameControlTest, testFramePreciseExposureChange)\n> {\n> \tPerFrameControls capture(camera_);\n> \tcapture.configure(StreamRole::VideoRecording);\n> \tcapture.testFramePreciseExposureChange();\n> }\n> \n\nGreat. Thanks for that. I didn't want to spend too much time inside\ngoogle test before getting feedback on the overall direction. Your\nproposal is perfect. I'll do that.\n\n> \n> \n> > diff --git a/src/apps/lc-compliance/meson.build b/src/apps/lc-compliance/meson.build\n> > index eb7b2d71..2a6f52af 100644\n> > --- a/src/apps/lc-compliance/meson.build\n> > +++ b/src/apps/lc-compliance/meson.build\n> > @@ -15,6 +15,7 @@ lc_compliance_sources = files([\n> > 'capture_test.cpp',\n> > 'environment.cpp',\n> > 'main.cpp',\n> > + 'per_frame_controls.cpp',\n> > 'simple_capture.cpp',\n> > 'time_sheet.cpp',\n> > ])\n> > diff --git a/src/apps/lc-compliance/per_frame_controls.cpp b/src/apps/lc-compliance/per_frame_controls.cpp\n> > new file mode 100644\n> > index 00000000..eb7164e0\n> > --- /dev/null\n> > +++ b/src/apps/lc-compliance/per_frame_controls.cpp\n> > @@ -0,0 +1,316 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2024, Ideas on Board Oy\n> > + *\n> > + * per_frame_controls.cpp - Tests for per frame controls\n> > + */\n> > +#include \"per_frame_controls.h\"\n> > +\n> > +#include \n> > +\n> > +#include \"time_sheet.h\"\n> > +\n> > +using namespace libcamera;\n> > +\n> > +static const bool doImageTests = true;\n> > +\n> > +PerFrameControls::PerFrameControls(std::shared_ptr camera)\n> > +\t: SimpleCapture(camera)\n> > +{\n> > +}\n> > +\n> > +std::shared_ptr\n> > +PerFrameControls::startCaptureWithTimeSheet(unsigned int framesToCapture, const ControlList *controls)\n> > +{\n> > +\tControlList ctrls(camera_->controls().idmap());\n> \n> Empty line please\n> \n> > +\t/* Ensure defined default values */\n> > +\tctrls.set(controls::AeEnable, false);\n> > +\tctrls.set(controls::AeExposureMode, controls::ExposureCustom);\n> > +\tctrls.set(controls::ExposureTime, 10000);\n> > +\tctrls.set(controls::AnalogueGain, 1.0);\n> > +\n> > +\tif (controls)\n> > +\t\tctrls.merge(*controls, ControlList::MergePolicy::OverwriteExisting);\n> > +\n> > +\tstart(&ctrls);\n> > +\n> > +\tqueueCount_ = 0;\n> > +\tcaptureCount_ = 0;\n> > +\tcaptureLimit_ = framesToCapture;\n> > +\n> > +\tauto timeSheet = std::make_shared(captureLimit_, camera_->controls().idmap());\n> > +\ttimeSheet_ = timeSheet;\n> \n> I'm sorry but I don't see why you would use a shared_ptr<> and a weak\n> reference when the timeSheet_ ownership is not shared with any other\n> component outside of this class\n\nThis class is still based on the Capture class. So you are free to write\ntests and capture without a timesheet. In such tests the weak ptr will\nautomatically be empty and the queueRequest and requestComplete\nfunctions still work properly. The lifetime of the timesheet is bound to\nthe scope of the test function. (You could even use the\nstartCaptureWithTimeSheet() and ignore the result, in wich case the\ntimesheet would get destroyed immediately).\n\n> \n> > +\treturn timeSheet;\n> > +}\n> > +\n> > +int PerFrameControls::queueRequest(Request *request)\n> > +{\n> > +\tqueueCount_++;\n> > +\tif (queueCount_ > captureLimit_)\n> > +\t\treturn 0;\n> > +\n> > +\tauto ts = timeSheet_.lock();\n> > +\tif (ts)\n> > +\t\tts->prepareForQueue(request, queueCount_ - 1);\n> > +\n> > +\treturn camera_->queueRequest(request);\n> > +}\n> > +\n> > +void PerFrameControls::requestComplete(Request *request)\n> > +{\n> > +\tauto ts = timeSheet_.lock();\n> > +\tif (ts)\n> > +\t\tts->handleCompleteRequest(request);\n> > +\n> > +\tcaptureCount_++;\n> > +\tif (captureCount_ >= captureLimit_) {\n> > +\t\tloop_->exit(0);\n> > +\t\treturn;\n> > +\t}\n> > +\n> > +\trequest->reuse(Request::ReuseBuffers);\n> > +\tif (queueRequest(request))\n> > +\t\tloop_->exit(-EINVAL);\n> > +}\n> > +\n> > +void PerFrameControls::runCaptureSession()\n> > +{\n> > +\tStream *stream = config_->at(0).stream();\n> > +\tconst std::vector> &buffers = allocator_->buffers(stream);\n> > +\n> > +\t/* Queue the recommended number of reqeuests. */\n> \n> s/reqeuests/requests/\n> \n> > +\tfor (const std::unique_ptr &buffer : buffers) {\n> > +\t\tstd::unique_ptr request = camera_->createRequest();\n> > +\t\trequest->addBuffer(stream, buffer.get());\n> > +\t\tqueueRequest(request.get());\n> > +\t\trequests_.push_back(std::move(request));\n> > +\t}\n> > +\n> > +\t/* Run capture session. */\n> > +\tloop_ = new EventLoop();\n> > +\tloop_->exec();\n> > +\tstop();\n> > +\tdelete loop_;\n> > +}\n> > +\n> > +void PerFrameControls::testExposureGainChangeOnSameFrame()\n> > +{\n> > +\tControlList startValues;\n> > +\tstartValues.set(controls::ExposureTime, 5000);\n> > +\tstartValues.set(controls::AnalogueGain, 1.0);\n> > +\n> > +\tauto timeSheet = startCaptureWithTimeSheet(10, &startValues);\n> > +\tauto &ts = *timeSheet;\n> \n> Why a temporary reference ?\n\nThe shared_ptr keeps the timesheet alive. The reference is just\nsyntactic shugar to be able to write ts[x]. I could replace these with\n(*ts)[x] or ts->get(x) if you like that better.\n\n> \n> > +\n> > +\t/* wait a few frames to settle */\n> > +\tts[7].controls().set(controls::ExposureTime, 10000);\n> > +\tts[7].controls().set(controls::AnalogueGain, 4.0);\n> > +\n> > +\trunCaptureSession();\n> > +\n> > +\t/* Uncomment this to debug the test */\n> > +\t/* ts.printAllInfos(); */\n> \n> Please drop\n> \n> > +\n> > +\tASSERT_TRUE(ts[5].metadata().contains(controls::ExposureTime.id())) << \"Required metadata entry is missing\";\n> > +\tASSERT_TRUE(ts[5].metadata().contains(controls::AnalogueGain.id())) << \"Required metadata entry is missing\";\n> \n> Break long lines when possible\n> \n> > +\n> > +\tEXPECT_NEAR(ts[3].metadata().get(controls::ExposureTime).value(), 5000, 20);\n> > +\tEXPECT_NEAR(ts[3].metadata().get(controls::AnalogueGain).value(), 1.0, 0.05);\n> > +\n> > +\t//find the frame with the changes\n> \n> No C++ comments please\n> \n> > +\tint exposureChangeIndex = 0;\n> > +\tfor (unsigned i = 3; i < ts.size(); i++) {\n> > +\t\tif (ts[i].metadata().get(controls::ExposureTime).value() > 7500) {\n> > +\t\t\texposureChangeIndex = i;\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\t}\n> > +\n> > +\tint gainChangeIndex = 0;\n> > +\tfor (unsigned i = 3; i < ts.size(); i++) {\n> > +\t\tif (ts[i].metadata().get(controls::AnalogueGain).value() > 2.0) {\n> > +\t\t\tgainChangeIndex = i;\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\t}\n> > +\n> > +\tEXPECT_NE(exposureChangeIndex, 0) << \"Exposure change not found in metadata\";\n> > +\tEXPECT_NE(gainChangeIndex, 0) << \"Gain change not found in metadata\";\n> > +\tEXPECT_EQ(exposureChangeIndex, gainChangeIndex)\n> > +\t\t<< \"Metadata contained gain and exposure changes on different frames\";\n> > +\n> > +\tif (doImageTests) {\n> \n> Why do you think it should be optional ? If it has to be made optional\n> it should be done in a way that doesn't depend on a compile time\n> constant defined in the source code. Either make a series of separate\n> image tests or add an option to lc-compliance.\n\nThe tests still have a value without the content based tests, so yes\nthis should propably be a option to lc-compliance. We could default that\nto false to express the 'optional' aspect.\n\n> \n> > +\t\tint brightnessChangeIndex = 0;\n> > +\t\tfor (unsigned i = 3; i < ts.size(); i++) {\n> \n> The usage of '3' seems to be there to ignore the first three frames,\n> right ? If so, what about defining a constant and add a comment ?\n> \n> > +\t\t\tif (ts[i].getBrightnessChange() > 1.3) {\n> > +\t\t\t\tEXPECT_EQ(brightnessChangeIndex, 0)\n> > +\t\t\t\t\t<< \"Detected multiple frames with brightness increase (Wrong control delays?)\";\n> > +\n> > +\t\t\t\tif (!brightnessChangeIndex)\n> > +\t\t\t\t\tbrightnessChangeIndex = i;\n> > +\t\t\t}\n> > +\t\t}\n> > +\n> > +\t\tEXPECT_EQ(exposureChangeIndex, brightnessChangeIndex)\n> > +\t\t\t<< \"Exposure change and mesaured brightness change were not on same frame. \"\n> > +\t\t\t<< \"(Wrong control delay?, Start frame event too late?)\";\n> > +\t\tEXPECT_EQ(exposureChangeIndex, gainChangeIndex)\n> > +\t\t\t<< \"Gain change and mesaured brightness change were not on same frame. \"\n> > +\t\t\t<< \"(Wrong control delay?, Start frame event too late?)\";\n> > +\t}\n> > +}\n> > +\n> > +void PerFrameControls::testFramePreciseExposureChange()\n> > +{\n> > +\tauto timeSheet = startCaptureWithTimeSheet(10);\n> > +\tauto &ts = *timeSheet;\n> > +\n> > +\tts[3].controls().set(controls::ExposureTime, 5000);\n> > +\t/* wait a few frames to settle */\n> > +\tts[6].controls().set(controls::ExposureTime, 20000);\n> > +\n> > +\trunCaptureSession();\n> > +\n> > +\t/* Uncomment this to debug the test */\n> > +\t/* ts.printAllInfos(); */\n> \n> ditto\n> \n> > +\n> > +\tASSERT_TRUE(ts[5].metadata().contains(controls::ExposureTime.id())) << \"Required metadata entry is missing\";\n> \n> break long lines\n> \n> > +\n> > +\tEXPECT_NEAR(ts[5].metadata().get(controls::ExposureTime).value(), 5000, 20);\n> > +\tEXPECT_NEAR(ts[6].metadata().get(controls::ExposureTime).value(), 20000, 20);\n> > +\n> > +\tif (doImageTests) {\n> > +\t\t/* No increase just before setting exposure */\n> > +\t\tEXPECT_NEAR(ts[5].getBrightnessChange(), 1.0, 0.05)\n> > +\t\t\t<< \"Brightness changed too much before the expected time of change (control delay too high?).\";\n> > +\t\t/*\n> > +\t\t* Todo: The change is brightness was a bit low\n> \n> Wrong alignment\n> \n> We don't Doxygen lc-compliance but try to use \\todo for consistency\n> with the rest of the code base\n> \n> > +\t\t* (Exposure time increase by 4x resulted in a brightness increase of < 2).\n> > +\t\t* This should be investigated.\n> \n> Might be platform specific issue ?\n> \n> > +\t\t*/\n> > +\t\tEXPECT_GT(ts[6].getBrightnessChange(), 1.3)\n> > +\t\t\t<< \"Brightness in frame \" << 6 << \" did not increase as expected (reference: \"\n> > +\t\t\t<< ts[3].getSpotBrightness() << \" current: \" << ts[6].getSpotBrightness() << \" )\" << std::endl;\n> > +\n> > +\t\t/* No increase just after setting exposure */\n> > +\t\tEXPECT_NEAR(ts[7].getBrightnessChange(), 1.0, 0.05)\n> > +\t\t\t<< \"Brightness changed too much after the expected time of change (control delay too low?).\";\n> > +\n> > +\t\t/* No increase just after setting exposure */\n> > +\t\tEXPECT_NEAR(ts[8].getBrightnessChange(), 1.0, 0.05)\n> > +\t\t\t<< \"Brightness changed too much 2 frames after the expected time of change (control delay too low?).\";\n> > +\t}\n> > +}\n> > +\n> > +void PerFrameControls::testFramePreciseGainChange()\n> > +{\n> > +\tauto timeSheet = startCaptureWithTimeSheet(10);\n> > +\tauto &ts = *timeSheet;\n> > +\n> > +\tts[3].controls().set(controls::AnalogueGain, 1.0);\n> > +\t/* wait a few frames to settle */\n> > +\tts[6].controls().set(controls::AnalogueGain, 4.0);\n> > +\n> > +\trunCaptureSession();\n> > +\n> > +\t/* Uncomment this, to debug the test */\n> > +\t/* ts.printAllInfos(); */\n> > +\n> > +\tASSERT_TRUE(ts[5].metadata().contains(controls::AnalogueGain.id())) << \"Required metadata entry is missing\";\n> \n> Break this long line\n> \n> > +\n> > +\tEXPECT_NEAR(ts[5].metadata().get(controls::AnalogueGain).value(), 1.0, 0.1);\n> > +\tEXPECT_NEAR(ts[6].metadata().get(controls::AnalogueGain).value(), 4.0, 0.1);\n> > +\n> > +\tif (doImageTests) {\n> > +\t\t/* No increase just before setting gain */\n> > +\t\tEXPECT_NEAR(ts[5].getBrightnessChange(), 1.0, 0.05)\n> > +\t\t\t<< \"Brightness changed too much before the expected time of change (control delay too high?).\";\n> > +\t\t/*\n> > +\t\t* Todo: I see a brightness change of roughly half the expected one.\n> \n> Wrong alignment here too, also \\todo\n> \n> > +\t\t* This is not yet understood and needs investigation\n> \n> Defintely some platform specific thing to investigate then ?\n> \n> > +\t\t*/\n> > +\t\tEXPECT_GT(ts[6].getBrightnessChange(), 1.7)\n> > +\t\t\t<< \"Brightness in frame \" << 6 << \" did not increase as expected (reference: \"\n> > +\t\t\t<< ts[5].getSpotBrightness() << \" current: \" << ts[6].getSpotBrightness() << \" )\" << std::endl;\n> > +\n> > +\t\t/* No increase just after setting gain */\n> > +\t\tEXPECT_NEAR(ts[7].getBrightnessChange(), 1.0, 0.05)\n> > +\t\t\t<< \"Brightness changed too much after the expected time of change (control delay too low?).\";\n> > +\n> > +\t\t/* No increase just after setting gain */\n> > +\t\tEXPECT_NEAR(ts[8].getBrightnessChange(), 1.0, 0.05)\n> > +\t\t\t<< \"Brightness changed too much after the expected time of change (control delay too low?).\";\n> > +\t}\n> > +}\n> > +\n> > +void PerFrameControls::testExposureGainFromFirstRequestGetsApplied()\n> > +{\n> > +\tauto timeSheet = startCaptureWithTimeSheet(5);\n> > +\tauto &ts = *timeSheet;\n> > +\n> > +\tts[0].controls().set(controls::ExposureTime, 10000);\n> > +\tts[0].controls().set(controls::AnalogueGain, 4.0);\n> > +\n> > +\trunCaptureSession();\n> > +\n> > +\tASSERT_TRUE(ts[4].metadata().contains(controls::ExposureTime.id())) << \"Required metadata entry is missing\";\n> > +\tASSERT_TRUE(ts[4].metadata().contains(controls::AnalogueGain.id())) << \"Required metadata entry is missing\";\n> \n> You can easily break these lines\n> \n> > +\n> > +\t/* We expect it to be applied after 3 frames, the latest*/\n> \n> What if a sensor takes a longer time to apply exposure and gain ?\n\nThen we should adjust the test :-)\n\n> \n> > +\tEXPECT_NEAR(ts[4].metadata().get(controls::ExposureTime).value(), 10000, 20);\n> > +\tEXPECT_NEAR(ts[4].metadata().get(controls::AnalogueGain).value(), 4.0, 0.1);\n> > +}\n> > +\n> > +void PerFrameControls::testExposureGainFromFirstAndSecondRequestGetsApplied()\n> > +{\n> > +\tauto timeSheet = startCaptureWithTimeSheet(5);\n> > +\tauto &ts = *timeSheet;\n> > +\n> > +\tts[0].controls().set(controls::ExposureTime, 8000);\n> > +\tts[0].controls().set(controls::AnalogueGain, 2.0);\n> > +\tts[1].controls().set(controls::ExposureTime, 10000);\n> > +\tts[1].controls().set(controls::AnalogueGain, 4.0);\n> \n> As a general question, how do we guarantee the values you use to set\n> exposure and gains are valid for all possible sensors ?\n> \n> Shouldn't you inspect inspect the ControlInfo limit from\n> Camera::controls() and clamp the values in the min/max range ?\n\nI thought about that too. I started with values that most if not all\nsensor we care atm should support. I'm weary if it is worth the effort\nand that it would make the tests way less readable. But you are right,\nthat is an unsolved issue.\n\n> \n> > +\n> > +\trunCaptureSession();\n> > +\n> > +\tASSERT_TRUE(ts[4].metadata().contains(controls::ExposureTime.id())) << \"Required metadata entry is missing\";\n> > +\tASSERT_TRUE(ts[4].metadata().contains(controls::AnalogueGain.id())) << \"Required metadata entry is missing\";\n> \n> Ditto\n> \n> > +\n> > +\t/* We expect it to be applied after 3 frames, the latest */\n> > +\tEXPECT_NEAR(ts[4].metadata().get(controls::ExposureTime).value(), 10000, 20);\n> > +\tEXPECT_NEAR(ts[4].metadata().get(controls::AnalogueGain).value(), 4.0, 0.1);\n> > +}\n> > +\n> > +void PerFrameControls::testExposureGainIsAppliedOnFirstFrame()\n> > +{\n> > +\tControlList startValues;\n> > +\tstartValues.set(controls::ExposureTime, 5000);\n> > +\tstartValues.set(controls::AnalogueGain, 1.0);\n> > +\n> > +\tauto ts1 = startCaptureWithTimeSheet(3, &startValues);\n> > +\n> > +\trunCaptureSession();\n> > +\n> > +\tASSERT_TRUE((*ts1)[0].metadata().contains(controls::ExposureTime.id())) << \"Required metadata entry is missing\";\n> > +\tASSERT_TRUE((*ts1)[0].metadata().contains(controls::AnalogueGain.id())) << \"Required metadata entry is missing\";\n> > +\n> > +\tEXPECT_NEAR((*ts1)[0].metadata().get(controls::ExposureTime).value(), 5000, 20);\n> > +\tEXPECT_NEAR((*ts1)[0].metadata().get(controls::AnalogueGain).value(), 1.0, 0.02);\n> > +\n> > +\t/* Second capture with different values to ensure we don't hit default/old values */\n> > +\tstartValues.set(controls::ExposureTime, 15000);\n> > +\tstartValues.set(controls::AnalogueGain, 4.0);\n> > +\n> > +\tauto ts2 = startCaptureWithTimeSheet(3, &startValues);\n> > +\n> > +\trunCaptureSession();\n> > +\n> > +\tEXPECT_NEAR((*ts2)[0].metadata().get(controls::ExposureTime).value(), 15000, 20);\n> > +\tEXPECT_NEAR((*ts2)[0].metadata().get(controls::AnalogueGain).value(), 4.0, 0.02);\n> > +\n> > +\tif (doImageTests) {\n> > +\t\t/* With 3x exposure and 4x gain we could expect a brightness increase of 2x */\n> > +\t\tdouble brightnessChange = ts2->get(1).getSpotBrightness() / ts1->get(1).getSpotBrightness();\n> > +\t\tEXPECT_GT(brightnessChange, 2.0);\n> > +\t}\n> > +}\n> > diff --git a/src/apps/lc-compliance/per_frame_controls.h b/src/apps/lc-compliance/per_frame_controls.h\n> > new file mode 100644\n> > index 00000000..a341c61f\n> > --- /dev/null\n> > +++ b/src/apps/lc-compliance/per_frame_controls.h\n> > @@ -0,0 +1,43 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2024, Ideas on Board Oy\n> > + *\n> > + * per_frame_controls.h - Tests for per frame controls\n> > + */\n> > +\n> > +#pragma once\n> > +\n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \"../common/event_loop.h\"\n> > +\n> > +#include \"simple_capture.h\"\n> > +#include \"time_sheet.h\"\n> > +\n> > +class PerFrameControls : public SimpleCapture\n> > +{\n> > +public:\n> > +\tPerFrameControls(std::shared_ptr camera);\n> > +\n> > +\tstd::shared_ptr\n> > +\tstartCaptureWithTimeSheet(unsigned int framesToCapture, const libcamera::ControlList *controls = nullptr);\n> > +\tvoid runCaptureSession();\n> > +\n> > +\tvoid testExposureGainChangeOnSameFrame();\n> > +\tvoid testFramePreciseExposureChange();\n> > +\tvoid testFramePreciseGainChange();\n> > +\tvoid testExposureGainIsAppliedOnFirstFrame();\n> > +\tvoid testExposureGainFromFirstRequestGetsApplied();\n> > +\tvoid testExposureGainFromFirstAndSecondRequestGetsApplied();\n> > +\n> > +\tint queueRequest(libcamera::Request *request);\n> > +\tvoid requestComplete(libcamera::Request *request) override;\n> > +\n> > +\tunsigned int queueCount_;\n> > +\tunsigned int captureCount_;\n> > +\tunsigned int captureLimit_;\n> > +\n> > +\tstd::weak_ptr timeSheet_;\n> > +};\n> > --\n> > 2.40.1\n> >\n\nThanks for all the input :-)\n\nCheers,\nStefan","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D1942BD808\n\tfor ;\n\tFri, 15 Mar 2024 15:53:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F0DDC62C8C;\n\tFri, 15 Mar 2024 16:53:07 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 90FF761C65\n\tfor ;\n\tFri, 15 Mar 2024 16:53:06 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:c477:4921:2179:342c])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 609FEB1;\n\tFri, 15 Mar 2024 16:52:42 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Uc5SgaPw\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1710517962;\n\tbh=QQv3KcQeTzMwD1RfYGtlmS2qULOEa999mLD7GYqeVlg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Uc5SgaPwG69ZNlE9L0DfOPPmQf1rLm1gGFEC30ZXqlcl5oTDbwD9wAEWX41YLf+mu\n\t83/IHJwnZoJhZZpUjixojs1dxWHARIJHQpdJOS+iwurVj4FY2UUlO5z9cTIH2wT2C9\n\t0bcWRLTe6yWZ+b3UqYO6m1zlgtx2xNsyBkk0lkgo=","Date":"Fri, 15 Mar 2024 16:53:03 +0100","From":"Stefan Klug ","To":"Jacopo Mondi ","Subject":"Re: [PATCH v2 03/12] libcamera: lc-compliance: Add initial set of\n\tper-frame-control tests","Message-ID":"<20240315155303.m2yfff7w2fcfydtw@jasper>","References":"<20240313121223.138150-1-stefan.klug@ideasonboard.com>\n\t<20240313121223.138150-4-stefan.klug@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29018,"web_url":"https://patchwork.libcamera.org/comment/29018/","msgid":"<752it44re455lb25sd4et5osimvpnupxn2ayoqz75f7t3nqblk@4sfpj3xgvg7e>","date":"2024-03-21T08:47:53","subject":"Re: [PATCH v2 03/12] libcamera: lc-compliance: Add initial set of\n\tper-frame-control tests","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Stefan\n\nOn Fri, Mar 15, 2024 at 04:53:03PM +0100, Stefan Klug wrote:\n> Hi Jacopo,\n>\n> thanks for the review.\n>\n> On Fri, Mar 15, 2024 at 03:42:39PM +0100, Jacopo Mondi wrote:\n> > Hi Stefan\n> >\n> > On Wed, Mar 13, 2024 at 01:12:14PM +0100, Stefan Klug wrote:\n> > > These tests check if controls (only exposure time and analogue gain at\n> > > the moment) get applied on the frame they were requested for.\n> > >\n> > > This is tested by looking at the metadata and the mean brightness\n> > > of the image center.\n> > >\n> > > At the moment these tests fail. Fixes for the pipelines will be delivered\n> > > in later patches.\n> >\n> > Nice!\n> >\n> > >\n> > > To run only the teste, one can run:\n> >\n> > s/teste/test\n> >\n> > > lc-compliance -c -f \"SingleStream.*\"\n> > >\n> > > Note that the current implementation is a bit picky on what the camera\n> > > actually sees. If it is too dark (or too bright), the tests will fail.\n> > > Looking at a white wall in a normally lit office usually works.\n> >\n> > Mmm, is this ok for a compliance suite ? Is this the reason the image\n> > tests are 'optional' ?\n>\n> Yes, in the beginning I wasn't sure how far we should go in first place.\n> You basically have two options here:\n> - Dynamically finding a \"base\" exposure time that works (e.g. bright\n> enough that you see a difference when the values increase and dim\n> enough to not saturate any pixel). This might work but has a large\n> dependency on the efficiency of the sensor and might still be flaky.\n> - Building and distributing a physical testrig with defined brightness.\n> This is cool, but a larger task.\n>\n> So I started small added this 'optional' term so that we could collect\n> some practical experience on how stable these tests are on different\n> devices.\n>\n> >\n> > >\n> > > Signed-off-by: Stefan Klug \n> > > ---\n> > > src/apps/lc-compliance/capture_test.cpp | 46 +++\n> > > src/apps/lc-compliance/meson.build | 1 +\n> > > src/apps/lc-compliance/per_frame_controls.cpp | 316 ++++++++++++++++++\n> > > src/apps/lc-compliance/per_frame_controls.h | 43 +++\n> > > 4 files changed, 406 insertions(+)\n> > > create mode 100644 src/apps/lc-compliance/per_frame_controls.cpp\n> > > create mode 100644 src/apps/lc-compliance/per_frame_controls.h\n> > >\n> > > diff --git a/src/apps/lc-compliance/capture_test.cpp b/src/apps/lc-compliance/capture_test.cpp\n> > > index 1dcfcf92..b19e8936 100644\n> > > --- a/src/apps/lc-compliance/capture_test.cpp\n> > > +++ b/src/apps/lc-compliance/capture_test.cpp\n> > > @@ -11,6 +11,7 @@\n> > > #include \n> > >\n> > > #include \"environment.h\"\n> > > +#include \"per_frame_controls.h\"\n> > > #include \"simple_capture.h\"\n> > >\n> > > using namespace libcamera;\n> > > @@ -133,3 +134,48 @@ INSTANTIATE_TEST_SUITE_P(CaptureTests,\n> > > \t\t\t testing::Combine(testing::ValuesIn(ROLES),\n> > > \t\t\t\t\t testing::ValuesIn(NUMREQUESTS)),\n> > > \t\t\t SingleStream::nameParameters);\n> > > +\n> > > +/*\n> > > + * Test Per frame controls\n> > > + */\n> > > +TEST_F(SingleStream, testExposureGainChangeOnSameFrame)\n> > > +{\n> > > +\tPerFrameControls capture(camera_);\n> > > +\tcapture.configure(StreamRole::VideoRecording);\n> > > +\tcapture.testExposureGainChangeOnSameFrame();\n> > > +}\n> > > +\n> > > +TEST_F(SingleStream, testFramePreciseExposureChange)\n> > > +{\n> > > +\tPerFrameControls capture(camera_);\n> > > +\tcapture.configure(StreamRole::VideoRecording);\n> > > +\tcapture.testFramePreciseExposureChange();\n> > > +}\n> > > +\n> > > +TEST_F(SingleStream, testFramePreciseGainChange)\n> > > +{\n> > > +\tPerFrameControls capture(camera_);\n> > > +\tcapture.configure(StreamRole::VideoRecording);\n> > > +\tcapture.testFramePreciseGainChange();\n> > > +}\n> > > +\n> > > +TEST_F(SingleStream, testExposureGainIsAppliedOnFirstFrame)\n> > > +{\n> > > +\tPerFrameControls capture(camera_);\n> > > +\tcapture.configure(StreamRole::VideoRecording);\n> > > +\tcapture.testExposureGainIsAppliedOnFirstFrame();\n> > > +}\n> > > +\n> > > +TEST_F(SingleStream, testExposureGainFromFirstRequestGetsApplied)\n> > > +{\n> > > +\tPerFrameControls capture(camera_);\n> > > +\tcapture.configure(StreamRole::VideoRecording);\n> > > +\tcapture.testExposureGainFromFirstRequestGetsApplied();\n> > > +}\n> > > +\n> > > +TEST_F(SingleStream, testExposureGainFromFirstAndSecondRequestGetsApplied)\n> > > +{\n> > > +\tPerFrameControls capture(camera_);\n> > > +\tcapture.configure(StreamRole::VideoRecording);\n> > > +\tcapture.testExposureGainFromFirstAndSecondRequestGetsApplied();\n> > > +}\n> >\n> > This now shows up as\n> >\n> > SingleStream.\n> > testExposureGainChangeOnSameFrame\n> > testFramePreciseExposureChange\n> > testFramePreciseGainChange\n> > testExposureGainIsAppliedOnFirstFrame\n> > testExposureGainFromFirstRequestGetsApplied\n> > testExposureGainFromFirstAndSecondRequestGetsApplied\n> >\n> > And we already have\n> >\n> > CaptureTests/SingleStream.\n> > Capture/Raw_1 # GetParam() = (Raw, 1)\n> > Capture/Raw_2 # GetParam() = (Raw, 2)\n> > Capture/Raw_3 # GetParam() = (Raw, 3)\n> > Capture/Raw_5 # GetParam() = (Raw, 5)\n> > Capture/Raw_8 # GetParam() = (Raw, 8)\n> > Capture/Raw_13 # GetParam() = (Raw, 13)\n> > Capture/Raw_21 # GetParam() = (Raw, 21)\n> > Capture/Raw_34 # GetParam() = (Raw, 34)\n> > Capture/Raw_55 # GetParam() = (Raw, 55)\n> > Capture/Raw_89 # GetParam() = (Raw, 89)\n> >\n> > I would have not instantiated these tests in capture_test.cpp but\n> > directly in per_frame_control.cpp and I would have named them\n> > \"PerFrameControl\". To do so you need to define a test class that\n> > derives from testing::Test in per_frame_control.cpp\n> >\n> >\n> > +/*\n> > + * Test Per frame controls\n> > + */\n> > +\n> > +class PerFrameControlTest : public testing::Test\n> > +{\n> > +protected:\n> > + void SetUp() override;\n> > + void TearDown() override;\n> > +\n> > + std::shared_ptr camera_;\n> > +};\n> > +\n> > +void PerFrameControlTest::SetUp()\n> > +{\n> > + Environment *env = Environment::get();\n> > +\n> > + camera_ = env->cm()->get(env->cameraId());\n> > +\n> > + ASSERT_EQ(camera_->acquire(), 0);\n> > +}\n> > +\n> > +void PerFrameControlTest::TearDown()\n> > +{\n> > + if (!camera_)\n> > + return;\n> > +\n> > + camera_->release();\n> > + camera_.reset();\n> > +}\n> > +\n> > +TEST_F(PerFrameControlTest, testExposureGainChangeOnSameFrame)\n> > .....\n> >\n> > With this you get a dedicated test suite\n> >\n> > PerFrameControlTest.\n> > testExposureGainChangeOnSameFrame\n> > testFramePreciseExposureChange\n> > testFramePreciseGainChange\n> > testExposureGainIsAppliedOnFirstFrame\n> > testExposureGainFromFirstRequestGetsApplied\n> > testExposureGainFromFirstAndSecondRequestGetsApplied\n> >\n> > Also, you now can drop the\n> >\n> > \tvoid testExposureGainChangeOnSameFrame();\n> > \tvoid testFramePreciseExposureChange();\n> > \tvoid testFramePreciseGainChange();\n> > \tvoid testExposureGainIsAppliedOnFirstFrame();\n> > \tvoid testExposureGainFromFirstRequestGetsApplied();\n> > \tvoid testExposureGainFromFirstAndSecondRequestGetsApplied();\n> >\n> > functions from the PerFrameControl class, and implement the tests in\n> > the test definition instead of having them as wrappers that call the\n> > PerFrameControl class' functions\n> >\n> > TL;DR do this:\n> >\n> > TEST_F(PerFrameControlTest, testExposureGainChangeOnSameFrame)\n> > {\n> > \tPerFrameControls capture(camera_);\n> > \tcapture.configure(StreamRole::VideoRecording);\n> >\n> > \tControlList startValues;\n> > \tstartValues.set(controls::ExposureTime, 5000);\n> > \tstartValues.set(controls::AnalogueGain, 1.0);\n> >\n> > \tauto timeSheet = capture.startCaptureWithTimeSheet(10, &startValues);\n> > \tauto &ts = *timeSheet;\n> >\n> > \t/* wait a few frames to settle */\n> > \tts[7].controls().set(controls::ExposureTime, 10000);\n> > \tts[7].controls().set(controls::AnalogueGain, 4.0);\n> >\n> > ...\n> > }\n> >\n> >\n> > in place of:\n> >\n> > TEST_F(PerFrameControlTest, testFramePreciseExposureChange)\n> > {\n> > \tPerFrameControls capture(camera_);\n> > \tcapture.configure(StreamRole::VideoRecording);\n> > \tcapture.testFramePreciseExposureChange();\n> > }\n> >\n>\n> Great. Thanks for that. I didn't want to spend too much time inside\n> google test before getting feedback on the overall direction. Your\n> proposal is perfect. I'll do that.\n>\n> >\n> >\n> > > diff --git a/src/apps/lc-compliance/meson.build b/src/apps/lc-compliance/meson.build\n> > > index eb7b2d71..2a6f52af 100644\n> > > --- a/src/apps/lc-compliance/meson.build\n> > > +++ b/src/apps/lc-compliance/meson.build\n> > > @@ -15,6 +15,7 @@ lc_compliance_sources = files([\n> > > 'capture_test.cpp',\n> > > 'environment.cpp',\n> > > 'main.cpp',\n> > > + 'per_frame_controls.cpp',\n> > > 'simple_capture.cpp',\n> > > 'time_sheet.cpp',\n> > > ])\n> > > diff --git a/src/apps/lc-compliance/per_frame_controls.cpp b/src/apps/lc-compliance/per_frame_controls.cpp\n> > > new file mode 100644\n> > > index 00000000..eb7164e0\n> > > --- /dev/null\n> > > +++ b/src/apps/lc-compliance/per_frame_controls.cpp\n> > > @@ -0,0 +1,316 @@\n> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > +/*\n> > > + * Copyright (C) 2024, Ideas on Board Oy\n> > > + *\n> > > + * per_frame_controls.cpp - Tests for per frame controls\n> > > + */\n> > > +#include \"per_frame_controls.h\"\n> > > +\n> > > +#include \n> > > +\n> > > +#include \"time_sheet.h\"\n> > > +\n> > > +using namespace libcamera;\n> > > +\n> > > +static const bool doImageTests = true;\n> > > +\n> > > +PerFrameControls::PerFrameControls(std::shared_ptr camera)\n> > > +\t: SimpleCapture(camera)\n> > > +{\n> > > +}\n> > > +\n> > > +std::shared_ptr\n> > > +PerFrameControls::startCaptureWithTimeSheet(unsigned int framesToCapture, const ControlList *controls)\n> > > +{\n> > > +\tControlList ctrls(camera_->controls().idmap());\n> >\n> > Empty line please\n> >\n> > > +\t/* Ensure defined default values */\n> > > +\tctrls.set(controls::AeEnable, false);\n> > > +\tctrls.set(controls::AeExposureMode, controls::ExposureCustom);\n> > > +\tctrls.set(controls::ExposureTime, 10000);\n> > > +\tctrls.set(controls::AnalogueGain, 1.0);\n> > > +\n> > > +\tif (controls)\n> > > +\t\tctrls.merge(*controls, ControlList::MergePolicy::OverwriteExisting);\n> > > +\n> > > +\tstart(&ctrls);\n> > > +\n> > > +\tqueueCount_ = 0;\n> > > +\tcaptureCount_ = 0;\n> > > +\tcaptureLimit_ = framesToCapture;\n> > > +\n> > > +\tauto timeSheet = std::make_shared(captureLimit_, camera_->controls().idmap());\n> > > +\ttimeSheet_ = timeSheet;\n> >\n> > I'm sorry but I don't see why you would use a shared_ptr<> and a weak\n> > reference when the timeSheet_ ownership is not shared with any other\n> > component outside of this class\n>\n> This class is still based on the Capture class. So you are free to write\n> tests and capture without a timesheet. In such tests the weak ptr will\n> automatically be empty and the queueRequest and requestComplete\n> functions still work properly. The lifetime of the timesheet is bound to\n> the scope of the test function. (You could even use the\n> startCaptureWithTimeSheet() and ignore the result, in wich case the\n> timesheet would get destroyed immediately).\n>\n\nLet me re-cap (I'm looking at v3)\n\nThe PerFrameControlsCapture class:\n\n- has a member std::weak_ptr timeSheet_\n- has a\n std::shared_ptr\n\tstartCaptureWithTimeSheet(unsigned int framesToCapture,\n\t\t\t\t const libcamera::ControlList *controls = nullptr);\n\n function that\n\n * creates a TimeSheet as shared_ptr<> and returns it to the caller\n * Initialize the weak_ref<> class member with the shared_ptr<>\n\n\tauto timeSheet = std::make_shared(captureLimit_,\n\t\t\t\t\t\t camera_->controls().idmap());\n\ttimeSheet_ = timeSheet;\n\treturn timeSheet;\n\nso, if I read it right, you create a shared_ptr<> (refcount = 1),\ninitialize a weak_ptr<> (no reference count increment) return a\nshared_ptr<> by copy (refcount = 2) and the end of the function scope\nthe local shared_ptr<> is destroyed (refcount = 1). Now the pointer\nownership is on the caller only, so you basically trasferred the\nownership outside of the class and refers to that within the class\nwith a weak_ptr<>.\n\nIn facts\n\n--- a/src/apps/lc-compliance/per_frame_controls_test.cpp\n+++ b/src/apps/lc-compliance/per_frame_controls_test.cpp\n@@ -157,6 +157,8 @@ TEST_F(PerFrameControlTests, testExposureGainChangeOnSameFrame)\n startValues.set(controls::AnalogueGain, 1.0);\n\n auto timeSheet = capture.startCaptureWithTimeSheet(10, &startValues);\n+ std::cerr << \" TIMESHEET USE COUNT: \" << timeSheet.use_count();\n+\n auto &ts = *timeSheet;\n\nGives me\n TIMESHEET USE COUNT: 1\n\nif ever the caller does\n\n {\n ts = capture.startCaptureWithTimeSheet(10, &startValues);\n }\n\nThe instance of the TimeSheet class the timeSheet_ member refers to gets\ndestroyed. It's 'safe' as it's a weak_ptr<> but I don't think that's what you\nwant.\n\nIf you want to share ownership of the timesheet between the\nPerFrameControlsCapture class and the caller, so that it gets\ndestroyed when the the last one of the two owners gets destroyed,\njust use a shared_ptr<>\n\n--- a/src/apps/lc-compliance/per_frame_controls_test.cpp\n+++ b/src/apps/lc-compliance/per_frame_controls_test.cpp\n@@ -60,7 +60,7 @@ public:\n unsigned int captureCount_;\n unsigned int captureLimit_;\n\n- std::weak_ptr timeSheet_;\n+ std::shared_ptr timeSheet_;\n };\n\n static const bool doImageTests = true;\n@@ -91,10 +91,9 @@ PerFrameControlsCapture::startCaptureWithTimeSheet(unsigned int framesToCapture,\n captureCount_ = 0;\n captureLimit_ = framesToCapture;\n\n- auto timeSheet = std::make_shared(captureLimit_,\n+ timeSheet_ = std::make_shared(captureLimit_,\n camera_->controls().idmap());\n- timeSheet_ = timeSheet;\n- return timeSheet;\n+ return timeSheet_;\n }\n\n int PerFrameControlsCapture::queueRequest(Request *request)\n@@ -103,18 +102,16 @@ int PerFrameControlsCapture::queueRequest(Request *request)\n if (queueCount_ > captureLimit_)\n return 0;\n\n- auto ts = timeSheet_.lock();\n- if (ts)\n- ts->prepareForQueue(request, queueCount_ - 1);\n+ if (timeSheet_)\n+ timeSheet_->prepareForQueue(request, queueCount_ - 1);\n\n return camera_->queueRequest(request);\n }\n\n void PerFrameControlsCapture::requestComplete(Request *request)\n {\n- auto ts = timeSheet_.lock();\n- if (ts)\n- ts->handleCompleteRequest(request);\n+ if (timeSheet_)\n+ timeSheet_->handleCompleteRequest(request);\n\n captureCount_++;\n if (captureCount_ >= captureLimit_) {\n\nSo that now\n\n\tauto timeSheet = capture.startCaptureWithTimeSheet(10, &startValues);\n\tstd::cerr << \" TIMESHEET USE COUNT: \" << timeSheet.use_count();\n\nReads as\n TIMESHEET USE COUNT: 2\n\n> >\n> > > +\treturn timeSheet;\n> > > +}\n> > > +\n> > > +int PerFrameControls::queueRequest(Request *request)\n> > > +{\n> > > +\tqueueCount_++;\n> > > +\tif (queueCount_ > captureLimit_)\n> > > +\t\treturn 0;\n> > > +\n> > > +\tauto ts = timeSheet_.lock();\n> > > +\tif (ts)\n> > > +\t\tts->prepareForQueue(request, queueCount_ - 1);\n> > > +\n> > > +\treturn camera_->queueRequest(request);\n> > > +}\n> > > +\n> > > +void PerFrameControls::requestComplete(Request *request)\n> > > +{\n> > > +\tauto ts = timeSheet_.lock();\n> > > +\tif (ts)\n> > > +\t\tts->handleCompleteRequest(request);\n> > > +\n> > > +\tcaptureCount_++;\n> > > +\tif (captureCount_ >= captureLimit_) {\n> > > +\t\tloop_->exit(0);\n> > > +\t\treturn;\n> > > +\t}\n> > > +\n> > > +\trequest->reuse(Request::ReuseBuffers);\n> > > +\tif (queueRequest(request))\n> > > +\t\tloop_->exit(-EINVAL);\n> > > +}\n> > > +\n> > > +void PerFrameControls::runCaptureSession()\n> > > +{\n> > > +\tStream *stream = config_->at(0).stream();\n> > > +\tconst std::vector> &buffers = allocator_->buffers(stream);\n> > > +\n> > > +\t/* Queue the recommended number of reqeuests. */\n> >\n> > s/reqeuests/requests/\n> >\n> > > +\tfor (const std::unique_ptr &buffer : buffers) {\n> > > +\t\tstd::unique_ptr request = camera_->createRequest();\n> > > +\t\trequest->addBuffer(stream, buffer.get());\n> > > +\t\tqueueRequest(request.get());\n> > > +\t\trequests_.push_back(std::move(request));\n> > > +\t}\n> > > +\n> > > +\t/* Run capture session. */\n> > > +\tloop_ = new EventLoop();\n> > > +\tloop_->exec();\n> > > +\tstop();\n> > > +\tdelete loop_;\n> > > +}\n> > > +\n> > > +void PerFrameControls::testExposureGainChangeOnSameFrame()\n> > > +{\n> > > +\tControlList startValues;\n> > > +\tstartValues.set(controls::ExposureTime, 5000);\n> > > +\tstartValues.set(controls::AnalogueGain, 1.0);\n> > > +\n> > > +\tauto timeSheet = startCaptureWithTimeSheet(10, &startValues);\n> > > +\tauto &ts = *timeSheet;\n> >\n> > Why a temporary reference ?\n>\n> The shared_ptr keeps the timesheet alive. The reference is just\n\nIt's a reference, so it doesn't increment the usage count\n\n> syntactic shugar to be able to write ts[x]. I could replace these with\n> (*ts)[x] or ts->get(x) if you like that better.\n>\n\nAh ok, seeing a reference just to be able to \"ts[]\" brings more\nquestions on why the reference is there than clarity imho. Up to you.\n\n> >\n> > > +\n> > > +\t/* wait a few frames to settle */\n> > > +\tts[7].controls().set(controls::ExposureTime, 10000);\n> > > +\tts[7].controls().set(controls::AnalogueGain, 4.0);\n> > > +\n> > > +\trunCaptureSession();\n> > > +\n> > > +\t/* Uncomment this to debug the test */\n> > > +\t/* ts.printAllInfos(); */\n> >\n> > Please drop\n> >\n> > > +\n> > > +\tASSERT_TRUE(ts[5].metadata().contains(controls::ExposureTime.id())) << \"Required metadata entry is missing\";\n> > > +\tASSERT_TRUE(ts[5].metadata().contains(controls::AnalogueGain.id())) << \"Required metadata entry is missing\";\n> >\n> > Break long lines when possible\n> >\n> > > +\n> > > +\tEXPECT_NEAR(ts[3].metadata().get(controls::ExposureTime).value(), 5000, 20);\n> > > +\tEXPECT_NEAR(ts[3].metadata().get(controls::AnalogueGain).value(), 1.0, 0.05);\n> > > +\n> > > +\t//find the frame with the changes\n> >\n> > No C++ comments please\n> >\n> > > +\tint exposureChangeIndex = 0;\n> > > +\tfor (unsigned i = 3; i < ts.size(); i++) {\n> > > +\t\tif (ts[i].metadata().get(controls::ExposureTime).value() > 7500) {\n> > > +\t\t\texposureChangeIndex = i;\n> > > +\t\t\tbreak;\n> > > +\t\t}\n> > > +\t}\n> > > +\n> > > +\tint gainChangeIndex = 0;\n> > > +\tfor (unsigned i = 3; i < ts.size(); i++) {\n> > > +\t\tif (ts[i].metadata().get(controls::AnalogueGain).value() > 2.0) {\n> > > +\t\t\tgainChangeIndex = i;\n> > > +\t\t\tbreak;\n> > > +\t\t}\n> > > +\t}\n> > > +\n> > > +\tEXPECT_NE(exposureChangeIndex, 0) << \"Exposure change not found in metadata\";\n> > > +\tEXPECT_NE(gainChangeIndex, 0) << \"Gain change not found in metadata\";\n> > > +\tEXPECT_EQ(exposureChangeIndex, gainChangeIndex)\n> > > +\t\t<< \"Metadata contained gain and exposure changes on different frames\";\n> > > +\n> > > +\tif (doImageTests) {\n> >\n> > Why do you think it should be optional ? If it has to be made optional\n> > it should be done in a way that doesn't depend on a compile time\n> > constant defined in the source code. Either make a series of separate\n> > image tests or add an option to lc-compliance.\n>\n> The tests still have a value without the content based tests, so yes\n> this should propably be a option to lc-compliance. We could default that\n> to false to express the 'optional' aspect.\n>\n> >\n> > > +\t\tint brightnessChangeIndex = 0;\n> > > +\t\tfor (unsigned i = 3; i < ts.size(); i++) {\n> >\n> > The usage of '3' seems to be there to ignore the first three frames,\n> > right ? If so, what about defining a constant and add a comment ?\n> >\n> > > +\t\t\tif (ts[i].getBrightnessChange() > 1.3) {\n> > > +\t\t\t\tEXPECT_EQ(brightnessChangeIndex, 0)\n> > > +\t\t\t\t\t<< \"Detected multiple frames with brightness increase (Wrong control delays?)\";\n> > > +\n> > > +\t\t\t\tif (!brightnessChangeIndex)\n> > > +\t\t\t\t\tbrightnessChangeIndex = i;\n> > > +\t\t\t}\n> > > +\t\t}\n> > > +\n> > > +\t\tEXPECT_EQ(exposureChangeIndex, brightnessChangeIndex)\n> > > +\t\t\t<< \"Exposure change and mesaured brightness change were not on same frame. \"\n> > > +\t\t\t<< \"(Wrong control delay?, Start frame event too late?)\";\n> > > +\t\tEXPECT_EQ(exposureChangeIndex, gainChangeIndex)\n> > > +\t\t\t<< \"Gain change and mesaured brightness change were not on same frame. \"\n> > > +\t\t\t<< \"(Wrong control delay?, Start frame event too late?)\";\n> > > +\t}\n> > > +}\n> > > +\n> > > +void PerFrameControls::testFramePreciseExposureChange()\n> > > +{\n> > > +\tauto timeSheet = startCaptureWithTimeSheet(10);\n> > > +\tauto &ts = *timeSheet;\n> > > +\n> > > +\tts[3].controls().set(controls::ExposureTime, 5000);\n> > > +\t/* wait a few frames to settle */\n> > > +\tts[6].controls().set(controls::ExposureTime, 20000);\n> > > +\n> > > +\trunCaptureSession();\n> > > +\n> > > +\t/* Uncomment this to debug the test */\n> > > +\t/* ts.printAllInfos(); */\n> >\n> > ditto\n> >\n> > > +\n> > > +\tASSERT_TRUE(ts[5].metadata().contains(controls::ExposureTime.id())) << \"Required metadata entry is missing\";\n> >\n> > break long lines\n> >\n> > > +\n> > > +\tEXPECT_NEAR(ts[5].metadata().get(controls::ExposureTime).value(), 5000, 20);\n> > > +\tEXPECT_NEAR(ts[6].metadata().get(controls::ExposureTime).value(), 20000, 20);\n> > > +\n> > > +\tif (doImageTests) {\n> > > +\t\t/* No increase just before setting exposure */\n> > > +\t\tEXPECT_NEAR(ts[5].getBrightnessChange(), 1.0, 0.05)\n> > > +\t\t\t<< \"Brightness changed too much before the expected time of change (control delay too high?).\";\n> > > +\t\t/*\n> > > +\t\t* Todo: The change is brightness was a bit low\n> >\n> > Wrong alignment\n> >\n> > We don't Doxygen lc-compliance but try to use \\todo for consistency\n> > with the rest of the code base\n> >\n> > > +\t\t* (Exposure time increase by 4x resulted in a brightness increase of < 2).\n> > > +\t\t* This should be investigated.\n> >\n> > Might be platform specific issue ?\n> >\n> > > +\t\t*/\n> > > +\t\tEXPECT_GT(ts[6].getBrightnessChange(), 1.3)\n> > > +\t\t\t<< \"Brightness in frame \" << 6 << \" did not increase as expected (reference: \"\n> > > +\t\t\t<< ts[3].getSpotBrightness() << \" current: \" << ts[6].getSpotBrightness() << \" )\" << std::endl;\n> > > +\n> > > +\t\t/* No increase just after setting exposure */\n> > > +\t\tEXPECT_NEAR(ts[7].getBrightnessChange(), 1.0, 0.05)\n> > > +\t\t\t<< \"Brightness changed too much after the expected time of change (control delay too low?).\";\n> > > +\n> > > +\t\t/* No increase just after setting exposure */\n> > > +\t\tEXPECT_NEAR(ts[8].getBrightnessChange(), 1.0, 0.05)\n> > > +\t\t\t<< \"Brightness changed too much 2 frames after the expected time of change (control delay too low?).\";\n> > > +\t}\n> > > +}\n> > > +\n> > > +void PerFrameControls::testFramePreciseGainChange()\n> > > +{\n> > > +\tauto timeSheet = startCaptureWithTimeSheet(10);\n> > > +\tauto &ts = *timeSheet;\n> > > +\n> > > +\tts[3].controls().set(controls::AnalogueGain, 1.0);\n> > > +\t/* wait a few frames to settle */\n> > > +\tts[6].controls().set(controls::AnalogueGain, 4.0);\n> > > +\n> > > +\trunCaptureSession();\n> > > +\n> > > +\t/* Uncomment this, to debug the test */\n> > > +\t/* ts.printAllInfos(); */\n> > > +\n> > > +\tASSERT_TRUE(ts[5].metadata().contains(controls::AnalogueGain.id())) << \"Required metadata entry is missing\";\n> >\n> > Break this long line\n> >\n> > > +\n> > > +\tEXPECT_NEAR(ts[5].metadata().get(controls::AnalogueGain).value(), 1.0, 0.1);\n> > > +\tEXPECT_NEAR(ts[6].metadata().get(controls::AnalogueGain).value(), 4.0, 0.1);\n> > > +\n> > > +\tif (doImageTests) {\n> > > +\t\t/* No increase just before setting gain */\n> > > +\t\tEXPECT_NEAR(ts[5].getBrightnessChange(), 1.0, 0.05)\n> > > +\t\t\t<< \"Brightness changed too much before the expected time of change (control delay too high?).\";\n> > > +\t\t/*\n> > > +\t\t* Todo: I see a brightness change of roughly half the expected one.\n> >\n> > Wrong alignment here too, also \\todo\n> >\n> > > +\t\t* This is not yet understood and needs investigation\n> >\n> > Defintely some platform specific thing to investigate then ?\n> >\n> > > +\t\t*/\n> > > +\t\tEXPECT_GT(ts[6].getBrightnessChange(), 1.7)\n> > > +\t\t\t<< \"Brightness in frame \" << 6 << \" did not increase as expected (reference: \"\n> > > +\t\t\t<< ts[5].getSpotBrightness() << \" current: \" << ts[6].getSpotBrightness() << \" )\" << std::endl;\n> > > +\n> > > +\t\t/* No increase just after setting gain */\n> > > +\t\tEXPECT_NEAR(ts[7].getBrightnessChange(), 1.0, 0.05)\n> > > +\t\t\t<< \"Brightness changed too much after the expected time of change (control delay too low?).\";\n> > > +\n> > > +\t\t/* No increase just after setting gain */\n> > > +\t\tEXPECT_NEAR(ts[8].getBrightnessChange(), 1.0, 0.05)\n> > > +\t\t\t<< \"Brightness changed too much after the expected time of change (control delay too low?).\";\n> > > +\t}\n> > > +}\n> > > +\n> > > +void PerFrameControls::testExposureGainFromFirstRequestGetsApplied()\n> > > +{\n> > > +\tauto timeSheet = startCaptureWithTimeSheet(5);\n> > > +\tauto &ts = *timeSheet;\n> > > +\n> > > +\tts[0].controls().set(controls::ExposureTime, 10000);\n> > > +\tts[0].controls().set(controls::AnalogueGain, 4.0);\n> > > +\n> > > +\trunCaptureSession();\n> > > +\n> > > +\tASSERT_TRUE(ts[4].metadata().contains(controls::ExposureTime.id())) << \"Required metadata entry is missing\";\n> > > +\tASSERT_TRUE(ts[4].metadata().contains(controls::AnalogueGain.id())) << \"Required metadata entry is missing\";\n> >\n> > You can easily break these lines\n> >\n> > > +\n> > > +\t/* We expect it to be applied after 3 frames, the latest*/\n> >\n> > What if a sensor takes a longer time to apply exposure and gain ?\n>\n> Then we should adjust the test :-)\n>\n> >\n> > > +\tEXPECT_NEAR(ts[4].metadata().get(controls::ExposureTime).value(), 10000, 20);\n> > > +\tEXPECT_NEAR(ts[4].metadata().get(controls::AnalogueGain).value(), 4.0, 0.1);\n> > > +}\n> > > +\n> > > +void PerFrameControls::testExposureGainFromFirstAndSecondRequestGetsApplied()\n> > > +{\n> > > +\tauto timeSheet = startCaptureWithTimeSheet(5);\n> > > +\tauto &ts = *timeSheet;\n> > > +\n> > > +\tts[0].controls().set(controls::ExposureTime, 8000);\n> > > +\tts[0].controls().set(controls::AnalogueGain, 2.0);\n> > > +\tts[1].controls().set(controls::ExposureTime, 10000);\n> > > +\tts[1].controls().set(controls::AnalogueGain, 4.0);\n> >\n> > As a general question, how do we guarantee the values you use to set\n> > exposure and gains are valid for all possible sensors ?\n> >\n> > Shouldn't you inspect inspect the ControlInfo limit from\n> > Camera::controls() and clamp the values in the min/max range ?\n>\n> I thought about that too. I started with values that most if not all\n> sensor we care atm should support. I'm weary if it is worth the effort\n> and that it would make the tests way less readable. But you are right,\n> that is an unsolved issue.\n\nRecording it with a \\todo comment is fine for now\n\n>\n> >\n> > > +\n> > > +\trunCaptureSession();\n> > > +\n> > > +\tASSERT_TRUE(ts[4].metadata().contains(controls::ExposureTime.id())) << \"Required metadata entry is missing\";\n> > > +\tASSERT_TRUE(ts[4].metadata().contains(controls::AnalogueGain.id())) << \"Required metadata entry is missing\";\n> >\n> > Ditto\n> >\n> > > +\n> > > +\t/* We expect it to be applied after 3 frames, the latest */\n> > > +\tEXPECT_NEAR(ts[4].metadata().get(controls::ExposureTime).value(), 10000, 20);\n> > > +\tEXPECT_NEAR(ts[4].metadata().get(controls::AnalogueGain).value(), 4.0, 0.1);\n> > > +}\n> > > +\n> > > +void PerFrameControls::testExposureGainIsAppliedOnFirstFrame()\n> > > +{\n> > > +\tControlList startValues;\n> > > +\tstartValues.set(controls::ExposureTime, 5000);\n> > > +\tstartValues.set(controls::AnalogueGain, 1.0);\n> > > +\n> > > +\tauto ts1 = startCaptureWithTimeSheet(3, &startValues);\n> > > +\n> > > +\trunCaptureSession();\n> > > +\n> > > +\tASSERT_TRUE((*ts1)[0].metadata().contains(controls::ExposureTime.id())) << \"Required metadata entry is missing\";\n> > > +\tASSERT_TRUE((*ts1)[0].metadata().contains(controls::AnalogueGain.id())) << \"Required metadata entry is missing\";\n> > > +\n> > > +\tEXPECT_NEAR((*ts1)[0].metadata().get(controls::ExposureTime).value(), 5000, 20);\n> > > +\tEXPECT_NEAR((*ts1)[0].metadata().get(controls::AnalogueGain).value(), 1.0, 0.02);\n> > > +\n> > > +\t/* Second capture with different values to ensure we don't hit default/old values */\n> > > +\tstartValues.set(controls::ExposureTime, 15000);\n> > > +\tstartValues.set(controls::AnalogueGain, 4.0);\n> > > +\n> > > +\tauto ts2 = startCaptureWithTimeSheet(3, &startValues);\n> > > +\n> > > +\trunCaptureSession();\n> > > +\n> > > +\tEXPECT_NEAR((*ts2)[0].metadata().get(controls::ExposureTime).value(), 15000, 20);\n> > > +\tEXPECT_NEAR((*ts2)[0].metadata().get(controls::AnalogueGain).value(), 4.0, 0.02);\n> > > +\n> > > +\tif (doImageTests) {\n> > > +\t\t/* With 3x exposure and 4x gain we could expect a brightness increase of 2x */\n> > > +\t\tdouble brightnessChange = ts2->get(1).getSpotBrightness() / ts1->get(1).getSpotBrightness();\n> > > +\t\tEXPECT_GT(brightnessChange, 2.0);\n> > > +\t}\n> > > +}\n> > > diff --git a/src/apps/lc-compliance/per_frame_controls.h b/src/apps/lc-compliance/per_frame_controls.h\n> > > new file mode 100644\n> > > index 00000000..a341c61f\n> > > --- /dev/null\n> > > +++ b/src/apps/lc-compliance/per_frame_controls.h\n> > > @@ -0,0 +1,43 @@\n> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > +/*\n> > > + * Copyright (C) 2024, Ideas on Board Oy\n> > > + *\n> > > + * per_frame_controls.h - Tests for per frame controls\n> > > + */\n> > > +\n> > > +#pragma once\n> > > +\n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +#include \"../common/event_loop.h\"\n> > > +\n> > > +#include \"simple_capture.h\"\n> > > +#include \"time_sheet.h\"\n> > > +\n> > > +class PerFrameControls : public SimpleCapture\n> > > +{\n> > > +public:\n> > > +\tPerFrameControls(std::shared_ptr camera);\n> > > +\n> > > +\tstd::shared_ptr\n> > > +\tstartCaptureWithTimeSheet(unsigned int framesToCapture, const libcamera::ControlList *controls = nullptr);\n> > > +\tvoid runCaptureSession();\n> > > +\n> > > +\tvoid testExposureGainChangeOnSameFrame();\n> > > +\tvoid testFramePreciseExposureChange();\n> > > +\tvoid testFramePreciseGainChange();\n> > > +\tvoid testExposureGainIsAppliedOnFirstFrame();\n> > > +\tvoid testExposureGainFromFirstRequestGetsApplied();\n> > > +\tvoid testExposureGainFromFirstAndSecondRequestGetsApplied();\n> > > +\n> > > +\tint queueRequest(libcamera::Request *request);\n> > > +\tvoid requestComplete(libcamera::Request *request) override;\n> > > +\n> > > +\tunsigned int queueCount_;\n> > > +\tunsigned int captureCount_;\n> > > +\tunsigned int captureLimit_;\n> > > +\n> > > +\tstd::weak_ptr timeSheet_;\n> > > +};\n> > > --\n> > > 2.40.1\n> > >\n>\n> Thanks for all the input :-)\n>\n> Cheers,\n> Stefan","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id A5AA5C3272\n\tfor ;\n\tThu, 21 Mar 2024 08:48:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A9B6D63055;\n\tThu, 21 Mar 2024 09:47:59 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5516F61C59\n\tfor ;\n\tThu, 21 Mar 2024 09:47:57 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2001:b07:5d2e:52c9:cc1e:e404:491f:e6ea])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2C9627E9;\n\tThu, 21 Mar 2024 09:47:29 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"MqquGOwA\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1711010849;\n\tbh=xdySIL1qgzBm4gWtZlrPQkNQ5LovNHWTGOU5b3OySo8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=MqquGOwAtZgOasHqnVywfMhCsru6azIB1i5cMggUZKrWSJSQQS8i8nyV6LQfXKFZ1\n\tuxmqTOUERTBMWRqKDBWpt2t6e2u4SmIepkj69WVpDQ4jSlIYyKKCohGhR/XN6lwYR0\n\tcX/6VsainIq6DNv8xnkYtaus8QzLQNvuNgrkW1mQ=","Date":"Thu, 21 Mar 2024 09:47:53 +0100","From":"Jacopo Mondi ","To":"Stefan Klug ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 03/12] libcamera: lc-compliance: Add initial set of\n\tper-frame-control tests","Message-ID":"<752it44re455lb25sd4et5osimvpnupxn2ayoqz75f7t3nqblk@4sfpj3xgvg7e>","References":"<20240313121223.138150-1-stefan.klug@ideasonboard.com>\n\t<20240313121223.138150-4-stefan.klug@ideasonboard.com>\n\t\n\t<20240315155303.m2yfff7w2fcfydtw@jasper>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240315155303.m2yfff7w2fcfydtw@jasper>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29027,"web_url":"https://patchwork.libcamera.org/comment/29027/","msgid":"<20240321113449.qlolurgw53myho6l@jasper>","date":"2024-03-21T11:34:49","subject":"Re: [PATCH v2 03/12] libcamera: lc-compliance: Add initial set of\n\tper-frame-control tests","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Thu, Mar 21, 2024 at 09:47:53AM +0100, Jacopo Mondi wrote:\n> Hi Stefan\n> \n> On Fri, Mar 15, 2024 at 04:53:03PM +0100, Stefan Klug wrote:\n> > Hi Jacopo,\n> >\n> > thanks for the review.\n> >\n> > On Fri, Mar 15, 2024 at 03:42:39PM +0100, Jacopo Mondi wrote:\n> > > Hi Stefan\n> > >\n> > > On Wed, Mar 13, 2024 at 01:12:14PM +0100, Stefan Klug wrote:\n> > > > These tests check if controls (only exposure time and analogue gain at\n> > > > the moment) get applied on the frame they were requested for.\n> > > >\n> > > > This is tested by looking at the metadata and the mean brightness\n> > > > of the image center.\n> > > >\n> > > > At the moment these tests fail. Fixes for the pipelines will be delivered\n> > > > in later patches.\n> > >\n> > > Nice!\n> > >\n> > > >\n> > > > To run only the teste, one can run:\n> > >\n> > > s/teste/test\n> > >\n> > > > lc-compliance -c -f \"SingleStream.*\"\n> > > >\n> > > > Note that the current implementation is a bit picky on what the camera\n> > > > actually sees. If it is too dark (or too bright), the tests will fail.\n> > > > Looking at a white wall in a normally lit office usually works.\n> > >\n> > > Mmm, is this ok for a compliance suite ? Is this the reason the image\n> > > tests are 'optional' ?\n> >\n> > Yes, in the beginning I wasn't sure how far we should go in first place.\n> > You basically have two options here:\n> > - Dynamically finding a \"base\" exposure time that works (e.g. bright\n> > enough that you see a difference when the values increase and dim\n> > enough to not saturate any pixel). This might work but has a large\n> > dependency on the efficiency of the sensor and might still be flaky.\n> > - Building and distributing a physical testrig with defined brightness.\n> > This is cool, but a larger task.\n> >\n> > So I started small added this 'optional' term so that we could collect\n> > some practical experience on how stable these tests are on different\n> > devices.\n> >\n> > >\n> > > >\n> > > > Signed-off-by: Stefan Klug \n> > > > ---\n> > > > src/apps/lc-compliance/capture_test.cpp | 46 +++\n> > > > src/apps/lc-compliance/meson.build | 1 +\n> > > > src/apps/lc-compliance/per_frame_controls.cpp | 316 ++++++++++++++++++\n> > > > src/apps/lc-compliance/per_frame_controls.h | 43 +++\n> > > > 4 files changed, 406 insertions(+)\n> > > > create mode 100644 src/apps/lc-compliance/per_frame_controls.cpp\n> > > > create mode 100644 src/apps/lc-compliance/per_frame_controls.h\n> > > >\n> > > > diff --git a/src/apps/lc-compliance/capture_test.cpp b/src/apps/lc-compliance/capture_test.cpp\n> > > > index 1dcfcf92..b19e8936 100644\n> > > > --- a/src/apps/lc-compliance/capture_test.cpp\n> > > > +++ b/src/apps/lc-compliance/capture_test.cpp\n> > > > @@ -11,6 +11,7 @@\n> > > > #include \n> > > >\n> > > > #include \"environment.h\"\n> > > > +#include \"per_frame_controls.h\"\n> > > > #include \"simple_capture.h\"\n> > > >\n> > > > using namespace libcamera;\n> > > > @@ -133,3 +134,48 @@ INSTANTIATE_TEST_SUITE_P(CaptureTests,\n> > > > \t\t\t testing::Combine(testing::ValuesIn(ROLES),\n> > > > \t\t\t\t\t testing::ValuesIn(NUMREQUESTS)),\n> > > > \t\t\t SingleStream::nameParameters);\n> > > > +\n> > > > +/*\n> > > > + * Test Per frame controls\n> > > > + */\n> > > > +TEST_F(SingleStream, testExposureGainChangeOnSameFrame)\n> > > > +{\n> > > > +\tPerFrameControls capture(camera_);\n> > > > +\tcapture.configure(StreamRole::VideoRecording);\n> > > > +\tcapture.testExposureGainChangeOnSameFrame();\n> > > > +}\n> > > > +\n> > > > +TEST_F(SingleStream, testFramePreciseExposureChange)\n> > > > +{\n> > > > +\tPerFrameControls capture(camera_);\n> > > > +\tcapture.configure(StreamRole::VideoRecording);\n> > > > +\tcapture.testFramePreciseExposureChange();\n> > > > +}\n> > > > +\n> > > > +TEST_F(SingleStream, testFramePreciseGainChange)\n> > > > +{\n> > > > +\tPerFrameControls capture(camera_);\n> > > > +\tcapture.configure(StreamRole::VideoRecording);\n> > > > +\tcapture.testFramePreciseGainChange();\n> > > > +}\n> > > > +\n> > > > +TEST_F(SingleStream, testExposureGainIsAppliedOnFirstFrame)\n> > > > +{\n> > > > +\tPerFrameControls capture(camera_);\n> > > > +\tcapture.configure(StreamRole::VideoRecording);\n> > > > +\tcapture.testExposureGainIsAppliedOnFirstFrame();\n> > > > +}\n> > > > +\n> > > > +TEST_F(SingleStream, testExposureGainFromFirstRequestGetsApplied)\n> > > > +{\n> > > > +\tPerFrameControls capture(camera_);\n> > > > +\tcapture.configure(StreamRole::VideoRecording);\n> > > > +\tcapture.testExposureGainFromFirstRequestGetsApplied();\n> > > > +}\n> > > > +\n> > > > +TEST_F(SingleStream, testExposureGainFromFirstAndSecondRequestGetsApplied)\n> > > > +{\n> > > > +\tPerFrameControls capture(camera_);\n> > > > +\tcapture.configure(StreamRole::VideoRecording);\n> > > > +\tcapture.testExposureGainFromFirstAndSecondRequestGetsApplied();\n> > > > +}\n> > >\n> > > This now shows up as\n> > >\n> > > SingleStream.\n> > > testExposureGainChangeOnSameFrame\n> > > testFramePreciseExposureChange\n> > > testFramePreciseGainChange\n> > > testExposureGainIsAppliedOnFirstFrame\n> > > testExposureGainFromFirstRequestGetsApplied\n> > > testExposureGainFromFirstAndSecondRequestGetsApplied\n> > >\n> > > And we already have\n> > >\n> > > CaptureTests/SingleStream.\n> > > Capture/Raw_1 # GetParam() = (Raw, 1)\n> > > Capture/Raw_2 # GetParam() = (Raw, 2)\n> > > Capture/Raw_3 # GetParam() = (Raw, 3)\n> > > Capture/Raw_5 # GetParam() = (Raw, 5)\n> > > Capture/Raw_8 # GetParam() = (Raw, 8)\n> > > Capture/Raw_13 # GetParam() = (Raw, 13)\n> > > Capture/Raw_21 # GetParam() = (Raw, 21)\n> > > Capture/Raw_34 # GetParam() = (Raw, 34)\n> > > Capture/Raw_55 # GetParam() = (Raw, 55)\n> > > Capture/Raw_89 # GetParam() = (Raw, 89)\n> > >\n> > > I would have not instantiated these tests in capture_test.cpp but\n> > > directly in per_frame_control.cpp and I would have named them\n> > > \"PerFrameControl\". To do so you need to define a test class that\n> > > derives from testing::Test in per_frame_control.cpp\n> > >\n> > >\n> > > +/*\n> > > + * Test Per frame controls\n> > > + */\n> > > +\n> > > +class PerFrameControlTest : public testing::Test\n> > > +{\n> > > +protected:\n> > > + void SetUp() override;\n> > > + void TearDown() override;\n> > > +\n> > > + std::shared_ptr camera_;\n> > > +};\n> > > +\n> > > +void PerFrameControlTest::SetUp()\n> > > +{\n> > > + Environment *env = Environment::get();\n> > > +\n> > > + camera_ = env->cm()->get(env->cameraId());\n> > > +\n> > > + ASSERT_EQ(camera_->acquire(), 0);\n> > > +}\n> > > +\n> > > +void PerFrameControlTest::TearDown()\n> > > +{\n> > > + if (!camera_)\n> > > + return;\n> > > +\n> > > + camera_->release();\n> > > + camera_.reset();\n> > > +}\n> > > +\n> > > +TEST_F(PerFrameControlTest, testExposureGainChangeOnSameFrame)\n> > > .....\n> > >\n> > > With this you get a dedicated test suite\n> > >\n> > > PerFrameControlTest.\n> > > testExposureGainChangeOnSameFrame\n> > > testFramePreciseExposureChange\n> > > testFramePreciseGainChange\n> > > testExposureGainIsAppliedOnFirstFrame\n> > > testExposureGainFromFirstRequestGetsApplied\n> > > testExposureGainFromFirstAndSecondRequestGetsApplied\n> > >\n> > > Also, you now can drop the\n> > >\n> > > \tvoid testExposureGainChangeOnSameFrame();\n> > > \tvoid testFramePreciseExposureChange();\n> > > \tvoid testFramePreciseGainChange();\n> > > \tvoid testExposureGainIsAppliedOnFirstFrame();\n> > > \tvoid testExposureGainFromFirstRequestGetsApplied();\n> > > \tvoid testExposureGainFromFirstAndSecondRequestGetsApplied();\n> > >\n> > > functions from the PerFrameControl class, and implement the tests in\n> > > the test definition instead of having them as wrappers that call the\n> > > PerFrameControl class' functions\n> > >\n> > > TL;DR do this:\n> > >\n> > > TEST_F(PerFrameControlTest, testExposureGainChangeOnSameFrame)\n> > > {\n> > > \tPerFrameControls capture(camera_);\n> > > \tcapture.configure(StreamRole::VideoRecording);\n> > >\n> > > \tControlList startValues;\n> > > \tstartValues.set(controls::ExposureTime, 5000);\n> > > \tstartValues.set(controls::AnalogueGain, 1.0);\n> > >\n> > > \tauto timeSheet = capture.startCaptureWithTimeSheet(10, &startValues);\n> > > \tauto &ts = *timeSheet;\n> > >\n> > > \t/* wait a few frames to settle */\n> > > \tts[7].controls().set(controls::ExposureTime, 10000);\n> > > \tts[7].controls().set(controls::AnalogueGain, 4.0);\n> > >\n> > > ...\n> > > }\n> > >\n> > >\n> > > in place of:\n> > >\n> > > TEST_F(PerFrameControlTest, testFramePreciseExposureChange)\n> > > {\n> > > \tPerFrameControls capture(camera_);\n> > > \tcapture.configure(StreamRole::VideoRecording);\n> > > \tcapture.testFramePreciseExposureChange();\n> > > }\n> > >\n> >\n> > Great. Thanks for that. I didn't want to spend too much time inside\n> > google test before getting feedback on the overall direction. Your\n> > proposal is perfect. I'll do that.\n> >\n> > >\n> > >\n> > > > diff --git a/src/apps/lc-compliance/meson.build b/src/apps/lc-compliance/meson.build\n> > > > index eb7b2d71..2a6f52af 100644\n> > > > --- a/src/apps/lc-compliance/meson.build\n> > > > +++ b/src/apps/lc-compliance/meson.build\n> > > > @@ -15,6 +15,7 @@ lc_compliance_sources = files([\n> > > > 'capture_test.cpp',\n> > > > 'environment.cpp',\n> > > > 'main.cpp',\n> > > > + 'per_frame_controls.cpp',\n> > > > 'simple_capture.cpp',\n> > > > 'time_sheet.cpp',\n> > > > ])\n> > > > diff --git a/src/apps/lc-compliance/per_frame_controls.cpp b/src/apps/lc-compliance/per_frame_controls.cpp\n> > > > new file mode 100644\n> > > > index 00000000..eb7164e0\n> > > > --- /dev/null\n> > > > +++ b/src/apps/lc-compliance/per_frame_controls.cpp\n> > > > @@ -0,0 +1,316 @@\n> > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2024, Ideas on Board Oy\n> > > > + *\n> > > > + * per_frame_controls.cpp - Tests for per frame controls\n> > > > + */\n> > > > +#include \"per_frame_controls.h\"\n> > > > +\n> > > > +#include \n> > > > +\n> > > > +#include \"time_sheet.h\"\n> > > > +\n> > > > +using namespace libcamera;\n> > > > +\n> > > > +static const bool doImageTests = true;\n> > > > +\n> > > > +PerFrameControls::PerFrameControls(std::shared_ptr camera)\n> > > > +\t: SimpleCapture(camera)\n> > > > +{\n> > > > +}\n> > > > +\n> > > > +std::shared_ptr\n> > > > +PerFrameControls::startCaptureWithTimeSheet(unsigned int framesToCapture, const ControlList *controls)\n> > > > +{\n> > > > +\tControlList ctrls(camera_->controls().idmap());\n> > >\n> > > Empty line please\n> > >\n> > > > +\t/* Ensure defined default values */\n> > > > +\tctrls.set(controls::AeEnable, false);\n> > > > +\tctrls.set(controls::AeExposureMode, controls::ExposureCustom);\n> > > > +\tctrls.set(controls::ExposureTime, 10000);\n> > > > +\tctrls.set(controls::AnalogueGain, 1.0);\n> > > > +\n> > > > +\tif (controls)\n> > > > +\t\tctrls.merge(*controls, ControlList::MergePolicy::OverwriteExisting);\n> > > > +\n> > > > +\tstart(&ctrls);\n> > > > +\n> > > > +\tqueueCount_ = 0;\n> > > > +\tcaptureCount_ = 0;\n> > > > +\tcaptureLimit_ = framesToCapture;\n> > > > +\n> > > > +\tauto timeSheet = std::make_shared(captureLimit_, camera_->controls().idmap());\n> > > > +\ttimeSheet_ = timeSheet;\n> > >\n> > > I'm sorry but I don't see why you would use a shared_ptr<> and a weak\n> > > reference when the timeSheet_ ownership is not shared with any other\n> > > component outside of this class\n> >\n> > This class is still based on the Capture class. So you are free to write\n> > tests and capture without a timesheet. In such tests the weak ptr will\n> > automatically be empty and the queueRequest and requestComplete\n> > functions still work properly. The lifetime of the timesheet is bound to\n> > the scope of the test function. (You could even use the\n> > startCaptureWithTimeSheet() and ignore the result, in wich case the\n> > timesheet would get destroyed immediately).\n> >\n> \n> Let me re-cap (I'm looking at v3)\n> \n> The PerFrameControlsCapture class:\n> \n> - has a member std::weak_ptr timeSheet_\n> - has a\n> std::shared_ptr\n> \tstartCaptureWithTimeSheet(unsigned int framesToCapture,\n> \t\t\t\t const libcamera::ControlList *controls = nullptr);\n> \n> function that\n> \n> * creates a TimeSheet as shared_ptr<> and returns it to the caller\n> * Initialize the weak_ref<> class member with the shared_ptr<>\n> \n> \tauto timeSheet = std::make_shared(captureLimit_,\n> \t\t\t\t\t\t camera_->controls().idmap());\n> \ttimeSheet_ = timeSheet;\n> \treturn timeSheet;\n> \n> so, if I read it right, you create a shared_ptr<> (refcount = 1),\n> initialize a weak_ptr<> (no reference count increment) return a\n> shared_ptr<> by copy (refcount = 2) and the end of the function scope\n> the local shared_ptr<> is destroyed (refcount = 1). Now the pointer\n> ownership is on the caller only, so you basically trasferred the\n> ownership outside of the class and refers to that within the class\n> with a weak_ptr<>.\n\nThat's was exactly the plan. The only reason for the weak_ptr to exist\nat all, is to be able to access a potential timesheet inside requestComplete\nand queueRequest.\n\n> \n> In facts\n> \n> --- a/src/apps/lc-compliance/per_frame_controls_test.cpp\n> +++ b/src/apps/lc-compliance/per_frame_controls_test.cpp\n> @@ -157,6 +157,8 @@ TEST_F(PerFrameControlTests, testExposureGainChangeOnSameFrame)\n> startValues.set(controls::AnalogueGain, 1.0);\n> \n> auto timeSheet = capture.startCaptureWithTimeSheet(10, &startValues);\n> + std::cerr << \" TIMESHEET USE COUNT: \" << timeSheet.use_count();\n> +\n> auto &ts = *timeSheet;\n> \n> Gives me\n> TIMESHEET USE COUNT: 1\n> \n> if ever the caller does\n> \n> {\n> ts = capture.startCaptureWithTimeSheet(10, &startValues);\n> }\n> \n> The instance of the TimeSheet class the timeSheet_ member refers to gets\n> destroyed. It's 'safe' as it's a weak_ptr<> but I don't think that's what you\n> want.\n\nNo, that was intended. As the lifetime of the timesheet shall be bound\nto the test function and not to the lifetime of the\nPerFrameControlsCapture instance.\n\nBut I think I understand what bothers you. And thinking about that I\ncame to a solution that might be easier to follow. What about the\nfollowing proposal (not compiled though):\n\n- startCaptureWithTimsheet becomes a simple startCapture\n- timesheet in instanciated in the test and passed as pointer to\n runCaptureSession.\n- that also removes the nasty \"auto &ts = *timesheet\" inside the tests-\n\n\ndiff --git a/src/apps/lc-compliance/per_frame_controls_test.cpp b/src/apps/lc-compliance/per_frame_controls_test.cpp\nindex 589ef517..d98ab3a7 100644\n--- a/src/apps/lc-compliance/per_frame_controls_test.cpp\n+++ b/src/apps/lc-compliance/per_frame_controls_test.cpp\n@@ -48,11 +48,10 @@ class PerFrameControlsCapture : public SimpleCapture\n public:\n PerFrameControlsCapture(std::shared_ptr camera);\n \n- std::shared_ptr\n- startCaptureWithTimeSheet(unsigned int framesToCapture,\n+ void startCapture(unsigned int framesToCapture,\n const libcamera::ControlList *controls = nullptr);\n \n- void runCaptureSession();\n+ void runCaptureSession(TimeSheet* ts);\n int queueRequest(libcamera::Request *request);\n void requestComplete(libcamera::Request *request) override;\n \n@@ -60,7 +59,7 @@ public:\n unsigned int captureCount_;\n unsigned int captureLimit_;\n \n- std::weak_ptr timeSheet_;\n+ TimeSheet* sessionTimeSheet_;\n };\n \n static const bool doImageTests = true;\n@@ -70,8 +69,7 @@ PerFrameControlsCapture::PerFrameControlsCapture(std::shared_ptr camera)\n {\n }\n \n-std::shared_ptr\n-PerFrameControlsCapture::startCaptureWithTimeSheet(unsigned int framesToCapture,\n+void PerFrameControlsCapture::startCapture(unsigned int framesToCapture,\n const ControlList *controls)\n {\n ControlList ctrls(camera_->controls().idmap());\n@@ -90,10 +88,6 @@ PerFrameControlsCapture::startCaptureWithTimeSheet(unsigned int framesToCapture,\n queueCount_ = 0;\n captureCount_ = 0;\n captureLimit_ = framesToCapture;\n-\n- auto timeSheet = std::make_shared(captureLimit_, camera_);\n- timeSheet_ = timeSheet;\n- return timeSheet;\n }\n \n int PerFrameControlsCapture::queueRequest(Request *request)\n@@ -102,18 +96,16 @@ int PerFrameControlsCapture::queueRequest(Request *request)\n if (queueCount_ > captureLimit_)\n return 0;\n \n- auto ts = timeSheet_.lock();\n- if (ts)\n- ts->prepareForQueue(request, queueCount_ - 1);\n+ if (sessionTimeSheet_)\n+ sessionTimeSheet_->prepareForQueue(request, queueCount_ - 1);\n \n return camera_->queueRequest(request);\n }\n \n void PerFrameControlsCapture::requestComplete(Request *request)\n {\n- auto ts = timeSheet_.lock();\n- if (ts)\n- ts->handleCompleteRequest(request);\n+ if (sessionTimeSheet_)\n+ sessionTimeSheet_->handleCompleteRequest(request);\n \n captureCount_++;\n if (captureCount_ >= captureLimit_) {\n@@ -126,11 +118,13 @@ void PerFrameControlsCapture::requestComplete(Request *request)\n loop_->exit(-EINVAL);\n }\n \n-void PerFrameControlsCapture::runCaptureSession()\n+void PerFrameControlsCapture::runCaptureSession(TimeSheet* ts)\n {\n Stream *stream = config_->at(0).stream();\n const std::vector> &buffers = allocator_->buffers(stream);\n \n+ sessionTimeSheet_ = ts;\n+\n /* Queue the recommended number of requests. */\n for (const std::unique_ptr &buffer : buffers) {\n std::unique_ptr request = camera_->createRequest();\n@@ -144,6 +138,7 @@ void PerFrameControlsCapture::runCaptureSession()\n loop_->exec();\n stop();\n delete loop_;\n+ sessionTimeSheet_ = nullptr;\n }\n \n TEST_F(PerFrameControlTests, testExposureGainChangeOnSameFrame)\n@@ -155,14 +150,14 @@ TEST_F(PerFrameControlTests, testExposureGainChangeOnSameFrame)\n startValues.set(controls::ExposureTime, 5000);\n startValues.set(controls::AnalogueGain, 1.0);\n \n- auto timeSheet = capture.startCaptureWithTimeSheet(10, &startValues);\n- auto &ts = *timeSheet;\n+ capture.startCaptureWithTimeSheet(10, &startValues);\n \n+ TimeSheet ts(10);\n /* wait a few frames to settle */\n ts[7].controls().set(controls::ExposureTime, 10000);\n ts[7].controls().set(controls::AnalogueGain, 4.0);\n \n- capture.runCaptureSession();\n+ capture.runCaptureSession(&ts);\n \n ASSERT_TRUE(ts[5].metadata().contains(controls::ExposureTime.id()))\n << \"Required metadata entry is missing\";\n\n\n> \n> If you want to share ownership of the timesheet between the\n> PerFrameControlsCapture class and the caller, so that it gets\n> destroyed when the the last one of the two owners gets destroyed,\n> just use a shared_ptr<>\n> \n> --- a/src/apps/lc-compliance/per_frame_controls_test.cpp\n> +++ b/src/apps/lc-compliance/per_frame_controls_test.cpp\n> @@ -60,7 +60,7 @@ public:\n> unsigned int captureCount_;\n> unsigned int captureLimit_;\n> \n> - std::weak_ptr timeSheet_;\n> + std::shared_ptr timeSheet_;\n> };\n> \n> static const bool doImageTests = true;\n> @@ -91,10 +91,9 @@ PerFrameControlsCapture::startCaptureWithTimeSheet(unsigned int framesToCapture,\n> captureCount_ = 0;\n> captureLimit_ = framesToCapture;\n> \n> - auto timeSheet = std::make_shared(captureLimit_,\n> + timeSheet_ = std::make_shared(captureLimit_,\n> camera_->controls().idmap());\n> - timeSheet_ = timeSheet;\n> - return timeSheet;\n> + return timeSheet_;\n> }\n> \n> int PerFrameControlsCapture::queueRequest(Request *request)\n> @@ -103,18 +102,16 @@ int PerFrameControlsCapture::queueRequest(Request *request)\n> if (queueCount_ > captureLimit_)\n> return 0;\n> \n> - auto ts = timeSheet_.lock();\n> - if (ts)\n> - ts->prepareForQueue(request, queueCount_ - 1);\n> + if (timeSheet_)\n> + timeSheet_->prepareForQueue(request, queueCount_ - 1);\n> \n> return camera_->queueRequest(request);\n> }\n> \n> void PerFrameControlsCapture::requestComplete(Request *request)\n> {\n> - auto ts = timeSheet_.lock();\n> - if (ts)\n> - ts->handleCompleteRequest(request);\n> + if (timeSheet_)\n> + timeSheet_->handleCompleteRequest(request);\n> \n> captureCount_++;\n> if (captureCount_ >= captureLimit_) {\n> \n> So that now\n> \n> \tauto timeSheet = capture.startCaptureWithTimeSheet(10, &startValues);\n> \tstd::cerr << \" TIMESHEET USE COUNT: \" << timeSheet.use_count();\n> \n> Reads as\n> TIMESHEET USE COUNT: 2\n> \n> > >\n> > > > +\treturn timeSheet;\n> > > > +}\n> > > > +\n> > > > +int PerFrameControls::queueRequest(Request *request)\n> > > > +{\n> > > > +\tqueueCount_++;\n> > > > +\tif (queueCount_ > captureLimit_)\n> > > > +\t\treturn 0;\n> > > > +\n> > > > +\tauto ts = timeSheet_.lock();\n> > > > +\tif (ts)\n> > > > +\t\tts->prepareForQueue(request, queueCount_ - 1);\n> > > > +\n> > > > +\treturn camera_->queueRequest(request);\n> > > > +}\n> > > > +\n> > > > +void PerFrameControls::requestComplete(Request *request)\n> > > > +{\n> > > > +\tauto ts = timeSheet_.lock();\n> > > > +\tif (ts)\n> > > > +\t\tts->handleCompleteRequest(request);\n> > > > +\n> > > > +\tcaptureCount_++;\n> > > > +\tif (captureCount_ >= captureLimit_) {\n> > > > +\t\tloop_->exit(0);\n> > > > +\t\treturn;\n> > > > +\t}\n> > > > +\n> > > > +\trequest->reuse(Request::ReuseBuffers);\n> > > > +\tif (queueRequest(request))\n> > > > +\t\tloop_->exit(-EINVAL);\n> > > > +}\n> > > > +\n> > > > +void PerFrameControls::runCaptureSession()\n> > > > +{\n> > > > +\tStream *stream = config_->at(0).stream();\n> > > > +\tconst std::vector> &buffers = allocator_->buffers(stream);\n> > > > +\n> > > > +\t/* Queue the recommended number of reqeuests. */\n> > >\n> > > s/reqeuests/requests/\n> > >\n> > > > +\tfor (const std::unique_ptr &buffer : buffers) {\n> > > > +\t\tstd::unique_ptr request = camera_->createRequest();\n> > > > +\t\trequest->addBuffer(stream, buffer.get());\n> > > > +\t\tqueueRequest(request.get());\n> > > > +\t\trequests_.push_back(std::move(request));\n> > > > +\t}\n> > > > +\n> > > > +\t/* Run capture session. */\n> > > > +\tloop_ = new EventLoop();\n> > > > +\tloop_->exec();\n> > > > +\tstop();\n> > > > +\tdelete loop_;\n> > > > +}\n> > > > +\n> > > > +void PerFrameControls::testExposureGainChangeOnSameFrame()\n> > > > +{\n> > > > +\tControlList startValues;\n> > > > +\tstartValues.set(controls::ExposureTime, 5000);\n> > > > +\tstartValues.set(controls::AnalogueGain, 1.0);\n> > > > +\n> > > > +\tauto timeSheet = startCaptureWithTimeSheet(10, &startValues);\n> > > > +\tauto &ts = *timeSheet;\n> > >\n> > > Why a temporary reference ?\n> >\n> > The shared_ptr keeps the timesheet alive. The reference is just\n> \n> It's a reference, so it doesn't increment the usage count\n> \n> > syntactic shugar to be able to write ts[x]. I could replace these with\n> > (*ts)[x] or ts->get(x) if you like that better.\n> >\n> \n> Ah ok, seeing a reference just to be able to \"ts[]\" brings more\n> questions on why the reference is there than clarity imho. Up to you.\n\nThats now solved by my proposal above :-)\n\n> \n> > >\n> > > > +\n> > > > +\t/* wait a few frames to settle */\n> > > > +\tts[7].controls().set(controls::ExposureTime, 10000);\n> > > > +\tts[7].controls().set(controls::AnalogueGain, 4.0);\n> > > > +\n> > > > +\trunCaptureSession();\n> > > > +\n> > > > +\t/* Uncomment this to debug the test */\n> > > > +\t/* ts.printAllInfos(); */\n> > >\n> > > Please drop\n> > >\n> > > > +\n> > > > +\tASSERT_TRUE(ts[5].metadata().contains(controls::ExposureTime.id())) << \"Required metadata entry is missing\";\n> > > > +\tASSERT_TRUE(ts[5].metadata().contains(controls::AnalogueGain.id())) << \"Required metadata entry is missing\";\n> > >\n> > > Break long lines when possible\n> > >\n> > > > +\n> > > > +\tEXPECT_NEAR(ts[3].metadata().get(controls::ExposureTime).value(), 5000, 20);\n> > > > +\tEXPECT_NEAR(ts[3].metadata().get(controls::AnalogueGain).value(), 1.0, 0.05);\n> > > > +\n> > > > +\t//find the frame with the changes\n> > >\n> > > No C++ comments please\n> > >\n> > > > +\tint exposureChangeIndex = 0;\n> > > > +\tfor (unsigned i = 3; i < ts.size(); i++) {\n> > > > +\t\tif (ts[i].metadata().get(controls::ExposureTime).value() > 7500) {\n> > > > +\t\t\texposureChangeIndex = i;\n> > > > +\t\t\tbreak;\n> > > > +\t\t}\n> > > > +\t}\n> > > > +\n> > > > +\tint gainChangeIndex = 0;\n> > > > +\tfor (unsigned i = 3; i < ts.size(); i++) {\n> > > > +\t\tif (ts[i].metadata().get(controls::AnalogueGain).value() > 2.0) {\n> > > > +\t\t\tgainChangeIndex = i;\n> > > > +\t\t\tbreak;\n> > > > +\t\t}\n> > > > +\t}\n> > > > +\n> > > > +\tEXPECT_NE(exposureChangeIndex, 0) << \"Exposure change not found in metadata\";\n> > > > +\tEXPECT_NE(gainChangeIndex, 0) << \"Gain change not found in metadata\";\n> > > > +\tEXPECT_EQ(exposureChangeIndex, gainChangeIndex)\n> > > > +\t\t<< \"Metadata contained gain and exposure changes on different frames\";\n> > > > +\n> > > > +\tif (doImageTests) {\n> > >\n> > > Why do you think it should be optional ? If it has to be made optional\n> > > it should be done in a way that doesn't depend on a compile time\n> > > constant defined in the source code. Either make a series of separate\n> > > image tests or add an option to lc-compliance.\n> >\n> > The tests still have a value without the content based tests, so yes\n> > this should propably be a option to lc-compliance. We could default that\n> > to false to express the 'optional' aspect.\n> >\n> > >\n> > > > +\t\tint brightnessChangeIndex = 0;\n> > > > +\t\tfor (unsigned i = 3; i < ts.size(); i++) {\n> > >\n> > > The usage of '3' seems to be there to ignore the first three frames,\n> > > right ? If so, what about defining a constant and add a comment ?\n> > >\n> > > > +\t\t\tif (ts[i].getBrightnessChange() > 1.3) {\n> > > > +\t\t\t\tEXPECT_EQ(brightnessChangeIndex, 0)\n> > > > +\t\t\t\t\t<< \"Detected multiple frames with brightness increase (Wrong control delays?)\";\n> > > > +\n> > > > +\t\t\t\tif (!brightnessChangeIndex)\n> > > > +\t\t\t\t\tbrightnessChangeIndex = i;\n> > > > +\t\t\t}\n> > > > +\t\t}\n> > > > +\n> > > > +\t\tEXPECT_EQ(exposureChangeIndex, brightnessChangeIndex)\n> > > > +\t\t\t<< \"Exposure change and mesaured brightness change were not on same frame. \"\n> > > > +\t\t\t<< \"(Wrong control delay?, Start frame event too late?)\";\n> > > > +\t\tEXPECT_EQ(exposureChangeIndex, gainChangeIndex)\n> > > > +\t\t\t<< \"Gain change and mesaured brightness change were not on same frame. \"\n> > > > +\t\t\t<< \"(Wrong control delay?, Start frame event too late?)\";\n> > > > +\t}\n> > > > +}\n> > > > +\n> > > > +void PerFrameControls::testFramePreciseExposureChange()\n> > > > +{\n> > > > +\tauto timeSheet = startCaptureWithTimeSheet(10);\n> > > > +\tauto &ts = *timeSheet;\n> > > > +\n> > > > +\tts[3].controls().set(controls::ExposureTime, 5000);\n> > > > +\t/* wait a few frames to settle */\n> > > > +\tts[6].controls().set(controls::ExposureTime, 20000);\n> > > > +\n> > > > +\trunCaptureSession();\n> > > > +\n> > > > +\t/* Uncomment this to debug the test */\n> > > > +\t/* ts.printAllInfos(); */\n> > >\n> > > ditto\n> > >\n> > > > +\n> > > > +\tASSERT_TRUE(ts[5].metadata().contains(controls::ExposureTime.id())) << \"Required metadata entry is missing\";\n> > >\n> > > break long lines\n> > >\n> > > > +\n> > > > +\tEXPECT_NEAR(ts[5].metadata().get(controls::ExposureTime).value(), 5000, 20);\n> > > > +\tEXPECT_NEAR(ts[6].metadata().get(controls::ExposureTime).value(), 20000, 20);\n> > > > +\n> > > > +\tif (doImageTests) {\n> > > > +\t\t/* No increase just before setting exposure */\n> > > > +\t\tEXPECT_NEAR(ts[5].getBrightnessChange(), 1.0, 0.05)\n> > > > +\t\t\t<< \"Brightness changed too much before the expected time of change (control delay too high?).\";\n> > > > +\t\t/*\n> > > > +\t\t* Todo: The change is brightness was a bit low\n> > >\n> > > Wrong alignment\n> > >\n> > > We don't Doxygen lc-compliance but try to use \\todo for consistency\n> > > with the rest of the code base\n> > >\n> > > > +\t\t* (Exposure time increase by 4x resulted in a brightness increase of < 2).\n> > > > +\t\t* This should be investigated.\n> > >\n> > > Might be platform specific issue ?\n> > >\n> > > > +\t\t*/\n> > > > +\t\tEXPECT_GT(ts[6].getBrightnessChange(), 1.3)\n> > > > +\t\t\t<< \"Brightness in frame \" << 6 << \" did not increase as expected (reference: \"\n> > > > +\t\t\t<< ts[3].getSpotBrightness() << \" current: \" << ts[6].getSpotBrightness() << \" )\" << std::endl;\n> > > > +\n> > > > +\t\t/* No increase just after setting exposure */\n> > > > +\t\tEXPECT_NEAR(ts[7].getBrightnessChange(), 1.0, 0.05)\n> > > > +\t\t\t<< \"Brightness changed too much after the expected time of change (control delay too low?).\";\n> > > > +\n> > > > +\t\t/* No increase just after setting exposure */\n> > > > +\t\tEXPECT_NEAR(ts[8].getBrightnessChange(), 1.0, 0.05)\n> > > > +\t\t\t<< \"Brightness changed too much 2 frames after the expected time of change (control delay too low?).\";\n> > > > +\t}\n> > > > +}\n> > > > +\n> > > > +void PerFrameControls::testFramePreciseGainChange()\n> > > > +{\n> > > > +\tauto timeSheet = startCaptureWithTimeSheet(10);\n> > > > +\tauto &ts = *timeSheet;\n> > > > +\n> > > > +\tts[3].controls().set(controls::AnalogueGain, 1.0);\n> > > > +\t/* wait a few frames to settle */\n> > > > +\tts[6].controls().set(controls::AnalogueGain, 4.0);\n> > > > +\n> > > > +\trunCaptureSession();\n> > > > +\n> > > > +\t/* Uncomment this, to debug the test */\n> > > > +\t/* ts.printAllInfos(); */\n> > > > +\n> > > > +\tASSERT_TRUE(ts[5].metadata().contains(controls::AnalogueGain.id())) << \"Required metadata entry is missing\";\n> > >\n> > > Break this long line\n> > >\n> > > > +\n> > > > +\tEXPECT_NEAR(ts[5].metadata().get(controls::AnalogueGain).value(), 1.0, 0.1);\n> > > > +\tEXPECT_NEAR(ts[6].metadata().get(controls::AnalogueGain).value(), 4.0, 0.1);\n> > > > +\n> > > > +\tif (doImageTests) {\n> > > > +\t\t/* No increase just before setting gain */\n> > > > +\t\tEXPECT_NEAR(ts[5].getBrightnessChange(), 1.0, 0.05)\n> > > > +\t\t\t<< \"Brightness changed too much before the expected time of change (control delay too high?).\";\n> > > > +\t\t/*\n> > > > +\t\t* Todo: I see a brightness change of roughly half the expected one.\n> > >\n> > > Wrong alignment here too, also \\todo\n> > >\n> > > > +\t\t* This is not yet understood and needs investigation\n> > >\n> > > Defintely some platform specific thing to investigate then ?\n> > >\n> > > > +\t\t*/\n> > > > +\t\tEXPECT_GT(ts[6].getBrightnessChange(), 1.7)\n> > > > +\t\t\t<< \"Brightness in frame \" << 6 << \" did not increase as expected (reference: \"\n> > > > +\t\t\t<< ts[5].getSpotBrightness() << \" current: \" << ts[6].getSpotBrightness() << \" )\" << std::endl;\n> > > > +\n> > > > +\t\t/* No increase just after setting gain */\n> > > > +\t\tEXPECT_NEAR(ts[7].getBrightnessChange(), 1.0, 0.05)\n> > > > +\t\t\t<< \"Brightness changed too much after the expected time of change (control delay too low?).\";\n> > > > +\n> > > > +\t\t/* No increase just after setting gain */\n> > > > +\t\tEXPECT_NEAR(ts[8].getBrightnessChange(), 1.0, 0.05)\n> > > > +\t\t\t<< \"Brightness changed too much after the expected time of change (control delay too low?).\";\n> > > > +\t}\n> > > > +}\n> > > > +\n> > > > +void PerFrameControls::testExposureGainFromFirstRequestGetsApplied()\n> > > > +{\n> > > > +\tauto timeSheet = startCaptureWithTimeSheet(5);\n> > > > +\tauto &ts = *timeSheet;\n> > > > +\n> > > > +\tts[0].controls().set(controls::ExposureTime, 10000);\n> > > > +\tts[0].controls().set(controls::AnalogueGain, 4.0);\n> > > > +\n> > > > +\trunCaptureSession();\n> > > > +\n> > > > +\tASSERT_TRUE(ts[4].metadata().contains(controls::ExposureTime.id())) << \"Required metadata entry is missing\";\n> > > > +\tASSERT_TRUE(ts[4].metadata().contains(controls::AnalogueGain.id())) << \"Required metadata entry is missing\";\n> > >\n> > > You can easily break these lines\n> > >\n> > > > +\n> > > > +\t/* We expect it to be applied after 3 frames, the latest*/\n> > >\n> > > What if a sensor takes a longer time to apply exposure and gain ?\n> >\n> > Then we should adjust the test :-)\n> >\n> > >\n> > > > +\tEXPECT_NEAR(ts[4].metadata().get(controls::ExposureTime).value(), 10000, 20);\n> > > > +\tEXPECT_NEAR(ts[4].metadata().get(controls::AnalogueGain).value(), 4.0, 0.1);\n> > > > +}\n> > > > +\n> > > > +void PerFrameControls::testExposureGainFromFirstAndSecondRequestGetsApplied()\n> > > > +{\n> > > > +\tauto timeSheet = startCaptureWithTimeSheet(5);\n> > > > +\tauto &ts = *timeSheet;\n> > > > +\n> > > > +\tts[0].controls().set(controls::ExposureTime, 8000);\n> > > > +\tts[0].controls().set(controls::AnalogueGain, 2.0);\n> > > > +\tts[1].controls().set(controls::ExposureTime, 10000);\n> > > > +\tts[1].controls().set(controls::AnalogueGain, 4.0);\n> > >\n> > > As a general question, how do we guarantee the values you use to set\n> > > exposure and gains are valid for all possible sensors ?\n> > >\n> > > Shouldn't you inspect inspect the ControlInfo limit from\n> > > Camera::controls() and clamp the values in the min/max range ?\n> >\n> > I thought about that too. I started with values that most if not all\n> > sensor we care atm should support. I'm weary if it is worth the effort\n> > and that it would make the tests way less readable. But you are right,\n> > that is an unsolved issue.\n> \n> Recording it with a \\todo comment is fine for now\n> \n> >\n> > >\n> > > > +\n> > > > +\trunCaptureSession();\n> > > > +\n> > > > +\tASSERT_TRUE(ts[4].metadata().contains(controls::ExposureTime.id())) << \"Required metadata entry is missing\";\n> > > > +\tASSERT_TRUE(ts[4].metadata().contains(controls::AnalogueGain.id())) << \"Required metadata entry is missing\";\n> > >\n> > > Ditto\n> > >\n> > > > +\n> > > > +\t/* We expect it to be applied after 3 frames, the latest */\n> > > > +\tEXPECT_NEAR(ts[4].metadata().get(controls::ExposureTime).value(), 10000, 20);\n> > > > +\tEXPECT_NEAR(ts[4].metadata().get(controls::AnalogueGain).value(), 4.0, 0.1);\n> > > > +}\n> > > > +\n> > > > +void PerFrameControls::testExposureGainIsAppliedOnFirstFrame()\n> > > > +{\n> > > > +\tControlList startValues;\n> > > > +\tstartValues.set(controls::ExposureTime, 5000);\n> > > > +\tstartValues.set(controls::AnalogueGain, 1.0);\n> > > > +\n> > > > +\tauto ts1 = startCaptureWithTimeSheet(3, &startValues);\n> > > > +\n> > > > +\trunCaptureSession();\n> > > > +\n> > > > +\tASSERT_TRUE((*ts1)[0].metadata().contains(controls::ExposureTime.id())) << \"Required metadata entry is missing\";\n> > > > +\tASSERT_TRUE((*ts1)[0].metadata().contains(controls::AnalogueGain.id())) << \"Required metadata entry is missing\";\n> > > > +\n> > > > +\tEXPECT_NEAR((*ts1)[0].metadata().get(controls::ExposureTime).value(), 5000, 20);\n> > > > +\tEXPECT_NEAR((*ts1)[0].metadata().get(controls::AnalogueGain).value(), 1.0, 0.02);\n> > > > +\n> > > > +\t/* Second capture with different values to ensure we don't hit default/old values */\n> > > > +\tstartValues.set(controls::ExposureTime, 15000);\n> > > > +\tstartValues.set(controls::AnalogueGain, 4.0);\n> > > > +\n> > > > +\tauto ts2 = startCaptureWithTimeSheet(3, &startValues);\n> > > > +\n> > > > +\trunCaptureSession();\n> > > > +\n> > > > +\tEXPECT_NEAR((*ts2)[0].metadata().get(controls::ExposureTime).value(), 15000, 20);\n> > > > +\tEXPECT_NEAR((*ts2)[0].metadata().get(controls::AnalogueGain).value(), 4.0, 0.02);\n> > > > +\n> > > > +\tif (doImageTests) {\n> > > > +\t\t/* With 3x exposure and 4x gain we could expect a brightness increase of 2x */\n> > > > +\t\tdouble brightnessChange = ts2->get(1).getSpotBrightness() / ts1->get(1).getSpotBrightness();\n> > > > +\t\tEXPECT_GT(brightnessChange, 2.0);\n> > > > +\t}\n> > > > +}\n> > > > diff --git a/src/apps/lc-compliance/per_frame_controls.h b/src/apps/lc-compliance/per_frame_controls.h\n> > > > new file mode 100644\n> > > > index 00000000..a341c61f\n> > > > --- /dev/null\n> > > > +++ b/src/apps/lc-compliance/per_frame_controls.h\n> > > > @@ -0,0 +1,43 @@\n> > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2024, Ideas on Board Oy\n> > > > + *\n> > > > + * per_frame_controls.h - Tests for per frame controls\n> > > > + */\n> > > > +\n> > > > +#pragma once\n> > > > +\n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +\n> > > > +#include \"../common/event_loop.h\"\n> > > > +\n> > > > +#include \"simple_capture.h\"\n> > > > +#include \"time_sheet.h\"\n> > > > +\n> > > > +class PerFrameControls : public SimpleCapture\n> > > > +{\n> > > > +public:\n> > > > +\tPerFrameControls(std::shared_ptr camera);\n> > > > +\n> > > > +\tstd::shared_ptr\n> > > > +\tstartCaptureWithTimeSheet(unsigned int framesToCapture, const libcamera::ControlList *controls = nullptr);\n> > > > +\tvoid runCaptureSession();\n> > > > +\n> > > > +\tvoid testExposureGainChangeOnSameFrame();\n> > > > +\tvoid testFramePreciseExposureChange();\n> > > > +\tvoid testFramePreciseGainChange();\n> > > > +\tvoid testExposureGainIsAppliedOnFirstFrame();\n> > > > +\tvoid testExposureGainFromFirstRequestGetsApplied();\n> > > > +\tvoid testExposureGainFromFirstAndSecondRequestGetsApplied();\n> > > > +\n> > > > +\tint queueRequest(libcamera::Request *request);\n> > > > +\tvoid requestComplete(libcamera::Request *request) override;\n> > > > +\n> > > > +\tunsigned int queueCount_;\n> > > > +\tunsigned int captureCount_;\n> > > > +\tunsigned int captureLimit_;\n> > > > +\n> > > > +\tstd::weak_ptr timeSheet_;\n> > > > +};\n> > > > --\n> > > > 2.40.1\n> > > >\n> >\n> > Thanks for all the input :-)\n> >\n> > Cheers,\n> > Stefan","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id A4DA8BD160\n\tfor ;\n\tThu, 21 Mar 2024 11:34:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AF89963055;\n\tThu, 21 Mar 2024 12:34:54 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0760662827\n\tfor ;\n\tThu, 21 Mar 2024 12:34:53 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:1bc6:8c62:a44e:3d82])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A74007E9;\n\tThu, 21 Mar 2024 12:34:24 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"YRLG06oz\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1711020864;\n\tbh=Z2QOXgDySZNMN37nbap3lQ6pQ9pt/82RSXT0seGSYiQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=YRLG06ozH4Zc9t2KItsvlr70kCLYEaVhRXt/ug92+Z8FyHfcxTlp+MKAFZwN3L0DD\n\t26tZZSoZOWLzn9WuNBBb93oGPS95YDz4M/SqTAwLeyJTRsoU4AQJ/be9gPGN7V2EUD\n\t8xFEZ0tbDl0CHgdR7jw0qu5s4iOB47Mjs2T5FW+A=","Date":"Thu, 21 Mar 2024 12:34:49 +0100","From":"Stefan Klug ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 03/12] libcamera: lc-compliance: Add initial set of\n\tper-frame-control tests","Message-ID":"<20240321113449.qlolurgw53myho6l@jasper>","References":"<20240313121223.138150-1-stefan.klug@ideasonboard.com>\n\t<20240313121223.138150-4-stefan.klug@ideasonboard.com>\n\t\n\t<20240315155303.m2yfff7w2fcfydtw@jasper>\n\t<752it44re455lb25sd4et5osimvpnupxn2ayoqz75f7t3nqblk@4sfpj3xgvg7e>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<752it44re455lb25sd4et5osimvpnupxn2ayoqz75f7t3nqblk@4sfpj3xgvg7e>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29030,"web_url":"https://patchwork.libcamera.org/comment/29030/","msgid":"","date":"2024-03-21T11:56:16","subject":"Re: [PATCH v2 03/12] libcamera: lc-compliance: Add initial set of\n\tper-frame-control tests","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Stefan\n\nOn Thu, Mar 21, 2024 at 12:34:49PM +0100, Stefan Klug wrote:\n> Hi Jacopo,\n>\n> On Thu, Mar 21, 2024 at 09:47:53AM +0100, Jacopo Mondi wrote:\n> > Hi Stefan\n> >\n> > On Fri, Mar 15, 2024 at 04:53:03PM +0100, Stefan Klug wrote:\n> > > Hi Jacopo,\n> > >\n> > > thanks for the review.\n> > >\n> > > On Fri, Mar 15, 2024 at 03:42:39PM +0100, Jacopo Mondi wrote:\n> > > > Hi Stefan\n> > > >\n> > > > On Wed, Mar 13, 2024 at 01:12:14PM +0100, Stefan Klug wrote:\n> > > > > These tests check if controls (only exposure time and analogue gain at\n> > > > > the moment) get applied on the frame they were requested for.\n> > > > >\n> > > > > This is tested by looking at the metadata and the mean brightness\n> > > > > of the image center.\n> > > > >\n> > > > > At the moment these tests fail. Fixes for the pipelines will be delivered\n> > > > > in later patches.\n> > > >\n> > > > Nice!\n> > > >\n> > > > >\n> > > > > To run only the teste, one can run:\n> > > >\n> > > > s/teste/test\n> > > >\n> > > > > lc-compliance -c -f \"SingleStream.*\"\n> > > > >\n> > > > > Note that the current implementation is a bit picky on what the camera\n> > > > > actually sees. If it is too dark (or too bright), the tests will fail.\n> > > > > Looking at a white wall in a normally lit office usually works.\n> > > >\n> > > > Mmm, is this ok for a compliance suite ? Is this the reason the image\n> > > > tests are 'optional' ?\n> > >\n> > > Yes, in the beginning I wasn't sure how far we should go in first place.\n> > > You basically have two options here:\n> > > - Dynamically finding a \"base\" exposure time that works (e.g. bright\n> > > enough that you see a difference when the values increase and dim\n> > > enough to not saturate any pixel). This might work but has a large\n> > > dependency on the efficiency of the sensor and might still be flaky.\n> > > - Building and distributing a physical testrig with defined brightness.\n> > > This is cool, but a larger task.\n> > >\n> > > So I started small added this 'optional' term so that we could collect\n> > > some practical experience on how stable these tests are on different\n> > > devices.\n> > >\n> > > >\n> > > > >\n> > > > > Signed-off-by: Stefan Klug \n> > > > > ---\n> > > > > src/apps/lc-compliance/capture_test.cpp | 46 +++\n> > > > > src/apps/lc-compliance/meson.build | 1 +\n> > > > > src/apps/lc-compliance/per_frame_controls.cpp | 316 ++++++++++++++++++\n> > > > > src/apps/lc-compliance/per_frame_controls.h | 43 +++\n> > > > > 4 files changed, 406 insertions(+)\n> > > > > create mode 100644 src/apps/lc-compliance/per_frame_controls.cpp\n> > > > > create mode 100644 src/apps/lc-compliance/per_frame_controls.h\n> > > > >\n> > > > > diff --git a/src/apps/lc-compliance/capture_test.cpp b/src/apps/lc-compliance/capture_test.cpp\n> > > > > index 1dcfcf92..b19e8936 100644\n> > > > > --- a/src/apps/lc-compliance/capture_test.cpp\n> > > > > +++ b/src/apps/lc-compliance/capture_test.cpp\n> > > > > @@ -11,6 +11,7 @@\n> > > > > #include \n> > > > >\n> > > > > #include \"environment.h\"\n> > > > > +#include \"per_frame_controls.h\"\n> > > > > #include \"simple_capture.h\"\n> > > > >\n> > > > > using namespace libcamera;\n> > > > > @@ -133,3 +134,48 @@ INSTANTIATE_TEST_SUITE_P(CaptureTests,\n> > > > > \t\t\t testing::Combine(testing::ValuesIn(ROLES),\n> > > > > \t\t\t\t\t testing::ValuesIn(NUMREQUESTS)),\n> > > > > \t\t\t SingleStream::nameParameters);\n> > > > > +\n> > > > > +/*\n> > > > > + * Test Per frame controls\n> > > > > + */\n> > > > > +TEST_F(SingleStream, testExposureGainChangeOnSameFrame)\n> > > > > +{\n> > > > > +\tPerFrameControls capture(camera_);\n> > > > > +\tcapture.configure(StreamRole::VideoRecording);\n> > > > > +\tcapture.testExposureGainChangeOnSameFrame();\n> > > > > +}\n> > > > > +\n> > > > > +TEST_F(SingleStream, testFramePreciseExposureChange)\n> > > > > +{\n> > > > > +\tPerFrameControls capture(camera_);\n> > > > > +\tcapture.configure(StreamRole::VideoRecording);\n> > > > > +\tcapture.testFramePreciseExposureChange();\n> > > > > +}\n> > > > > +\n> > > > > +TEST_F(SingleStream, testFramePreciseGainChange)\n> > > > > +{\n> > > > > +\tPerFrameControls capture(camera_);\n> > > > > +\tcapture.configure(StreamRole::VideoRecording);\n> > > > > +\tcapture.testFramePreciseGainChange();\n> > > > > +}\n> > > > > +\n> > > > > +TEST_F(SingleStream, testExposureGainIsAppliedOnFirstFrame)\n> > > > > +{\n> > > > > +\tPerFrameControls capture(camera_);\n> > > > > +\tcapture.configure(StreamRole::VideoRecording);\n> > > > > +\tcapture.testExposureGainIsAppliedOnFirstFrame();\n> > > > > +}\n> > > > > +\n> > > > > +TEST_F(SingleStream, testExposureGainFromFirstRequestGetsApplied)\n> > > > > +{\n> > > > > +\tPerFrameControls capture(camera_);\n> > > > > +\tcapture.configure(StreamRole::VideoRecording);\n> > > > > +\tcapture.testExposureGainFromFirstRequestGetsApplied();\n> > > > > +}\n> > > > > +\n> > > > > +TEST_F(SingleStream, testExposureGainFromFirstAndSecondRequestGetsApplied)\n> > > > > +{\n> > > > > +\tPerFrameControls capture(camera_);\n> > > > > +\tcapture.configure(StreamRole::VideoRecording);\n> > > > > +\tcapture.testExposureGainFromFirstAndSecondRequestGetsApplied();\n> > > > > +}\n> > > >\n> > > > This now shows up as\n> > > >\n> > > > SingleStream.\n> > > > testExposureGainChangeOnSameFrame\n> > > > testFramePreciseExposureChange\n> > > > testFramePreciseGainChange\n> > > > testExposureGainIsAppliedOnFirstFrame\n> > > > testExposureGainFromFirstRequestGetsApplied\n> > > > testExposureGainFromFirstAndSecondRequestGetsApplied\n> > > >\n> > > > And we already have\n> > > >\n> > > > CaptureTests/SingleStream.\n> > > > Capture/Raw_1 # GetParam() = (Raw, 1)\n> > > > Capture/Raw_2 # GetParam() = (Raw, 2)\n> > > > Capture/Raw_3 # GetParam() = (Raw, 3)\n> > > > Capture/Raw_5 # GetParam() = (Raw, 5)\n> > > > Capture/Raw_8 # GetParam() = (Raw, 8)\n> > > > Capture/Raw_13 # GetParam() = (Raw, 13)\n> > > > Capture/Raw_21 # GetParam() = (Raw, 21)\n> > > > Capture/Raw_34 # GetParam() = (Raw, 34)\n> > > > Capture/Raw_55 # GetParam() = (Raw, 55)\n> > > > Capture/Raw_89 # GetParam() = (Raw, 89)\n> > > >\n> > > > I would have not instantiated these tests in capture_test.cpp but\n> > > > directly in per_frame_control.cpp and I would have named them\n> > > > \"PerFrameControl\". To do so you need to define a test class that\n> > > > derives from testing::Test in per_frame_control.cpp\n> > > >\n> > > >\n> > > > +/*\n> > > > + * Test Per frame controls\n> > > > + */\n> > > > +\n> > > > +class PerFrameControlTest : public testing::Test\n> > > > +{\n> > > > +protected:\n> > > > + void SetUp() override;\n> > > > + void TearDown() override;\n> > > > +\n> > > > + std::shared_ptr camera_;\n> > > > +};\n> > > > +\n> > > > +void PerFrameControlTest::SetUp()\n> > > > +{\n> > > > + Environment *env = Environment::get();\n> > > > +\n> > > > + camera_ = env->cm()->get(env->cameraId());\n> > > > +\n> > > > + ASSERT_EQ(camera_->acquire(), 0);\n> > > > +}\n> > > > +\n> > > > +void PerFrameControlTest::TearDown()\n> > > > +{\n> > > > + if (!camera_)\n> > > > + return;\n> > > > +\n> > > > + camera_->release();\n> > > > + camera_.reset();\n> > > > +}\n> > > > +\n> > > > +TEST_F(PerFrameControlTest, testExposureGainChangeOnSameFrame)\n> > > > .....\n> > > >\n> > > > With this you get a dedicated test suite\n> > > >\n> > > > PerFrameControlTest.\n> > > > testExposureGainChangeOnSameFrame\n> > > > testFramePreciseExposureChange\n> > > > testFramePreciseGainChange\n> > > > testExposureGainIsAppliedOnFirstFrame\n> > > > testExposureGainFromFirstRequestGetsApplied\n> > > > testExposureGainFromFirstAndSecondRequestGetsApplied\n> > > >\n> > > > Also, you now can drop the\n> > > >\n> > > > \tvoid testExposureGainChangeOnSameFrame();\n> > > > \tvoid testFramePreciseExposureChange();\n> > > > \tvoid testFramePreciseGainChange();\n> > > > \tvoid testExposureGainIsAppliedOnFirstFrame();\n> > > > \tvoid testExposureGainFromFirstRequestGetsApplied();\n> > > > \tvoid testExposureGainFromFirstAndSecondRequestGetsApplied();\n> > > >\n> > > > functions from the PerFrameControl class, and implement the tests in\n> > > > the test definition instead of having them as wrappers that call the\n> > > > PerFrameControl class' functions\n> > > >\n> > > > TL;DR do this:\n> > > >\n> > > > TEST_F(PerFrameControlTest, testExposureGainChangeOnSameFrame)\n> > > > {\n> > > > \tPerFrameControls capture(camera_);\n> > > > \tcapture.configure(StreamRole::VideoRecording);\n> > > >\n> > > > \tControlList startValues;\n> > > > \tstartValues.set(controls::ExposureTime, 5000);\n> > > > \tstartValues.set(controls::AnalogueGain, 1.0);\n> > > >\n> > > > \tauto timeSheet = capture.startCaptureWithTimeSheet(10, &startValues);\n> > > > \tauto &ts = *timeSheet;\n> > > >\n> > > > \t/* wait a few frames to settle */\n> > > > \tts[7].controls().set(controls::ExposureTime, 10000);\n> > > > \tts[7].controls().set(controls::AnalogueGain, 4.0);\n> > > >\n> > > > ...\n> > > > }\n> > > >\n> > > >\n> > > > in place of:\n> > > >\n> > > > TEST_F(PerFrameControlTest, testFramePreciseExposureChange)\n> > > > {\n> > > > \tPerFrameControls capture(camera_);\n> > > > \tcapture.configure(StreamRole::VideoRecording);\n> > > > \tcapture.testFramePreciseExposureChange();\n> > > > }\n> > > >\n> > >\n> > > Great. Thanks for that. I didn't want to spend too much time inside\n> > > google test before getting feedback on the overall direction. Your\n> > > proposal is perfect. I'll do that.\n> > >\n> > > >\n> > > >\n> > > > > diff --git a/src/apps/lc-compliance/meson.build b/src/apps/lc-compliance/meson.build\n> > > > > index eb7b2d71..2a6f52af 100644\n> > > > > --- a/src/apps/lc-compliance/meson.build\n> > > > > +++ b/src/apps/lc-compliance/meson.build\n> > > > > @@ -15,6 +15,7 @@ lc_compliance_sources = files([\n> > > > > 'capture_test.cpp',\n> > > > > 'environment.cpp',\n> > > > > 'main.cpp',\n> > > > > + 'per_frame_controls.cpp',\n> > > > > 'simple_capture.cpp',\n> > > > > 'time_sheet.cpp',\n> > > > > ])\n> > > > > diff --git a/src/apps/lc-compliance/per_frame_controls.cpp b/src/apps/lc-compliance/per_frame_controls.cpp\n> > > > > new file mode 100644\n> > > > > index 00000000..eb7164e0\n> > > > > --- /dev/null\n> > > > > +++ b/src/apps/lc-compliance/per_frame_controls.cpp\n> > > > > @@ -0,0 +1,316 @@\n> > > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > > > +/*\n> > > > > + * Copyright (C) 2024, Ideas on Board Oy\n> > > > > + *\n> > > > > + * per_frame_controls.cpp - Tests for per frame controls\n> > > > > + */\n> > > > > +#include \"per_frame_controls.h\"\n> > > > > +\n> > > > > +#include \n> > > > > +\n> > > > > +#include \"time_sheet.h\"\n> > > > > +\n> > > > > +using namespace libcamera;\n> > > > > +\n> > > > > +static const bool doImageTests = true;\n> > > > > +\n> > > > > +PerFrameControls::PerFrameControls(std::shared_ptr camera)\n> > > > > +\t: SimpleCapture(camera)\n> > > > > +{\n> > > > > +}\n> > > > > +\n> > > > > +std::shared_ptr\n> > > > > +PerFrameControls::startCaptureWithTimeSheet(unsigned int framesToCapture, const ControlList *controls)\n> > > > > +{\n> > > > > +\tControlList ctrls(camera_->controls().idmap());\n> > > >\n> > > > Empty line please\n> > > >\n> > > > > +\t/* Ensure defined default values */\n> > > > > +\tctrls.set(controls::AeEnable, false);\n> > > > > +\tctrls.set(controls::AeExposureMode, controls::ExposureCustom);\n> > > > > +\tctrls.set(controls::ExposureTime, 10000);\n> > > > > +\tctrls.set(controls::AnalogueGain, 1.0);\n> > > > > +\n> > > > > +\tif (controls)\n> > > > > +\t\tctrls.merge(*controls, ControlList::MergePolicy::OverwriteExisting);\n> > > > > +\n> > > > > +\tstart(&ctrls);\n> > > > > +\n> > > > > +\tqueueCount_ = 0;\n> > > > > +\tcaptureCount_ = 0;\n> > > > > +\tcaptureLimit_ = framesToCapture;\n> > > > > +\n> > > > > +\tauto timeSheet = std::make_shared(captureLimit_, camera_->controls().idmap());\n> > > > > +\ttimeSheet_ = timeSheet;\n> > > >\n> > > > I'm sorry but I don't see why you would use a shared_ptr<> and a weak\n> > > > reference when the timeSheet_ ownership is not shared with any other\n> > > > component outside of this class\n> > >\n> > > This class is still based on the Capture class. So you are free to write\n> > > tests and capture without a timesheet. In such tests the weak ptr will\n> > > automatically be empty and the queueRequest and requestComplete\n> > > functions still work properly. The lifetime of the timesheet is bound to\n> > > the scope of the test function. (You could even use the\n> > > startCaptureWithTimeSheet() and ignore the result, in wich case the\n> > > timesheet would get destroyed immediately).\n> > >\n> >\n> > Let me re-cap (I'm looking at v3)\n> >\n> > The PerFrameControlsCapture class:\n> >\n> > - has a member std::weak_ptr timeSheet_\n> > - has a\n> > std::shared_ptr\n> > \tstartCaptureWithTimeSheet(unsigned int framesToCapture,\n> > \t\t\t\t const libcamera::ControlList *controls = nullptr);\n> >\n> > function that\n> >\n> > * creates a TimeSheet as shared_ptr<> and returns it to the caller\n> > * Initialize the weak_ref<> class member with the shared_ptr<>\n> >\n> > \tauto timeSheet = std::make_shared(captureLimit_,\n> > \t\t\t\t\t\t camera_->controls().idmap());\n> > \ttimeSheet_ = timeSheet;\n> > \treturn timeSheet;\n> >\n> > so, if I read it right, you create a shared_ptr<> (refcount = 1),\n> > initialize a weak_ptr<> (no reference count increment) return a\n> > shared_ptr<> by copy (refcount = 2) and the end of the function scope\n> > the local shared_ptr<> is destroyed (refcount = 1). Now the pointer\n> > ownership is on the caller only, so you basically trasferred the\n> > ownership outside of the class and refers to that within the class\n> > with a weak_ptr<>.\n>\n> That's was exactly the plan. The only reason for the weak_ptr to exist\n> at all, is to be able to access a potential timesheet inside requestComplete\n> and queueRequest.\n>\n> >\n> > In facts\n> >\n> > --- a/src/apps/lc-compliance/per_frame_controls_test.cpp\n> > +++ b/src/apps/lc-compliance/per_frame_controls_test.cpp\n> > @@ -157,6 +157,8 @@ TEST_F(PerFrameControlTests, testExposureGainChangeOnSameFrame)\n> > startValues.set(controls::AnalogueGain, 1.0);\n> >\n> > auto timeSheet = capture.startCaptureWithTimeSheet(10, &startValues);\n> > + std::cerr << \" TIMESHEET USE COUNT: \" << timeSheet.use_count();\n> > +\n> > auto &ts = *timeSheet;\n> >\n> > Gives me\n> > TIMESHEET USE COUNT: 1\n> >\n> > if ever the caller does\n> >\n> > {\n> > ts = capture.startCaptureWithTimeSheet(10, &startValues);\n> > }\n> >\n> > The instance of the TimeSheet class the timeSheet_ member refers to gets\n> > destroyed. It's 'safe' as it's a weak_ptr<> but I don't think that's what you\n> > want.\n>\n> No, that was intended. As the lifetime of the timesheet shall be bound\n> to the test function and not to the lifetime of the\n> PerFrameControlsCapture instance.\n>\n\nAh ok, it's weird than that the timesheet is created by the\nPerFrameControlsCapture class then.\n\n> But I think I understand what bothers you. And thinking about that I\n> came to a solution that might be easier to follow. What about the\n> following proposal (not compiled though):\n>\n> - startCaptureWithTimsheet becomes a simple startCapture\n> - timesheet in instanciated in the test and passed as pointer to\n> runCaptureSession.\n> - that also removes the nasty \"auto &ts = *timesheet\" inside the tests-\n>\n\nI like it ! Let's make it clear what owns what to avoid passing\nownerships around!\n\n>\n> diff --git a/src/apps/lc-compliance/per_frame_controls_test.cpp b/src/apps/lc-compliance/per_frame_controls_test.cpp\n> index 589ef517..d98ab3a7 100644\n> --- a/src/apps/lc-compliance/per_frame_controls_test.cpp\n> +++ b/src/apps/lc-compliance/per_frame_controls_test.cpp\n> @@ -48,11 +48,10 @@ class PerFrameControlsCapture : public SimpleCapture\n> public:\n> PerFrameControlsCapture(std::shared_ptr camera);\n>\n> - std::shared_ptr\n> - startCaptureWithTimeSheet(unsigned int framesToCapture,\n> + void startCapture(unsigned int framesToCapture,\n> const libcamera::ControlList *controls = nullptr);\n>\n> - void runCaptureSession();\n> + void runCaptureSession(TimeSheet* ts);\n> int queueRequest(libcamera::Request *request);\n> void requestComplete(libcamera::Request *request) override;\n>\n> @@ -60,7 +59,7 @@ public:\n> unsigned int captureCount_;\n> unsigned int captureLimit_;\n>\n> - std::weak_ptr timeSheet_;\n> + TimeSheet* sessionTimeSheet_;\n> };\n>\n> static const bool doImageTests = true;\n> @@ -70,8 +69,7 @@ PerFrameControlsCapture::PerFrameControlsCapture(std::shared_ptr camera)\n> {\n> }\n>\n> -std::shared_ptr\n> -PerFrameControlsCapture::startCaptureWithTimeSheet(unsigned int framesToCapture,\n> +void PerFrameControlsCapture::startCapture(unsigned int framesToCapture,\n> const ControlList *controls)\n> {\n> ControlList ctrls(camera_->controls().idmap());\n> @@ -90,10 +88,6 @@ PerFrameControlsCapture::startCaptureWithTimeSheet(unsigned int framesToCapture,\n> queueCount_ = 0;\n> captureCount_ = 0;\n> captureLimit_ = framesToCapture;\n> -\n> - auto timeSheet = std::make_shared(captureLimit_, camera_);\n> - timeSheet_ = timeSheet;\n> - return timeSheet;\n> }\n>\n> int PerFrameControlsCapture::queueRequest(Request *request)\n> @@ -102,18 +96,16 @@ int PerFrameControlsCapture::queueRequest(Request *request)\n> if (queueCount_ > captureLimit_)\n> return 0;\n>\n> - auto ts = timeSheet_.lock();\n> - if (ts)\n> - ts->prepareForQueue(request, queueCount_ - 1);\n> + if (sessionTimeSheet_)\n> + sessionTimeSheet_->prepareForQueue(request, queueCount_ - 1);\n>\n> return camera_->queueRequest(request);\n> }\n>\n> void PerFrameControlsCapture::requestComplete(Request *request)\n> {\n> - auto ts = timeSheet_.lock();\n> - if (ts)\n> - ts->handleCompleteRequest(request);\n> + if (sessionTimeSheet_)\n> + sessionTimeSheet_->handleCompleteRequest(request);\n>\n> captureCount_++;\n> if (captureCount_ >= captureLimit_) {\n> @@ -126,11 +118,13 @@ void PerFrameControlsCapture::requestComplete(Request *request)\n> loop_->exit(-EINVAL);\n> }\n>\n> -void PerFrameControlsCapture::runCaptureSession()\n> +void PerFrameControlsCapture::runCaptureSession(TimeSheet* ts)\n> {\n> Stream *stream = config_->at(0).stream();\n> const std::vector> &buffers = allocator_->buffers(stream);\n>\n> + sessionTimeSheet_ = ts;\n> +\n> /* Queue the recommended number of requests. */\n> for (const std::unique_ptr &buffer : buffers) {\n> std::unique_ptr request = camera_->createRequest();\n> @@ -144,6 +138,7 @@ void PerFrameControlsCapture::runCaptureSession()\n> loop_->exec();\n> stop();\n> delete loop_;\n> + sessionTimeSheet_ = nullptr;\n> }\n>\n> TEST_F(PerFrameControlTests, testExposureGainChangeOnSameFrame)\n> @@ -155,14 +150,14 @@ TEST_F(PerFrameControlTests, testExposureGainChangeOnSameFrame)\n> startValues.set(controls::ExposureTime, 5000);\n> startValues.set(controls::AnalogueGain, 1.0);\n>\n> - auto timeSheet = capture.startCaptureWithTimeSheet(10, &startValues);\n> - auto &ts = *timeSheet;\n> + capture.startCaptureWithTimeSheet(10, &startValues);\n>\n> + TimeSheet ts(10);\n> /* wait a few frames to settle */\n> ts[7].controls().set(controls::ExposureTime, 10000);\n> ts[7].controls().set(controls::AnalogueGain, 4.0);\n>\n> - capture.runCaptureSession();\n> + capture.runCaptureSession(&ts);\n>\n> ASSERT_TRUE(ts[5].metadata().contains(controls::ExposureTime.id()))\n> << \"Required metadata entry is missing\";\n>\n>\n> >\n> > If you want to share ownership of the timesheet between the\n> > PerFrameControlsCapture class and the caller, so that it gets\n> > destroyed when the the last one of the two owners gets destroyed,\n> > just use a shared_ptr<>\n> >\n> > --- a/src/apps/lc-compliance/per_frame_controls_test.cpp\n> > +++ b/src/apps/lc-compliance/per_frame_controls_test.cpp\n> > @@ -60,7 +60,7 @@ public:\n> > unsigned int captureCount_;\n> > unsigned int captureLimit_;\n> >\n> > - std::weak_ptr timeSheet_;\n> > + std::shared_ptr timeSheet_;\n> > };\n> >\n> > static const bool doImageTests = true;\n> > @@ -91,10 +91,9 @@ PerFrameControlsCapture::startCaptureWithTimeSheet(unsigned int framesToCapture,\n> > captureCount_ = 0;\n> > captureLimit_ = framesToCapture;\n> >\n> > - auto timeSheet = std::make_shared(captureLimit_,\n> > + timeSheet_ = std::make_shared(captureLimit_,\n> > camera_->controls().idmap());\n> > - timeSheet_ = timeSheet;\n> > - return timeSheet;\n> > + return timeSheet_;\n> > }\n> >\n> > int PerFrameControlsCapture::queueRequest(Request *request)\n> > @@ -103,18 +102,16 @@ int PerFrameControlsCapture::queueRequest(Request *request)\n> > if (queueCount_ > captureLimit_)\n> > return 0;\n> >\n> > - auto ts = timeSheet_.lock();\n> > - if (ts)\n> > - ts->prepareForQueue(request, queueCount_ - 1);\n> > + if (timeSheet_)\n> > + timeSheet_->prepareForQueue(request, queueCount_ - 1);\n> >\n> > return camera_->queueRequest(request);\n> > }\n> >\n> > void PerFrameControlsCapture::requestComplete(Request *request)\n> > {\n> > - auto ts = timeSheet_.lock();\n> > - if (ts)\n> > - ts->handleCompleteRequest(request);\n> > + if (timeSheet_)\n> > + timeSheet_->handleCompleteRequest(request);\n> >\n> > captureCount_++;\n> > if (captureCount_ >= captureLimit_) {\n> >\n> > So that now\n> >\n> > \tauto timeSheet = capture.startCaptureWithTimeSheet(10, &startValues);\n> > \tstd::cerr << \" TIMESHEET USE COUNT: \" << timeSheet.use_count();\n> >\n> > Reads as\n> > TIMESHEET USE COUNT: 2\n> >\n> > > >\n> > > > > +\treturn timeSheet;\n> > > > > +}\n> > > > > +\n> > > > > +int PerFrameControls::queueRequest(Request *request)\n> > > > > +{\n> > > > > +\tqueueCount_++;\n> > > > > +\tif (queueCount_ > captureLimit_)\n> > > > > +\t\treturn 0;\n> > > > > +\n> > > > > +\tauto ts = timeSheet_.lock();\n> > > > > +\tif (ts)\n> > > > > +\t\tts->prepareForQueue(request, queueCount_ - 1);\n> > > > > +\n> > > > > +\treturn camera_->queueRequest(request);\n> > > > > +}\n> > > > > +\n> > > > > +void PerFrameControls::requestComplete(Request *request)\n> > > > > +{\n> > > > > +\tauto ts = timeSheet_.lock();\n> > > > > +\tif (ts)\n> > > > > +\t\tts->handleCompleteRequest(request);\n> > > > > +\n> > > > > +\tcaptureCount_++;\n> > > > > +\tif (captureCount_ >= captureLimit_) {\n> > > > > +\t\tloop_->exit(0);\n> > > > > +\t\treturn;\n> > > > > +\t}\n> > > > > +\n> > > > > +\trequest->reuse(Request::ReuseBuffers);\n> > > > > +\tif (queueRequest(request))\n> > > > > +\t\tloop_->exit(-EINVAL);\n> > > > > +}\n> > > > > +\n> > > > > +void PerFrameControls::runCaptureSession()\n> > > > > +{\n> > > > > +\tStream *stream = config_->at(0).stream();\n> > > > > +\tconst std::vector> &buffers = allocator_->buffers(stream);\n> > > > > +\n> > > > > +\t/* Queue the recommended number of reqeuests. */\n> > > >\n> > > > s/reqeuests/requests/\n> > > >\n> > > > > +\tfor (const std::unique_ptr &buffer : buffers) {\n> > > > > +\t\tstd::unique_ptr request = camera_->createRequest();\n> > > > > +\t\trequest->addBuffer(stream, buffer.get());\n> > > > > +\t\tqueueRequest(request.get());\n> > > > > +\t\trequests_.push_back(std::move(request));\n> > > > > +\t}\n> > > > > +\n> > > > > +\t/* Run capture session. */\n> > > > > +\tloop_ = new EventLoop();\n> > > > > +\tloop_->exec();\n> > > > > +\tstop();\n> > > > > +\tdelete loop_;\n> > > > > +}\n> > > > > +\n> > > > > +void PerFrameControls::testExposureGainChangeOnSameFrame()\n> > > > > +{\n> > > > > +\tControlList startValues;\n> > > > > +\tstartValues.set(controls::ExposureTime, 5000);\n> > > > > +\tstartValues.set(controls::AnalogueGain, 1.0);\n> > > > > +\n> > > > > +\tauto timeSheet = startCaptureWithTimeSheet(10, &startValues);\n> > > > > +\tauto &ts = *timeSheet;\n> > > >\n> > > > Why a temporary reference ?\n> > >\n> > > The shared_ptr keeps the timesheet alive. The reference is just\n> >\n> > It's a reference, so it doesn't increment the usage count\n> >\n> > > syntactic shugar to be able to write ts[x]. I could replace these with\n> > > (*ts)[x] or ts->get(x) if you like that better.\n> > >\n> >\n> > Ah ok, seeing a reference just to be able to \"ts[]\" brings more\n> > questions on why the reference is there than clarity imho. Up to you.\n>\n> Thats now solved by my proposal above :-)\n>\n\nEven better, thanks for coming up with a nice solution :)\n\n> >\n> > > >\n> > > > > +\n> > > > > +\t/* wait a few frames to settle */\n> > > > > +\tts[7].controls().set(controls::ExposureTime, 10000);\n> > > > > +\tts[7].controls().set(controls::AnalogueGain, 4.0);\n> > > > > +\n> > > > > +\trunCaptureSession();\n> > > > > +\n> > > > > +\t/* Uncomment this to debug the test */\n> > > > > +\t/* ts.printAllInfos(); */\n> > > >\n> > > > Please drop\n> > > >\n> > > > > +\n> > > > > +\tASSERT_TRUE(ts[5].metadata().contains(controls::ExposureTime.id())) << \"Required metadata entry is missing\";\n> > > > > +\tASSERT_TRUE(ts[5].metadata().contains(controls::AnalogueGain.id())) << \"Required metadata entry is missing\";\n> > > >\n> > > > Break long lines when possible\n> > > >\n> > > > > +\n> > > > > +\tEXPECT_NEAR(ts[3].metadata().get(controls::ExposureTime).value(), 5000, 20);\n> > > > > +\tEXPECT_NEAR(ts[3].metadata().get(controls::AnalogueGain).value(), 1.0, 0.05);\n> > > > > +\n> > > > > +\t//find the frame with the changes\n> > > >\n> > > > No C++ comments please\n> > > >\n> > > > > +\tint exposureChangeIndex = 0;\n> > > > > +\tfor (unsigned i = 3; i < ts.size(); i++) {\n> > > > > +\t\tif (ts[i].metadata().get(controls::ExposureTime).value() > 7500) {\n> > > > > +\t\t\texposureChangeIndex = i;\n> > > > > +\t\t\tbreak;\n> > > > > +\t\t}\n> > > > > +\t}\n> > > > > +\n> > > > > +\tint gainChangeIndex = 0;\n> > > > > +\tfor (unsigned i = 3; i < ts.size(); i++) {\n> > > > > +\t\tif (ts[i].metadata().get(controls::AnalogueGain).value() > 2.0) {\n> > > > > +\t\t\tgainChangeIndex = i;\n> > > > > +\t\t\tbreak;\n> > > > > +\t\t}\n> > > > > +\t}\n> > > > > +\n> > > > > +\tEXPECT_NE(exposureChangeIndex, 0) << \"Exposure change not found in metadata\";\n> > > > > +\tEXPECT_NE(gainChangeIndex, 0) << \"Gain change not found in metadata\";\n> > > > > +\tEXPECT_EQ(exposureChangeIndex, gainChangeIndex)\n> > > > > +\t\t<< \"Metadata contained gain and exposure changes on different frames\";\n> > > > > +\n> > > > > +\tif (doImageTests) {\n> > > >\n> > > > Why do you think it should be optional ? If it has to be made optional\n> > > > it should be done in a way that doesn't depend on a compile time\n> > > > constant defined in the source code. Either make a series of separate\n> > > > image tests or add an option to lc-compliance.\n> > >\n> > > The tests still have a value without the content based tests, so yes\n> > > this should propably be a option to lc-compliance. We could default that\n> > > to false to express the 'optional' aspect.\n> > >\n> > > >\n> > > > > +\t\tint brightnessChangeIndex = 0;\n> > > > > +\t\tfor (unsigned i = 3; i < ts.size(); i++) {\n> > > >\n> > > > The usage of '3' seems to be there to ignore the first three frames,\n> > > > right ? If so, what about defining a constant and add a comment ?\n> > > >\n> > > > > +\t\t\tif (ts[i].getBrightnessChange() > 1.3) {\n> > > > > +\t\t\t\tEXPECT_EQ(brightnessChangeIndex, 0)\n> > > > > +\t\t\t\t\t<< \"Detected multiple frames with brightness increase (Wrong control delays?)\";\n> > > > > +\n> > > > > +\t\t\t\tif (!brightnessChangeIndex)\n> > > > > +\t\t\t\t\tbrightnessChangeIndex = i;\n> > > > > +\t\t\t}\n> > > > > +\t\t}\n> > > > > +\n> > > > > +\t\tEXPECT_EQ(exposureChangeIndex, brightnessChangeIndex)\n> > > > > +\t\t\t<< \"Exposure change and mesaured brightness change were not on same frame. \"\n> > > > > +\t\t\t<< \"(Wrong control delay?, Start frame event too late?)\";\n> > > > > +\t\tEXPECT_EQ(exposureChangeIndex, gainChangeIndex)\n> > > > > +\t\t\t<< \"Gain change and mesaured brightness change were not on same frame. \"\n> > > > > +\t\t\t<< \"(Wrong control delay?, Start frame event too late?)\";\n> > > > > +\t}\n> > > > > +}\n> > > > > +\n> > > > > +void PerFrameControls::testFramePreciseExposureChange()\n> > > > > +{\n> > > > > +\tauto timeSheet = startCaptureWithTimeSheet(10);\n> > > > > +\tauto &ts = *timeSheet;\n> > > > > +\n> > > > > +\tts[3].controls().set(controls::ExposureTime, 5000);\n> > > > > +\t/* wait a few frames to settle */\n> > > > > +\tts[6].controls().set(controls::ExposureTime, 20000);\n> > > > > +\n> > > > > +\trunCaptureSession();\n> > > > > +\n> > > > > +\t/* Uncomment this to debug the test */\n> > > > > +\t/* ts.printAllInfos(); */\n> > > >\n> > > > ditto\n> > > >\n> > > > > +\n> > > > > +\tASSERT_TRUE(ts[5].metadata().contains(controls::ExposureTime.id())) << \"Required metadata entry is missing\";\n> > > >\n> > > > break long lines\n> > > >\n> > > > > +\n> > > > > +\tEXPECT_NEAR(ts[5].metadata().get(controls::ExposureTime).value(), 5000, 20);\n> > > > > +\tEXPECT_NEAR(ts[6].metadata().get(controls::ExposureTime).value(), 20000, 20);\n> > > > > +\n> > > > > +\tif (doImageTests) {\n> > > > > +\t\t/* No increase just before setting exposure */\n> > > > > +\t\tEXPECT_NEAR(ts[5].getBrightnessChange(), 1.0, 0.05)\n> > > > > +\t\t\t<< \"Brightness changed too much before the expected time of change (control delay too high?).\";\n> > > > > +\t\t/*\n> > > > > +\t\t* Todo: The change is brightness was a bit low\n> > > >\n> > > > Wrong alignment\n> > > >\n> > > > We don't Doxygen lc-compliance but try to use \\todo for consistency\n> > > > with the rest of the code base\n> > > >\n> > > > > +\t\t* (Exposure time increase by 4x resulted in a brightness increase of < 2).\n> > > > > +\t\t* This should be investigated.\n> > > >\n> > > > Might be platform specific issue ?\n> > > >\n> > > > > +\t\t*/\n> > > > > +\t\tEXPECT_GT(ts[6].getBrightnessChange(), 1.3)\n> > > > > +\t\t\t<< \"Brightness in frame \" << 6 << \" did not increase as expected (reference: \"\n> > > > > +\t\t\t<< ts[3].getSpotBrightness() << \" current: \" << ts[6].getSpotBrightness() << \" )\" << std::endl;\n> > > > > +\n> > > > > +\t\t/* No increase just after setting exposure */\n> > > > > +\t\tEXPECT_NEAR(ts[7].getBrightnessChange(), 1.0, 0.05)\n> > > > > +\t\t\t<< \"Brightness changed too much after the expected time of change (control delay too low?).\";\n> > > > > +\n> > > > > +\t\t/* No increase just after setting exposure */\n> > > > > +\t\tEXPECT_NEAR(ts[8].getBrightnessChange(), 1.0, 0.05)\n> > > > > +\t\t\t<< \"Brightness changed too much 2 frames after the expected time of change (control delay too low?).\";\n> > > > > +\t}\n> > > > > +}\n> > > > > +\n> > > > > +void PerFrameControls::testFramePreciseGainChange()\n> > > > > +{\n> > > > > +\tauto timeSheet = startCaptureWithTimeSheet(10);\n> > > > > +\tauto &ts = *timeSheet;\n> > > > > +\n> > > > > +\tts[3].controls().set(controls::AnalogueGain, 1.0);\n> > > > > +\t/* wait a few frames to settle */\n> > > > > +\tts[6].controls().set(controls::AnalogueGain, 4.0);\n> > > > > +\n> > > > > +\trunCaptureSession();\n> > > > > +\n> > > > > +\t/* Uncomment this, to debug the test */\n> > > > > +\t/* ts.printAllInfos(); */\n> > > > > +\n> > > > > +\tASSERT_TRUE(ts[5].metadata().contains(controls::AnalogueGain.id())) << \"Required metadata entry is missing\";\n> > > >\n> > > > Break this long line\n> > > >\n> > > > > +\n> > > > > +\tEXPECT_NEAR(ts[5].metadata().get(controls::AnalogueGain).value(), 1.0, 0.1);\n> > > > > +\tEXPECT_NEAR(ts[6].metadata().get(controls::AnalogueGain).value(), 4.0, 0.1);\n> > > > > +\n> > > > > +\tif (doImageTests) {\n> > > > > +\t\t/* No increase just before setting gain */\n> > > > > +\t\tEXPECT_NEAR(ts[5].getBrightnessChange(), 1.0, 0.05)\n> > > > > +\t\t\t<< \"Brightness changed too much before the expected time of change (control delay too high?).\";\n> > > > > +\t\t/*\n> > > > > +\t\t* Todo: I see a brightness change of roughly half the expected one.\n> > > >\n> > > > Wrong alignment here too, also \\todo\n> > > >\n> > > > > +\t\t* This is not yet understood and needs investigation\n> > > >\n> > > > Defintely some platform specific thing to investigate then ?\n> > > >\n> > > > > +\t\t*/\n> > > > > +\t\tEXPECT_GT(ts[6].getBrightnessChange(), 1.7)\n> > > > > +\t\t\t<< \"Brightness in frame \" << 6 << \" did not increase as expected (reference: \"\n> > > > > +\t\t\t<< ts[5].getSpotBrightness() << \" current: \" << ts[6].getSpotBrightness() << \" )\" << std::endl;\n> > > > > +\n> > > > > +\t\t/* No increase just after setting gain */\n> > > > > +\t\tEXPECT_NEAR(ts[7].getBrightnessChange(), 1.0, 0.05)\n> > > > > +\t\t\t<< \"Brightness changed too much after the expected time of change (control delay too low?).\";\n> > > > > +\n> > > > > +\t\t/* No increase just after setting gain */\n> > > > > +\t\tEXPECT_NEAR(ts[8].getBrightnessChange(), 1.0, 0.05)\n> > > > > +\t\t\t<< \"Brightness changed too much after the expected time of change (control delay too low?).\";\n> > > > > +\t}\n> > > > > +}\n> > > > > +\n> > > > > +void PerFrameControls::testExposureGainFromFirstRequestGetsApplied()\n> > > > > +{\n> > > > > +\tauto timeSheet = startCaptureWithTimeSheet(5);\n> > > > > +\tauto &ts = *timeSheet;\n> > > > > +\n> > > > > +\tts[0].controls().set(controls::ExposureTime, 10000);\n> > > > > +\tts[0].controls().set(controls::AnalogueGain, 4.0);\n> > > > > +\n> > > > > +\trunCaptureSession();\n> > > > > +\n> > > > > +\tASSERT_TRUE(ts[4].metadata().contains(controls::ExposureTime.id())) << \"Required metadata entry is missing\";\n> > > > > +\tASSERT_TRUE(ts[4].metadata().contains(controls::AnalogueGain.id())) << \"Required metadata entry is missing\";\n> > > >\n> > > > You can easily break these lines\n> > > >\n> > > > > +\n> > > > > +\t/* We expect it to be applied after 3 frames, the latest*/\n> > > >\n> > > > What if a sensor takes a longer time to apply exposure and gain ?\n> > >\n> > > Then we should adjust the test :-)\n> > >\n> > > >\n> > > > > +\tEXPECT_NEAR(ts[4].metadata().get(controls::ExposureTime).value(), 10000, 20);\n> > > > > +\tEXPECT_NEAR(ts[4].metadata().get(controls::AnalogueGain).value(), 4.0, 0.1);\n> > > > > +}\n> > > > > +\n> > > > > +void PerFrameControls::testExposureGainFromFirstAndSecondRequestGetsApplied()\n> > > > > +{\n> > > > > +\tauto timeSheet = startCaptureWithTimeSheet(5);\n> > > > > +\tauto &ts = *timeSheet;\n> > > > > +\n> > > > > +\tts[0].controls().set(controls::ExposureTime, 8000);\n> > > > > +\tts[0].controls().set(controls::AnalogueGain, 2.0);\n> > > > > +\tts[1].controls().set(controls::ExposureTime, 10000);\n> > > > > +\tts[1].controls().set(controls::AnalogueGain, 4.0);\n> > > >\n> > > > As a general question, how do we guarantee the values you use to set\n> > > > exposure and gains are valid for all possible sensors ?\n> > > >\n> > > > Shouldn't you inspect inspect the ControlInfo limit from\n> > > > Camera::controls() and clamp the values in the min/max range ?\n> > >\n> > > I thought about that too. I started with values that most if not all\n> > > sensor we care atm should support. I'm weary if it is worth the effort\n> > > and that it would make the tests way less readable. But you are right,\n> > > that is an unsolved issue.\n> >\n> > Recording it with a \\todo comment is fine for now\n> >\n> > >\n> > > >\n> > > > > +\n> > > > > +\trunCaptureSession();\n> > > > > +\n> > > > > +\tASSERT_TRUE(ts[4].metadata().contains(controls::ExposureTime.id())) << \"Required metadata entry is missing\";\n> > > > > +\tASSERT_TRUE(ts[4].metadata().contains(controls::AnalogueGain.id())) << \"Required metadata entry is missing\";\n> > > >\n> > > > Ditto\n> > > >\n> > > > > +\n> > > > > +\t/* We expect it to be applied after 3 frames, the latest */\n> > > > > +\tEXPECT_NEAR(ts[4].metadata().get(controls::ExposureTime).value(), 10000, 20);\n> > > > > +\tEXPECT_NEAR(ts[4].metadata().get(controls::AnalogueGain).value(), 4.0, 0.1);\n> > > > > +}\n> > > > > +\n> > > > > +void PerFrameControls::testExposureGainIsAppliedOnFirstFrame()\n> > > > > +{\n> > > > > +\tControlList startValues;\n> > > > > +\tstartValues.set(controls::ExposureTime, 5000);\n> > > > > +\tstartValues.set(controls::AnalogueGain, 1.0);\n> > > > > +\n> > > > > +\tauto ts1 = startCaptureWithTimeSheet(3, &startValues);\n> > > > > +\n> > > > > +\trunCaptureSession();\n> > > > > +\n> > > > > +\tASSERT_TRUE((*ts1)[0].metadata().contains(controls::ExposureTime.id())) << \"Required metadata entry is missing\";\n> > > > > +\tASSERT_TRUE((*ts1)[0].metadata().contains(controls::AnalogueGain.id())) << \"Required metadata entry is missing\";\n> > > > > +\n> > > > > +\tEXPECT_NEAR((*ts1)[0].metadata().get(controls::ExposureTime).value(), 5000, 20);\n> > > > > +\tEXPECT_NEAR((*ts1)[0].metadata().get(controls::AnalogueGain).value(), 1.0, 0.02);\n> > > > > +\n> > > > > +\t/* Second capture with different values to ensure we don't hit default/old values */\n> > > > > +\tstartValues.set(controls::ExposureTime, 15000);\n> > > > > +\tstartValues.set(controls::AnalogueGain, 4.0);\n> > > > > +\n> > > > > +\tauto ts2 = startCaptureWithTimeSheet(3, &startValues);\n> > > > > +\n> > > > > +\trunCaptureSession();\n> > > > > +\n> > > > > +\tEXPECT_NEAR((*ts2)[0].metadata().get(controls::ExposureTime).value(), 15000, 20);\n> > > > > +\tEXPECT_NEAR((*ts2)[0].metadata().get(controls::AnalogueGain).value(), 4.0, 0.02);\n> > > > > +\n> > > > > +\tif (doImageTests) {\n> > > > > +\t\t/* With 3x exposure and 4x gain we could expect a brightness increase of 2x */\n> > > > > +\t\tdouble brightnessChange = ts2->get(1).getSpotBrightness() / ts1->get(1).getSpotBrightness();\n> > > > > +\t\tEXPECT_GT(brightnessChange, 2.0);\n> > > > > +\t}\n> > > > > +}\n> > > > > diff --git a/src/apps/lc-compliance/per_frame_controls.h b/src/apps/lc-compliance/per_frame_controls.h\n> > > > > new file mode 100644\n> > > > > index 00000000..a341c61f\n> > > > > --- /dev/null\n> > > > > +++ b/src/apps/lc-compliance/per_frame_controls.h\n> > > > > @@ -0,0 +1,43 @@\n> > > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > > > +/*\n> > > > > + * Copyright (C) 2024, Ideas on Board Oy\n> > > > > + *\n> > > > > + * per_frame_controls.h - Tests for per frame controls\n> > > > > + */\n> > > > > +\n> > > > > +#pragma once\n> > > > > +\n> > > > > +#include \n> > > > > +\n> > > > > +#include \n> > > > > +\n> > > > > +#include \"../common/event_loop.h\"\n> > > > > +\n> > > > > +#include \"simple_capture.h\"\n> > > > > +#include \"time_sheet.h\"\n> > > > > +\n> > > > > +class PerFrameControls : public SimpleCapture\n> > > > > +{\n> > > > > +public:\n> > > > > +\tPerFrameControls(std::shared_ptr camera);\n> > > > > +\n> > > > > +\tstd::shared_ptr\n> > > > > +\tstartCaptureWithTimeSheet(unsigned int framesToCapture, const libcamera::ControlList *controls = nullptr);\n> > > > > +\tvoid runCaptureSession();\n> > > > > +\n> > > > > +\tvoid testExposureGainChangeOnSameFrame();\n> > > > > +\tvoid testFramePreciseExposureChange();\n> > > > > +\tvoid testFramePreciseGainChange();\n> > > > > +\tvoid testExposureGainIsAppliedOnFirstFrame();\n> > > > > +\tvoid testExposureGainFromFirstRequestGetsApplied();\n> > > > > +\tvoid testExposureGainFromFirstAndSecondRequestGetsApplied();\n> > > > > +\n> > > > > +\tint queueRequest(libcamera::Request *request);\n> > > > > +\tvoid requestComplete(libcamera::Request *request) override;\n> > > > > +\n> > > > > +\tunsigned int queueCount_;\n> > > > > +\tunsigned int captureCount_;\n> > > > > +\tunsigned int captureLimit_;\n> > > > > +\n> > > > > +\tstd::weak_ptr timeSheet_;\n> > > > > +};\n> > > > > --\n> > > > > 2.40.1\n> > > > >\n> > >\n> > > Thanks for all the input :-)\n> > >\n> > > Cheers,\n> > > Stefan","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id AC01DC3272\n\tfor ;\n\tThu, 21 Mar 2024 11:56:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BBC3B6305E;\n\tThu, 21 Mar 2024 12:56:21 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 943BC63036\n\tfor ;\n\tThu, 21 Mar 2024 12:56:19 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2001:b07:5d2e:52c9:cc1e:e404:491f:e6ea])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4A5097E9;\n\tThu, 21 Mar 2024 12:55:51 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"LzJJsUYC\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1711022151;\n\tbh=E1oFj8lMTBVP7Xf4DRrPnJsTBjJiCRVWvuA1f7/pv1c=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LzJJsUYCyeLDIrhBxQZYyVC+Thom+xurc36Ub4EokcodFBvvzQ5vEsMUERkMlw0zE\n\tsHp6xF3QkZw6nLEs/V96qaSuMC0D68aXxnlFjWr83TETElDFdocoMykSLnmY/aqV8r\n\tMsqS0u4Ydp1YLUcmx+J42QSCv2MSPs0fFxDgXSC8=","Date":"Thu, 21 Mar 2024 12:56:16 +0100","From":"Jacopo Mondi ","To":"Stefan Klug ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 03/12] libcamera: lc-compliance: Add initial set of\n\tper-frame-control tests","Message-ID":"","References":"<20240313121223.138150-1-stefan.klug@ideasonboard.com>\n\t<20240313121223.138150-4-stefan.klug@ideasonboard.com>\n\t\n\t<20240315155303.m2yfff7w2fcfydtw@jasper>\n\t<752it44re455lb25sd4et5osimvpnupxn2ayoqz75f7t3nqblk@4sfpj3xgvg7e>\n\t<20240321113449.qlolurgw53myho6l@jasper>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240321113449.qlolurgw53myho6l@jasper>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]