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

Kieran Bingham kieran.bingham at ideasonboard.com
Sun May 30 01:18:26 CEST 2021


On 28/05/2021 08:55, Naushir Patuck wrote:
> On Thu, 27 May 2021 at 17:10, Laurent Pinchart
> <laurent.pinchart at ideasonboard.com
> <mailto: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?  

I'm curious here too (and I'm scared it's originated with some work I
did several weeks ago), do we have a specific distinction between these
terms? or is it simply that the intel algorithm library referenced a
frame timestamp (statParams->frame_timestamp), and that name has
propagated ?


--
Kieran



>     > >> - 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
>     <mailto:umang.jain at ideasonboard.com>>
>     > >> Reviewed-by: Laurent Pinchart
>     <laurent.pinchart at ideasonboard.com
>     <mailto: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
> 

-- 
Regards
--
Kieran


More information about the libcamera-devel mailing list