[libcamera-devel] [PATCH v2 2/2] ipa: ipu3: Provide frame timestamps through IPU3Event

[Laurent Pinchart]

Hi Umang,

Thank you for the patch.

On Wed, May 26, 2021 at 06:40:25PM +0530, Umang Jain wrote:
> Pass in frame timestamps from IPU3 pipeline handler to IPU3 IPA via
> IPU3Event. Frame timestamps are helpful to IPA algorithms to
> convergence, by setting them via IPA stats.

This looks good. There's room for improvement, but on top, as it would
be too much yak-shaving:

- Using std::chrono types would be nice for timestamps, but that should
  be done throughout libcamera.

- We should move away from IPU3Event, now that the IPA interface is
  specific to each pipeline handler. Instead of a single processEvent()
  method in IPAIPU3Interface, we should have 3 methods, with ad-hoc
  arguments.

> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> ---
>  include/libcamera/ipa/ipu3.mojom     | 1 +
>  src/ipa/ipu3/ipu3.cpp                | 4 +++-
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 5 +++--
>  3 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom
> index 32c046ad..29b4c805 100644
> --- a/include/libcamera/ipa/ipu3.mojom
> +++ b/include/libcamera/ipa/ipu3.mojom
> @@ -21,6 +21,7 @@ enum IPU3Operations {
>  struct IPU3Event {
>  	IPU3Operations op;
>  	uint32 frame;
> +	int64 frameTimestamp;
>  	uint32 bufferId;
>  	libcamera.ControlList controls;
>  };
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 769c24d3..581297be 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -53,6 +53,7 @@ private:
>  	void processControls(unsigned int frame, const ControlList &controls);
>  	void fillParams(unsigned int frame, ipu3_uapi_params *params);
>  	void parseStatistics(unsigned int frame,
> +			     int64_t frameTimestamp,
>  			     const ipu3_uapi_stats_3a *stats);
>  
>  	void setControls(unsigned int frame);
> @@ -214,7 +215,7 @@ void IPAIPU3::processEvent(const IPU3Event &event)
>  		const ipu3_uapi_stats_3a *stats =
>  			reinterpret_cast<ipu3_uapi_stats_3a *>(mem.data());
>  
> -		parseStatistics(event.frame, stats);
> +		parseStatistics(event.frame, event.frameTimestamp, stats);
>  		break;
>  	}
>  	case EventFillParams: {
> @@ -257,6 +258,7 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)
>  }
>  
>  void IPAIPU3::parseStatistics(unsigned int frame,
> +			      [[maybe_unused]] int64_t frameTimestamp,
>  			      [[maybe_unused]] const ipu3_uapi_stats_3a *stats)
>  {
>  	ControlList ctrls(controls::controls);
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 750880ed..58923bc7 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -1357,6 +1357,8 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
>  	if (!info)
>  		return;
>  
> +	Request *request = info->request;
> +
>  	if (buffer->metadata().status == FrameMetadata::FrameCancelled) {
>  		info->metadataProcessed = true;
>  
> @@ -1364,8 +1366,6 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
>  		 * tryComplete() will delete info if it completes the IPU3Frame.
>  		 * In that event, we must have obtained the Request before hand.
>  		 */
> -		Request *request = info->request;
> -
>  		if (frameInfos_.tryComplete(info))
>  			pipe_->completeRequest(request);
>  
> @@ -1376,6 +1376,7 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
>  	ev.op = ipa::ipu3::EventStatReady;
>  	ev.frame = info->id;
>  	ev.bufferId = info->statBuffer->cookie();
> +	ev.frameTimestamp = request->metadata().get(controls::SensorTimestamp);
>  	ipa_->processEvent(ev);
>  }
>  

-- 
Regards,

Laurent Pinchart