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

[Cheng-Hao Yang]

Hi Laurent,

On Fri, Nov 29, 2024 at 3:53 PM Jacopo Mondi
<jacopo.mondi at ideasonboard.com> wrote:
>
> 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.

In mtkisp7's case, it's required for tuning, where they need the real frame
numbers.

As the tuning is already done, we might not need it. Not sure if it's required
for other devices' tuning as well though.


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

Chromebook ciri with pipeline handler mtkisp7.

BR,
Harvey

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