[libcamera-devel] [PATCH 1/4] libcamera: controls: Add focus Figure of Merit (FoM) control

[Laurent Pinchart]

Hi Naush,

On Fri, Jul 03, 2020 at 08:29:49AM +0100, Naushir Patuck wrote:
> On Fri, 3 Jul 2020 at 02:21, Laurent Pinchart wrote:
> > On Mon, Jun 29, 2020 at 11:41:45AM +0100, Naushir Patuck wrote:
> > > On Mon, 29 Jun 2020 at 01:15, Laurent Pinchart wrote:
> > > > On Fri, Jun 26, 2020 at 11:25:28AM +0100, Naushir Patuck wrote:
> > > > > Provide a control to allow the IPA to return a FoM to indicate how
> > > > > in-focus a scene is. Note, this is not to be used as a means to
> > > > > implement a focus algorithm by the application, rather an indication of
> > > > > how in-focus a scene is.
> > > > >
> > > > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > > > ---
> > > > >  src/libcamera/control_ids.yaml | 11 +++++++++++
> > > > >  1 file changed, 11 insertions(+)
> > > > >
> > > > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> > > > > index 8c3e4c72..b46693ec 100644
> > > > > --- a/src/libcamera/control_ids.yaml
> > > > > +++ b/src/libcamera/control_ids.yaml
> > > > > @@ -251,4 +251,15 @@ controls:
> > > > >          higher than anyone could reasonably want. Negative values are
> > > > >          not allowed. Note also that sharpening is not applied to raw
> > > > >          streams.
> > > > > +
> > > > > +  - FocusFoM:
> > > > > +      type: int32_t
> > > > > +      description: |
> > > > > +        Reports a Figure of Merit (FoM) to indicate how in-focus the frame is.
> > > > > +        A larger FocusFoM value indicates a more in-focus frame. This control
> > > > > +        depends on the IPA to gather ISP statistics from the defined focus
> > > > > +        region, and combine them in a suitable way to generate a FocusFoM value.
> > > > > +        In this respect, it is not necessarily aimed a providing a way to
> > > > > +        implement a focus algorithm by the application, rather an indication of
> > > > > +        how in-focus a frame is.
> > > >
> > > > Do you think this would be sufficient for applications, or would it be
> > > > better to use an array control that would return an array of values ?
> > > > Looking a 4/4 your ISP returns a grid of 4x3 values, so that could fit
> > > > in a control without a large performance impact.
> > >
> > > David and I did discuss this at length.  We concluded that a single
> > > value here was good because it simplified what the application wants
> > > to know from this control, i.e. how good is the focus in this scene.
> > > Anything more complicated, like a real focus algorithm would need
> > > access to all of the grid, and that can live happily in the IPAs.
> >
> > Agreed, auto-focus should live in the IPA.
> >
> > I'm curious, what are the use cases for applications, how would they
> > consume this value ?
> 
> The immediate use would be for the HQ camera with manual focus lenses.
> Relying on observing the viewfinder image is generally not good enough
> to determine the best focus position.  Instead, users can observe the
> FoM to get a numeric measure of the best focus position when manually
> focussing.  Of course, this is only really applicable if the scene is
> static and the camera is sat at a fixed position.

Do we need to display the focus value in qcam then ? :-)

> > Also, do you see any issue with the FocusFoM control as defined here
> > when used with sensors that produce PDAF data ? I have no particular
> > concern, but you have more experience than I do in this area.
> 
> Figure of Merit is a generic enough term that I think could encompass
> PDAF metrics as well.  Again, this is just a measure of "how good is
> the focus on this scene", and it is up to the IPAs to determine how to
> provide a numeric value to that.
> 
> > Another question, I assume that in most (all ?) case the focus
> > statistics are a measure of sharpness, and that two images with the same
> > focus quality but with a very different level of details in the scene
> > would have different focus FoM values, right ? I'd be tempted to name
> > the control to refer to sharpness instead of focus then, but we already
> > have a sharpness control that is completely unrelated. We can of course
> > always change names later, but I'd like your opinion here. Also, is my
> > understanding applicable to PDAF too, or would there be differences
> > there ?
> 
> Two scenes with the same focus quality but with a very different level
> of details would indeed produce different FoM values.  I did dither
> about calling it a sharpness or contrast measure, but that might be
> confusing as we have sharpness and contrast controls to set for the
> image.  And as you pointed out, PDAF won't necessarily provide a
> contrast/sharpness value.  Happy to change the control name if we can
> agree on something suitable :)

For what it's worth, Android calls this sharpness, and uses edge
enhancement for what we call sharpness. It doesn't seem bad, but we
don't have to follow that naming scheme.

As I just told David in another e-mail, I'm sure there will be a large
cleanup of controls before we stabilize the API, so we'll have an
occasion to rename controls to make everything consistent. Of course,
already picking a name that we think will stay is best.

> > > I am ok to extend this to an array to provide all 12 values, but if we
> > > do that, arguably we should add another control like FocusFomRegions
> > > that list the region coordinates for each FoM.  This will allow the
> > > control to be more generic for other HW vendors.
> >
> > Yes, I agree that would make sense.
> 
> If the consensus is to provide all 12 regions I can easily make the
> changes, and include the new FocusFomRegions control.

I'm fine going for a single value for now, we can reconsider this later
in the context of providing full statistics to the application, with
helpers to interpret them.

> > > > This leads to the next question of whether we should then report all
> > > > statistics in a buffer instead, and possibly even standardize the
> > > > statistics format. That could open pandora's box though, it's a rabbit
> > > > hole that could end up being pretty deep. I tried to look around for
> > > > other implementations, and there's support for histogram and focus
> > > > (sharpness) data in the Android camera HAL API, but marked as
> > > > "future". It seems to have been work in progress pretty much from the
> > > > start, without much interest in finalizing it. Looking at OpenKCam could
> > > > be interesting too, but there's no public spec I could find. Are you
> > > > aware of any other attempt at standardizing something in this area ?
> > >
> > > I recall we talked about standard formats for statistics a year (or
> > > so) ago and got very nervous about doing such a thing :)  I was
> > > briefly involved in the OpenKCam effort, and we did talk about exactly
> > > this.  If I recall, the consensus between hardware vendors was the
> > > same - we might spend too much effort (and add lots of complexity) to
> > > define a generic statistics format, which might never get used by
> > > users, and vendors never implement.
> >
> > I share the same concern. It may quickly become too complicated to
> > really be useful.
> >
> > > Part of me feels that is still the case with libcamera.  Vendors like
> > > Raspberry Pi do have the statistics buffer documented.  In the rare
> > > case a user does want statistics to do some magic stuff in the
> > > application, they can request the stats buffer (when the code is
> > > available), and resign to the fact that how they handle the statistics
> > > data will not be portable with other vendors.
> >
> > I think this makes sense. If we decide to standardize statistics, it
> > would probably be better implemented as a library that parses vendor
> > statistics formats, instead of implemented in the IPA.
> >
> > I however don't rule out, for some statistics, the possibility of having
> > the IPA reporting metadata about the statistics format, in order to ease
> > usage in applications, or even to allow coding sharing of algorithms
> > between different vendors. That's a topic for later though.
> >
> > > > >  ...

-- 
Regards,

Laurent Pinchart