| Message ID | 20251024085130.995967-20-stefan.klug@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Stefan On Fri, Oct 24, 2025 at 10:50:43AM +0200, Stefan Klug wrote: > Update the algorithm documentation to reflect the changed timing model. > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > --- > src/ipa/libipa/algorithm.cpp | 28 ++++++++++++++-------------- > 1 file changed, 14 insertions(+), 14 deletions(-) > > diff --git a/src/ipa/libipa/algorithm.cpp b/src/ipa/libipa/algorithm.cpp > index 201efdfdba25..44b6bdfa6972 100644 > --- a/src/ipa/libipa/algorithm.cpp > +++ b/src/ipa/libipa/algorithm.cpp > @@ -76,11 +76,12 @@ namespace ipa { > * > * This function is called for each request queued to the camera. It provides > * the controls stored in the request to the algorithm. The \a frame number > - * is the Request sequence number and identifies the desired corresponding > + * is the sensor sequence number and identifies the desired corresponding I wonder if the frame/request sequence number is even meaningful for algorithsm. After all, this function should mostly pass to the algorithms the control to use to change the algorithm's state. The only usage I see of frame in algos' queueRequest is to find out if this is the first call (frame == 0) and the ISP block needs to be fully configured or not (something which might be handled with a flag set/reset at configure() time ?) > * frame to target for the controls to take effect. > * > * Algorithms shall read the applicable controls and store their value for later > - * use during frame processing. > + * use during frame processing. All values that are already known shall be > + * updated in \a frameContext. I'm not sure I get what you mean. Is this to suggest that the IPA that has to accummulate controls which have arrived too late ? > */ > > /** > @@ -98,14 +99,18 @@ namespace ipa { > * Algorithms shall fill in the parameter structure fields appropriately to > * configure the ISP processing blocks that they are responsible for. This > * includes setting fields and flags that enable those processing blocks. > + * > + * Additionally \a frameContext shall be updated with the most up to date values > + * from active state. I think this needs to be expanded a little. I interpret this as the frame context shall be kept up-to-date with the values that are effectivelly set in the parameters buffer, right ? Is the idea to move the values from the active state where they have been populated by process() to the frame context at preprare() time, so that the frame context is only updated when the parameters buffer is programmed ? I would keep this note as part of the previous paragraph, and take the occasion to modify it slightly. * Algorithms shall fill in the \a params buffer appropriately to * configure the ISP processing blocks that they are responsible for. * This includes setting fields and flags that enable those processing * blocks. The values used to populate \a params should be used to * update \a frameContext so that it reflects the ISP configuration * used to produce \a frame. */ Take in whatever you think is approprate from this > */ > > /** > * \fn Algorithm::process() > * \brief Process ISP statistics, and run algorithm operations > * \param[in] context The shared IPA context > - * \param[in] frame The frame context sequence number > - * \param[in] frameContext The current frame's context > + * \param[in] frame The frame sequence number that produces the stats for which stats have been produced ? > + * \param[in] frameContext The frame context for the frame that produced the > + * stats same here > * \param[in] stats The IPA statistics and ISP results > * \param[out] metadata Metadata for the frame, to be filled by the algorithm > * > @@ -118,19 +123,14 @@ namespace ipa { > * computationally expensive calculations or operations must be handled > * asynchronously in a separate thread. > * > - * Algorithms can store state in their respective IPAFrameContext structures, > - * and reference state from the IPAFrameContext of other algorithms. > - * > - * \todo Historical data may be required as part of the processing. > - * Either the previous frame, or the IPAFrameContext state of the frame > - * that generated the statistics for this operation may be required for > - * some advanced algorithms to prevent oscillations or support control > - * loops correctly. Only a single IPAFrameContext is available currently, > - * and so any data stored may represent the results of the previously > - * completed operations. > + * Algorithms shall update the active state. The frameContext shall *not* be > + * updated as that frame was already produced. I would rather modify the first paragraph otherwise it feels like this has been added here to tell us not to do what we have been doing so far. The end result might be: * This function is called while camera is running for every frame * processed by the ISP, to process statistics generated from that * frame. Algorithms shall use this data to run calculations, update * the active state and fill the frame metadata with the results of * the calculations. > * > * Care shall be taken to ensure the ordering of access to the information > * such that the algorithms use up to date state as required. > + * > + * The \a stats parameter can be null in which case ony the frame metadata shall s/ony/only s/frame metadata/ \a metadata / > + * be filled. s/filled/populated ? > */ > > /** > -- > 2.48.1 >
Quoting Jacopo Mondi (2025-11-07 10:18:54) > Hi Stefan > > On Fri, Oct 24, 2025 at 10:50:43AM +0200, Stefan Klug wrote: > > Update the algorithm documentation to reflect the changed timing model. > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > --- > > src/ipa/libipa/algorithm.cpp | 28 ++++++++++++++-------------- > > 1 file changed, 14 insertions(+), 14 deletions(-) > > > > diff --git a/src/ipa/libipa/algorithm.cpp b/src/ipa/libipa/algorithm.cpp > > index 201efdfdba25..44b6bdfa6972 100644 > > --- a/src/ipa/libipa/algorithm.cpp > > +++ b/src/ipa/libipa/algorithm.cpp > > @@ -76,11 +76,12 @@ namespace ipa { Sending out my inline comments and discussions notes while talking through with Stefan which has helped me comprehend all the changes going on in libipa: Context location: queueRequest() > > * > > * This function is called for each request queued to the camera. It provides > > * the controls stored in the request to the algorithm. The \a frame number > > - * is the Request sequence number and identifies the desired corresponding > > + * is the sensor sequence number and identifies the desired corresponding > > I wonder if the frame/request sequence number is even meaningful for > algorithsm. After all, this function should mostly pass to the > algorithms the control to use to change the algorithm's state. > > The only usage I see of frame in algos' queueRequest is to find out if > this is the first call (frame == 0) and the ISP block needs to be > fully configured or not (something which might be handled with a > flag set/reset at configure() time ?) Talking through this with Stefan, it sounds like we might make more updates here. This is really: "Initialise frame context for a given sensor sequence number" This changes the mechanics, because if we're freely running with no external requests, then initialise frame context can still be called for every frame, (either actively by queueRequest, or passively by computeParams/Algorithm::prepare if it has not yet been initialised). This way the IPA can run even when we may have underruns from the application. > > * frame to target for the controls to take effect. > > * > > * Algorithms shall read the applicable controls and store their value for later > > - * use during frame processing. > > + * use during frame processing. All values that are already known shall be > > + * updated in \a frameContext. > > I'm not sure I get what you mean. > > Is this to suggest that the IPA that has to accummulate controls which > have arrived too late ? Stefan: "If we're in manual exposure mode, we know what exposure (or other manual controls) are to be set so we can set them already". With that context: So we can already decide if we know values for this frame. Anything manually set in active state manual controls should be applied here. So how can we word this more explicitly? """ Algorithms shall read the applicable controls and store their value for later use during frame processing. Incoming controls are merged with the current known state to update the \a frameContext. """ > > > */ > > > > /** > > @@ -98,14 +99,18 @@ namespace ipa { Context: prepare() > > * Algorithms shall fill in the parameter structure fields appropriately to > > * configure the ISP processing blocks that they are responsible for. This > > * includes setting fields and flags that enable those processing blocks. > > + * > > + * Additionally \a frameContext shall be updated with the most up to date values > > + * from active state. > > I think this needs to be expanded a little. > > I interpret this as the frame context shall be kept up-to-date with > the values that are effectivelly set in the parameters buffer, right ? > > Is the idea to move the values from the active state where they have > been populated by process() to the frame context at preprare() time, > so that the frame context is only updated when the parameters buffer is > programmed ? Yes ? (Is my interpretation). > I would keep this note as part of the previous paragraph, and take the > occasion to modify it slightly. > > * Algorithms shall fill in the \a params buffer appropriately to > * configure the ISP processing blocks that they are responsible for. > * This includes setting fields and flags that enable those processing > * blocks. The values used to populate \a params should be used to > * update \a frameContext so that it reflects the ISP configuration > * used to produce \a frame. > */ > > Take in whatever you think is approprate from this That sounds reasonable to me ... my only hesitation is that from my current understanding there will still be a split between which data we use or rather report as 'the metadata that was used to generate this frame': - Lux, ColourTemperature, ... vs the metadata that 'is represented in this frame': - Exposure, Gain, ... Maybe somehow we have to find a way to categorise or group these two types of metadata... Especially as those early types can have two values! We can output both of: - lux - The lux used by algorithms (from active state) to decide what to do to generate this frame (which is what we will report with https://patchwork.libcamera.org/project/libcamera/list/?series=5595) - The lux level *of* this frame. Which is what 'I used to think' this represents. - ColourTemperature: - The ColourTemperature (from active state, or a manual control) to decide what gains to generate in Awb. - The colour temperature of this image. I have no idea how we can convey these two numbers of the same metadata. > > */ > > > > /** > > * \fn Algorithm::process() > > * \brief Process ISP statistics, and run algorithm operations > > * \param[in] context The shared IPA context > > - * \param[in] frame The frame context sequence number > > - * \param[in] frameContext The current frame's context > > + * \param[in] frame The frame sequence number that produces the stats > > for which stats have been produced > > ? > > > + * \param[in] frameContext The frame context for the frame that produced the > > + * stats > > same here > > > * \param[in] stats The IPA statistics and ISP results > > * \param[out] metadata Metadata for the frame, to be filled by the algorithm > > * > > @@ -118,19 +123,14 @@ namespace ipa { context: Still in process(): > > * computationally expensive calculations or operations must be handled > > * asynchronously in a separate thread. > > * > > - * Algorithms can store state in their respective IPAFrameContext structures, > > - * and reference state from the IPAFrameContext of other algorithms. > > - * > > - * \todo Historical data may be required as part of the processing. > > - * Either the previous frame, or the IPAFrameContext state of the frame > > - * that generated the statistics for this operation may be required for > > - * some advanced algorithms to prevent oscillations or support control > > - * loops correctly. Only a single IPAFrameContext is available currently, > > - * and so any data stored may represent the results of the previously > > - * completed operations. > > + * Algorithms shall update the active state. The frameContext shall *not* be > > + * updated as that frame was already produced. > > I would rather modify the first paragraph otherwise it feels like this > has been added here to tell us not to do what we have been doing so > far. > > The end result might be: > > * This function is called while camera is running for every frame while the camera > * processed by the ISP, to process statistics generated from that > * frame. Algorithms shall use this data to run calculations, update > * the active state and fill the frame metadata with the results of > * the calculations. Frame metadata might be updated with the values from the framecontext ... not the most recent calculations. But I still think that's dependent/specific to different variables (past variables, that report what's used for this frame, vs present state variables which are what are measured from the frame). > > > * > > * Care shall be taken to ensure the ordering of access to the information > > * such that the algorithms use up to date state as required. > > + * > > + * The \a stats parameter can be null in which case ony the frame metadata shall > > s/ony/only > s/frame metadata/ \a metadata / > > > + * be filled. > > s/filled/populated ? > > > */ > > > > /** > > -- > > 2.48.1 > >
Hi Kieran On Wed, Nov 19, 2025 at 02:47:45PM +0000, Kieran Bingham wrote: > Quoting Jacopo Mondi (2025-11-07 10:18:54) > > Hi Stefan > > > > On Fri, Oct 24, 2025 at 10:50:43AM +0200, Stefan Klug wrote: > > > Update the algorithm documentation to reflect the changed timing model. > > > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > > --- > > > src/ipa/libipa/algorithm.cpp | 28 ++++++++++++++-------------- > > > 1 file changed, 14 insertions(+), 14 deletions(-) > > > > > > diff --git a/src/ipa/libipa/algorithm.cpp b/src/ipa/libipa/algorithm.cpp > > > index 201efdfdba25..44b6bdfa6972 100644 > > > --- a/src/ipa/libipa/algorithm.cpp > > > +++ b/src/ipa/libipa/algorithm.cpp > > > @@ -76,11 +76,12 @@ namespace ipa { > > > Sending out my inline comments and discussions notes while talking > through with Stefan which has helped me comprehend all the changes going > on in libipa: > > > Context location: > queueRequest() > > > > * > > > * This function is called for each request queued to the camera. It provides > > > * the controls stored in the request to the algorithm. The \a frame number > > > - * is the Request sequence number and identifies the desired corresponding > > > + * is the sensor sequence number and identifies the desired corresponding > > > > I wonder if the frame/request sequence number is even meaningful for > > algorithsm. After all, this function should mostly pass to the > > algorithms the control to use to change the algorithm's state. > > > > The only usage I see of frame in algos' queueRequest is to find out if > > this is the first call (frame == 0) and the ISP block needs to be > > fully configured or not (something which might be handled with a > > flag set/reset at configure() time ?) > > > Talking through this with Stefan, it sounds like we might make more > updates here. > > This is really: > > "Initialise frame context for a given sensor sequence number" see, this is where I got disconnected.. queueRequest() is called in response to an application action and it it is associated with a request sequence. I understood Stefan did a a lot of work in the pipeline to associate to a Request the frame sequence it is tied to, but I wonder if this means all pipelines should go through that complicated mechanism. > > This changes the mechanics, because if we're freely running with no > external requests, then initialise frame context can still be called for > every frame, (either actively by queueRequest, or passively by > computeParams/Algorithm::prepare if it has not yet been initialised). I agree we shall have a frame context for every sensor frame. Possibly the whole IPA::queueRequest() thing is wrong and what we should really have is an IPA::frameStarted(). The pipeline could keep a queue of pending request, and at every frame start consume the less recent one and associate it with the frame sequence that just started and then call IPA::frameStarted() with the controls associated to the just-consumed request. Take this as just a discussion starter, because without a proper design I'm basically just thinking out loud. > > This way the IPA can run even when we may have underruns from the > application. > > > > > * frame to target for the controls to take effect. > > > * > > > * Algorithms shall read the applicable controls and store their value for later > > > - * use during frame processing. > > > + * use during frame processing. All values that are already known shall be > > > + * updated in \a frameContext. > > > > I'm not sure I get what you mean. > > > > Is this to suggest that the IPA that has to accummulate controls which > > have arrived too late ? > > Stefan: "If we're in manual exposure mode, we know what exposure (or > other manual controls) are to be set so we can set them already". > > With that context: > > So we can already decide if we know values for this frame. Anything > manually set in active state manual controls should be applied here. > > So how can we word this more explicitly? > > """ > Algorithms shall read the applicable controls and store their value for > later use during frame processing. Incoming controls are merged with the > current known state to update the \a frameContext. > """ Ack, thanks for clarifying this > > > > > > */ > > > > > > /** > > > @@ -98,14 +99,18 @@ namespace ipa { > > Context: prepare() > > > > * Algorithms shall fill in the parameter structure fields appropriately to > > > * configure the ISP processing blocks that they are responsible for. This > > > * includes setting fields and flags that enable those processing blocks. > > > + * > > > + * Additionally \a frameContext shall be updated with the most up to date values > > > + * from active state. > > > > I think this needs to be expanded a little. > > > > I interpret this as the frame context shall be kept up-to-date with > > the values that are effectivelly set in the parameters buffer, right ? > > > > Is the idea to move the values from the active state where they have > > been populated by process() to the frame context at preprare() time, > > so that the frame context is only updated when the parameters buffer is > > programmed ? > > Yes ? (Is my interpretation). > Maybe we need to expand this a bit in a general document about the whole IPA state machine. Or do you think what we have here is enough ? > > > I would keep this note as part of the previous paragraph, and take the > > occasion to modify it slightly. > > > > * Algorithms shall fill in the \a params buffer appropriately to > > * configure the ISP processing blocks that they are responsible for. > > * This includes setting fields and flags that enable those processing > > * blocks. The values used to populate \a params should be used to > > * update \a frameContext so that it reflects the ISP configuration > > * used to produce \a frame. > > */ > > > > Take in whatever you think is approprate from this > > That sounds reasonable to me ... my only hesitation is that from my > current understanding there will still be a split between which data we > use or rather report as 'the metadata that was used to generate this > frame': > - Lux, ColourTemperature, ... > > vs the metadata that 'is represented in this frame': > - Exposure, Gain, ... > > Maybe somehow we have to find a way to categorise or group these two > types of metadata... Especially as those early types can have two > values! > > > We can output both of: > - lux > - The lux used by algorithms (from active state) to decide what to > do to generate this frame (which is what we will report with > https://patchwork.libcamera.org/project/libcamera/list/?series=5595) > - The lux level *of* this frame. Which is what 'I used to think' > this represents. > > - ColourTemperature: > - The ColourTemperature (from active state, or a manual control) to > decide what gains to generate in Awb. > - The colour temperature of this image. > > > I have no idea how we can convey these two numbers of the same metadata. > I'm sorry but isn't the "lux used by algorithms" the same as the "lux of this image" ? Or is the "lux used by algorithm" being estimated on older images ? > > > > */ > > > > > > /** > > > * \fn Algorithm::process() > > > * \brief Process ISP statistics, and run algorithm operations > > > * \param[in] context The shared IPA context > > > - * \param[in] frame The frame context sequence number > > > - * \param[in] frameContext The current frame's context > > > + * \param[in] frame The frame sequence number that produces the stats > > > > for which stats have been produced > > > > ? > > > > > + * \param[in] frameContext The frame context for the frame that produced the > > > + * stats > > > > same here > > > > > * \param[in] stats The IPA statistics and ISP results > > > * \param[out] metadata Metadata for the frame, to be filled by the algorithm > > > * > > > @@ -118,19 +123,14 @@ namespace ipa { > > context: Still in process(): > > > > * computationally expensive calculations or operations must be handled > > > * asynchronously in a separate thread. > > > * > > > - * Algorithms can store state in their respective IPAFrameContext structures, > > > - * and reference state from the IPAFrameContext of other algorithms. > > > - * > > > - * \todo Historical data may be required as part of the processing. > > > - * Either the previous frame, or the IPAFrameContext state of the frame > > > - * that generated the statistics for this operation may be required for > > > - * some advanced algorithms to prevent oscillations or support control > > > - * loops correctly. Only a single IPAFrameContext is available currently, > > > - * and so any data stored may represent the results of the previously > > > - * completed operations. > > > + * Algorithms shall update the active state. The frameContext shall *not* be > > > + * updated as that frame was already produced. > > > > I would rather modify the first paragraph otherwise it feels like this > > has been added here to tell us not to do what we have been doing so > > far. > > > > The end result might be: > > > > * This function is called while camera is running for every frame > > while the camera > > > * processed by the ISP, to process statistics generated from that > > * frame. Algorithms shall use this data to run calculations, update > > * the active state and fill the frame metadata with the results of > > * the calculations. > > Frame metadata might be updated with the values from the framecontext > ... not the most recent calculations. > > But I still think that's dependent/specific to different variables (past > variables, that report what's used for this frame, vs present state variables > which are what are measured from the frame). > > > > > > > * > > > * Care shall be taken to ensure the ordering of access to the information > > > * such that the algorithms use up to date state as required. > > > + * > > > + * The \a stats parameter can be null in which case ony the frame metadata shall > > > > s/ony/only > > s/frame metadata/ \a metadata / > > > > > + * be filled. > > > > s/filled/populated ? > > > > > */ > > > > > > /** > > > -- > > > 2.48.1 > > >
diff --git a/src/ipa/libipa/algorithm.cpp b/src/ipa/libipa/algorithm.cpp index 201efdfdba25..44b6bdfa6972 100644 --- a/src/ipa/libipa/algorithm.cpp +++ b/src/ipa/libipa/algorithm.cpp @@ -76,11 +76,12 @@ namespace ipa { * * This function is called for each request queued to the camera. It provides * the controls stored in the request to the algorithm. The \a frame number - * is the Request sequence number and identifies the desired corresponding + * is the sensor sequence number and identifies the desired corresponding * frame to target for the controls to take effect. * * Algorithms shall read the applicable controls and store their value for later - * use during frame processing. + * use during frame processing. All values that are already known shall be + * updated in \a frameContext. */ /** @@ -98,14 +99,18 @@ namespace ipa { * Algorithms shall fill in the parameter structure fields appropriately to * configure the ISP processing blocks that they are responsible for. This * includes setting fields and flags that enable those processing blocks. + * + * Additionally \a frameContext shall be updated with the most up to date values + * from active state. */ /** * \fn Algorithm::process() * \brief Process ISP statistics, and run algorithm operations * \param[in] context The shared IPA context - * \param[in] frame The frame context sequence number - * \param[in] frameContext The current frame's context + * \param[in] frame The frame sequence number that produces the stats + * \param[in] frameContext The frame context for the frame that produced the + * stats * \param[in] stats The IPA statistics and ISP results * \param[out] metadata Metadata for the frame, to be filled by the algorithm * @@ -118,19 +123,14 @@ namespace ipa { * computationally expensive calculations or operations must be handled * asynchronously in a separate thread. * - * Algorithms can store state in their respective IPAFrameContext structures, - * and reference state from the IPAFrameContext of other algorithms. - * - * \todo Historical data may be required as part of the processing. - * Either the previous frame, or the IPAFrameContext state of the frame - * that generated the statistics for this operation may be required for - * some advanced algorithms to prevent oscillations or support control - * loops correctly. Only a single IPAFrameContext is available currently, - * and so any data stored may represent the results of the previously - * completed operations. + * Algorithms shall update the active state. The frameContext shall *not* be + * updated as that frame was already produced. * * Care shall be taken to ensure the ordering of access to the information * such that the algorithms use up to date state as required. + * + * The \a stats parameter can be null in which case ony the frame metadata shall + * be filled. */ /**
Update the algorithm documentation to reflect the changed timing model. Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> --- src/ipa/libipa/algorithm.cpp | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-)