[libcamera-devel,v2] android: camera_mode: Reserve 'data' vectors
diff mbox series

Message ID 20201201155930.58584-1-jacopo@jmondi.org
State Accepted
Headers show
Series
  • [libcamera-devel,v2] android: camera_mode: Reserve 'data' vectors
Related show

Commit Message

Jacopo Mondi Dec. 1, 2020, 3:59 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.

The number of entries to be passed to Android is computed using the
vector's size which is initialized at vector creation time to the
maximum number of available entries.

In order to report the correct number of metadata do not create the
vector with the largest possible number of elements but only reserve
space for them using std::vector::reserve() which does not modify the
vector's size.

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

--
2.29.1

Comments

Kieran Bingham Dec. 1, 2020, 4:03 p.m. UTC | #1 
Hi Jacopo,

On 01/12/2020 15:59, 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.
> 
> The number of entries to be passed to Android is computed using the
> vector's size which is initialized at vector creation time to the
> maximum number of available entries.
> 
> In order to report the correct number of metadata do not create the
> vector with the largest possible number of elements but only reserve
> space for them using std::vector::reserve() which does not modify the
> vector's size.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
> This patch fixes cros_camera_test:
> Camera3DeviceTest/Camera3DeviceDefaultSettings.ConstructDefaultSettings/1
> ---

Be careful about removing the extra '---' when applying, but this looks
good now.

I still wish we could automate the overall sizing of the metadata
components, but that's quite different to this patch (but it reminds me
of it...)

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>


>  src/android/camera_device.cpp | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 4690346e05cb..4eb05df0fdc2 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())
> @@ -782,7 +783,8 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
>  				  &maxFaceCount, 1);
> 
>  	{
> -		std::vector<uint8_t> data(2);
> +		std::vector<uint8_t> data;
> +		data.reserve(2);
>  		const auto &infoMap = controlsInfo.find(&controls::draft::LensShadingMapMode);
>  		if (infoMap != controlsInfo.end()) {
>  			for (const auto &value : infoMap->second.values())
> @@ -850,7 +852,8 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
> 
>  	/* Noise reduction modes. */
>  	{
> -		std::vector<uint8_t> data(5);
> +		std::vector<uint8_t> data;
> +		data.reserve(5);
>  		const auto &infoMap = controlsInfo.find(&controls::draft::NoiseReductionMode);
>  		if (infoMap != controlsInfo.end()) {
>  			for (const auto &value : infoMap->second.values())
> --
> 2.29.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
>
Jacopo Mondi Dec. 1, 2020, 4:38 p.m. UTC | #2 
Hi Jacopo,
   where does "camera_mode:" in subject come from ? 0_0

On Tue, Dec 01, 2020 at 04:03:17PM +0000, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 01/12/2020 15:59, 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.
> >
> > The number of entries to be passed to Android is computed using the
> > vector's size which is initialized at vector creation time to the
> > maximum number of available entries.
> >
> > In order to report the correct number of metadata do not create the
> > vector with the largest possible number of elements but only reserve
> > space for them using std::vector::reserve() which does not modify the
> > vector's size.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> > This patch fixes cros_camera_test:
> > Camera3DeviceTest/Camera3DeviceDefaultSettings.ConstructDefaultSettings/1
> > ---
>
> Be careful about removing the extra '---' when applying, but this looks
> good now.
>
> I still wish we could automate the overall sizing of the metadata
> components, but that's quite different to this patch (but it reminds me
> of it...)
>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>

Thanks Kieran

>
> >  src/android/camera_device.cpp | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index 4690346e05cb..4eb05df0fdc2 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())
> > @@ -782,7 +783,8 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
> >  				  &maxFaceCount, 1);
> >
> >  	{
> > -		std::vector<uint8_t> data(2);
> > +		std::vector<uint8_t> data;
> > +		data.reserve(2);
> >  		const auto &infoMap = controlsInfo.find(&controls::draft::LensShadingMapMode);
> >  		if (infoMap != controlsInfo.end()) {
> >  			for (const auto &value : infoMap->second.values())
> > @@ -850,7 +852,8 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
> >
> >  	/* Noise reduction modes. */
> >  	{
> > -		std::vector<uint8_t> data(5);
> > +		std::vector<uint8_t> data;
> > +		data.reserve(5);
> >  		const auto &infoMap = controlsInfo.find(&controls::draft::NoiseReductionMode);
> >  		if (infoMap != controlsInfo.end()) {
> >  			for (const auto &value : infoMap->second.values())
> > --
> > 2.29.1
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
> >
>
> --
> Regards
> --
> Kieran
Laurent Pinchart Dec. 1, 2020, 5:01 p.m. UTC | #3 
Hi Jacopo,

Thank you for the patch.

On Tue, Dec 01, 2020 at 04:59:30PM +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.
> 
> The number of entries to be passed to Android is computed using the
> vector's size which is initialized at vector creation time to the
> maximum number of available entries.
> 
> In order to report the correct number of metadata do not create the
> vector with the largest possible number of elements but only reserve
> space for them using std::vector::reserve() which does not modify the
> vector's size.

Indeed, I had even missed that we were creating vectors with a few
default-initiliazed entries, and added additional entries after that.

> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> This patch fixes cros_camera_test:
> Camera3DeviceTest/Camera3DeviceDefaultSettings.ConstructDefaultSettings/1

You could include this in the commit message.

> ---
>  src/android/camera_device.cpp | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 4690346e05cb..4eb05df0fdc2 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())
> @@ -782,7 +783,8 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
>  				  &maxFaceCount, 1);
> 
>  	{
> -		std::vector<uint8_t> data(2);
> +		std::vector<uint8_t> data;
> +		data.reserve(2);
>  		const auto &infoMap = controlsInfo.find(&controls::draft::LensShadingMapMode);
>  		if (infoMap != controlsInfo.end()) {
>  			for (const auto &value : infoMap->second.values())
> @@ -850,7 +852,8 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
> 
>  	/* Noise reduction modes. */
>  	{
> -		std::vector<uint8_t> data(5);
> +		std::vector<uint8_t> data;
> +		data.reserve(5);
>  		const auto &infoMap = controlsInfo.find(&controls::draft::NoiseReductionMode);
>  		if (infoMap != controlsInfo.end()) {
>  			for (const auto &value : infoMap->second.values())

Patch
diff mbox series 

diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index 4690346e05cb..4eb05df0fdc2 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())
@@ -782,7 +783,8 @@  const camera_metadata_t *CameraDevice::getStaticMetadata()
 				  &maxFaceCount, 1);

 	{
-		std::vector<uint8_t> data(2);
+		std::vector<uint8_t> data;
+		data.reserve(2);
 		const auto &infoMap = controlsInfo.find(&controls::draft::LensShadingMapMode);
 		if (infoMap != controlsInfo.end()) {
 			for (const auto &value : infoMap->second.values())
@@ -850,7 +852,8 @@  const camera_metadata_t *CameraDevice::getStaticMetadata()

 	/* Noise reduction modes. */
 	{
-		std::vector<uint8_t> data(5);
+		std::vector<uint8_t> data;
+		data.reserve(5);
 		const auto &infoMap = controlsInfo.find(&controls::draft::NoiseReductionMode);
 		if (infoMap != controlsInfo.end()) {
 			for (const auto &value : infoMap->second.values())