[libcamera-devel] [PATCH v7 0/8] 2D transforms

[David Plowman]

Hi Laurent

Thanks very much for the review. I'll send out a v8 version tomorrow
with those various comments incorporated. There were just a couple of
slight changes I made.

Firstly if you recall this little snippet:

> +     /*
> +      * We also check if the sensor doesn't do h/vflips at all, in which
> +      * case we clear them, and the application will have to do everything.
> +      */
> +     if (!data_->supportsFlips_ && !!combined) {
> +             transform &= Transform::Transpose;
> +             combined = Transform::Identity;
> +             status = Adjusted;
> +     }

I think you're right that I got it slightly wrong, though I think it's
a bit subtle. The camera is set to implement the "combined" transform,
so if it can't do any transforms then the only value for combined that
it can perform correctly is the identity. If combined is something
else then we must change it to the identity, and the configuration is
therefore "adjusted". We set the user transform to be the transform
that gives a combined transform of the identity, which must therefore
be the inverse of the rotation (recall combined = user transform *
rotation).

For example, in the case of the imx477 the rotation is 180 degrees. If
the camera couldn't do any flips (this is hypothetical) and the
application asked for an identity transform, then combined would be
rot180 and would be coerced to the identity. The configuration would
be returned as "adjusted", and the application would discover its
transform had been changed to rot180 - informing it that it will get
its images upside down compared to what it wanted. I think that makes
sense. Anyway, I've fixed that and put in yet more comments.
Transforms are always so head-scratching...

The other little detail was the effect of transposition on the BayerFormat.

If a camera ever did transposition you'd have to know where it comes
relative to the flips, otherwise the Bayer order will come out wrong -
flips and transpose don't commute, of course. That does complicate
things somewhat, and I'm not really sure how you'd handle such a
thing. So for now I've continued to ignore transpose, but added some
words to that effect - I hope that seems reasonable!

Thanks again
David

On Sun, 6 Sep 2020 at 01:45, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi David,
>
> On Fri, Sep 04, 2020 at 11:30:34AM +0100, David Plowman wrote:
> > Hi everyone
> >
> > Thanks for the various comments from Kieran since last time. The
> > latest version I think incorporates that feedback - I moved the
> > combinedTransform_ field into the RPiCameraConfiguration, and removed
> > that extra one I had called userTransform_ because it's the same as
> > the field already named transform. Apart from that everything else is
> > unchanged, except for maybe a couple of improved comments. So still 8
> > separate commits as last time:
> >
> > 1. The revert commit - unchanged.
> >
> > 2. V4L2Device::controlInfo method - unchanged.
> >
> > 3. Transform enum - unchanged.
> >
> > 4. BayerFormat class - unchanged apart from cosmetic tidying as per
> > feedback.
> >
> > 5. Add transform to CameraConfiguration - unchanged.
> >
> > 6. This is where combinedTransform_ moves into the
> > RPiCameraConfiguration, and userTransform_ is deleted (being just a
> > duplicate of the transform field).
> >
> > 7 and 8. Unchanged.
> >
> > I think that's it!
>
> Nice work, it's nice to see pieces falling into place. The BayerFormat
> class is a good addition, I think we'll end up extending it to support
> PixelFormat in addition to V4L2PixelFormat.
>
> The patches are mostly OK. I thought I could handle the last few fixups
> myself when applying (after getting your feedback on a few questions),
> but patch 4/8 needs a bit more refactoring, so if you don't mind a (I
> believe final) v8 would probably be best.
>
> > David Plowman (8):
> >   libcamera: pipeline: raspberrypi: Revert "Set sensor default
> >     orientation before configure()"
> >   libcamera: Allow access to v4l2_query_ext_ctrl structure for a V4L2
> >     control
> >   libcamera: Add Transform enum to represent 2D plane transforms.
> >   libcamera: Add BayerFormat type
> >   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/internal/bayer_format.h     |  60 ++++
> >  include/libcamera/internal/v4l2_device.h      |   2 +
> >  include/libcamera/meson.build                 |   1 +
> >  include/libcamera/transform.h                 |  78 +++++
> >  src/ipa/raspberrypi/controller/camera_mode.h  |   4 +
> >  src/ipa/raspberrypi/controller/rpi/alsc.cpp   |  13 +-
> >  src/ipa/raspberrypi/raspberrypi.cpp           |  48 +--
> >  src/libcamera/bayer_format.cpp                | 231 +++++++++++++
> >  src/libcamera/camera.cpp                      |  16 +-
> >  src/libcamera/meson.build                     |   2 +
> >  src/libcamera/pipeline/ipu3/ipu3.cpp          |   5 +
> >  .../pipeline/raspberrypi/raspberrypi.cpp      | 155 ++++++++-
> >  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                   | 322 ++++++++++++++++++
> >  src/libcamera/v4l2_device.cpp                 |  15 +
> >  19 files changed, 941 insertions(+), 34 deletions(-)
> >  create mode 100644 include/libcamera/internal/bayer_format.h
> >  create mode 100644 include/libcamera/transform.h
> >  create mode 100644 src/libcamera/bayer_format.cpp
> >  create mode 100644 src/libcamera/transform.cpp
>
> --
> Regards,
>
> Laurent Pinchart