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

[Kieran Bingham]

Quoting paul.elder at 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 at 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 at 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) };
> > >   }
> > >   /**