[libcamera-devel] [PATCH v3 0/6] Raspberry Pi AGC: initial frame drop count

[Laurent Pinchart]

Hi David,

On Tue, Dec 08, 2020 at 08:44:35PM +0000, David Plowman wrote:
> Hi everyone
> 
> Thanks for all the reviews, comments and discussion! I've lost track a
> little bit of which comments were in which thread, so perhaps I can
> address a few of the remaining issues here.

Thanks for the summary :-)

> 1. Hide/Mistrust frames at Startup/ModeSwitch
> 
> Yes, I see that some of these are the same now. I'm slightly inclined
> to keep all 4 variants for the moment as it seems to me they might be
> different, however, if it transpires that new cameras that come along
> still don't need them maybe we do some simplification then?

Sounds good to me. We plan to move this information to the libcamera
core, so at that point we'll propose a rename, if you don't beat us to
it.

> 2. Should GetConvergenceFrames be in the Algorithm class?
> 
> Again, I'm slightly inclined to avoid it just because there are only 2
> implementations of this, yet there are actually over a dozen
> "algorithms". But like all things, we keep an eye on it!

Sounds good to me.

> 3. "Hiding" vs. "Mistrusting"
> 
> Just to clarify the distinction here, "hiding" means "do not show this
> to the user because it will look unexpected in some
> way". "Mistrusting" means "do not allow algorithms to process this
> frame because it will cause them to wobble or do something bad".

I'd like to move away from "hiding". The CamHelper class should report
information about the camera sensor, not contain logic that dictates how
frames should be handled by the IPA and pipeline handler. It may very
well be a semantic difference only, and it doesn't have to be renamed
now, we can address that when moving this information to the libcamera
core.

> "Mistrusting" includes both statistics and metadata from the device. I

As statistics are computed from the frame by the ISP, incorrect
statistics imply an incorrect frame, right ? In the event that a sensor
would produce a first frame containing both bogus data and bogus
metadata, and a second frame containing bogus metadata only, do I
understand correctly that an algorithm that consumes statistics only
could run on the second frame already, while an algorithm that consumes
metadata would need to skip the first two frames ?

> suppose if the ISP statistics are bad then the frame may be bad too,
> so it would feature in the "hide" number already. It is possible that
> you could get frames that look OK but where the device is lying about
> its metadata, so there is a distinction between the two.
> 
> Yet perhaps this distinction never really happens in practice - so
> this should go on this list of things we keep an eye on with a view to
> simplification.
> 
> As regards the patches themselves, they are structured exactly as
> before. The main differences are:
> 
> 1. No change.
> 
> 2. and 3. GetConvergenceFrames has dropped the parameter (that's
> handled back in the IPA now, patch 6).
> 
> 4. Pwl::Inverse is a bit more careful about adding to its existing end
> points as it goes.
> 
> 5. No more std::move.
> 
> 6. The adding of the "mistrusted" frames is handled here now.
> 
> And I think that's it, please remind me if anything has slipped
> through the cracks!
>
> David Plowman (6):
>   src: raspberrypi: Pass the drop frame count in start, not configure
>   src: ipa: raspberrypi: agc: Add GetConvergenceFrames method to AGC
>     base class
>   src: ipa: raspberrypi: awb: Add GetConvergenceFrames method to AWB
>     base class
>   src: ipa: raspberrypi: Compute inverse of piecewise linear function
>   src: ipa: raspberrypi: Estimate the colour temerature if starting with
>     fixed colour gains
>   src: ipa: raspberrypi: Move initial frame drop decision to AGC/AWB

Minor style issue, we don't normally include the "src: " prefix.

It all looks good to me, so I've pushed the patches with the commit
subjects updated.

>  src/ipa/raspberrypi/cam_helper.cpp            |  6 +-
>  src/ipa/raspberrypi/cam_helper_ov5647.cpp     | 10 +++
>  .../raspberrypi/controller/agc_algorithm.hpp  |  1 +
>  .../raspberrypi/controller/awb_algorithm.hpp  |  1 +
>  src/ipa/raspberrypi/controller/pwl.cpp        | 30 ++++++++
>  src/ipa/raspberrypi/controller/pwl.hpp        |  3 +
>  src/ipa/raspberrypi/controller/rpi/agc.cpp    | 11 +++
>  src/ipa/raspberrypi/controller/rpi/agc.hpp    |  2 +
>  src/ipa/raspberrypi/controller/rpi/alsc.cpp   |  6 +-
>  src/ipa/raspberrypi/controller/rpi/awb.cpp    | 25 +++++++
>  src/ipa/raspberrypi/controller/rpi/awb.hpp    |  3 +
>  src/ipa/raspberrypi/raspberrypi.cpp           | 73 ++++++++++++++-----
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 25 ++++---
>  13 files changed, 160 insertions(+), 36 deletions(-)

-- 
Regards,

Laurent Pinchart