[libcamera-devel] [PATCH v3 3/5] libcamera: raspberrypi: Set camera flips correctly from user transform

[David Plowman]

Hi Laurent

Actually I did maybe get something wrong here, so thanks for making me
think again....

On Sun, 23 Aug 2020 at 02:51, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi David,
>
> Thank you for the patch.
>
> On Fri, Aug 21, 2020 at 04:56:39PM +0100, David Plowman wrote:
> > The Raspberry Pi pipeline handler allows all transforms except those
> > involving a transpose. The user transform is combined with any
> > inherent rotation of the camera, and the camera's H and V flip bits
> > are set accordingly.
> >
> > Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> > ---
> >  .../pipeline/raspberrypi/raspberrypi.cpp      | 34 ++++++++++++++++---
> >  1 file changed, 30 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 236aa5c..a3f8438 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -324,6 +324,8 @@ public:
> >       uint32_t expectedSequence_;
> >       bool sensorMetadata_;
> >
> > +     Transform transform_;
> > +
> >       /*
> >        * All the functions in this class are called from a single calling
> >        * thread. So, we do not need to have any mutex to protect access to any
> > @@ -400,8 +402,27 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
> >       if (config_.empty())
> >               return Invalid;
> >
> > -     if (transform != Transform::Identity) {
> > -             transform = Transform::Identity;
> > +     /*
> > +      * What if the platform has a non-90 degree rotation? We can't even
> > +      * "adjust" the configuration and carry on. Alternatively, raising an
> > +      * error means the platform can never run. Let's just print a warning
> > +      * and continue regardless; the rotation is effectively set to zero.
> > +      */
> > +     int32_t rotation = data_->sensor_->properties().get(properties::Rotation);
> > +     bool success;
> > +     Transform combined = transform * transformFromRotation(rotation, &success);
> > +     if (!success)
> > +             LOG(RPI, Warning) << "Invalid rotation of " << rotation
> > +                               << " degrees - ignoring";
> > +
> > +     /*
> > +      * We combine the platform and user transform, but must "adjust away"
> > +      * any combined result that includes a transform, as we can't do those.
> > +      * Flipping the transpose bit in either input transform causes the
> > +      * corresponding bit in the combined result to flip.
> > +      */
> > +     if (!!(combined & Transform::Transpose)) {
> > +             transform ^= Transform::Transpose;
> >               status = Adjusted;
> >       }
>
> I wonder if this wouldn't be confusing for the application. Imagine the
> following use case. We have a device with a screen, meant to operate in
> portrait mode. The camera sensor will typically be mounted with a 90°
> (or 270°) rotation, in order to match the aspect ratio of the scene and
> the screen (otherwise the scene would be captured in landscape mode and
> couldn't be displayed full-screen). Assuming the ISP can't transpose, as
> in the Raspberry Pi case the application will have to rotate the image
> by 90° before displaying it. Let's further assume the user doesn't need
> to apply any h/v flip on the camera side.
>
> transformFromRotation() returns Rot90 or Rot270. As transform is set to
> Identity by the application, combined is equal to Rot90 or Rot270, which
> has the Transpose bit set. The code above will XOR the Transpose bit
> out, leaving transform set to HFlip or VFlip. This seems an unexpect
> side effect to me.
>
> We could of course argue that the application should look at the
> Rotation property and compensate for the 90° rotation by requesting
> Rot90 or Rot270, but is that the best option, especially given that we
> don't give a way to applications to enumerate what transformations are
> supported. Maybe this is good enough for now as we don't really claim to
> support 90° or 270° rotations yet, but I feel this will need to be
> revisited sooner than later.
>

So, when the camera has a particular (non-zero) rotation with respect
to the "world view", then I think the aim is, if the application uses
the default Identity transform, to correct the camera rotation so that
the image that comes out is in the "world view". Is that right? (This
is the single most fundamental point, I think!)

For example, I think this means that if the camera rotation is, say,
90 degrees, we should therefore automatically be applying 270 degrees
(the inverse of 90 degrees) to correct this. Actually I'm not totally
clear on this and this may be where my mistake was. Suppose the world
looks like this (i.e. this is what you actually see):

AB
CD

If the camera has no rotation, then the camera image will look
identical. But now let's suppose the camera has a rotation of 90
degrees (according to its rotation property). Will the uncorrected
camera image look like this (90 degree clockwise rotation):

CA
DB

or (270 aka. -90 degree rotation)

BD
AC
?

So where I'd previously composed the user transform with the camera
rotation, I think I possibly needed to compose it with the *inverse*
of the camera rotation (depending on the answer to the above!)

On the final point, flipping only the transpose bit seems helpful to
me. It means that you always get the transform you asked for, up to
the transpose. So you either get what you wanted, or you still have to
do (just) a plain transpose, and you don't have to handle 90 degree
rotations or the opposite-diagonal transpose (not to mention the
hassle of figuring out which one you actually need!). Does that make
sense?

Thanks!
David

> >
> > @@ -610,6 +631,9 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
> >       for (auto const stream : data->streams_)
> >               stream->reset();
> >
> > +     /* We will want to know the transform requested by the application. */
> > +     data->transform_ = config->transform;
> > +
> >       Size maxSize, sensorSize;
> >       unsigned int maxIndex = 0;
> >       bool rawStream = false;
> > @@ -1174,8 +1198,10 @@ int RPiCameraData::configureIPA()
> >               /* Configure the H/V flip controls based on the sensor rotation. */
> >               ControlList ctrls(unicam_[Unicam::Image].dev()->controls());
> >               int32_t rotation = sensor_->properties().get(properties::Rotation);
> > -             ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation));
> > -             ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation));
> > +             /* The rotation was already checked in RPiCameraConfiguration::validate. */
> > +             Transform combined = transform_ * transformFromRotation(rotation);
> > +             ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!(combined & Transform::HFlip)));
> > +             ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!(combined & Transform::VFlip)));
> >               unicam_[Unicam::Image].dev()->setControls(&ctrls);
> >       }
> >
>
> --
> Regards,
>
> Laurent Pinchart