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

9415 mbox series

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.

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?)

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?

* 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'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?

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

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
>