[libcamera-devel,RFC] ipa: ipu3: Retain request controls in the IPA

15988 diff mbox series

To ensure consistency in processing control lists, an application does
not have to continually set controls that were already set, but not
expected to change.

To ensure we keep that state, merge incoming request control lists with
a stored ControlList that is owned by the IPA and store the state of
that merged list to the per frame FrameContext so that algorithms know
the full expected control state at that time.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

---

RFC:

1)

If ControlList.update() were implemented like .merge, but with
overriding existing values, instead of leaving them then this could be
simplified to :

+	retainedControls_.update(controls);
+	context_.frameContexts[frame % kMaxFrameContexts] = { frame, std::copy(retainedControls_) };

Not yet sure if that's more efficient or if it doesn't make much
difference.

2)

This will not support triggered controls that are set once to
'fire'... Do we have / plan to have any of those?

3)

Do we want to keep both a delta control list (the incoming controls) as
well as the merged/retained list for full state so IPA's can know if
something was changed?

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/ipa/ipu3/ipu3.cpp | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

Hi Kieran,

On 5/19/22 16:16, Kieran Bingham via libcamera-devel wrote:
> To ensure consistency in processing control lists, an application does
> not have to continually set controls that were already set, but not
> expected to change.


So what is this implying...

If application sends in manual-exposure=100 just for frame 25th, what is 
it expected to do to "unset" it from frame 26th?

- Send in manual-exposure = 0 ?

- send in no manual-exposure, in that case, I assume that the retained 
control list (purpose of this patch) is to set exposure=100 for frame 
26th, which is probably right thing to do?, I am not sure...


>
> To ensure we keep that state, merge incoming request control lists with
> a stored ControlList that is owned by the IPA and store the state of
> that merged list to the per frame FrameContext so that algorithms know
> the full expected control state at that time.
>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
> ---
>
> RFC:
>
> 1)
>
> If ControlList.update() were implemented like .merge, but with
> overriding existing values, instead of leaving them then this could be
> simplified to :
>
> +	retainedControls_.update(controls);
> +	context_.frameContexts[frame % kMaxFrameContexts] = { frame, std::copy(retainedControls_) };
>
> Not yet sure if that's more efficient or if it doesn't make much
> difference.
>
> 2)
>
> This will not support triggered controls that are set once to
> 'fire'... Do we have / plan to have any of those?
>
> 3)
>
> Do we want to keep both a delta control list (the incoming controls) as
> well as the merged/retained list for full state so IPA's can know if
> something was changed?


I think so yes. Algorithms might need to look up the delta to know in 
advance whether or not a manual control is set for a frame or not. 
Accordingly, they might decide the relevant parameter(s)
For e.g. manual exposure but automatic gain.

