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

[Jacopo Mondi]

Hi Naush

On Tue, May 20, 2025 at 12:57:19PM +0100, Naushir Patuck wrote:
> 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.

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.


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

Thanks
  j

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