[{"id":21531,"web_url":"https://patchwork.libcamera.org/comment/21531/","msgid":"<20211201150457.z7klzuzifthwni3n@uno.localdomain>","date":"2021-12-01T15:04:57","subject":"Re: [libcamera-devel] [PATCH v9 1/3] libcamera: camera_sensor:\n\tReference test pattern modes by enum type","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n\n  while running CTS with this series applied I have noticed that once\na test pattern is set in one of the test, the rest of the session is\nrun with a test mode applied. I noticed as the displayed images are\nactually the sensor's test patterns. Running the Camera app after CTS\nhas completed displays test pattern modes.\n\nIt's pretty obvious that the test pattern mode is reset to Off during\nCameraSensor::init() which is called only at pipeline handler\ninitialization time, so if the library is not tore down the test\npattern mode is never reset.\n\nCould you check if the same happens on your side by running CTS please ?\n\nOn Mon, Nov 29, 2021 at 05:34:22PM +0900, Hirokazu Honda wrote:\n> The CameraSensor stores TestPatternModes as an int32_t. This prevents\n> the compiler from verifying the usage against the defined enum types.\n>\n> Fix references to the TestPatternMode to store the value as the\n> TestPatternModeEnum type which is defined by the control generator.\n>\n> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>\n> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>\n> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> ---\n>  include/libcamera/internal/camera_sensor.h            | 10 +++++++---\n>  include/libcamera/internal/camera_sensor_properties.h |  3 ++-\n>  src/libcamera/camera_sensor.cpp                       |  4 ++--\n>  src/libcamera/pipeline/ipu3/ipu3.cpp                  |  7 ++++---\n>  4 files changed, 15 insertions(+), 9 deletions(-)\n>\n> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> index 2facbf3c..310571ca 100644\n> --- a/include/libcamera/internal/camera_sensor.h\n> +++ b/include/libcamera/internal/camera_sensor.h\n> @@ -14,8 +14,10 @@\n>  #include <libcamera/base/class.h>\n>  #include <libcamera/base/log.h>\n>\n> +#include <libcamera/control_ids.h>\n>  #include <libcamera/controls.h>\n>  #include <libcamera/geometry.h>\n> +\n>  #include <libcamera/ipa/core_ipa_interface.h>\n>\n>  #include \"libcamera/internal/formats.h\"\n> @@ -40,7 +42,8 @@ public:\n>  \tconst std::vector<unsigned int> &mbusCodes() const { return mbusCodes_; }\n>  \tconst std::vector<Size> sizes(unsigned int mbusCode) const;\n>  \tSize resolution() const;\n> -\tconst std::vector<int32_t> &testPatternModes() const\n> +\tconst std::vector<controls::draft::TestPatternModeEnum>\n> +\t\t&testPatternModes() const\n>  \t{\n>  \t\treturn testPatternModes_;\n>  \t}\n> @@ -71,7 +74,8 @@ private:\n>  \tvoid initVimcDefaultProperties();\n>  \tvoid initStaticProperties();\n>  \tvoid initTestPatternModes(\n> -\t\tconst std::map<int32_t, int32_t> &testPatternModeMap);\n> +\t\tconst std::map<controls::draft::TestPatternModeEnum, int32_t>\n> +\t\t\t&testPatternModeMap);\n>  \tint initProperties();\n>\n>  \tconst MediaEntity *entity_;\n> @@ -84,7 +88,7 @@ private:\n>  \tV4L2Subdevice::Formats formats_;\n>  \tstd::vector<unsigned int> mbusCodes_;\n>  \tstd::vector<Size> sizes_;\n> -\tstd::vector<int32_t> testPatternModes_;\n> +\tstd::vector<controls::draft::TestPatternModeEnum> testPatternModes_;\n>\n>  \tSize pixelArraySize_;\n>  \tRectangle activeArea_;\n> diff --git a/include/libcamera/internal/camera_sensor_properties.h b/include/libcamera/internal/camera_sensor_properties.h\n> index af381a12..1ee3cb99 100644\n> --- a/include/libcamera/internal/camera_sensor_properties.h\n> +++ b/include/libcamera/internal/camera_sensor_properties.h\n> @@ -10,6 +10,7 @@\n>  #include <map>\n>  #include <string>\n>\n> +#include <libcamera/control_ids.h>\n>  #include <libcamera/geometry.h>\n>\n>  namespace libcamera {\n> @@ -18,7 +19,7 @@ struct CameraSensorProperties {\n>  \tstatic const CameraSensorProperties *get(const std::string &sensor);\n>\n>  \tSize unitCellSize;\n> -\tstd::map<int32_t, int32_t> testPatternModes;\n> +\tstd::map<controls::draft::TestPatternModeEnum, int32_t> testPatternModes;\n>  };\n>\n>  } /* namespace libcamera */\n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index 9fdb8c09..f0aa9f24 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -311,7 +311,7 @@ void CameraSensor::initStaticProperties()\n>  }\n>\n>  void CameraSensor::initTestPatternModes(\n> -\tconst std::map<int32_t, int32_t> &testPatternModes)\n> +\tconst std::map<controls::draft::TestPatternModeEnum, int32_t> &testPatternModes)\n>  {\n>  \tconst auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);\n>  \tif (v4l2TestPattern == controls().end()) {\n> @@ -327,7 +327,7 @@ void CameraSensor::initTestPatternModes(\n>  \t * control index is supported in the below for loop that creates the\n>  \t * list of supported test patterns.\n>  \t */\n> -\tstd::map<int32_t, int32_t> indexToTestPatternMode;\n> +\tstd::map<int32_t, controls::draft::TestPatternModeEnum> indexToTestPatternMode;\n>  \tfor (const auto &it : testPatternModes)\n>  \t\tindexToTestPatternMode[it.second] = it.first;\n>\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index c65afdb2..25490dcf 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -982,13 +982,14 @@ int PipelineHandlerIPU3::updateControls(IPU3CameraData *data)\n>  \t\treturn ret;\n>\n>  \tControlInfoMap::Map controls = IPU3Controls;\n> -\tconst std::vector<int32_t> &testPatternModes = sensor->testPatternModes();\n> +\tconst std::vector<controls::draft::TestPatternModeEnum>\n> +\t\t&testPatternModes = sensor->testPatternModes();\n>  \tif (!testPatternModes.empty()) {\n>  \t\tstd::vector<ControlValue> values;\n>  \t\tvalues.reserve(testPatternModes.size());\n>\n> -\t\tfor (int32_t pattern : testPatternModes)\n> -\t\t\tvalues.emplace_back(pattern);\n> +\t\tfor (auto pattern : testPatternModes)\n> +\t\t\tvalues.emplace_back(static_cast<int32_t>(pattern));\n>\n>  \t\tcontrols[&controls::draft::TestPatternMode] = ControlInfo(values);\n>  \t}\n> --\n> 2.34.0.rc2.393.gf8c9666880-goog\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 81A7CBDB13\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed,  1 Dec 2021 15:04:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D216760732;\n\tWed,  1 Dec 2021 16:04:07 +0100 (CET)","from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[217.70.183.200])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1EEB1604FC\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed,  1 Dec 2021 16:04:07 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay7-d.mail.gandi.net (Postfix) with ESMTPSA id 637C720010;\n\tWed,  1 Dec 2021 15:04:05 +0000 (UTC)"],"Date":"Wed, 1 Dec 2021 16:04:57 +0100","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Hirokazu Honda <hiroh@chromium.org>","Message-ID":"<20211201150457.z7klzuzifthwni3n@uno.localdomain>","References":"<20211129083424.3136533-1-hiroh@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211129083424.3136533-1-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v9 1/3] libcamera: camera_sensor:\n\tReference test pattern modes by enum type","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>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":21587,"web_url":"https://patchwork.libcamera.org/comment/21587/","msgid":"<CAO5uPHN=g4JL4izS3_zsTRm6QbJjAptBVYHXJ0rmhzkTjaYmxA@mail.gmail.com>","date":"2021-12-03T22:15:14","subject":"Re: [libcamera-devel] [PATCH v9 1/3] libcamera: camera_sensor:\n\tReference test pattern modes by enum type","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Jacopo,\n\nOn Thu, Dec 2, 2021 at 12:04 AM Jacopo Mondi <jacopo@jmondi.org> wrote:\n>\n> Hi Hiro,\n>\n>   while running CTS with this series applied I have noticed that once\n> a test pattern is set in one of the test, the rest of the session is\n> run with a test mode applied. I noticed as the displayed images are\n> actually the sensor's test patterns. Running the Camera app after CTS\n> has completed displays test pattern modes.\n>\n> It's pretty obvious that the test pattern mode is reset to Off during\n> CameraSensor::init() which is called only at pipeline handler\n> initialization time, so if the library is not tore down the test\n> pattern mode is never reset.\n>\n> Could you check if the same happens on your side by running CTS please ?\n>\n\nThanks. It is reproducible for me.\nI confirmed the issue is resolved if I call setTestPatternMode(off) in\nIPU3 pipeline handler configure().\nDo you think it is a correct fix?\n\nThanks,\n-Hiro\n\n\n\n> On Mon, Nov 29, 2021 at 05:34:22PM +0900, Hirokazu Honda wrote:\n> > The CameraSensor stores TestPatternModes as an int32_t. This prevents\n> > the compiler from verifying the usage against the defined enum types.\n> >\n> > Fix references to the TestPatternMode to store the value as the\n> > TestPatternModeEnum type which is defined by the control generator.\n> >\n> > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>\n> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>\n> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> > ---\n> >  include/libcamera/internal/camera_sensor.h            | 10 +++++++---\n> >  include/libcamera/internal/camera_sensor_properties.h |  3 ++-\n> >  src/libcamera/camera_sensor.cpp                       |  4 ++--\n> >  src/libcamera/pipeline/ipu3/ipu3.cpp                  |  7 ++++---\n> >  4 files changed, 15 insertions(+), 9 deletions(-)\n> >\n> > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> > index 2facbf3c..310571ca 100644\n> > --- a/include/libcamera/internal/camera_sensor.h\n> > +++ b/include/libcamera/internal/camera_sensor.h\n> > @@ -14,8 +14,10 @@\n> >  #include <libcamera/base/class.h>\n> >  #include <libcamera/base/log.h>\n> >\n> > +#include <libcamera/control_ids.h>\n> >  #include <libcamera/controls.h>\n> >  #include <libcamera/geometry.h>\n> > +\n> >  #include <libcamera/ipa/core_ipa_interface.h>\n> >\n> >  #include \"libcamera/internal/formats.h\"\n> > @@ -40,7 +42,8 @@ public:\n> >       const std::vector<unsigned int> &mbusCodes() const { return mbusCodes_; }\n> >       const std::vector<Size> sizes(unsigned int mbusCode) const;\n> >       Size resolution() const;\n> > -     const std::vector<int32_t> &testPatternModes() const\n> > +     const std::vector<controls::draft::TestPatternModeEnum>\n> > +             &testPatternModes() const\n> >       {\n> >               return testPatternModes_;\n> >       }\n> > @@ -71,7 +74,8 @@ private:\n> >       void initVimcDefaultProperties();\n> >       void initStaticProperties();\n> >       void initTestPatternModes(\n> > -             const std::map<int32_t, int32_t> &testPatternModeMap);\n> > +             const std::map<controls::draft::TestPatternModeEnum, int32_t>\n> > +                     &testPatternModeMap);\n> >       int initProperties();\n> >\n> >       const MediaEntity *entity_;\n> > @@ -84,7 +88,7 @@ private:\n> >       V4L2Subdevice::Formats formats_;\n> >       std::vector<unsigned int> mbusCodes_;\n> >       std::vector<Size> sizes_;\n> > -     std::vector<int32_t> testPatternModes_;\n> > +     std::vector<controls::draft::TestPatternModeEnum> testPatternModes_;\n> >\n> >       Size pixelArraySize_;\n> >       Rectangle activeArea_;\n> > diff --git a/include/libcamera/internal/camera_sensor_properties.h b/include/libcamera/internal/camera_sensor_properties.h\n> > index af381a12..1ee3cb99 100644\n> > --- a/include/libcamera/internal/camera_sensor_properties.h\n> > +++ b/include/libcamera/internal/camera_sensor_properties.h\n> > @@ -10,6 +10,7 @@\n> >  #include <map>\n> >  #include <string>\n> >\n> > +#include <libcamera/control_ids.h>\n> >  #include <libcamera/geometry.h>\n> >\n> >  namespace libcamera {\n> > @@ -18,7 +19,7 @@ struct CameraSensorProperties {\n> >       static const CameraSensorProperties *get(const std::string &sensor);\n> >\n> >       Size unitCellSize;\n> > -     std::map<int32_t, int32_t> testPatternModes;\n> > +     std::map<controls::draft::TestPatternModeEnum, int32_t> testPatternModes;\n> >  };\n> >\n> >  } /* namespace libcamera */\n> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > index 9fdb8c09..f0aa9f24 100644\n> > --- a/src/libcamera/camera_sensor.cpp\n> > +++ b/src/libcamera/camera_sensor.cpp\n> > @@ -311,7 +311,7 @@ void CameraSensor::initStaticProperties()\n> >  }\n> >\n> >  void CameraSensor::initTestPatternModes(\n> > -     const std::map<int32_t, int32_t> &testPatternModes)\n> > +     const std::map<controls::draft::TestPatternModeEnum, int32_t> &testPatternModes)\n> >  {\n> >       const auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);\n> >       if (v4l2TestPattern == controls().end()) {\n> > @@ -327,7 +327,7 @@ void CameraSensor::initTestPatternModes(\n> >        * control index is supported in the below for loop that creates the\n> >        * list of supported test patterns.\n> >        */\n> > -     std::map<int32_t, int32_t> indexToTestPatternMode;\n> > +     std::map<int32_t, controls::draft::TestPatternModeEnum> indexToTestPatternMode;\n> >       for (const auto &it : testPatternModes)\n> >               indexToTestPatternMode[it.second] = it.first;\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index c65afdb2..25490dcf 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -982,13 +982,14 @@ int PipelineHandlerIPU3::updateControls(IPU3CameraData *data)\n> >               return ret;\n> >\n> >       ControlInfoMap::Map controls = IPU3Controls;\n> > -     const std::vector<int32_t> &testPatternModes = sensor->testPatternModes();\n> > +     const std::vector<controls::draft::TestPatternModeEnum>\n> > +             &testPatternModes = sensor->testPatternModes();\n> >       if (!testPatternModes.empty()) {\n> >               std::vector<ControlValue> values;\n> >               values.reserve(testPatternModes.size());\n> >\n> > -             for (int32_t pattern : testPatternModes)\n> > -                     values.emplace_back(pattern);\n> > +             for (auto pattern : testPatternModes)\n> > +                     values.emplace_back(static_cast<int32_t>(pattern));\n> >\n> >               controls[&controls::draft::TestPatternMode] = ControlInfo(values);\n> >       }\n> > --\n> > 2.34.0.rc2.393.gf8c9666880-goog\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 D9F9FBDB13\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri,  3 Dec 2021 22:15:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2F64B60832;\n\tFri,  3 Dec 2021 23:15:26 +0100 (CET)","from mail-ed1-x52e.google.com (mail-ed1-x52e.google.com\n\t[IPv6:2a00:1450:4864:20::52e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0D2AD60725\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri,  3 Dec 2021 23:15:25 +0100 (CET)","by mail-ed1-x52e.google.com with SMTP id x6so16975640edr.5\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 03 Dec 2021 14:15:24 -0800 (PST)"],"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=\"cKJmbKhM\"; 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=I9fPWcnsTOOXOhza+gxZq2OCDqN7IoPlbkV1dtNdt6A=;\n\tb=cKJmbKhMvvpVzmC4XvRTEoZVwr5cxUgqBEvZTNpuI+zsRFL6GRfRrl7BQ118gxMkIq\n\t+Fc8Mb5eepCtoE58qbJYlox6yedWh3NjuwPmdcGiLCF8cam7z06xS2Q1fAGJFVUCton1\n\tMKU0HGieJBGNeejL9s/zCBAKPPTcF+AZFjYnc=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=I9fPWcnsTOOXOhza+gxZq2OCDqN7IoPlbkV1dtNdt6A=;\n\tb=hSKe+THU1G8r19OJ4mqVXXL42nLx9acIwn5LbxzzA8kbm22VFk4R2Jp2SjYTYzREhH\n\tpr0Y5lUM/rE5GRuY7dTXt6+gwDehjRrjRtTLZEHd01oHWtD8hQ8RIi5HIKccbobuQ6ID\n\tIqJcWTfyLtptNp3S2aquY3p4Nc8F8l6yuXmhi2U9c1imTNDeHDWoW7QOklkfI4dnlt5u\n\tqMihN4od/Vb9hhszLWEmj/Voo9kqfq990VFb/9UEOZhMNvcaSgma6yd0Yidj7osXcgsk\n\tPrDOEupXeF/lV2cfCms2aEipRiF57RbSnJC2ZFjfk/iCalxf/w4sp8cuEyzwuGWkD1tU\n\tPYIw==","X-Gm-Message-State":"AOAM533EkLvSzovXozjdbf+fIDNJKMbKks0C86+FeQpo9ThMaokzMy+r\n\tw8RhysRNhEip+zs8Vu7aWl5YzyvV7Jx/jDLPlypmGQ==","X-Google-Smtp-Source":"ABdhPJy9XMH9jeklXv2sUDFyXxAnyhzoEmgq6ZL+W4tPhiChl2lFpccTTvfWoQ+LLZebLVHZc0wHZRJMqoYqvxAy/+o=","X-Received":"by 2002:a17:906:390:: with SMTP id\n\tb16mr25758932eja.522.1638569724388; \n\tFri, 03 Dec 2021 14:15:24 -0800 (PST)","MIME-Version":"1.0","References":"<20211129083424.3136533-1-hiroh@chromium.org>\n\t<20211201150457.z7klzuzifthwni3n@uno.localdomain>","In-Reply-To":"<20211201150457.z7klzuzifthwni3n@uno.localdomain>","From":"Hirokazu Honda <hiroh@chromium.org>","Date":"Sat, 4 Dec 2021 07:15:14 +0900","Message-ID":"<CAO5uPHN=g4JL4izS3_zsTRm6QbJjAptBVYHXJ0rmhzkTjaYmxA@mail.gmail.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v9 1/3] libcamera: camera_sensor:\n\tReference test pattern modes by enum type","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>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":21595,"web_url":"https://patchwork.libcamera.org/comment/21595/","msgid":"<163865015812.3153335.7643292856688284572@Monstersaurus>","date":"2021-12-04T20:35:58","subject":"Re: [libcamera-devel] [PATCH v9 1/3] libcamera: camera_sensor:\n\tReference test pattern modes by enum type","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Hiro,\n\nQuoting Hirokazu Honda (2021-12-03 22:15:14)\n> Hi Jacopo,\n> \n> On Thu, Dec 2, 2021 at 12:04 AM Jacopo Mondi <jacopo@jmondi.org> wrote:\n> >\n> > Hi Hiro,\n> >\n> >   while running CTS with this series applied I have noticed that once\n> > a test pattern is set in one of the test, the rest of the session is\n> > run with a test mode applied. I noticed as the displayed images are\n> > actually the sensor's test patterns. Running the Camera app after CTS\n> > has completed displays test pattern modes.\n> >\n> > It's pretty obvious that the test pattern mode is reset to Off during\n> > CameraSensor::init() which is called only at pipeline handler\n> > initialization time, so if the library is not tore down the test\n> > pattern mode is never reset.\n> >\n> > Could you check if the same happens on your side by running CTS please ?\n> >\n> \n> Thanks. It is reproducible for me.\n> I confirmed the issue is resolved if I call setTestPatternMode(off) in\n> IPU3 pipeline handler configure().\n> Do you think it is a correct fix?\n\nYes, I would think that configuring the testPatternMode as off directly\nduring configure() is appropriate and correct.\n\nA test pattern should only then be applied if a request comes in with a\ntest pattern mode explicitly set.\n\n--\nKieran\n\n> \n> Thanks,\n> -Hiro\n> \n> \n> \n> > On Mon, Nov 29, 2021 at 05:34:22PM +0900, Hirokazu Honda wrote:\n> > > The CameraSensor stores TestPatternModes as an int32_t. This prevents\n> > > the compiler from verifying the usage against the defined enum types.\n> > >\n> > > Fix references to the TestPatternMode to store the value as the\n> > > TestPatternModeEnum type which is defined by the control generator.\n> > >\n> > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>\n> > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>\n> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> > > ---\n> > >  include/libcamera/internal/camera_sensor.h            | 10 +++++++---\n> > >  include/libcamera/internal/camera_sensor_properties.h |  3 ++-\n> > >  src/libcamera/camera_sensor.cpp                       |  4 ++--\n> > >  src/libcamera/pipeline/ipu3/ipu3.cpp                  |  7 ++++---\n> > >  4 files changed, 15 insertions(+), 9 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> > > index 2facbf3c..310571ca 100644\n> > > --- a/include/libcamera/internal/camera_sensor.h\n> > > +++ b/include/libcamera/internal/camera_sensor.h\n> > > @@ -14,8 +14,10 @@\n> > >  #include <libcamera/base/class.h>\n> > >  #include <libcamera/base/log.h>\n> > >\n> > > +#include <libcamera/control_ids.h>\n> > >  #include <libcamera/controls.h>\n> > >  #include <libcamera/geometry.h>\n> > > +\n> > >  #include <libcamera/ipa/core_ipa_interface.h>\n> > >\n> > >  #include \"libcamera/internal/formats.h\"\n> > > @@ -40,7 +42,8 @@ public:\n> > >       const std::vector<unsigned int> &mbusCodes() const { return mbusCodes_; }\n> > >       const std::vector<Size> sizes(unsigned int mbusCode) const;\n> > >       Size resolution() const;\n> > > -     const std::vector<int32_t> &testPatternModes() const\n> > > +     const std::vector<controls::draft::TestPatternModeEnum>\n> > > +             &testPatternModes() const\n> > >       {\n> > >               return testPatternModes_;\n> > >       }\n> > > @@ -71,7 +74,8 @@ private:\n> > >       void initVimcDefaultProperties();\n> > >       void initStaticProperties();\n> > >       void initTestPatternModes(\n> > > -             const std::map<int32_t, int32_t> &testPatternModeMap);\n> > > +             const std::map<controls::draft::TestPatternModeEnum, int32_t>\n> > > +                     &testPatternModeMap);\n> > >       int initProperties();\n> > >\n> > >       const MediaEntity *entity_;\n> > > @@ -84,7 +88,7 @@ private:\n> > >       V4L2Subdevice::Formats formats_;\n> > >       std::vector<unsigned int> mbusCodes_;\n> > >       std::vector<Size> sizes_;\n> > > -     std::vector<int32_t> testPatternModes_;\n> > > +     std::vector<controls::draft::TestPatternModeEnum> testPatternModes_;\n> > >\n> > >       Size pixelArraySize_;\n> > >       Rectangle activeArea_;\n> > > diff --git a/include/libcamera/internal/camera_sensor_properties.h b/include/libcamera/internal/camera_sensor_properties.h\n> > > index af381a12..1ee3cb99 100644\n> > > --- a/include/libcamera/internal/camera_sensor_properties.h\n> > > +++ b/include/libcamera/internal/camera_sensor_properties.h\n> > > @@ -10,6 +10,7 @@\n> > >  #include <map>\n> > >  #include <string>\n> > >\n> > > +#include <libcamera/control_ids.h>\n> > >  #include <libcamera/geometry.h>\n> > >\n> > >  namespace libcamera {\n> > > @@ -18,7 +19,7 @@ struct CameraSensorProperties {\n> > >       static const CameraSensorProperties *get(const std::string &sensor);\n> > >\n> > >       Size unitCellSize;\n> > > -     std::map<int32_t, int32_t> testPatternModes;\n> > > +     std::map<controls::draft::TestPatternModeEnum, int32_t> testPatternModes;\n> > >  };\n> > >\n> > >  } /* namespace libcamera */\n> > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > > index 9fdb8c09..f0aa9f24 100644\n> > > --- a/src/libcamera/camera_sensor.cpp\n> > > +++ b/src/libcamera/camera_sensor.cpp\n> > > @@ -311,7 +311,7 @@ void CameraSensor::initStaticProperties()\n> > >  }\n> > >\n> > >  void CameraSensor::initTestPatternModes(\n> > > -     const std::map<int32_t, int32_t> &testPatternModes)\n> > > +     const std::map<controls::draft::TestPatternModeEnum, int32_t> &testPatternModes)\n> > >  {\n> > >       const auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);\n> > >       if (v4l2TestPattern == controls().end()) {\n> > > @@ -327,7 +327,7 @@ void CameraSensor::initTestPatternModes(\n> > >        * control index is supported in the below for loop that creates the\n> > >        * list of supported test patterns.\n> > >        */\n> > > -     std::map<int32_t, int32_t> indexToTestPatternMode;\n> > > +     std::map<int32_t, controls::draft::TestPatternModeEnum> indexToTestPatternMode;\n> > >       for (const auto &it : testPatternModes)\n> > >               indexToTestPatternMode[it.second] = it.first;\n> > >\n> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > index c65afdb2..25490dcf 100644\n> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > @@ -982,13 +982,14 @@ int PipelineHandlerIPU3::updateControls(IPU3CameraData *data)\n> > >               return ret;\n> > >\n> > >       ControlInfoMap::Map controls = IPU3Controls;\n> > > -     const std::vector<int32_t> &testPatternModes = sensor->testPatternModes();\n> > > +     const std::vector<controls::draft::TestPatternModeEnum>\n> > > +             &testPatternModes = sensor->testPatternModes();\n> > >       if (!testPatternModes.empty()) {\n> > >               std::vector<ControlValue> values;\n> > >               values.reserve(testPatternModes.size());\n> > >\n> > > -             for (int32_t pattern : testPatternModes)\n> > > -                     values.emplace_back(pattern);\n> > > +             for (auto pattern : testPatternModes)\n> > > +                     values.emplace_back(static_cast<int32_t>(pattern));\n> > >\n> > >               controls[&controls::draft::TestPatternMode] = ControlInfo(values);\n> > >       }\n> > > --\n> > > 2.34.0.rc2.393.gf8c9666880-goog\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 865BABF415\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSat,  4 Dec 2021 20:36:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 98DC560868;\n\tSat,  4 Dec 2021 21:36:02 +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 9CDB1605C4\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat,  4 Dec 2021 21:36:00 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 120AA30C;\n\tSat,  4 Dec 2021 21:36:00 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"U7qS5aaE\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1638650160;\n\tbh=LfsXXUthMLExakU6iCF+p4DpkjnPkHf2T8QRQ9LFLUE=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=U7qS5aaEIGoe/YxR1ke2VPUpSj0f9eW/cpAHdGbiP4NkUOPK+w+987Jr72P97BV7t\n\tXKp5nINm253vGvYfPkGMhcT5NZSjgEZkGiXpg8FYzQ08NS39CnjWePzd2eKHAE4ng1\n\teUAm5RYSApqNOHh3sWGOufNlyKO2eacGCsm6ofW0=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<CAO5uPHN=g4JL4izS3_zsTRm6QbJjAptBVYHXJ0rmhzkTjaYmxA@mail.gmail.com>","References":"<20211129083424.3136533-1-hiroh@chromium.org>\n\t<20211201150457.z7klzuzifthwni3n@uno.localdomain>\n\t<CAO5uPHN=g4JL4izS3_zsTRm6QbJjAptBVYHXJ0rmhzkTjaYmxA@mail.gmail.com>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","To":"Hirokazu Honda <hiroh@chromium.org>, Jacopo Mondi <jacopo@jmondi.org>","Date":"Sat, 04 Dec 2021 20:35:58 +0000","Message-ID":"<163865015812.3153335.7643292856688284572@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v9 1/3] libcamera: camera_sensor:\n\tReference test pattern modes by enum type","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>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]