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

[Laurent Pinchart]

Hi Kieran,

On Fri, Mar 20, 2020 at 01:29:17PM +0000, Kieran Bingham wrote:
> 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.
> 
> "This will need to be revisited when stabilizing the ABI anyway, ..."
> 
> Putting 'anyway' that early in the sentence sounds odd.
> 
> Maybe there's a grammatical rule for it - but I don't know it specifically.

OK :-)

> > 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;
> >  	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 */
> >  static_assert(sizeof(ControlValue) == 16, "Invalid size of ControlValue class");
> >  
> >  /**

-- 
Regards,

Laurent Pinchart