[libcamera-devel] [PATCH v3 03/14] libcamera: v4l2_device: Add method to retrieve all supported controls
Kieran Bingham
kieran.bingham at ideasonboard.com
Tue Jul 2 11:13:31 CEST 2019
Hi Laurent,
On 01/07/2019 12:00, Laurent Pinchart wrote:
> Hi Kieran,
>
> On Mon, Jul 01, 2019 at 09:03:09AM +0100, Kieran Bingham wrote:
>> On 01/07/2019 00:38, Laurent Pinchart wrote:
>>> Add a new controls() method to the V4L2Device class to retrieve the map
>>> of all supported controls. This is needed in order to dynamically query
>>> the supported controls, for instance for drivers that support different
>>> sets of controls depending on the device model.
>>>
>>> To make the API easier to use, create a type alias for the control ID to
>>> ControlInfo and use it.
>>>
>>> Remove the getControlInfo() method that is not used externally, as it
>>> can now be replaced by accessing the full list of controls.
>>
>> Maybe it's the C-dev in me but I kinda prefer the lookup, with a null
>> return check over a lookup with open coding the iter != controls_.end();
>>
>> Functionally the same I guess, I think it's just more ingrained in my
>> head, but the .end() check is more 'STL' so it's certainly not a problem
>> really.
>
> I agree, so if we find this is too much hasle to use, I'm fine adding
> back a method to look up a single control (or I wonder if we could add a
> helper method to make look up easier on std::map).
>
>>> Update the CameraSensor API accordingly.
>>
>> Small comment on the map type chosen, but I think either std::map is
>> probably fine too.
>>
>> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>>
>>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>>> ---
>>> src/libcamera/camera_sensor.cpp | 10 ++++------
>>> src/libcamera/include/camera_sensor.h | 5 ++---
>>> src/libcamera/include/v4l2_controls.h | 6 ++++--
>>> src/libcamera/include/v4l2_device.h | 9 ++++-----
>>> src/libcamera/v4l2_controls.cpp | 5 +++++
>>> src/libcamera/v4l2_device.cpp | 25 +++++++++----------------
>>> 6 files changed, 28 insertions(+), 32 deletions(-)
>>>
>>> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
>>> index 471c3e2e6e47..14f631e8ecf7 100644
>>> --- a/src/libcamera/camera_sensor.cpp
>>> +++ b/src/libcamera/camera_sensor.cpp
>>> @@ -248,14 +248,12 @@ int CameraSensor::setFormat(V4L2SubdeviceFormat *format)
>>> }
>>>
>>> /**
>>> - * \brief Retrieve information about a control
>>> - * \param[in] id The control ID
>>> - * \return A pointer to the V4L2ControlInfo for control \a id, or nullptr
>>> - * if the sensor doesn't have such a control
>>> + * \brief Retrieve the supported V4L2 controls
>>> + * \return A map of the V4L2 controls supported by the sensor
>>> */
>>> -const V4L2ControlInfo *CameraSensor::getControlInfo(unsigned int id) const
>>> +const V4L2ControlInfoMap &CameraSensor::controls() const
>>> {
>>> - return subdev_->getControlInfo(id);
>>> + return subdev_->controls();
>>> }
>>>
>>> /**
>>> diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h
>>> index b42e7b8e1343..fe033fb374c1 100644
>>> --- a/src/libcamera/include/camera_sensor.h
>>> +++ b/src/libcamera/include/camera_sensor.h
>>> @@ -13,12 +13,11 @@
>>> #include <libcamera/geometry.h>
>>>
>>> #include "log.h"
>>> +#include "v4l2_controls.h"
>>>
>>> namespace libcamera {
>>>
>>> class MediaEntity;
>>> -class V4L2ControlInfo;
>>> -class V4L2ControlList;
>>> class V4L2Subdevice;
>>>
>>> struct V4L2SubdeviceFormat;
>>> @@ -43,7 +42,7 @@ public:
>>> const Size &size) const;
>>> int setFormat(V4L2SubdeviceFormat *format);
>>>
>>> - const V4L2ControlInfo *getControlInfo(unsigned int id) const;
>>> + const V4L2ControlInfoMap &controls() const;
>>> int getControls(V4L2ControlList *ctrls);
>>> int setControls(V4L2ControlList *ctrls);
>>>
>>> diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h
>>> index 0047efab11fa..10b726504951 100644
>>> --- a/src/libcamera/include/v4l2_controls.h
>>> +++ b/src/libcamera/include/v4l2_controls.h
>>> @@ -8,11 +8,11 @@
>>> #ifndef __LIBCAMERA_V4L2_CONTROLS_H__
>>> #define __LIBCAMERA_V4L2_CONTROLS_H__
>>>
>>> +#include <map>
>>> +#include <stdint.h>
>>> #include <string>
>>> #include <vector>
>>>
>>> -#include <stdint.h>
>>> -
>>> #include <linux/v4l2-controls.h>
>>> #include <linux/videodev2.h>
>>>
>>> @@ -41,6 +41,8 @@ private:
>>> int64_t max_;
>>> };
>>>
>>> +using V4L2ControlInfoMap = std::map<unsigned int, V4L2ControlInfo>;
>>
>> Does V4L2ControlInfoMap need to be ordered?
>> (I don't think we do at this part do we?)
>>
>> std::unordered_map could be faster if we don't need to order the map.
>> I think at the scale of controls we'll have it probably won't make much
>> difference...
>>
>> We could do some measurements perhaps on a map of 1000 elements vs an
>> unordered_map of 1000 elements, but I think that's the approx max scale
>> that we'll contain right?
>
> I think we'll be one, if not two orders of magnitude smaller. Would you
> like to perform measurements ? :-)
I've measured doing 1000 random lookups on maps of 1000 V4L2controlInfo
structures:
times in nanoSeconds:
std::map<unsigned int, V4L2ControlInfo>; ~479000 nS
std::unordered_map<unsigned int, V4L2ControlInfo>; ~175000 nS
When the number of entries is less than 50, the differences are very minor.
When it comes to creating a map and inserting only a small number of
entries, in fact the std::map starts to win.
Inserting 10 items, for 1000000 repeats
2707779741 nS: std::map: Create and Insert
3415575028 nS: std::unordered_map: Create and Insert
Yet, we're still talking the difference between 2.7 microSeconds and 3.4
microseconds per list. So I think the optimisations here are minimal :)
--
Kieran
>> As the key is an unsigned int, I think the hasher will function out of
>> the box?
>
> Correct.
>
>>> +
>>> class V4L2Control
>>> {
>>> public:
>>> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
>>> index 24a0134a2cba..e7e9829cb05f 100644
>>> --- a/src/libcamera/include/v4l2_device.h
>>> +++ b/src/libcamera/include/v4l2_device.h
>>> @@ -13,19 +13,18 @@
>>> #include <linux/videodev2.h>
>>>
>>> #include "log.h"
>>> +#include "v4l2_controls.h"
>>>
>>> namespace libcamera {
>>>
>>> -class V4L2ControlInfo;
>>> -class V4L2ControlList;
>>> -
>>> class V4L2Device : protected Loggable
>>> {
>>> public:
>>> void close();
>>> bool isOpen() const { return fd_ != -1; }
>>>
>>> - const V4L2ControlInfo *getControlInfo(unsigned int id) const;
>>> + const V4L2ControlInfoMap &controls() const { return controls_; }
>>> +
>>> int getControls(V4L2ControlList *ctrls);
>>> int setControls(V4L2ControlList *ctrls);
>>>
>>> @@ -48,7 +47,7 @@ private:
>>> const struct v4l2_ext_control *v4l2Ctrls,
>>> unsigned int count);
>>>
>>> - std::map<unsigned int, V4L2ControlInfo> controls_;
>>> + V4L2ControlInfoMap controls_;
>>> std::string deviceNode_;
>>> int fd_;
>>> };
>>> diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp
>>> index af017bce48ba..84258d9954d0 100644
>>> --- a/src/libcamera/v4l2_controls.cpp
>>> +++ b/src/libcamera/v4l2_controls.cpp
>>> @@ -114,6 +114,11 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)
>>> * \return The V4L2 control maximum value
>>> */
>>>
>>> +/**
>>> + * \typedef V4L2ControlInfoMap
>>> + * \brief A map of control ID to V4L2ControlInfo
>>> + */
>>> +
>>> /**
>>> * \class V4L2Control
>>> * \brief A V4L2 control value
>>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
>>> index 06939dead705..59fc98cefd4e 100644
>>> --- a/src/libcamera/v4l2_device.cpp
>>> +++ b/src/libcamera/v4l2_device.cpp
>>> @@ -111,19 +111,10 @@ void V4L2Device::close()
>>> */
>>>
>>> /**
>>> - * \brief Retrieve information about a control
>>> - * \param[in] id The control ID
>>> - * \return A pointer to the V4L2ControlInfo for control \a id, or nullptr
>>> - * if the device doesn't have such a control
>>> + * \fn V4L2Device::controls()
>>> + * \brief Retrieve the supported V4L2 controls
>>> + * \return A map of the V4L2 controls supported by the device
>>> */
>>> -const V4L2ControlInfo *V4L2Device::getControlInfo(unsigned int id) const
>>> -{
>>> - auto it = controls_.find(id);
>>> - if (it == controls_.end())
>>> - return nullptr;
>>> -
>>> - return &it->second;
>>> -}
>>>
>>> /**
>>> * \brief Read controls from the device
>>> @@ -158,13 +149,14 @@ int V4L2Device::getControls(V4L2ControlList *ctrls)
>>>
>>> for (unsigned int i = 0; i < count; ++i) {
>>> const V4L2Control *ctrl = ctrls->getByIndex(i);
>>> - const V4L2ControlInfo *info = getControlInfo(ctrl->id());
>>> - if (!info) {
>>> + const auto iter = controls_.find(ctrl->id());
>>> + if (iter == controls_.end()) {
>>> LOG(V4L2, Error)
>>> << "Control '" << ctrl->id() << "' not found";
>>> return -EINVAL;
>>> }
>>>
>>> + const V4L2ControlInfo *info = &iter->second;
>>> controlInfo[i] = info;
>>> v4l2Ctrls[i].id = info->id();
>>> }
>>> @@ -232,13 +224,14 @@ int V4L2Device::setControls(V4L2ControlList *ctrls)
>>>
>>> for (unsigned int i = 0; i < count; ++i) {
>>> const V4L2Control *ctrl = ctrls->getByIndex(i);
>>> - const V4L2ControlInfo *info = getControlInfo(ctrl->id());
>>> - if (!info) {
>>> + const auto iter = controls_.find(ctrl->id());
>>> + if (iter == controls_.end()) {
>>> LOG(V4L2, Error)
>>> << "Control '" << ctrl->id() << "' not found";
>>> return -EINVAL;
>>> }
>>>
>>> + const V4L2ControlInfo *info = &iter->second;
>>> controlInfo[i] = info;
>>> v4l2Ctrls[i].id = info->id();
>>>
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list