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

[Jacopo Mondi]

Hi Laurent

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 ?

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