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

[Jacopo Mondi]

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