[PATCH v3 7/7] pipeline: rpi: Handler controls::rpi::ScalerCrops
Naushir Patuck
naush at raspberrypi.com
Tue Oct 29 09:58:46 CET 2024
Hi Laurent,
On Mon, 28 Oct 2024 at 22:35, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Naush,
>
> Thank you for the patch.
>
> On Thu, Oct 03, 2024 at 08:47:20AM +0100, 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.
>
> That contradicts patch 1/7 that states
>
> If both rpi::ScalerCrops and ScalerCrop controls are present in a
> ControlList, the latter is discarded, and crops are obtained from this
> control.
>
> and also contradicts the code below.
Yes, this is an error in the commit message. I'll update this to
reflect that rpi::ScalerCrops take precedence.
>
> >
> > 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>
> > Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> > ---
> > .../pipeline/rpi/common/pipeline_base.cpp | 76 ++++++++++++++-----
> > 1 file changed, 59 insertions(+), 17 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > index 267e6bd9cd70..eba4dad6d212 100644
> > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > @@ -181,12 +181,14 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
> >
> > rawStreams_.clear();
> > outStreams_.clear();
> > + unsigned int rawStreamIndex = 0;
> > + unsigned int outStreamIndex = 0;
> >
> > - for (const auto &[index, cfg] : utils::enumerate(config_)) {
> > + for (auto &cfg : config_) {
> > if (PipelineHandlerBase::isRaw(cfg.pixelFormat))
> > - rawStreams_.emplace_back(index, &cfg);
> > + rawStreams_.emplace_back(rawStreamIndex++, &cfg);
> > else
> > - outStreams_.emplace_back(index, &cfg);
> > + outStreams_.emplace_back(outStreamIndex++, &cfg);
>
> This changes the indices, but as far as I can tell, they're currently
> unused, so that should be fine. Do I assume correctly the will be used
> in the Pi 5 platformConfigure() implementation to populate cropParams_
> in the right order ?
That's correct, the VC4 pipeline handler doesn't use this index, it's
only for PISP where the control is used.
>
> > }
> >
> > /* Sort the streams so the highest resolution is first. */
> > @@ -565,10 +567,24 @@ int PipelineHandlerBase::configure(Camera *camera, CameraConfiguration *config)
> > const auto cropParamsIt = data->cropParams_.find(0);
> > if (cropParamsIt != data->cropParams_.end()) {
> > const CameraData::CropParams &cropParams = cropParamsIt->second;
> > - /* Add the ScalerCrop control limits based on the current mode. */
> > + /*
> > + * Add the ScalerCrop control limits based on the current mode and
> > + * the first configured stream.
> > + */
> > Rectangle ispMinCrop = data->scaleIspCrop(Rectangle(cropParams.ispMinCropSize));
> > ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, data->sensorInfo_.analogCrop,
> > data->scaleIspCrop(cropParams.ispCrop));
> > + 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.
>
> That sounds like a real hack. I'm fine with it for now, but should it be
> documented in 1/7 ? Or would you rather not have anyone depending on
> this ?
100% a hack. I'd be happy to see us get rid of this asap when we have
real per-stream controls. I intend on working on this next...
>
> > + */
> > + ctrlMap[&controls::rpi::ScalerCrops] =
> > + ControlInfo(data->scaleIspCrop(data->cropParams_.at(0).ispCrop),
> > + data->scaleIspCrop(data->cropParams_.at(1).ispCrop),
> > + ctrlMap[&controls::ScalerCrop].def());
> > + }
> > }
> >
> > data->controlInfo_ = ControlInfoMap(std::move(ctrlMap), result.controlInfo.idmap());
> > @@ -1295,11 +1311,29 @@ Rectangle CameraData::scaleIspCrop(const Rectangle &ispCrop) const
> >
> > void CameraData::applyScalerCrop(const ControlList &controls)
> > {
> > - const auto &scalerCrop = controls.get<Rectangle>(controls::ScalerCrop);
> > - const auto cropParamsIt = cropParams_.find(0);
> > - if (scalerCrop && cropParamsIt != cropParams_.end()) {
> > - Rectangle nativeCrop = *scalerCrop;
> > - CropParams &cropParams = cropParamsIt->second;
> > + const auto &scalerCropRPi = controls.get<Span<const Rectangle>>(controls::rpi::ScalerCrops);
> > + const auto &scalerCropCore = controls.get<Rectangle>(controls::ScalerCrop);
> > + 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.
>
> This also contradicts the code below. You can pick the begaviour you
> want, but let's make documentation, comments, commit messages and code
> consistent.
As above, I'll fix up the comment here as well.
Regards,
Naush
>
> > + */
> > + 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 };
> > @@ -1315,13 +1349,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_.at(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_.at(i).ispCrop) {
> > + cropParams_.at(i).ispCrop = ispCrop;
> > + platformSetIspCrop(cropParams_.at(i).ispIndex, ispCrop);
> > }
> > }
> > }
> > @@ -1478,10 +1512,18 @@ void CameraData::fillRequestMetadata(const ControlList &bufferControls, Request
> > request->metadata().set(controls::SensorTimestamp,
> > bufferControls.get(controls::SensorTimestamp).value_or(0));
> >
> > - const auto cropParamsIt = cropParams_.find(0);
> > - if (cropParamsIt != cropParams_.end())
> > - request->metadata().set(controls::ScalerCrop,
> > - scaleIspCrop(cropParamsIt->second.ispCrop));
> > + if (cropParams_.size()) {
> > + std::vector<Rectangle> crops;
> > +
> > + for (auto const &[k, v] : cropParams_)
> > + crops.push_back(scaleIspCrop(v.ispCrop));
> > +
> > + request->metadata().set(controls::ScalerCrop, crops[0]);
> > + if (crops.size() > 1) {
> > + 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