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