[libcamera-devel] [PATCH 09/10] android: camera_device: Use Camera properties for static Metadata

[Laurent Pinchart]

Hi Jacopo,

On Thu, Dec 05, 2019 at 10:45:50AM +0100, Jacopo Mondi wrote:
> On Wed, Dec 04, 2019 at 06:32:35PM +0200, Laurent Pinchart wrote:
> > On Wed, Dec 04, 2019 at 02:21:05PM +0100, Jacopo Mondi wrote:
> > > Construct two example static metadata to be reported to the Android
> > > framework using the properties reported by the Camera.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > ---
> > >  src/android/camera_device.cpp | 29 +++++++++++++++++++++++++++--
> > >  1 file changed, 27 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > index 065e0292e186..674867d313ac 100644
> > > --- a/src/android/camera_device.cpp
> > > +++ b/src/android/camera_device.cpp
> > > @@ -7,6 +7,9 @@
> > >
> > >  #include "camera_device.h"
> > >
> > > +#include <libcamera/controls.h>
> > > +#include <libcamera/property_ids.h>
> > > +
> > >  #include "log.h"
> > >  #include "utils.h"
> > >
> > > @@ -97,6 +100,8 @@ camera_metadata_t *CameraDevice::getStaticMetadata()
> > >  	if (staticMetadata_)
> > >  		return staticMetadata_->get();
> > >
> > > +	const ControlList &properties = camera_->properties();
> > > +
> > >  	/*
> > >  	 * The here reported metadata are enough to implement a basic capture
> > >  	 * example application, but a real camera implementation will require
> > > @@ -261,9 +266,15 @@ camera_metadata_t *CameraDevice::getStaticMetadata()
> > >  	staticMetadata_->addEntry(ANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE,
> > >  				  &exposureTimeRange, 2);
> > >
> > > +	/*
> > > +	 * Android reports orientation as clockwise correction, while Linux
> > > +	 * uses counter-clockwise.
> >
> > What Linux does isn't relevant I think, what matters here is what
> > libcamera does.
> 
> Libcamera does what Linux does, might that be because the same person
> did both ends ? Do we want to introduce a different handling of
> rotation in libcamera ?

I'm not saying we should, I'm just saying that the HAL implementation,
which translates from libcamera to Android, shouldn't mention Linux as
such, especially given that Linux means V4L2 in this context and we may
in the future support other APIs too.

The fact that the libcamera rotation control uses the same semantics as
V4L2 isn't good or bad in this context, but is relevant to the lower
layer, not this layer.

> > Android defines the rotation as
> >
> > "Clockwise angle through which the output image needs to be rotated to
> > be upright on the device screen in its native orientation."
> >
> > I have to say I have a hard time parsing the description of the
> > libcamera rotation property:
> >
> > "Camera mounting rotation expressed as counterclockwise rotation degrees
> > towards the axis perpendicular to the sensor surface and directed away
> > from it."
> 
> That's the mounting rotation description, almost copied from the
> dt-bindings property description. Might it be better to make the
> libcamera rotation about the correction and not the mounting rotation
> ? In that case the below [(360 - rotation) % 360] would be performed
> by the CameraSensor class and not here..

Not necessarily, we don't need to align the libcamera and Android
controls perfectly, but at the very least I need to understand what the
libcamera definition of the rotation control means :-)

> > I can't figure out what this means :-S Is it just me ?
> 
> Didn't we have this currently in review in linux-media as a
> dt-property ? :)

I'll review the series and bring this up there.

> > > +	 */
> > >  	int32_t orientation = 0;
> > > -	staticMetadata_->addEntry(ANDROID_SENSOR_ORIENTATION,
> > > -				  &orientation, 1);
> > > +	if (properties.contains(properties::Rotation))
> > > +		orientation = (360 - properties.get(properties::Rotation))
> >
> > This could be made slightly more efficient if you used .find() to avoid
> > the double lookup. Same below.
> 
> Indeed, I'll fix
> 
> > > +			    % 360;
> > > +	staticMetadata_->addEntry(ANDROID_SENSOR_ORIENTATION, &orientation, 1);
> > >
> > >  	std::vector<int32_t> testPatterModes = {
> > >  		ANDROID_SENSOR_TEST_PATTERN_MODE_OFF,
> > > @@ -310,6 +321,20 @@ camera_metadata_t *CameraDevice::getStaticMetadata()
> > >  				  lensApertures.size());
> > >
> > >  	uint8_t lensFacing = ANDROID_LENS_FACING_FRONT;
> > > +	if (properties.contains(properties::Location)) {
> > > +		int32_t location = properties.get(properties::Location);
> > > +		switch (location) {
> > > +		case CAMERA_LOCATION_FRONT:
> > > +			lensFacing = ANDROID_LENS_FACING_FRONT;
> > > +			break;
> > > +		case CAMERA_LOCATION_BACK:
> > > +			lensFacing = ANDROID_LENS_FACING_BACK;
> > > +			break;
> > > +		case CAMERA_LOCATION_EXTERNAL:
> > > +			lensFacing = ANDROID_LENS_FACING_EXTERNAL;
> > > +			break;
> > > +		}
> > > +	}
> > >  	staticMetadata_->addEntry(ANDROID_LENS_FACING, &lensFacing, 1);
> > >
> > >  	std::vector<float> lensFocalLenghts = {

-- 
Regards,

Laurent Pinchart