[libcamera-devel] [PATCH 4/4] android: camera_device: Initialize pixel array properties

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Mon, Nov 09, 2020 at 02:04:08PM +0100, Jacopo Mondi wrote:
> On Mon, Nov 09, 2020 at 01:44:54PM +0100, Niklas Söderlund wrote:
> > On 2020-11-06 16:49:47 +0100, Jacopo Mondi wrote:
> > > Initialize pixel array properties in the Android camera HAL
> > > inspecting the camera properties.
> > >
> > > If the camera does not provide any suitable property, not static
> > > metadata is registered to the Android framework.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > ---
> > >  src/android/camera_device.cpp | 38 +++++++++++++++++++++++++----------
> > >  1 file changed, 27 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > index 4690346e05cb..8a71d15be8ca 100644
> > > --- a/src/android/camera_device.cpp
> > > +++ b/src/android/camera_device.cpp
> > > @@ -593,6 +593,7 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
> > >  	}
> > >
> > >  	const ControlInfoMap &controlsInfo = camera_->controls();
> > > +	const ControlList &properties = camera_->properties();
> > >
> > >  	/* Color correction static metadata. */
> > >  	{
> > > @@ -724,17 +725,32 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
> > >  	staticMetadata_->addEntry(ANDROID_JPEG_MAX_SIZE, &maxJpegBufferSize_, 1);
> > >
> > >  	/* Sensor static metadata. */
> > > -	int32_t pixelArraySize[] = {
> > > -		2592, 1944,
> > > -	};
> > > -	staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,
> > > -				  &pixelArraySize, 2);
> > > -
> > > -	int32_t sensorSizes[] = {
> > > -		0, 0, 2560, 1920,
> > > -	};
> > > -	staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,
> > > -				  &sensorSizes, 4);
> > > +	{
> >
> > Why this extra code block level?
> >
> 
> I wrapped handling of each control in a separate block for two
> reasons (one of which is totally personal taste)
> 1) I re-use the same names for throwaway variables like 'size', 'data' and
>    'infoMap' that are only required for the conversion between libcamera
>    controls and android metadata format. I really don't want to have
>    'dataPixelArraySize' or 'infoMapLensShading' for variables with
>    such limited usage

But the variables are defined within the if () { ... } scope, so there
should be no issue. As there's already a specific scope, I'd tend to
drop the extra level. In the other locations where no specific scope
exist, it makes sense to create one.

Nice to see two hardcoded metadata going away :-)

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

> 2) I like separating each control translation in its own block, but
>    that's merely personal taste.
> 
> > > +		if (properties.contains(properties::PixelArraySize)) {
> > > +			const Size &size =
> > > +				properties.get<Size>(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());
> > > +		}
> > > +	}
> > > +	{
> > > +		if (properties.contains(properties::PixelArrayActiveAreas)) {
> > > +			const Span<const Rectangle> &rects =
> > > +				properties.get<Span<const Rectangle>>(properties::PixelArrayActiveAreas);
> > > +			std::vector<int32_t> data{
> > > +				static_cast<int32_t>(rects[0].x),
> > > +				static_cast<int32_t>(rects[0].y),
> > > +				static_cast<int32_t>(rects[0].width),
> > > +				static_cast<int32_t>(rects[0].height),
> > > +			};
> > > +			staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,
> > > +						  data.data(), data.size());
> > > +		}
> > > +	}
> > >
> > >  	int32_t sensitivityRange[] = {
> > >  		32, 2400,

-- 
Regards,

Laurent Pinchart