[libcamera-devel] [PATCH 2/2] libcamera: controls: Don't over-optimize ControlValue layout

[Kieran Bingham]

Hi Laurent,

On 20/03/2020 00:55, Laurent Pinchart wrote:
> The ControlValue class size should be minimized to save space, as it can
> be instantiated in large numbers. The current implementation does so by
> specifying several members as bitfields, but does so too aggressively,
> resulting in fields being packed in an inefficient to access way on some
> platforms. For instance, on 32-bit x86, the numElements_ field is offset
> by 7 bits in a 32-bit word. This additionally causes a static assert
> that checks the size of the class to fail.
> 
> Relax the constraints on the isArray_ and numElements_ fields to avoid
> inefficient access, and to ensure that the class size is identical
> across all platforms. This will anyway need to be revisited when
> stabilizing the ABI, so add a \todo comment as a reminder.
> 
> Fixes: 1fa4b43402a0 ("libcamera: controls: Support array controls in ControlValue")
> Reported-by: Jan Engelhardt <jengelh at inai.de>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  include/libcamera/controls.h | 4 ++--
>  src/libcamera/controls.cpp   | 1 +
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> index 2fca975f7512..40a29189ba01 100644
> --- a/include/libcamera/controls.h
> +++ b/include/libcamera/controls.h
> @@ -158,8 +158,8 @@ public:
>  
>  private:
>  	ControlType type_ : 8;
> -	bool isArray_ : 1;
> -	std::size_t numElements_ : 16;
> +	bool isArray_;
> +	std::size_t numElements_ : 32;

Out of interest, is using "std::size_t : 32;" here any better than using
int32_t?

>  	union {
>  		uint64_t value_;
>  		void *storage_;
> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> index 4615e71d7e3c..3c910d38f05d 100644
> --- a/src/libcamera/controls.cpp
> +++ b/src/libcamera/controls.cpp
> @@ -83,6 +83,7 @@ static constexpr size_t ControlValueSize[] = {
>   * \brief Abstract type representing the value of a control
>   */
>  
> +/** \todo Revisit the ControlValue layout when stabilizing the ABI */

Revisit how? I assume if the ControlValue size changes the assert will
fire anyway so it's somewhat self documenting at that point ?

Not objecting to the comment/todo - but curious as to what (in X months)
the reader will have to interpret that message as, to determine 'what'
needs to be done to satisfy this todo.

Anyway, nothing to block:


Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

>  static_assert(sizeof(ControlValue) == 16, "Invalid size of ControlValue class");
>  
>  /**
> 

-- 
Regards
--
Kieran