| Message ID | 20200828144110.17303-6-david.plowman@raspberrypi.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi David, Thank you for the patch. On Fri, Aug 28, 2020 at 03:41:08PM +0100, David Plowman wrote: > The Raspberry Pi pipeline handler allows all transforms except those > involving a transpose. The user transform is combined with any > inherent rotation of the camera, and the camera's H and V flip bits > are set accordingly. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > --- > .../pipeline/raspberrypi/raspberrypi.cpp | 42 +++++++++++++++---- > 1 file changed, 34 insertions(+), 8 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index dc36f53..6ea1432 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -400,20 +400,46 @@ CameraConfiguration::Status RPiCameraConfiguration::validate() > if (config_.empty()) > return Invalid; > > - if (transform != Transform::Identity) { > - transform = Transform::Identity; > + /* > + * What if the platform has a non-90 degree rotation? We can't even > + * "adjust" the configuration and carry on. Alternatively, raising an > + * error means the platform can never run. Let's just print a warning > + * and continue regardless; the rotation is effectively set to zero. > + */ > + int32_t rotation = data_->sensor_->properties().get(properties::Rotation); > + bool success; > + Transform combined = transform * transformFromRotation(rotation, &success); > + if (!success) > + LOG(RPI, Warning) << "Invalid rotation of " << rotation > + << " degrees - ignoring"; > + > + /* > + * We combine the platform and user transform, but must "adjust away" > + * any combined result that includes a transform, as we can't do those. > + * In this case, flipping only the transpose bit is helpful to > + * applications - they either get the transform they requested, or have > + * to do a simple transpose themselves (they don't have to worry about > + * the other possible cases). > + */ > + if (!!(combined & Transform::Transpose)) { > + /* > + * Flipping the transpose bit in "transform" flips it in > + * combined result too (as it's the last thing that happens). > + */ > + transform ^= Transform::Transpose; > + combined ^= Transform::Transpose; > status = Adjusted; > } > > /* > - * Configure the H/V flip controls based on the sensor rotation. We do > - * this here so that the sensor has the correct Bayer format that will > - * get advertised in the configuration of any raw streams. > + * Configure the H/V flip controls based on the combination of the > + * sensor rotation and the user transform. We do this here so that the > + * sensor has the correct Bayer format that will get advertised in the > + * configuration of any raw streams. > */ > ControlList ctrls(data_->unicam_[Unicam::Image].dev()->controls()); > - int32_t rotation = data_->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)); > + ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!(combined & Transform::HFlip))); > + ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!(combined & Transform::VFlip))); > data_->unicam_[Unicam::Image].dev()->setControls(&ctrls); The logic looks good, but as discussed elsewhere in the thread, this should be done in configure(). > > unsigned int rawCount = 0, outCount = 0, count = 0, maxIndex = 0;
Hi David, On 28/08/2020 15:41, David Plowman wrote: > The Raspberry Pi pipeline handler allows all transforms except those > involving a transpose. The user transform is combined with any > inherent rotation of the camera, and the camera's H and V flip bits > are set accordingly. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > --- > .../pipeline/raspberrypi/raspberrypi.cpp | 42 +++++++++++++++---- > 1 file changed, 34 insertions(+), 8 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index dc36f53..6ea1432 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -400,20 +400,46 @@ CameraConfiguration::Status RPiCameraConfiguration::validate() > if (config_.empty()) > return Invalid; > > - if (transform != Transform::Identity) { > - transform = Transform::Identity; > + /* > + * What if the platform has a non-90 degree rotation? We can't even > + * "adjust" the configuration and carry on. Alternatively, raising an > + * error means the platform can never run. Let's just print a warning > + * and continue regardless; the rotation is effectively set to zero. > + */ Should we clamp to the 'nearest' value? I don't imagine it's helpful either way ... > + int32_t rotation = data_->sensor_->properties().get(properties::Rotation); > + bool success; > + Transform combined = transform * transformFromRotation(rotation, &success); > + if (!success) > + LOG(RPI, Warning) << "Invalid rotation of " << rotation > + << " degrees - ignoring"; > + > + /* > + * We combine the platform and user transform, but must "adjust away" > + * any combined result that includes a transform, as we can't do those. > + * In this case, flipping only the transpose bit is helpful to > + * applications - they either get the transform they requested, or have > + * to do a simple transpose themselves (they don't have to worry about > + * the other possible cases). > + */ > + if (!!(combined & Transform::Transpose)) { Are the !! operators required here, does the underlying value not represent something that can be interpreted as boolean otherwise? I don't think I object to the double ! operator, just curious about the 'requirement' of it. > + /* > + * Flipping the transpose bit in "transform" flips it in > + * combined result too (as it's the last thing that happens). > + */ > + transform ^= Transform::Transpose; > + combined ^= Transform::Transpose; > status = Adjusted; > } > > /* > - * Configure the H/V flip controls based on the sensor rotation. We do > - * this here so that the sensor has the correct Bayer format that will > - * get advertised in the configuration of any raw streams. > + * Configure the H/V flip controls based on the combination of the > + * sensor rotation and the user transform. We do this here so that the > + * sensor has the correct Bayer format that will get advertised in the > + * configuration of any raw streams. > */ > ControlList ctrls(data_->unicam_[Unicam::Image].dev()->controls()); > - int32_t rotation = data_->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)); > + ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!(combined & Transform::HFlip))); > + ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!(combined & Transform::VFlip))); > data_->unicam_[Unicam::Image].dev()->setControls(&ctrls); Ah, that hflip/vflip is much more readable now :-) As highlighted from Laurent though, setting this in validate feels quite wrong to me. I'm not sure if a stream could be actively running while we validate a configuration, but in theory I think that's the aim - you should be able to have an active stream - and validate the next configuration without interfering with a currently active stream... (Perhaps for example, while determining if a reconfiguration is possible, without tearing down the stream until you have a valid set of parameters to apply). > > unsigned int rawCount = 0, outCount = 0, count = 0, maxIndex = 0; >
Hi Kieran, On Fri, Aug 28, 2020 at 04:56:17PM +0100, Kieran Bingham wrote: > On 28/08/2020 15:41, David Plowman wrote: > > The Raspberry Pi pipeline handler allows all transforms except those > > involving a transpose. The user transform is combined with any > > inherent rotation of the camera, and the camera's H and V flip bits > > are set accordingly. > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > --- > > .../pipeline/raspberrypi/raspberrypi.cpp | 42 +++++++++++++++---- > > 1 file changed, 34 insertions(+), 8 deletions(-) > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index dc36f53..6ea1432 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -400,20 +400,46 @@ CameraConfiguration::Status RPiCameraConfiguration::validate() > > if (config_.empty()) > > return Invalid; > > > > - if (transform != Transform::Identity) { > > - transform = Transform::Identity; > > + /* > > + * What if the platform has a non-90 degree rotation? We can't even > > + * "adjust" the configuration and carry on. Alternatively, raising an > > + * error means the platform can never run. Let's just print a warning > > + * and continue regardless; the rotation is effectively set to zero. > > + */ > > Should we clamp to the 'nearest' value? I don't imagine it's helpful > either way ... I don't think we support non-90° rotations anywhere, so I wouldn't worry about them. If we had to support that, I think we'd need a different control to report a 3D pose (translation and rotation) of the sensor. > > + int32_t rotation = data_->sensor_->properties().get(properties::Rotation); > > + bool success; > > + Transform combined = transform * transformFromRotation(rotation, &success); > > + if (!success) > > + LOG(RPI, Warning) << "Invalid rotation of " << rotation > > + << " degrees - ignoring"; > > + > > + /* > > + * We combine the platform and user transform, but must "adjust away" > > + * any combined result that includes a transform, as we can't do those. > > + * In this case, flipping only the transpose bit is helpful to > > + * applications - they either get the transform they requested, or have > > + * to do a simple transpose themselves (they don't have to worry about > > + * the other possible cases). > > + */ > > + if (!!(combined & Transform::Transpose)) { > > Are the !! operators required here, does the underlying value not > represent something that can be interpreted as boolean otherwise? Unfortunately not, it's an enum class, not an enum. > I don't think I object to the double ! operator, just curious about the > 'requirement' of it. > > > + /* > > + * Flipping the transpose bit in "transform" flips it in > > + * combined result too (as it's the last thing that happens). > > + */ > > + transform ^= Transform::Transpose; > > + combined ^= Transform::Transpose; > > status = Adjusted; > > } > > > > /* > > - * Configure the H/V flip controls based on the sensor rotation. We do > > - * this here so that the sensor has the correct Bayer format that will > > - * get advertised in the configuration of any raw streams. > > + * Configure the H/V flip controls based on the combination of the > > + * sensor rotation and the user transform. We do this here so that the > > + * sensor has the correct Bayer format that will get advertised in the > > + * configuration of any raw streams. > > */ > > ControlList ctrls(data_->unicam_[Unicam::Image].dev()->controls()); > > - int32_t rotation = data_->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)); > > + ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!(combined & Transform::HFlip))); > > + ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!(combined & Transform::VFlip))); > > data_->unicam_[Unicam::Image].dev()->setControls(&ctrls); > > Ah, that hflip/vflip is much more readable now :-) > > As highlighted from Laurent though, setting this in validate feels quite > wrong to me. > > I'm not sure if a stream could be actively running while we validate a > configuration, but in theory I think that's the aim - you should be able > to have an active stream - and validate the next configuration without > interfering with a currently active stream... > > (Perhaps for example, while determining if a reconfiguration is > possible, without tearing down the stream until you have a valid set of > parameters to apply). > > > > > unsigned int rawCount = 0, outCount = 0, count = 0, maxIndex = 0; > >
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index dc36f53..6ea1432 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -400,20 +400,46 @@ CameraConfiguration::Status RPiCameraConfiguration::validate() if (config_.empty()) return Invalid; - if (transform != Transform::Identity) { - transform = Transform::Identity; + /* + * What if the platform has a non-90 degree rotation? We can't even + * "adjust" the configuration and carry on. Alternatively, raising an + * error means the platform can never run. Let's just print a warning + * and continue regardless; the rotation is effectively set to zero. + */ + int32_t rotation = data_->sensor_->properties().get(properties::Rotation); + bool success; + Transform combined = transform * transformFromRotation(rotation, &success); + if (!success) + LOG(RPI, Warning) << "Invalid rotation of " << rotation + << " degrees - ignoring"; + + /* + * We combine the platform and user transform, but must "adjust away" + * any combined result that includes a transform, as we can't do those. + * In this case, flipping only the transpose bit is helpful to + * applications - they either get the transform they requested, or have + * to do a simple transpose themselves (they don't have to worry about + * the other possible cases). + */ + if (!!(combined & Transform::Transpose)) { + /* + * Flipping the transpose bit in "transform" flips it in + * combined result too (as it's the last thing that happens). + */ + transform ^= Transform::Transpose; + combined ^= Transform::Transpose; status = Adjusted; } /* - * Configure the H/V flip controls based on the sensor rotation. We do - * this here so that the sensor has the correct Bayer format that will - * get advertised in the configuration of any raw streams. + * Configure the H/V flip controls based on the combination of the + * sensor rotation and the user transform. We do this here so that the + * sensor has the correct Bayer format that will get advertised in the + * configuration of any raw streams. */ ControlList ctrls(data_->unicam_[Unicam::Image].dev()->controls()); - int32_t rotation = data_->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)); + ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!(combined & Transform::HFlip))); + ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!(combined & Transform::VFlip))); data_->unicam_[Unicam::Image].dev()->setControls(&ctrls); unsigned int rawCount = 0, outCount = 0, count = 0, maxIndex = 0;
The Raspberry Pi pipeline handler allows all transforms except those involving a transpose. The user transform is combined with any inherent rotation of the camera, and the camera's H and V flip bits are set accordingly. Signed-off-by: David Plowman <david.plowman@raspberrypi.com> --- .../pipeline/raspberrypi/raspberrypi.cpp | 42 +++++++++++++++---- 1 file changed, 34 insertions(+), 8 deletions(-)