| Message ID | 20210526131025.675024-3-umang.jain@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, thank you for the patch. On Wed, May 26, 2021 at 10:10 PM Umang Jain <umang.jain@ideasonboard.com> 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. > > Signed-off-by: Umang Jain <umang.jain@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) > Will the passed frameTimestamp be used in a follow up patch? Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > { > 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); > } > > -- > 2.26.2 > >
Hi Umang, Thank you for the patch. 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. - 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@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@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); > } >
Hi Hiro, On Wed, May 26, 2021 at 10:53:29PM +0900, Hirokazu Honda wrote: > On Wed, May 26, 2021 at 10:10 PM 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. > > > > Signed-off-by: Umang Jain <umang.jain@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) > > > > Will the passed frameTimestamp be used in a follow up patch? The IPU3 IPA based on the Intel closed-source library (https://git.libcamera.org/libcamera/ipu3-ipa.git/) will use this. I'm sure the in-tree open-source IPU3 IPA will follow at some point. > Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > > > { > > 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); > > } > >
Hi Laurent, On Wed, May 26, 2021 at 11:04 PM Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Hiro, > > On Wed, May 26, 2021 at 10:53:29PM +0900, Hirokazu Honda wrote: > > On Wed, May 26, 2021 at 10:10 PM 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. > > > > > > Signed-off-by: Umang Jain <umang.jain@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) > > > > > > > Will the passed frameTimestamp be used in a follow up patch? > > The IPU3 IPA based on the Intel closed-source library > (https://git.libcamera.org/libcamera/ipu3-ipa.git/) will use this. I'm > sure the in-tree open-source IPU3 IPA will follow at some point. > > Acked. > > Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > > > > > { > > > 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 >
Hi On Wed, May 26, 2021 at 04:54:32PM +0300, Laurent Pinchart wrote: > Hi Umang, > > Thank you for the patch. > > 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 ? > > - 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@ideasonboard.com> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@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
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@ideasonboard.com> > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@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); > > > } > > >
Hi Jacopo On 5/27/21 1:31 AM, Jacopo Mondi wrote: > Hi > > On Wed, May 26, 2021 at 04:54:32PM +0300, Laurent Pinchart wrote: >> Hi Umang, >> >> Thank you for the patch. >> >> 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); >> - 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@ideasonboard.com> >> Reviewed-by: Laurent Pinchart <laurent.pinchart@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
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). > >> - 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@ideasonboard.com> > >> Reviewed-by: Laurent Pinchart <laurent.pinchart@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); > >>> } > >>>
On Thu, 27 May 2021 at 17:10, Laurent Pinchart < laurent.pinchart@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@ideasonboard.com> > > >> Reviewed-by: Laurent Pinchart <laurent.pinchart@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 >
On 28/05/2021 08:55, Naushir Patuck wrote: > On Thu, 27 May 2021 at 17:10, Laurent Pinchart > <laurent.pinchart@ideasonboard.com > <mailto:laurent.pinchart@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@ideasonboard.com > <mailto:umang.jain@ideasonboard.com>> > > >> Reviewed-by: Laurent Pinchart > <laurent.pinchart@ideasonboard.com > <mailto:laurent.pinchart@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 >
Hello, On Sun, May 30, 2021 at 12:18:26AM +0100, Kieran Bingham wrote: > On 28/05/2021 08:55, Naushir Patuck wrote: > > On Thu, 27 May 2021 at 17:10, Laurent Pinchart wrote: > >> 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 ? There's no strick definition, "sensor timestamp" is often used to refer to the start of exposure timestamp, while "frame timestamp" is often used to refer to the capture DMA timestamp (but is also used to refer to the start of exposure timestamp). The latter isn't very useful as such, but is often used as an approximation of the former. The distinction between the two terms doesn't matter much here, I don't think we need to care. > >>>>> - 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@ideasonboard.com> > >>>>> > >>>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@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); > >>>>>> } > >>>>>>
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); }
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. Signed-off-by: Umang Jain <umang.jain@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(-)