[libcamera-devel] [PATCH v3 3/4] android: camera_device: Report sensor physical size

[Jacopo Mondi]

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 at 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 at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund