[libcamera-devel] [PATCH] ipa: rpi: Fix the reporting of Focus FoMs

[Laurent Pinchart]

Hi David,

On Thu, Jul 27, 2023 at 11:23:37AM +0100, David Plowman via libcamera-devel wrote:
> On Thu, 27 Jul 2023 at 11:13, Kieran Bingham wrote:
> > Quoting David Plowman via libcamera-devel (2023-07-20 13:48:23)
> > > The FocusFom metadata was no longer being reported back because the
> > > "focus.status" metadata was never being created.
> > >
> > > Additionally, the scaling of the focus FoMs was over-zealous, rounding
> > > just about everything down to zero.
> > >
> > > Fixes: ac7511dc4c59 ("ipa: raspberrypi: Generalise the focus reporting code")
> > > Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> > > ---
> > >  src/ipa/rpi/common/ipa_base.cpp | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
> > > index f40f2e71..2a033264 100644
> > > --- a/src/ipa/rpi/common/ipa_base.cpp
> > > +++ b/src/ipa/rpi/common/ipa_base.cpp
> > > @@ -458,6 +458,9 @@ void IpaBase::processStats(const ProcessParams &params)
> > >
> > >                 RPiController::StatisticsPtr statistics = platformProcessStats(it->second.planes()[0]);
> > >
> > > +               /* reportMetadata() will pick this up and set the FocusFoM metadata */
> > > +               rpiMetadata.set("focus.status", statistics->focusRegions);
> > > +
> > >                 helper_->process(statistics, rpiMetadata);
> > >                 controller_.process(statistics, &rpiMetadata);
> > >
> > > @@ -1197,7 +1200,7 @@ void IpaBase::reportMetadata(unsigned int ipaContext)
> > >                         }
> > >                 }
> > >
> > > -               uint32_t focusFoM = (sum / numRegions) >> 16;
> > > +               uint32_t focusFoM = sum / numRegions;
> >
> > There's no specific units for this control anyway is there, so this
> > still seems reasonable. I wonder if we should have platforms scale the
> > values to a defined range somehow - but I don't know what that range
> > would be or if it would make much difference anyway.
> >
> > But having a usable value is better than not so...
> 
> Yes, it is an utterly random unitless number that comes out of the
> hardware. It's basically the accumulated energy in the response of
> some arbitrary sharpening filter, I think.
> 
> The most typical use case is to watch it while you twiddle your manual
> focus lens, for which it's fine!

Having no unit isn't an issue, but it would be useful to either report
the camera-specific minimum and maximum values, or scale the number to a
range set in the control documentation. Could this be done ?

This isn't a blocker for this patch, so

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >
> > >                 libcameraMetadata_.set(controls::FocusFoM, focusFoM);
> > >         }
> > >

-- 
Regards,

Laurent Pinchart