[{"id":16934,"web_url":"https://patchwork.libcamera.org/comment/16934/","msgid":"<20210513101744.w4vowg6oqjwegqka@uno.localdomain>","date":"2021-05-13T10:17:44","subject":"Re: [libcamera-devel] [PATCH v4 4/6] libcamera: CameraSensor:\n\tEnable retrieving supported test pattern modes","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n\nOn Thu, May 06, 2021 at 04:54:47PM +0900, Hirokazu Honda wrote:\n> This enables retrieving supported test pattern modes through\n> CameraSensorInfo.\n\nI'm afraid this hits a shortcoming of the current CameraSensor class.\n\nIn order:\n- The CameraSensor class accepts lists V4L2 controls in get/setControls\n- The CameraSensor class exposes libcamera::properties through\n properties()\n- The CameraSensor class exposes the -subdev- V4L2 controls through\n controls()\n- The CameraSensor class allows to get a snapshot of the current\n sensor configuration through CameraSensorInfo (mostly for IPAs, in\n facts the structure definition is in the process of being moved to\n the IPA headers).\n\nSo we have quite some confusion there, and we need to design better\nthe control interface for the class.\n\nIn the meantime, items like test patters have not a real place where\nthey belong:\n\n- We can't expose the raw V4L2 control indices and let the ph\n translate them to libcamera controls like we do with exposures, crop\n rectangles and such, as the translation requires accessing the\n sensor database, which is not (ideally) exposed to ph\n- We can't model them as properties, the supported test patters are static\n information but they can be enabled/disabled, something we don't\n allow for properties (at least conceptually).\n- CameraSensorInfo reports the current configuration parameters, it\n would be fair to report if test pattern is enabled or disabled, but\n the list of supported patterns does not really belong there.\n\nwe've discussed the issue with the group, and for the time being it is\nprobably easier to create and ad-hoc methods along the lines of\nCameraSensor::testPatternModes() to retrieve them. It's not ideal but\nit's probably the easiest decision to safely back-track later on.\n\nSorry for conflicting directions, the CameraSensor class control\ninterface will need to be re-thought with this new use-case in mind.\n\nThanks\n j\n\n\n>\n> Signed-off-by: Hirokazu Honda \n> ---\n> include/libcamera/internal/camera_sensor.h | 5 ++++\n> src/libcamera/camera_sensor.cpp | 28 ++++++++++++++++++++++\n> 2 files changed, 33 insertions(+)\n>\n> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> index 3fa3a419..115e266d 100644\n> --- a/include/libcamera/internal/camera_sensor.h\n> +++ b/include/libcamera/internal/camera_sensor.h\n> @@ -38,6 +38,8 @@ struct CameraSensorInfo {\n>\n> \tuint32_t minFrameLength;\n> \tuint32_t maxFrameLength;\n> +\n> +\tstd::vector testPatternModes;\n> };\n>\n> class CameraSensor : protected Loggable\n> @@ -79,6 +81,8 @@ private:\n> \tvoid initVimcDefaultProperties();\n> \tvoid initStaticProperties();\n> \tint initProperties();\n> +\tvoid initTestPatternModes(\n> +\t\tconst std::map &testPatternModeMap);\n>\n> \tconst MediaEntity *entity_;\n> \tstd::unique_ptr subdev_;\n> @@ -90,6 +94,7 @@ private:\n> \tV4L2Subdevice::Formats formats_;\n> \tstd::vector mbusCodes_;\n> \tstd::vector sizes_;\n> +\tstd::vector testPatternModes_;\n>\n> \tSize pixelArraySize_;\n> \tRectangle activeArea_;\n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index 1db263cf..eedd2f89 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -408,6 +408,32 @@ void CameraSensor::initVimcDefaultProperties()\n> \tactiveArea_ = Rectangle(pixelArraySize_);\n> }\n>\n> +void CameraSensor::initTestPatternModes(\n> +\tconst std::map &testPatternModeMap)\n> +{\n> +\tconst auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);\n> +\tif (v4l2TestPattern == controls().end()) {\n> +\t\tLOG(CameraSensor, Debug) << \"No static test pattern map for \\'\"\n> +\t\t\t\t\t << model() << \"\\'\";\n> +\t\treturn;\n> +\t}\n> +\n> +\tfor (const ControlValue &value : v4l2TestPattern->second.values()) {\n> +\t\tconst int32_t index = value.get();\n> +\n> +\t\tconst auto it = testPatternModeMap.find(index);\n> +\t\tif (it != testPatternModeMap.end()) {\n> +\t\t\tLOG(CameraSensor, Debug) << \"Test pattern mode (index=\"\n> +\t\t\t\t\t\t << index << \") ignored\";\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\tLOG(CameraSensor, Debug) << \"Test pattern mode (index=\"\n> +\t\t\t\t\t << index << \") added\";\n> +\t\ttestPatternModes_.push_back(it->second);\n> +\t}\n> +}\n> +\n> void CameraSensor::initStaticProperties()\n> {\n> \tconst CameraSensorProperties *props = CameraSensorProperties::get(model_);\n> @@ -416,6 +442,8 @@ void CameraSensor::initStaticProperties()\n>\n> \t/* Register the properties retrieved from the sensor database. */\n> \tproperties_.set(properties::UnitCellSize, props->unitCellSize);\n> +\n> +\tinitTestPatternModes(props->testPatternModeMap);\n> }\n>\n> int CameraSensor::initProperties()\n> --\n> 2.31.1.607.g51e8a6a459-goog\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 792F6C31EB\n\tfor ;\n\tThu, 13 May 2021 10:17:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DC57F6891A;\n\tThu, 13 May 2021 12:17:02 +0200 (CEST)","from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 442976153C\n\tfor ;\n\tThu, 13 May 2021 12:17:01 +0200 (CEST)","from uno.localdomain (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 8D4151C0004;\n\tThu, 13 May 2021 10:17:00 +0000 (UTC)"],"X-Originating-IP":"93.61.96.190","Date":"Thu, 13 May 2021 12:17:44 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20210513101744.w4vowg6oqjwegqka@uno.localdomain>","References":"<20210506075449.1761752-1-hiroh@chromium.org>\n\t<20210506075449.1761752-5-hiroh@chromium.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210506075449.1761752-5-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v4 4/6] libcamera: CameraSensor:\n\tEnable retrieving supported test pattern modes","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","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16935,"web_url":"https://patchwork.libcamera.org/comment/16935/","msgid":"","date":"2021-05-14T03:19:51","subject":"Re: [libcamera-devel] [PATCH v4 4/6] libcamera: CameraSensor:\n\tEnable retrieving supported test pattern modes","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Jacopo,\n\nOn Thu, May 13, 2021 at 7:17 PM Jacopo Mondi wrote:\n\n> Hi Hiro,\n>\n> On Thu, May 06, 2021 at 04:54:47PM +0900, Hirokazu Honda wrote:\n> > This enables retrieving supported test pattern modes through\n> > CameraSensorInfo.\n>\n> I'm afraid this hits a shortcoming of the current CameraSensor class.\n>\n> In order:\n> - The CameraSensor class accepts lists V4L2 controls in get/setControls\n> - The CameraSensor class exposes libcamera::properties through\n> properties()\n> - The CameraSensor class exposes the -subdev- V4L2 controls through\n> controls()\n> - The CameraSensor class allows to get a snapshot of the current\n> sensor configuration through CameraSensorInfo (mostly for IPAs, in\n> facts the structure definition is in the process of being moved to\n> the IPA headers).\n>\n> So we have quite some confusion there, and we need to design better\n> the control interface for the class.\n>\n> In the meantime, items like test patters have not a real place where\n> they belong:\n>\n> - We can't expose the raw V4L2 control indices and let the ph\n> translate them to libcamera controls like we do with exposures, crop\n> rectangles and such, as the translation requires accessing the\n> sensor database, which is not (ideally) exposed to ph\n> - We can't model them as properties, the supported test patters are static\n> information but they can be enabled/disabled, something we don't\n> allow for properties (at least conceptually).\n> - CameraSensorInfo reports the current configuration parameters, it\n> would be fair to report if test pattern is enabled or disabled, but\n> the list of supported patterns does not really belong there.\n>\n> we've discussed the issue with the group, and for the time being it is\n> probably easier to create and ad-hoc methods along the lines of\n> CameraSensor::testPatternModes() to retrieve them. It's not ideal but\n> it's probably the easiest decision to safely back-track later on.\n>\n> Sorry for conflicting directions, the CameraSensor class control\n> interface will need to be re-thought with this new use-case in mind.\n>\n>\nI see. Can you review other patches in this series assuming\nCameraSensor::testPatternModes()?\n\n -Hiro\n\nThanks\n> j\n>\n>\n> >\n> > Signed-off-by: Hirokazu Honda \n> > ---\n> > include/libcamera/internal/camera_sensor.h | 5 ++++\n> > src/libcamera/camera_sensor.cpp | 28 ++++++++++++++++++++++\n> > 2 files changed, 33 insertions(+)\n> >\n> > diff --git a/include/libcamera/internal/camera_sensor.h\n> b/include/libcamera/internal/camera_sensor.h\n> > index 3fa3a419..115e266d 100644\n> > --- a/include/libcamera/internal/camera_sensor.h\n> > +++ b/include/libcamera/internal/camera_sensor.h\n> > @@ -38,6 +38,8 @@ struct CameraSensorInfo {\n> >\n> > uint32_t minFrameLength;\n> > uint32_t maxFrameLength;\n> > +\n> > + std::vector testPatternModes;\n> > };\n> >\n> > class CameraSensor : protected Loggable\n> > @@ -79,6 +81,8 @@ private:\n> > void initVimcDefaultProperties();\n> > void initStaticProperties();\n> > int initProperties();\n> > + void initTestPatternModes(\n> > + const std::map &testPatternModeMap);\n> >\n> > const MediaEntity *entity_;\n> > std::unique_ptr subdev_;\n> > @@ -90,6 +94,7 @@ private:\n> > V4L2Subdevice::Formats formats_;\n> > std::vector mbusCodes_;\n> > std::vector sizes_;\n> > + std::vector testPatternModes_;\n> >\n> > Size pixelArraySize_;\n> > Rectangle activeArea_;\n> > diff --git a/src/libcamera/camera_sensor.cpp\n> b/src/libcamera/camera_sensor.cpp\n> > index 1db263cf..eedd2f89 100644\n> > --- a/src/libcamera/camera_sensor.cpp\n> > +++ b/src/libcamera/camera_sensor.cpp\n> > @@ -408,6 +408,32 @@ void CameraSensor::initVimcDefaultProperties()\n> > activeArea_ = Rectangle(pixelArraySize_);\n> > }\n> >\n> > +void CameraSensor::initTestPatternModes(\n> > + const std::map &testPatternModeMap)\n> > +{\n> > + const auto &v4l2TestPattern =\n> controls().find(V4L2_CID_TEST_PATTERN);\n> > + if (v4l2TestPattern == controls().end()) {\n> > + LOG(CameraSensor, Debug) << \"No static test pattern map\n> for \\'\"\n> > + << model() << \"\\'\";\n> > + return;\n> > + }\n> > +\n> > + for (const ControlValue &value : v4l2TestPattern->second.values())\n> {\n> > + const int32_t index = value.get();\n> > +\n> > + const auto it = testPatternModeMap.find(index);\n> > + if (it != testPatternModeMap.end()) {\n> > + LOG(CameraSensor, Debug) << \"Test pattern mode\n> (index=\"\n> > + << index << \") ignored\";\n> > + continue;\n> > + }\n> > +\n> > + LOG(CameraSensor, Debug) << \"Test pattern mode (index=\"\n> > + << index << \") added\";\n> > + testPatternModes_.push_back(it->second);\n> > + }\n> > +}\n> > +\n> > void CameraSensor::initStaticProperties()\n> > {\n> > const CameraSensorProperties *props =\n> CameraSensorProperties::get(model_);\n> > @@ -416,6 +442,8 @@ void CameraSensor::initStaticProperties()\n> >\n> > /* Register the properties retrieved from the sensor database. */\n> > properties_.set(properties::UnitCellSize, props->unitCellSize);\n> > +\n> > + initTestPatternModes(props->testPatternModeMap);\n> > }\n> >\n> > int CameraSensor::initProperties()\n> > --\n> > 2.31.1.607.g51e8a6a459-goog\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\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 73B91C31F5\n\tfor ;\n\tFri, 14 May 2021 03:20:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B0B8A68920;\n\tFri, 14 May 2021 05:20:05 +0200 (CEST)","from mail-ed1-x533.google.com (mail-ed1-x533.google.com\n\t[IPv6:2a00:1450:4864:20::533])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 30C44602B4\n\tfor ;\n\tFri, 14 May 2021 05:20:03 +0200 (CEST)","by mail-ed1-x533.google.com with SMTP id v5so22079621edc.8\n\tfor ;\n\tThu, 13 May 2021 20:20:03 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"INbSJgNo\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=y/a9I/KbKYnTeCRfKSBHqCttsdoJ/aA09mHWGuDJoeY=;\n\tb=INbSJgNojZ217BpEnPqSs3hvTiBWB1ZCHnRokpywW2dKhDImiVU89v4JUkz+BLbfVk\n\t2tSGt5CSks7aAl8DcEsAjb/JvY38+tz2QWmKu/sz/bmYOByjf4a3e2TcMPWAuucyyWZ3\n\t4jUVUlpF7UoPZ2szGwj1cz9x1aIS0KWXwR6D8=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=y/a9I/KbKYnTeCRfKSBHqCttsdoJ/aA09mHWGuDJoeY=;\n\tb=lr2gPCwF2NeVD5ZHQPLhcdUpiXqIl232os94hC7rR0Hgb2BuE+pIWxb23p/7fbDzOb\n\tiuE7BnhQZeTwnRGjA/w7xudQ5i5SlPWvgieN1ncN6KjTb1dR9d1tFlIxBBjOtxXy5uZQ\n\t6+MjBUoycP6VhWhSiYQKNFvbzPYe5x7nHbifcIrbWndhkul9m59P3gPQ6Km+56j+vQx9\n\t/xCEPdl4iGtDXaRxDlECXIBRF408TaZN1r1G8RUdS429qXJ1k7NBnxOr6gSo1ORQpEn9\n\tcC54x91utXPK/gKJxj/Wb/27Tvf1jgRFQESasacfer1SXrk1O2pp250VjesjgCjWEMRx\n\tAmjQ==","X-Gm-Message-State":"AOAM532eMnmU/G4emf2VBms9A1GYAnaRn7jRdEVWMUUYH1gjN9SRJC/T\n\tcdDyRgSXKCWZI16tJbH8DEQw2T59SGIVV+jYLoSx7g==","X-Google-Smtp-Source":"ABdhPJxGu52FIPCWhGz/BjXyqKc3yytURCBbBjz5obIkfTcvcx5cWdBPne31VAKhP7b2Dhfkwk3Ii/RD1QXN3EHFz/c=","X-Received":"by 2002:a50:8e44:: with SMTP id 4mr53654507edx.244.1620962402664;\n\tThu, 13 May 2021 20:20:02 -0700 (PDT)","MIME-Version":"1.0","References":"<20210506075449.1761752-1-hiroh@chromium.org>\n\t<20210506075449.1761752-5-hiroh@chromium.org>\n\t<20210513101744.w4vowg6oqjwegqka@uno.localdomain>","In-Reply-To":"<20210513101744.w4vowg6oqjwegqka@uno.localdomain>","From":"Hirokazu Honda ","Date":"Fri, 14 May 2021 12:19:51 +0900","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH v4 4/6] libcamera: CameraSensor:\n\tEnable retrieving supported test pattern modes","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 ","Content-Type":"multipart/mixed;\n\tboundary=\"===============7994503339596362141==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]