[libcamera-devel] [PATCH 0/5] Transform implementation

[Laurent Pinchart]

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

-- 
Regards,

Laurent Pinchart