| Message ID | 20241016131730.3634805-1-chenghaoyang@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi On Wed, Oct 16, 2024 at 01:17:28PM +0000, Harvey Yang wrote: > From: Yudhistira Erlandinata <yerlandinata@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@chromium.org> > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org> > Signed-off-by: Harvey Yang <chenghaoyang@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@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 >
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@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@chromium.org> > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org> > > Signed-off-by: Harvey Yang <chenghaoyang@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@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 > >
Hi Kieran and Jacopo, On Sun, Oct 20, 2024 at 6:23 AM Kieran Bingham <kieran.bingham@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@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@chromium.org> > > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org> > > > Signed-off-by: Harvey Yang <chenghaoyang@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. 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@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 > > >
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@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@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@chromium.org> > > > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org> > > > > Signed-off-by: Harvey Yang <chenghaoyang@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@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 > > > >
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@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. > > > > > > > > Signed-off-by: Yudhistira Erlandinata <yerlandinata@chromium.org> > > > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org> > > > > Signed-off-by: Harvey Yang <chenghaoyang@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@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; > > > >
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@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@chromium.org> > > > > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org> > > > > > Signed-off-by: Harvey Yang <chenghaoyang@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@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
Hi Laurent, On Fri, Nov 29, 2024 at 3:53 PM Jacopo Mondi <jacopo.mondi@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@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@chromium.org> > > > > > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org> > > > > > > Signed-off-by: Harvey Yang <chenghaoyang@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@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
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 + * + * \a FrameMetadata::sequence auto-corrects the initial value to zero on frame + * 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;