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

[Jacopo Mondi]

Hi Laurent

On Thu, Nov 28, 2024 at 10:01:44PM +0200, Laurent Pinchart wrote:
> On Tue, Oct 22, 2024 at 01:48:46AM +0800, Cheng-Hao Yang wrote:
> > On Sun, Oct 20, 2024 at 6:23 AM Kieran Bingham wrote:
> > > Quoting Jacopo Mondi (2024-10-16 17:10:24)
> > > > 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.
>
> What are the use cases for this ? The sequence number provided by V4L2
> is supposed to be internal only, why does it need to be exposed to
> applications ? Or is this meant for internal use only ? If so it
> shouldn't be exposed in the public API.
>

We currently zero the sequence number
https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/v4l2_videodevice.cpp#n1871

Please note getting somewhere back the 'real' frame number is relevant
for PFC as well.

> > > > >
> > > > > 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.
>
> What device is that ?
>
> > > > > ---
> > > > >  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;
> > > > >
>
> --
> Regards,
>
> Laurent Pinchart