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

[Laurent Pinchart]

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.

> 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