[libcamera-devel] android: camera_mode: Resize 'data' vectors
diff mbox series

Message ID 20201201150056.52378-1-jacopo@jmondi.org
State Superseded, archived
Delegated to: Jacopo Mondi
Headers show
Series
  • [libcamera-devel] android: camera_mode: Resize 'data' vectors
Related show

Commit Message

Jacopo Mondi Dec. 1, 2020, 3 p.m. UTC 
The CameraDevice::getStaticMetadata() function populates the
entries for Android's static metadata by walking the ControlInfo
supported values reported by the libcamera pipeline.

For each entry a vector of the size of the maximum number of possible
entries is reserved, populated and then stored in the Android's metadata
pack.

The number of actual entries to be passed to Android is computed using
the vector's size which, for this reason, shall be resized to the actual
number of entries it stores.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
This patch fixes cros_camera_test:
Camera3DeviceTest/Camera3DeviceDefaultSettings.ConstructDefaultSettings/1
---
 src/android/camera_device.cpp | 6 ++++++
 1 file changed, 6 insertions(+)

--
2.29.1

Comments

Laurent Pinchart Dec. 1, 2020, 3:06 p.m. UTC | #1 
Hi Jacopo,

Thank you for the patch.

On Tue, Dec 01, 2020 at 04:00:56PM +0100, Jacopo Mondi wrote:
> The CameraDevice::getStaticMetadata() function populates the
> entries for Android's static metadata by walking the ControlInfo
> supported values reported by the libcamera pipeline.
> 
> For each entry a vector of the size of the maximum number of possible
> entries is reserved, populated and then stored in the Android's metadata
> pack.
> 
> The number of actual entries to be passed to Android is computed using
> the vector's size which, for this reason, shall be resized to the actual
> number of entries it stores.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
> This patch fixes cros_camera_test:
> Camera3DeviceTest/Camera3DeviceDefaultSettings.ConstructDefaultSettings/1
> ---
>  src/android/camera_device.cpp | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index d43db3600b20..d559f0fc4b81 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -602,8 +602,10 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
>  		if (infoMap != controlsInfo.end()) {
>  			for (const auto &value : infoMap->second.values())
>  				data.push_back(value.get<int32_t>());
> +			data.resize(infoMap->second.values().size());
>  		} else {
>  			data.push_back(ANDROID_COLOR_CORRECTION_ABERRATION_MODE_OFF);
> +			data.resize(1);
>  		}

Wouldn't it be better to avoid creating the vector with a too large size
then ? Maybe something along the lines of the following ?

diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index 4690346e05cb..a4d4914c2f80 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -596,7 +596,8 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()

 	/* Color correction static metadata. */
 	{
-		std::vector<uint8_t> data(3);
+		std::vector<uint8_t> data;
+		data.reserve(3);
 		const auto &infoMap = controlsInfo.find(&controls::draft::ColorCorrectionAberrationMode);
 		if (infoMap != controlsInfo.end()) {
 			for (const auto &value : infoMap->second.values())


>  		staticMetadata_->addEntry(ANDROID_COLOR_CORRECTION_AVAILABLE_ABERRATION_MODES,
>  					  data.data(), data.size());
> @@ -803,8 +805,10 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
>  		if (infoMap != controlsInfo.end()) {
>  			for (const auto &value : infoMap->second.values())
>  				data.push_back(value.get<int32_t>());
> +			data.resize(infoMap->second.values().size());
>  		} else {
>  			data.push_back(ANDROID_STATISTICS_LENS_SHADING_MAP_MODE_OFF);
> +			data.resize(1);
>  		}
>  		staticMetadata_->addEntry(ANDROID_STATISTICS_INFO_AVAILABLE_LENS_SHADING_MAP_MODES,
>  					  data.data(), data.size());
> @@ -871,8 +875,10 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
>  		if (infoMap != controlsInfo.end()) {
>  			for (const auto &value : infoMap->second.values())
>  				data.push_back(value.get<int32_t>());
> +			data.resize(infoMap->second.values().size());
>  		} else {
>  			data.push_back(ANDROID_NOISE_REDUCTION_MODE_OFF);
> +			data.resize(1);
>  		}
>  		staticMetadata_->addEntry(ANDROID_NOISE_REDUCTION_AVAILABLE_NOISE_REDUCTION_MODES,
>  					  data.data(), data.size());
Jacopo Mondi Dec. 1, 2020, 3:16 p.m. UTC | #2 
Hi Laurent,

On Tue, Dec 01, 2020 at 05:06:30PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Tue, Dec 01, 2020 at 04:00:56PM +0100, Jacopo Mondi wrote:
> > The CameraDevice::getStaticMetadata() function populates the
> > entries for Android's static metadata by walking the ControlInfo
> > supported values reported by the libcamera pipeline.
> >
> > For each entry a vector of the size of the maximum number of possible
> > entries is reserved, populated and then stored in the Android's metadata
> > pack.
> >
> > The number of actual entries to be passed to Android is computed using
> > the vector's size which, for this reason, shall be resized to the actual
> > number of entries it stores.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> > This patch fixes cros_camera_test:
> > Camera3DeviceTest/Camera3DeviceDefaultSettings.ConstructDefaultSettings/1
> > ---
> >  src/android/camera_device.cpp | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index d43db3600b20..d559f0fc4b81 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -602,8 +602,10 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
> >  		if (infoMap != controlsInfo.end()) {
> >  			for (const auto &value : infoMap->second.values())
> >  				data.push_back(value.get<int32_t>());
> > +			data.resize(infoMap->second.values().size());
> >  		} else {
> >  			data.push_back(ANDROID_COLOR_CORRECTION_ABERRATION_MODE_OFF);
> > +			data.resize(1);
> >  		}
>
> Wouldn't it be better to avoid creating the vector with a too large size
> then ? Maybe something along the lines of the following ?
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 4690346e05cb..a4d4914c2f80 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -596,7 +596,8 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
>
>  	/* Color correction static metadata. */
>  	{
> -		std::vector<uint8_t> data(3);
> +		std::vector<uint8_t> data;
> +		data.reserve(3);

Oh, std::vector::reserve doesn't change the size of the vector.

That's better than resizing (even if the number of possible relocation
is still 0 if I'm not mistaken)

>  		const auto &infoMap = controlsInfo.find(&controls::draft::ColorCorrectionAberrationMode);
>  		if (infoMap != controlsInfo.end()) {
>  			for (const auto &value : infoMap->second.values())
>
>
> >  		staticMetadata_->addEntry(ANDROID_COLOR_CORRECTION_AVAILABLE_ABERRATION_MODES,
> >  					  data.data(), data.size());
> > @@ -803,8 +805,10 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
> >  		if (infoMap != controlsInfo.end()) {
> >  			for (const auto &value : infoMap->second.values())
> >  				data.push_back(value.get<int32_t>());
> > +			data.resize(infoMap->second.values().size());
> >  		} else {
> >  			data.push_back(ANDROID_STATISTICS_LENS_SHADING_MAP_MODE_OFF);
> > +			data.resize(1);
> >  		}
> >  		staticMetadata_->addEntry(ANDROID_STATISTICS_INFO_AVAILABLE_LENS_SHADING_MAP_MODES,
> >  					  data.data(), data.size());
> > @@ -871,8 +875,10 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
> >  		if (infoMap != controlsInfo.end()) {
> >  			for (const auto &value : infoMap->second.values())
> >  				data.push_back(value.get<int32_t>());
> > +			data.resize(infoMap->second.values().size());
> >  		} else {
> >  			data.push_back(ANDROID_NOISE_REDUCTION_MODE_OFF);
> > +			data.resize(1);
> >  		}
> >  		staticMetadata_->addEntry(ANDROID_NOISE_REDUCTION_AVAILABLE_NOISE_REDUCTION_MODES,
> >  					  data.data(), data.size());
>
> --
> Regards,
>
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index d43db3600b20..d559f0fc4b81 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -602,8 +602,10 @@  const camera_metadata_t *CameraDevice::getStaticMetadata()
 		if (infoMap != controlsInfo.end()) {
 			for (const auto &value : infoMap->second.values())
 				data.push_back(value.get<int32_t>());
+			data.resize(infoMap->second.values().size());
 		} else {
 			data.push_back(ANDROID_COLOR_CORRECTION_ABERRATION_MODE_OFF);
+			data.resize(1);
 		}
 		staticMetadata_->addEntry(ANDROID_COLOR_CORRECTION_AVAILABLE_ABERRATION_MODES,
 					  data.data(), data.size());
@@ -803,8 +805,10 @@  const camera_metadata_t *CameraDevice::getStaticMetadata()
 		if (infoMap != controlsInfo.end()) {
 			for (const auto &value : infoMap->second.values())
 				data.push_back(value.get<int32_t>());
+			data.resize(infoMap->second.values().size());
 		} else {
 			data.push_back(ANDROID_STATISTICS_LENS_SHADING_MAP_MODE_OFF);
+			data.resize(1);
 		}
 		staticMetadata_->addEntry(ANDROID_STATISTICS_INFO_AVAILABLE_LENS_SHADING_MAP_MODES,
 					  data.data(), data.size());
@@ -871,8 +875,10 @@  const camera_metadata_t *CameraDevice::getStaticMetadata()
 		if (infoMap != controlsInfo.end()) {
 			for (const auto &value : infoMap->second.values())
 				data.push_back(value.get<int32_t>());
+			data.resize(infoMap->second.values().size());
 		} else {
 			data.push_back(ANDROID_NOISE_REDUCTION_MODE_OFF);
+			data.resize(1);
 		}
 		staticMetadata_->addEntry(ANDROID_NOISE_REDUCTION_AVAILABLE_NOISE_REDUCTION_MODES,
 					  data.data(), data.size());