[libcamera-devel,v6,18/12] fixup! libcamera: Use CameraConfiguration::orientation
diff mbox series

Message ID 20231022224159.30298-6-laurent.pinchart@ideasonboard.com
State Accepted
Headers show
Series
  • libcamera: Replace CameraConfiguration::transform
Related show

Commit Message

Laurent Pinchart Oct. 22, 2023, 10:41 p.m. UTC
- Use division operator in rpi pipeline handler

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Jacopo Mondi Oct. 23, 2023, 8:54 a.m. UTC | #1
Hi

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

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

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
>
Laurent Pinchart Oct. 23, 2023, 10:20 a.m. UTC | #2
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@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 ?

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

> 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);
Jacopo Mondi Oct. 23, 2023, 10:35 a.m. UTC | #3
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@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
Laurent Pinchart Oct. 23, 2023, 11:11 a.m. UTC | #4
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@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);
Laurent Pinchart Oct. 23, 2023, 11:21 a.m. UTC | #5
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@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);
Jacopo Mondi Oct. 23, 2023, 11:23 a.m. UTC | #6
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@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
David Plowman Oct. 23, 2023, 12:39 p.m. UTC | #7
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. 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.

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.

David

On Mon, 23 Oct 2023 at 12:23, Jacopo Mondi via libcamera-devel
<libcamera-devel@lists.libcamera.org> wrote:
>
> 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@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
Laurent Pinchart Oct. 23, 2023, 1:05 p.m. UTC | #8
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@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);

Patch
diff mbox series

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);
 
 	/* Ready the IPA - it must know about the sensor resolution. */
 	ret = ipa_->configure(sensorInfo_, params, result);