[libcamera-devel] [PATCH 06/13] libcamera: controls: Allow read only access to control values

[Kieran Bingham]

Hi Jacopo,

On 28/08/2019 17:20, Jacopo Mondi wrote:
> Hi Niklas,
> 
> On Wed, Aug 28, 2019 at 03:17:03AM +0200, Niklas Söderlund wrote:
>> Allow the control values in a ControlList to be examined from a const
>> environment.
>>
>> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
>> ---
>>  include/libcamera/controls.h |  1 +
>>  src/libcamera/controls.cpp   | 34 ++++++++++++++++++++++++++++++++++
>>  2 files changed, 35 insertions(+)
>>
>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
>> index fbb3a62274c64259..eee5ef87b8fd2633 100644
>> --- a/include/libcamera/controls.h
>> +++ b/include/libcamera/controls.h
>> @@ -124,6 +124,7 @@ public:
>>  	void clear() { controls_.clear(); }
>>
>>  	ControlValue &operator[](ControlId id);
>> +	const ControlValue &operator[](ControlId id) const;
>>  	ControlValue &operator[](const ControlInfo *info) { return controls_[info]; }
>>
>>  	void update(const ControlList &list);
>> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
>> index 54efef1bb337e945..424fe515fa69f8d8 100644
>> --- a/src/libcamera/controls.cpp
>> +++ b/src/libcamera/controls.cpp
>> @@ -517,6 +517,40 @@ ControlValue &ControlList::operator[](ControlId id)
>>  	return controls_[&iter->second];
>>  }
>>
>> +/**
>> + * \brief Access a control specified by \a id
>> + * \param[in] id The control ID
>> + *
>> + * This method returns a reference to the control identified by \a id, if it
>> + * exsists or an empty control if it does not.
> 
> s/exsists/exists
> 
> I would rather return nullptr, both here and in the existing version
> of operator[], it would save creating the static empty control,
> doesn't it?
> 
>> + *
>> + * The behaviour is undefined if the control \a id is not supported by the
>> + * camera that the ControlList refers to.
> 
> If the control is not in the InfoMap returned by camera_->controls()
> won't iter be == controls.end() ?

I think this refers to the fact that if you try to have a control in a
ControlList which is not supported by the Controls on the Camera then it
is not valid.

> 
>> + *
>> + * \return A reference to the value of the control identified by \a id
>> + */
>> +const ControlValue &ControlList::operator[](ControlId id) const
>> +{
>> +	const ControlInfoMap &controls = camera_->controls();
>> +	static ControlValue empty;
>> +
>> +	const auto iter = controls.find(id);
>> +	if (iter == controls.end()) {
>> +		LOG(Controls, Error)
>> +			<< "Camera " << camera_->name()
>> +			<< " does not support control " << id;
>> +		return empty;
>> +	}
>> +
>> +	const auto it = controls_.find(&iter->second);
>> +	if (it == controls_.end()) {
>> +		LOG(Controls, Error) << "list does not have control " << id;
>> +		return empty;
>> +	}
> 
> Not on your patch, but why do we keep two lists of controls ??

This is a ControlList, which stores a list of controls as controls_.

But those Controls are required to be supported by the list of
camera_->controls(), as the ControlInfo data is specific to a camera.

IOW, you can't generate a controllist from one camera and apply it to
another.

--
Kieran


> 
>> +
>> +	return it->second;
>> +}
>> +
>>  /**
>>   * \fn ControlList::operator[](const ControlInfo *info)
>>   * \brief Access or insert the control specified by \a info
>> --
>> 2.22.1
>>
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel at lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel
>>
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel at lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards
--
Kieran