[PATCH v1 1/6] libcamera: framebuffer: Add FrameMetadata::Status::FrameStartup

[Naushir Patuck]

Hi Jacopo,


On Tue, 20 May 2025 at 11:17, Jacopo Mondi <jacopo.mondi at ideasonboard.com>
wrote:

> Hi Naush
>
> On Tue, May 20, 2025 at 09:05:33AM +0100, Naushir Patuck wrote:
> > Hi Jacopo,
> >
> > On Tue, 20 May 2025 at 07:52, Jacopo Mondi <
> jacopo.mondi at ideasonboard.com>
> > wrote:
> >
> > > Hi Naush
> > >
> > > 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.


>
> >
> > >
> > >
> > > >   * \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.

Regards,
Naush


> Up to you
>
> >
> > Regards,
> > Naush
> >
> >
> > >
> > > Thanks
> > >   j
> > >
> > > > + * valid.
> > > >   */
> > > >
> > > >  /**
> > > > --
> > > > 2.43.0
> > > >
> > >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20250520/277dd9b6/attachment.htm>