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

[Laurent Pinchart]

And CC'ing David and Naush, as the issue may affect them.

On Mon, Oct 23, 2023 at 02:11:53PM +0300, Laurent Pinchart via libcamera-devel 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.
> 
> > 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