[PATCH v1 7/7] pipeline: rpi: Handler controls::rpi::ScalerCrops
Naushir Patuck
naush at raspberrypi.com
Wed Aug 28 13:10:18 CEST 2024
Hi Jacopo,
Thanks for the review.
On Wed, 28 Aug 2024 at 11:07, Jacopo Mondi
<jacopo.mondi at ideasonboard.com> wrote:
>
> Hi Naush
>
> On Thu, Aug 08, 2024 at 11:23:46AM GMT, Naushir Patuck wrote:
> > Handle multiple scaler crops being set through the rpi::ScalerCrops
> > control. We now populate the cropParams_ map in the loop where we handle
> > the output stream configuration items. The key of this map is the index
> > of the stream configuration structure set by the application. This will
> > also be the same index used to specify the crop rectangles through the
> > ScalerCrops control.
> >
> > CameraData::applyScalerCrop() has been adapted to look at either
> > controls::ScalerCrop or controls::rpi::ScalerCrops. The former takes
> > priority over the latter, and if present, will apply the same scaler
> > crop to all output streams.
> >
> > Finally return all crops through the same ScalerCrops control via
> > request metadata. The first configure stream's crop rectangle is also
> > returned via the ScalerCrop control in the request metadata.
> >
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > ---
> > .../pipeline/rpi/common/pipeline_base.cpp | 69 +++++++++++++++----
> > 1 file changed, 56 insertions(+), 13 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > index a6ea4e9c47dd..b9759b682f0d 100644
> > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > @@ -561,11 +561,28 @@ int PipelineHandlerBase::configure(Camera *camera, CameraConfiguration *config)
> > for (auto const &c : result.controlInfo)
> > ctrlMap.emplace(c.first, c.second);
> >
> > - if (data->cropParams_.count(0)) {
> > - /* Add the ScalerCrop control limits based on the current mode. */
> > + if (data->cropParams_.size()) {
>
> This comment applies to the previous patch, but is there a case where
> data->cropParams_ is not populated at all ?
Both vc4 and pisp will populate this. I was being a bit too cautious
here perhaps. I'll remove the check.
>
> > + /*
> > + * Add the ScalerCrop control limits based on the current mode and
> > + * the first configured stream.
> > + */
> > Rectangle ispMinCrop = data->scaleIspCrop(Rectangle(data->cropParams_[0].ispMinCropSize));
> > ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, data->sensorInfo_.analogCrop,
> > data->scaleIspCrop(data->cropParams_[0].ispCrop));
>
> empty line maybe ?
ack
>
> > + if (data->cropParams_.size() == 2) {
> > + /*
> > + * The control map for rpi::ScalerCrops has the min value
> > + * as the default crop for stream 0, max value as the default
> > + * value for stream 1.
>
> mmm, I guess this is some internal agreement between pipeline_base and
> pipeline_pisp that initializes cropParams in this way, right ?
>
>
> > + */
> > + ctrlMap[&controls::rpi::ScalerCrops] =
> > + ControlInfo(data->scaleIspCrop(data->cropParams_[0].ispCrop),
> > + data->scaleIspCrop(data->cropParams_[1].ispCrop),
> > + ctrlMap[&controls::ScalerCrop].def());
> > + } else {
> > + /* Match the same ControlInfo for rpi::ScalerCrops. */
> > + ctrlMap[&controls::rpi::ScalerCrops] = ctrlMap[&controls::ScalerCrop];
> > + }
> > }
> >
> > data->controlInfo_ = ControlInfoMap(std::move(ctrlMap), result.controlInfo.idmap());
> > @@ -1292,10 +1309,29 @@ Rectangle CameraData::scaleIspCrop(const Rectangle &ispCrop) const
> >
> > void CameraData::applyScalerCrop(const ControlList &controls)
> > {
> > - const auto &scalerCrop = controls.get<Rectangle>(controls::ScalerCrop);
> > - if (scalerCrop && cropParams_.count(0)) {
> > - CropParams &cropParams = cropParams_[0];
> > - Rectangle nativeCrop = *scalerCrop;
> > + const auto &scalerCropRPi = controls.get<Span<const Rectangle>>(controls::rpi::ScalerCrops);
> > + const auto &scalerCropCore = controls.get<Rectangle>(controls::ScalerCrop);
>
> can both be specified in the same requests ? Does the usage of
> ScalerCrops make ScalerCrop invalid ?
If they are both specified in the same request,
controls::rpi::ScalerCrops will be used instead of
controls::ScalerCrop.
>
> It feels to me like pips should only register ScalerCrops and vc4 only
> ScalerCrop. Yes, the application has to adapt to this in this case,
> not sure how bad that would be
I think that would be workable.
>
> > + std::vector<Rectangle> scalerCrops;
> > +
> > + /*
> > + * First thing to do is create a vector of crops to apply to each ISP output
> > + * based on either controls::ScalerCrop or controls::rpi::ScalerCrops if
> > + * present.
> > + *
> > + * If controls::ScalerCrop is present, apply the same crop to all ISP output
> > + * streams. Otherwise if controls::rpi::ScalerCrops, apply the given crops
> > + * to the ISP output streams, indexed by the same order in which they had
> > + * been configured. This is not the same as the ISP output index.
> > + */
> > + for (unsigned int i = 0; i < cropParams_.size(); i++) {
> > + if (scalerCropRPi && i < scalerCropRPi->size())
> > + scalerCrops.push_back(scalerCropRPi->data()[i]);
> > + else if (scalerCropCore)
> > + scalerCrops.push_back(*scalerCropCore);
> > + }
> > +
> > + for (auto const &[i, scalerCrop] : utils::enumerate(scalerCrops)) {
> > + Rectangle nativeCrop = scalerCrop;
> >
> > if (!nativeCrop.width || !nativeCrop.height)
> > nativeCrop = { 0, 0, 1, 1 };
> > @@ -1311,13 +1347,13 @@ void CameraData::applyScalerCrop(const ControlList &controls)
> > * 2. With the same mid-point, if possible.
> > * 3. But it can't go outside the sensor area.
> > */
> > - Size minSize = cropParams.ispMinCropSize.expandedToAspectRatio(nativeCrop.size());
> > + Size minSize = cropParams_[i].ispMinCropSize.expandedToAspectRatio(nativeCrop.size());
> > Size size = ispCrop.size().expandedTo(minSize);
> > ispCrop = size.centeredTo(ispCrop.center()).enclosedIn(Rectangle(sensorInfo_.outputSize));
> >
> > - if (ispCrop != cropParams.ispCrop) {
> > - cropParams.ispCrop = ispCrop;
> > - platformSetIspCrop(cropParams.ispIndex, ispCrop);
> > + if (ispCrop != cropParams_[i].ispCrop) {
> > + cropParams_[i].ispCrop = ispCrop;
> > + platformSetIspCrop(cropParams_[i].ispIndex, ispCrop);
>
> You can really just pass cropsParams_[i] to platformSetIspCrop. It
> contains the index and the updated ispCrop.
Ack.
>
> > }
> > }
> > }
> > @@ -1474,9 +1510,16 @@ void CameraData::fillRequestMetadata(const ControlList &bufferControls, Request
> > request->metadata().set(controls::SensorTimestamp,
> > bufferControls.get(controls::SensorTimestamp).value_or(0));
> >
> > - if (cropParams_.count(0))
> > - request->metadata().set(controls::ScalerCrop,
> > - scaleIspCrop(cropParams_[0].ispCrop));
> > + if (cropParams_.size()) {
>
> Again I might have missed in which case cropParams_ is not populated
> at all
As above, I can remove the check.
Regards,
Naush
>
> > + std::vector<Rectangle> crops;
> > +
> > + for (auto const &[k, v] : cropParams_)
> > + crops.push_back(scaleIspCrop(v.ispCrop));
> > +
> > + request->metadata().set(controls::ScalerCrop, crops[0]);
> > + request->metadata().set(controls::rpi::ScalerCrops,
> > + Span<const Rectangle>(crops.data(), crops.size()));
> > + }
> > }
> >
> > } /* namespace libcamera */
> > --
> > 2.34.1
> >
More information about the libcamera-devel
mailing list