| Message ID | 20250519092245.269048-2-naush@raspberrypi.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush Thanks for the patch. On Mon, 19 May 2025 at 10:22, Naushir Patuck <naush@raspberrypi.com> 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@raspberrypi.com> This looks fine to me! Reviewed-by: David Plowman <david.plowman@raspberrypi.com> Thanks David > --- > 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. > * \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. > + * However, the IPA is in a cold-start or reset phase and will result in image > + * quality parameters producing unusable images. Applications are recommended to > + * not consume these frames. All fields of the FrameMetadata structure are > + * valid. > */ > > /** > -- > 2.43.0 >
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@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. 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. 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. > * \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 ? ".. 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 ? Thanks j > + * valid. > */ > > /** > -- > 2.43.0 >
Hi Jacopo, On Tue, 20 May 2025 at 07:52, Jacopo Mondi <jacopo.mondi@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@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. 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? " This status may also indicate corrupt frames produced by the sensor during its startup or restart phase. " > 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. > > > > * \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. Regards, Naush > > Thanks > j > > > + * valid. > > */ > > > > /** > > -- > > 2.43.0 > > >
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@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@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 > > > > >
Hi Jacopo, On Tue, 20 May 2025 at 11:17, Jacopo Mondi <jacopo.mondi@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@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@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 > > > > > > > >
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@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@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@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 > > > > > > > > > > >
Hi Naush one more question on this patch 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@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. > * \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. > + * However, the IPA is in a cold-start or reset phase and will result in image > + * quality parameters producing unusable images. Applications are recommended to > + * not consume these frames. All fields of the FrameMetadata structure are > + * valid. Will the metadata associated with a FrameStartup frame be considered valid ? > */ > > /** > -- > 2.43.0 >
Hi Jacopo, On Wed, 21 May 2025 at 11:35, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > Hi Naush > one more question on this patch > > 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@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. > > * \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. > > + * However, the IPA is in a cold-start or reset phase and will result > in image > > + * quality parameters producing unusable images. Applications are > recommended to > > + * not consume these frames. All fields of the FrameMetadata structure > are > > + * valid. > > Will the metadata associated with a FrameStartup frame be considered > valid ? > Yes, all metadata associated with FrameStartup ought to be accurate/valid. Regards, Naush > > */ > > > > /** > > -- > > 2.43.0 > > >
On Tue, May 20, 2025 at 02:41:20PM +0200, Jacopo Mondi wrote: > On Tue, May 20, 2025 at 12:57:19PM +0100, Naushir Patuck wrote: > > On Tue, 20 May 2025 at 11:17, Jacopo Mondi wrote: > > > On Tue, May 20, 2025 at 09:05:33AM +0100, Naushir Patuck wrote: > > > > On Tue, 20 May 2025 at 07:52, Jacopo Mondi wrote: > > > > > 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@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. I find example useful to better understand the API. How about the following ? * The frame data is partly or fully corrupted, missing or otherwise invalid. * This can for instance indicate a hardware transmission error, or invalid data * produced by the sensor during its startup phase. The sequence and timestamp * fields of the FrameMetadata structure is valid, all 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. > > > > > > > > + * valid. > > > > > > */ > > > > > > > > > > > > /**
Hi Laurent, On Wed, 21 May 2025 at 12:38, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > On Tue, May 20, 2025 at 02:41:20PM +0200, Jacopo Mondi wrote: > > On Tue, May 20, 2025 at 12:57:19PM +0100, Naushir Patuck wrote: > > > On Tue, 20 May 2025 at 11:17, Jacopo Mondi wrote: > > > > On Tue, May 20, 2025 at 09:05:33AM +0100, Naushir Patuck wrote: > > > > > On Tue, 20 May 2025 at 07:52, Jacopo Mondi wrote: > > > > > > 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@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. > > I find example useful to better understand the API. How about the > following ? > > * The frame data is partly or fully corrupted, missing or otherwise > invalid. > * This can for instance indicate a hardware transmission error, or > invalid data > * produced by the sensor during its startup phase. The sequence and > timestamp > * fields of the FrameMetadata structure is valid, all the other fields > may be > * invalid. > That works for me, I'll update the text as above. Naush > > > > > > > > * \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. > > > > > > > > > > + * valid. > > > > > > > */ > > > > > > > > > > > > > > /** > > -- > Regards, > > Laurent Pinchart >
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. * \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. + * However, the IPA is in a cold-start or reset phase and will result in image + * quality parameters producing unusable images. Applications are recommended to + * not consume these frames. All fields of the FrameMetadata structure are + * valid. */ /**
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@raspberrypi.com> --- include/libcamera/framebuffer.h | 1 + src/libcamera/framebuffer.cpp | 11 +++++++++-- 2 files changed, 10 insertions(+), 2 deletions(-)