[libcamera-devel,v3,3/4] android: camera_device: Report sensor physical size
diff mbox series

Message ID 20201228165203.53771-4-jacopo@jmondi.org
State Superseded
Delegated to: Jacopo Mondi
Headers show
Series
  • libcamera: Introduce sensor database
Related show

Commit Message

Jacopo Mondi Dec. 28, 2020, 4:52 p.m. UTC 
Calculate the value of the ANDROID_SENSOR_INFO_PHYSICAL_SIZE property
multiplying the pixel unit cell size with the number of sensor's readable
pixels.

Maintain a default value to support pipelines that do not register
the UnitCellSize property.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/android/camera_device.cpp | 38 ++++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 16 deletions(-)

Comments

Paul Elder Dec. 29, 2020, 4:26 a.m. UTC | #1 
Hi Jacopo,

On Mon, Dec 28, 2020 at 05:52:02PM +0100, Jacopo Mondi wrote:
> Calculate the value of the ANDROID_SENSOR_INFO_PHYSICAL_SIZE property
> multiplying the pixel unit cell size with the number of sensor's readable
> pixels.
> 
> Maintain a default value to support pipelines that do not register
> the UnitCellSize property.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> ---
>  src/android/camera_device.cpp | 38 ++++++++++++++++++++---------------
>  1 file changed, 22 insertions(+), 16 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 7678d4485ce9..9ad417ee6c3a 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -851,24 +851,37 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
>  	staticMetadata_->addEntry(ANDROID_JPEG_MAX_SIZE, &maxJpegBufferSize_, 1);
>  
>  	/* Sensor static metadata. */
> +	std::vector<int32_t> pixelArraySize(2);
>  	if (properties.contains(properties::PixelArraySize)) {
>  		const Size &size =
>  			properties.get(properties::PixelArraySize);
> -		std::vector<int32_t> data{
> -			static_cast<int32_t>(size.width),
> -			static_cast<int32_t>(size.height),
> -		};
> -		staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,
> -					  data.data(), data.size());
> +		pixelArraySize[0] = static_cast<int32_t>(size.width);
> +		pixelArraySize[1] = static_cast<int32_t>(size.height);
>  	} else {
>  		/*
>  		 * \todo Drop the default once the ov5670 and ov13858 drivers
>  		 * are updated to report the pixel array size.
>  		 */
> -		int32_t data[] = { 2592, 1944 };
> -		staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,
> -					  data, 2);
> +		pixelArraySize = { 2592, 1944 };
>  	}
> +	staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,
> +				  pixelArraySize.data(), 2);
> +
> +	std::vector<float> physicalSize(2);
> +	if (properties.contains(properties::UnitCellSize)) {
> +		const Size &unitCellSize =
> +			properties.get<Size>(properties::UnitCellSize);
> +		physicalSize[0] = unitCellSize.width * pixelArraySize[0] / 1e6f;
> +		physicalSize[1] = unitCellSize.height * pixelArraySize[1] / 1e6f;
> +	} else {
> +		/*
> +		 * \todo Drop the default once all camera sensors report
> +		 * the pixel unit size.
> +		 */
> +		physicalSize = { 2.592, 1.944 };
> +	}
> +	staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PHYSICAL_SIZE,
> +				  physicalSize.data(), physicalSize.size());
>  
>  	if (properties.contains(properties::PixelArrayActiveAreas)) {
>  		const Span<const Rectangle> &rects =
> @@ -919,13 +932,6 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
>  				  testPatterModes.data(),
>  				  testPatterModes.size());
>  
> -	std::vector<float> physicalSize = {
> -		2592, 1944,
> -	};
> -	staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PHYSICAL_SIZE,
> -				  physicalSize.data(),
> -				  physicalSize.size());
> -
>  	uint8_t timestampSource = ANDROID_SENSOR_INFO_TIMESTAMP_SOURCE_UNKNOWN;
>  	staticMetadata_->addEntry(ANDROID_SENSOR_INFO_TIMESTAMP_SOURCE,
>  				  &timestampSource, 1);
> -- 
> 2.29.2
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Niklas Söderlund Dec. 29, 2020, 5:31 p.m. UTC | #2 
Hi Jacopo,

Thanks for your work.

