[{"id":2751,"web_url":"https://patchwork.libcamera.org/comment/2751/","msgid":"<20191003190513.GF1322@bigcity.dyn.berto.se>","date":"2019-10-03T19:05:13","subject":"Re: [libcamera-devel] [PATCH v2 03/13] libcamera: controls: Use\n\texplicit 32-bit integer types","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your patch.\n\nOn 2019-09-29 22:02:44 +0300, Laurent Pinchart wrote:\n> Make the control API more explicit when dealing with integer controls by\n> specifying the size. We already do so for 64-bit integers, using int64_t\n> and ControlTypeInteger64, do the same for 32-bit integers.\n> \n> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>\n\nReviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n\n> ---\n>  include/libcamera/controls.h        |  6 ++---\n>  src/libcamera/controls.cpp          | 36 ++++++++++++++---------------\n>  src/libcamera/pipeline/uvcvideo.cpp | 10 ++++----\n>  src/libcamera/pipeline/vimc.cpp     |  6 ++---\n>  test/controls/control_info.cpp      | 10 ++++----\n>  test/controls/control_list.cpp      | 16 ++++++-------\n>  test/controls/control_value.cpp     |  6 ++---\n>  7 files changed, 46 insertions(+), 44 deletions(-)\n> \n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index 0886370f0901..a3089064c4b5 100644\n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -21,7 +21,7 @@ class Camera;\n>  enum ControlType {\n>  \tControlTypeNone,\n>  \tControlTypeBool,\n> -\tControlTypeInteger,\n> +\tControlTypeInteger32,\n>  \tControlTypeInteger64,\n>  };\n>  \n> @@ -30,7 +30,7 @@ class ControlValue\n>  public:\n>  \tControlValue();\n>  \tControlValue(bool value);\n> -\tControlValue(int value);\n> +\tControlValue(int32_t value);\n>  \tControlValue(int64_t value);\n>  \n>  \tControlType type() const { return type_; };\n> @@ -48,7 +48,7 @@ private:\n>  \n>  \tunion {\n>  \t\tbool bool_;\n> -\t\tint integer_;\n> +\t\tint32_t integer32_;\n>  \t\tint64_t integer64_;\n>  \t};\n>  };\n> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> index 88aab88db327..295ccd55a622 100644\n> --- a/src/libcamera/controls.cpp\n> +++ b/src/libcamera/controls.cpp\n> @@ -31,8 +31,8 @@ LOG_DEFINE_CATEGORY(Controls)\n>   * Invalid type, for empty values\n>   * \\var ControlTypeBool\n>   * The control stores a boolean value\n> - * \\var ControlTypeInteger\n> - * The control stores an integer value\n> + * \\var ControlTypeInteger32\n> + * The control stores a 32-bit integer value\n>   * \\var ControlTypeInteger64\n>   * The control stores a 64-bit integer value\n>   */\n> @@ -63,8 +63,8 @@ ControlValue::ControlValue(bool value)\n>   * \\brief Construct an integer ControlValue\n>   * \\param[in] value Integer value to store\n>   */\n> -ControlValue::ControlValue(int value)\n> -\t: type_(ControlTypeInteger), integer_(value)\n> +ControlValue::ControlValue(int32_t value)\n> +\t: type_(ControlTypeInteger32), integer32_(value)\n>  {\n>  }\n>  \n> @@ -115,17 +115,17 @@ const bool &ControlValue::get<bool>() const\n>  }\n>  \n>  template<>\n> -const int &ControlValue::get<int>() const\n> +const int32_t &ControlValue::get<int32_t>() const\n>  {\n> -\tASSERT(type_ == ControlTypeInteger || type_ == ControlTypeInteger64);\n> +\tASSERT(type_ == ControlTypeInteger32 || type_ == ControlTypeInteger64);\n>  \n> -\treturn integer_;\n> +\treturn integer32_;\n>  }\n>  \n>  template<>\n>  const int64_t &ControlValue::get<int64_t>() const\n>  {\n> -\tASSERT(type_ == ControlTypeInteger || type_ == ControlTypeInteger64);\n> +\tASSERT(type_ == ControlTypeInteger32 || type_ == ControlTypeInteger64);\n>  \n>  \treturn integer64_;\n>  }\n> @@ -138,10 +138,10 @@ void ControlValue::set<bool>(const bool &value)\n>  }\n>  \n>  template<>\n> -void ControlValue::set<int>(const int &value)\n> +void ControlValue::set<int32_t>(const int32_t &value)\n>  {\n> -\ttype_ = ControlTypeInteger;\n> -\tinteger_ = value;\n> +\ttype_ = ControlTypeInteger32;\n> +\tinteger32_ = value;\n>  }\n>  \n>  template<>\n> @@ -163,8 +163,8 @@ std::string ControlValue::toString() const\n>  \t\treturn \"<None>\";\n>  \tcase ControlTypeBool:\n>  \t\treturn bool_ ? \"True\" : \"False\";\n> -\tcase ControlTypeInteger:\n> -\t\treturn std::to_string(integer_);\n> +\tcase ControlTypeInteger32:\n> +\t\treturn std::to_string(integer32_);\n>  \tcase ControlTypeInteger64:\n>  \t\treturn std::to_string(integer64_);\n>  \t}\n> @@ -186,35 +186,35 @@ std::string ControlValue::toString() const\n>  \n>  /**\n>   * \\var Brightness\n> - * ControlType: Integer\n> + * ControlType: Integer32\n>   *\n>   * Specify a fixed brightness parameter.\n>   */\n>  \n>  /**\n>   * \\var Contrast\n> - * ControlType: Integer\n> + * ControlType: Integer32\n>   *\n>   * Specify a fixed contrast parameter.\n>   */\n>  \n>  /**\n>   * \\var Saturation\n> - * ControlType: Integer\n> + * ControlType: Integer32\n>   *\n>   * Specify a fixed saturation parameter.\n>   */\n>  \n>  /**\n>   * \\var ManualExposure\n> - * ControlType: Integer\n> + * ControlType: Integer32\n>   *\n>   * Specify a fixed exposure time in milli-seconds\n>   */\n>  \n>  /**\n>   * \\var ManualGain\n> - * ControlType: Integer\n> + * ControlType: Integer32\n>   *\n>   * Specify a fixed gain parameter\n>   */\n> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> index 81c548af2c64..0d56758e3e1d 100644\n> --- a/src/libcamera/pipeline/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> @@ -235,24 +235,24 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)\n>  \n>  \t\tswitch (ci->id()) {\n>  \t\tcase Brightness:\n> -\t\t\tcontrols.add(V4L2_CID_BRIGHTNESS, value.get<int>());\n> +\t\t\tcontrols.add(V4L2_CID_BRIGHTNESS, value.get<int32_t>());\n>  \t\t\tbreak;\n>  \n>  \t\tcase Contrast:\n> -\t\t\tcontrols.add(V4L2_CID_CONTRAST, value.get<int>());\n> +\t\t\tcontrols.add(V4L2_CID_CONTRAST, value.get<int32_t>());\n>  \t\t\tbreak;\n>  \n>  \t\tcase Saturation:\n> -\t\t\tcontrols.add(V4L2_CID_SATURATION, value.get<int>());\n> +\t\t\tcontrols.add(V4L2_CID_SATURATION, value.get<int32_t>());\n>  \t\t\tbreak;\n>  \n>  \t\tcase ManualExposure:\n>  \t\t\tcontrols.add(V4L2_CID_EXPOSURE_AUTO, 1);\n> -\t\t\tcontrols.add(V4L2_CID_EXPOSURE_ABSOLUTE, value.get<int>());\n> +\t\t\tcontrols.add(V4L2_CID_EXPOSURE_ABSOLUTE, value.get<int32_t>());\n>  \t\t\tbreak;\n>  \n>  \t\tcase ManualGain:\n> -\t\t\tcontrols.add(V4L2_CID_GAIN, value.get<int>());\n> +\t\t\tcontrols.add(V4L2_CID_GAIN, value.get<int32_t>());\n>  \t\t\tbreak;\n>  \n>  \t\tdefault:\n> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> index 3e34e7a0edbf..e549dc64b996 100644\n> --- a/src/libcamera/pipeline/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc.cpp\n> @@ -288,15 +288,15 @@ int PipelineHandlerVimc::processControls(VimcCameraData *data, Request *request)\n>  \n>  \t\tswitch (ci->id()) {\n>  \t\tcase Brightness:\n> -\t\t\tcontrols.add(V4L2_CID_BRIGHTNESS, value.get<int>());\n> +\t\t\tcontrols.add(V4L2_CID_BRIGHTNESS, value.get<int32_t>());\n>  \t\t\tbreak;\n>  \n>  \t\tcase Contrast:\n> -\t\t\tcontrols.add(V4L2_CID_CONTRAST, value.get<int>());\n> +\t\t\tcontrols.add(V4L2_CID_CONTRAST, value.get<int32_t>());\n>  \t\t\tbreak;\n>  \n>  \t\tcase Saturation:\n> -\t\t\tcontrols.add(V4L2_CID_SATURATION, value.get<int>());\n> +\t\t\tcontrols.add(V4L2_CID_SATURATION, value.get<int32_t>());\n>  \t\t\tbreak;\n>  \n>  \t\tdefault:\n> diff --git a/test/controls/control_info.cpp b/test/controls/control_info.cpp\n> index faefaaa444d9..2aba568a302e 100644\n> --- a/test/controls/control_info.cpp\n> +++ b/test/controls/control_info.cpp\n> @@ -26,13 +26,14 @@ protected:\n>  \t\tControlInfo info(Brightness);\n>  \n>  \t\tif (info.id() != Brightness ||\n> -\t\t    info.type() != ControlTypeInteger ||\n> +\t\t    info.type() != ControlTypeInteger32 ||\n>  \t\t    info.name() != std::string(\"Brightness\")) {\n>  \t\t\tcout << \"Invalid control identification for Brightness\" << endl;\n>  \t\t\treturn TestFail;\n>  \t\t}\n>  \n> -\t\tif (info.min().get<int>() != 0 || info.max().get<int>() != 0) {\n> +\t\tif (info.min().get<int32_t>() != 0 ||\n> +\t\t    info.max().get<int32_t>() != 0) {\n>  \t\t\tcout << \"Invalid control range for Brightness\" << endl;\n>  \t\t\treturn TestFail;\n>  \t\t}\n> @@ -44,13 +45,14 @@ protected:\n>  \t\tinfo = ControlInfo(Contrast, 10, 200);\n>  \n>  \t\tif (info.id() != Contrast ||\n> -\t\t    info.type() != ControlTypeInteger ||\n> +\t\t    info.type() != ControlTypeInteger32 ||\n>  \t\t    info.name() != std::string(\"Contrast\")) {\n>  \t\t\tcout << \"Invalid control identification for Contrast\" << endl;\n>  \t\t\treturn TestFail;\n>  \t\t}\n>  \n> -\t\tif (info.min().get<int>() != 10 || info.max().get<int>() != 200) {\n> +\t\tif (info.min().get<int32_t>() != 10 ||\n> +\t\t    info.max().get<int32_t>() != 200) {\n>  \t\t\tcout << \"Invalid control range for Contrast\" << endl;\n>  \t\t\treturn TestFail;\n>  \t\t}\n> diff --git a/test/controls/control_list.cpp b/test/controls/control_list.cpp\n> index 0402e7c23dba..5215840b1c4e 100644\n> --- a/test/controls/control_list.cpp\n> +++ b/test/controls/control_list.cpp\n> @@ -96,7 +96,7 @@ protected:\n>  \t\t\treturn TestFail;\n>  \t\t}\n>  \n> -\t\tif (list[Brightness].get<int>() != 255) {\n> +\t\tif (list[Brightness].get<int32_t>() != 255) {\n>  \t\t\tcout << \"Incorrest Brightness control value\" << endl;\n>  \t\t\treturn TestFail;\n>  \t\t}\n> @@ -125,8 +125,8 @@ protected:\n>  \t\t/*\n>  \t\t * Test control value retrieval and update through ControlInfo.\n>  \t\t */\n> -\t\tif (list[brightness].get<int>() != 64 ||\n> -\t\t    list[contrast].get<int>() != 128) {\n> +\t\tif (list[brightness].get<int32_t>() != 64 ||\n> +\t\t    list[contrast].get<int32_t>() != 128) {\n>  \t\t\tcout << \"Failed to retrieve control value\" << endl;\n>  \t\t\treturn TestFail;\n>  \t\t}\n> @@ -134,8 +134,8 @@ protected:\n>  \t\tlist[brightness] = 10;\n>  \t\tlist[contrast] = 20;\n>  \n> -\t\tif (list[brightness].get<int>() != 10 ||\n> -\t\t    list[contrast].get<int>() != 20) {\n> +\t\tif (list[brightness].get<int32_t>() != 10 ||\n> +\t\t    list[contrast].get<int32_t>() != 20) {\n>  \t\t\tcout << \"Failed to update control value\" << endl;\n>  \t\t\treturn TestFail;\n>  \t\t}\n> @@ -185,9 +185,9 @@ protected:\n>  \t\t\treturn TestFail;\n>  \t\t}\n>  \n> -\t\tif (newList[Brightness].get<int>() != 10 ||\n> -\t\t    newList[Contrast].get<int>() != 20 ||\n> -\t\t    newList[Saturation].get<int>() != 255) {\n> +\t\tif (newList[Brightness].get<int32_t>() != 10 ||\n> +\t\t    newList[Contrast].get<int32_t>() != 20 ||\n> +\t\t    newList[Saturation].get<int32_t>() != 255) {\n>  \t\t\tcout << \"New list contains incorrect values\" << endl;\n>  \t\t\treturn TestFail;\n>  \t\t}\n> diff --git a/test/controls/control_value.cpp b/test/controls/control_value.cpp\n> index 397c43f799ad..a1ffa842f29e 100644\n> --- a/test/controls/control_value.cpp\n> +++ b/test/controls/control_value.cpp\n> @@ -27,7 +27,7 @@ protected:\n>  \t\t     << \" Bool: \" << boolean.toString()\n>  \t\t     << endl;\n>  \n> -\t\tif (integer.get<int>() != 1234) {\n> +\t\tif (integer.get<int32_t>() != 1234) {\n>  \t\t\tcerr << \"Failed to get Integer\" << endl;\n>  \t\t\treturn TestFail;\n>  \t\t}\n> @@ -56,8 +56,8 @@ protected:\n>  \t\t\treturn TestFail;\n>  \t\t}\n>  \n> -\t\tvalue.set<int>(10);\n> -\t\tif (value.get<int>() != 10) {\n> +\t\tvalue.set<int32_t>(10);\n> +\t\tif (value.get<int32_t>() != 10) {\n>  \t\t\tcerr << \"Failed to get Integer\" << endl;\n>  \t\t\treturn TestFail;\n>  \t\t}\n> -- \n> Regards,\n> \n> Laurent Pinchart\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<niklas.soderlund@ragnatech.se>","Received":["from mail-lf1-x141.google.com (mail-lf1-x141.google.com\n\t[IPv6:2a00:1450:4864:20::141])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0727160BE8\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu,  3 Oct 2019 21:05:15 +0200 (CEST)","by mail-lf1-x141.google.com with SMTP id r22so2689520lfm.1\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 03 Oct 2019 12:05:14 -0700 (PDT)","from localhost (h-93-159.A463.priv.bahnhof.se. [46.59.93.159])\n\tby smtp.gmail.com with ESMTPSA id\n\tx76sm986202ljb.81.2019.10.03.12.05.13\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 03 Oct 2019 12:05:13 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=8LFxD+nTXxyQDoLo/Iqyaf4wNEpILlpIQtKxHUFvaW0=;\n\tb=0Ft4lzeukVyMcOQ6TQYjqk+q6DQ+K1p8PGBvrPHGgF02wzM11n2NBZPRBi7P+e6FDT\n\tSW+yiM3Paw7lSDIfPFkstTqOs8vRcavfT1Khcnvx0CVo2ZJdrdvBkeXLO4InBArSG8Jy\n\ta4s2hBbVvw7vR2S1lOqtQzpjS7K+fULQ5R221pEPT5Cnsd9Rdiqgen+rNIObV97QtNho\n\tI9SnizD7sA1N3ydB3QGeEUftcNf4hiC3djrGz9hTFy1ToPPqKdKsvwunM4+jnn5JpFAp\n\tJxCRC2HlsrXJs+qlHdcqxeJXSrP661enuO1G9ktACDjcpp+AZJ64al3H3U5dMeHNaC1J\n\tOorQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=8LFxD+nTXxyQDoLo/Iqyaf4wNEpILlpIQtKxHUFvaW0=;\n\tb=ntGLgcNZ3f9wzGlbx+3Dd73mE+GwNWQjPQYNsz5FFRVs+EfEjB+5rEMwTQGitugipO\n\tB7feAm3cUtkeLq6D2KoNFi2BiWb0U4IKRLOtkbRVn4wfXZp5eAJLkbG+f8RQPpnV5Tmp\n\tuFu4eo9wBmqTR5Fgp2xRJhxvbP03iAWJ66jhfLAOLCzJr3sNnkdCgn//RdkCXv+f0UPq\n\tEHkjwaNKi+CKoS/Ft0y9RwJG71f6Xs490D3fAxjO23TQ3Rj88+BezUBd2zmrN2JRxRen\n\tobiJha0BtJsQw6IRFLqAZ2fJwp+Hno8p+Ezmm3LNb35bqO4zLmfCjqVjhrIawudrHidk\n\tpdoA==","X-Gm-Message-State":"APjAAAUlg/qQzPGwNdX6yyu86+oOv1lz5a6LXtBmDhlfOm1AOSiI0OzP\n\t7GZtdIbFVVv5sC7PEKuAs2uVKW+fOJ0=","X-Google-Smtp-Source":"APXvYqxFEhjQukhXx35ZaoJ02WbgYe84SWVKgpYBVD1zKlriGn1vjFnQ+JM2+Oao1ZfF2VWhychwDQ==","X-Received":"by 2002:a19:428f:: with SMTP id\n\tp137mr6639551lfa.149.1570129514105; \n\tThu, 03 Oct 2019 12:05:14 -0700 (PDT)","Date":"Thu, 3 Oct 2019 21:05:13 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= <niklas.soderlund@ragnatech.se>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191003190513.GF1322@bigcity.dyn.berto.se>","References":"<20190929190254.18920-1-laurent.pinchart@ideasonboard.com>\n\t<20190929190254.18920-4-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190929190254.18920-4-laurent.pinchart@ideasonboard.com>","User-Agent":"Mutt/1.12.1 (2019-06-15)","Subject":"Re: [libcamera-devel] [PATCH v2 03/13] libcamera: controls: Use\n\texplicit 32-bit integer types","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>","X-List-Received-Date":"Thu, 03 Oct 2019 19:05:15 -0000"}}]