| Message ID | 20210125184858.16339-1-david.plowman@raspberrypi.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi David, On Mon, Jan 25, 2021 at 06:48:53PM +0000, David Plowman wrote: > Hi again everyone > > Version 2 of this set again makes no functional changes beyond debug > logging. There are the following differences compared to v1: > > * I've removed some debug messages that were unnecessary (either just > tracing statements or duplicates). > > * A number of messages repeated the algorithm name which is > unnecessary with libcamera logging. > > * One or two that were actually warnings without using LOG(RPiXxx, > Warning) have been amended. > > * A bit more whitespace tidying. > > I've actually folded the changes into the original commits, I think > it's easy enough to follow like this. > > Most of Laurent's other suggestions I'd be fine with if we wanted to > pursue them (except the one about sharing frame counts between > algorithms - I'm actually really paranoid about that!). I'm not challenging your opinion, but for my information, could you briefly explain why ? > David Plowman (5): > ipa: raspberrypi: controller: Replace Raspberry Pi debug with > libcamera debug > ipa: raspberrypi: alsc: Replace Raspberry Pi debug with libcamera > debug > ipa: raspberrypi: awb: Replace Raspberry Pi debug with libcamera debug > ipa: raspberrypi: Replace Raspberry Pi debug with libcamera debug > ipa: raspberrypi: Remove legacy Rasberry Pi logging > > src/ipa/raspberrypi/controller/algorithm.hpp | 1 - > src/ipa/raspberrypi/controller/controller.cpp | 19 ++-- > src/ipa/raspberrypi/controller/logging.hpp | 30 ------ > src/ipa/raspberrypi/controller/rpi/alsc.cpp | 66 +++++++------ > src/ipa/raspberrypi/controller/rpi/awb.cpp | 97 ++++++++++--------- > .../controller/rpi/black_level.cpp | 11 ++- > src/ipa/raspberrypi/controller/rpi/ccm.cpp | 26 +++-- > .../raspberrypi/controller/rpi/contrast.cpp | 20 +++- > src/ipa/raspberrypi/controller/rpi/dpc.cpp | 8 +- > src/ipa/raspberrypi/controller/rpi/geq.cpp | 20 ++-- > src/ipa/raspberrypi/controller/rpi/lux.cpp | 11 ++- > src/ipa/raspberrypi/controller/rpi/noise.cpp | 14 ++- > src/ipa/raspberrypi/controller/rpi/sdn.cpp | 21 ++-- > .../raspberrypi/controller/rpi/sharpen.cpp | 11 ++- > 14 files changed, 192 insertions(+), 163 deletions(-) > delete mode 100644 src/ipa/raspberrypi/controller/logging.hpp
Hi Laurent No, that's fine, I'm quite happy to explain, just don't expect it to be 100% rational! On Tue, 26 Jan 2021 at 08:28, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi David, > > On Mon, Jan 25, 2021 at 06:48:53PM +0000, David Plowman wrote: > > Hi again everyone > > > > Version 2 of this set again makes no functional changes beyond debug > > logging. There are the following differences compared to v1: > > > > * I've removed some debug messages that were unnecessary (either just > > tracing statements or duplicates). > > > > * A number of messages repeated the algorithm name which is > > unnecessary with libcamera logging. > > > > * One or two that were actually warnings without using LOG(RPiXxx, > > Warning) have been amended. > > > > * A bit more whitespace tidying. > > > > I've actually folded the changes into the original commits, I think > > it's easy enough to follow like this. > > > > Most of Laurent's other suggestions I'd be fine with if we wanted to > > pursue them (except the one about sharing frame counts between > > algorithms - I'm actually really paranoid about that!). > > I'm not challenging your opinion, but for my information, could you > briefly explain why ? The main thing I'm scared of is starting to develop "tentacles" that make the different algorithms depend on each other, or on anything external (the only exception to this is that one algorithm is allowed to ask for another's "status" object from the metadata, but that's it). I'm desperately keen to try and keep them all as self-contained as I can. You can probably tell that I've been here before in a previous life, and the mess that it grew into back then ended up very difficult. There are some other minor issues about the frame counts perhaps not always being quite the same (some algorithms might be skipped just after the camera is started), but the real reason is the one above. Like I said, it's not entirely rational! Best regards David > > > David Plowman (5): > > ipa: raspberrypi: controller: Replace Raspberry Pi debug with > > libcamera debug > > ipa: raspberrypi: alsc: Replace Raspberry Pi debug with libcamera > > debug > > ipa: raspberrypi: awb: Replace Raspberry Pi debug with libcamera debug > > ipa: raspberrypi: Replace Raspberry Pi debug with libcamera debug > > ipa: raspberrypi: Remove legacy Rasberry Pi logging > > > > src/ipa/raspberrypi/controller/algorithm.hpp | 1 - > > src/ipa/raspberrypi/controller/controller.cpp | 19 ++-- > > src/ipa/raspberrypi/controller/logging.hpp | 30 ------ > > src/ipa/raspberrypi/controller/rpi/alsc.cpp | 66 +++++++------ > > src/ipa/raspberrypi/controller/rpi/awb.cpp | 97 ++++++++++--------- > > .../controller/rpi/black_level.cpp | 11 ++- > > src/ipa/raspberrypi/controller/rpi/ccm.cpp | 26 +++-- > > .../raspberrypi/controller/rpi/contrast.cpp | 20 +++- > > src/ipa/raspberrypi/controller/rpi/dpc.cpp | 8 +- > > src/ipa/raspberrypi/controller/rpi/geq.cpp | 20 ++-- > > src/ipa/raspberrypi/controller/rpi/lux.cpp | 11 ++- > > src/ipa/raspberrypi/controller/rpi/noise.cpp | 14 ++- > > src/ipa/raspberrypi/controller/rpi/sdn.cpp | 21 ++-- > > .../raspberrypi/controller/rpi/sharpen.cpp | 11 ++- > > 14 files changed, 192 insertions(+), 163 deletions(-) > > delete mode 100644 src/ipa/raspberrypi/controller/logging.hpp > > -- > Regards, > > Laurent Pinchart
Hi David, On Tue, Jan 26, 2021 at 08:57:49AM +0000, David Plowman wrote: > Hi Laurent > > No, that's fine, I'm quite happy to explain, just don't expect it to > be 100% rational! > > On Tue, 26 Jan 2021 at 08:28, Laurent Pinchart wrote: > > On Mon, Jan 25, 2021 at 06:48:53PM +0000, David Plowman wrote: > > > Hi again everyone > > > > > > Version 2 of this set again makes no functional changes beyond debug > > > logging. There are the following differences compared to v1: > > > > > > * I've removed some debug messages that were unnecessary (either just > > > tracing statements or duplicates). > > > > > > * A number of messages repeated the algorithm name which is > > > unnecessary with libcamera logging. > > > > > > * One or two that were actually warnings without using LOG(RPiXxx, > > > Warning) have been amended. > > > > > > * A bit more whitespace tidying. > > > > > > I've actually folded the changes into the original commits, I think > > > it's easy enough to follow like this. > > > > > > Most of Laurent's other suggestions I'd be fine with if we wanted to > > > pursue them (except the one about sharing frame counts between > > > algorithms - I'm actually really paranoid about that!). > > > > I'm not challenging your opinion, but for my information, could you > > briefly explain why ? > > The main thing I'm scared of is starting to develop "tentacles" that > make the different algorithms depend on each other, or on anything > external (the only exception to this is that one algorithm is allowed > to ask for another's "status" object from the metadata, but that's > it). I'm desperately keen to try and keep them all as self-contained > as I can. You can probably tell that I've been here before in a > previous life, and the mess that it grew into back then ended up very > difficult. I can relate to that feeling, so this is perfectly understandable :-) > There are some other minor issues about the frame counts perhaps not > always being quite the same (some algorithms might be skipped just > after the camera is started), but the real reason is the one above. > Like I said, it's not entirely rational! I'm wondering if it would make sense for "services" such as frame count to be handled in the base Algorithm class, to avoid code duplication. Algorithms would still be independent from each other (they would all have their own frame count) but wouldn't be required to duplicate the code. It's really minor for the frame count, but could be useful for other features. In any case it's a topic for later. > > > David Plowman (5): > > > ipa: raspberrypi: controller: Replace Raspberry Pi debug with > > > libcamera debug > > > ipa: raspberrypi: alsc: Replace Raspberry Pi debug with libcamera > > > debug > > > ipa: raspberrypi: awb: Replace Raspberry Pi debug with libcamera debug > > > ipa: raspberrypi: Replace Raspberry Pi debug with libcamera debug > > > ipa: raspberrypi: Remove legacy Rasberry Pi logging > > > > > > src/ipa/raspberrypi/controller/algorithm.hpp | 1 - > > > src/ipa/raspberrypi/controller/controller.cpp | 19 ++-- > > > src/ipa/raspberrypi/controller/logging.hpp | 30 ------ > > > src/ipa/raspberrypi/controller/rpi/alsc.cpp | 66 +++++++------ > > > src/ipa/raspberrypi/controller/rpi/awb.cpp | 97 ++++++++++--------- > > > .../controller/rpi/black_level.cpp | 11 ++- > > > src/ipa/raspberrypi/controller/rpi/ccm.cpp | 26 +++-- > > > .../raspberrypi/controller/rpi/contrast.cpp | 20 +++- > > > src/ipa/raspberrypi/controller/rpi/dpc.cpp | 8 +- > > > src/ipa/raspberrypi/controller/rpi/geq.cpp | 20 ++-- > > > src/ipa/raspberrypi/controller/rpi/lux.cpp | 11 ++- > > > src/ipa/raspberrypi/controller/rpi/noise.cpp | 14 ++- > > > src/ipa/raspberrypi/controller/rpi/sdn.cpp | 21 ++-- > > > .../raspberrypi/controller/rpi/sharpen.cpp | 11 ++- > > > 14 files changed, 192 insertions(+), 163 deletions(-) > > > delete mode 100644 src/ipa/raspberrypi/controller/logging.hpp
Hi again everyone Version 2 of this set again makes no functional changes beyond debug logging. There are the following differences compared to v1: * I've removed some debug messages that were unnecessary (either just tracing statements or duplicates). * A number of messages repeated the algorithm name which is unnecessary with libcamera logging. * One or two that were actually warnings without using LOG(RPiXxx, Warning) have been amended. * A bit more whitespace tidying. I've actually folded the changes into the original commits, I think it's easy enough to follow like this. Most of Laurent's other suggestions I'd be fine with if we wanted to pursue them (except the one about sharing frame counts between algorithms - I'm actually really paranoid about that!). Thanks! David David Plowman (5): ipa: raspberrypi: controller: Replace Raspberry Pi debug with libcamera debug ipa: raspberrypi: alsc: Replace Raspberry Pi debug with libcamera debug ipa: raspberrypi: awb: Replace Raspberry Pi debug with libcamera debug ipa: raspberrypi: Replace Raspberry Pi debug with libcamera debug ipa: raspberrypi: Remove legacy Rasberry Pi logging src/ipa/raspberrypi/controller/algorithm.hpp | 1 - src/ipa/raspberrypi/controller/controller.cpp | 19 ++-- src/ipa/raspberrypi/controller/logging.hpp | 30 ------ src/ipa/raspberrypi/controller/rpi/alsc.cpp | 66 +++++++------ src/ipa/raspberrypi/controller/rpi/awb.cpp | 97 ++++++++++--------- .../controller/rpi/black_level.cpp | 11 ++- src/ipa/raspberrypi/controller/rpi/ccm.cpp | 26 +++-- .../raspberrypi/controller/rpi/contrast.cpp | 20 +++- src/ipa/raspberrypi/controller/rpi/dpc.cpp | 8 +- src/ipa/raspberrypi/controller/rpi/geq.cpp | 20 ++-- src/ipa/raspberrypi/controller/rpi/lux.cpp | 11 ++- src/ipa/raspberrypi/controller/rpi/noise.cpp | 14 ++- src/ipa/raspberrypi/controller/rpi/sdn.cpp | 21 ++-- .../raspberrypi/controller/rpi/sharpen.cpp | 11 ++- 14 files changed, 192 insertions(+), 163 deletions(-) delete mode 100644 src/ipa/raspberrypi/controller/logging.hpp