[libcamera-devel] [PATCH 1/1] libcamera: controls: Add StartupFrame control

[Laurent Pinchart]

Hello,

On Tue, Jun 13, 2023 at 09:19:25AM +0100, Naushir Patuck via libcamera-devel wrote:
> On Mon, 12 Jun 2023 at 23:33, Kieran Bingham wrote:
> > Quoting David Plowman via libcamera-devel (2023-06-12 10:43:52)
> > > On Mon, 5 Jun 2023 at 10:32, Naushir Patuck wrote:
> > > > On Mon, 5 Jun 2023 at 09:17, Laurent Pinchart wrote:
> > > > > On Wed, May 31, 2023 at 01:57:16PM +0100, Naushir Patuck via libcamera-devel wrote:
> > > > > > Hi David,
> > > > > >
> > > > > > Thank you for this patch.  Indeed removing the drop frame logic the pipeline
> > > > > > handler would simplify things!
> > > > >
> > > > > That's a change I would welcome :-)
> > > > >
> > > > > > On Wed, 31 May 2023 at 13:50, David Plowman via libcamera-devel wrote:
> > > > > > >
> > > > > > > This control is passed back in a frame as metadata to indicate whether
> > > > > > > the camera system is still in a startup phase, and the application is
> > > > > > > advised to avoid using the frame.
> > > > > > >
> > > > > > > Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> > > > > > > ---
> > > > > > >  src/libcamera/control_ids.yaml | 15 +++++++++++++++
> > > > > > >  1 file changed, 15 insertions(+)
> > > > > > >
> > > > > > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> > > > > > > index adea5f90..4742d907 100644
> > > > > > > --- a/src/libcamera/control_ids.yaml
> > > > > > > +++ b/src/libcamera/control_ids.yaml
> > > > > > > @@ -694,6 +694,21 @@ controls:
> > > > > > >              Continuous AF is paused. No further state changes or lens movements
> > > > > > >              will occur until the AfPauseResume control is sent.
> > > > > > >
> > > > > > > +  - StartupFrame:
> > > > > > > +      type: bool
> > > > > > > +      description: |
> > > > > > > +        The value true indicates that the camera system is still in a startup
> > > > > > > +        phase where the output images may not be reliable, or that certain of
> > > > > > > +        the control algorithms (such as AEC/AGC, AWB, and possibly others) may
> > > > > > > +        still be changing quite rapidly.
> > > > >
> > > > > I think we need to decide with a bit more details what constitute a
> > > > > "startup frame" and what doesn't, or we will have all kinds of
> > > > > inconsistent behaviour.
> > > > >
> > > > > I read it that we have multiple criteria on which we base the decision:
> > > > >
> > > > > - output images not being "reliable"
> > > > > - 3A algorithms convergence
> > > > > - "possibly others"
> > > > >
> > > > > The second criteria is fairly clear, but I'm thinking we should possibly
> > > > > exclude it from the startup frames and report convergence of the 3A
> > > > > algorithms instead.
> > > >
> > > > The 3A "startup" convergence is included as a criteria here because during
> > > > startup, we drive the AE/AWB/ALSC as aggressively (i.e. no filtering) as
> > > > possible to achieve convergence as fast as we possibly can.  This means the
> > > > image output can oscillate quite badly - hence applications should avoid
> > > > displaying or consuming them.
> > > >
> > > > I feel that this startup phase needs to be treated differently compared to a
> > > > normal "converging" phase where the algorithms are filtering the outputs and the
> > > > transitions our smooth, and conversely the application can (and probably should)
> > > > display/consume these frames.
> > >
> > > Basically, yes. The startup phase is different. For some algorithms we
> > > indicate "converged" already, but that doesn't mean you shouldn't
> > > display "unconverged" frames - of course we do.

I don't get this.

> > > But during startup we
> > > can expect algorithms possibly to overshoot and the recommendation is
> > > simply not to use them.

That I agree with.

> > > I suppose it doesn't mean an application
> > > couldn't do something different - but we want to make the recommended
> > > behaviour easy.

Applications can easily differentiate the startup phase from the rest of
the camera operation, as startup occurs, by definition, at startup :-)
We could thus tell applications that they should ignore unconverged
frames immediately after starting the camera, but not later.

What I can't tell at the moment is whether the algorithm convergence is
the right criteria at start time. I can imagine that the convergence
phase could be split into a short initial part with large oscillations,
and a second part with smoother transitions until full convergence is
reached. Now that I've written this I of course expect someone to tell
that they absolutely need to differenciate between the two, but is it
needed for real ?

