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

[Jacopo Mondi]

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.

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

Up to you

>
> Regards,
> Naush
>
>
> >
> > Thanks
> >   j
> >
> > > + * valid.
> > >   */
> > >
> > >  /**
> > > --
> > > 2.43.0
> > >
> >