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

[Christian Rauch]

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