> > I'm pleased to see this metadata introduction. I already see visible
> > 'flashes' on the RkISP pipeline handler in my tests as the frames aren't
> > dropped there, nor are they reported to the applications as being
> > not-yet-ready for consumption.
> >
> > I know in the RPi pipeline handler it's well supported to capture a
> > single frame with pre-set manual controls. I guess we would expect
> > everything to be reported as converged in that instance, so it wouldn't
> > get marked as a StartupFrame? (Just trying to see if I can find odd
> > corner cases, but I expect this would already be fine).
> 
> If the user requests fully manual controls (i.e. shutter speed, analogue gain,
> manual red/blue gains), then we don't signal any drop frames for algorithm
> convergence.  However, we would still signal drop frames if the sensor is
> known to produce N garbage frames on startup.
> 
> > > > > The first and third criteria are quite vague. If I recall correctly, the
> > > > > first one includes bad frames from the sensor (as in completely
> > > > > corrupted frames, such as frames that are all black or made of random
> > > > > data). Those are completely unusable for applications, is there a value
> > > > > in still making them available instead of dropping them correctly ?
> > > >
> > > > This is what we do now (plus dropping the startup frames that may be good from
> > > > the sensor but still in the 3A startup phase), but I think a key goals of this
> > > > change is be consistent (i.e. let the application handle all frames always),

If the sensor produces frames that can't be used in any circumstance, I
really see no value in exposing them to applications. From an
application point of view this wouldn't be an inconsistent behaviour,
those frames would simply not exist.

> > > > and
> > > > also help with avoiding some framebuffer allocations to help with startup
> > > > convergence performance.

I'm not sure to get this.

> > > > > What are the other reasons a frame would be a "startup frame" ?
> > >
> > > There is a genuine discussion to have here, I think. There are some
> > > very clear reasons for telling an application not to use a frame - if
> > > it's complete garbage, for example. And there are some more "advisory"
> > > ones, which is what we can have for several frames. Categorising the
> > > "usability" of a frame is certainly an idea, though I don't know if we
> > > want to do that without a clear reason.
> >
> > I expect minimising latency to getting the first 'usable' frame to be a
> > priority on occasions here - so if applications can know some quantative
> > level of detail about 'how' usable the frame is, that could potetnially
> > reduce the number of frames an appliction might discard in some use
> > cases.
> >
> > I'm not sure how easy 'quantifying' the usability of the frame will be
> > though.

I can't imagine a way to do so at the moment, but I'd be happy to hear
proposals :-)

> > > But it's also very hard to predict the behaviour of all pipeline
> > > handlers in this respect. Perhaps someone has a pipeline handler where
> > > the first frame after every mode switch comes out badly,

We need to discuss how "mode switch" and "startup" interact. From the
point of view of the application, the libcamera API doesn't expose a
"mode switch" concept. We can stop, reconfigure and restart the camera,
and the documented behaviour doesn't differ from the first start. I know
how IPA modules can take advantage of information from the previous
capture session to accelerate algorithm convergence, and they should do
so as an internal optimization, but I don't think I want to expose this
concept explicitly. In particular, I don't like the "when the camera
system starts for the first time" in the documentation.

> > > perhaps it
> > > needs to consume a frame first for its IPAs to sort themselves out.
> > > But maybe another PH doesn't. So the idea is to have a portable way to
> > > indicate this kind of thing so that applications don't have to start
> > > guessing the behaviour underneath.
> > >
> > > Note that these "behaviours" can be quite complex. For us, it's
> > > completely different if you request fixed exposure and gain, and
> > > colour gains. But only slightly different, IIRC, if you don't give the
> > > colour gains. Forcing this kind of stuff into applications,
> > > particularly ones that expect to use different PHs, feels quite
> > > undesirable.

I'm not following here either, I don't see what you're forcing onto
applications here.

> > > Don't know if I've really added anything, but I hope it makes a bit of sense!
> > >
> > > > > > > +
> > > > > > > +        Applications are advised to avoid using these frames. Mostly, they will
> > > > > > > +        occur when the camera system starts for the first time, although,
> > > > > > > +        depending on the sensor and the implementation, they could occur at
> > > > > > > +        other times.
> > > > > > > +
> > > > > > > +        The value false indicates that this is a normal frame.
> >
> > Presumably an application can also 'assume' that a lack of this metadata
> > would also indicate 'false' ...
> >
> > > > > >
> > > > > > Just throwing it out there, but would it be useful if this control was an
> > > > > > integer with the count of startup frames left to handle? A value of 0, or the
> > > > > > absence of the control would indicate this is a "valid" frame.
> >
> > Ah yes - that's what I mean ;-)
> >
> > > > > >
> > > > > > > +
> > > > > > >    # ----------------------------------------------------------------------------
> > > > > > >    # Draft controls section
> > > > > > >

-- 
Regards,

Laurent Pinchart