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

[Jacopo Mondi]

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.


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