[{"id":4120,"web_url":"https://patchwork.libcamera.org/comment/4120/","msgid":"<20200320011204.GB3558939@oden.dyn.berto.se>","date":"2020-03-20T01:12:04","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: controls: Don't\n\tover-optimize ControlValue layout","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 work.\n\nOn 2020-03-20 02:55:51 +0200, Laurent Pinchart wrote:\n> The ControlValue class size should be minimized to save space, as it can\n> be instantiated in large numbers. The current implementation does so by\n> specifying several members as bitfields, but does so too aggressively,\n> resulting in fields being packed in an inefficient to access way on some\n> platforms. For instance, on 32-bit x86, the numElements_ field is offset\n> by 7 bits in a 32-bit word. This additionally causes a static assert\n> that checks the size of the class to fail.\n> \n> Relax the constraints on the isArray_ and numElements_ fields to avoid\n> inefficient access, and to ensure that the class size is identical\n> across all platforms. This will anyway need to be revisited when\n> stabilizing the ABI, so add a \\todo comment as a reminder.\n> \n> Fixes: 1fa4b43402a0 (\"libcamera: controls: Support array controls in ControlValue\")\n> Reported-by: Jan Engelhardt <jengelh@inai.de>\n> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\nReviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n\n> ---\n>  include/libcamera/controls.h | 4 ++--\n>  src/libcamera/controls.cpp   | 1 +\n>  2 files changed, 3 insertions(+), 2 deletions(-)\n> \n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index 2fca975f7512..40a29189ba01 100644\n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -158,8 +158,8 @@ public:\n>  \n>  private:\n>  \tControlType type_ : 8;\n> -\tbool isArray_ : 1;\n> -\tstd::size_t numElements_ : 16;\n> +\tbool isArray_;\n> +\tstd::size_t numElements_ : 32;\n>  \tunion {\n>  \t\tuint64_t value_;\n>  \t\tvoid *storage_;\n> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> index 4615e71d7e3c..3c910d38f05d 100644\n> --- a/src/libcamera/controls.cpp\n> +++ b/src/libcamera/controls.cpp\n> @@ -83,6 +83,7 @@ static constexpr size_t ControlValueSize[] = {\n>   * \\brief Abstract type representing the value of a control\n>   */\n>  \n> +/** \\todo Revisit the ControlValue layout when stabilizing the ABI */\n>  static_assert(sizeof(ControlValue) == 16, \"Invalid size of ControlValue class\");\n>  \n>  /**\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-lj1-x242.google.com (mail-lj1-x242.google.com\n\t[IPv6:2a00:1450:4864:20::242])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 143B860418\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 20 Mar 2020 02:12:06 +0100 (CET)","by mail-lj1-x242.google.com with SMTP id g12so4712217ljj.3\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 19 Mar 2020 18:12:06 -0700 (PDT)","from localhost (h-200-138.A463.priv.bahnhof.se. [176.10.200.138])\n\tby smtp.gmail.com with ESMTPSA id\n\tf8sm2403782ljc.86.2020.03.19.18.12.04\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 19 Mar 2020 18:12:04 -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\tbh=bpsVcmx0iqIHXN5m22aYaeyRmeXp5QcQ6jsWwCiQ2mg=;\n\tb=GLzuW+BcfUuTs2wRbB+IfVy3ix41LiV6aa5E3qamig7tDfCyIxc41F5f4kDp3rHyyg\n\tLmQcgyjQB+S/jjh83AmXwA/Rdi6jIF8aIwMeNOmVSr30VPcS0BpVdU2MgbVkpWSJ7igw\n\tFdz3EXKmxPVIDTAxCdOoL0TO9Vy/4yorQE3a9DiB9kAqeyjROGh0mOHWEtF3I2NN37W4\n\tuddoy775rWkpSiy1cH/bNY0StqW7iSs0WQ7WXbUPGFaBUURShV/pipFFoLdP9Yw4oZx+\n\t1Q6+SKY65WOpoyelB8Sl5rtgXkZB3qA7NIpM5nLa9ry1PasQwgHE8Vd89H4fBSrE3cTs\n\tnSrg==","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;\n\tbh=bpsVcmx0iqIHXN5m22aYaeyRmeXp5QcQ6jsWwCiQ2mg=;\n\tb=LUKGpavyP3r5oKHAzCimatGatq9xKMoDlU9XbfnwR+ww5+c99/IByVIycZBFIS9BDK\n\tsGUb3PmELM1hiI/HkdmRRCAWQUHBYgWQOhJTQO0mvar7RcPvO5HDV/7wBFabkKbbexaY\n\tnpI62CJQXqOWqsgcIgrHXgp5n1iSqFHJNcuvasb4DFEiVCMNZKRVNmgdzgNIegP6jOOx\n\tCkF8YXXAYXKlbs8XNZlKypgSkwp0BM2PMUM9sJ4xlFu5DUkM23ODQ+y3eQui9baRLPFB\n\tBmdr3BzPAmjYnbMnwiK53PTav2W20wo1b/MJUPMD8c3vwVg6fVIB/5w71oFr9mo2Ossr\n\tYMaA==","X-Gm-Message-State":"ANhLgQ2zcgHuHSI2KdhDiRUWOt3nyLgEKoUWUry3xxSzqjoSuk5mA6y6\n\t9tcdLJ8zjFV86DcLnFowfmngig==","X-Google-Smtp-Source":"ADFU+vvWb+dfj+FzHti8R2S5onmjnkNRK8JlJSpZWD8kMTPVK2elOCU67imdgO1EmDXkmLz6b4DeWg==","X-Received":"by 2002:a2e:9c8e:: with SMTP id x14mr3733053lji.65.1584666725320;\n\tThu, 19 Mar 2020 18:12:05 -0700 (PDT)","Date":"Fri, 20 Mar 2020 02:12:04 +0100","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, Jan Engelhardt <jengelh@inai.de>","Message-ID":"<20200320011204.GB3558939@oden.dyn.berto.se>","References":"<20200320005551.10226-1-laurent.pinchart@ideasonboard.com>\n\t<20200320005551.10226-2-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":"<20200320005551.10226-2-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: controls: Don't\n\tover-optimize ControlValue layout","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":"Fri, 20 Mar 2020 01:12:06 -0000"}},{"id":4129,"web_url":"https://patchwork.libcamera.org/comment/4129/","msgid":"<9fa96ed0-f54b-913f-5943-7293f551beb0@ideasonboard.com>","date":"2020-03-20T12:57:38","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: controls: Don't\n\tover-optimize ControlValue layout","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 20/03/2020 00:55, Laurent Pinchart wrote:\n> The ControlValue class size should be minimized to save space, as it can\n> be instantiated in large numbers. The current implementation does so by\n> specifying several members as bitfields, but does so too aggressively,\n> resulting in fields being packed in an inefficient to access way on some\n> platforms. For instance, on 32-bit x86, the numElements_ field is offset\n> by 7 bits in a 32-bit word. This additionally causes a static assert\n> that checks the size of the class to fail.\n> \n> Relax the constraints on the isArray_ and numElements_ fields to avoid\n> inefficient access, and to ensure that the class size is identical\n> across all platforms. This will anyway need to be revisited when\n> stabilizing the ABI, so add a \\todo comment as a reminder.\n> \n> Fixes: 1fa4b43402a0 (\"libcamera: controls: Support array controls in ControlValue\")\n> Reported-by: Jan Engelhardt <jengelh@inai.de>\n> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> ---\n>  include/libcamera/controls.h | 4 ++--\n>  src/libcamera/controls.cpp   | 1 +\n>  2 files changed, 3 insertions(+), 2 deletions(-)\n> \n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index 2fca975f7512..40a29189ba01 100644\n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -158,8 +158,8 @@ public:\n>  \n>  private:\n>  \tControlType type_ : 8;\n> -\tbool isArray_ : 1;\n> -\tstd::size_t numElements_ : 16;\n> +\tbool isArray_;\n> +\tstd::size_t numElements_ : 32;\n\nOut of interest, is using \"std::size_t : 32;\" here any better than using\nint32_t?\n\n>  \tunion {\n>  \t\tuint64_t value_;\n>  \t\tvoid *storage_;\n> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> index 4615e71d7e3c..3c910d38f05d 100644\n> --- a/src/libcamera/controls.cpp\n> +++ b/src/libcamera/controls.cpp\n> @@ -83,6 +83,7 @@ static constexpr size_t ControlValueSize[] = {\n>   * \\brief Abstract type representing the value of a control\n>   */\n>  \n> +/** \\todo Revisit the ControlValue layout when stabilizing the ABI */\n\nRevisit how? I assume if the ControlValue size changes the assert will\nfire anyway so it's somewhat self documenting at that point ?\n\nNot objecting to the comment/todo - but curious as to what (in X months)\nthe reader will have to interpret that message as, to determine 'what'\nneeds to be done to satisfy this todo.\n\nAnyway, nothing to block:\n\n\nReviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n\n>  static_assert(sizeof(ControlValue) == 16, \"Invalid size of ControlValue class\");\n>  \n>  /**\n>","headers":{"Return-Path":"<kieran.bingham@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BFE6760416\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 20 Mar 2020 13:57:41 +0100 (CET)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 125CF504;\n\tFri, 20 Mar 2020 13:57:41 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1584709061;\n\tbh=gbgmwH/wIWinZxf2XDLN/H8fL88lBeYwcl6u3cwsckE=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=NURFMgzn+vWrPd4FAKYxdOW1zjGj0CtzAfgCVxyEX6ZxyWZRStWW9cQpNmPjQPyd2\n\tLjYcRE03KG0rQ7LC/E8IBoy6xrnsc9tceI5QZSm8yZdiiQbEGISumFBCXmBgYVDqar\n\tvRrxDkrDqtuksTVfWKSOmGU9g2+9sNec+pPvfnDs=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Cc":"Jan Engelhardt <jengelh@inai.de>","References":"<20200320005551.10226-1-laurent.pinchart@ideasonboard.com>\n\t<20200320005551.10226-2-laurent.pinchart@ideasonboard.com>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<9fa96ed0-f54b-913f-5943-7293f551beb0@ideasonboard.com>","Date":"Fri, 20 Mar 2020 12:57:38 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.9.1","MIME-Version":"1.0","In-Reply-To":"<20200320005551.10226-2-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: controls: Don't\n\tover-optimize ControlValue layout","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":"Fri, 20 Mar 2020 12:57:41 -0000"}},{"id":4130,"web_url":"https://patchwork.libcamera.org/comment/4130/","msgid":"<19fb0b0e-7f8e-4e66-2c5b-61981cb14c5f@ideasonboard.com>","date":"2020-03-20T13:29:17","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: controls: Don't\n\tover-optimize ControlValue layout","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 20/03/2020 00:55, Laurent Pinchart wrote:\n> The ControlValue class size should be minimized to save space, as it can\n> be instantiated in large numbers. The current implementation does so by\n> specifying several members as bitfields, but does so too aggressively,\n> resulting in fields being packed in an inefficient to access way on some\n> platforms. For instance, on 32-bit x86, the numElements_ field is offset\n> by 7 bits in a 32-bit word. This additionally causes a static assert\n> that checks the size of the class to fail.\n> \n> Relax the constraints on the isArray_ and numElements_ fields to avoid\n> inefficient access, and to ensure that the class size is identical\n> across all platforms. This will anyway need to be revisited when\n> stabilizing the ABI, so add a \\todo comment as a reminder.\n\n\"This will need to be revisited when stabilizing the ABI anyway, ...\"\n\nPutting 'anyway' that early in the sentence sounds odd.\n\nMaybe there's a grammatical rule for it - but I don't know it specifically.\n\n> Fixes: 1fa4b43402a0 (\"libcamera: controls: Support array controls in ControlValue\")\n> Reported-by: Jan Engelhardt <jengelh@inai.de>\n> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> ---\n>  include/libcamera/controls.h | 4 ++--\n>  src/libcamera/controls.cpp   | 1 +\n>  2 files changed, 3 insertions(+), 2 deletions(-)\n> \n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index 2fca975f7512..40a29189ba01 100644\n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -158,8 +158,8 @@ public:\n>  \n>  private:\n>  \tControlType type_ : 8;\n> -\tbool isArray_ : 1;\n> -\tstd::size_t numElements_ : 16;\n> +\tbool isArray_;\n> +\tstd::size_t numElements_ : 32;\n>  \tunion {\n>  \t\tuint64_t value_;\n>  \t\tvoid *storage_;\n> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> index 4615e71d7e3c..3c910d38f05d 100644\n> --- a/src/libcamera/controls.cpp\n> +++ b/src/libcamera/controls.cpp\n> @@ -83,6 +83,7 @@ static constexpr size_t ControlValueSize[] = {\n>   * \\brief Abstract type representing the value of a control\n>   */\n>  \n> +/** \\todo Revisit the ControlValue layout when stabilizing the ABI */\n>  static_assert(sizeof(ControlValue) == 16, \"Invalid size of ControlValue class\");\n>  \n>  /**\n>","headers":{"Return-Path":"<kieran.bingham@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AA91A60416\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 20 Mar 2020 14:29:22 +0100 (CET)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EB374504;\n\tFri, 20 Mar 2020 14:29:21 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1584710962;\n\tbh=ebn8/dhwNjtpjyFmv7dOUiobFEAIlkadqgyUYs8b6vo=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=cd8oQ5+xnZg+KCs3XelvaAM+h6VsdQoSkhDW4jVB3M4YfzUyHyhR6dA2EqYtd5Ecx\n\t2BPhpO70BYb4jKtTEDRWHim3A3axafuDvSsxXxo9RKbqrsm3I3ykCnVjmfZRqpXy3k\n\tp3otY3vgbMVFR2zTnpOd6Wsdf7l1VKHzMZh1Sduo=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Cc":"Jan Engelhardt <jengelh@inai.de>","References":"<20200320005551.10226-1-laurent.pinchart@ideasonboard.com>\n\t<20200320005551.10226-2-laurent.pinchart@ideasonboard.com>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<19fb0b0e-7f8e-4e66-2c5b-61981cb14c5f@ideasonboard.com>","Date":"Fri, 20 Mar 2020 13:29:17 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.9.1","MIME-Version":"1.0","In-Reply-To":"<20200320005551.10226-2-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: controls: Don't\n\tover-optimize ControlValue layout","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":"Fri, 20 Mar 2020 13:29:22 -0000"}},{"id":4135,"web_url":"https://patchwork.libcamera.org/comment/4135/","msgid":"<20200320140753.GJ5193@pendragon.ideasonboard.com>","date":"2020-03-20T14:07:53","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: controls: Don't\n\tover-optimize ControlValue layout","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Fri, Mar 20, 2020 at 12:57:38PM +0000, Kieran Bingham wrote:\n> On 20/03/2020 00:55, Laurent Pinchart wrote:\n> > The ControlValue class size should be minimized to save space, as it can\n> > be instantiated in large numbers. The current implementation does so by\n> > specifying several members as bitfields, but does so too aggressively,\n> > resulting in fields being packed in an inefficient to access way on some\n> > platforms. For instance, on 32-bit x86, the numElements_ field is offset\n> > by 7 bits in a 32-bit word. This additionally causes a static assert\n> > that checks the size of the class to fail.\n> > \n> > Relax the constraints on the isArray_ and numElements_ fields to avoid\n> > inefficient access, and to ensure that the class size is identical\n> > across all platforms. This will anyway need to be revisited when\n> > stabilizing the ABI, so add a \\todo comment as a reminder.\n> > \n> > Fixes: 1fa4b43402a0 (\"libcamera: controls: Support array controls in ControlValue\")\n> > Reported-by: Jan Engelhardt <jengelh@inai.de>\n> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> > ---\n> >  include/libcamera/controls.h | 4 ++--\n> >  src/libcamera/controls.cpp   | 1 +\n> >  2 files changed, 3 insertions(+), 2 deletions(-)\n> > \n> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > index 2fca975f7512..40a29189ba01 100644\n> > --- a/include/libcamera/controls.h\n> > +++ b/include/libcamera/controls.h\n> > @@ -158,8 +158,8 @@ public:\n> >  \n> >  private:\n> >  \tControlType type_ : 8;\n> > -\tbool isArray_ : 1;\n> > -\tstd::size_t numElements_ : 16;\n> > +\tbool isArray_;\n> > +\tstd::size_t numElements_ : 32;\n> \n> Out of interest, is using \"std::size_t : 32;\" here any better than using\n> int32_t?\n\nsize_t is the type used in the C++ standard library to store sizes, and\nit makes sense to use it through the API I think. It's 64-bit long on 64\nbit platforms, which would be overkill here :-)\n\n(and it's an unsigned type, so it would be uint32_t)\n\n> >  \tunion {\n> >  \t\tuint64_t value_;\n> >  \t\tvoid *storage_;\n> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > index 4615e71d7e3c..3c910d38f05d 100644\n> > --- a/src/libcamera/controls.cpp\n> > +++ b/src/libcamera/controls.cpp\n> > @@ -83,6 +83,7 @@ static constexpr size_t ControlValueSize[] = {\n> >   * \\brief Abstract type representing the value of a control\n> >   */\n> >  \n> > +/** \\todo Revisit the ControlValue layout when stabilizing the ABI */\n> \n> Revisit how? I assume if the ControlValue size changes the assert will\n> fire anyway so it's somewhat self documenting at that point ?\n\nI mean that, when stabilizing the ABI, we'll have to look at the\nControlValue layout and decide how to pack it efficiently. As long as\nthe ControlValue implementation is still in flux it would be premature\noptimization I think.\n\n> Not objecting to the comment/todo - but curious as to what (in X months)\n> the reader will have to interpret that message as, to determine 'what'\n> needs to be done to satisfy this todo.\n> \n> Anyway, nothing to block:\n> \n> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> \n> >  static_assert(sizeof(ControlValue) == 16, \"Invalid size of ControlValue class\");\n> >  \n> >  /**","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C943260416\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 20 Mar 2020 15:07:59 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4EADF504;\n\tFri, 20 Mar 2020 15:07:59 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1584713279;\n\tbh=EiKgElmVnOYU5TpHfPjDa2H4X/ShOdVgjp1/dJcR+HE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=O6bau0lGnNIpII6by17ko+zpehh1ImXrMHohDT2q9y+yvNOzlaVcyYO+LqArSTWmO\n\tL+pfxqsqVY2SjjjRgZYJUmIQK20T3okG3dysJcxQ6W6PAyJMZG5/GjJSLgp6ktKXcW\n\th4hSsuIWwYlBA1kwtKWRX8XrR9nYZ5DZV3Jupn1g=","Date":"Fri, 20 Mar 2020 16:07:53 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org, Jan Engelhardt <jengelh@inai.de>","Message-ID":"<20200320140753.GJ5193@pendragon.ideasonboard.com>","References":"<20200320005551.10226-1-laurent.pinchart@ideasonboard.com>\n\t<20200320005551.10226-2-laurent.pinchart@ideasonboard.com>\n\t<9fa96ed0-f54b-913f-5943-7293f551beb0@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<9fa96ed0-f54b-913f-5943-7293f551beb0@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: controls: Don't\n\tover-optimize ControlValue layout","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":"Fri, 20 Mar 2020 14:08:00 -0000"}},{"id":4136,"web_url":"https://patchwork.libcamera.org/comment/4136/","msgid":"<nycvar.YFH.7.76.2003201502530.9833@n3.vanv.qr>","date":"2020-03-20T14:12:50","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: controls: Don't\n\tover-optimize ControlValue layout","submitter":{"id":41,"url":"https://patchwork.libcamera.org/api/people/41/","name":"Jan Engelhardt","email":"jengelh@inai.de"},"content":"On Friday 2020-03-20 14:29, Kieran Bingham wrote:\n>\n>>This will anyway need to be revisited when stabilizing the ABI...\n>\n>Putting 'anyway' that early in the sentence sounds odd.\n>\n>Maybe there's a grammatical rule for it - but I don't know it specifically.\n\nYes, here:\nhttp://www.mit.edu/course/21/21.guide/cnj-adv.htm","headers":{"Return-Path":"<jengelh@inai.de>","Received":["from a3.inai.de (a3.inai.de [IPv6:2a01:4f8:10b:45d8::f5])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B495460416\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 20 Mar 2020 15:12:50 +0100 (CET)","by a3.inai.de (Postfix, from userid 25121)\n\tid 38ADE5872C742; Fri, 20 Mar 2020 15:12:50 +0100 (CET)","from localhost (localhost [127.0.0.1])\n\tby a3.inai.de (Postfix) with ESMTP id 37CC460DB7119;\n\tFri, 20 Mar 2020 15:12:50 +0100 (CET)"],"Date":"Fri, 20 Mar 2020 15:12:50 +0100 (CET)","From":"Jan Engelhardt <jengelh@inai.de>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","cc":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>, \n\tlibcamera-devel@lists.libcamera.org","In-Reply-To":"<19fb0b0e-7f8e-4e66-2c5b-61981cb14c5f@ideasonboard.com>","Message-ID":"<nycvar.YFH.7.76.2003201502530.9833@n3.vanv.qr>","References":"<20200320005551.10226-1-laurent.pinchart@ideasonboard.com>\n\t<20200320005551.10226-2-laurent.pinchart@ideasonboard.com>\n\t<19fb0b0e-7f8e-4e66-2c5b-61981cb14c5f@ideasonboard.com>","User-Agent":"Alpine 2.21 (LSU 202 2017-01-01)","MIME-Version":"1.0","Content-Type":"text/plain; charset=US-ASCII","Subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: controls: Don't\n\tover-optimize ControlValue layout","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":"Fri, 20 Mar 2020 14:12:51 -0000"}},{"id":4137,"web_url":"https://patchwork.libcamera.org/comment/4137/","msgid":"<20200320141736.GK5193@pendragon.ideasonboard.com>","date":"2020-03-20T14:17:36","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: controls: Don't\n\tover-optimize ControlValue layout","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Fri, Mar 20, 2020 at 01:29:17PM +0000, Kieran Bingham wrote:\n> On 20/03/2020 00:55, Laurent Pinchart wrote:\n> > The ControlValue class size should be minimized to save space, as it can\n> > be instantiated in large numbers. The current implementation does so by\n> > specifying several members as bitfields, but does so too aggressively,\n> > resulting in fields being packed in an inefficient to access way on some\n> > platforms. For instance, on 32-bit x86, the numElements_ field is offset\n> > by 7 bits in a 32-bit word. This additionally causes a static assert\n> > that checks the size of the class to fail.\n> > \n> > Relax the constraints on the isArray_ and numElements_ fields to avoid\n> > inefficient access, and to ensure that the class size is identical\n> > across all platforms. This will anyway need to be revisited when\n> > stabilizing the ABI, so add a \\todo comment as a reminder.\n> \n> \"This will need to be revisited when stabilizing the ABI anyway, ...\"\n> \n> Putting 'anyway' that early in the sentence sounds odd.\n> \n> Maybe there's a grammatical rule for it - but I don't know it specifically.\n\nOK :-)\n\n> > Fixes: 1fa4b43402a0 (\"libcamera: controls: Support array controls in ControlValue\")\n> > Reported-by: Jan Engelhardt <jengelh@inai.de>\n> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> > ---\n> >  include/libcamera/controls.h | 4 ++--\n> >  src/libcamera/controls.cpp   | 1 +\n> >  2 files changed, 3 insertions(+), 2 deletions(-)\n> > \n> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > index 2fca975f7512..40a29189ba01 100644\n> > --- a/include/libcamera/controls.h\n> > +++ b/include/libcamera/controls.h\n> > @@ -158,8 +158,8 @@ public:\n> >  \n> >  private:\n> >  \tControlType type_ : 8;\n> > -\tbool isArray_ : 1;\n> > -\tstd::size_t numElements_ : 16;\n> > +\tbool isArray_;\n> > +\tstd::size_t numElements_ : 32;\n> >  \tunion {\n> >  \t\tuint64_t value_;\n> >  \t\tvoid *storage_;\n> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > index 4615e71d7e3c..3c910d38f05d 100644\n> > --- a/src/libcamera/controls.cpp\n> > +++ b/src/libcamera/controls.cpp\n> > @@ -83,6 +83,7 @@ static constexpr size_t ControlValueSize[] = {\n> >   * \\brief Abstract type representing the value of a control\n> >   */\n> >  \n> > +/** \\todo Revisit the ControlValue layout when stabilizing the ABI */\n> >  static_assert(sizeof(ControlValue) == 16, \"Invalid size of ControlValue class\");\n> >  \n> >  /**","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C160860415\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 20 Mar 2020 15:17:42 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2D088504;\n\tFri, 20 Mar 2020 15:17:42 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1584713862;\n\tbh=GrD86QwqsKwKawv7z2MH8esQt4iPwMBLoGsRzX5Poqo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Vu4aPGSNraOHjwUzWP4yQSYrdMTUe6v7ifb/9Sj5md5EyS4+XWkfkLjZVO7RYyDWT\n\tMx87qH5zHTEr5Mr6MZGpJYX8Y3HCFq8kfgwWi6vnXoxl9JxCynBgp8pvURniFGbyIx\n\tJwhRLXzrK/rZIQDcoM7Y2zNC3UgFIHAoFRIYxcAk=","Date":"Fri, 20 Mar 2020 16:17:36 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org, Jan Engelhardt <jengelh@inai.de>","Message-ID":"<20200320141736.GK5193@pendragon.ideasonboard.com>","References":"<20200320005551.10226-1-laurent.pinchart@ideasonboard.com>\n\t<20200320005551.10226-2-laurent.pinchart@ideasonboard.com>\n\t<19fb0b0e-7f8e-4e66-2c5b-61981cb14c5f@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<19fb0b0e-7f8e-4e66-2c5b-61981cb14c5f@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: controls: Don't\n\tover-optimize ControlValue layout","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":"Fri, 20 Mar 2020 14:17:42 -0000"}},{"id":4138,"web_url":"https://patchwork.libcamera.org/comment/4138/","msgid":"<c7b26fd0-a190-069b-e9ff-881780f1768a@ideasonboard.com>","date":"2020-03-20T14:26:39","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: controls: Don't\n\tover-optimize ControlValue layout","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Jan,\n\nOn 20/03/2020 14:12, Jan Engelhardt wrote:\n> \n> On Friday 2020-03-20 14:29, Kieran Bingham wrote:\n>>\n>>> This will anyway need to be revisited when stabilizing the ABI...\n>>\n>> Putting 'anyway' that early in the sentence sounds odd.\n>>\n>> Maybe there's a grammatical rule for it - but I don't know it specifically.\n> \n> Yes, here:\n> http://www.mit.edu/course/21/21.guide/cnj-adv.htm\n\nAha thanks, the disadvantage to being a native speaker is grammatical\nfaults trigger the 'warning light' - but it's just a single flashing\nstatus LED with no error value linking to the documentation :-D","headers":{"Return-Path":"<kieran.bingham@ideasonboard.com>","Received":["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 F034760415\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 20 Mar 2020 15:26:43 +0100 (CET)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4AF86504;\n\tFri, 20 Mar 2020 15:26:43 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1584714403;\n\tbh=fJ267XuVHCpD+ac+6ZVS46N2DYoHHkNW4HDs4LpE9nc=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=du7PNARhN9VNtCJGCCFr0XA5BvE7s/hlVWrm9V6S6rwRsCz6QGj/nzEfCKeBojZMP\n\t4G+imGyiMBPsaewdEEmbHlK7XkE925XDArz7W/sxdHcpKMa7IeLgnzBqKD+hj9Djm4\n\tNHDPDrXQIYQUyXBxSdbImauUvyOcqQ3bnj4KvY0Q=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Jan Engelhardt <jengelh@inai.de>","Cc":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","References":"<20200320005551.10226-1-laurent.pinchart@ideasonboard.com>\n\t<20200320005551.10226-2-laurent.pinchart@ideasonboard.com>\n\t<19fb0b0e-7f8e-4e66-2c5b-61981cb14c5f@ideasonboard.com>\n\t<nycvar.YFH.7.76.2003201502530.9833@n3.vanv.qr>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<c7b26fd0-a190-069b-e9ff-881780f1768a@ideasonboard.com>","Date":"Fri, 20 Mar 2020 14:26:39 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.9.1","MIME-Version":"1.0","In-Reply-To":"<nycvar.YFH.7.76.2003201502530.9833@n3.vanv.qr>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: controls: Don't\n\tover-optimize ControlValue layout","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":"Fri, 20 Mar 2020 14:26:44 -0000"}}]