[libcamera-devel] [PATCH 6/6] libcamera: control_serializer: Serialize info::def()

[Laurent Pinchart]

On Fri, Sep 03, 2021 at 02:42:28PM +0900, paul.elder at ideasonboard.com wrote:
> On Wed, Sep 01, 2021 at 04:38:00PM +0200, Jacopo Mondi wrote:
> > The ControlInfo class was originally designed to only transport
> > the control's minimum and maximum values which represent the control's
> > valid limits.
> > 
> > Later the default value of the control has been added to the ControlInfo
> > class, but the control serializer implementation has not been updated
> > accordingly.
> > 
> > This causes issues to IPA modules making use of ControlInfo::def() as,
> 
> s/to/in/
> 
> > when running in isolation, they would receive a 0 value as default.
> 
> s/ as default//
> 
> There's no way to turn off receiving the "default" :D
> 
> > Fix that by serializing and deserializing the additional ControlValue
> > and update the protocol description accordingly.
> > 
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/libcamera/control_serializer.cpp |  6 ++++--
> >  src/libcamera/ipa_controls.cpp       | 14 ++++++++------
> >  2 files changed, 12 insertions(+), 8 deletions(-)
> > 
> > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp
> > index 7eef041a16ee..4e8045251938 100644
> > --- a/src/libcamera/control_serializer.cpp
> > +++ b/src/libcamera/control_serializer.cpp
> > @@ -105,7 +105,7 @@ size_t ControlSerializer::binarySize(const ControlValue &value)
> >  
> >  size_t ControlSerializer::binarySize(const ControlInfo &info)
> >  {
> > -	return binarySize(info.min()) + binarySize(info.max());
> > +	return binarySize(info.min()) + binarySize(info.max()) + binarySize(info.def());
> >  }
> >  
> >  /**
> > @@ -158,6 +158,7 @@ void ControlSerializer::store(const ControlInfo &info, ByteStreamBuffer &buffer)
> >  {
> >  	store(info.min(), buffer);
> >  	store(info.max(), buffer);
> > +	store(info.def(), buffer);
> >  }
> >  
> >  /**
> > @@ -346,8 +347,9 @@ ControlInfo ControlSerializer::loadControlInfo(ControlType type,
> >  
> >  	ControlValue min = loadControlValue(type, b);
> >  	ControlValue max = loadControlValue(type, b);
> > +	ControlValue def = loadControlValue(type, b);
> >  
> > -	return ControlInfo(min, max);
> > +	return ControlInfo(min, max, def);
> >  }
> >  
> >  /**
> > diff --git a/src/libcamera/ipa_controls.cpp b/src/libcamera/ipa_controls.cpp
> > index fb98cda30ede..3d172d66b30f 100644
> > --- a/src/libcamera/ipa_controls.cpp
> > +++ b/src/libcamera/ipa_controls.cpp
> > @@ -108,17 +108,19 @@
> >   *           +-------------------------+       .
> >   *         / | ...                     |       | entry[n].offset
> >   *         | +-------------------------+ <-----´
> > - *    Data | | minimum value (#n)      | \
> > - * section | +-------------------------+ | Entry #n
> > - *         | | maximum value (#n)      | /
> > + *         | | minimum value (#n)      | \
> > + *    Data | +-------------------------+ |
> > + * section | | maximum value (#n)      | | Entry #n
> > + *         | +-------------------------+ |
> > + *         | | default value (#n)      | /
> >   *         | +-------------------------+
> >   *         \ | ...                     |
> >   *           +-------------------------+
> >   * ~~~~
> >   *
> > - * The minimum and maximum value are stored in the platform's native data
> > - * format. The ipa_control_info_entry::offset field stores the offset from the
> > - * beginning of the data section to the info data.
> > + * The minimum, maximum and default value are stored in the platform's native
> 
> s/maximum/maximum,/

Oxford commas https://en.wikipedia.org/wiki/Serial_comma :-) (I would
have gone without it here, or have kept value as a singular to only
refer to default.)

> s/value/values/

Agreed.

> Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> > + * data format. The ipa_control_info_entry::offset field stores the offset from
> > + * the beginning of the data section to the info data.
> >   *
> >   * Info data in the data section shall be stored in the same order as the
> >   * entries array, shall be aligned to a multiple of 8 bytes, and shall be

-- 
Regards,

Laurent Pinchart