<div dir="ltr"><div dir="ltr"><div>Hi Jacopo,</div><div><br></div></div><br><div class="gmail_quote gmail_quote_container"><div dir="ltr" class="gmail_attr">On Tue, 20 May 2025 at 11:17, Jacopo Mondi <<a href="mailto:jacopo.mondi@ideasonboard.com">jacopo.mondi@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">Hi Naush<br>
<br>
On Tue, May 20, 2025 at 09:05:33AM +0100, Naushir Patuck wrote:<br>
> Hi Jacopo,<br>
><br>
> On Tue, 20 May 2025 at 07:52, Jacopo Mondi <<a href="mailto:jacopo.mondi@ideasonboard.com" target="_blank">jacopo.mondi@ideasonboard.com</a>><br>
> wrote:<br>
><br>
> > Hi Naush<br>
> ><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<br>
> > partly<br>
> > > - * or fully invalid. The sequence and timestamp fields of the<br>
> > FrameMetadata<br>
> > > - * structure is valid, the other fields may be invalid.<br>
> > > + * or fully invalid. This status may also indicate an invalid frame<br>
> > produced by<br>
> > > + * the sensor during its startup or restart phase. The sequence and<br>
> > timestamp<br>
> > > + * fields of the FrameMetadata structure is valid, the other fields may<br>
> > 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>
><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>
><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>
><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>
><br>
> Maybe I should replace the word "invalid" with "corrupt" in the new<br>
> definition?<br>
><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<br>
"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>
<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>
><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>
><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>
<br>
No no, I'm not against 2 flags, I would simply avoid mentioning sensor<br>
corrupted frames in the FrameError definition.<br></blockquote><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
><br>
> ><br>
> ><br>
> > >   * \var FrameMetadata::FrameCancelled<br>
> > >   * Capture stopped before the frame completed. The frame data is not<br>
> > valid. All<br>
> > >   * fields of the FrameMetadata structure but the status field are<br>
> > invalid.<br>
> > > + * \var FrameMetadata::FrameStartup The frame has been successfully<br>
> > 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<br>
> > in image<br>
> ><br>
> > Do we need to mention IPA ?<br>
> ><br>
><br>
> Maybe s/IPA/imaging algorithms (3A)/ ?<br>
><br>
><br>
> ><br>
> >       ".. however the capture pipeline is in a cold-start or reset<br>
> >        phase"<br>
> ><br>
> ><br>
> > > + * quality parameters producing unusable images. Applications are<br>
> > recommended to<br>
> > > + * not consume these frames. All fields of the FrameMetadata structure<br>
> > are<br>
> ><br>
> > All other fields ? Or are you ok with how it is ?<br>
> ><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></blockquote><div><br></div><div>"All other fields" sounds correct, I'll update this.</div><div><br></div><div>Regards,</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>
Up to you<br>
<br>
><br>
> Regards,<br>
> Naush<br>
><br>
><br>
> ><br>
> > Thanks<br>
> >   j<br>
> ><br>
> > > + * valid.<br>
> > >   */<br>
> > ><br>
> > >  /**<br>
> > > --<br>
> > > 2.43.0<br>
> > ><br>
> ><br>
</blockquote></div></div>