[{"id":29038,"web_url":"https://patchwork.libcamera.org/comment/29038/","msgid":"<m64322mv4g3rdawau3x2uazbiat7qjfjtnor6xf5x2geeot53g@zu2h2l6oojxs>","date":"2024-03-22T11:25:33","subject":"Re: [PATCH v3 03/16] 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 Tue, Mar 19, 2024 at 01:05:04PM +0100, Stefan Klug wrote:\n> Add tests that check if controls (only exposure time and analogue gain at the\n> moment) get applied on the frame they were requested for. This is tested by\n> looking at the metadata and the mean brightness of the image center. At the\n> moment these tests fail. Fixes for the pipelines will be delivered in later\n> patches (rkisp1 for now).\n>\n> To run the pfc tests only:\n> lc-compliance -c <cam> -f \"PerFrameControlTests.*\"\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> These tests are known to pass using a imx219 (RPi cam v2) with a imx8mp\n> (debix-som) using the rkisp1 pipeline\n>\n> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>\n> ---\n\n[snip]\n\n> +\n> +TEST_F(PerFrameControlTests, testExposureGainChangeOnSameFrame)\n> +{\n> +\tPerFrameControlsCapture 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> +\tcapture.runCaptureSession();\n> +\n> +\tASSERT_TRUE(ts[5].metadata().contains(controls::ExposureTime.id()))\n> +\t\t<< \"Required metadata entry is missing\";\n> +\tASSERT_TRUE(ts[5].metadata().contains(controls::AnalogueGain.id()))\n> +\t\t<< \"Required metadata entry is missing\";\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> +\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> +\t\tint brightnessChangeIndex = 0;\n> +\t\tfor (unsigned i = 3; i < ts.size(); i++) {\n> +\t\t\tif (ts[i].brightnessChange() > 1.3) {\n> +\t\t\t\tEXPECT_EQ(brightnessChangeIndex, 0)\n> +\t\t\t\t\t<< \"Detected multiple frames with brightness increase\"\n> +\t\t\t\t\t<< \" (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 measured brightness change were not on same\"\n> +\t\t\t<< \" frame. (Wrong control delay?, Start frame event too late?)\";\n> +\t\tEXPECT_EQ(exposureChangeIndex, gainChangeIndex)\n> +\t\t\t<< \"Gain change and measured brightness change were not on same \"\n> +\t\t\t<< \" frame. (Wrong control delay?, Start frame event too late?)\";\n> +\t}\n> +}\n\nI think this is a valid test, we want to make sure exposure and gain\nchange on the same frame\n\n> +\n> +TEST_F(PerFrameControlTests, testFramePreciseExposureChange)\n> +{\n> +\tPerFrameControlsCapture capture(camera_);\n> +\tcapture.configure(StreamRole::VideoRecording);\n> +\n> +\tauto timeSheet = capture.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> +\tcapture.runCaptureSession();\n> +\n> +\tASSERT_TRUE(ts[5].metadata().contains(controls::ExposureTime.id()))\n> +\t\t<< \"Required metadata entry is missing\";\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\nWhy do you need near ? The camera can't get the precise exposure time ?\n\nI think the test is valid, however it uses absolute values (3, 6,\n5000, 20000) while in future these should be parametrized (using the\npipeline depth and the exposure min, max) but we already agree with\nthis.\n\n\n> +\n> +\tif (doImageTests) {\n> +\t\t/* No increase just before setting exposure */\n> +\t\tEXPECT_NEAR(ts[5].brightnessChange(), 1.0, 0.05)\n> +\t\t\t<< \"Brightness changed too much before the expected time of change\"\n> +\t\t\t<< \" (control delay too high?).\";\n> +\t\t/*\n> +\t\t * \\todo The change is brightness was a bit low\n> +\t\t * (Exposure time increase by 4x resulted in a brightness increase of < 2).\n> +\t\t * This should be investigated.\n> +\t\t */\n> +\t\tEXPECT_GT(ts[6].brightnessChange(), 1.3)\n> +\t\t\t<< \"Brightness in frame \" << 6 << \" did not increase as expected\"\n> +\t\t\t<< \" (reference: \" << ts[3].spotBrightness() << \" current: \"\n> +\t\t\t<< ts[6].spotBrightness() << \" )\" << std::endl;\n> +\n> +\t\t/* No increase just after setting exposure */\n> +\t\tEXPECT_NEAR(ts[7].brightnessChange(), 1.0, 0.05)\n> +\t\t\t<< \"Brightness changed too much after the expected time of change\"\n> +\t\t\t<< \" (control delay too low?).\";\n> +\n> +\t\t/* No increase just after setting exposure */\n> +\t\tEXPECT_NEAR(ts[8].brightnessChange(), 1.0, 0.05)\n> +\t\t\t<< \"Brightness changed too much 2 frames after the expected time\"\n> +\t\t\t<< \" of change (control delay too low?).\";\n> +\t}\n> +}\n> +\n> +TEST_F(PerFrameControlTests, testFramePreciseGainChange)\n> +{\n> +\tPerFrameControlsCapture capture(camera_);\n> +\tcapture.configure(StreamRole::VideoRecording);\n> +\n> +\tauto timeSheet = capture.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> +\tcapture.runCaptureSession();\n> +\n> +\tASSERT_TRUE(ts[5].metadata().contains(controls::AnalogueGain.id()))\n> +\t\t<< \"Required metadata entry is missing\";\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\nSame comment as the for the above test.\n\n> +\tif (doImageTests) {\n> +\t\t/* No increase just before setting gain */\n> +\t\tEXPECT_NEAR(ts[5].brightnessChange(), 1.0, 0.05)\n> +\t\t\t<< \"Brightness changed too much before the expected time of change\"\n> +\t\t\t<< \" (control delay too high?).\";\n> +\t\t/*\n> +\t\t * \\todo I see a brightness change of roughly half the expected one.\n> +\t\t * This is not yet understood and needs investigation\n> +\t\t */\n> +\t\tEXPECT_GT(ts[6].brightnessChange(), 1.7)\n> +\t\t\t<< \"Brightness in frame \" << 6 << \" did not increase as expected\"\n> +\t\t\t<< \" (reference: \" << ts[5].spotBrightness()\n> +\t\t\t<< \" current: \" << ts[6].spotBrightness() << \")\" << std::endl;\n> +\n> +\t\t/* No increase just after setting gain */\n> +\t\tEXPECT_NEAR(ts[7].brightnessChange(), 1.0, 0.05)\n> +\t\t\t<< \"Brightness changed too much after the expected time of change\"\n> +\t\t\t<< \" (control delay too low?).\";\n> +\n> +\t\t/* No increase just after setting gain */\n> +\t\tEXPECT_NEAR(ts[8].brightnessChange(), 1.0, 0.05)\n> +\t\t\t<< \"Brightness changed too much after the expected time of change\"\n> +\t\t\t<< \" (control delay too low?).\";\n> +\t}\n> +}\n> +\n> +TEST_F(PerFrameControlTests, testExposureGainFromFirstRequestGetsApplied)\n> +{\n> +\tPerFrameControlsCapture capture(camera_);\n> +\tcapture.configure(StreamRole::VideoRecording);\n> +\n> +\tauto timeSheet = capture.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> +\tcapture.runCaptureSession();\n> +\n> +\tASSERT_TRUE(ts[4].metadata().contains(controls::ExposureTime.id()))\n> +\t\t<< \"Required metadata entry is missing\";\n> +\tASSERT_TRUE(ts[4].metadata().contains(controls::AnalogueGain.id()))\n> +\t\t<< \"Required metadata entry is missing\";\n> +\n> +\t/* We expect it to be applied after 3 frames, the latest*/\n\nShould the 'startup' frames be only the sensor delays or the full\npipeline depth ?\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> +TEST_F(PerFrameControlTests, testExposureGainFromFirstAndSecondRequestGetsApplied)\n> +{\n> +\tPerFrameControlsCapture capture(camera_);\n> +\tcapture.configure(StreamRole::VideoRecording);\n> +\n> +\tauto timeSheet = capture.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> +\tcapture.runCaptureSession();\n> +\n> +\tASSERT_TRUE(ts[4].metadata().contains(controls::ExposureTime.id()))\n> +\t\t<< \"Required metadata entry is missing\";\n> +\tASSERT_TRUE(ts[4].metadata().contains(controls::AnalogueGain.id()))\n> +\t\t<< \"Required metadata entry is missing\";\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\nEh, interesting. How should we expect the startup sequence to be\nhandled ?\n\nI presume ts[0] is not applied before streaming is started, right ?\n\nLet me graph your test expectations (for ExposureTime only)\n\n       0     1     2     3     4     5     6     7     8     9\nframe  +-----+-----+-----+-----+-----+-----+-----+-----+-----+-----+\n\nET       8k    10k\nsensor [init]\n            [init]  [init] 8k\n                    [init] 8k    10k\n\nShould we define \"per frame control\" as the expectations that:\n- given no requests underrun (the application queues requests fast\n  enough to have at least 'depth' requests in queue\n\nControls associated with Request[x] will be applied at Frame[x + depth] ?\n\n\n> +}\n> +\n> +TEST_F(PerFrameControlTests, testExposureGainIsAppliedOnFirstFrame)\n> +{\n> +\tPerFrameControlsCapture 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 ts1 = capture.startCaptureWithTimeSheet(3, &startValues);\n> +\n> +\tcapture.runCaptureSession();\n> +\n> +\tASSERT_TRUE((*ts1)[0].metadata().contains(controls::ExposureTime.id()))\n> +\t\t<< \"Required metadata entry is missing\";\n> +\tASSERT_TRUE((*ts1)[0].metadata().contains(controls::AnalogueGain.id()))\n> +\t\t<< \"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 = capture.startCaptureWithTimeSheet(3, &startValues);\n> +\n> +\tcapture.runCaptureSession();\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).spotBrightness() / ts1->get(1).spotBrightness();\n> +\t\tEXPECT_GT(brightnessChange, 2.0);\n> +\t}\n> +}\n> --\n> 2.40.1\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 6D907BD160\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 22 Mar 2024 11:25:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7506261C45;\n\tFri, 22 Mar 2024 12:25:38 +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 9182861C45\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 22 Mar 2024 12:25:36 +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 9972EBEB;\n\tFri, 22 Mar 2024 12:25:07 +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=\"nkwlC9Nu\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1711106707;\n\tbh=Tq1TmvR62veEtVCmCDruTiTrfx4mbL61Oa/cJDSgG7M=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=nkwlC9NuHWIyoA0c44WXdpaQp3zaV8MpauZph1pC7BRnhvLVt7BXUE1d881gVSI5A\n\tqxrrl8YWV59XZ6dZAMtFllfn4i3l3WDvHSwI474d64+xZSWwvmVA7ItS3x016que8m\n\tSFbia6/VlbZd2zbubD/xleBZj/7NhSBuy9zHHnSM=","Date":"Fri, 22 Mar 2024 12:25:33 +0100","From":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","To":"Stefan Klug <stefan.klug@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v3 03/16] libcamera: lc-compliance: Add initial set of\n\tper-frame-control tests","Message-ID":"<m64322mv4g3rdawau3x2uazbiat7qjfjtnor6xf5x2geeot53g@zu2h2l6oojxs>","References":"<20240319120517.362082-1-stefan.klug@ideasonboard.com>\n\t<20240319120517.362082-4-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240319120517.362082-4-stefan.klug@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":29039,"web_url":"https://patchwork.libcamera.org/comment/29039/","msgid":"<20240322131517.waf2jh4oy7ojwdwm@jasper>","date":"2024-03-22T13:15:17","subject":"Re: [PATCH v3 03/16] 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 looking at the tests.\n\nOn Fri, Mar 22, 2024 at 12:25:33PM +0100, Jacopo Mondi wrote:\n> Hi Stefan\n> \n> On Tue, Mar 19, 2024 at 01:05:04PM +0100, Stefan Klug wrote:\n> > Add tests that check if controls (only exposure time and analogue gain at the\n> > moment) get applied on the frame they were requested for. This is tested by\n> > looking at the metadata and the mean brightness of the image center. At the\n> > moment these tests fail. Fixes for the pipelines will be delivered in later\n> > patches (rkisp1 for now).\n> >\n> > To run the pfc tests only:\n> > lc-compliance -c <cam> -f \"PerFrameControlTests.*\"\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> > These tests are known to pass using a imx219 (RPi cam v2) with a imx8mp\n> > (debix-som) using the rkisp1 pipeline\n> >\n> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>\n> > ---\n> \n> [snip]\n> \n> > +\n> > +TEST_F(PerFrameControlTests, testExposureGainChangeOnSameFrame)\n> > +{\n> > +\tPerFrameControlsCapture 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> > +\tcapture.runCaptureSession();\n> > +\n> > +\tASSERT_TRUE(ts[5].metadata().contains(controls::ExposureTime.id()))\n> > +\t\t<< \"Required metadata entry is missing\";\n> > +\tASSERT_TRUE(ts[5].metadata().contains(controls::AnalogueGain.id()))\n> > +\t\t<< \"Required metadata entry is missing\";\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> > +\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> > +\t\tint brightnessChangeIndex = 0;\n> > +\t\tfor (unsigned i = 3; i < ts.size(); i++) {\n> > +\t\t\tif (ts[i].brightnessChange() > 1.3) {\n> > +\t\t\t\tEXPECT_EQ(brightnessChangeIndex, 0)\n> > +\t\t\t\t\t<< \"Detected multiple frames with brightness increase\"\n> > +\t\t\t\t\t<< \" (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 measured brightness change were not on same\"\n> > +\t\t\t<< \" frame. (Wrong control delay?, Start frame event too late?)\";\n> > +\t\tEXPECT_EQ(exposureChangeIndex, gainChangeIndex)\n> > +\t\t\t<< \"Gain change and measured brightness change were not on same \"\n> > +\t\t\t<< \" frame. (Wrong control delay?, Start frame event too late?)\";\n> > +\t}\n> > +}\n> \n> I think this is a valid test, we want to make sure exposure and gain\n> change on the same frame\n> \n> > +\n> > +TEST_F(PerFrameControlTests, testFramePreciseExposureChange)\n> > +{\n> > +\tPerFrameControlsCapture capture(camera_);\n> > +\tcapture.configure(StreamRole::VideoRecording);\n> > +\n> > +\tauto timeSheet = capture.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> > +\tcapture.runCaptureSession();\n> > +\n> > +\tASSERT_TRUE(ts[5].metadata().contains(controls::ExposureTime.id()))\n> > +\t\t<< \"Required metadata entry is missing\";\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> Why do you need near ? The camera can't get the precise exposure time ?\n\nSome quantization always happens and that is reported back in the\nmetadata. That raises the question: Does libcamera make any such promise?\n\n> \n> I think the test is valid, however it uses absolute values (3, 6,\n> 5000, 20000) while in future these should be parametrized (using the\n> pipeline depth and the exposure min, max) but we already agree with\n> this.\n> \n> \n> > +\n> > +\tif (doImageTests) {\n> > +\t\t/* No increase just before setting exposure */\n> > +\t\tEXPECT_NEAR(ts[5].brightnessChange(), 1.0, 0.05)\n> > +\t\t\t<< \"Brightness changed too much before the expected time of change\"\n> > +\t\t\t<< \" (control delay too high?).\";\n> > +\t\t/*\n> > +\t\t * \\todo The change is brightness was a bit low\n> > +\t\t * (Exposure time increase by 4x resulted in a brightness increase of < 2).\n> > +\t\t * This should be investigated.\n> > +\t\t */\n> > +\t\tEXPECT_GT(ts[6].brightnessChange(), 1.3)\n> > +\t\t\t<< \"Brightness in frame \" << 6 << \" did not increase as expected\"\n> > +\t\t\t<< \" (reference: \" << ts[3].spotBrightness() << \" current: \"\n> > +\t\t\t<< ts[6].spotBrightness() << \" )\" << std::endl;\n> > +\n> > +\t\t/* No increase just after setting exposure */\n> > +\t\tEXPECT_NEAR(ts[7].brightnessChange(), 1.0, 0.05)\n> > +\t\t\t<< \"Brightness changed too much after the expected time of change\"\n> > +\t\t\t<< \" (control delay too low?).\";\n> > +\n> > +\t\t/* No increase just after setting exposure */\n> > +\t\tEXPECT_NEAR(ts[8].brightnessChange(), 1.0, 0.05)\n> > +\t\t\t<< \"Brightness changed too much 2 frames after the expected time\"\n> > +\t\t\t<< \" of change (control delay too low?).\";\n> > +\t}\n> > +}\n> > +\n> > +TEST_F(PerFrameControlTests, testFramePreciseGainChange)\n> > +{\n> > +\tPerFrameControlsCapture capture(camera_);\n> > +\tcapture.configure(StreamRole::VideoRecording);\n> > +\n> > +\tauto timeSheet = capture.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> > +\tcapture.runCaptureSession();\n> > +\n> > +\tASSERT_TRUE(ts[5].metadata().contains(controls::AnalogueGain.id()))\n> > +\t\t<< \"Required metadata entry is missing\";\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> \n> Same comment as the for the above test.\n> \n> > +\tif (doImageTests) {\n> > +\t\t/* No increase just before setting gain */\n> > +\t\tEXPECT_NEAR(ts[5].brightnessChange(), 1.0, 0.05)\n> > +\t\t\t<< \"Brightness changed too much before the expected time of change\"\n> > +\t\t\t<< \" (control delay too high?).\";\n> > +\t\t/*\n> > +\t\t * \\todo I see a brightness change of roughly half the expected one.\n> > +\t\t * This is not yet understood and needs investigation\n> > +\t\t */\n> > +\t\tEXPECT_GT(ts[6].brightnessChange(), 1.7)\n> > +\t\t\t<< \"Brightness in frame \" << 6 << \" did not increase as expected\"\n> > +\t\t\t<< \" (reference: \" << ts[5].spotBrightness()\n> > +\t\t\t<< \" current: \" << ts[6].spotBrightness() << \")\" << std::endl;\n> > +\n> > +\t\t/* No increase just after setting gain */\n> > +\t\tEXPECT_NEAR(ts[7].brightnessChange(), 1.0, 0.05)\n> > +\t\t\t<< \"Brightness changed too much after the expected time of change\"\n> > +\t\t\t<< \" (control delay too low?).\";\n> > +\n> > +\t\t/* No increase just after setting gain */\n> > +\t\tEXPECT_NEAR(ts[8].brightnessChange(), 1.0, 0.05)\n> > +\t\t\t<< \"Brightness changed too much after the expected time of change\"\n> > +\t\t\t<< \" (control delay too low?).\";\n> > +\t}\n> > +}\n> > +\n> > +TEST_F(PerFrameControlTests, testExposureGainFromFirstRequestGetsApplied)\n> > +{\n> > +\tPerFrameControlsCapture capture(camera_);\n> > +\tcapture.configure(StreamRole::VideoRecording);\n> > +\n> > +\tauto timeSheet = capture.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> > +\tcapture.runCaptureSession();\n> > +\n> > +\tASSERT_TRUE(ts[4].metadata().contains(controls::ExposureTime.id()))\n> > +\t\t<< \"Required metadata entry is missing\";\n> > +\tASSERT_TRUE(ts[4].metadata().contains(controls::AnalogueGain.id()))\n> > +\t\t<< \"Required metadata entry is missing\";\n> > +\n> > +\t/* We expect it to be applied after 3 frames, the latest*/\n> \n> Should the 'startup' frames be only the sensor delays or the full\n> pipeline depth ?\n\nThat depends on the other discussions. My (personal) expectation would be\nthat only the sensor delay applies as we are running in manual mode.\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> > +TEST_F(PerFrameControlTests, testExposureGainFromFirstAndSecondRequestGetsApplied)\n> > +{\n> > +\tPerFrameControlsCapture capture(camera_);\n> > +\tcapture.configure(StreamRole::VideoRecording);\n> > +\n> > +\tauto timeSheet = capture.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> > +\tcapture.runCaptureSession();\n> > +\n> > +\tASSERT_TRUE(ts[4].metadata().contains(controls::ExposureTime.id()))\n> > +\t\t<< \"Required metadata entry is missing\";\n> > +\tASSERT_TRUE(ts[4].metadata().contains(controls::AnalogueGain.id()))\n> > +\t\t<< \"Required metadata entry is missing\";\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> Eh, interesting. How should we expect the startup sequence to be\n> handled ?\n> \n> I presume ts[0] is not applied before streaming is started, right ?\n\nyes.\n\n> \n> Let me graph your test expectations (for ExposureTime only)\n> \n>        0     1     2     3     4     5     6     7     8     9\n> frame  +-----+-----+-----+-----+-----+-----+-----+-----+-----+-----+\n> \n> ET       8k    10k\n> sensor [init]\n>             [init]  [init] 8k\n>                     [init] 8k    10k\n> \n> Should we define \"per frame control\" as the expectations that:\n> - given no requests underrun (the application queues requests fast\n>   enough to have at least 'depth' requests in queue\n> \n> Controls associated with Request[x] will be applied at Frame[x + depth] ?\n\nAhh thati's interesting. My expectations are a bit different. (For\nthe sake of completeness I added an additional 5k a bit later in the\npipeline). I would expect the 8k to be lost. I'm not shure if understood\nyour lines below sensor correctly, so I squashed them into one line to\nrepresent the state that is active in the sensor.\n\nDavid mentioned a god reason why it makes sense to lose the 8k:\n\"Actually our implementation takes the 2nd approach even though I prefer\nthe first. The reason is that the coupling of requests and controls\nmeans you end up with a theoretically unbounded delay between them which\nis (theoretically) annoying to handle.\"\n\n\nSo my diagram would be:\n       0     1     2     3     4     5     6     7     8     9\nframe  +-----+-----+-----+-----+-----+-----+-----+-----+-----+-----+\n\nET       8k    10k                     5k\nsensor [init] [init] 10k               5k\n\n\n> \n> \n> > +}\n> > +\n> > +TEST_F(PerFrameControlTests, testExposureGainIsAppliedOnFirstFrame)\n> > +{\n> > +\tPerFrameControlsCapture 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 ts1 = capture.startCaptureWithTimeSheet(3, &startValues);\n> > +\n> > +\tcapture.runCaptureSession();\n> > +\n> > +\tASSERT_TRUE((*ts1)[0].metadata().contains(controls::ExposureTime.id()))\n> > +\t\t<< \"Required metadata entry is missing\";\n> > +\tASSERT_TRUE((*ts1)[0].metadata().contains(controls::AnalogueGain.id()))\n> > +\t\t<< \"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 = capture.startCaptureWithTimeSheet(3, &startValues);\n> > +\n> > +\tcapture.runCaptureSession();\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).spotBrightness() / ts1->get(1).spotBrightness();\n> > +\t\tEXPECT_GT(brightnessChange, 2.0);\n> > +\t}\n> > +}\n> > --\n> > 2.40.1\n> >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 E1FB8C3274\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 22 Mar 2024 13:15:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 92A9C63354;\n\tFri, 22 Mar 2024 14:15:28 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8A4D563079\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 22 Mar 2024 14:15:20 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:e5:ab7c:4271:bf1f])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B7199D50;\n\tFri, 22 Mar 2024 14:14: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=\"Ihq066Sk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1711113291;\n\tbh=/y94Thuc9CDv685yZ+ZUuS3/y5NDOqazFM8N5dj0n0M=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Ihq066Sk9SStZqe5jfs4eDpWeJc3s8QTTlAzQ8eEMI6L4fXHITHon607ZcNY2rnYs\n\tGSER0HwT6KwZ2M1XuzAFzi2AZpJeZ2cIKVItbZptvwmZaAVzmvqyaHOd+OMFJ5p/Yu\n\tVIza1RYzD7WUxO/1jnRw4GTRl2aONLGiAm8xmHNI=","Date":"Fri, 22 Mar 2024 14:15:17 +0100","From":"Stefan Klug <stefan.klug@ideasonboard.com>","To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v3 03/16] libcamera: lc-compliance: Add initial set of\n\tper-frame-control tests","Message-ID":"<20240322131517.waf2jh4oy7ojwdwm@jasper>","References":"<20240319120517.362082-1-stefan.klug@ideasonboard.com>\n\t<20240319120517.362082-4-stefan.klug@ideasonboard.com>\n\t<m64322mv4g3rdawau3x2uazbiat7qjfjtnor6xf5x2geeot53g@zu2h2l6oojxs>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<m64322mv4g3rdawau3x2uazbiat7qjfjtnor6xf5x2geeot53g@zu2h2l6oojxs>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":29041,"web_url":"https://patchwork.libcamera.org/comment/29041/","msgid":"<byonxuvl6kkea4dcx5zxo3q2tcebdix2wdfpam7eu2vti7mlsi@innxfkbgn7hm>","date":"2024-03-22T14:16:25","subject":"Re: [PATCH v3 03/16] 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 22, 2024 at 02:15:17PM +0100, Stefan Klug wrote:\n> Hi Jacopo,\n>\n> thanks for looking at the tests.\n>\n> On Fri, Mar 22, 2024 at 12:25:33PM +0100, Jacopo Mondi wrote:\n> > Hi Stefan\n> >\n> > On Tue, Mar 19, 2024 at 01:05:04PM +0100, Stefan Klug wrote:\n> > > Add tests that check if controls (only exposure time and analogue gain at the\n> > > moment) get applied on the frame they were requested for. This is tested by\n> > > looking at the metadata and the mean brightness of the image center. At the\n> > > moment these tests fail. Fixes for the pipelines will be delivered in later\n> > > patches (rkisp1 for now).\n> > >\n> > > To run the pfc tests only:\n> > > lc-compliance -c <cam> -f \"PerFrameControlTests.*\"\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> > > These tests are known to pass using a imx219 (RPi cam v2) with a imx8mp\n> > > (debix-som) using the rkisp1 pipeline\n> > >\n> > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>\n> > > ---\n> >\n> > [snip]\n> >\n> > > +\n> > > +TEST_F(PerFrameControlTests, testExposureGainChangeOnSameFrame)\n> > > +{\n> > > +\tPerFrameControlsCapture 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> > > +\tcapture.runCaptureSession();\n> > > +\n> > > +\tASSERT_TRUE(ts[5].metadata().contains(controls::ExposureTime.id()))\n> > > +\t\t<< \"Required metadata entry is missing\";\n> > > +\tASSERT_TRUE(ts[5].metadata().contains(controls::AnalogueGain.id()))\n> > > +\t\t<< \"Required metadata entry is missing\";\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> > > +\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> > > +\t\tint brightnessChangeIndex = 0;\n> > > +\t\tfor (unsigned i = 3; i < ts.size(); i++) {\n> > > +\t\t\tif (ts[i].brightnessChange() > 1.3) {\n> > > +\t\t\t\tEXPECT_EQ(brightnessChangeIndex, 0)\n> > > +\t\t\t\t\t<< \"Detected multiple frames with brightness increase\"\n> > > +\t\t\t\t\t<< \" (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 measured brightness change were not on same\"\n> > > +\t\t\t<< \" frame. (Wrong control delay?, Start frame event too late?)\";\n> > > +\t\tEXPECT_EQ(exposureChangeIndex, gainChangeIndex)\n> > > +\t\t\t<< \"Gain change and measured brightness change were not on same \"\n> > > +\t\t\t<< \" frame. (Wrong control delay?, Start frame event too late?)\";\n> > > +\t}\n> > > +}\n> >\n> > I think this is a valid test, we want to make sure exposure and gain\n> > change on the same frame\n> >\n> > > +\n> > > +TEST_F(PerFrameControlTests, testFramePreciseExposureChange)\n> > > +{\n> > > +\tPerFrameControlsCapture capture(camera_);\n> > > +\tcapture.configure(StreamRole::VideoRecording);\n> > > +\n> > > +\tauto timeSheet = capture.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> > > +\tcapture.runCaptureSession();\n> > > +\n> > > +\tASSERT_TRUE(ts[5].metadata().contains(controls::ExposureTime.id()))\n> > > +\t\t<< \"Required metadata entry is missing\";\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> > Why do you need near ? The camera can't get the precise exposure time ?\n>\n> Some quantization always happens and that is reported back in the\n> metadata. That raises the question: Does libcamera make any such promise?\n>\n> >\n> > I think the test is valid, however it uses absolute values (3, 6,\n> > 5000, 20000) while in future these should be parametrized (using the\n> > pipeline depth and the exposure min, max) but we already agree with\n> > this.\n> >\n> >\n> > > +\n> > > +\tif (doImageTests) {\n> > > +\t\t/* No increase just before setting exposure */\n> > > +\t\tEXPECT_NEAR(ts[5].brightnessChange(), 1.0, 0.05)\n> > > +\t\t\t<< \"Brightness changed too much before the expected time of change\"\n> > > +\t\t\t<< \" (control delay too high?).\";\n> > > +\t\t/*\n> > > +\t\t * \\todo The change is brightness was a bit low\n> > > +\t\t * (Exposure time increase by 4x resulted in a brightness increase of < 2).\n> > > +\t\t * This should be investigated.\n> > > +\t\t */\n> > > +\t\tEXPECT_GT(ts[6].brightnessChange(), 1.3)\n> > > +\t\t\t<< \"Brightness in frame \" << 6 << \" did not increase as expected\"\n> > > +\t\t\t<< \" (reference: \" << ts[3].spotBrightness() << \" current: \"\n> > > +\t\t\t<< ts[6].spotBrightness() << \" )\" << std::endl;\n> > > +\n> > > +\t\t/* No increase just after setting exposure */\n> > > +\t\tEXPECT_NEAR(ts[7].brightnessChange(), 1.0, 0.05)\n> > > +\t\t\t<< \"Brightness changed too much after the expected time of change\"\n> > > +\t\t\t<< \" (control delay too low?).\";\n> > > +\n> > > +\t\t/* No increase just after setting exposure */\n> > > +\t\tEXPECT_NEAR(ts[8].brightnessChange(), 1.0, 0.05)\n> > > +\t\t\t<< \"Brightness changed too much 2 frames after the expected time\"\n> > > +\t\t\t<< \" of change (control delay too low?).\";\n> > > +\t}\n> > > +}\n> > > +\n> > > +TEST_F(PerFrameControlTests, testFramePreciseGainChange)\n> > > +{\n> > > +\tPerFrameControlsCapture capture(camera_);\n> > > +\tcapture.configure(StreamRole::VideoRecording);\n> > > +\n> > > +\tauto timeSheet = capture.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> > > +\tcapture.runCaptureSession();\n> > > +\n> > > +\tASSERT_TRUE(ts[5].metadata().contains(controls::AnalogueGain.id()))\n> > > +\t\t<< \"Required metadata entry is missing\";\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> >\n> > Same comment as the for the above test.\n> >\n> > > +\tif (doImageTests) {\n> > > +\t\t/* No increase just before setting gain */\n> > > +\t\tEXPECT_NEAR(ts[5].brightnessChange(), 1.0, 0.05)\n> > > +\t\t\t<< \"Brightness changed too much before the expected time of change\"\n> > > +\t\t\t<< \" (control delay too high?).\";\n> > > +\t\t/*\n> > > +\t\t * \\todo I see a brightness change of roughly half the expected one.\n> > > +\t\t * This is not yet understood and needs investigation\n> > > +\t\t */\n> > > +\t\tEXPECT_GT(ts[6].brightnessChange(), 1.7)\n> > > +\t\t\t<< \"Brightness in frame \" << 6 << \" did not increase as expected\"\n> > > +\t\t\t<< \" (reference: \" << ts[5].spotBrightness()\n> > > +\t\t\t<< \" current: \" << ts[6].spotBrightness() << \")\" << std::endl;\n> > > +\n> > > +\t\t/* No increase just after setting gain */\n> > > +\t\tEXPECT_NEAR(ts[7].brightnessChange(), 1.0, 0.05)\n> > > +\t\t\t<< \"Brightness changed too much after the expected time of change\"\n> > > +\t\t\t<< \" (control delay too low?).\";\n> > > +\n> > > +\t\t/* No increase just after setting gain */\n> > > +\t\tEXPECT_NEAR(ts[8].brightnessChange(), 1.0, 0.05)\n> > > +\t\t\t<< \"Brightness changed too much after the expected time of change\"\n> > > +\t\t\t<< \" (control delay too low?).\";\n> > > +\t}\n> > > +}\n> > > +\n> > > +TEST_F(PerFrameControlTests, testExposureGainFromFirstRequestGetsApplied)\n> > > +{\n> > > +\tPerFrameControlsCapture capture(camera_);\n> > > +\tcapture.configure(StreamRole::VideoRecording);\n> > > +\n> > > +\tauto timeSheet = capture.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> > > +\tcapture.runCaptureSession();\n> > > +\n> > > +\tASSERT_TRUE(ts[4].metadata().contains(controls::ExposureTime.id()))\n> > > +\t\t<< \"Required metadata entry is missing\";\n> > > +\tASSERT_TRUE(ts[4].metadata().contains(controls::AnalogueGain.id()))\n> > > +\t\t<< \"Required metadata entry is missing\";\n> > > +\n> > > +\t/* We expect it to be applied after 3 frames, the latest*/\n> >\n> > Should the 'startup' frames be only the sensor delays or the full\n> > pipeline depth ?\n>\n> That depends on the other discussions. My (personal) expectation would be\n> that only the sensor delay applies as we are running in manual mode.\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> > > +TEST_F(PerFrameControlTests, testExposureGainFromFirstAndSecondRequestGetsApplied)\n> > > +{\n> > > +\tPerFrameControlsCapture capture(camera_);\n> > > +\tcapture.configure(StreamRole::VideoRecording);\n> > > +\n> > > +\tauto timeSheet = capture.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> > > +\tcapture.runCaptureSession();\n> > > +\n> > > +\tASSERT_TRUE(ts[4].metadata().contains(controls::ExposureTime.id()))\n> > > +\t\t<< \"Required metadata entry is missing\";\n> > > +\tASSERT_TRUE(ts[4].metadata().contains(controls::AnalogueGain.id()))\n> > > +\t\t<< \"Required metadata entry is missing\";\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> > Eh, interesting. How should we expect the startup sequence to be\n> > handled ?\n> >\n> > I presume ts[0] is not applied before streaming is started, right ?\n>\n> yes.\n>\n> >\n> > Let me graph your test expectations (for ExposureTime only)\n> >\n> >        0     1     2     3     4     5     6     7     8     9\n> > frame  +-----+-----+-----+-----+-----+-----+-----+-----+-----+-----+\n> >\n> > ET       8k    10k\n> > sensor [init]\n> >             [init]  [init] 8k\n> >                     [init] 8k    10k\n> >\n> > Should we define \"per frame control\" as the expectations that:\n> > - given no requests underrun (the application queues requests fast\n> >   enough to have at least 'depth' requests in queue\n> >\n> > Controls associated with Request[x] will be applied at Frame[x + depth] ?\n>\n> Ahh thati's interesting. My expectations are a bit different. (For\n> the sake of completeness I added an additional 5k a bit later in the\n> pipeline). I would expect the 8k to be lost. I'm not shure if understood\n> your lines below sensor correctly, so I squashed them into one line to\n> represent the state that is active in the sensor.\n>\n> David mentioned a god reason why it makes sense to lose the 8k:\n> \"Actually our implementation takes the 2nd approach even though I prefer\n> the first. The reason is that the coupling of requests and controls\n> means you end up with a theoretically unbounded delay between them which\n> is (theoretically) annoying to handle.\"\n>\n>\n> So my diagram would be:\n>        0     1     2     3     4     5     6     7     8     9\n> frame  +-----+-----+-----+-----+-----+-----+-----+-----+-----+-----+\n>\n> ET       8k    10k                     5k\n> sensor [init] [init] 10k               5k\n>\n\nMaybe I should clarify the meaning of the lines first:\n\nEW: time at wich we setControl() on the subdev\nsensor: Effective control value\n\nAnd maybe I was considering a delay of 2 and it's just 1 instead ?\n\nAnyway, even if the delay is one frame, isn't this:\n\n\n        0     1     2     3     4     5     6     7     8     9\n frame  +-----+-----+-----+-----+-----+-----+-----+-----+-----+-----+\n\n ET       8k    10k                     5k\n sensor [init] [init] 8k    10k   10k   10k   10k   5k\n\n\nso in my undertanding, with a delay of 1, if we setControl() while\nframe X is being exposed, the setting gets applied to the next frame\n(X + 1) and has a delay of 1 to have it realized (X + 2)\n\nWhat am I missing ?\n\n>\n> >\n> >\n> > > +}\n> > > +\n> > > +TEST_F(PerFrameControlTests, testExposureGainIsAppliedOnFirstFrame)\n> > > +{\n> > > +\tPerFrameControlsCapture 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 ts1 = capture.startCaptureWithTimeSheet(3, &startValues);\n> > > +\n> > > +\tcapture.runCaptureSession();\n> > > +\n> > > +\tASSERT_TRUE((*ts1)[0].metadata().contains(controls::ExposureTime.id()))\n> > > +\t\t<< \"Required metadata entry is missing\";\n> > > +\tASSERT_TRUE((*ts1)[0].metadata().contains(controls::AnalogueGain.id()))\n> > > +\t\t<< \"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 = capture.startCaptureWithTimeSheet(3, &startValues);\n> > > +\n> > > +\tcapture.runCaptureSession();\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).spotBrightness() / ts1->get(1).spotBrightness();\n> > > +\t\tEXPECT_GT(brightnessChange, 2.0);\n> > > +\t}\n> > > +}\n> > > --\n> > > 2.40.1\n> > >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 64E40BD160\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 22 Mar 2024 14:16:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1967F63055;\n\tFri, 22 Mar 2024 15:16:30 +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 9A19361C45\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 22 Mar 2024 15:16:28 +0100 (CET)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A9B4D82A;\n\tFri, 22 Mar 2024 15:15:59 +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=\"bNfaQCPK\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1711116959;\n\tbh=hojv1ePyb2aKvuveuQmJKSR6asrjWHGNxDV7qQ8X5v0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=bNfaQCPKK7j3PVfRZWssdGoPpX6RSR13prPAlO8bzsobzE2AbzaaqE/YhPOUHt7gz\n\tn9vwxMJfe262O9/dVJtZgJvrrKDhMNgEXV9efRxOYt0BVpuBFIeOsnRlByHRb+ADIp\n\tSSDKY5Ck9bca7qSsvtwM/DxcLkNQud6Xha+cv4qU=","Date":"Fri, 22 Mar 2024 15:16:25 +0100","From":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","To":"Stefan Klug <stefan.klug@ideasonboard.com>","Cc":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>, \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v3 03/16] libcamera: lc-compliance: Add initial set of\n\tper-frame-control tests","Message-ID":"<byonxuvl6kkea4dcx5zxo3q2tcebdix2wdfpam7eu2vti7mlsi@innxfkbgn7hm>","References":"<20240319120517.362082-1-stefan.klug@ideasonboard.com>\n\t<20240319120517.362082-4-stefan.klug@ideasonboard.com>\n\t<m64322mv4g3rdawau3x2uazbiat7qjfjtnor6xf5x2geeot53g@zu2h2l6oojxs>\n\t<20240322131517.waf2jh4oy7ojwdwm@jasper>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240322131517.waf2jh4oy7ojwdwm@jasper>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":29044,"web_url":"https://patchwork.libcamera.org/comment/29044/","msgid":"<20240322145932.ctwikyypsi24pjsm@jasper>","date":"2024-03-22T14:59:32","subject":"Re: [PATCH v3 03/16] 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 Fri, Mar 22, 2024 at 03:16:25PM +0100, Jacopo Mondi wrote:\n> Hi Stefan\n> \n> On Fri, Mar 22, 2024 at 02:15:17PM +0100, Stefan Klug wrote:\n> > Hi Jacopo,\n> >\n> > thanks for looking at the tests.\n> >\n> > On Fri, Mar 22, 2024 at 12:25:33PM +0100, Jacopo Mondi wrote:\n> > > Hi Stefan\n> > >\n> > > On Tue, Mar 19, 2024 at 01:05:04PM +0100, Stefan Klug wrote:\n> > > > Add tests that check if controls (only exposure time and analogue gain at the\n> > > > moment) get applied on the frame they were requested for. This is tested by\n> > > > looking at the metadata and the mean brightness of the image center. At the\n> > > > moment these tests fail. Fixes for the pipelines will be delivered in later\n> > > > patches (rkisp1 for now).\n> > > >\n> > > > To run the pfc tests only:\n> > > > lc-compliance -c <cam> -f \"PerFrameControlTests.*\"\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> > > > These tests are known to pass using a imx219 (RPi cam v2) with a imx8mp\n> > > > (debix-som) using the rkisp1 pipeline\n> > > >\n> > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>\n> > > > ---\n> > >\n> > > [snip]\n> > >\n> > > > +\n> > > > +TEST_F(PerFrameControlTests, testExposureGainChangeOnSameFrame)\n> > > > +{\n> > > > +\tPerFrameControlsCapture 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> > > > +\tcapture.runCaptureSession();\n> > > > +\n> > > > +\tASSERT_TRUE(ts[5].metadata().contains(controls::ExposureTime.id()))\n> > > > +\t\t<< \"Required metadata entry is missing\";\n> > > > +\tASSERT_TRUE(ts[5].metadata().contains(controls::AnalogueGain.id()))\n> > > > +\t\t<< \"Required metadata entry is missing\";\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> > > > +\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> > > > +\t\tint brightnessChangeIndex = 0;\n> > > > +\t\tfor (unsigned i = 3; i < ts.size(); i++) {\n> > > > +\t\t\tif (ts[i].brightnessChange() > 1.3) {\n> > > > +\t\t\t\tEXPECT_EQ(brightnessChangeIndex, 0)\n> > > > +\t\t\t\t\t<< \"Detected multiple frames with brightness increase\"\n> > > > +\t\t\t\t\t<< \" (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 measured brightness change were not on same\"\n> > > > +\t\t\t<< \" frame. (Wrong control delay?, Start frame event too late?)\";\n> > > > +\t\tEXPECT_EQ(exposureChangeIndex, gainChangeIndex)\n> > > > +\t\t\t<< \"Gain change and measured brightness change were not on same \"\n> > > > +\t\t\t<< \" frame. (Wrong control delay?, Start frame event too late?)\";\n> > > > +\t}\n> > > > +}\n> > >\n> > > I think this is a valid test, we want to make sure exposure and gain\n> > > change on the same frame\n> > >\n> > > > +\n> > > > +TEST_F(PerFrameControlTests, testFramePreciseExposureChange)\n> > > > +{\n> > > > +\tPerFrameControlsCapture capture(camera_);\n> > > > +\tcapture.configure(StreamRole::VideoRecording);\n> > > > +\n> > > > +\tauto timeSheet = capture.startCaptureWithTimeSheet(10);\n> > > > +\tauto &ts = *timeSheet;\n> > > > +\n> > > > +\tts[3].controls().set(controls::ExposureTime, 5000);\n\n> > > > +\t/* wait a few frames to settle */\n> > > > +\tts[6].controls().set(controls::ExposureTime, 20000);\n> > > > +\n> > > > +\tcapture.runCaptureSession();\n> > > > +\n> > > > +\tASSERT_TRUE(ts[5].metadata().contains(controls::ExposureTime.id()))\n> > > > +\t\t<< \"Required metadata entry is missing\";\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> > > Why do you need near ? The camera can't get the precise exposure time ?\n> >\n> > Some quantization always happens and that is reported back in the\n> > metadata. That raises the question: Does libcamera make any such promise?\n> >\n> > >\n> > > I think the test is valid, however it uses absolute values (3, 6,\n> > > 5000, 20000) while in future these should be parametrized (using the\n> > > pipeline depth and the exposure min, max) but we already agree with\n> > > this.\n> > >\n> > >\n> > > > +\n> > > > +\tif (doImageTests) {\n> > > > +\t\t/* No increase just before setting exposure */\n> > > > +\t\tEXPECT_NEAR(ts[5].brightnessChange(), 1.0, 0.05)\n> > > > +\t\t\t<< \"Brightness changed too much before the expected time of change\"\n> > > > +\t\t\t<< \" (control delay too high?).\";\n> > > > +\t\t/*\n> > > > +\t\t * \\todo The change is brightness was a bit low\n> > > > +\t\t * (Exposure time increase by 4x resulted in a brightness increase of < 2).\n> > > > +\t\t * This should be investigated.\n> > > > +\t\t */\n> > > > +\t\tEXPECT_GT(ts[6].brightnessChange(), 1.3)\n> > > > +\t\t\t<< \"Brightness in frame \" << 6 << \" did not increase as expected\"\n> > > > +\t\t\t<< \" (reference: \" << ts[3].spotBrightness() << \" current: \"\n> > > > +\t\t\t<< ts[6].spotBrightness() << \" )\" << std::endl;\n> > > > +\n> > > > +\t\t/* No increase just after setting exposure */\n> > > > +\t\tEXPECT_NEAR(ts[7].brightnessChange(), 1.0, 0.05)\n> > > > +\t\t\t<< \"Brightness changed too much after the expected time of change\"\n> > > > +\t\t\t<< \" (control delay too low?).\";\n> > > > +\n> > > > +\t\t/* No increase just after setting exposure */\n> > > > +\t\tEXPECT_NEAR(ts[8].brightnessChange(), 1.0, 0.05)\n> > > > +\t\t\t<< \"Brightness changed too much 2 frames after the expected time\"\n> > > > +\t\t\t<< \" of change (control delay too low?).\";\n> > > > +\t}\n> > > > +}\n> > > > +\n> > > > +TEST_F(PerFrameControlTests, testFramePreciseGainChange)\n> > > > +{\n> > > > +\tPerFrameControlsCapture capture(camera_);\n> > > > +\tcapture.configure(StreamRole::VideoRecording);\n> > > > +\n> > > > +\tauto timeSheet = capture.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> > > > +\tcapture.runCaptureSession();\n> > > > +\n> > > > +\tASSERT_TRUE(ts[5].metadata().contains(controls::AnalogueGain.id()))\n> > > > +\t\t<< \"Required metadata entry is missing\";\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> > >\n> > > Same comment as the for the above test.\n> > >\n> > > > +\tif (doImageTests) {\n> > > > +\t\t/* No increase just before setting gain */\n> > > > +\t\tEXPECT_NEAR(ts[5].brightnessChange(), 1.0, 0.05)\n> > > > +\t\t\t<< \"Brightness changed too much before the expected time of change\"\n> > > > +\t\t\t<< \" (control delay too high?).\";\n> > > > +\t\t/*\n> > > > +\t\t * \\todo I see a brightness change of roughly half the expected one.\n> > > > +\t\t * This is not yet understood and needs investigation\n> > > > +\t\t */\n> > > > +\t\tEXPECT_GT(ts[6].brightnessChange(), 1.7)\n> > > > +\t\t\t<< \"Brightness in frame \" << 6 << \" did not increase as expected\"\n> > > > +\t\t\t<< \" (reference: \" << ts[5].spotBrightness()\n> > > > +\t\t\t<< \" current: \" << ts[6].spotBrightness() << \")\" << std::endl;\n> > > > +\n> > > > +\t\t/* No increase just after setting gain */\n> > > > +\t\tEXPECT_NEAR(ts[7].brightnessChange(), 1.0, 0.05)\n> > > > +\t\t\t<< \"Brightness changed too much after the expected time of change\"\n> > > > +\t\t\t<< \" (control delay too low?).\";\n> > > > +\n> > > > +\t\t/* No increase just after setting gain */\n> > > > +\t\tEXPECT_NEAR(ts[8].brightnessChange(), 1.0, 0.05)\n> > > > +\t\t\t<< \"Brightness changed too much after the expected time of change\"\n> > > > +\t\t\t<< \" (control delay too low?).\";\n> > > > +\t}\n> > > > +}\n> > > > +\n> > > > +TEST_F(PerFrameControlTests, testExposureGainFromFirstRequestGetsApplied)\n> > > > +{\n> > > > +\tPerFrameControlsCapture capture(camera_);\n> > > > +\tcapture.configure(StreamRole::VideoRecording);\n> > > > +\n> > > > +\tauto timeSheet = capture.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> > > > +\tcapture.runCaptureSession();\n> > > > +\n> > > > +\tASSERT_TRUE(ts[4].metadata().contains(controls::ExposureTime.id()))\n> > > > +\t\t<< \"Required metadata entry is missing\";\n> > > > +\tASSERT_TRUE(ts[4].metadata().contains(controls::AnalogueGain.id()))\n> > > > +\t\t<< \"Required metadata entry is missing\";\n> > > > +\n> > > > +\t/* We expect it to be applied after 3 frames, the latest*/\n> > >\n> > > Should the 'startup' frames be only the sensor delays or the full\n> > > pipeline depth ?\n> >\n> > That depends on the other discussions. My (personal) expectation would be\n> > that only the sensor delay applies as we are running in manual mode.\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> > > > +TEST_F(PerFrameControlTests, testExposureGainFromFirstAndSecondRequestGetsApplied)\n> > > > +{\n> > > > +\tPerFrameControlsCapture capture(camera_);\n> > > > +\tcapture.configure(StreamRole::VideoRecording);\n> > > > +\n> > > > +\tauto timeSheet = capture.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> > > > +\tcapture.runCaptureSession();\n> > > > +\n> > > > +\tASSERT_TRUE(ts[4].metadata().contains(controls::ExposureTime.id()))\n> > > > +\t\t<< \"Required metadata entry is missing\";\n> > > > +\tASSERT_TRUE(ts[4].metadata().contains(controls::AnalogueGain.id()))\n> > > > +\t\t<< \"Required metadata entry is missing\";\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> > > Eh, interesting. How should we expect the startup sequence to be\n> > > handled ?\n> > >\n> > > I presume ts[0] is not applied before streaming is started, right ?\n> >\n> > yes.\n> >\n> > >\n> > > Let me graph your test expectations (for ExposureTime only)\n> > >\n> > >        0     1     2     3     4     5     6     7     8     9\n> > > frame  +-----+-----+-----+-----+-----+-----+-----+-----+-----+-----+\n> > >\n> > > ET       8k    10k\n> > > sensor [init]\n> > >             [init]  [init] 8k\n> > >                     [init] 8k    10k\n> > >\n> > > Should we define \"per frame control\" as the expectations that:\n> > > - given no requests underrun (the application queues requests fast\n> > >   enough to have at least 'depth' requests in queue\n> > >\n> > > Controls associated with Request[x] will be applied at Frame[x + depth] ?\n> >\n> > Ahh thati's interesting. My expectations are a bit different. (For\n> > the sake of completeness I added an additional 5k a bit later in the\n> > pipeline). I would expect the 8k to be lost. I'm not shure if understood\n> > your lines below sensor correctly, so I squashed them into one line to\n> > represent the state that is active in the sensor.\n> >\n> > David mentioned a god reason why it makes sense to lose the 8k:\n> > \"Actually our implementation takes the 2nd approach even though I prefer\n> > the first. The reason is that the coupling of requests and controls\n> > means you end up with a theoretically unbounded delay between them which\n> > is (theoretically) annoying to handle.\"\n> >\n> >\n> > So my diagram would be:\n> >        0     1     2     3     4     5     6     7     8     9\n> > frame  +-----+-----+-----+-----+-----+-----+-----+-----+-----+-----+\n> >\n> > ET       8k    10k                     5k\n> > sensor [init] [init] 10k               5k\n> >\n> \n> Maybe I should clarify the meaning of the lines first:\n> \n> EW: time at wich we setControl() on the subdev\n> sensor: Effective control value\n> \n> And maybe I was considering a delay of 2 and it's just 1 instead ?\n> \n> Anyway, even if the delay is one frame, isn't this:\n> \n> \n>         0     1     2     3     4     5     6     7     8     9\n>  frame  +-----+-----+-----+-----+-----+-----+-----+-----+-----+-----+\n> \n>  ET       8k    10k                     5k\n>  sensor [init] [init] 8k    10k   10k   10k   10k   5k\n> \n> \n> so in my undertanding, with a delay of 1, if we setControl() while\n> frame X is being exposed, the setting gets applied to the next frame\n> (X + 1) and has a delay of 1 to have it realized (X + 2)\n> \n> What am I missing ?\n\nAhh I think I got it. I misinterpreted the ET line as the ExposureTime\non the corresponsing request. \n\nI assume that roughly at start of exposure of frame 0 all requests are\nqueued in. So the system can look ahead in the requests. I further\nassume that the EW value for column 0 is the one written in response to\nthe SoF signal for frame 0 (at that point in time, all/enough requests\nare queued). I added a R line to show the requests:\n\n       0     1     2     3     4     5     6     7     8     9\nframe  +-----+-----+-----+-----+-----+-----+-----+-----+-----+-----+\n\nR\t 8k    10k                     5k\nET      10k                 5k\nsensor [init] [init] 10k               5k\n\n\n> \n> >\n> > >\n> > >\n> > > > +}\n> > > > +\n> > > > +TEST_F(PerFrameControlTests, testExposureGainIsAppliedOnFirstFrame)\n> > > > +{\n> > > > +\tPerFrameControlsCapture 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 ts1 = capture.startCaptureWithTimeSheet(3, &startValues);\n> > > > +\n> > > > +\tcapture.runCaptureSession();\n> > > > +\n> > > > +\tASSERT_TRUE((*ts1)[0].metadata().contains(controls::ExposureTime.id()))\n> > > > +\t\t<< \"Required metadata entry is missing\";\n> > > > +\tASSERT_TRUE((*ts1)[0].metadata().contains(controls::AnalogueGain.id()))\n> > > > +\t\t<< \"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 = capture.startCaptureWithTimeSheet(3, &startValues);\n> > > > +\n> > > > +\tcapture.runCaptureSession();\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).spotBrightness() / ts1->get(1).spotBrightness();\n> > > > +\t\tEXPECT_GT(brightnessChange, 2.0);\n> > > > +\t}\n> > > > +}\n> > > > --\n> > > > 2.40.1\n> > > >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 08A21C3272\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 22 Mar 2024 14:59:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 560EA63075;\n\tFri, 22 Mar 2024 15:59:36 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EFEE263036\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 22 Mar 2024 15:59:34 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:e5:ab7c:4271:bf1f])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EA55F82A;\n\tFri, 22 Mar 2024 15:59:05 +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=\"TcDzKUPU\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1711119546;\n\tbh=XsDmx4tySMmWy9fthZHNPd17q13RxZmF0kdt6pkHK6Q=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=TcDzKUPUVi897qrY2rLneoRsgdcXY20V1GDs86UNa3HuPpLvFF2riLx46WZ/nKn8k\n\t8FDreNLyeaQsMYpxwP4RkJ3y5k0OgI46lMtPeyvDQJRmHkjp8CX9zHRQekcg2ayZwG\n\tpqL910ROPVZoeK7xEkqoX2E4wAgeY1KAKDnXhMuc=","Date":"Fri, 22 Mar 2024 15:59:32 +0100","From":"Stefan Klug <stefan.klug@ideasonboard.com>","To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v3 03/16] libcamera: lc-compliance: Add initial set of\n\tper-frame-control tests","Message-ID":"<20240322145932.ctwikyypsi24pjsm@jasper>","References":"<20240319120517.362082-1-stefan.klug@ideasonboard.com>\n\t<20240319120517.362082-4-stefan.klug@ideasonboard.com>\n\t<m64322mv4g3rdawau3x2uazbiat7qjfjtnor6xf5x2geeot53g@zu2h2l6oojxs>\n\t<20240322131517.waf2jh4oy7ojwdwm@jasper>\n\t<byonxuvl6kkea4dcx5zxo3q2tcebdix2wdfpam7eu2vti7mlsi@innxfkbgn7hm>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<byonxuvl6kkea4dcx5zxo3q2tcebdix2wdfpam7eu2vti7mlsi@innxfkbgn7hm>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]