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

[Laurent Pinchart]

On Mon, Oct 23, 2023 at 01:39:57PM +0100, David Plowman wrote:
> 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.

Is that what you have in your tuning files ?

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

Knowing in which orientation the tables have been computed is certainly
useful information. This could be conveyed in the tuning file, or we
could require a particular orientation to be used unconditionally. The
latter seems simpler, but maybe it would cause other issues ?

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

OK, I'll then merge the series :-)

> On Mon, 23 Oct 2023 at 12:23, Jacopo Mondi via libcamera-devel wrote:
> > 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