<div dir="ltr"><div dir="ltr"><div>On Thu, 27 May 2021 at 17:10, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Umang,<br>
<br>
On Thu, May 27, 2021 at 09:28:02AM +0530, Umang Jain wrote:<br>
> On 5/27/21 1:31 AM, Jacopo Mondi wrote:<br>
> > On Wed, May 26, 2021 at 04:54:32PM +0300, Laurent Pinchart wrote:<br>
> >> On Wed, May 26, 2021 at 06:40:25PM +0530, Umang Jain wrote:<br>
> >>> Pass in frame timestamps from IPU3 pipeline handler to IPU3 IPA via<br>
> >>> IPU3Event. Frame timestamps are helpful to IPA algorithms to<br>
> >>> convergence, by setting them via IPA stats.<br>
> >> This looks good. There's room for improvement, but on top, as it would<br>
> >> be too much yak-shaving:<br>
> >><br>
> >> - Using std::chrono types would be nice for timestamps, but that should<br>
> >> be done throughout libcamera.<br>
> ><br>
> > Just to mention that Nuash's utils::Duration will be a nice fit to<br>
> > replace all the custom timestampings in the library<br>
> ><br>
> > For the patch:<br>
> > - We create frameTimestamp by copying SensorTimestamp. Would it be<br>
> > better to use the same name ?<br>
><br>
> The IPA is expecting a frame timestamp on it's side. It's entirely upto <br>
> the PH to pass in, whatever it thinks closest as the timestamp of the <br>
> frame. In this case, that's SensorTimestamp. I don't support that the <br>
> naming should be changed just because we are passing in Sensor's <br>
> timestamp. I am open to suggestions nevertheless, maybe an explicit <br>
> comment on PH side :<br>
> <br>
> /* As of now, SensorTimestamp is the best approximation for the <br>
> frame-timestamp */<br>
> <br>
> Interestingly raspberry-pi IPA also does similar,<br>
> <br>
> src/ipa/raspberrypi/raspberrypi.cpp:911: int64_t frameTimestamp = <br>
> data.controls.get(controls::SensorTimestamp);<br>
<br>
The RPi IPA uses the timestamp to implement rate-limiting of the<br>
algorithms. I'd expect IPAs to care more about the timestamp delta<br>
between frames than the absolute timestamp, so from that point of view<br>
it doesn't matter much how we approximate the timestamp, as long as we<br>
do so consistently (minimizing jitter would however be important).<br></blockquote><div><br></div><div>Pardon the silly question, but what is the difference between "frame timestamp"</div><div>and "sensor timestamp" in this context? </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> >> - We should move away from IPU3Event, now that the IPA interface is<br>
> >> specific to each pipeline handler. Instead of a single processEvent()<br>
> >> method in IPAIPU3Interface, we should have 3 methods, with ad-hoc<br>
> >> arguments.<br>
> >><br>
> >>> Signed-off-by: Umang Jain <<a href="mailto:umang.jain@ideasonboard.com" target="_blank">umang.jain@ideasonboard.com</a>><br>
> >> Reviewed-by: Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>><br>
> >><br>
> >>> ---<br>
> >>> include/libcamera/ipa/ipu3.mojom | 1 +<br>
> >>> src/ipa/ipu3/ipu3.cpp | 4 +++-<br>
> >>> src/libcamera/pipeline/ipu3/ipu3.cpp | 5 +++--<br>
> >>> 3 files changed, 7 insertions(+), 3 deletions(-)<br>
> >>><br>
> >>> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom<br>
> >>> index 32c046ad..29b4c805 100644<br>
> >>> --- a/include/libcamera/ipa/ipu3.mojom<br>
> >>> +++ b/include/libcamera/ipa/ipu3.mojom<br>
> >>> @@ -21,6 +21,7 @@ enum IPU3Operations {<br>
> >>> struct IPU3Event {<br>
> >>> IPU3Operations op;<br>
> >>> uint32 frame;<br>
> >>> + int64 frameTimestamp;<br>
> >>> uint32 bufferId;<br>
> >>> libcamera.ControlList controls;<br>
> >>> };<br>
> >>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp<br>
> >>> index 769c24d3..581297be 100644<br>
> >>> --- a/src/ipa/ipu3/ipu3.cpp<br>
> >>> +++ b/src/ipa/ipu3/ipu3.cpp<br>
> >>> @@ -53,6 +53,7 @@ private:<br>
> >>> void processControls(unsigned int frame, const ControlList &controls);<br>
> >>> void fillParams(unsigned int frame, ipu3_uapi_params *params);<br>
> >>> void parseStatistics(unsigned int frame,<br>
> >>> + int64_t frameTimestamp,<br>
> >>> const ipu3_uapi_stats_3a *stats);<br>
> >>><br>
> >>> void setControls(unsigned int frame);<br>
> >>> @@ -214,7 +215,7 @@ void IPAIPU3::processEvent(const IPU3Event &event)<br>
> >>> const ipu3_uapi_stats_3a *stats =<br>
> >>> reinterpret_cast<ipu3_uapi_stats_3a *>(mem.data());<br>
> >>><br>
> >>> - parseStatistics(event.frame, stats);<br>
> >>> + parseStatistics(event.frame, event.frameTimestamp, stats);<br>
> >>> break;<br>
> >>> }<br>
> >>> case EventFillParams: {<br>
> >>> @@ -257,6 +258,7 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)<br>
> >>> }<br>
> >>><br>
> >>> void IPAIPU3::parseStatistics(unsigned int frame,<br>
> >>> + [[maybe_unused]] int64_t frameTimestamp,<br>
> >>> [[maybe_unused]] const ipu3_uapi_stats_3a *stats)<br>
> >>> {<br>
> >>> ControlList ctrls(controls::controls);<br>
> >>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp<br>
> >>> index 750880ed..58923bc7 100644<br>
> >>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp<br>
> >>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp<br>
> >>> @@ -1357,6 +1357,8 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)<br>
> >>> if (!info)<br>
> >>> return;<br>
> >>><br>
> >>> + Request *request = info->request;<br>
> >>> +<br>
> >>> if (buffer->metadata().status == FrameMetadata::FrameCancelled) {<br>
> >>> info->metadataProcessed = true;<br>
> >>><br>
> >>> @@ -1364,8 +1366,6 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)<br>
> >>> * tryComplete() will delete info if it completes the IPU3Frame.<br>
> >>> * In that event, we must have obtained the Request before hand.<br>
> >>> */<br>
> >>> - Request *request = info->request;<br>
> >>> -<br>
> >>> if (frameInfos_.tryComplete(info))<br>
> >>> pipe_->completeRequest(request);<br>
> >>><br>
> >>> @@ -1376,6 +1376,7 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)<br>
> >>> ev.op = ipa::ipu3::EventStatReady;<br>
> >>> ev.frame = info->id;<br>
> >>> ev.bufferId = info->statBuffer->cookie();<br>
> >>> + ev.frameTimestamp = request->metadata().get(controls::SensorTimestamp);<br>
> >>> ipa_->processEvent(ev);<br>
> >>> }<br>
> >>><br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>