On 2020-12-28 17:52:02 +0100, Jacopo Mondi wrote:
> Calculate the value of the ANDROID_SENSOR_INFO_PHYSICAL_SIZE property
> multiplying the pixel unit cell size with the number of sensor's readable
> pixels.
> 
> Maintain a default value to support pipelines that do not register
> the UnitCellSize property.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/android/camera_device.cpp | 38 ++++++++++++++++++++---------------
>  1 file changed, 22 insertions(+), 16 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 7678d4485ce9..9ad417ee6c3a 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -851,24 +851,37 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
>  	staticMetadata_->addEntry(ANDROID_JPEG_MAX_SIZE, &maxJpegBufferSize_, 1);
>  
>  	/* Sensor static metadata. */
> +	std::vector<int32_t> pixelArraySize(2);
>  	if (properties.contains(properties::PixelArraySize)) {
>  		const Size &size =
>  			properties.get(properties::PixelArraySize);
> -		std::vector<int32_t> data{
> -			static_cast<int32_t>(size.width),
> -			static_cast<int32_t>(size.height),
> -		};
> -		staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,
> -					  data.data(), data.size());
> +		pixelArraySize[0] = static_cast<int32_t>(size.width);
> +		pixelArraySize[1] = static_cast<int32_t>(size.height);
>  	} else {
>  		/*
>  		 * \todo Drop the default once the ov5670 and ov13858 drivers
>  		 * are updated to report the pixel array size.
>  		 */
> -		int32_t data[] = { 2592, 1944 };
> -		staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,
> -					  data, 2);
> +		pixelArraySize = { 2592, 1944 };
>  	}
> +	staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,
> +				  pixelArraySize.data(), 2);
> +
> +	std::vector<float> physicalSize(2);
> +	if (properties.contains(properties::UnitCellSize)) {
> +		const Size &unitCellSize =
> +			properties.get<Size>(properties::UnitCellSize);
> +		physicalSize[0] = unitCellSize.width * pixelArraySize[0] / 1e6f;
> +		physicalSize[1] = unitCellSize.height * pixelArraySize[1] / 1e6f;
> +	} else {
> +		/*
> +		 * \todo Drop the default once all camera sensors report
> +		 * the pixel unit size.
> +		 */
> +		physicalSize = { 2.592, 1.944 };
> +	}

Instead of adding special cases for cameras that does not report all 
controls that the HAL needs would it make sens to add default values in 
the CameraSensor/SensorDatabase classes? I'm thinking this could create 
a leaner HAL implementation as the HAL is already rather complex and we 
would collect all defaults values in components that are designed to 
provided them. As an added bonus the controls would also be visible 
outside the HAL and possibly increasing the usage of them in other parts 
of the code?

> +	staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PHYSICAL_SIZE,
> +				  physicalSize.data(), physicalSize.size());
>  
>  	if (properties.contains(properties::PixelArrayActiveAreas)) {
>  		const Span<const Rectangle> &rects =
> @@ -919,13 +932,6 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
>  				  testPatterModes.data(),
>  				  testPatterModes.size());
>  
> -	std::vector<float> physicalSize = {
> -		2592, 1944,
> -	};
> -	staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PHYSICAL_SIZE,
> -				  physicalSize.data(),
> -				  physicalSize.size());
> -
>  	uint8_t timestampSource = ANDROID_SENSOR_INFO_TIMESTAMP_SOURCE_UNKNOWN;
>  	staticMetadata_->addEntry(ANDROID_SENSOR_INFO_TIMESTAMP_SOURCE,
>  				  &timestampSource, 1);
> -- 
> 2.29.2
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Jacopo Mondi Dec. 30, 2020, 8:19 a.m. UTC | #3 
Hi Niklas,

On Tue, Dec 29, 2020 at 06:31:30PM +0100, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your work.
>
> On 2020-12-28 17:52:02 +0100, Jacopo Mondi wrote:
> > Calculate the value of the ANDROID_SENSOR_INFO_PHYSICAL_SIZE property
> > multiplying the pixel unit cell size with the number of sensor's readable
> > pixels.
> >
> > Maintain a default value to support pipelines that do not register
> > the UnitCellSize property.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  src/android/camera_device.cpp | 38 ++++++++++++++++++++---------------
> >  1 file changed, 22 insertions(+), 16 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index 7678d4485ce9..9ad417ee6c3a 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -851,24 +851,37 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
> >  	staticMetadata_->addEntry(ANDROID_JPEG_MAX_SIZE, &maxJpegBufferSize_, 1);
> >
> >  	/* Sensor static metadata. */
> > +	std::vector<int32_t> pixelArraySize(2);
> >  	if (properties.contains(properties::PixelArraySize)) {
> >  		const Size &size =
> >  			properties.get(properties::PixelArraySize);
> > -		std::vector<int32_t> data{
> > -			static_cast<int32_t>(size.width),
> > -			static_cast<int32_t>(size.height),
> > -		};
> > -		staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,
> > -					  data.data(), data.size());
> > +		pixelArraySize[0] = static_cast<int32_t>(size.width);
> > +		pixelArraySize[1] = static_cast<int32_t>(size.height);
> >  	} else {
> >  		/*
> >  		 * \todo Drop the default once the ov5670 and ov13858 drivers
> >  		 * are updated to report the pixel array size.
> >  		 */
> > -		int32_t data[] = { 2592, 1944 };
> > -		staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,
> > -					  data, 2);
> > +		pixelArraySize = { 2592, 1944 };
> >  	}
> > +	staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,
> > +				  pixelArraySize.data(), 2);
> > +
> > +	std::vector<float> physicalSize(2);
> > +	if (properties.contains(properties::UnitCellSize)) {
> > +		const Size &unitCellSize =
> > +			properties.get<Size>(properties::UnitCellSize);
> > +		physicalSize[0] = unitCellSize.width * pixelArraySize[0] / 1e6f;
> > +		physicalSize[1] = unitCellSize.height * pixelArraySize[1] / 1e6f;
> > +	} else {
> > +		/*
> > +		 * \todo Drop the default once all camera sensors report
> > +		 * the pixel unit size.
> > +		 */
> > +		physicalSize = { 2.592, 1.944 };
> > +	}
>
> Instead of adding special cases for cameras that does not report all
> controls that the HAL needs would it make sens to add default values in
> the CameraSensor/SensorDatabase classes? I'm thinking this could create
> a leaner HAL implementation as the HAL is already rather complex and we
> would collect all defaults values in components that are designed to
> provided them. As an added bonus the controls would also be visible
> outside the HAL and possibly increasing the usage of them in other parts
> of the code?

