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

[Jacopo Mondi]

Hi Laurent,

On Tue, Dec 01, 2020 at 09:04:48PM +0200, Laurent Pinchart wrote:
> 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.

Ack

>
> Nice to see two hardcoded metadata going away :-)
>

If a pipeline does not report these information, Android will not
receive it and will probably fail at all validation/testing.

I think this is expected, or should we try to default to something ?

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

Thanks
  j

> > 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