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

18855 diff mbox series

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@raspberrypi.com>
---
 src/ipa/rpi/common/ipa_base.cpp | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Hi David,

Thank you for this fix!

On Thu, 20 Jul 2023 at 13:48, David Plowman via libcamera-devel
<libcamera-devel@lists.libcamera.org> wrote:
>
> 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@raspberrypi.com>

Reviewed-by: Naushir Patuck <naush@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;
>                 libcameraMetadata_.set(controls::FocusFoM, focusFoM);
>         }
>
> --
> 2.30.2
>

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@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...

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

>                 libcameraMetadata_.set(controls::FocusFoM, focusFoM);
>         }
>  
> -- 
> 2.30.2
>

Hi Kieran

Thanks for the review!

On Thu, 27 Jul 2023 at 11:13, Kieran Bingham
<kieran.bingham@ideasonboard.com> 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@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!

Thanks
David

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

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@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@ideasonboard.com>

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