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

[Kieran Bingham]

Quoting Cheng-Hao Yang (2024-10-21 18:48:46)
> 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...?

Aside from missing a full stop at the end I don't think so?

I was saying I don't mind tracking this or removing the code I added
earlier

> 
> >
> >
> > 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>

Can you do an update with Jacopo's suggestions please?

--
Kieran


> > >
> > > 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
> > > >