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

[Naushir Patuck]

On Thu, 27 May 2021 at 17:10, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:

> Hi Umang,
>
> On Thu, May 27, 2021 at 09:28:02AM +0530, Umang Jain wrote:
> > On 5/27/21 1:31 AM, 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
> > >
> > > For the patch:
> > > - We create frameTimestamp by copying SensorTimestamp. Would it be
> > >    better to use the same name ?
> >
> > The IPA is expecting a frame timestamp on it's side. It's entirely upto
> > the PH to pass in, whatever it thinks closest as the timestamp of the
> > frame. In this case, that's SensorTimestamp. I don't support that the
> > naming should be changed just because we are passing in Sensor's
> > timestamp. I am open to suggestions nevertheless, maybe an explicit
> > comment on PH side :
> >
> >      /* As of now, SensorTimestamp is the best approximation for the
> > frame-timestamp */
> >
> > Interestingly raspberry-pi IPA also does similar,
> >
> > src/ipa/raspberrypi/raspberrypi.cpp:911:        int64_t frameTimestamp =
> > data.controls.get(controls::SensorTimestamp);
>
> The RPi IPA uses the timestamp to implement rate-limiting of the
> algorithms. I'd expect IPAs to care more about the timestamp delta
> between frames than the absolute timestamp, so from that point of view
> it doesn't matter much how we approximate the timestamp, as long as we
> do so consistently (minimizing jitter would however be important).
>

Pardon the silly question, but what is the difference between "frame
timestamp"
and "sensor timestamp" in this context?


>
> > >> - 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210528/12301aa8/attachment.htm>