[libcamera-devel,v7,0/8] 2D transforms
mbox series

Message ID 20200904103042.1593-1-david.plowman@raspberrypi.com
Headers show
Series
  • 2D transforms
Related show

Message

David Plowman Sept. 4, 2020, 10:30 a.m. UTC 
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!

Thanks and best regards
David

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

Comments

Laurent Pinchart Sept. 6, 2020, 12:44 a.m. UTC | #1 
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
David Plowman Sept. 6, 2020, 5:21 p.m. UTC | #2 
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@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
Laurent Pinchart Sept. 6, 2020, 9:54 p.m. UTC | #3 
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