| Message ID | 20210122102211.12768-1-david.plowman@raspberrypi.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi David, On 22/01/2021 10:22, David Plowman wrote: > Hi everyone > > This patch set removes the old Raspberry Pi logging from all our > control algorithms and replaces it with libcamera logging. There is > literally nothing in this patch set except for the necessary macro > replacements and a few whitespace adjustments to keep the style > checker happy. > > This is actually quite an important change because, now that we're > about to publish libcamera versions of our legacy applications, it > makes it much easier to get debug information from our customers. Aha, great, indeed it will tie into the existing logging infrastructure nicely. > Perhaps the biggest question is whether to squish all the patches > together? I've not done this yet as it's easier to squish than to > un-squish, though I did roll up all the "minor" algorithms into a > single commit. But I'll happily do that if it's tidier! I don't think that matters too much here. Indeed we could squash 1-4, or split 4/5 further - but I don't think any of that effort is required here. It's interesting that you can now enabled/disable specific algorithm debug explictily. I wonder if sometime later we might want to be able to 'group' multiple debug categories to enable all IPA debug without enabling V4L2 debug for instance, or perhaps to have a way to 'disable' some categories. But that's speculation on future features - for this whole series Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Thanks and 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 | 29 +++--- > src/ipa/raspberrypi/controller/logging.hpp | 30 ------ > src/ipa/raspberrypi/controller/rpi/alsc.cpp | 59 ++++++------ > src/ipa/raspberrypi/controller/rpi/awb.cpp | 92 ++++++++++--------- > .../controller/rpi/black_level.cpp | 8 +- > src/ipa/raspberrypi/controller/rpi/ccm.cpp | 24 +++-- > .../raspberrypi/controller/rpi/contrast.cpp | 15 ++- > src/ipa/raspberrypi/controller/rpi/dpc.cpp | 8 +- > src/ipa/raspberrypi/controller/rpi/geq.cpp | 18 ++-- > src/ipa/raspberrypi/controller/rpi/lux.cpp | 12 ++- > src/ipa/raspberrypi/controller/rpi/noise.cpp | 14 ++- > src/ipa/raspberrypi/controller/rpi/sdn.cpp | 21 +++-- > .../raspberrypi/controller/rpi/sharpen.cpp | 8 +- > 14 files changed, 179 insertions(+), 160 deletions(-) > delete mode 100644 src/ipa/raspberrypi/controller/logging.hpp >
Hi David, Thank you for your patch. On Fri, 22 Jan 2021 at 10:22, David Plowman <david.plowman@raspberrypi.com> wrote: > Hi everyone > > This patch set removes the old Raspberry Pi logging from all our > control algorithms and replaces it with libcamera logging. There is > literally nothing in this patch set except for the necessary macro > replacements and a few whitespace adjustments to keep the style > checker happy. > > This is actually quite an important change because, now that we're > about to publish libcamera versions of our legacy applications, it > makes it much easier to get debug information from our customers. > > Perhaps the biggest question is whether to squish all the patches > together? I've not done this yet as it's easier to squish than to > un-squish, though I did roll up all the "minor" algorithms into a > single commit. But I'll happily do that if it's tidier! > > Thanks and best regards > David > Very useful! For the entire series: Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > > 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 | 29 +++--- > src/ipa/raspberrypi/controller/logging.hpp | 30 ------ > src/ipa/raspberrypi/controller/rpi/alsc.cpp | 59 ++++++------ > src/ipa/raspberrypi/controller/rpi/awb.cpp | 92 ++++++++++--------- > .../controller/rpi/black_level.cpp | 8 +- > src/ipa/raspberrypi/controller/rpi/ccm.cpp | 24 +++-- > .../raspberrypi/controller/rpi/contrast.cpp | 15 ++- > src/ipa/raspberrypi/controller/rpi/dpc.cpp | 8 +- > src/ipa/raspberrypi/controller/rpi/geq.cpp | 18 ++-- > src/ipa/raspberrypi/controller/rpi/lux.cpp | 12 ++- > src/ipa/raspberrypi/controller/rpi/noise.cpp | 14 ++- > src/ipa/raspberrypi/controller/rpi/sdn.cpp | 21 +++-- > .../raspberrypi/controller/rpi/sharpen.cpp | 8 +- > 14 files changed, 179 insertions(+), 160 deletions(-) > delete mode 100644 src/ipa/raspberrypi/controller/logging.hpp > > -- > 2.20.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel >
Hi David and Kieran, On Fri, Jan 22, 2021 at 11:31:58AM +0000, Kieran Bingham wrote: > On 22/01/2021 10:22, David Plowman wrote: > > This patch set removes the old Raspberry Pi logging from all our > > control algorithms and replaces it with libcamera logging. There is > > literally nothing in this patch set except for the necessary macro > > replacements and a few whitespace adjustments to keep the style > > checker happy. > > > > This is actually quite an important change because, now that we're > > about to publish libcamera versions of our legacy applications, it > > makes it much easier to get debug information from our customers. > > Aha, great, indeed it will tie into the existing logging infrastructure > nicely. > > > Perhaps the biggest question is whether to squish all the patches > > together? I've not done this yet as it's easier to squish than to > > un-squish, though I did roll up all the "minor" algorithms into a > > single commit. But I'll happily do that if it's tidier! > > I don't think that matters too much here. Indeed we could squash 1-4, or > split 4/5 further - but I don't think any of that effort is required here. Agreed, we can keep it as-is. It's certainly easier to review with 5 patches than with a single one. > It's interesting that you can now enabled/disable specific algorithm > debug explictily. I wonder if sometime later we might want to be able to > 'group' multiple debug categories to enable all IPA debug without > enabling V4L2 debug for instance, or perhaps to have a way to 'disable' > some categories. We can use a wildcard * at the end of a category name, so the feature is already here. > But that's speculation on future features - for this whole series > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > Thanks and 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 | 29 +++--- > > src/ipa/raspberrypi/controller/logging.hpp | 30 ------ > > src/ipa/raspberrypi/controller/rpi/alsc.cpp | 59 ++++++------ > > src/ipa/raspberrypi/controller/rpi/awb.cpp | 92 ++++++++++--------- > > .../controller/rpi/black_level.cpp | 8 +- > > src/ipa/raspberrypi/controller/rpi/ccm.cpp | 24 +++-- > > .../raspberrypi/controller/rpi/contrast.cpp | 15 ++- > > src/ipa/raspberrypi/controller/rpi/dpc.cpp | 8 +- > > src/ipa/raspberrypi/controller/rpi/geq.cpp | 18 ++-- > > src/ipa/raspberrypi/controller/rpi/lux.cpp | 12 ++- > > src/ipa/raspberrypi/controller/rpi/noise.cpp | 14 ++- > > src/ipa/raspberrypi/controller/rpi/sdn.cpp | 21 +++-- > > .../raspberrypi/controller/rpi/sharpen.cpp | 8 +- > > 14 files changed, 179 insertions(+), 160 deletions(-) > > delete mode 100644 src/ipa/raspberrypi/controller/logging.hpp
Hi Laurent Thanks for the review of all these patches. I guess I was just swapping out the macros, but I think your observations are fair enough: * Debug that is just tracing can probably just be removed. It is inherited from the platform where this code originally came from, and back then it was all new and I probably liked the reassurance about what was happening, especially as we were just processing single images. * Things like hierarchical categories sound nice, but I'm not sure it's that important for us. The most likely use case for us will be when some particular algorithm (e.g. AWB) is mis-behaviing and we want to see what that single algorithm is doing. * Yes, there seem to be some messages that are warnings but were not categorised initially with RPI_WARN. Not sure why, but they probably should be changed. As regards making these changes, would you fold them into the existing commits, or would you rather have them as additional changes (i.e. keep the macro-swapping separate from anything else)? For myself, I don't mind either way! Let me know what you think, and I can do a v2 set accordingly. Thanks again David On Sat, 23 Jan 2021 at 11:16, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi David and Kieran, > > On Fri, Jan 22, 2021 at 11:31:58AM +0000, Kieran Bingham wrote: > > On 22/01/2021 10:22, David Plowman wrote: > > > This patch set removes the old Raspberry Pi logging from all our > > > control algorithms and replaces it with libcamera logging. There is > > > literally nothing in this patch set except for the necessary macro > > > replacements and a few whitespace adjustments to keep the style > > > checker happy. > > > > > > This is actually quite an important change because, now that we're > > > about to publish libcamera versions of our legacy applications, it > > > makes it much easier to get debug information from our customers. > > > > Aha, great, indeed it will tie into the existing logging infrastructure > > nicely. > > > > > Perhaps the biggest question is whether to squish all the patches > > > together? I've not done this yet as it's easier to squish than to > > > un-squish, though I did roll up all the "minor" algorithms into a > > > single commit. But I'll happily do that if it's tidier! > > > > I don't think that matters too much here. Indeed we could squash 1-4, or > > split 4/5 further - but I don't think any of that effort is required here. > > Agreed, we can keep it as-is. It's certainly easier to review with 5 > patches than with a single one. > > > It's interesting that you can now enabled/disable specific algorithm > > debug explictily. I wonder if sometime later we might want to be able to > > 'group' multiple debug categories to enable all IPA debug without > > enabling V4L2 debug for instance, or perhaps to have a way to 'disable' > > some categories. > > We can use a wildcard * at the end of a category name, so the feature is > already here. > > > But that's speculation on future features - for this whole series > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > Thanks and 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 | 29 +++--- > > > src/ipa/raspberrypi/controller/logging.hpp | 30 ------ > > > src/ipa/raspberrypi/controller/rpi/alsc.cpp | 59 ++++++------ > > > src/ipa/raspberrypi/controller/rpi/awb.cpp | 92 ++++++++++--------- > > > .../controller/rpi/black_level.cpp | 8 +- > > > src/ipa/raspberrypi/controller/rpi/ccm.cpp | 24 +++-- > > > .../raspberrypi/controller/rpi/contrast.cpp | 15 ++- > > > src/ipa/raspberrypi/controller/rpi/dpc.cpp | 8 +- > > > src/ipa/raspberrypi/controller/rpi/geq.cpp | 18 ++-- > > > src/ipa/raspberrypi/controller/rpi/lux.cpp | 12 ++- > > > src/ipa/raspberrypi/controller/rpi/noise.cpp | 14 ++- > > > src/ipa/raspberrypi/controller/rpi/sdn.cpp | 21 +++-- > > > .../raspberrypi/controller/rpi/sharpen.cpp | 8 +- > > > 14 files changed, 179 insertions(+), 160 deletions(-) > > > delete mode 100644 src/ipa/raspberrypi/controller/logging.hpp > > -- > Regards, > > Laurent Pinchart
Hi David, On Mon, Jan 25, 2021 at 10:58:37AM +0000, David Plowman wrote: > Hi Laurent > > Thanks for the review of all these patches. > > I guess I was just swapping out the macros, but I think your > observations are fair enough: > > * Debug that is just tracing can probably just be removed. It is > inherited from the platform where this code originally came from, and > back then it was all new and I probably liked the reassurance about > what was happening, especially as we were just processing single > images. > > * Things like hierarchical categories sound nice, but I'm not sure > it's that important for us. The most likely use case for us will be > when some particular algorithm (e.g. AWB) is mis-behaviing and we want > to see what that single algorithm is doing. > > * Yes, there seem to be some messages that are warnings but were not > categorised initially with RPI_WARN. Not sure why, but they probably > should be changed. > > As regards making these changes, would you fold them into the existing > commits, or would you rather have them as additional changes (i.e. > keep the macro-swapping separate from anything else)? For myself, I > don't mind either way! Whatever is easier for you :-) Having separate changes produces a bit of a cleaner history, but I doubt anyone will need to read the history of these changes. > Let me know what you think, and I can do a v2 set accordingly. > > On Sat, 23 Jan 2021 at 11:16, Laurent Pinchart wrote: > > On Fri, Jan 22, 2021 at 11:31:58AM +0000, Kieran Bingham wrote: > > > On 22/01/2021 10:22, David Plowman wrote: > > > > This patch set removes the old Raspberry Pi logging from all our > > > > control algorithms and replaces it with libcamera logging. There is > > > > literally nothing in this patch set except for the necessary macro > > > > replacements and a few whitespace adjustments to keep the style > > > > checker happy. > > > > > > > > This is actually quite an important change because, now that we're > > > > about to publish libcamera versions of our legacy applications, it > > > > makes it much easier to get debug information from our customers. > > > > > > Aha, great, indeed it will tie into the existing logging infrastructure > > > nicely. > > > > > > > Perhaps the biggest question is whether to squish all the patches > > > > together? I've not done this yet as it's easier to squish than to > > > > un-squish, though I did roll up all the "minor" algorithms into a > > > > single commit. But I'll happily do that if it's tidier! > > > > > > I don't think that matters too much here. Indeed we could squash 1-4, or > > > split 4/5 further - but I don't think any of that effort is required here. > > > > Agreed, we can keep it as-is. It's certainly easier to review with 5 > > patches than with a single one. > > > > > It's interesting that you can now enabled/disable specific algorithm > > > debug explictily. I wonder if sometime later we might want to be able to > > > 'group' multiple debug categories to enable all IPA debug without > > > enabling V4L2 debug for instance, or perhaps to have a way to 'disable' > > > some categories. > > > > We can use a wildcard * at the end of a category name, so the feature is > > already here. > > > > > But that's speculation on future features - for this whole series > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > Thanks and 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 | 29 +++--- > > > > src/ipa/raspberrypi/controller/logging.hpp | 30 ------ > > > > src/ipa/raspberrypi/controller/rpi/alsc.cpp | 59 ++++++------ > > > > src/ipa/raspberrypi/controller/rpi/awb.cpp | 92 ++++++++++--------- > > > > .../controller/rpi/black_level.cpp | 8 +- > > > > src/ipa/raspberrypi/controller/rpi/ccm.cpp | 24 +++-- > > > > .../raspberrypi/controller/rpi/contrast.cpp | 15 ++- > > > > src/ipa/raspberrypi/controller/rpi/dpc.cpp | 8 +- > > > > src/ipa/raspberrypi/controller/rpi/geq.cpp | 18 ++-- > > > > src/ipa/raspberrypi/controller/rpi/lux.cpp | 12 ++- > > > > src/ipa/raspberrypi/controller/rpi/noise.cpp | 14 ++- > > > > src/ipa/raspberrypi/controller/rpi/sdn.cpp | 21 +++-- > > > > .../raspberrypi/controller/rpi/sharpen.cpp | 8 +- > > > > 14 files changed, 179 insertions(+), 160 deletions(-) > > > > delete mode 100644 src/ipa/raspberrypi/controller/logging.hpp
Hi everyone This patch set removes the old Raspberry Pi logging from all our control algorithms and replaces it with libcamera logging. There is literally nothing in this patch set except for the necessary macro replacements and a few whitespace adjustments to keep the style checker happy. This is actually quite an important change because, now that we're about to publish libcamera versions of our legacy applications, it makes it much easier to get debug information from our customers. Perhaps the biggest question is whether to squish all the patches together? I've not done this yet as it's easier to squish than to un-squish, though I did roll up all the "minor" algorithms into a single commit. But I'll happily do that if it's tidier! Thanks and 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 | 29 +++--- src/ipa/raspberrypi/controller/logging.hpp | 30 ------ src/ipa/raspberrypi/controller/rpi/alsc.cpp | 59 ++++++------ src/ipa/raspberrypi/controller/rpi/awb.cpp | 92 ++++++++++--------- .../controller/rpi/black_level.cpp | 8 +- src/ipa/raspberrypi/controller/rpi/ccm.cpp | 24 +++-- .../raspberrypi/controller/rpi/contrast.cpp | 15 ++- src/ipa/raspberrypi/controller/rpi/dpc.cpp | 8 +- src/ipa/raspberrypi/controller/rpi/geq.cpp | 18 ++-- src/ipa/raspberrypi/controller/rpi/lux.cpp | 12 ++- src/ipa/raspberrypi/controller/rpi/noise.cpp | 14 ++- src/ipa/raspberrypi/controller/rpi/sdn.cpp | 21 +++-- .../raspberrypi/controller/rpi/sharpen.cpp | 8 +- 14 files changed, 179 insertions(+), 160 deletions(-) delete mode 100644 src/ipa/raspberrypi/controller/logging.hpp