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

[Jacopo Mondi]

Hi Kieran

On Fri, Oct 09, 2020 at 02:17:56PM +0100, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 09/10/2020 14:19, Jacopo Mondi wrote:
> > 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 :)
>
> To me, I see that we would handle this with the
>  "Pipeline Validation Tool" *1

Without digressing for too long, compliance with Android is not
something we should try to verify ourselves imho. Versioning would be
hell, just to name one issue that comes to mind.

There's plenty of tools provided by Android for verify compliance. We
should better focus on libcamera features :)

>
> We can simply add tests there for any android specific requirements.
>

If only they were 'simple' to express, version and update. It's just
too complex imho. Look at the CTS CameraTest size in example

> then a pipeline can be 'passed' as android capabable if it implements
> and passes those tests, and - simply not otherwise.
>
>
> *1 - ah yes the fabled tool that we really should start ...
> - Lets discuss next week, and see if we can get at least a small one
> started!, then it's easier to continue if it exists ;-)
>
> --
> Kieran
>
>
> >
> >>
> >> 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
>
> --
> Regards
> --
> Kieran