| Message ID | 20200704005227.21782-9-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, Thank you for your patch. On Sat, 4 Jul 2020 at 01:52, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Instead of receiving sensor orientation configuration from the IPA, > retrieve it from the CameraSensor Rotation property, and configure the > HFLIP and VFLIP controls accordingly. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 74bee6895dcd..fda6831e6a7e 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -16,6 +16,7 @@ > #include <libcamera/formats.h> > #include <libcamera/ipa/raspberrypi.h> > #include <libcamera/logging.h> > +#include <libcamera/property_ids.h> > #include <libcamera/request.h> > #include <libcamera/stream.h> > > @@ -1174,6 +1175,13 @@ int RPiCameraData::configureIPA() > ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig, > nullptr); > > + /* Configure the H/V flip controls based on the sensor rotation. */ > + ControlList ctrls(unicam_[Unicam::Image].dev()->controls()); > + int32_t rotation = sensor_->properties().get(properties::Rotation); > + ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation)); > + ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation)); Are you missing a ">> 1" or similar in the above code? Otherwise, both H/V FLIP will be always set to the same value. Regards, Naush > + unicam_[Unicam::Image].dev()->setControls(&ctrls); > + > return 0; > } > > @@ -1202,10 +1210,6 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData > { V4L2_CID_EXPOSURE, action.data[1] } }); > sensorMetadata_ = action.data[2]; > } > - > - /* Set the sensor orientation here as well. */ > - ControlList controls = action.controls[0]; > - unicam_[Unicam::Image].dev()->setControls(&controls); > return; > } > > -- > Regards, > > Laurent Pinchart >
Hi Laurent, On Sat, Jul 04, 2020 at 03:52:23AM +0300, Laurent Pinchart wrote: > Instead of receiving sensor orientation configuration from the IPA, > retrieve it from the CameraSensor Rotation property, and configure the > HFLIP and VFLIP controls accordingly. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 74bee6895dcd..fda6831e6a7e 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -16,6 +16,7 @@ > #include <libcamera/formats.h> > #include <libcamera/ipa/raspberrypi.h> > #include <libcamera/logging.h> > +#include <libcamera/property_ids.h> > #include <libcamera/request.h> > #include <libcamera/stream.h> > > @@ -1174,6 +1175,13 @@ int RPiCameraData::configureIPA() > ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig, > nullptr); > > + /* Configure the H/V flip controls based on the sensor rotation. */ > + ControlList ctrls(unicam_[Unicam::Image].dev()->controls()); > + int32_t rotation = sensor_->properties().get(properties::Rotation); > + ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation)); > + ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation)); mmm, I see in the IPA this, which is where I assume you got this from ctrls.set(V4L2_CID_HFLIP, (int32_t) !!(orientation & RPi::CamTransform_HFLIP)); ctrls.set(V4L2_CID_VFLIP, (int32_t) !!(orientation & RPi::CamTransform_VFLIP)); Where the 'orientation' flag is retrieved from RPi::CamTransform orientation = helper_->GetOrientation(); and expressed through members of this enumeration static constexpr CamTransform CamTransform_IDENTITY = 0; static constexpr CamTransform CamTransform_HFLIP = 1; static constexpr CamTransform CamTransform_VFLIP = 2; Now, 'rotation' is expressed in multiple of 90 degrees in the [0-360[ interval, shouldn't we: if (rotation == 180) HFLIP if (rotation == 90 || roation == 270) VFLIP ? > + unicam_[Unicam::Image].dev()->setControls(&ctrls); > + > return 0; > } > > @@ -1202,10 +1210,6 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData > { V4L2_CID_EXPOSURE, action.data[1] } }); > sensorMetadata_ = action.data[2]; > } > - > - /* Set the sensor orientation here as well. */ > - ControlList controls = action.controls[0]; > - unicam_[Unicam::Image].dev()->setControls(&controls); Do you think we can now remove the custom rotation handling from the IPA helpers ? > return; > } > > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, Laurent, everyone! I have a couple of questions. 1. Does this allow an application to set a transformation when the camera is started? For example, it's not uncommon for a horizontal flip to be requested from the camera depending on the application. Certainly in the Raspberry Pi world you can't tell a priori what a sensor's orientation is. 2nd question below... On Mon, 6 Jul 2020 at 10:12, Jacopo Mondi <jacopo@jmondi.org> wrote: > > Hi Laurent, > > On Sat, Jul 04, 2020 at 03:52:23AM +0300, Laurent Pinchart wrote: > > Instead of receiving sensor orientation configuration from the IPA, > > retrieve it from the CameraSensor Rotation property, and configure the > > HFLIP and VFLIP controls accordingly. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++---- > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index 74bee6895dcd..fda6831e6a7e 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -16,6 +16,7 @@ > > #include <libcamera/formats.h> > > #include <libcamera/ipa/raspberrypi.h> > > #include <libcamera/logging.h> > > +#include <libcamera/property_ids.h> > > #include <libcamera/request.h> > > #include <libcamera/stream.h> > > > > @@ -1174,6 +1175,13 @@ int RPiCameraData::configureIPA() > > ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig, > > nullptr); > > > > + /* Configure the H/V flip controls based on the sensor rotation. */ > > + ControlList ctrls(unicam_[Unicam::Image].dev()->controls()); > > + int32_t rotation = sensor_->properties().get(properties::Rotation); > > + ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation)); > > + ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation)); > > mmm, I see in the IPA this, which is where I assume you got this from > > ctrls.set(V4L2_CID_HFLIP, (int32_t) !!(orientation & RPi::CamTransform_HFLIP)); > ctrls.set(V4L2_CID_VFLIP, (int32_t) !!(orientation & RPi::CamTransform_VFLIP)); > > Where the 'orientation' flag is retrieved from > RPi::CamTransform orientation = helper_->GetOrientation(); > > and expressed through members of this enumeration > static constexpr CamTransform CamTransform_IDENTITY = 0; > static constexpr CamTransform CamTransform_HFLIP = 1; > static constexpr CamTransform CamTransform_VFLIP = 2; > > Now, 'rotation' is expressed in multiple of 90 degrees in the [0-360[ > interval, shouldn't we: > if (rotation == 180) > HFLIP > if (rotation == 90 || roation == 270) > VFLIP > ? How are we handling transformations that aren't rotations, e.g. horizontal flip. As I said previously, it's a significant use case. Also, how are we handling platforms that don't support things like 90/270 degree rotations - it's certain that many platforms won't be able to do that. Best regards David > > > > + unicam_[Unicam::Image].dev()->setControls(&ctrls); > > + > > return 0; > > } > > > > @@ -1202,10 +1210,6 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData > > { V4L2_CID_EXPOSURE, action.data[1] } }); > > sensorMetadata_ = action.data[2]; > > } > > - > > - /* Set the sensor orientation here as well. */ > > - ControlList controls = action.controls[0]; > > - unicam_[Unicam::Image].dev()->setControls(&controls); > > Do you think we can now remove the custom rotation handling from the > IPA helpers ? > > > return; > > } > > > > -- > > Regards, > > > > Laurent Pinchart > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi David, On Mon, Jul 06, 2020 at 10:29:21AM +0100, David Plowman wrote: > Hi Jacopo, Laurent, everyone! > > I have a couple of questions. > > 1. Does this allow an application to set a transformation when the > camera is started? For example, it's not uncommon for a horizontal > flip to be requested from the camera depending on the application. > Certainly in the Raspberry Pi world you can't tell a priori what a > sensor's orientation is. > Isn't this mostly related to allowing application provide a list of controls at camera start/configure time ? Those will have to be taken into account and matched with the camera sensor reported orientation which is handled here I guess. > 2nd question below... > > On Mon, 6 Jul 2020 at 10:12, Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > Hi Laurent, > > > > On Sat, Jul 04, 2020 at 03:52:23AM +0300, Laurent Pinchart wrote: > > > Instead of receiving sensor orientation configuration from the IPA, > > > retrieve it from the CameraSensor Rotation property, and configure the > > > HFLIP and VFLIP controls accordingly. > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++---- > > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > index 74bee6895dcd..fda6831e6a7e 100644 > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > @@ -16,6 +16,7 @@ > > > #include <libcamera/formats.h> > > > #include <libcamera/ipa/raspberrypi.h> > > > #include <libcamera/logging.h> > > > +#include <libcamera/property_ids.h> > > > #include <libcamera/request.h> > > > #include <libcamera/stream.h> > > > > > > @@ -1174,6 +1175,13 @@ int RPiCameraData::configureIPA() > > > ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig, > > > nullptr); > > > > > > + /* Configure the H/V flip controls based on the sensor rotation. */ > > > + ControlList ctrls(unicam_[Unicam::Image].dev()->controls()); > > > + int32_t rotation = sensor_->properties().get(properties::Rotation); > > > + ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation)); > > > + ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation)); > > > > mmm, I see in the IPA this, which is where I assume you got this from > > > > ctrls.set(V4L2_CID_HFLIP, (int32_t) !!(orientation & RPi::CamTransform_HFLIP)); > > ctrls.set(V4L2_CID_VFLIP, (int32_t) !!(orientation & RPi::CamTransform_VFLIP)); > > > > Where the 'orientation' flag is retrieved from > > RPi::CamTransform orientation = helper_->GetOrientation(); > > > > and expressed through members of this enumeration > > static constexpr CamTransform CamTransform_IDENTITY = 0; > > static constexpr CamTransform CamTransform_HFLIP = 1; > > static constexpr CamTransform CamTransform_VFLIP = 2; > > > > Now, 'rotation' is expressed in multiple of 90 degrees in the [0-360[ > > interval, shouldn't we: > > if (rotation == 180) > > HFLIP This should be VFLIP, as the control reads "mirror in the vertical direction", not "along the horizontal axis" as I interpreted it > > if (rotation == 90 || roation == 270) > > VFLIP And this is just wrong. Mirroring in H/V directions won't compensate for such rotations. > > ? > > How are we handling transformations that aren't rotations, e.g. > horizontal flip. As I said previously, it's a significant use case. I think they merit a control to let application handle them I think this was meant to compensate the camera mounting rotation only. > Also, how are we handling platforms that don't support things like > 90/270 degree rotations - it's certain that many platforms won't be > able to do that. That is a more generic question: how do pipeline handlers advertise which control they support, or do you think H/V flips are different ? Thanks j > > Best regards > David > > > > > > > > + unicam_[Unicam::Image].dev()->setControls(&ctrls); > > > + > > > return 0; > > > } > > > > > > @@ -1202,10 +1210,6 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData > > > { V4L2_CID_EXPOSURE, action.data[1] } }); > > > sensorMetadata_ = action.data[2]; > > > } > > > - > > > - /* Set the sensor orientation here as well. */ > > > - ControlList controls = action.controls[0]; > > > - unicam_[Unicam::Image].dev()->setControls(&controls); > > > > Do you think we can now remove the custom rotation handling from the > > IPA helpers ? > > > > > return; > > > } > > > > > > -- > > > Regards, > > > > > > Laurent Pinchart > > > > > > _______________________________________________ > > > libcamera-devel mailing list > > > libcamera-devel@lists.libcamera.org > > > https://lists.libcamera.org/listinfo/libcamera-devel > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo Thanks for the quick reply! Let me just clarify my understanding of your answers... On Mon, 6 Jul 2020 at 11:01, Jacopo Mondi <jacopo@jmondi.org> wrote: > > Hi David, > > On Mon, Jul 06, 2020 at 10:29:21AM +0100, David Plowman wrote: > > Hi Jacopo, Laurent, everyone! > > > > I have a couple of questions. > > > > 1. Does this allow an application to set a transformation when the > > camera is started? For example, it's not uncommon for a horizontal > > flip to be requested from the camera depending on the application. > > Certainly in the Raspberry Pi world you can't tell a priori what a > > sensor's orientation is. > > > > Isn't this mostly related to allowing application provide a list of > controls at camera start/configure time ? Those will have to be taken > into account and matched with the camera sensor reported orientation > which is handled here I guess. So far as I'm aware there is no control for transformations (h/vflip, even transpose) currently, is that correct? I'm also not sure the usual request->controls() mechanism is the right one as it sort-of implies you can set it per frame, which I think is something we shouldn't allow. Perhaps it belongs in the StreamConfiguration? Some platforms might allow per-stream transformations, others (many?) will not. What do people think? > > > 2nd question below... > > > > On Mon, 6 Jul 2020 at 10:12, Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > > > Hi Laurent, > > > > > > On Sat, Jul 04, 2020 at 03:52:23AM +0300, Laurent Pinchart wrote: > > > > Instead of receiving sensor orientation configuration from the IPA, > > > > retrieve it from the CameraSensor Rotation property, and configure the > > > > HFLIP and VFLIP controls accordingly. > > > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > --- > > > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++---- > > > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > index 74bee6895dcd..fda6831e6a7e 100644 > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > @@ -16,6 +16,7 @@ > > > > #include <libcamera/formats.h> > > > > #include <libcamera/ipa/raspberrypi.h> > > > > #include <libcamera/logging.h> > > > > +#include <libcamera/property_ids.h> > > > > #include <libcamera/request.h> > > > > #include <libcamera/stream.h> > > > > > > > > @@ -1174,6 +1175,13 @@ int RPiCameraData::configureIPA() > > > > ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig, > > > > nullptr); > > > > > > > > + /* Configure the H/V flip controls based on the sensor rotation. */ > > > > + ControlList ctrls(unicam_[Unicam::Image].dev()->controls()); > > > > + int32_t rotation = sensor_->properties().get(properties::Rotation); > > > > + ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation)); > > > > + ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation)); > > > > > > mmm, I see in the IPA this, which is where I assume you got this from > > > > > > ctrls.set(V4L2_CID_HFLIP, (int32_t) !!(orientation & RPi::CamTransform_HFLIP)); > > > ctrls.set(V4L2_CID_VFLIP, (int32_t) !!(orientation & RPi::CamTransform_VFLIP)); > > > > > > Where the 'orientation' flag is retrieved from > > > RPi::CamTransform orientation = helper_->GetOrientation(); > > > > > > and expressed through members of this enumeration > > > static constexpr CamTransform CamTransform_IDENTITY = 0; > > > static constexpr CamTransform CamTransform_HFLIP = 1; > > > static constexpr CamTransform CamTransform_VFLIP = 2; > > > > > > Now, 'rotation' is expressed in multiple of 90 degrees in the [0-360[ > > > interval, shouldn't we: > > > if (rotation == 180) > > > HFLIP > > This should be VFLIP, as the control reads "mirror in the vertical > direction", not "along the horizontal axis" as I interpreted it > > > > if (rotation == 90 || roation == 270) > > > VFLIP > > And this is just wrong. Mirroring in H/V directions won't compensate > for such rotations. > > > > ? > > > > How are we handling transformations that aren't rotations, e.g. > > horizontal flip. As I said previously, it's a significant use case. > > I think they merit a control to let application handle them > I think this was meant to compensate the camera mounting rotation > only. > > > Also, how are we handling platforms that don't support things like > > 90/270 degree rotations - it's certain that many platforms won't be > > able to do that. > > That is a more generic question: how do pipeline handlers advertise > which control they support, or do you think H/V flips are different ? As I said above, I'm not sure the usual "controls" mechanism is the right one, and it's also not clear to me how a platform should advertise what it supports. (Do we have any mechanism for signalling that kind of thing?) Perhaps one could take the view that, for the time being, we just overwrite anything we don't handle and report the configuration as "adjusted"? (Perhaps I am hijacking the original discussion... in which case please say and I'll re-start it on its own.) Best regards David > > Thanks > j > > > > > Best regards > > David > > > > > > > > > > > > + unicam_[Unicam::Image].dev()->setControls(&ctrls); > > > > + > > > > return 0; > > > > } > > > > > > > > @@ -1202,10 +1210,6 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData > > > > { V4L2_CID_EXPOSURE, action.data[1] } }); > > > > sensorMetadata_ = action.data[2]; > > > > } > > > > - > > > > - /* Set the sensor orientation here as well. */ > > > > - ControlList controls = action.controls[0]; > > > > - unicam_[Unicam::Image].dev()->setControls(&controls); > > > > > > Do you think we can now remove the custom rotation handling from the > > > IPA helpers ? > > > > > > > return; > > > > } > > > > > > > > -- > > > > Regards, > > > > > > > > Laurent Pinchart > > > > > > > > _______________________________________________ > > > > libcamera-devel mailing list > > > > libcamera-devel@lists.libcamera.org > > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > _______________________________________________ > > > libcamera-devel mailing list > > > libcamera-devel@lists.libcamera.org > > > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Naush, On Mon, Jul 06, 2020 at 09:31:31AM +0100, Naushir Patuck wrote: > On Sat, 4 Jul 2020 at 01:52, Laurent Pinchart wrote: > > > > Instead of receiving sensor orientation configuration from the IPA, > > retrieve it from the CameraSensor Rotation property, and configure the > > HFLIP and VFLIP controls accordingly. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++---- > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index 74bee6895dcd..fda6831e6a7e 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -16,6 +16,7 @@ > > #include <libcamera/formats.h> > > #include <libcamera/ipa/raspberrypi.h> > > #include <libcamera/logging.h> > > +#include <libcamera/property_ids.h> > > #include <libcamera/request.h> > > #include <libcamera/stream.h> > > > > @@ -1174,6 +1175,13 @@ int RPiCameraData::configureIPA() > > ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig, > > nullptr); > > > > + /* Configure the H/V flip controls based on the sensor rotation. */ > > + ControlList ctrls(unicam_[Unicam::Image].dev()->controls()); > > + int32_t rotation = sensor_->properties().get(properties::Rotation); > > + ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation)); > > + ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation)); > > Are you missing a ">> 1" or similar in the above code? Otherwise, > both H/V FLIP will be always set to the same value. That's because the RPi IPA currently handles 0° and 180° rotations only, without separate control of h/v flip. We need to add h/v flip controls, and they will then be combined here in the pipeline handler. > > + unicam_[Unicam::Image].dev()->setControls(&ctrls); > > + > > return 0; > > } > > > > @@ -1202,10 +1210,6 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData > > { V4L2_CID_EXPOSURE, action.data[1] } }); > > sensorMetadata_ = action.data[2]; > > } > > - > > - /* Set the sensor orientation here as well. */ > > - ControlList controls = action.controls[0]; > > - unicam_[Unicam::Image].dev()->setControls(&controls); > > return; > > } > >
Hi Jacopo, On Mon, Jul 06, 2020 at 11:16:11AM +0200, Jacopo Mondi wrote: > On Sat, Jul 04, 2020 at 03:52:23AM +0300, Laurent Pinchart wrote: > > Instead of receiving sensor orientation configuration from the IPA, > > retrieve it from the CameraSensor Rotation property, and configure the > > HFLIP and VFLIP controls accordingly. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++---- > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index 74bee6895dcd..fda6831e6a7e 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -16,6 +16,7 @@ > > #include <libcamera/formats.h> > > #include <libcamera/ipa/raspberrypi.h> > > #include <libcamera/logging.h> > > +#include <libcamera/property_ids.h> > > #include <libcamera/request.h> > > #include <libcamera/stream.h> > > > > @@ -1174,6 +1175,13 @@ int RPiCameraData::configureIPA() > > ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig, > > nullptr); > > > > + /* Configure the H/V flip controls based on the sensor rotation. */ > > + ControlList ctrls(unicam_[Unicam::Image].dev()->controls()); > > + int32_t rotation = sensor_->properties().get(properties::Rotation); > > + ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation)); > > + ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation)); > > mmm, I see in the IPA this, which is where I assume you got this from > > ctrls.set(V4L2_CID_HFLIP, (int32_t) !!(orientation & RPi::CamTransform_HFLIP)); > ctrls.set(V4L2_CID_VFLIP, (int32_t) !!(orientation & RPi::CamTransform_VFLIP)); > > Where the 'orientation' flag is retrieved from > RPi::CamTransform orientation = helper_->GetOrientation(); > > and expressed through members of this enumeration > static constexpr CamTransform CamTransform_IDENTITY = 0; > static constexpr CamTransform CamTransform_HFLIP = 1; > static constexpr CamTransform CamTransform_VFLIP = 2; > > Now, 'rotation' is expressed in multiple of 90 degrees in the [0-360[ > interval, shouldn't we: > if (rotation == 180) > HFLIP > if (rotation == 90 || roation == 270) > VFLIP > ? 90° or 270° rotations can't be achieved through flipping, they require a real rotation engine. Only 180° can be achieved with H+V flip. Note that the RPi IPA currently sets either both or neither of the flip controls, based on hard-coded per-sensor values. > > + unicam_[Unicam::Image].dev()->setControls(&ctrls); > > + > > return 0; > > } > > > > @@ -1202,10 +1210,6 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData > > { V4L2_CID_EXPOSURE, action.data[1] } }); > > sensorMetadata_ = action.data[2]; > > } > > - > > - /* Set the sensor orientation here as well. */ > > - ControlList controls = action.controls[0]; > > - unicam_[Unicam::Image].dev()->setControls(&controls); > > Do you think we can now remove the custom rotation handling from the > IPA helpers ? See other patches in this series :-) > > return; > > } > >
Hi David and Jacopo, On Mon, Jul 06, 2020 at 11:41:13AM +0100, David Plowman wrote: > On Mon, 6 Jul 2020 at 11:01, Jacopo Mondi <jacopo@jmondi.org> wrote: > > On Mon, Jul 06, 2020 at 10:29:21AM +0100, David Plowman wrote: > >> Hi Jacopo, Laurent, everyone! > >> > >> I have a couple of questions. > >> > >> 1. Does this allow an application to set a transformation when the > >> camera is started? For example, it's not uncommon for a horizontal > >> flip to be requested from the camera depending on the application. > >> Certainly in the Raspberry Pi world you can't tell a priori what a > >> sensor's orientation is. > > > > Isn't this mostly related to allowing application provide a list of > > controls at camera start/configure time ? Those will have to be taken > > into account and matched with the camera sensor reported orientation > > which is handled here I guess. > > So far as I'm aware there is no control for transformations (h/vflip, > even transpose) currently, is that correct? Correct. I think they should be added though. > I'm also not sure the > usual request->controls() mechanism is the right one as it sort-of > implies you can set it per frame, which I think is something we > shouldn't allow. Why not ? If platforms allow per-frame control of flipping, wouldn't it make sense to allow it in the API ? We've thought about adding a ControlList to CameraConfiguration to set controls that can't change per frame. Pipeline handlers could then decide if they want to support the flip controls per frame, or only at configure time. We could extend the ControlInfo class to report whether a control can be set per frame or only at configure time. > Perhaps it belongs in the StreamConfiguration? Some platforms might > allow per-stream transformations, others (many?) will not. What do > people think? > > >> 2nd question below... > >> > >> On Mon, 6 Jul 2020 at 10:12, Jacopo Mondi <jacopo@jmondi.org> wrote: > >>> > >>> Hi Laurent, > >>> > >>> On Sat, Jul 04, 2020 at 03:52:23AM +0300, Laurent Pinchart wrote: > >>>> Instead of receiving sensor orientation configuration from the IPA, > >>>> retrieve it from the CameraSensor Rotation property, and configure the > >>>> HFLIP and VFLIP controls accordingly. > >>>> > >>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>>> --- > >>>> src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++---- > >>>> 1 file changed, 8 insertions(+), 4 deletions(-) > >>>> > >>>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > >>>> index 74bee6895dcd..fda6831e6a7e 100644 > >>>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > >>>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > >>>> @@ -16,6 +16,7 @@ > >>>> #include <libcamera/formats.h> > >>>> #include <libcamera/ipa/raspberrypi.h> > >>>> #include <libcamera/logging.h> > >>>> +#include <libcamera/property_ids.h> > >>>> #include <libcamera/request.h> > >>>> #include <libcamera/stream.h> > >>>> > >>>> @@ -1174,6 +1175,13 @@ int RPiCameraData::configureIPA() > >>>> ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig, > >>>> nullptr); > >>>> > >>>> + /* Configure the H/V flip controls based on the sensor rotation. */ > >>>> + ControlList ctrls(unicam_[Unicam::Image].dev()->controls()); > >>>> + int32_t rotation = sensor_->properties().get(properties::Rotation); > >>>> + ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation)); > >>>> + ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation)); > >>> > >>> mmm, I see in the IPA this, which is where I assume you got this from > >>> > >>> ctrls.set(V4L2_CID_HFLIP, (int32_t) !!(orientation & RPi::CamTransform_HFLIP)); > >>> ctrls.set(V4L2_CID_VFLIP, (int32_t) !!(orientation & RPi::CamTransform_VFLIP)); > >>> > >>> Where the 'orientation' flag is retrieved from > >>> RPi::CamTransform orientation = helper_->GetOrientation(); > >>> > >>> and expressed through members of this enumeration > >>> static constexpr CamTransform CamTransform_IDENTITY = 0; > >>> static constexpr CamTransform CamTransform_HFLIP = 1; > >>> static constexpr CamTransform CamTransform_VFLIP = 2; > >>> > >>> Now, 'rotation' is expressed in multiple of 90 degrees in the [0-360[ > >>> interval, shouldn't we: > >>> if (rotation == 180) > >>> HFLIP > > > > This should be VFLIP, as the control reads "mirror in the vertical > > direction", not "along the horizontal axis" as I interpreted it > > > >>> if (rotation == 90 || roation == 270) > >>> VFLIP > > > > And this is just wrong. Mirroring in H/V directions won't compensate > > for such rotations. > > > >>> ? > >> > >> How are we handling transformations that aren't rotations, e.g. > >> horizontal flip. As I said previously, it's a significant use case. > > > > I think they merit a control to let application handle them > > I think this was meant to compensate the camera mounting rotation > > only. > > > >> Also, how are we handling platforms that don't support things like > >> 90/270 degree rotations - it's certain that many platforms won't be > >> able to do that. > > > > That is a more generic question: how do pipeline handlers advertise > > which control they support, or do you think H/V flips are different ? > > As I said above, I'm not sure the usual "controls" mechanism is the > right one, and it's also not clear to me how a platform should > advertise what it supports. (Do we have any mechanism for signalling > that kind of thing?) Perhaps one could take the view that, for the > time being, we just overwrite anything we don't handle and report the > configuration as "adjusted"? > > (Perhaps I am hijacking the original discussion... in which case > please say and I'll re-start it on its own.) Hijacking discussions is how we usually operate, no worries about that :-) I don't think it should prevent this patch from being merged though. > >>>> + unicam_[Unicam::Image].dev()->setControls(&ctrls); > >>>> + > >>>> return 0; > >>>> } > >>>> > >>>> @@ -1202,10 +1210,6 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData > >>>> { V4L2_CID_EXPOSURE, action.data[1] } }); > >>>> sensorMetadata_ = action.data[2]; > >>>> } > >>>> - > >>>> - /* Set the sensor orientation here as well. */ > >>>> - ControlList controls = action.controls[0]; > >>>> - unicam_[Unicam::Image].dev()->setControls(&controls); > >>> > >>> Do you think we can now remove the custom rotation handling from the > >>> IPA helpers ? > >>> > >>>> return; > >>>> } > >>>>
Hi David and Jacopo, On Mon, 6 Jul 2020 at 11:41, David Plowman <david.plowman@raspberrypi.com> wrote: > > Hi Jacopo > > Thanks for the quick reply! Let me just clarify my understanding of > your answers... > > On Mon, 6 Jul 2020 at 11:01, Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > Hi David, > > > > On Mon, Jul 06, 2020 at 10:29:21AM +0100, David Plowman wrote: > > > Hi Jacopo, Laurent, everyone! > > > > > > I have a couple of questions. > > > > > > 1. Does this allow an application to set a transformation when the > > > camera is started? For example, it's not uncommon for a horizontal > > > flip to be requested from the camera depending on the application. > > > Certainly in the Raspberry Pi world you can't tell a priori what a > > > sensor's orientation is. > > > > > > > Isn't this mostly related to allowing application provide a list of > > controls at camera start/configure time ? Those will have to be taken > > into account and matched with the camera sensor reported orientation > > which is handled here I guess. > > So far as I'm aware there is no control for transformations (h/vflip, > even transpose) currently, is that correct? I'm also not sure the > usual request->controls() mechanism is the right one as it sort-of > implies you can set it per frame, which I think is something we > shouldn't allow. > > Perhaps it belongs in the StreamConfiguration? Some platforms might > allow per-stream transformations, others (many?) will not. What do > people think? > > > > > > 2nd question below... > > > > > > On Mon, 6 Jul 2020 at 10:12, Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > > > > > Hi Laurent, > > > > > > > > On Sat, Jul 04, 2020 at 03:52:23AM +0300, Laurent Pinchart wrote: > > > > > Instead of receiving sensor orientation configuration from the IPA, > > > > > retrieve it from the CameraSensor Rotation property, and configure the > > > > > HFLIP and VFLIP controls accordingly. > > > > > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > --- > > > > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++---- > > > > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > > index 74bee6895dcd..fda6831e6a7e 100644 > > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > > @@ -16,6 +16,7 @@ > > > > > #include <libcamera/formats.h> > > > > > #include <libcamera/ipa/raspberrypi.h> > > > > > #include <libcamera/logging.h> > > > > > +#include <libcamera/property_ids.h> > > > > > #include <libcamera/request.h> > > > > > #include <libcamera/stream.h> > > > > > > > > > > @@ -1174,6 +1175,13 @@ int RPiCameraData::configureIPA() > > > > > ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig, > > > > > nullptr); > > > > > > > > > > + /* Configure the H/V flip controls based on the sensor rotation. */ > > > > > + ControlList ctrls(unicam_[Unicam::Image].dev()->controls()); > > > > > + int32_t rotation = sensor_->properties().get(properties::Rotation); > > > > > + ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation)); > > > > > + ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation)); > > > > > > > > mmm, I see in the IPA this, which is where I assume you got this from > > > > > > > > ctrls.set(V4L2_CID_HFLIP, (int32_t) !!(orientation & RPi::CamTransform_HFLIP)); > > > > ctrls.set(V4L2_CID_VFLIP, (int32_t) !!(orientation & RPi::CamTransform_VFLIP)); > > > > > > > > Where the 'orientation' flag is retrieved from > > > > RPi::CamTransform orientation = helper_->GetOrientation(); > > > > > > > > and expressed through members of this enumeration > > > > static constexpr CamTransform CamTransform_IDENTITY = 0; > > > > static constexpr CamTransform CamTransform_HFLIP = 1; > > > > static constexpr CamTransform CamTransform_VFLIP = 2; > > > > > > > > Now, 'rotation' is expressed in multiple of 90 degrees in the [0-360[ > > > > interval, shouldn't we: > > > > if (rotation == 180) > > > > HFLIP > > > > This should be VFLIP, as the control reads "mirror in the vertical > > direction", not "along the horizontal axis" as I interpreted it > > > > > > if (rotation == 90 || roation == 270) > > > > VFLIP > > > > And this is just wrong. Mirroring in H/V directions won't compensate > > for such rotations. > > > > > > ? > > > > > > How are we handling transformations that aren't rotations, e.g. > > > horizontal flip. As I said previously, it's a significant use case. > > > > I think they merit a control to let application handle them > > I think this was meant to compensate the camera mounting rotation > > only. > > > > > Also, how are we handling platforms that don't support things like > > > 90/270 degree rotations - it's certain that many platforms won't be > > > able to do that. > > > > That is a more generic question: how do pipeline handlers advertise > > which control they support, or do you think H/V flips are different ? > > As I said above, I'm not sure the usual "controls" mechanism is the > right one, and it's also not clear to me how a platform should > advertise what it supports. (Do we have any mechanism for signalling > that kind of thing?) Perhaps one could take the view that, for the > time being, we just overwrite anything we don't handle and report the > configuration as "adjusted"? > > (Perhaps I am hijacking the original discussion... in which case > please say and I'll re-start it on its own.) If I am not mistaken, the pipeline handlers need to maintain a list of controls supported, like in include/libcamera/ipa/raspberrypi.h (RPiControls). This then gets exported to the CameraData base class member controlInfo_. ARequest will not allow setting a libcamera::control that is not listed in CameraData::controlInfo_ Regards, Naush > > Best regards > David > > > > > Thanks > > j > > > > > > > > Best regards > > > David > > > > > > > > > > > > > > > > + unicam_[Unicam::Image].dev()->setControls(&ctrls); > > > > > + > > > > > return 0; > > > > > } > > > > > > > > > > @@ -1202,10 +1210,6 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData > > > > > { V4L2_CID_EXPOSURE, action.data[1] } }); > > > > > sensorMetadata_ = action.data[2]; > > > > > } > > > > > - > > > > > - /* Set the sensor orientation here as well. */ > > > > > - ControlList controls = action.controls[0]; > > > > > - unicam_[Unicam::Image].dev()->setControls(&controls); > > > > > > > > Do you think we can now remove the custom rotation handling from the > > > > IPA helpers ? > > > > > > > > > return; > > > > > } > > > > > > > > > > -- > > > > > Regards, > > > > > > > > > > Laurent Pinchart > > > > > > > > > > _______________________________________________ > > > > > libcamera-devel mailing list > > > > > libcamera-devel@lists.libcamera.org > > > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > _______________________________________________ > > > > libcamera-devel mailing list > > > > libcamera-devel@lists.libcamera.org > > > > https://lists.libcamera.org/listinfo/libcamera-devel > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Naush, On Mon, Jul 06, 2020 at 02:21:21PM +0300, Laurent Pinchart wrote: > On Mon, Jul 06, 2020 at 09:31:31AM +0100, Naushir Patuck wrote: > > On Sat, 4 Jul 2020 at 01:52, Laurent Pinchart wrote: > > > > > > Instead of receiving sensor orientation configuration from the IPA, > > > retrieve it from the CameraSensor Rotation property, and configure the > > > HFLIP and VFLIP controls accordingly. > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++---- > > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > index 74bee6895dcd..fda6831e6a7e 100644 > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > @@ -16,6 +16,7 @@ > > > #include <libcamera/formats.h> > > > #include <libcamera/ipa/raspberrypi.h> > > > #include <libcamera/logging.h> > > > +#include <libcamera/property_ids.h> > > > #include <libcamera/request.h> > > > #include <libcamera/stream.h> > > > > > > @@ -1174,6 +1175,13 @@ int RPiCameraData::configureIPA() > > > ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig, > > > nullptr); > > > > > > + /* Configure the H/V flip controls based on the sensor rotation. */ > > > + ControlList ctrls(unicam_[Unicam::Image].dev()->controls()); > > > + int32_t rotation = sensor_->properties().get(properties::Rotation); > > > + ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation)); > > > + ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation)); > > > > Are you missing a ">> 1" or similar in the above code? Otherwise, > > both H/V FLIP will be always set to the same value. > > That's because the RPi IPA currently handles 0° and 180° rotations only, > without separate control of h/v flip. We need to add h/v flip controls, > and they will then be combined here in the pipeline handler. Do you think that's fine (in which case could you send me a R-b tag), or are changes needed ? > > > + unicam_[Unicam::Image].dev()->setControls(&ctrls); > > > + > > > return 0; > > > } > > > > > > @@ -1202,10 +1210,6 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData > > > { V4L2_CID_EXPOSURE, action.data[1] } }); > > > sensorMetadata_ = action.data[2]; > > > } > > > - > > > - /* Set the sensor orientation here as well. */ > > > - ControlList controls = action.controls[0]; > > > - unicam_[Unicam::Image].dev()->setControls(&controls); > > > return; > > > } > > >
Hi Laurent, On Wed, 8 Jul 2020 at 21:49, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Naush, > > On Mon, Jul 06, 2020 at 02:21:21PM +0300, Laurent Pinchart wrote: > > On Mon, Jul 06, 2020 at 09:31:31AM +0100, Naushir Patuck wrote: > > > On Sat, 4 Jul 2020 at 01:52, Laurent Pinchart wrote: > > > > > > > > Instead of receiving sensor orientation configuration from the IPA, > > > > retrieve it from the CameraSensor Rotation property, and configure the > > > > HFLIP and VFLIP controls accordingly. > > > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > --- > > > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++---- > > > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > index 74bee6895dcd..fda6831e6a7e 100644 > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > @@ -16,6 +16,7 @@ > > > > #include <libcamera/formats.h> > > > > #include <libcamera/ipa/raspberrypi.h> > > > > #include <libcamera/logging.h> > > > > +#include <libcamera/property_ids.h> > > > > #include <libcamera/request.h> > > > > #include <libcamera/stream.h> > > > > > > > > @@ -1174,6 +1175,13 @@ int RPiCameraData::configureIPA() > > > > ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig, > > > > nullptr); > > > > > > > > + /* Configure the H/V flip controls based on the sensor rotation. */ > > > > + ControlList ctrls(unicam_[Unicam::Image].dev()->controls()); > > > > + int32_t rotation = sensor_->properties().get(properties::Rotation); > > > > + ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation)); > > > > + ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation)); > > > > > > Are you missing a ">> 1" or similar in the above code? Otherwise, > > > both H/V FLIP will be always set to the same value. > > > > That's because the RPi IPA currently handles 0° and 180° rotations only, > > without separate control of h/v flip. We need to add h/v flip controls, > > and they will then be combined here in the pipeline handler. > > Do you think that's fine (in which case could you send me a R-b tag), or > are changes needed ? Sorry, thought I had already replied to this one. Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > > > > > + unicam_[Unicam::Image].dev()->setControls(&ctrls); > > > > + > > > > return 0; > > > > } > > > > > > > > @@ -1202,10 +1210,6 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData > > > > { V4L2_CID_EXPOSURE, action.data[1] } }); > > > > sensorMetadata_ = action.data[2]; > > > > } > > > > - > > > > - /* Set the sensor orientation here as well. */ > > > > - ControlList controls = action.controls[0]; > > > > - unicam_[Unicam::Image].dev()->setControls(&controls); > > > > return; > > > > } > > > > > > -- > Regards, > > Laurent Pinchart
Hi Naush, On Mon, Jul 06, 2020 at 04:02:17PM +0100, Naushir Patuck wrote: > Hi David and Jacopo, > > > On Mon, 6 Jul 2020 at 11:41, David Plowman > <david.plowman@raspberrypi.com> wrote: > > > > Hi Jacopo > > > > Thanks for the quick reply! Let me just clarify my understanding of > > your answers... > > > > On Mon, 6 Jul 2020 at 11:01, Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > > > Hi David, > > > > > > On Mon, Jul 06, 2020 at 10:29:21AM +0100, David Plowman wrote: > > > > Hi Jacopo, Laurent, everyone! > > > > > > > > I have a couple of questions. > > > > > > > > 1. Does this allow an application to set a transformation when the > > > > camera is started? For example, it's not uncommon for a horizontal > > > > flip to be requested from the camera depending on the application. > > > > Certainly in the Raspberry Pi world you can't tell a priori what a > > > > sensor's orientation is. > > > > > > > > > > Isn't this mostly related to allowing application provide a list of > > > controls at camera start/configure time ? Those will have to be taken > > > into account and matched with the camera sensor reported orientation > > > which is handled here I guess. > > > > So far as I'm aware there is no control for transformations (h/vflip, > > even transpose) currently, is that correct? I'm also not sure the > > usual request->controls() mechanism is the right one as it sort-of > > implies you can set it per frame, which I think is something we > > shouldn't allow. > > > > Perhaps it belongs in the StreamConfiguration? Some platforms might > > allow per-stream transformations, others (many?) will not. What do > > people think? > > > > > > > > > 2nd question below... > > > > > > > > On Mon, 6 Jul 2020 at 10:12, Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > > > > > > > Hi Laurent, > > > > > > > > > > On Sat, Jul 04, 2020 at 03:52:23AM +0300, Laurent Pinchart wrote: > > > > > > Instead of receiving sensor orientation configuration from the IPA, > > > > > > retrieve it from the CameraSensor Rotation property, and configure the > > > > > > HFLIP and VFLIP controls accordingly. > > > > > > > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > --- > > > > > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++---- > > > > > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > > > > > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > > > index 74bee6895dcd..fda6831e6a7e 100644 > > > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > > > @@ -16,6 +16,7 @@ > > > > > > #include <libcamera/formats.h> > > > > > > #include <libcamera/ipa/raspberrypi.h> > > > > > > #include <libcamera/logging.h> > > > > > > +#include <libcamera/property_ids.h> > > > > > > #include <libcamera/request.h> > > > > > > #include <libcamera/stream.h> > > > > > > > > > > > > @@ -1174,6 +1175,13 @@ int RPiCameraData::configureIPA() > > > > > > ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig, > > > > > > nullptr); > > > > > > > > > > > > + /* Configure the H/V flip controls based on the sensor rotation. */ > > > > > > + ControlList ctrls(unicam_[Unicam::Image].dev()->controls()); > > > > > > + int32_t rotation = sensor_->properties().get(properties::Rotation); > > > > > > + ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation)); > > > > > > + ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation)); > > > > > > > > > > mmm, I see in the IPA this, which is where I assume you got this from > > > > > > > > > > ctrls.set(V4L2_CID_HFLIP, (int32_t) !!(orientation & RPi::CamTransform_HFLIP)); > > > > > ctrls.set(V4L2_CID_VFLIP, (int32_t) !!(orientation & RPi::CamTransform_VFLIP)); > > > > > > > > > > Where the 'orientation' flag is retrieved from > > > > > RPi::CamTransform orientation = helper_->GetOrientation(); > > > > > > > > > > and expressed through members of this enumeration > > > > > static constexpr CamTransform CamTransform_IDENTITY = 0; > > > > > static constexpr CamTransform CamTransform_HFLIP = 1; > > > > > static constexpr CamTransform CamTransform_VFLIP = 2; > > > > > > > > > > Now, 'rotation' is expressed in multiple of 90 degrees in the [0-360[ > > > > > interval, shouldn't we: > > > > > if (rotation == 180) > > > > > HFLIP > > > > > > This should be VFLIP, as the control reads "mirror in the vertical > > > direction", not "along the horizontal axis" as I interpreted it > > > > > > > > if (rotation == 90 || roation == 270) > > > > > VFLIP > > > > > > And this is just wrong. Mirroring in H/V directions won't compensate > > > for such rotations. > > > > > > > > ? > > > > > > > > How are we handling transformations that aren't rotations, e.g. > > > > horizontal flip. As I said previously, it's a significant use case. > > > > > > I think they merit a control to let application handle them > > > I think this was meant to compensate the camera mounting rotation > > > only. > > > > > > > Also, how are we handling platforms that don't support things like > > > > 90/270 degree rotations - it's certain that many platforms won't be > > > > able to do that. > > > > > > That is a more generic question: how do pipeline handlers advertise > > > which control they support, or do you think H/V flips are different ? > > > > As I said above, I'm not sure the usual "controls" mechanism is the > > right one, and it's also not clear to me how a platform should > > advertise what it supports. (Do we have any mechanism for signalling > > that kind of thing?) Perhaps one could take the view that, for the > > time being, we just overwrite anything we don't handle and report the > > configuration as "adjusted"? > > > > (Perhaps I am hijacking the original discussion... in which case > > please say and I'll re-start it on its own.) > > If I am not mistaken, the pipeline handlers need to maintain a list of > controls supported, like in include/libcamera/ipa/raspberrypi.h > (RPiControls). This then gets exported to the CameraData base class > member controlInfo_. ARequest will not allow setting a > libcamera::control that is not listed in CameraData::controlInfo_ That's correct, it seems to me from this discussion, a ControlInfo could be augmented with information about a control being supported per-frame or just at at configuration time (or else...) > > Regards, > Naush > > > > > > Best regards > > David > > > > > > > > Thanks > > > j > > > > > > > > > > > Best regards > > > > David > > > > > > > > > > > > > > > > > > > > + unicam_[Unicam::Image].dev()->setControls(&ctrls); > > > > > > + > > > > > > return 0; > > > > > > } > > > > > > > > > > > > @@ -1202,10 +1210,6 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData > > > > > > { V4L2_CID_EXPOSURE, action.data[1] } }); > > > > > > sensorMetadata_ = action.data[2]; > > > > > > } > > > > > > - > > > > > > - /* Set the sensor orientation here as well. */ > > > > > > - ControlList controls = action.controls[0]; > > > > > > - unicam_[Unicam::Image].dev()->setControls(&controls); > > > > > > > > > > Do you think we can now remove the custom rotation handling from the > > > > > IPA helpers ? > > > > > > > > > > > return; > > > > > > } > > > > > > > > > > > > -- > > > > > > Regards, > > > > > > > > > > > > Laurent Pinchart > > > > > > > > > > > > _______________________________________________ > > > > > > libcamera-devel mailing list > > > > > > libcamera-devel@lists.libcamera.org > > > > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > > _______________________________________________ > > > > > libcamera-devel mailing list > > > > > libcamera-devel@lists.libcamera.org > > > > > https://lists.libcamera.org/listinfo/libcamera-devel > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo On Wed, 8 Jul 2020 at 22:58, Jacopo Mondi <jacopo@jmondi.org> wrote: > > Hi Naush, > > On Mon, Jul 06, 2020 at 04:02:17PM +0100, Naushir Patuck wrote: > > Hi David and Jacopo, > > > > > > On Mon, 6 Jul 2020 at 11:41, David Plowman > > <david.plowman@raspberrypi.com> wrote: > > > > > > Hi Jacopo > > > > > > Thanks for the quick reply! Let me just clarify my understanding of > > > your answers... > > > > > > On Mon, 6 Jul 2020 at 11:01, Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > > > > > Hi David, > > > > > > > > On Mon, Jul 06, 2020 at 10:29:21AM +0100, David Plowman wrote: > > > > > Hi Jacopo, Laurent, everyone! > > > > > > > > > > I have a couple of questions. > > > > > > > > > > 1. Does this allow an application to set a transformation when the > > > > > camera is started? For example, it's not uncommon for a horizontal > > > > > flip to be requested from the camera depending on the application. > > > > > Certainly in the Raspberry Pi world you can't tell a priori what a > > > > > sensor's orientation is. > > > > > > > > > > > > > Isn't this mostly related to allowing application provide a list of > > > > controls at camera start/configure time ? Those will have to be taken > > > > into account and matched with the camera sensor reported orientation > > > > which is handled here I guess. > > > > > > So far as I'm aware there is no control for transformations (h/vflip, > > > even transpose) currently, is that correct? I'm also not sure the > > > usual request->controls() mechanism is the right one as it sort-of > > > implies you can set it per frame, which I think is something we > > > shouldn't allow. > > > > > > Perhaps it belongs in the StreamConfiguration? Some platforms might > > > allow per-stream transformations, others (many?) will not. What do > > > people think? > > > > > > > > > > > > 2nd question below... > > > > > > > > > > On Mon, 6 Jul 2020 at 10:12, Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > > > > > > > > > Hi Laurent, > > > > > > > > > > > > On Sat, Jul 04, 2020 at 03:52:23AM +0300, Laurent Pinchart wrote: > > > > > > > Instead of receiving sensor orientation configuration from the IPA, > > > > > > > retrieve it from the CameraSensor Rotation property, and configure the > > > > > > > HFLIP and VFLIP controls accordingly. > > > > > > > > > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > > --- > > > > > > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++---- > > > > > > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > > > > > > > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > > > > index 74bee6895dcd..fda6831e6a7e 100644 > > > > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > > > > @@ -16,6 +16,7 @@ > > > > > > > #include <libcamera/formats.h> > > > > > > > #include <libcamera/ipa/raspberrypi.h> > > > > > > > #include <libcamera/logging.h> > > > > > > > +#include <libcamera/property_ids.h> > > > > > > > #include <libcamera/request.h> > > > > > > > #include <libcamera/stream.h> > > > > > > > > > > > > > > @@ -1174,6 +1175,13 @@ int RPiCameraData::configureIPA() > > > > > > > ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig, > > > > > > > nullptr); > > > > > > > > > > > > > > + /* Configure the H/V flip controls based on the sensor rotation. */ > > > > > > > + ControlList ctrls(unicam_[Unicam::Image].dev()->controls()); > > > > > > > + int32_t rotation = sensor_->properties().get(properties::Rotation); > > > > > > > + ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation)); > > > > > > > + ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation)); > > > > > > > > > > > > mmm, I see in the IPA this, which is where I assume you got this from > > > > > > > > > > > > ctrls.set(V4L2_CID_HFLIP, (int32_t) !!(orientation & RPi::CamTransform_HFLIP)); > > > > > > ctrls.set(V4L2_CID_VFLIP, (int32_t) !!(orientation & RPi::CamTransform_VFLIP)); > > > > > > > > > > > > Where the 'orientation' flag is retrieved from > > > > > > RPi::CamTransform orientation = helper_->GetOrientation(); > > > > > > > > > > > > and expressed through members of this enumeration > > > > > > static constexpr CamTransform CamTransform_IDENTITY = 0; > > > > > > static constexpr CamTransform CamTransform_HFLIP = 1; > > > > > > static constexpr CamTransform CamTransform_VFLIP = 2; > > > > > > > > > > > > Now, 'rotation' is expressed in multiple of 90 degrees in the [0-360[ > > > > > > interval, shouldn't we: > > > > > > if (rotation == 180) > > > > > > HFLIP > > > > > > > > This should be VFLIP, as the control reads "mirror in the vertical > > > > direction", not "along the horizontal axis" as I interpreted it > > > > > > > > > > if (rotation == 90 || roation == 270) > > > > > > VFLIP > > > > > > > > And this is just wrong. Mirroring in H/V directions won't compensate > > > > for such rotations. > > > > > > > > > > ? > > > > > > > > > > How are we handling transformations that aren't rotations, e.g. > > > > > horizontal flip. As I said previously, it's a significant use case. > > > > > > > > I think they merit a control to let application handle them > > > > I think this was meant to compensate the camera mounting rotation > > > > only. > > > > > > > > > Also, how are we handling platforms that don't support things like > > > > > 90/270 degree rotations - it's certain that many platforms won't be > > > > > able to do that. > > > > > > > > That is a more generic question: how do pipeline handlers advertise > > > > which control they support, or do you think H/V flips are different ? > > > > > > As I said above, I'm not sure the usual "controls" mechanism is the > > > right one, and it's also not clear to me how a platform should > > > advertise what it supports. (Do we have any mechanism for signalling > > > that kind of thing?) Perhaps one could take the view that, for the > > > time being, we just overwrite anything we don't handle and report the > > > configuration as "adjusted"? > > > > > > (Perhaps I am hijacking the original discussion... in which case > > > please say and I'll re-start it on its own.) > > > > If I am not mistaken, the pipeline handlers need to maintain a list of > > controls supported, like in include/libcamera/ipa/raspberrypi.h > > (RPiControls). This then gets exported to the CameraData base class > > member controlInfo_. ARequest will not allow setting a > > libcamera::control that is not listed in CameraData::controlInfo_ > > That's correct, it seems to me from this discussion, a ControlInfo > could be augmented with information about a control being supported > per-frame or just at at configuration time (or else...) That sounds like a good idea to me. Actually, I tried to start a new thread for this discussion (not very successfully, it seems!) so perhaps I can copy your comments onto that thread? But thanks for the feedback! Best regards David > > > > > Regards, > > Naush > > > > > > > > > > Best regards > > > David > > > > > > > > > > > Thanks > > > > j > > > > > > > > > > > > > > Best regards > > > > > David > > > > > > > > > > > > > > > > > > > > > > > > + unicam_[Unicam::Image].dev()->setControls(&ctrls); > > > > > > > + > > > > > > > return 0; > > > > > > > } > > > > > > > > > > > > > > @@ -1202,10 +1210,6 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData > > > > > > > { V4L2_CID_EXPOSURE, action.data[1] } }); > > > > > > > sensorMetadata_ = action.data[2]; > > > > > > > } > > > > > > > - > > > > > > > - /* Set the sensor orientation here as well. */ > > > > > > > - ControlList controls = action.controls[0]; > > > > > > > - unicam_[Unicam::Image].dev()->setControls(&controls); > > > > > > > > > > > > Do you think we can now remove the custom rotation handling from the > > > > > > IPA helpers ? > > > > > > > > > > > > > return; > > > > > > > } > > > > > > > > > > > > > > -- > > > > > > > Regards, > > > > > > > > > > > > > > Laurent Pinchart > > > > > > > > > > > > > > _______________________________________________ > > > > > > > libcamera-devel mailing list > > > > > > > libcamera-devel@lists.libcamera.org > > > > > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > > > _______________________________________________ > > > > > > libcamera-devel mailing list > > > > > > libcamera-devel@lists.libcamera.org > > > > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > _______________________________________________ > > > libcamera-devel mailing list > > > libcamera-devel@lists.libcamera.org > > > https://lists.libcamera.org/listinfo/libcamera-devel
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index 74bee6895dcd..fda6831e6a7e 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -16,6 +16,7 @@ #include <libcamera/formats.h> #include <libcamera/ipa/raspberrypi.h> #include <libcamera/logging.h> +#include <libcamera/property_ids.h> #include <libcamera/request.h> #include <libcamera/stream.h> @@ -1174,6 +1175,13 @@ int RPiCameraData::configureIPA() ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig, nullptr); + /* Configure the H/V flip controls based on the sensor rotation. */ + ControlList ctrls(unicam_[Unicam::Image].dev()->controls()); + int32_t rotation = sensor_->properties().get(properties::Rotation); + ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation)); + ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation)); + unicam_[Unicam::Image].dev()->setControls(&ctrls); + return 0; } @@ -1202,10 +1210,6 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData { V4L2_CID_EXPOSURE, action.data[1] } }); sensorMetadata_ = action.data[2]; } - - /* Set the sensor orientation here as well. */ - ControlList controls = action.controls[0]; - unicam_[Unicam::Image].dev()->setControls(&controls); return; }
Instead of receiving sensor orientation configuration from the IPA, retrieve it from the CameraSensor Rotation property, and configure the HFLIP and VFLIP controls accordingly. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)