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

[Kieran Bingham]

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

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