[libcamera-devel] [PATCH v3 3/3] libcamera: V4L2Device: Use Span in updateControls()
Kieran Bingham
kieran.bingham at ideasonboard.com
Wed Apr 21 12:21:36 CEST 2021
Hi Hiro,
On 21/04/2021 11:16, Laurent Pinchart wrote:
> Hi Hiro,
>
> Thank you for the patch.
>
> On Wed, Apr 21, 2021 at 06:38:57PM +0900, Hirokazu Honda wrote:
>> V4L2Device::updateControls() takes two arguments, raw array and
>> its size, for the v4l2_ext_control values. This replaces it with
>> libcamera::Span.
>>
>> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
>> ---
>> include/libcamera/internal/v4l2_device.h | 4 ++--
>> src/libcamera/v4l2_device.cpp | 13 ++++++-------
>> 2 files changed, 8 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h
>> index d006bf68..7c79ee97 100644
>> --- a/include/libcamera/internal/v4l2_device.h
>> +++ b/include/libcamera/internal/v4l2_device.h
>> @@ -14,6 +14,7 @@
>> #include <linux/videodev2.h>
>>
>> #include <libcamera/signal.h>
>> +#include <libcamera/span.h>
>>
>> #include "libcamera/internal/log.h"
>> #include "libcamera/internal/v4l2_controls.h"
>> @@ -55,8 +56,7 @@ protected:
>> private:
>> void listControls();
>> void updateControls(ControlList *ctrls,
>> - const struct v4l2_ext_control *v4l2Ctrls,
>> - unsigned int count);
>> + libcamera::Span<const v4l2_ext_control> v4l2Ctrls);
Given that we are in the libcamera namespace here, is the libcamera::
prefix required?
>>
>> void eventAvailable(EventNotifier *notifier);
>>
>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
>> index 109269b2..a59e8378 100644
>> --- a/src/libcamera/v4l2_device.cpp
>> +++ b/src/libcamera/v4l2_device.cpp
>> @@ -250,7 +250,8 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids)
>> v4l2Ctrls.resize(errorIdx);
>> }
>>
>> - updateControls(&ctrls, v4l2Ctrls.data(), v4l2Ctrls.size());
>> + updateControls(&ctrls,
>> + libcamera::Span<const v4l2_ext_control>(v4l2Ctrls));
>
> The Span constructor that takes a container is implicit, so I think you
> can write
>
> updateControls(&ctrls, v4l2Ctrls);
>
>>
>> return ctrls;
>> }
>> @@ -349,7 +350,8 @@ int V4L2Device::setControls(ControlList *ctrls)
>> ret = errorIdx;
>> }
>>
>> - updateControls(ctrls, v4l2Ctrls.data(), v4l2Ctrls.size());
>> + updateControls(ctrls,
>> + libcamera::Span<const v4l2_ext_control>(v4l2Ctrls));
>
> Same here.
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
>>
>> return ret;
>> }
>> @@ -513,14 +515,11 @@ void V4L2Device::listControls()
>> * values in \a v4l2Ctrls
>> * \param[inout] ctrls List of V4L2 controls to update
>> * \param[in] v4l2Ctrls List of V4L2 extended controls as returned by the driver
>> - * \param[in] count The number of controls to update
>> */
>> void V4L2Device::updateControls(ControlList *ctrls,
>> - const struct v4l2_ext_control *v4l2Ctrls,
>> - unsigned int count)
>> + libcamera::Span<const v4l2_ext_control> v4l2Ctrls)
>> {
>> - for (unsigned int i = 0; i < count; i++) {
>> - const struct v4l2_ext_control &v4l2Ctrl = v4l2Ctrls[i];
>> + for (const v4l2_ext_control &v4l2Ctrl : v4l2Ctrls) {
That's nicer indeed.
Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>> const unsigned int id = v4l2Ctrl.id;
>> if (!ctrls->contains(id)) {
>> LOG(V4L2, Error) << "ControlList doesn't contain id: "
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list