[libcamera-devel,v3,3/3] libcamera: V4L2Device: Use Span in updateControls()
diff mbox series

Message ID 20210421093857.391409-3-hiroh@chromium.org
State Accepted
Headers show
Series
  • [libcamera-devel,v3,1/3] libcamera: V4L2Device: Use std::vector for v4l2_ext_control in getControls()
Related show

Commit Message

Hirokazu Honda April 21, 2021, 9:38 a.m. UTC 
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@chromium.org>
---
 include/libcamera/internal/v4l2_device.h |  4 ++--
 src/libcamera/v4l2_device.cpp            | 13 ++++++-------
 2 files changed, 8 insertions(+), 9 deletions(-)

Comments

Laurent Pinchart April 21, 2021, 10:16 a.m. UTC | #1 
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@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);
>  
>  	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@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) {
>  		const unsigned int id = v4l2Ctrl.id;
>  		if (!ctrls->contains(id)) {
>  			LOG(V4L2, Error) << "ControlList doesn't contain id: "
Kieran Bingham April 21, 2021, 10:21 a.m. UTC | #2 
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@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@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@ideasonboard.com>

>>  		const unsigned int id = v4l2Ctrl.id;
>>  		if (!ctrls->contains(id)) {
>>  			LOG(V4L2, Error) << "ControlList doesn't contain id: "
>

Patch
diff mbox series 

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);
 
 	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));
 
 	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));
 
 	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) {
 		const unsigned int id = v4l2Ctrl.id;
 		if (!ctrls->contains(id)) {
 			LOG(V4L2, Error) << "ControlList doesn't contain id: "