[libcamera-devel] [RFC PATCH v2 11/12] pipeline: ipu3: Set request metadata for FULL compliance

[Jacopo Mondi]

Hi Paul,

On Thu, Apr 22, 2021 at 06:41:01PM +0900, Paul Elder wrote:
> Set the request metadata as required by FULL hardware level.
>
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 67 ++++++++++++++++++++++++++--
>  1 file changed, 64 insertions(+), 3 deletions(-)
>
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 70a5e9ce..de90b9fe 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -75,7 +75,8 @@ class IPU3CameraData : public CameraData
>  {
>  public:
>  	IPU3CameraData(PipelineHandler *pipe)
> -		: CameraData(pipe), exposureTime_(0), supportsFlips_(false)
> +		: CameraData(pipe), exposureTime_(0), supportsFlips_(false),
> +		  lastTimestamp_(0)
>  	{
>  	}
>
> @@ -106,6 +107,8 @@ public:
>  private:
>  	void queueFrameAction(unsigned int id,
>  			      const ipa::ipu3::IPU3Action &action);
> +
> +	int64_t lastTimestamp_;
>  };
>
>  class IPU3CameraConfiguration : public CameraConfiguration
> @@ -1249,12 +1252,65 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)
>
>  	request->metadata().set(controls::draft::PipelineDepth, 3);
>  	/* \todo Move the ExposureTime control to the IPA. */
> -	request->metadata().set(controls::ExposureTime, exposureTime_);
> +	request->metadata().set(controls::ExposureTime,
> +				request->controls().contains(controls::ExposureTime) ?
> +				request->controls().get(controls::ExposureTime) :
> +				exposureTime_);
>  	/* \todo Actually apply the scaler crop region to the ImgU. */
>  	if (request->controls().contains(controls::ScalerCrop))
>  		cropRegion_ = request->controls().get(controls::ScalerCrop);
>  	request->metadata().set(controls::ScalerCrop, cropRegion_);
>
> +	request->metadata().set(controls::draft::BlackLevelLocked,
> +				request->controls().contains(controls::draft::BlackLevelLocked) ?
> +			        request->controls().get(controls::draft::BlackLevelLocked) :
> +				false);

I feel like we're missing a policy here
What is the meaning of:

Frame   Control                 Present? Value
1       BlackLevelLocked        yes      true
2       BlackLevelLocked        no
3       BlackLevelLocked        yes      false

Should the black level lock be held in frame 2) or should be removed ?
Do we want application to -always- specify all the control values, or
do we assume if a control is not there it has not changed value ?
This has impacts on the IPA implementation too, let alone the HAL
(would be interesting how gstreamer behaves, for example)

Thanks
   j

> +
> +	request->metadata().set(controls::AeLocked,
> +				request->controls().contains(controls::AeLocked) ?
> +				request->controls().get(controls::AeLocked) :
> +				false);
> +
> +	request->metadata().set(controls::draft::AePrecaptureTrigger,
> +				request->controls().contains(controls::draft::AePrecaptureTrigger) ?
> +				request->controls().get(controls::draft::AePrecaptureTrigger) :
> +				controls::draft::AePrecaptureTriggerIdle);
> +
> +	request->metadata().set(controls::AwbMode,
> +				request->controls().contains(controls::AwbMode) ?
> +				request->controls().get(controls::AwbMode) :
> +				controls::AwbAuto);
> +
> +	request->metadata().set(controls::AwbLocked,
> +				request->controls().contains(controls::AwbLocked) ?
> +				request->controls().get(controls::AwbLocked) :
> +				false);
> +
> +	request->metadata().set(controls::draft::EdgeMode,
> +				request->controls().contains(controls::draft::EdgeMode) ?
> +				request->controls().get(controls::draft::EdgeMode) :
> +				(uint8_t)controls::draft::EdgeModeOff);
> +
> +	request->metadata().set(controls::draft::NoiseReductionMode,
> +				request->controls().contains(controls::draft::NoiseReductionMode) ?
> +				request->controls().get(controls::draft::NoiseReductionMode) :
> +				controls::draft::NoiseReductionModeOff);
> +
> +	request->metadata().set(controls::draft::SensorSensitivity,
> +				request->controls().contains(controls::draft::SensorSensitivity) ?
> +				request->controls().get(controls::draft::SensorSensitivity) :
> +				32);
> +
> +	if (request->metadata().get(controls::draft::FrameDuration) <
> +	    request->metadata().get(controls::ExposureTime) * 1000)
> +			request->metadata().set(controls::draft::FrameDuration,
> +						request->metadata().get(controls::ExposureTime) * 1000);
> +
> +	request->metadata().set(controls::draft::TonemapMode,
> +				request->controls().contains(controls::draft::TonemapMode) ?
> +				request->controls().get(controls::draft::TonemapMode) :
> +				(uint8_t)controls::draft::TonemapModeFast);
> +
>  	if (frameInfos_.tryComplete(info))
>  		pipe_->completeRequest(request);
>  }
> @@ -1280,8 +1336,13 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)
>  	 * \todo The sensor timestamp should be better estimated by connecting
>  	 * to the V4L2Device::frameStart signal.
>  	 */
> +	int64_t timestamp = buffer->metadata().timestamp;
>  	request->metadata().set(controls::SensorTimestamp,
> -				buffer->metadata().timestamp);
> +				timestamp);
> +
> +	request->metadata().set(controls::draft::FrameDuration,
> +				timestamp - lastTimestamp_);
> +	lastTimestamp_ = timestamp;
>
>  	/* If the buffer is cancelled force a complete of the whole request. */
>  	if (buffer->metadata().status == FrameMetadata::FrameCancelled) {
> --
> 2.27.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel