| Message ID | 20200829115429.30010-7-david.plowman@raspberrypi.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi David, Thank you for the patch. On Sat, Aug 29, 2020 at 12:54:27PM +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. > > Note that the validate() method has to work out what the final Bayer > order of any raw streams will be, before configure() actually applies > the transform to the sensor. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > --- > .../pipeline/raspberrypi/raspberrypi.cpp | 82 ++++++++++++++++--- > 1 file changed, 71 insertions(+), 11 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index c554532..333aa94 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -294,7 +294,7 @@ public: > void frameStarted(uint32_t sequence); > > int loadIPA(); > - int configureIPA(); > + int configureIPA(CameraConfiguration *config); > > void queueFrameAction(unsigned int frame, const IPAOperationData &action); > > @@ -400,8 +400,34 @@ 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; > } Shouldn't thus logic handle the case where the sensor can't flip ? I think we can keep it simple that considering that either both or neither flips are supported. > > @@ -414,13 +440,42 @@ CameraConfiguration::Status RPiCameraConfiguration::validate() > * Calculate the best sensor mode we can use based on > * the user request. > */ > - V4L2VideoDevice::Formats fmts = data_->unicam_[Unicam::Image].dev()->formats(); > + V4L2VideoDevice *dev = data_->unicam_[Unicam::Image].dev(); > + V4L2VideoDevice::Formats fmts = dev->formats(); > V4L2DeviceFormat sensorFormat = findBestMode(fmts, cfg.size); > - int ret = data_->unicam_[Unicam::Image].dev()->tryFormat(&sensorFormat); > + int ret = dev->tryFormat(&sensorFormat); > if (ret) > return Invalid; > > - PixelFormat sensorPixFormat = sensorFormat.fourcc.toPixelFormat(); > + /* > + * Some sensors change their Bayer order when they are > + * h-flipped or v-flipped, according to the transform. > + * If the controls own up to "modifying the layout" we > + * will assume that's what is going on and advertise > + * the transformed Bayer order in the stream. > + */ > + V4L2PixelFormat fourcc = sensorFormat.fourcc; > + > + /* > + * The camera may already be transformed, so we must > + * only transform the fourcc if the new transform is > + * different. > + */ > + ControlList ctrls = dev->getControls({ V4L2_CID_HFLIP, V4L2_CID_VFLIP }); > + > + const struct v4l2_query_ext_ctrl *hflipCtrl = dev->queryControl(V4L2_CID_HFLIP); > + if (hflipCtrl && > + (hflipCtrl->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) && > + ctrls.get(V4L2_CID_HFLIP).get<int32_t>() != !!(combined & Transform::HFlip)) > + fourcc = fourcc.transform(Transform::HFlip); > + > + const struct v4l2_query_ext_ctrl *vflipCtrl = dev->queryControl(V4L2_CID_VFLIP); > + if (vflipCtrl && > + (vflipCtrl->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) && > + ctrls.get(V4L2_CID_VFLIP).get<int32_t>() != !!(combined & Transform::VFlip)) > + fourcc = fourcc.transform(Transform::VFlip); To simplify this, would it make sense to reset the h/v flip controls at init time, and then read and store the bayer format ? That would be the native bayer format, and we would only need to care about the V4L2_CTRL_FLAG_MODIFY_LAYOUT flags here. If we consider it safe to assume that V4L2_CTRL_FLAG_MODIFY_LAYOUT will be set for either both or neither flip controls, the code would be simplified to V4L2PixelFormat fourcc = data_->sensorFormat_; const struct v4l2_query_ext_ctrl *hflipCtrl = dev->queryControl(V4L2_CID_HFLIP); const struct v4l2_query_ext_ctrl *vflipCtrl = dev->queryControl(V4L2_CID_VFLIP); if (hflipCtrl && vflipCtrl && (hflipCtrl->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) && (vflipCtrl->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT)) fourcc = fourcc.transform(combined); > + > + PixelFormat sensorPixFormat = fourcc.toPixelFormat(); > if (cfg.size != sensorFormat.size || > cfg.pixelFormat != sensorPixFormat) { > cfg.size = sensorFormat.size; > @@ -756,7 +811,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config) > crop.y = (sensorFormat.size.height - crop.height) >> 1; > data->isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop); > > - ret = data->configureIPA(); > + ret = data->configureIPA(config); > if (ret) > LOG(RPI, Error) << "Failed to configure the IPA: " << ret; > > @@ -1112,7 +1167,7 @@ int RPiCameraData::loadIPA() > return ipa_->init(settings); > } > > -int RPiCameraData::configureIPA() > +int RPiCameraData::configureIPA(CameraConfiguration *config) > { > std::map<unsigned int, IPAStream> streamConfig; > std::map<unsigned int, const ControlInfoMap &> entityControls; > @@ -1170,11 +1225,16 @@ int RPiCameraData::configureIPA() > sensorMetadata_ = result.data[2]; > } > > - /* Configure the H/V flip controls based on the sensor rotation. */ > + /* > + * Configure the H/V flip controls based on the sensor rotation > + * and user transform. > + */ > 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)); > + /* The rotation angle was already checked in validate(). */ > + Transform combined = config->transform * transformFromRotation(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))); > unicam_[Unicam::Image].dev()->setControls(&ctrls); > } >
Hi David, Another small comment. On Tue, Sep 01, 2020 at 03:42:14AM +0300, Laurent Pinchart wrote: > On Sat, Aug 29, 2020 at 12:54:27PM +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. > > > > Note that the validate() method has to work out what the final Bayer > > order of any raw streams will be, before configure() actually applies > > the transform to the sensor. > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > --- > > .../pipeline/raspberrypi/raspberrypi.cpp | 82 ++++++++++++++++--- > > 1 file changed, 71 insertions(+), 11 deletions(-) > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index c554532..333aa94 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -294,7 +294,7 @@ public: > > void frameStarted(uint32_t sequence); > > > > int loadIPA(); > > - int configureIPA(); > > + int configureIPA(CameraConfiguration *config); > > > > void queueFrameAction(unsigned int frame, const IPAOperationData &action); > > > > @@ -400,8 +400,34 @@ 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; > > } > > Shouldn't thus logic handle the case where the sensor can't flip ? I > think we can keep it simple that considering that either both or neither > flips are supported. > > > > > @@ -414,13 +440,42 @@ CameraConfiguration::Status RPiCameraConfiguration::validate() > > * Calculate the best sensor mode we can use based on > > * the user request. > > */ > > - V4L2VideoDevice::Formats fmts = data_->unicam_[Unicam::Image].dev()->formats(); > > + V4L2VideoDevice *dev = data_->unicam_[Unicam::Image].dev(); > > + V4L2VideoDevice::Formats fmts = dev->formats(); > > V4L2DeviceFormat sensorFormat = findBestMode(fmts, cfg.size); > > - int ret = data_->unicam_[Unicam::Image].dev()->tryFormat(&sensorFormat); > > + int ret = dev->tryFormat(&sensorFormat); > > if (ret) > > return Invalid; > > > > - PixelFormat sensorPixFormat = sensorFormat.fourcc.toPixelFormat(); > > + /* > > + * Some sensors change their Bayer order when they are > > + * h-flipped or v-flipped, according to the transform. > > + * If the controls own up to "modifying the layout" we > > + * will assume that's what is going on and advertise > > + * the transformed Bayer order in the stream. > > + */ > > + V4L2PixelFormat fourcc = sensorFormat.fourcc; > > + > > + /* > > + * The camera may already be transformed, so we must > > + * only transform the fourcc if the new transform is > > + * different. > > + */ > > + ControlList ctrls = dev->getControls({ V4L2_CID_HFLIP, V4L2_CID_VFLIP }); > > + > > + const struct v4l2_query_ext_ctrl *hflipCtrl = dev->queryControl(V4L2_CID_HFLIP); > > + if (hflipCtrl && > > + (hflipCtrl->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) && > > + ctrls.get(V4L2_CID_HFLIP).get<int32_t>() != !!(combined & Transform::HFlip)) > > + fourcc = fourcc.transform(Transform::HFlip); > > + > > + const struct v4l2_query_ext_ctrl *vflipCtrl = dev->queryControl(V4L2_CID_VFLIP); > > + if (vflipCtrl && > > + (vflipCtrl->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) && > > + ctrls.get(V4L2_CID_VFLIP).get<int32_t>() != !!(combined & Transform::VFlip)) > > + fourcc = fourcc.transform(Transform::VFlip); > > To simplify this, would it make sense to reset the h/v flip controls at > init time, and then read and store the bayer format ? That would be the > native bayer format, and we would only need to care about the > V4L2_CTRL_FLAG_MODIFY_LAYOUT flags here. > > If we consider it safe to assume that V4L2_CTRL_FLAG_MODIFY_LAYOUT will > be set for either both or neither flip controls, the code would be > simplified to > > V4L2PixelFormat fourcc = data_->sensorFormat_; > > const struct v4l2_query_ext_ctrl *hflipCtrl = dev->queryControl(V4L2_CID_HFLIP); > const struct v4l2_query_ext_ctrl *vflipCtrl = dev->queryControl(V4L2_CID_VFLIP); > > if (hflipCtrl && vflipCtrl && > (hflipCtrl->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) && > (vflipCtrl->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT)) > fourcc = fourcc.transform(combined); > > > + > > + PixelFormat sensorPixFormat = fourcc.toPixelFormat(); > > if (cfg.size != sensorFormat.size || > > cfg.pixelFormat != sensorPixFormat) { > > cfg.size = sensorFormat.size; > > @@ -756,7 +811,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config) > > crop.y = (sensorFormat.size.height - crop.height) >> 1; > > data->isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop); > > > > - ret = data->configureIPA(); > > + ret = data->configureIPA(config); > > if (ret) > > LOG(RPI, Error) << "Failed to configure the IPA: " << ret; > > > > @@ -1112,7 +1167,7 @@ int RPiCameraData::loadIPA() > > return ipa_->init(settings); > > } > > > > -int RPiCameraData::configureIPA() > > +int RPiCameraData::configureIPA(CameraConfiguration *config) > > { > > std::map<unsigned int, IPAStream> streamConfig; > > std::map<unsigned int, const ControlInfoMap &> entityControls; > > @@ -1170,11 +1225,16 @@ int RPiCameraData::configureIPA() > > sensorMetadata_ = result.data[2]; > > } > > > > - /* Configure the H/V flip controls based on the sensor rotation. */ > > + /* There's a trailing white space here. > > + * Configure the H/V flip controls based on the sensor rotation > > + * and user transform. > > + */ > > 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)); > > + /* The rotation angle was already checked in validate(). */ > > + Transform combined = config->transform * transformFromRotation(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))); > > unicam_[Unicam::Image].dev()->setControls(&ctrls); > > } > >
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index c554532..333aa94 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -294,7 +294,7 @@ public: void frameStarted(uint32_t sequence); int loadIPA(); - int configureIPA(); + int configureIPA(CameraConfiguration *config); void queueFrameAction(unsigned int frame, const IPAOperationData &action); @@ -400,8 +400,34 @@ 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; } @@ -414,13 +440,42 @@ CameraConfiguration::Status RPiCameraConfiguration::validate() * Calculate the best sensor mode we can use based on * the user request. */ - V4L2VideoDevice::Formats fmts = data_->unicam_[Unicam::Image].dev()->formats(); + V4L2VideoDevice *dev = data_->unicam_[Unicam::Image].dev(); + V4L2VideoDevice::Formats fmts = dev->formats(); V4L2DeviceFormat sensorFormat = findBestMode(fmts, cfg.size); - int ret = data_->unicam_[Unicam::Image].dev()->tryFormat(&sensorFormat); + int ret = dev->tryFormat(&sensorFormat); if (ret) return Invalid; - PixelFormat sensorPixFormat = sensorFormat.fourcc.toPixelFormat(); + /* + * Some sensors change their Bayer order when they are + * h-flipped or v-flipped, according to the transform. + * If the controls own up to "modifying the layout" we + * will assume that's what is going on and advertise + * the transformed Bayer order in the stream. + */ + V4L2PixelFormat fourcc = sensorFormat.fourcc; + + /* + * The camera may already be transformed, so we must + * only transform the fourcc if the new transform is + * different. + */ + ControlList ctrls = dev->getControls({ V4L2_CID_HFLIP, V4L2_CID_VFLIP }); + + const struct v4l2_query_ext_ctrl *hflipCtrl = dev->queryControl(V4L2_CID_HFLIP); + if (hflipCtrl && + (hflipCtrl->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) && + ctrls.get(V4L2_CID_HFLIP).get<int32_t>() != !!(combined & Transform::HFlip)) + fourcc = fourcc.transform(Transform::HFlip); + + const struct v4l2_query_ext_ctrl *vflipCtrl = dev->queryControl(V4L2_CID_VFLIP); + if (vflipCtrl && + (vflipCtrl->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) && + ctrls.get(V4L2_CID_VFLIP).get<int32_t>() != !!(combined & Transform::VFlip)) + fourcc = fourcc.transform(Transform::VFlip); + + PixelFormat sensorPixFormat = fourcc.toPixelFormat(); if (cfg.size != sensorFormat.size || cfg.pixelFormat != sensorPixFormat) { cfg.size = sensorFormat.size; @@ -756,7 +811,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config) crop.y = (sensorFormat.size.height - crop.height) >> 1; data->isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop); - ret = data->configureIPA(); + ret = data->configureIPA(config); if (ret) LOG(RPI, Error) << "Failed to configure the IPA: " << ret; @@ -1112,7 +1167,7 @@ int RPiCameraData::loadIPA() return ipa_->init(settings); } -int RPiCameraData::configureIPA() +int RPiCameraData::configureIPA(CameraConfiguration *config) { std::map<unsigned int, IPAStream> streamConfig; std::map<unsigned int, const ControlInfoMap &> entityControls; @@ -1170,11 +1225,16 @@ int RPiCameraData::configureIPA() sensorMetadata_ = result.data[2]; } - /* Configure the H/V flip controls based on the sensor rotation. */ + /* + * Configure the H/V flip controls based on the sensor rotation + * and user transform. + */ 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)); + /* The rotation angle was already checked in validate(). */ + Transform combined = config->transform * transformFromRotation(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))); unicam_[Unicam::Image].dev()->setControls(&ctrls); }
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. Note that the validate() method has to work out what the final Bayer order of any raw streams will be, before configure() actually applies the transform to the sensor. Signed-off-by: David Plowman <david.plowman@raspberrypi.com> --- .../pipeline/raspberrypi/raspberrypi.cpp | 82 ++++++++++++++++--- 1 file changed, 71 insertions(+), 11 deletions(-)