>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>   src/ipa/ipu3/ipu3.cpp | 23 ++++++++++++++++++++++-
>   1 file changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 2f6bb672f7bb..89fc34f86e46 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -176,6 +176,9 @@ private:
>   
>   	/* Local parameter storage */
>   	struct IPAContext context_;
> +
> +	/* Control retention to maintain mode states */
> +	ControlList retainedControls_;
>   };
>   
>   /**
> @@ -456,6 +459,8 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
>   
>   	/* Clean IPAActiveState at each reconfiguration. */
>   	context_.activeState = {};
> +	retainedControls_.clear();
> +
>   	IPAFrameContext initFrameContext;
>   	context_.frameContexts.fill(initFrameContext);
>   
> @@ -617,8 +622,24 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
>    */
>   void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)
>   {
> +	/*
> +	 * Controls are retained, to ensure any mode updates are persistant.
> +	 * We merge them into our class control storage before assigning to
> +	 * the frame context.
> +	 *
> +	 * \todo This will not support trigger controls and may need rework if
> +	 *	 we add any to prevent continually retriggering.
> +	 *
> +	 * \todo We may wish to store both the full merged control list, as well
> +	 *	 as the delta (\a controls) to facilitate algorithms identifying
> +	 *	 when things have changed.
> +	 */
> +	ControlList mergeControls = controls;
> +	mergeControls.merge(retainedControls_);
> +	retainedControls_ = mergeControls;
> +
>   	/* \todo Start processing for 'frame' based on 'controls'. */
> -	context_.frameContexts[frame % kMaxFrameContexts] = { frame, controls };
> +	context_.frameContexts[frame % kMaxFrameContexts] = { frame, std::move(mergeControls) };
>   }
>   
>   /**

Hello,

On Thu, May 19, 2022 at 06:02:29PM +0200, Umang Jain via libcamera-devel wrote:
> Hi Kieran,
> 
> On 5/19/22 16:16, Kieran Bingham via libcamera-devel wrote:
> > To ensure consistency in processing control lists, an application does
> > not have to continually set controls that were already set, but not
> > expected to change.
> 
> 
> So what is this implying...
> 
> If application sends in manual-exposure=100 just for frame 25th, what is it
> expected to do to "unset" it from frame 26th?

imo it depends on what the desired effect is by "unsetting" it. If the
desire is to revert to the default value, then it's not possible (unless
the application saves it and re-applies it). If the desire is to revert
to auto then just set auto mode back on.

> 
> - Send in manual-exposure = 0 ?

That'll just set exposure to 0 :D

> 
> - send in no manual-exposure, in that case, I assume that the retained
> control list (purpose of this patch) is to set exposure=100 for frame 26th,
> which is probably right thing to do?, I am not sure...

Yeah, that's what should happen (what this patch enables).

> 
> 
> > 
> > To ensure we keep that state, merge incoming request control lists with
> > a stored ControlList that is owned by the IPA and store the state of
> > that merged list to the per frame FrameContext so that algorithms know
> > the full expected control state at that time.
> > 
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > 
> > ---
> > 
> > RFC:
> > 
> > 1)
> > 
> > If ControlList.update() were implemented like .merge, but with
> > overriding existing values, instead of leaving them then this could be
> > simplified to :
> > 
> > +	retainedControls_.update(controls);
> > +	context_.frameContexts[frame % kMaxFrameContexts] = { frame, std::copy(retainedControls_) };
> > 
> > Not yet sure if that's more efficient or if it doesn't make much
> > difference.

It makes the code look nicer :p

> > 
> > 2)
> > 
> > This will not support triggered controls that are set once to
> > 'fire'... Do we have / plan to have any of those?

Yeah that's kind of a big deal for AE.

Although the solution could be as simple (read: hacky) as having the IPA
overwrite the ExposureValue/AnalogueGain values at control application
time to the values calculated by AE when in auto mode. Those controls
need to report in metadata the actual values that were used anyway, so
it might actually be a viable solution.

Then, when the mode is switched from auto -> disabled and the control
isn't resubmitted, it'll retain the value that it had from the last time
auto was on (because the IPA overwrote them).

tl;dr, If the mode is auto and the manual controls are supplied, IPA
should overwrite the requested manual control values in the retained
requested contols probably at the same time as it reports them in
metadata. This is reationalized as "submitting manual controls is
invalid if the mode is auto" anyway, plus "the manual controls in
metadata are used to report the actual values that were used".

> > 
> > 3)
> > 
> > Do we want to keep both a delta control list (the incoming controls) as
> > well as the merged/retained list for full state so IPA's can know if
> > something was changed?
> 
> I think so yes. Algorithms might need to look up the delta to know in
> advance whether or not a manual control is set for a frame or not.
> Accordingly, they might decide the relevant parameter(s)
> For e.g. manual exposure but automatic gain.

I actually don't think this is necessary. If a control doesn't change,
it doesn't need to be resubmitted and it'll take the most recently set
value. From the IPA's point of view (with retention), every control will
be set every request. So it's not important to see the delta.

The exception of course is with trigger controls, which we don't want to
allow. AE almost had this, but I think we can rationalize it as
mentioned above: submitting manual controls during auto is invalid, so
we can do whatever we want, thus we don't even have to treat it as a
trigger, we can just overwrite it.


Paul

> > 
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> >   src/ipa/ipu3/ipu3.cpp | 23 ++++++++++++++++++++++-
> >   1 file changed, 22 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> > index 2f6bb672f7bb..89fc34f86e46 100644
> > --- a/src/ipa/ipu3/ipu3.cpp
> > +++ b/src/ipa/ipu3/ipu3.cpp
> > @@ -176,6 +176,9 @@ private:
> >   	/* Local parameter storage */
> >   	struct IPAContext context_;
> > +
> > +	/* Control retention to maintain mode states */
> > +	ControlList retainedControls_;
> >   };
> >   /**
> > @@ -456,6 +459,8 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
> >   	/* Clean IPAActiveState at each reconfiguration. */
> >   	context_.activeState = {};
> > +	retainedControls_.clear();
> > +
> >   	IPAFrameContext initFrameContext;
> >   	context_.frameContexts.fill(initFrameContext);
> > @@ -617,8 +622,24 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
> >    */
> >   void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)
> >   {
> > +	/*
> > +	 * Controls are retained, to ensure any mode updates are persistant.
> > +	 * We merge them into our class control storage before assigning to
> > +	 * the frame context.
> > +	 *
> > +	 * \todo This will not support trigger controls and may need rework if
> > +	 *	 we add any to prevent continually retriggering.
> > +	 *
> > +	 * \todo We may wish to store both the full merged control list, as well
> > +	 *	 as the delta (\a controls) to facilitate algorithms identifying
> > +	 *	 when things have changed.
> > +	 */
> > +	ControlList mergeControls = controls;
> > +	mergeControls.merge(retainedControls_);
> > +	retainedControls_ = mergeControls;
> > +
> >   	/* \todo Start processing for 'frame' based on 'controls'. */
> > -	context_.frameContexts[frame % kMaxFrameContexts] = { frame, controls };
> > +	context_.frameContexts[frame % kMaxFrameContexts] = { frame, std::move(mergeControls) };
> >   }
> >   /**

On Thu, May 19, 2022 at 04:16:42PM +0200, Kieran Bingham via libcamera-devel wrote:
> To ensure consistency in processing control lists, an application does
> not have to continually set controls that were already set, but not
> expected to change.
> 
> To ensure we keep that state, merge incoming request control lists with
> a stored ControlList that is owned by the IPA and store the state of
> that merged list to the per frame FrameContext so that algorithms know
> the full expected control state at that time.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Oops, I forgot:

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

I think this patch as-is is totally fine. Assuming merge() behaves
properly, this does exactly what we want. The "trigger" (though not
really trigger) controls for AE we can implement them
differently/separately/on top, so this patch is fine.

> 
> ---
> 
> RFC:
> 
> 1)
> 
> If ControlList.update() were implemented like .merge, but with
> overriding existing values, instead of leaving them then this could be
> simplified to :
> 
> +	retainedControls_.update(controls);
> +	context_.frameContexts[frame % kMaxFrameContexts] = { frame, std::copy(retainedControls_) };
> 
> Not yet sure if that's more efficient or if it doesn't make much
> difference.
> 
> 2)
> 
> This will not support triggered controls that are set once to
> 'fire'... Do we have / plan to have any of those?
> 
> 3)
> 
> Do we want to keep both a delta control list (the incoming controls) as
> well as the merged/retained list for full state so IPA's can know if
> something was changed?
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  src/ipa/ipu3/ipu3.cpp | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 2f6bb672f7bb..89fc34f86e46 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -176,6 +176,9 @@ private:
>  
>  	/* Local parameter storage */
>  	struct IPAContext context_;
> +
> +	/* Control retention to maintain mode states */
> +	ControlList retainedControls_;
>  };
>  
>  /**
> @@ -456,6 +459,8 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
>  
>  	/* Clean IPAActiveState at each reconfiguration. */
>  	context_.activeState = {};
> +	retainedControls_.clear();
> +
>  	IPAFrameContext initFrameContext;
>  	context_.frameContexts.fill(initFrameContext);
>  
> @@ -617,8 +622,24 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
>   */
>  void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)
>  {
> +	/*
> +	 * Controls are retained, to ensure any mode updates are persistant.
> +	 * We merge them into our class control storage before assigning to
> +	 * the frame context.
> +	 *
> +	 * \todo This will not support trigger controls and may need rework if
> +	 *	 we add any to prevent continually retriggering.
> +	 *
> +	 * \todo We may wish to store both the full merged control list, as well
> +	 *	 as the delta (\a controls) to facilitate algorithms identifying
> +	 *	 when things have changed.
> +	 */
> +	ControlList mergeControls = controls;
> +	mergeControls.merge(retainedControls_);
> +	retainedControls_ = mergeControls;
> +
>  	/* \todo Start processing for 'frame' based on 'controls'. */
> -	context_.frameContexts[frame % kMaxFrameContexts] = { frame, controls };
> +	context_.frameContexts[frame % kMaxFrameContexts] = { frame, std::move(mergeControls) };
>  }
>  
>  /**
> -- 
> 2.25.1
>

Quoting paul.elder@ideasonboard.com (2022-05-20 10:12:55)
> Hello,
> 
> On Thu, May 19, 2022 at 06:02:29PM +0200, Umang Jain via libcamera-devel wrote:
> > Hi Kieran,
> > 
> > On 5/19/22 16:16, Kieran Bingham via libcamera-devel wrote:
> > > To ensure consistency in processing control lists, an application does
> > > not have to continually set controls that were already set, but not
> > > expected to change.
> > 
> > 
> > So what is this implying...
> > 
> > If application sends in manual-exposure=100 just for frame 25th, what is it
> > expected to do to "unset" it from frame 26th?
> 
> imo it depends on what the desired effect is by "unsetting" it. If the
> desire is to revert to the default value, then it's not possible (unless
> the application saves it and re-applies it). If the desire is to revert
> to auto then just set auto mode back on.
> 
> > 
> > - Send in manual-exposure = 0 ?
> 
> That'll just set exposure to 0 :D
> 
> > 
> > - send in no manual-exposure, in that case, I assume that the retained
> > control list (purpose of this patch) is to set exposure=100 for frame 26th,
> > which is probably right thing to do?, I am not sure...
> 
> Yeah, that's what should happen (what this patch enables).
> 
> > 
> > 
> > > 
> > > To ensure we keep that state, merge incoming request control lists with
> > > a stored ControlList that is owned by the IPA and store the state of
> > > that merged list to the per frame FrameContext so that algorithms know
> > > the full expected control state at that time.
> > > 
> > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > 
> > > ---
> > > 
> > > RFC:
> > > 
> > > 1)
> > > 
> > > If ControlList.update() were implemented like .merge, but with
> > > overriding existing values, instead of leaving them then this could be
> > > simplified to :
> > > 
> > > +   retainedControls_.update(controls);
> > > +   context_.frameContexts[frame % kMaxFrameContexts] = { frame, std::copy(retainedControls_) };
> > > 
> > > Not yet sure if that's more efficient or if it doesn't make much
> > > difference.
> 
> It makes the code look nicer :p

This patch also has a bug as the ControlList is not initialized with
(controls::controls) so the Info map is invalid.

I almost wish we had more explicitly typed ControlLists for 'libcamera
controls' and 'v4l2 controls' (and properties ...) which I think Laurent
started to work on but didn't like quite some time ago.


> > > 
> > > 2)
> > > 
> > > This will not support triggered controls that are set once to
> > > 'fire'... Do we have / plan to have any of those?
> 
> Yeah that's kind of a big deal for AE.
> 
> Although the solution could be as simple (read: hacky) as having the IPA
> overwrite the ExposureValue/AnalogueGain values at control application
> time to the values calculated by AE when in auto mode. Those controls
> need to report in metadata the actual values that were used anyway, so
> it might actually be a viable solution.
> 
> Then, when the mode is switched from auto -> disabled and the control
> isn't resubmitted, it'll retain the value that it had from the last time
> auto was on (because the IPA overwrote them).
> 
> tl;dr, If the mode is auto and the manual controls are supplied, IPA
> should overwrite the requested manual control values in the retained
> requested contols probably at the same time as it reports them in
> metadata. This is reationalized as "submitting manual controls is
> invalid if the mode is auto" anyway, plus "the manual controls in
> metadata are used to report the actual values that were used".

Hrm. ... I'll think more about this. The problem there is that the
'retainedControls' can be duplicated from two sequentially queued
Requests which have not yet been processed by the algorithms themselves.

--
Kieran




> 
> > > 
> > > 3)
> > > 
> > > Do we want to keep both a delta control list (the incoming controls) as
> > > well as the merged/retained list for full state so IPA's can know if
> > > something was changed?
> > 
> > I think so yes. Algorithms might need to look up the delta to know in
> > advance whether or not a manual control is set for a frame or not.
> > Accordingly, they might decide the relevant parameter(s)
> > For e.g. manual exposure but automatic gain.
> 
> I actually don't think this is necessary. If a control doesn't change,
> it doesn't need to be resubmitted and it'll take the most recently set
> value. From the IPA's point of view (with retention), every control will
> be set every request. So it's not important to see the delta.
> 
> The exception of course is with trigger controls, which we don't want to
> allow. AE almost had this, but I think we can rationalize it as
> mentioned above: submitting manual controls during auto is invalid, so
> we can do whatever we want, thus we don't even have to treat it as a
> trigger, we can just overwrite it.
> 
> 
> Paul
> 
> > > 
> > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > ---
> > >   src/ipa/ipu3/ipu3.cpp | 23 ++++++++++++++++++++++-
> > >   1 file changed, 22 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> > > index 2f6bb672f7bb..89fc34f86e46 100644
> > > --- a/src/ipa/ipu3/ipu3.cpp
> > > +++ b/src/ipa/ipu3/ipu3.cpp
> > > @@ -176,6 +176,9 @@ private:
> > >     /* Local parameter storage */
> > >     struct IPAContext context_;
> > > +
> > > +   /* Control retention to maintain mode states */
> > > +   ControlList retainedControls_;
> > >   };
> > >   /**
> > > @@ -456,6 +459,8 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
> > >     /* Clean IPAActiveState at each reconfiguration. */
> > >     context_.activeState = {};
> > > +   retainedControls_.clear();
> > > +
> > >     IPAFrameContext initFrameContext;
> > >     context_.frameContexts.fill(initFrameContext);
> > > @@ -617,8 +622,24 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
> > >    */
> > >   void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)
> > >   {
> > > +   /*
> > > +    * Controls are retained, to ensure any mode updates are persistant.
> > > +    * We merge them into our class control storage before assigning to
> > > +    * the frame context.
> > > +    *
> > > +    * \todo This will not support trigger controls and may need rework if
> > > +    *       we add any to prevent continually retriggering.
> > > +    *
> > > +    * \todo We may wish to store both the full merged control list, as well
> > > +    *       as the delta (\a controls) to facilitate algorithms identifying
> > > +    *       when things have changed.
> > > +    */
> > > +   ControlList mergeControls = controls;
> > > +   mergeControls.merge(retainedControls_);
> > > +   retainedControls_ = mergeControls;
> > > +
> > >     /* \todo Start processing for 'frame' based on 'controls'. */
> > > -   context_.frameContexts[frame % kMaxFrameContexts] = { frame, controls };
> > > +   context_.frameContexts[frame % kMaxFrameContexts] = { frame, std::move(mergeControls) };
> > >   }
> > >   /**

Hi Paul

On 5/20/22 11:14, Paul Elder via libcamera-devel wrote:
> On Thu, May 19, 2022 at 04:16:42PM +0200, Kieran Bingham via libcamera-devel wrote:
>> To ensure consistency in processing control lists, an application does
>> not have to continually set controls that were already set, but not
>> expected to change.
>>
>> To ensure we keep that state, merge incoming request control lists with
>> a stored ControlList that is owned by the IPA and store the state of
>> that merged list to the per frame FrameContext so that algorithms know
>> the full expected control state at that time.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Oops, I forgot:
>
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
>
> I think this patch as-is is totally fine. Assuming merge() behaves
> properly, this does exactly what we want. The "trigger" (though not
> really trigger) controls for AE we can implement them
> differently/separately/on top, so this patch is fine.


Ack.

As long as we are on the same page design-wise, I don't have any 
objections to the patch, but I'll hold my R-B for now since the design 
work needs to be solidified in coming days.

>
>> ---
>>
>> RFC:
>>
>> 1)
>>
>> If ControlList.update() were implemented like .merge, but with
>> overriding existing values, instead of leaving them then this could be
>> simplified to :
>>
>> +	retainedControls_.update(controls);
>> +	context_.frameContexts[frame % kMaxFrameContexts] = { frame, std::copy(retainedControls_) };
>>
>> Not yet sure if that's more efficient or if it doesn't make much
>> difference.
>>
>> 2)
>>
>> This will not support triggered controls that are set once to
>> 'fire'... Do we have / plan to have any of those?
>>
>> 3)
>>
>> Do we want to keep both a delta control list (the incoming controls) as
>> well as the merged/retained list for full state so IPA's can know if
>> something was changed?
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> ---
>>   src/ipa/ipu3/ipu3.cpp | 23 ++++++++++++++++++++++-
>>   1 file changed, 22 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
>> index 2f6bb672f7bb..89fc34f86e46 100644
>> --- a/src/ipa/ipu3/ipu3.cpp
>> +++ b/src/ipa/ipu3/ipu3.cpp
>> @@ -176,6 +176,9 @@ private:
>>   
>>   	/* Local parameter storage */
>>   	struct IPAContext context_;
>> +
>> +	/* Control retention to maintain mode states */
>> +	ControlList retainedControls_;
>>   };
>>   
>>   /**
>> @@ -456,6 +459,8 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
>>   
>>   	/* Clean IPAActiveState at each reconfiguration. */
>>   	context_.activeState = {};
>> +	retainedControls_.clear();
>> +
>>   	IPAFrameContext initFrameContext;
>>   	context_.frameContexts.fill(initFrameContext);
>>   
>> @@ -617,8 +622,24 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
>>    */
>>   void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)
>>   {
>> +	/*
>> +	 * Controls are retained, to ensure any mode updates are persistant.
>> +	 * We merge them into our class control storage before assigning to
>> +	 * the frame context.
>> +	 *
>> +	 * \todo This will not support trigger controls and may need rework if
>> +	 *	 we add any to prevent continually retriggering.
>> +	 *
>> +	 * \todo We may wish to store both the full merged control list, as well
>> +	 *	 as the delta (\a controls) to facilitate algorithms identifying
>> +	 *	 when things have changed.
>> +	 */
>> +	ControlList mergeControls = controls;
>> +	mergeControls.merge(retainedControls_);
>> +	retainedControls_ = mergeControls;
>> +
>>   	/* \todo Start processing for 'frame' based on 'controls'. */
>> -	context_.frameContexts[frame % kMaxFrameContexts] = { frame, controls };
>> +	context_.frameContexts[frame % kMaxFrameContexts] = { frame, std::move(mergeControls) };
>>   }
>>   
>>   /**
>> -- 
>> 2.25.1
>>