[PATCH v1 7/7] pipeline: rpi: Handler controls::rpi::ScalerCrops
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Aug 28 19:01:54 CEST 2024
Hello,
On Wed, Aug 28, 2024 at 12:10:18PM +0100, Naushir Patuck wrote:
> On Wed, 28 Aug 2024 at 11:07, Jacopo Mondi wrote:
> > 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.
This should be documented in the controls::rpi::ScalerCrops definition.
> > >
> > > 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 ?
Could this be handled in a bit cleaner way ?
> > > + */
> > > + 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.
This contradicts the commit message which specifies the opposite order.
> > 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.
>
> > > + 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 */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list