[libcamera-devel] [PATCH v6 18/12] fixup! libcamera: Use CameraConfiguration::orientation

[David Plowman]

Ah, *sigh*. Transforms and orientations.

I agree it's probably only ALSC that cares about the orientation. It
does seem slightly strange to pass the config->orientation (or
config->transform previously) to the IPA rather than the
combinedTransform. But I guess maybe it depends on the orientation of
the LSC tables in the tuning files.

The quoted code looks to me as though it stands a chance of being
correct if the tables are stored in the "normal way up" orientation,
i.e. with the sensor applying both h and v flips for our modules,
compensating for the mounting orientation. If the tables were in the
no h/vflips at all orientation, then I would guess it's wrong? I'm not
entirely sure what it actually does but perhaps we can give it the
benefit of the doubt for a moment.

Though even there I'm a bit doubtful about the behaviour. If another
vendor made a board with a different mounting orientation, then I
suspect that would go wrong? Possibly we ought to pass the
combinedTransform over, and either flip our tables over, or store a
"reference transform/orientation" in the tuning file which we could
compare against.

If that sounds plausible to everyone, then I'm hoping that there's
nothing mega-urgent here, though this last point probably wants fixing
in due course.

David

On Mon, 23 Oct 2023 at 12:23, Jacopo Mondi via libcamera-devel
<libcamera-devel at lists.libcamera.org> wrote:
>
> Hi Laurent
>
> On Mon, Oct 23, 2023 at 02:11:53PM +0300, Laurent Pinchart wrote:
> > On Mon, Oct 23, 2023 at 12:35:53PM +0200, Jacopo Mondi wrote:
> > > On Mon, Oct 23, 2023 at 01:20:01PM +0300, Laurent Pinchart wrote:
> > > > On Mon, Oct 23, 2023 at 10:54:52AM +0200, Jacopo Mondi wrote:
> > > > > On Mon, Oct 23, 2023 at 01:41:56AM +0300, Laurent Pinchart via libcamera-devel wrote:
> > > > > > - Use division operator in rpi pipeline handler
> > > > > >
> > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > > > > ---
> > > > > >  src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 4 ++--
> > > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > > > > index da52d7fafcee..ee222d060e4a 100644
> > > > > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > > > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > > > > @@ -1234,8 +1234,8 @@ int CameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::Config
> > > > > >       }
> > > > > >
> > > > > >       /* Always send the user transform to the IPA. */
> > > > > > -     params.transform =
> > > > > > -             static_cast<unsigned int>(transformFromOrientation(config->orientation));
> > > > > > +     Transform transform = config->orientation / Orientation::Rotate0;
> > > > > > +     params.transform = static_cast<unsigned int>(transform);
> > > > >
> > > > > I wonder if this was correct in first place.
> > > >
> > > > Do you mean in "libcamera: Use CameraConfiguration::orientation", or
> > > > before that ?
> > >
> > > Even before that, when we had
> > >
> > >        params.transform = static_cast<unsigned int>(config->transform);
> > >
> > > > > config->orientation could be adjusted to report the sensor's mounting
> > > > > orientation if the user requested orientation cannot be obtained, in
> > > > > which case the "user transform" will be set to Identity (see
> > > > > CameraSensor::computeTransform()).
> > > >
> > > > Let's assume a sensor mounted with a 180° rotation, and the user asks
> > > > for Rotate270 rotation. This can't be achieved, so config->orientation
> > > > is adjusted to Rotate180. params.transform will be Rot180 in that case,
> > > > not Identiy. Am I missing something ?
> > >
> > > I'm probably missing what's the use of transform is in the IPA
> > >
> > > Looking at the alsc.cpp algorithms from RPi (which seems to me to be
> > > the only user of the transform field)
> > >
> > >             if (!!(cameraMode.transform & libcamera::Transform::HFlip)) {
> > >                     xLo[i] = X - 1 - xLo[i];
> > >                     xHi[i] = X - 1 - xHi[i];
> > >             }
> > >
> > >             ...
> > >
> > >             if (!!(cameraMode.transform & libcamera::Transform::VFlip)) {
> > >                     yLo = Y - 1 - yLo;
> > >                     yHi = Y - 1 - yHi;
> > >             }
> > >
> > > I'm not sure I fully got what the code does, but it seems to inspect
> > > the actual transform as applied by the PH/CameraSensor, which
> > > corresponds to the combinedTransform_ variable, which in your example
> > > is Identity, and not the image orientation in the buffers, which in
> > > your example is Rot180 ?
> >
> > As far as I understand, it's only the sensor-side transform that needs
> > to be taken into account by that code, not the combined transform from
> > the camera sensor and the ISP.
> >
>
> I agree, but in this case you would be passing in Rotate180 (which is
> the mounting rotation) instead of the actual transform applied on the
> sensor (which, in case of rpi and all the other platforms we have is
> the transform applied on the sensor, as the ISP does not do any transform)
>
> David, Naush, should this be changed on top ?
>
> > > Anyway, as said this was here already, so it's not strictly related to
> > > this patch and was here already
> > >
> > > > > Wouldn't it be better to send to the IPA 'combinedTransform_' ?
> > > > >
> > > > > >
> > > > > >       /* Ready the IPA - it must know about the sensor resolution. */
> > > > > >       ret = ipa_->configure(sensorInfo_, params, result);
> >
> > --
> > Regards,
> >
> > Laurent Pinchart