| Message ID | 20201116164918.2055-1-david.plowman@raspberrypi.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi David, Thank you for your patch. Review to follow shortly, but for the series: Tested-by: Naushir Patuck <naush@raspberrypi.com> Regards, Naush On Mon, 16 Nov 2020 at 16:49, David Plowman <david.plowman@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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel >
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@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@raspberrypi.com > <mailto:naush@raspberrypi.com>> > > Regards, > Naush > > > On Mon, 16 Nov 2020 at 16:49, David Plowman > <david.plowman@raspberrypi.com <mailto:david.plowman@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@lists.libcamera.org > <mailto:libcamera-devel@lists.libcamera.org> > https://lists.libcamera.org/listinfo/libcamera-devel > > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel >
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(-)