I would rather think the contrary: if the property is not available
libcamera should not expose it. There's no much sense in providing
default information, it might even mis-lead application.

We have defaults here to give time to all the test devices we have to
catch up with new requirements and do not break developments in the
meantime. After all we started with defaults in the HAL, so we're not
making things worst by still reporting them. Down the line, as the
todo entry reports, we should aim at removing defaults from here too.

>
> > +	staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PHYSICAL_SIZE,
> > +				  physicalSize.data(), physicalSize.size());
> >
> >  	if (properties.contains(properties::PixelArrayActiveAreas)) {
> >  		const Span<const Rectangle> &rects =
> > @@ -919,13 +932,6 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
> >  				  testPatterModes.data(),
> >  				  testPatterModes.size());
> >
> > -	std::vector<float> physicalSize = {
> > -		2592, 1944,
> > -	};
> > -	staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PHYSICAL_SIZE,
> > -				  physicalSize.data(),
> > -				  physicalSize.size());
> > -
> >  	uint8_t timestampSource = ANDROID_SENSOR_INFO_TIMESTAMP_SOURCE_UNKNOWN;
> >  	staticMetadata_->addEntry(ANDROID_SENSOR_INFO_TIMESTAMP_SOURCE,
> >  				  &timestampSource, 1);
> > --
> > 2.29.2
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund

Patch
diff mbox series 

diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index 7678d4485ce9..9ad417ee6c3a 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -851,24 +851,37 @@  const camera_metadata_t *CameraDevice::getStaticMetadata()
 	staticMetadata_->addEntry(ANDROID_JPEG_MAX_SIZE, &maxJpegBufferSize_, 1);
 
 	/* Sensor static metadata. */
+	std::vector<int32_t> pixelArraySize(2);
 	if (properties.contains(properties::PixelArraySize)) {
 		const Size &size =
 			properties.get(properties::PixelArraySize);
-		std::vector<int32_t> data{
-			static_cast<int32_t>(size.width),
-			static_cast<int32_t>(size.height),
-		};
-		staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,
-					  data.data(), data.size());
+		pixelArraySize[0] = static_cast<int32_t>(size.width);
+		pixelArraySize[1] = static_cast<int32_t>(size.height);
 	} else {
 		/*
 		 * \todo Drop the default once the ov5670 and ov13858 drivers
 		 * are updated to report the pixel array size.
 		 */
-		int32_t data[] = { 2592, 1944 };
-		staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,
-					  data, 2);
+		pixelArraySize = { 2592, 1944 };
 	}
+	staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,
+				  pixelArraySize.data(), 2);
+
+	std::vector<float> physicalSize(2);
+	if (properties.contains(properties::UnitCellSize)) {
+		const Size &unitCellSize =
+			properties.get<Size>(properties::UnitCellSize);
+		physicalSize[0] = unitCellSize.width * pixelArraySize[0] / 1e6f;
+		physicalSize[1] = unitCellSize.height * pixelArraySize[1] / 1e6f;
+	} else {
+		/*
+		 * \todo Drop the default once all camera sensors report
+		 * the pixel unit size.
+		 */
+		physicalSize = { 2.592, 1.944 };
+	}
+	staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PHYSICAL_SIZE,
+				  physicalSize.data(), physicalSize.size());
 
 	if (properties.contains(properties::PixelArrayActiveAreas)) {
 		const Span<const Rectangle> &rects =
@@ -919,13 +932,6 @@  const camera_metadata_t *CameraDevice::getStaticMetadata()
 				  testPatterModes.data(),
 				  testPatterModes.size());
 
-	std::vector<float> physicalSize = {
-		2592, 1944,
-	};
-	staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PHYSICAL_SIZE,
-				  physicalSize.data(),
-				  physicalSize.size());
-
 	uint8_t timestampSource = ANDROID_SENSOR_INFO_TIMESTAMP_SOURCE_UNKNOWN;
 	staticMetadata_->addEntry(ANDROID_SENSOR_INFO_TIMESTAMP_SOURCE,
 				  &timestampSource, 1);