| Message ID | 20241216154124.203650-12-stefan.klug@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Stefan On Mon, Dec 16, 2024 at 04:40:51PM +0100, Stefan Klug wrote: > ScalerCrop is specified as being in sensor coordinates. The current > dewarper implementation on the imx8mp handles ScalerCrop in dewarper > coordinates. This leads to unexpected results and an 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> > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > Changes in v3: > - Rename dewarpSensorCrop_ to scalerMaxCrop_ Why not scalerCropMax_ ? :) The rest, according to my memories of your clarifications seems good to me Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > - Remove unnecessary ScalerCrop max calculation > > Changes in v2: > - Initialize dewarperSensorCrop_ to sane defaults > - Collect tags > --- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 50 +++++++++++++++++++----- > 1 file changed, 41 insertions(+), 9 deletions(-) > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index 56192451eb3c..ef4aa38478f5 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -205,6 +205,7 @@ private: > RkISP1SelfPath selfPath_; > > std::unique_ptr<V4L2M2MConverter> dewarper_; > + Rectangle scalerMaxCrop_; > bool useDewarper_; > > std::optional<Rectangle> activeCrop_; > @@ -861,6 +862,15 @@ 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. > + */ > + scalerMaxCrop_ = > + outputCrop.transformedBetween(inputCrop, > + sensorInfo.analogCrop); > } > } else if (hasSelfPath_) { > ret = selfPath_.configure(cfg, format); > @@ -1226,10 +1236,19 @@ int PipelineHandlerRkISP1::updateControls(RkISP1CameraData *data) > else > cropLimits = dewarper_->inputCropBounds(); > > - 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.transformedBetween(cropLimits.second, > + scalerMaxCrop_); > + > + controls[&controls::ScalerCrop] = ControlInfo(min, > + scalerMaxCrop_, > + scalerMaxCrop_); > + activeCrop_ = scalerMaxCrop_; > } > > /* Add the IPA registered controls to list of camera controls. */ > @@ -1257,6 +1276,8 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor) > /* Initialize the camera properties. */ > data->properties_ = data->sensor_->properties(); > > + scalerMaxCrop_ = Rectangle(data->sensor_->resolution()); > + > const CameraSensorProperties::SensorDelays &delays = data->sensor_->sensorDelays(); > std::unordered_map<uint32_t, DelayedControls::ControlParams> params = { > { V4L2_CID_ANALOGUE_GAIN, { delays.gainDelay, false } }, > @@ -1476,22 +1497,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.transformedBetween(scalerMaxCrop_, cropLimits.second); > int ret = dewarper_->setInputCrop(&data->mainPathStream_, > - &appliedRect); > - if (!ret && appliedRect != crop.value()) { > + &rect); > + rect = rect.transformedBetween(cropLimits.second, scalerMaxCrop_); > + 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 Jacopo, Thank you for the review. On Mon, Dec 16, 2024 at 07:29:56PM +0100, Jacopo Mondi wrote: > Hi Stefan > > On Mon, Dec 16, 2024 at 04:40:51PM +0100, Stefan Klug wrote: > > ScalerCrop is specified as being in sensor coordinates. The current > > dewarper implementation on the imx8mp handles ScalerCrop in dewarper > > coordinates. This leads to unexpected results and an 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> > > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > > > > --- > > Changes in v3: > > - Rename dewarpSensorCrop_ to scalerMaxCrop_ > > Why not scalerCropMax_ ? > :) Haha... I actually thought about renaming it to scalerCropMax_ because I also like it better. But in Message-ID: <3i57xvgmtl7ey26isg5t7xubghj26pitm37szvyq6q5ingi7wz@d2udte44eqsq> you proposed scalerMaxCrop_ and I confirmed it afterwards and then didn't want to bring that up again :-). Good to know that we actually preferred the same. Let's leave it like that for now. In the upcoming dewarper series the variable will be removed anyways. > > The rest, according to my memories of your clarifications seems good > to me > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks, Stefan > > Thanks > j > > > - Remove unnecessary ScalerCrop max calculation > > > > Changes in v2: > > - Initialize dewarperSensorCrop_ to sane defaults > > - Collect tags > > --- > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 50 +++++++++++++++++++----- > > 1 file changed, 41 insertions(+), 9 deletions(-) > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > index 56192451eb3c..ef4aa38478f5 100644 > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > @@ -205,6 +205,7 @@ private: > > RkISP1SelfPath selfPath_; > > > > std::unique_ptr<V4L2M2MConverter> dewarper_; > > + Rectangle scalerMaxCrop_; > > bool useDewarper_; > > > > std::optional<Rectangle> activeCrop_; > > @@ -861,6 +862,15 @@ 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. > > + */ > > + scalerMaxCrop_ = > > + outputCrop.transformedBetween(inputCrop, > > + sensorInfo.analogCrop); > > } > > } else if (hasSelfPath_) { > > ret = selfPath_.configure(cfg, format); > > @@ -1226,10 +1236,19 @@ int PipelineHandlerRkISP1::updateControls(RkISP1CameraData *data) > > else > > cropLimits = dewarper_->inputCropBounds(); > > > > - 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.transformedBetween(cropLimits.second, > > + scalerMaxCrop_); > > + > > + controls[&controls::ScalerCrop] = ControlInfo(min, > > + scalerMaxCrop_, > > + scalerMaxCrop_); > > + activeCrop_ = scalerMaxCrop_; > > } > > > > /* Add the IPA registered controls to list of camera controls. */ > > @@ -1257,6 +1276,8 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor) > > /* Initialize the camera properties. */ > > data->properties_ = data->sensor_->properties(); > > > > + scalerMaxCrop_ = Rectangle(data->sensor_->resolution()); > > + > > const CameraSensorProperties::SensorDelays &delays = data->sensor_->sensorDelays(); > > std::unordered_map<uint32_t, DelayedControls::ControlParams> params = { > > { V4L2_CID_ANALOGUE_GAIN, { delays.gainDelay, false } }, > > @@ -1476,22 +1497,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.transformedBetween(scalerMaxCrop_, cropLimits.second); > > int ret = dewarper_->setInputCrop(&data->mainPathStream_, > > - &appliedRect); > > - if (!ret && appliedRect != crop.value()) { > > + &rect); > > + rect = rect.transformedBetween(cropLimits.second, scalerMaxCrop_); > > + 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 > >
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp index 56192451eb3c..ef4aa38478f5 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp @@ -205,6 +205,7 @@ private: RkISP1SelfPath selfPath_; std::unique_ptr<V4L2M2MConverter> dewarper_; + Rectangle scalerMaxCrop_; bool useDewarper_; std::optional<Rectangle> activeCrop_; @@ -861,6 +862,15 @@ 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. + */ + scalerMaxCrop_ = + outputCrop.transformedBetween(inputCrop, + sensorInfo.analogCrop); } } else if (hasSelfPath_) { ret = selfPath_.configure(cfg, format); @@ -1226,10 +1236,19 @@ int PipelineHandlerRkISP1::updateControls(RkISP1CameraData *data) else cropLimits = dewarper_->inputCropBounds(); - 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.transformedBetween(cropLimits.second, + scalerMaxCrop_); + + controls[&controls::ScalerCrop] = ControlInfo(min, + scalerMaxCrop_, + scalerMaxCrop_); + activeCrop_ = scalerMaxCrop_; } /* Add the IPA registered controls to list of camera controls. */ @@ -1257,6 +1276,8 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor) /* Initialize the camera properties. */ data->properties_ = data->sensor_->properties(); + scalerMaxCrop_ = Rectangle(data->sensor_->resolution()); + const CameraSensorProperties::SensorDelays &delays = data->sensor_->sensorDelays(); std::unordered_map<uint32_t, DelayedControls::ControlParams> params = { { V4L2_CID_ANALOGUE_GAIN, { delays.gainDelay, false } }, @@ -1476,22 +1497,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.transformedBetween(scalerMaxCrop_, cropLimits.second); int ret = dewarper_->setInputCrop(&data->mainPathStream_, - &appliedRect); - if (!ret && appliedRect != crop.value()) { + &rect); + rect = rect.transformedBetween(cropLimits.second, scalerMaxCrop_); + 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; } /*