| Message ID | 20231022224159.30298-6-laurent.pinchart@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
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 >
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);
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
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);
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);
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
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
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);
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);
- 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(-)