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

[Laurent Pinchart]

Hi Jacopo,

On Wed, May 26, 2021 at 10:01:10PM +0200, Jacopo Mondi wrote:
> On Wed, May 26, 2021 at 04:54:32PM +0300, Laurent Pinchart wrote:
> > 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.
> 
> Just to mention that Nuash's utils::Duration will be a nice fit to
> replace all the custom timestampings in the library

Note that this is a timestamp, not a duration. It should thus be
modelled with a std::chrono::time_point<> instance.

> For the patch:
> - We create frameTimestamp by copying SensorTimestamp. Would it be
>   better to use the same name ?

I don't have a strong preference here, but note that we fill the
SensorTimestamp metadata with an approximation of the sensor timestamp,
which is the CIO2 frame timestamp. We could consider that the IPU3 IPA
API would be less misleading with a name that reflects the actual
content of the value.

This being said, I'd like to see at some point improvements to the
timestamp approximation, and I don't think we should then rename the
field in the IPU3Event structure, so maybe we can already call it
sensorTimestamp.

> > - 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