[PATCH v1 1/6] libcamera: framebuffer: Add FrameMetadata::Status::FrameStartup
Naushir Patuck
naush at raspberrypi.com
Wed May 21 13:52:11 CEST 2025
Hi Laurent,
On Wed, 21 May 2025 at 12:38, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:
> On Tue, May 20, 2025 at 02:41:20PM +0200, Jacopo Mondi wrote:
> > On Tue, May 20, 2025 at 12:57:19PM +0100, Naushir Patuck wrote:
> > > On Tue, 20 May 2025 at 11:17, Jacopo Mondi wrote:
> > > > On Tue, May 20, 2025 at 09:05:33AM +0100, Naushir Patuck wrote:
> > > > > On Tue, 20 May 2025 at 07:52, Jacopo Mondi wrote:
> > > > > > On Mon, May 19, 2025 at 10:20:49AM +0100, Naushir Patuck wrote:
> > > > > > > Add a new status enum, FrameStartup, used to denote that even
> though
> > > > > > > the frame has been successfully captured, the IQ parameters
> set by the
> > > > > > > IPA will cause the frame to be unusable and applications are
> advised to
> > > > > > > not consume this frame. An example of this would be on a
> cold-start of
> > > > > > > the 3A algorithms, and there will be large oscillations to
> converge to
> > > > > > > a stable state quickly.
> > > > > > >
> > > > > > > Additional, update the definition of the FrameError state to
> include its
> > > > > > > usage when the sensor is known to produce a number of
> invalid/error
> > > > > > > frames after stream-on.
> > > > > > >
> > > > > > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > > > > > ---
> > > > > > > include/libcamera/framebuffer.h | 1 +
> > > > > > > src/libcamera/framebuffer.cpp | 11 +++++++++--
> > > > > > > 2 files changed, 10 insertions(+), 2 deletions(-)
> > > > > > >
> > > > > > > diff --git a/include/libcamera/framebuffer.h
> > > > > > b/include/libcamera/framebuffer.h
> > > > > > > index ff83924300ac..e83825b466aa 100644
> > > > > > > --- a/include/libcamera/framebuffer.h
> > > > > > > +++ b/include/libcamera/framebuffer.h
> > > > > > > @@ -26,6 +26,7 @@ struct FrameMetadata {
> > > > > > > FrameSuccess,
> > > > > > > FrameError,
> > > > > > > FrameCancelled,
> > > > > > > + FrameStartup,
> > > > > > > };
> > > > > > >
> > > > > > > struct Plane {
> > > > > > > diff --git a/src/libcamera/framebuffer.cpp
> > > > > > b/src/libcamera/framebuffer.cpp
> > > > > > > index 826848f75a56..36e56d593bc3 100644
> > > > > > > --- a/src/libcamera/framebuffer.cpp
> > > > > > > +++ b/src/libcamera/framebuffer.cpp
> > > > > > > @@ -44,11 +44,18 @@ LOG_DEFINE_CATEGORY(Buffer)
> > > > > > > * of the FrameMetadata structure are valid.
> > > > > > > * \var FrameMetadata::FrameError
> > > > > > > * An error occurred during capture of the frame. The frame
> data may be partly
> > > > > > > - * or fully invalid. The sequence and timestamp fields of the
> FrameMetadata
> > > > > > > - * structure is valid, the other fields may be invalid.
> > > > > > > + * or fully invalid. This status may also indicate an invalid
> frame produced by
> > > > > > > + * the sensor during its startup or restart phase. The
> sequence and timestamp
> > > > > > > + * fields of the FrameMetadata structure is valid, the other
> fields may be
> > > > > > > + * invalid.
> > > > > >
> > > > > > This suggests to me that either you expect pipelines to set
> > > > > >
> > > > > > FrameError | FrameStartup
> > > > > >
> > > > > > to indicate a 'sensor startup' frame, which I don't think it's
> the
> > > > > > intention here.
> > > > > >
> > > > > > I wonder if we actually add anything here.
> > > > >
> > > > > The intention is for the enum flags to be exclusive, i.e. either
> FrameError
> > > > > or FrameStartup.
> > > >
> > > > Ack, this was my understanding as well
> > > >
> > > > > Here, I've overloaded the FrameError flag to capture the existing
> usage
> > > > > like random transmission errors plus the bad frames produced by
> the sensor
> > > > > device on startup.
> > > > >
> > > > > > Or maybe just change the definition to:
> > > > > >
> > > > > > * The frame data may be partly or fully invalid. The sequence
> and
> > > > > > * timestamp fields of the FrameMetadata structure is valid, the
> other
> > > > > > * fields may be invalid.
> > > > > >
> > > > > > to remove the "An error occurred" part, as "sensor startup"
> frames are
> > > > > > not errors, but contain known-to-be-invalid data.
> > > > >
> > > > > Maybe I should replace the word "invalid" with "corrupt" in the new
> > > > > definition?
> > > >
> > > > Idk, "invalid" seems more generic than "corrupt", and since we use
> FrameError
> > > > to report frames whose post-processing failed, I would still use
> "Invalid"
> > > >
> > > > > "
> > > > > This status may also indicate corrupt frames produced by
> > > > > the sensor during its startup or restart phase.
> > > > > "
> > > >
> > > > I would rather not mention specific cases here, otherwise we should
> do
> > > > the same for all sources of invalid/corrupted frames.
> > > >
> > > > > > What matters for applications, is, after all, knowing if frames
> can be
> > > > > > consumed or not, so I think we can drop mentioning "errors" or
> more
> > > > > > precise failure conditions.
> > > > >
> > > > > Having 2 flags does allow the application to know what's going on
> under the
> > > > > hood, i.e. bad frames from the device, or bad IQ due to software.
> But I
> > > > > could be convinced that these could be folded into one error flag
> if that's
> > > > > a better way.
> > > >
> > > > No no, I'm not against 2 flags, I would simply avoid mentioning
> sensor
> > > > corrupted frames in the FrameError definition.
> > >
> > > Ah, I think I understand - so you suggest that we make no change to the
> > > definition of FrameError, and just include the "invalid" frames to the
> > > category? I'm ok with this.
> >
> > My suggestion is to drop the first sentence that mentions "errors
> > during the capture"
> >
> > From
> >
> > * \var FrameMetadata::FrameError
> > * An error occurred during capture of the frame. The frame data may be
> partly
> > * or fully invalid. The sequence and timestamp fields of the
> FrameMetadata
> > * structure is valid, the other fields may be invalid.
> >
> > to
> >
> > * \var FrameMetadata::FrameError
> > * The frame data may be partly or fully invalid. The sequence and
> > * timestamp fields of the FrameMetadata structure is valid, the other
> > * fields may be invalid.
>
> I find example useful to better understand the API. How about the
> following ?
>
> * The frame data is partly or fully corrupted, missing or otherwise
> invalid.
> * This can for instance indicate a hardware transmission error, or
> invalid data
> * produced by the sensor during its startup phase. The sequence and
> timestamp
> * fields of the FrameMetadata structure is valid, all the other fields
> may be
> * invalid.
>
That works for me, I'll update the text as above.
Naush
>
> > > > > > > * \var FrameMetadata::FrameCancelled
> > > > > > > * Capture stopped before the frame completed. The frame data
> is not valid. All
> > > > > > > * fields of the FrameMetadata structure but the status field
> are invalid.
> > > > > > > + * \var FrameMetadata::FrameStartup The frame has been
> successfully captured.
> > > > > >
> > > > > > Following the above I would say that
> > > > > >
> > > > > > "The frame data is valid, however the...
> > > > > >
> > > > > > > + * However, the IPA is in a cold-start or reset phase and
> will result in image
> > > > > >
> > > > > > Do we need to mention IPA ?
> > > > >
> > > > > Maybe s/IPA/imaging algorithms (3A)/ ?
> > > > >
> > > > > > ".. however the capture pipeline is in a cold-start or
> reset
> > > > > > phase"
> > > > > >
> > > > > > > + * quality parameters producing unusable images. Applications
> are recommended to
> > > > > > > + * not consume these frames. All fields of the FrameMetadata
> structure are
> > > > > >
> > > > > > All other fields ? Or are you ok with how it is ?
> > > > >
> > > > > Yes, I think all other metadata fields ought to report valid data
> in these
> > > > > cases.
> > > >
> > > > Yeah, my question was only about:
> > > >
> > > > "All other fields" vs
> > > > "All fields"
> > >
> > > "All other fields" sounds correct, I'll update this.
> > >
> > > > > > > + * valid.
> > > > > > > */
> > > > > > >
> > > > > > > /**
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20250521/ee36f407/attachment.htm>
More information about the libcamera-devel
mailing list