[libcamera-devel] [PATCH v4 0/7] 2D transforms

[Kieran Bingham]

Hi David,

On 28/08/2020 15:41, David Plowman wrote:
> Hi everyone
> 
> Here's the latest version of 2D transforms. It's practically identical
> to the previous version, except for the (mostly documentation)
> improvements that were suggested, and the following points noted
> below.
> 
> Firstly, there are 2 new commits at the start of this set. The first
> of these reverts the commit 1e8c91b65695449c5246d17ba7dc439c8058b781
> that Kieran kindly pushed for us yesterday. Unfortunately that patch,
> whilst fine when Naush originally authored it, made it impossible to
> implement user-supplied transforms. Had it not been lurking in a
> rather forgotten state for so long I might have woken up to its
> problems sooner, so apologies for that. Anyway, I'm not quite sure if
> you have any conventions for exactly how to revert a patch.

Oh my :-( I'm sorry - That's a pain.

I think the revert commit you've sent is the right way to do this.


> The second of these new patches moves the change made by the now
> reverted patch into the validate() method. This is early enough to fix
> the original bug (which was that raw streams were advertising an
> incorrect Bayer format), but co-exists happily with the transform
> code. (Would these two be better squished together?)

Keeping them separate is fine, if you want to keep the intent clear.
Otherwise you could move it directly too - but I don't think it's a
problem either way.

> Beyond that the only issues to bring up are in what is now the fifth
> commit of this series (third in the previous version). These relate to
> that email I sent a few days back. Specifically:
> 
> * I assume the intention is, when no user-supplied transform is given,
>   that we give out images that are corrected for any camera
>   rotation. That is, the image, when viewed without any supplementary
>   manipulations, should look like the world did when the picture was
>   taken. Is this correct?

That would be my expectation.

> * So if a camera advertises itself as having a 90 degree rotation, do
>   I need to perform a 90 degree clockwise rotation to the image to
>   correct it, or counter-clockwise? I read the documentation on the
>   rotation property but must confess I was left a little hazy! (The
>   consequence would be that the rotation angle may need negating.)

I think if a camera advertises itself as rotated, we need to either:

a) Report that rotation if we don't make any change:
b) Change it - and make sure the 'new' rotation is reported?

Sometimes b might not be possible, so you would have to do a) as a
minimum, (I think?)

> * I've added a comment to say why I only flip the transpose bit when
>   the combined transform is one we don't support. It makes life easier
>   for the application because it either gets the transform it
>   requested, or it only has to do a simple transpose. All the other
>   possible transforms an application might worry about having to do
>   just can't happen. Does that make sense?

Not having to worry about things that can't happen makes sense ;-)

Hopefully the code will explain the rest.

> 
> I think that's everything!
> 
> Thanks and best regards
> David
> 
> David Plowman (7):
>   libcamera: pipeline: raspberrypi: Revert "Set sensor default
>     orientation before configure()"
>   libcamera: pipeline: raspberrypi: Set sensor orientation during
>     validate
>   libcamera: Add Transform enum to represet 2D plane transforms.
>   libcamera: Add user Transform to CameraConfiguration
>   libcamera: raspberrypi: Set camera flips correctly from user transform
>   libcamera: raspberrypi: Plumb user transform through to IPA
>   libcamera: ipa: raspberrypi: ALSC: Handle user transform
> 
>  include/libcamera/camera.h                    |   3 +
>  include/libcamera/meson.build                 |   1 +
>  include/libcamera/transform.h                 |  73 ++++
>  src/ipa/raspberrypi/controller/camera_mode.h  |   4 +
>  src/ipa/raspberrypi/controller/rpi/alsc.cpp   |  13 +-
>  src/ipa/raspberrypi/raspberrypi.cpp           |  48 +--
>  src/libcamera/camera.cpp                      |  16 +-
>  src/libcamera/meson.build                     |   1 +
>  src/libcamera/pipeline/ipu3/ipu3.cpp          |   5 +
>  .../pipeline/raspberrypi/raspberrypi.cpp      |  60 +++-
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp      |   5 +
>  src/libcamera/pipeline/simple/simple.cpp      |   5 +
>  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp  |   5 +
>  src/libcamera/pipeline/vimc/vimc.cpp          |   5 +
>  src/libcamera/transform.cpp                   | 312 ++++++++++++++++++
>  15 files changed, 522 insertions(+), 34 deletions(-)
>  create mode 100644 include/libcamera/transform.h
>  create mode 100644 src/libcamera/transform.cpp
> 

-- 
Regards
--
Kieran