[PATCH] libcamera: reserve frame sequence reported from the hardware

[Cheng-Hao Yang]

Hi Kieran and Jacopo,

On Sun, Oct 20, 2024 at 6:23 AM Kieran Bingham
<kieran.bingham at ideasonboard.com> wrote:
>
> Quoting Jacopo Mondi (2024-10-16 17:10:24)
> > Hi
> >
> > On Wed, Oct 16, 2024 at 01:17:28PM +0000, Harvey Yang wrote:
> > > From: Yudhistira Erlandinata <yerlandinata at chromium.org>
> > >
> > > Originally libcamera resets the sequence to 0 on streamOn. However,
> > > However, there are occasions when the user needs the original
> >
> > However, there's one However too much

Yes, sorry. I'll remove it in the next version.
If we decide to merge this one, please just remove it for me to avoid
the spam :)

> >
> > > hardware sequence to query processing information of the particular
> > > frame. The patch reserves the hwSequence in the FrameMetadata.
> > >
> > > Signed-off-by: Yudhistira Erlandinata <yerlandinata at chromium.org>
> > > Co-developed-by: Harvey Yang <chenghaoyang at chromium.org>
> > > Signed-off-by: Harvey Yang <chenghaoyang at chromium.org>
> >
> > I'm more on the opinion that we should stop zeroing the actual HW
> > sequences.
> >
> > Particularly, as we move towards a model where the camera pipeline
> > should be running even without buffers queued to the video capture
> > devices (iow producing frames from the sensor and stas from the ISP), an
> > application could queue a video buffer later, and we should be able to
> > detect how many frames have been "missed" and the existing
> >
> >         if (!firstFrame_.has_value()) {
> >
> > check in V4L2VideoDevice::dequeueBuffer() will produce unwanted
> > Info messages.
> >
> > Kieran in cc as I recall this is something he introduced.
> >
> > In the meantime, storing the actual sequence number somewhere I think
> > it's fine.
>
> So - yes - I think 'knowing' the real hardware sequence number is a good
> thing probably. The original aim of my patch was to make it consistent
> from libcamera's perspective that frames start at zero... which I think
> is what drivers are /supposed/ to do.
>
> So not starting at zero is/was I think a kernel driver bug .. but alas
> they occur.
>
> And yes - the other side of this was to be able to try to align frame
> sequences for pfc ... but if we are going to handle that well then I
> wouldn't object to removing the zeroing or handling it in a different
> fashion - or simply continuing with a zero indexed base for applications
> and tracking

I guess you're cut-off'ed again...?

>
>
> I'd be curious to see what the device that needs this is doing at
> startup ... Do you always get a start at 1 ? or do you get starting
> offsets at 'random' values or something else ?

I got some `1`s and `5`s. Seems quite consistent though, FYI.

BR,
Harvey

>
> --
> Kieran
>
> >
> >
> > > ---
> > >  include/libcamera/framebuffer.h    | 1 +
> > >  src/libcamera/framebuffer.cpp      | 9 +++++++++
> > >  src/libcamera/v4l2_videodevice.cpp | 1 +
> > >  3 files changed, 11 insertions(+)
> > >
> > > diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h
> > > index ff8392430..fccfaa82a 100644
> > > --- a/include/libcamera/framebuffer.h
> > > +++ b/include/libcamera/framebuffer.h
> > > @@ -34,6 +34,7 @@ struct FrameMetadata {
> > >
> > >       Status status;
> > >       unsigned int sequence;
> > > +     unsigned int hwSequence;
> > >       uint64_t timestamp;
> > >
> > >       Span<Plane> planes() { return planes_; }
> > > diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp
> > > index 826848f75..d9d6294bc 100644
> > > --- a/src/libcamera/framebuffer.cpp
> > > +++ b/src/libcamera/framebuffer.cpp
> > > @@ -86,6 +86,15 @@ LOG_DEFINE_CATEGORY(Buffer)
> > >   * Gaps in the sequence numbers indicate dropped frames.
> > >   */
> > >
> > > +/**
> > > + * \var FrameMetadata::hwSequence
> > > + * \brief The real hardware Frame sequence number
> >
> >    * \brief The hardware frame sequence number as reported by the video device
> > > + *
> > > + * \a FrameMetadata::sequence auto-corrects the initial value to zero on frame
> >
> >     * V4L2VideoDevice::dequeueBuffer() auto-corrects
> >     * FrameMetadata::sequence to zero on stream start. This values
> >     * stores the non-corrected hardware sequence number as reported by
> >     * the video device.
> >     */
> >
> > Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> >
> > Thanks
> >    j
> >
> > > + * start. This value keeps the original hardware sequence to allow users to
> > > + * query processing information of particular frames.
> > > + */
> > > +
> > >  /**
> > >   * \var FrameMetadata::timestamp
> > >   * \brief Time when the frame was captured
> > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > > index 14eba0561..9bc677edf 100644
> > > --- a/src/libcamera/v4l2_videodevice.cpp
> > > +++ b/src/libcamera/v4l2_videodevice.cpp
> > > @@ -1862,6 +1862,7 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()
> > >                       ? FrameMetadata::FrameError
> > >                       : FrameMetadata::FrameSuccess;
> > >       metadata.sequence = buf.sequence;
> > > +     metadata.hwSequence = buf.sequence;
> > >       metadata.timestamp = buf.timestamp.tv_sec * 1000000000ULL
> > >                          + buf.timestamp.tv_usec * 1000ULL;
> > >
> > > --
> > > 2.47.0.rc1.288.g06298d1525-goog
> > >