[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