[libcamera-devel] [PATCH 07/10] android: camera_device: Handle PIPELINE_MAX_DEPTH

[Jacopo Mondi]

Hi Kieran,

On Fri, Oct 09, 2020 at 01:44:19PM +0100, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 09/10/2020 13:20, Jacopo Mondi wrote:
> > Register the ANDROID_REQUEST_PIPELINE_MAX_DEPTH static property
> > inspecting what the value returned by the pipeline handler
> > through the properties::draft::PipelineMaxDepth libcamera property.
> >
> > Report the result metadata ANDROID_REQUEST_PIPELINE_DEPTH that
> > reports the processing stage a frame went through by inspecting the
> > controls::draft::PipelineDepth libcamera control reported in the
> > completed Request.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/android/camera_device.cpp | 28 ++++++++++++++++++++++------
> >  src/android/camera_device.h   |  3 ++-
> >  2 files changed, 24 insertions(+), 7 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index 0a94c1ae17ac..bccec358ea13 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -12,6 +12,7 @@
> >  #include <tuple>
> >  #include <vector>
> >
> > +#include <libcamera/control_ids.h>
> >  #include <libcamera/controls.h>
> >  #include <libcamera/formats.h>
> >  #include <libcamera/property_ids.h>
> > @@ -576,6 +577,8 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
> >  		return nullptr;
> >  	}
> >
> > +	const ControlList &cameraProperties = camera_->properties();
> > +
> >  	/* Color correction static metadata. */
> >  	std::vector<uint8_t> aberrationModes = {
> >  		ANDROID_COLOR_CORRECTION_ABERRATION_MODE_OFF,
> > @@ -867,9 +870,12 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
> >  	staticMetadata_->addEntry(ANDROID_REQUEST_PARTIAL_RESULT_COUNT,
> >  				  &partialResultCount, 1);
> >
> > -	uint8_t maxPipelineDepth = 2;
> > -	staticMetadata_->addEntry(ANDROID_REQUEST_PIPELINE_MAX_DEPTH,
> > -				  &maxPipelineDepth, 1);
> > +	if (cameraProperties.contains(properties::draft::PipelineMaxDepth)) {
> > +		uint8_t maxPipelineDepth =
> > +			cameraProperties.get<int32_t>(properties::draft::PipelineMaxDepth);
> > +		staticMetadata_->addEntry(ANDROID_REQUEST_PIPELINE_MAX_DEPTH,
> > +					  &maxPipelineDepth, 1);
> > +	}
>
> I assume if the pipeline handler doesn't specify a max depth, and one
> doesn't get added here, that's not a problem for android.

That's the right question to ask!

By points:
1) Is this is problem for Android: it depends, Do you want to be
compliant with the Android requirements for the reported Android HW
level ? Yes, that might be a problem, depending on which
property/control the pipeline handler does not report.

2) What policy do we want ? It boils down to the concept of
"android-aware" pipeline handler. The Android HAL cannot, in my
opinion, be fully Android-compliant without support in the pipeline
handler. It cannot produce 'default' values for properties, controls
and metadata without breaking compliance with CTS and other tests.
The pipeline handler needs to report and handle the values required
by Android, I don't see many ways around it :)

>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> >
> >  	/* LIMITED does not support reprocessing. */
> >  	uint32_t maxNumInputStreams = 0;
> > @@ -1480,7 +1486,8 @@ void CameraDevice::requestComplete(Request *request)
> >  	 * pipeline handlers) timestamp in the Request itself.
> >  	 */
> >  	FrameBuffer *buffer = buffers.begin()->second;
> > -	resultMetadata = getResultMetadata(descriptor->frameNumber,
> > +	resultMetadata = getResultMetadata(request->metadata(),
> > +					   descriptor->frameNumber,
> >  					   buffer->metadata().timestamp);
> >
> >  	/* Handle any JPEG compression. */
> > @@ -1600,7 +1607,8 @@ void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream)
> >   * Produce a set of fixed result metadata.
> >   */
> >  std::unique_ptr<CameraMetadata>
> > -CameraDevice::getResultMetadata([[maybe_unused]] int frame_number,
> > +CameraDevice::getResultMetadata(ControlList &metadata,
> > +				[[maybe_unused]] int frame_number,
> >  				int64_t timestamp)
> >  {
> >  	/*
> > @@ -1608,7 +1616,7 @@ CameraDevice::getResultMetadata([[maybe_unused]] int frame_number,
> >  	 * Currently: 18 entries, 62 bytes
> >  	 */
> >  	std::unique_ptr<CameraMetadata> resultMetadata =
> > -		std::make_unique<CameraMetadata>(18, 62);
> > +		std::make_unique<CameraMetadata>(19, 63);
> >  	if (!resultMetadata->isValid()) {
> >  		LOG(HAL, Error) << "Failed to allocate static metadata";
> >  		return nullptr;
> > @@ -1658,6 +1666,14 @@ CameraDevice::getResultMetadata([[maybe_unused]] int frame_number,
> >  	resultMetadata->addEntry(ANDROID_STATISTICS_SCENE_FLICKER,
> >  				 &scene_flicker, 1);
> >
> > +	/* Add metadata tags reported by libcamera. */
> > +	if (metadata.contains(controls::draft::PipelineDepth)) {
> > +		uint8_t pipeline_depth =
> > +			metadata.get<int32_t>(controls::draft::PipelineDepth);
> > +		resultMetadata->addEntry(ANDROID_REQUEST_PIPELINE_DEPTH,
> > +					 &pipeline_depth, 1);
> > +	}
> > +
> >  	/*
> >  	 * Return the result metadata pack even is not valid: get() will return
> >  	 * nullptr.
> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > index 777d1a35e801..8cf1006c0840 100644
> > --- a/src/android/camera_device.h
> > +++ b/src/android/camera_device.h
> > @@ -102,7 +102,8 @@ private:
> >  	void notifyError(uint32_t frameNumber, camera3_stream_t *stream);
> >  	CameraMetadata *requestTemplatePreview();
> >  	libcamera::PixelFormat toPixelFormat(int format);
> > -	std::unique_ptr<CameraMetadata> getResultMetadata(int frame_number,
> > +	std::unique_ptr<CameraMetadata> getResultMetadata(libcamera::ControlList &metadata,
> > +							  int frame_number,
> >  							  int64_t timestamp);
> >
> >  	unsigned int id_;
> >
>
> --
> Regards
> --
> Kieran