[libcamera-devel] [PATCH 6/6] android: camera_device: Fix preview template

[Laurent Pinchart]

Hi Jacopo,

On Sat, Jul 25, 2020 at 02:36:52PM +0200, Jacopo Mondi wrote:
> On Fri, Jul 24, 2020 at 07:07:15PM +0300, Laurent Pinchart wrote:
> > On Fri, Jul 24, 2020 at 04:21:20PM +0200, Jacopo Mondi wrote:
> > > Add 5 controls to the generate preview template to comply with the
> > > camera3 specification.
> > >
> > > This change fixes CTS 9.0.r12 test [14/253]
> > > android.hardware.camera2.cts.CameraDeviceTest#testCameraDevicePreviewTemplate
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > ---
> > >  src/android/camera_device.cpp | 32 +++++++++++++++++++++++++++++---
> > >  1 file changed, 29 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > index b8cb118a960e..58901a119c18 100644
> > > --- a/src/android/camera_device.cpp
> > > +++ b/src/android/camera_device.cpp
> > > @@ -377,7 +377,7 @@ std::tuple<uint32_t, uint32_t> CameraDevice::calculateStaticMetadataSize()
> > >  	 * Currently: 50 entries, 647 bytes of static metadata
> > >  	 */
> > >  	uint32_t numEntries = 50;
> > > -	uint32_t byteSize = 647;
> > > +	uint32_t byteSize = 667;
> > >
> > >  	/*
> > >  	 * Calculate space occupation in bytes for dynamically built metadata
> > > @@ -779,6 +779,8 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
> > >  		ANDROID_CONTROL_AE_MODE,
> > >  		ANDROID_CONTROL_AE_EXPOSURE_COMPENSATION,
> > >  		ANDROID_CONTROL_AE_PRECAPTURE_TRIGGER,
> > > +		ANDROID_CONTROL_AE_TARGET_FPS_RANGE,
> > > +		ANDROID_CONTROL_AE_ANTIBANDING_MODE,
> > >  		ANDROID_CONTROL_AE_LOCK,
> > >  		ANDROID_CONTROL_AF_TRIGGER,
> > >  		ANDROID_CONTROL_AWB_MODE,
> > > @@ -787,6 +789,9 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
> > >  		ANDROID_STATISTICS_FACE_DETECT_MODE,
> > >  		ANDROID_NOISE_REDUCTION_MODE,
> > >  		ANDROID_COLOR_CORRECTION_ABERRATION_MODE,
> > > +		ANDROID_LENS_APERTURE,
> > > +		ANDROID_LENS_OPTICAL_STABILIZATION_MODE,
> > > +		ANDROID_CONTROL_MODE,
> > >  		ANDROID_CONTROL_CAPTURE_INTENT,
> > >  	};
> > >  	staticMetadata_->addEntry(ANDROID_REQUEST_AVAILABLE_REQUEST_KEYS,
> > > @@ -825,9 +830,9 @@ const CameraMetadata *CameraDevice::captureTemplatePreview()
> > >  {
> > >  	/*
> > >  	 * \todo Keep this in sync with the actual number of entries.
> > > -	 * Currently: 12 entries, 15 bytes
> > > +	 * Currently: 20 entries, 35 bytes
> > >  	 */
> > > -	CameraMetadata *requestTemplate = new CameraMetadata(15, 20);
> > > +	CameraMetadata *requestTemplate = new CameraMetadata(20, 35);
> >
> > It's lovely how (15, 20) didn't match the comment :-) I'll be happy when
> > we'll get a better API.
> 
> It's a real pain, yes
> 
> > >  	if (!requestTemplate->isValid())
> > >  		return nullptr;
> > >
> > > @@ -847,6 +852,17 @@ const CameraMetadata *CameraDevice::captureTemplatePreview()
> > >  	requestTemplate->addEntry(ANDROID_CONTROL_AE_LOCK,
> > >  				  &aeLock, 1);
> > >
> > > +	std::vector<int32_t> aeFpsTarget = {
> > > +		15, 30,
> > > +	};
> > > +	requestTemplate->addEntry(ANDROID_CONTROL_AE_TARGET_FPS_RANGE,
> > > +				  aeFpsTarget.data(),
> > > +				  aeFpsTarget.size());
> > > +
> > > +	uint8_t aeAntibandingMode = ANDROID_CONTROL_AE_ANTIBANDING_MODE_AUTO;
> >
> > Should we report OFF instead, as we don't support antibanding at the
> > moment ?
> 
> CTS complains if I report OFF.
> 
> I'm not sure I got why, as we report OFF in the available anti-banding
> modes reported with ANDROID_CONTROL_AE_AVAILABLE_ANTIBANDING_MODES.

I think it's due to

commit 9ecdbaf9b0f50dcb9bf456310415238b4767e59b
Author: Yin-Chia Yeh <yinchiayeh at google.com>
Date:   Tue Nov 25 11:56:01 2014 -0800

    Camera2: update antibanding spec

    Allow auto mode not always available. When auto is unavailable,
    the default should be either 50HZ or 60HZ and both 50HZ and 60HZ
    modes should be available.

    Bug: 18503791
    Change-Id: I32c99ca64fe53d2cef8caaedd4208357b24221bc

(https://android.googlesource.com/platform/cts)

If auto mode isn't supported, CTS requires manual mode to be supported.
It's actually documented in platform/system/media/camera/docs/docs.html:

"AUTO mode is the default if it is available on given camera device. When
AUTO mode is not available, the default will be either 50HZ or 60HZ, and
both 50HZ and 60HZ will be available."

> I think I'll stick with AUTO for the moment.

If we mandate support of anti-banding in IPAs, it will be easier to
implement manual mode and auto mode. Should we start by exposing manual
mode ?

> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >
> > > +	requestTemplate->addEntry(ANDROID_CONTROL_AE_ANTIBANDING_MODE,
> > > +				  &aeAntibandingMode, 1);
> > > +
> > >  	uint8_t afTrigger = ANDROID_CONTROL_AF_TRIGGER_IDLE;
> > >  	requestTemplate->addEntry(ANDROID_CONTROL_AF_TRIGGER,
> > >  				  &afTrigger, 1);
> > > @@ -875,6 +891,16 @@ const CameraMetadata *CameraDevice::captureTemplatePreview()
> > >  	requestTemplate->addEntry(ANDROID_COLOR_CORRECTION_ABERRATION_MODE,
> > >  				  &aberrationMode, 1);
> > >
> > > +	uint8_t controlMode = ANDROID_CONTROL_MODE_AUTO;
> > > +	requestTemplate->addEntry(ANDROID_CONTROL_MODE, &controlMode, 1);
> > > +
> > > +	float lensAperture = 2.53 / 100;
> > > +	requestTemplate->addEntry(ANDROID_LENS_APERTURE, &lensAperture, 1);
> > > +
> > > +	uint8_t opticalStabilization = ANDROID_LENS_OPTICAL_STABILIZATION_MODE_OFF;
> > > +	requestTemplate->addEntry(ANDROID_LENS_OPTICAL_STABILIZATION_MODE,
> > > +				  &opticalStabilization, 1);
> > > +
> > >  	uint8_t captureIntent = ANDROID_CONTROL_CAPTURE_INTENT_PREVIEW;
> > >  	requestTemplate->addEntry(ANDROID_CONTROL_CAPTURE_INTENT,
> > >  				  &captureIntent, 1);

-- 
Regards,

Laurent Pinchart