[libcamera-devel] libcamera: control: initialise control info to ControlTypeNone by default
diff mbox series

Message ID 20220826173426.633461-1-Rauch.Christian@gmx.de
State Superseded
Headers show
Series
  • [libcamera-devel] libcamera: control: initialise control info to ControlTypeNone by default
Related show

Commit Message

Christian Rauch Aug. 26, 2022, 5:34 p.m. UTC 
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@gmx.de>
---
 include/libcamera/controls.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

--
2.34.1

Comments

Jacopo Mondi Aug. 30, 2022, 7:30 a.m. UTC | #1 
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@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
>
Jacopo Mondi Aug. 30, 2022, 9:07 a.m. UTC | #2 
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@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
> >
Christian Rauch Aug. 30, 2022, 8:24 p.m. UTC | #3 
Hi Jacopo,

I could reproduce the issue with the 'controls / control_info' test and
fixed it with v2 of this. The 'serialization / ipa_data_serializer_test'
test was skipped with "exit status 77".

Best,
Christian


Am 30.08.22 um 11:07 schrieb 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@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
>>>

Patch
diff mbox series 

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);