[libcamera-devel] [PATCH] libcamera: control: initialise control info to ControlTypeNone by default

[Jacopo Mondi]

Hi Christian
  I had two failures in running ninja test

   7/71 libcamera:controls / control_info
   FAIL            0.11s   killed by signal 6 SIGABRT

   27/71 libcamera:serialization / ipa_data_serializer_test
   FAIL            0.03s   (exit status 255 or signal 127 SIGinvalid)

Could you check if you can reproduce on your side ?

Thanks
  j

On Tue, Aug 30, 2022 at 09:30:49AM +0200, Jacopo Mondi via libcamera-devel wrote:
> Hi Christian
>
> On Fri, Aug 26, 2022 at 07:34:26PM +0200, Christian Rauch via libcamera-devel wrote:
> > The default ControlInfo constructor allows to partially initialised the
> > min/max/def values. Uninitialised values are assigned to 0 by default. This
> > implicit initialisation makes it impossible to distinguish between and
> > uninitialised and an explicitly 0-initialised ControlValue.
> >
> > Default construct the ControlValue in the ControlInfo default contructor to
> > explicitly represent uninitialised values by the ControlTypeNone type.
> >
> > Signed-off-by: Christian Rauch <Rauch.Christian at gmx.de>
>
> I think the idea is good: make sure we don't leave the ControlInfo
> members unintentionally initialized to 0. This would certainly lead to
> a more robust code base.
>
> However, as soon as we try to access a min/max/def which is now
> initialized we get into a runtime error, and I'm not sure in how many
> places in the code base we happily access a 0-initialized member
> without noticing.
>
> Hence, I think this patch is worth being run through  at least CTS,
> preferably throught all the run-time tests we, and the projects using,
> libcamera have (I'm thinking libcamera-apps, in example).
>
> We'll sync internally on how to give this patch a good brush.
>
> Thanks
>    j
>
> > ---
> >  include/libcamera/controls.h | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > index ebc168fc..38d0a3e8 100644
> > --- a/include/libcamera/controls.h
> > +++ b/include/libcamera/controls.h
> > @@ -268,9 +268,9 @@ private:
> >  class ControlInfo
> >  {
> >  public:
> > -	explicit ControlInfo(const ControlValue &min = 0,
> > -			     const ControlValue &max = 0,
> > -			     const ControlValue &def = 0);
> > +	explicit ControlInfo(const ControlValue &min = {},
> > +			     const ControlValue &max = {},
> > +			     const ControlValue &def = {});
> >  	explicit ControlInfo(Span<const ControlValue> values,
> >  			     const ControlValue &def = {});
> >  	explicit ControlInfo(std::set<bool> values, bool def);
> > --
> > 2.34.1
> >