[libcamera-devel] [PATCH 00/10] Raspberry Pi AGC

[Kieran Bingham]

Hi David,

This series is checkstyle clean, and builds cleanly in my compiler matrix.

All the patches are within src/ipa/raspberrypi - so with Naush's tags,
this is enough to go in as far as I can see.

It's hard to do a full review here, as I still have lots to learn on the
IPA details, and I won't let that block your work.

For the series:

Acked-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

There are some comments that I see have been mentioned, so when that's
handled, just let me know and we can push.

--
Kieran



On 17/11/2020 10:29, Naushir Patuck wrote:
> Hi David,
> 
> Thank you for your patch.  Review to follow shortly, but for the series:
> 
> Tested-by: Naushir Patuck <naush at raspberrypi.com
> <mailto:naush at raspberrypi.com>>
> 
> Regards,
> Naush
> 
> 
> On Mon, 16 Nov 2020 at 16:49, David Plowman
> <david.plowman at raspberrypi.com <mailto:david.plowman at raspberrypi.com>>
> wrote:
> 
>     Hi everyone
> 
>     Some time ago I had said I was doing some maintanence on the Raspberry
>     Pi AGC algorithm, so here it all is (finally). Some of the changes are
>     just for tidying, but others do also make it work better (even
>     correctly). Some of the changes are in particular designed to go with
>     Naush's patch set that allows exposure/gain to be set before the
>     camera starts, though they all function in their own right too.
> 
>     These are the last substantial set of changes I have before we can
>     publish our libcamera apps, though I think Naush still has more. (I am
>     doing work on some of the other algorithms, but these are not on the
>     critical path and can be added later.)
> 
>     Let me summarise all the individual patches here:
> 
>     #1. Replace Raspberry Pi debug with libcamera debug.
> 
>     #2. Code simplification. A number of copies and mutexes are
>      unnecessary on the libcamera/vc4 platform. Not really a functional
>      change though it does happen to fix a bug (where the AeLocked status
>      was never correctly passed out in the AgcStatus).
> 
>     #3. Function renamed to be clearer!
> 
>     #4. An improvement in the way average Y values are calculated. This
>      does actually change the AGC behaviour, making it less fiercely
>      centre-weighted.
> 
>     #5. Code that fetches the AwbStatus from the metadata is factored out,
>      so that we don't keep doing it all over the place. No functional
>      difference.
> 
>     #6. An update to the AWB so that it gives us its up-to-date values
>      when a camera mode switch happens. There's actually quite a lot of
>      tidying to do to the AWB, but I'd like to save that for when I have
>      more substantial changes there (and don't worry, they will come!).
> 
>     #7. This passes out correct gain/exposure values when the camera
>      changes mode (or starts). This is the one most important to Naush.
> 
>     #8. Clear up some uninitialised variables. This is Tomi's patch from a
>      little while back.
> 
>     #9. A small improvement to the gain update calculation. It's worth
>      working a little harder with the statistics that we have to save
>      having to wait for more frames.
> 
>     #10. Improvements to the "AE locked" logic. Previously it could fail
>      ever to "lock" in some circumstances; now it will always do so once
>      the AGC is steady.
> 
>     Sorry there's quite such a lot of stuff here. Please let me know if it
>     might be better broken up into a few smaller patch sets.
> 
>     Thanks and best regards
>     David
> 
>     David Plowman (10):
>       libcamera: ipa: raspberrypi: agc: Use libcamera debug
>       libcamera: ipa: raspberrypi: agc: Remove unnecessary locking
>       libcamera: ipa: raspberrypi: agc: Rename method to divideUpExposure
>       libcamera: ipa: raspberrypi: agc: Improve centre-weighted luminance
>         calucation
>       libcamera: ipa: raspberrypi: agc: Fetch AWB status only once
>       libcamera: ipa: raspberrypi: awb: Add SwitchMode method to output AWB
>         status
>       libcamera: ipa: raspberrypi: agc: Report fixed exposure/gain values
>         during SwitchMode
>       libcamera: src: ipa: raspberrypi: agc: Fix uninitialised members in
>         status_
>       libcamera: src: ipa: raspberrypi: agc: Improve gain update calculation
>         for partly saturated images
>       libcamera: src: ipa: raspberrypi: agc: Improve AE locked logic
> 
>      src/ipa/raspberrypi/controller/rpi/agc.cpp | 393 ++++++++++++---------
>      src/ipa/raspberrypi/controller/rpi/agc.hpp |  17 +-
>      src/ipa/raspberrypi/controller/rpi/awb.cpp |  14 +
>      src/ipa/raspberrypi/controller/rpi/awb.hpp |   1 +
>      4 files changed, 248 insertions(+), 177 deletions(-)
> 
>     -- 
>     2.20.1
> 
>     _______________________________________________
>     libcamera-devel mailing list
>     libcamera-devel at lists.libcamera.org
>     <mailto:libcamera-devel at lists.libcamera.org>
>     https://lists.libcamera.org/listinfo/libcamera-devel
> 
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
> 

-- 
Regards
--
Kieran