| Message ID | 20241120085753.1993436-7-stefan.klug@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
On Wed, Nov 20, 2024 at 09:57:45AM +0100, Stefan Klug wrote: > ScalerCrop is specified as beeing in sensor coordinates. The current s/beeing/being/ > dewarper implementation on the imx8mp handles ScalerCrop in dewarper > coordinates. This leads to unexpected results and a unusable ScalerCrop s/ a / an / > control in camshark. Fix that by transforming back and forth between > sensor coordinates and dewarper coordinates. > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> Looks good. Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > --- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 49 +++++++++++++++++++----- > 1 file changed, 40 insertions(+), 9 deletions(-) > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index 4a226d9b809f..c2ce38b1c253 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -204,6 +204,7 @@ private: > RkISP1SelfPath selfPath_; > > std::unique_ptr<V4L2M2MConverter> dewarper_; > + Rectangle dewarperSensorCrop_; > bool useDewarper_; > > std::optional<Rectangle> activeCrop_; > @@ -862,6 +863,14 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > outputCfgs.push_back(const_cast<StreamConfiguration &>(cfg)); > ret = dewarper_->configure(cfg, outputCfgs); > useDewarper_ = ret ? false : true; > + > + /* > + * Calculate the crop rectangle of the data > + * flowing into the dewarper in sensor > + * coordinates. > + */ > + dewarperSensorCrop_ = outputCrop.mappedBetween(inputCrop, > + ipaConfig.sensorInfo.analogCrop); > } > } else if (hasSelfPath_) { > ret = selfPath_.configure(cfg, format); > @@ -1224,10 +1233,21 @@ int PipelineHandlerRkISP1::updateControls(RkISP1CameraData *data) > std::pair<Rectangle, Rectangle> cropLimits = > dewarper_->inputCropBounds(&data->mainPathStream_); > > - controls[&controls::ScalerCrop] = ControlInfo(cropLimits.first, > - cropLimits.second, > - cropLimits.second); > - activeCrop_ = cropLimits.second; > + /* > + * ScalerCrop is specified to be in Sensor coordinates. > + * So we need to transform the limits to sensor coordinates. > + * We can safely assume that the maximum crop limit contains the > + * full fov of the dewarper. > + */ > + Rectangle min = cropLimits.first.mappedBetween(cropLimits.second, > + dewarperSensorCrop_); > + Rectangle max = cropLimits.second.mappedBetween(cropLimits.second, > + dewarperSensorCrop_); > + > + controls[&controls::ScalerCrop] = ControlInfo(min, > + max, > + max); > + activeCrop_ = max; > } > > /* Add the IPA registered controls to list of camera controls. */ > @@ -1478,22 +1498,33 @@ void PipelineHandlerRkISP1::imageBufferReady(FrameBuffer *buffer) > /* Handle scaler crop control. */ > const auto &crop = request->controls().get(controls::ScalerCrop); > if (crop) { > - Rectangle appliedRect = crop.value(); > + Rectangle rect = crop.value(); > + > + /* > + * ScalerCrop is specified to be in Sensor coordinates. > + * So we need to transform it into dewarper coordinates. > + * We can safely assume that the maximum crop limit contains the > + * full fov of the dewarper. > + */ > + std::pair<Rectangle, Rectangle> cropLimits = > + dewarper_->inputCropBounds(&data->mainPathStream_); > > + rect = rect.mappedBetween(dewarperSensorCrop_, cropLimits.second); > int ret = dewarper_->setInputCrop(&data->mainPathStream_, > - &appliedRect); > - if (!ret && appliedRect != crop.value()) { > + &rect); > + rect = rect.mappedBetween(cropLimits.second, dewarperSensorCrop_); > + if (!ret && rect != crop.value()) { > /* > * If the rectangle is changed by setInputCrop on the > * dewarper, log a debug message and cache the actual > * applied rectangle for metadata reporting. > */ > LOG(RkISP1, Debug) > - << "Applied rectangle " << appliedRect.toString() > + << "Applied rectangle " << rect.toString() > << " differs from requested " << crop.value().toString(); > } > > - activeCrop_ = appliedRect; > + activeCrop_ = rect; > } > > /* > -- > 2.43.0 >
Hi Stefan On Wed, Nov 20, 2024 at 09:57:45AM +0100, Stefan Klug wrote: > ScalerCrop is specified as beeing in sensor coordinates. The current > dewarper implementation on the imx8mp handles ScalerCrop in dewarper > coordinates. This leads to unexpected results and a unusable ScalerCrop > control in camshark. Fix that by transforming back and forth between > sensor coordinates and dewarper coordinates. > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > --- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 49 +++++++++++++++++++----- > 1 file changed, 40 insertions(+), 9 deletions(-) > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index 4a226d9b809f..c2ce38b1c253 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -204,6 +204,7 @@ private: > RkISP1SelfPath selfPath_; > > std::unique_ptr<V4L2M2MConverter> dewarper_; > + Rectangle dewarperSensorCrop_; > bool useDewarper_; > > std::optional<Rectangle> activeCrop_; > @@ -862,6 +863,14 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > outputCfgs.push_back(const_cast<StreamConfiguration &>(cfg)); > ret = dewarper_->configure(cfg, outputCfgs); > useDewarper_ = ret ? false : true; > + > + /* > + * Calculate the crop rectangle of the data > + * flowing into the dewarper in sensor > + * coordinates. > + */ > + dewarperSensorCrop_ = outputCrop.mappedBetween(inputCrop, > + ipaConfig.sensorInfo.analogCrop); ScalerCrop is expressed in sensor native coordinates, not in respect to the current analogCrop ? > } > } else if (hasSelfPath_) { > ret = selfPath_.configure(cfg, format); > @@ -1224,10 +1233,21 @@ int PipelineHandlerRkISP1::updateControls(RkISP1CameraData *data) > std::pair<Rectangle, Rectangle> cropLimits = > dewarper_->inputCropBounds(&data->mainPathStream_); Looking at the implementation of V4L2M2MConverter::inputCropBound() it seems to me that inputCropBounds_ is only initialized after a configure(). As updateControls() is called by PipelineHandlerRkISP1::createCamera(), how are these fields initialized ? Also dewarperSensorCrop_ needs to be initialized to some default ? > > - controls[&controls::ScalerCrop] = ControlInfo(cropLimits.first, > - cropLimits.second, > - cropLimits.second); > - activeCrop_ = cropLimits.second; > + /* > + * ScalerCrop is specified to be in Sensor coordinates. > + * So we need to transform the limits to sensor coordinates. > + * We can safely assume that the maximum crop limit contains the > + * full fov of the dewarper. > + */ > + Rectangle min = cropLimits.first.mappedBetween(cropLimits.second, What is the reference system of the dewarper crop bound rectangle ? Shouldn't it be the size of the format configured in its sink pad ? Or is it what is returned in cropLimits.second already ? (It seems so, as it is the result of TGT_CROP_BOUNDS) > + dewarperSensorCrop_); > + Rectangle max = cropLimits.second.mappedBetween(cropLimits.second, > + dewarperSensorCrop_); > + > + controls[&controls::ScalerCrop] = ControlInfo(min, > + max, > + max); > + activeCrop_ = max; > } > > /* Add the IPA registered controls to list of camera controls. */ > @@ -1478,22 +1498,33 @@ void PipelineHandlerRkISP1::imageBufferReady(FrameBuffer *buffer) > /* Handle scaler crop control. */ > const auto &crop = request->controls().get(controls::ScalerCrop); > if (crop) { > - Rectangle appliedRect = crop.value(); > + Rectangle rect = crop.value(); > + > + /* > + * ScalerCrop is specified to be in Sensor coordinates. > + * So we need to transform it into dewarper coordinates. > + * We can safely assume that the maximum crop limit contains the > + * full fov of the dewarper. > + */ > + std::pair<Rectangle, Rectangle> cropLimits = > + dewarper_->inputCropBounds(&data->mainPathStream_); > > + rect = rect.mappedBetween(dewarperSensorCrop_, cropLimits.second); > int ret = dewarper_->setInputCrop(&data->mainPathStream_, > - &appliedRect); > - if (!ret && appliedRect != crop.value()) { > + &rect); > + rect = rect.mappedBetween(cropLimits.second, dewarperSensorCrop_); > + if (!ret && rect != crop.value()) { > /* > * If the rectangle is changed by setInputCrop on the > * dewarper, log a debug message and cache the actual > * applied rectangle for metadata reporting. > */ > LOG(RkISP1, Debug) > - << "Applied rectangle " << appliedRect.toString() > + << "Applied rectangle " << rect.toString() > << " differs from requested " << crop.value().toString(); > } > > - activeCrop_ = appliedRect; > + activeCrop_ = rect; I guess the only real question is what is the system's state before configure(). Thanks j > } > > /* > -- > 2.43.0 >
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp index 4a226d9b809f..c2ce38b1c253 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp @@ -204,6 +204,7 @@ private: RkISP1SelfPath selfPath_; std::unique_ptr<V4L2M2MConverter> dewarper_; + Rectangle dewarperSensorCrop_; bool useDewarper_; std::optional<Rectangle> activeCrop_; @@ -862,6 +863,14 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) outputCfgs.push_back(const_cast<StreamConfiguration &>(cfg)); ret = dewarper_->configure(cfg, outputCfgs); useDewarper_ = ret ? false : true; + + /* + * Calculate the crop rectangle of the data + * flowing into the dewarper in sensor + * coordinates. + */ + dewarperSensorCrop_ = outputCrop.mappedBetween(inputCrop, + ipaConfig.sensorInfo.analogCrop); } } else if (hasSelfPath_) { ret = selfPath_.configure(cfg, format); @@ -1224,10 +1233,21 @@ int PipelineHandlerRkISP1::updateControls(RkISP1CameraData *data) std::pair<Rectangle, Rectangle> cropLimits = dewarper_->inputCropBounds(&data->mainPathStream_); - controls[&controls::ScalerCrop] = ControlInfo(cropLimits.first, - cropLimits.second, - cropLimits.second); - activeCrop_ = cropLimits.second; + /* + * ScalerCrop is specified to be in Sensor coordinates. + * So we need to transform the limits to sensor coordinates. + * We can safely assume that the maximum crop limit contains the + * full fov of the dewarper. + */ + Rectangle min = cropLimits.first.mappedBetween(cropLimits.second, + dewarperSensorCrop_); + Rectangle max = cropLimits.second.mappedBetween(cropLimits.second, + dewarperSensorCrop_); + + controls[&controls::ScalerCrop] = ControlInfo(min, + max, + max); + activeCrop_ = max; } /* Add the IPA registered controls to list of camera controls. */ @@ -1478,22 +1498,33 @@ void PipelineHandlerRkISP1::imageBufferReady(FrameBuffer *buffer) /* Handle scaler crop control. */ const auto &crop = request->controls().get(controls::ScalerCrop); if (crop) { - Rectangle appliedRect = crop.value(); + Rectangle rect = crop.value(); + + /* + * ScalerCrop is specified to be in Sensor coordinates. + * So we need to transform it into dewarper coordinates. + * We can safely assume that the maximum crop limit contains the + * full fov of the dewarper. + */ + std::pair<Rectangle, Rectangle> cropLimits = + dewarper_->inputCropBounds(&data->mainPathStream_); + rect = rect.mappedBetween(dewarperSensorCrop_, cropLimits.second); int ret = dewarper_->setInputCrop(&data->mainPathStream_, - &appliedRect); - if (!ret && appliedRect != crop.value()) { + &rect); + rect = rect.mappedBetween(cropLimits.second, dewarperSensorCrop_); + if (!ret && rect != crop.value()) { /* * If the rectangle is changed by setInputCrop on the * dewarper, log a debug message and cache the actual * applied rectangle for metadata reporting. */ LOG(RkISP1, Debug) - << "Applied rectangle " << appliedRect.toString() + << "Applied rectangle " << rect.toString() << " differs from requested " << crop.value().toString(); } - activeCrop_ = appliedRect; + activeCrop_ = rect; } /*
ScalerCrop is specified as beeing in sensor coordinates. The current dewarper implementation on the imx8mp handles ScalerCrop in dewarper coordinates. This leads to unexpected results and a unusable ScalerCrop control in camshark. Fix that by transforming back and forth between sensor coordinates and dewarper coordinates. Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> --- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 49 +++++++++++++++++++----- 1 file changed, 40 insertions(+), 9 deletions(-)