| Message ID | 20200729093154.12296-1-david.plowman@raspberrypi.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi David, On Wed, Jul 29, 2020 at 10:31:49AM +0100, David Plowman wrote: > Hi everyone > > This patch set isn't complete, however, I thought it was worth giving > people the chance to have a look, and this also allows me to ask a few > more questions! > > There are 5 patches. > > 1. The first one merely adds the Transform class and inserts a > Transform field into the CameraConfiguration. This is the only > non-Raspberry Pi code. > > 2. Handles the transform in the RPi pipeline handler so that the > correct flips are applied to the camera. Also passes the transform to > the IPA. > > 3. Plumbs the transform through to all the separate control algorithms > via the SwitchMode method (but does nothing further to handle the > transform). > > 4. Handles the transform correctly in ALSC. > > 5. As part of patch 4, I noticed it wasn't dealing correctly with the > luminance tables, irrespective of the transform. This fixes that and > actually improves the handling of camera mode switches in general. > > Comments/Questions: > > * In patch #1, transform.h is labelled as "Copyright Google". Or > should I put Raspberry Pi? Does it matter? You're the author of the code, why should the copyright be assigned to Google ? :-) I expect copyright Raspberry Pi Trading. > * Patch #1 needs documentation. It looks like I need to write a > transform.cpp file in the manner of (for example) geometry.cpp. Is > that right, or is there anything else? (Do we mind if this file has > nothing but comments?) How would I test the generated documentation > is correct? That's correct. We have other .cpp files that only have comments, so it's fine, but there are a couple of functions that are a bit larger and would make sense to move to the .cpp file. I'll comment on that specifically when reviewing 1/5. > * Patch #2 has an ugly cast... I'll comment on that separately too :-) > I think that's everything for now. Suggestions welcome as ever...! > > David Plowman (5): > libcamera: Add Transform class > libcamera: raspberrypi: Apply transform and pass through to IPA > libcamera: raspberrypi: Plumb user transform through to individual > IPAs > libcamera: raspberrypi: ALSC: Handle transform in colour tables > libcamera: raspberrypi: ALSC: Fix crop/transform of luminance table > > include/libcamera/camera.h | 3 + > include/libcamera/ipa/raspberrypi.h | 1 + > include/libcamera/meson.build | 1 + > include/libcamera/transform.h | 193 ++++++++++++++++++ > src/ipa/raspberrypi/controller/algorithm.cpp | 4 +- > src/ipa/raspberrypi/controller/algorithm.hpp | 3 +- > src/ipa/raspberrypi/controller/controller.cpp | 5 +- > src/ipa/raspberrypi/controller/controller.hpp | 5 +- > src/ipa/raspberrypi/controller/rpi/agc.cpp | 3 +- > src/ipa/raspberrypi/controller/rpi/agc.hpp | 3 +- > src/ipa/raspberrypi/controller/rpi/alsc.cpp | 108 +++++++--- > src/ipa/raspberrypi/controller/rpi/alsc.hpp | 9 +- > src/ipa/raspberrypi/controller/rpi/noise.cpp | 4 +- > src/ipa/raspberrypi/controller/rpi/noise.hpp | 3 +- > .../raspberrypi/controller/rpi/sharpen.cpp | 3 +- > .../raspberrypi/controller/rpi/sharpen.hpp | 3 +- > src/ipa/raspberrypi/raspberrypi.cpp | 44 ++-- > .../pipeline/raspberrypi/raspberrypi.cpp | 27 ++- > 18 files changed, 356 insertions(+), 66 deletions(-) > create mode 100644 include/libcamera/transform.h
Hi everyone This patch set isn't complete, however, I thought it was worth giving people the chance to have a look, and this also allows me to ask a few more questions! There are 5 patches. 1. The first one merely adds the Transform class and inserts a Transform field into the CameraConfiguration. This is the only non-Raspberry Pi code. 2. Handles the transform in the RPi pipeline handler so that the correct flips are applied to the camera. Also passes the transform to the IPA. 3. Plumbs the transform through to all the separate control algorithms via the SwitchMode method (but does nothing further to handle the transform). 4. Handles the transform correctly in ALSC. 5. As part of patch 4, I noticed it wasn't dealing correctly with the luminance tables, irrespective of the transform. This fixes that and actually improves the handling of camera mode switches in general. Comments/Questions: * In patch #1, transform.h is labelled as "Copyright Google". Or should I put Raspberry Pi? Does it matter? * Patch #1 needs documentation. It looks like I need to write a transform.cpp file in the manner of (for example) geometry.cpp. Is that right, or is there anything else? (Do we mind if this file has nothing but comments?) How would I test the generated documentation is correct? * Patch #2 has an ugly cast... I think that's everything for now. Suggestions welcome as ever...! Best regards David David Plowman (5): libcamera: Add Transform class libcamera: raspberrypi: Apply transform and pass through to IPA libcamera: raspberrypi: Plumb user transform through to individual IPAs libcamera: raspberrypi: ALSC: Handle transform in colour tables libcamera: raspberrypi: ALSC: Fix crop/transform of luminance table include/libcamera/camera.h | 3 + include/libcamera/ipa/raspberrypi.h | 1 + include/libcamera/meson.build | 1 + include/libcamera/transform.h | 193 ++++++++++++++++++ src/ipa/raspberrypi/controller/algorithm.cpp | 4 +- src/ipa/raspberrypi/controller/algorithm.hpp | 3 +- src/ipa/raspberrypi/controller/controller.cpp | 5 +- src/ipa/raspberrypi/controller/controller.hpp | 5 +- src/ipa/raspberrypi/controller/rpi/agc.cpp | 3 +- src/ipa/raspberrypi/controller/rpi/agc.hpp | 3 +- src/ipa/raspberrypi/controller/rpi/alsc.cpp | 108 +++++++--- src/ipa/raspberrypi/controller/rpi/alsc.hpp | 9 +- src/ipa/raspberrypi/controller/rpi/noise.cpp | 4 +- src/ipa/raspberrypi/controller/rpi/noise.hpp | 3 +- .../raspberrypi/controller/rpi/sharpen.cpp | 3 +- .../raspberrypi/controller/rpi/sharpen.hpp | 3 +- src/ipa/raspberrypi/raspberrypi.cpp | 44 ++-- .../pipeline/raspberrypi/raspberrypi.cpp | 27 ++- 18 files changed, 356 insertions(+), 66 deletions(-) create mode 100644 include/libcamera/transform.h