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

[Laurent Pinchart]

On Sun, Sep 06, 2020 at 06:21:02PM +0100, David Plowman wrote:
> 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.

Yes, I think it makes sense. Thanks for adding comments, this is a topic
that will really benefit from documentation.

> 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!

Sounds good to me, given that we don't support any sensor that can
transpose the image.

> On Sun, 6 Sep 2020 at 01:45, Laurent Pinchart wrote:
> > 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