[PATCH v2 4/7] pipeline: rpi: Introduce CameraData::CropParams
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Tue Oct 1 20:23:02 CEST 2024
Hi Naush
On Tue, Oct 01, 2024 at 12:12:46PM GMT, Naushir Patuck wrote:
> Hi Jacopo,
>
> On Tue, 1 Oct 2024 at 07:30, Jacopo Mondi <jacopo.mondi at ideasonboard.com> wrote:
> >
> > Hi Naush,
> > one more nit
> >
> > On Mon, Sep 30, 2024 at 03:14:12PM GMT, Naushir Patuck wrote:
> > > In preparation for assigning separate crop windows for each stream, add
> > > a new CropParams structure that stores the existing ispCrop_ and
> > > ispMinCropSize_ as fields. Use a new std::map to store a CropParams
> > > structure where the map key is the index of the stream configuration in
> > > the CameraConfiguration vector.
> > >
> > > At preset, only a single CropParams structure will be set at key == 0 to
> > > preserve the existing crop handling logic.
> > >
> > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > ---
> > > .../pipeline/rpi/common/pipeline_base.cpp | 28 +++++++++++++------
> > > .../pipeline/rpi/common/pipeline_base.h | 21 ++++++++++++--
> > > src/libcamera/pipeline/rpi/vc4/vc4.cpp | 12 ++++++--
> > > 3 files changed, 47 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > index 2de6111bacfd..220c7b962280 100644
> > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > @@ -533,6 +533,7 @@ int PipelineHandlerBase::configure(Camera *camera, CameraConfiguration *config)
> > > * Platform specific internal stream configuration. This also assigns
> > > * external streams which get configured below.
> > > */
> > > + data->cropParams_.clear();
> > > ret = data->platformConfigure(rpiConfig);
> > > if (ret)
> > > return ret;
> > > @@ -561,10 +562,14 @@ int PipelineHandlerBase::configure(Camera *camera, CameraConfiguration *config)
> > > for (auto const &c : result.controlInfo)
> > > ctrlMap.emplace(c.first, c.second);
> > >
> > > - /* Add the ScalerCrop control limits based on the current mode. */
> > > - Rectangle ispMinCrop = data->scaleIspCrop(Rectangle(data->ispMinCropSize_));
> > > - ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, data->sensorInfo_.analogCrop,
> > > - data->scaleIspCrop(data->ispCrop_));
> > > + 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. */
> > > + Rectangle ispMinCrop = data->scaleIspCrop(Rectangle(cropParams.ispMinCropSize));
> > > + ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, data->sensorInfo_.analogCrop,
> > > + data->scaleIspCrop(cropParams.ispCrop));
> > > + }
> > >
> > > data->controlInfo_ = ControlInfoMap(std::move(ctrlMap), result.controlInfo.idmap());
> > >
> > > @@ -1291,8 +1296,10 @@ Rectangle CameraData::scaleIspCrop(const Rectangle &ispCrop) const
> > > void CameraData::applyScalerCrop(const ControlList &controls)
> > > {
> > > const auto &scalerCrop = controls.get<Rectangle>(controls::ScalerCrop);
> > > - if (scalerCrop) {
> > > + const auto cropParamsIt = cropParams_.find(0);
> > > + if (scalerCrop && cropParamsIt != cropParams_.end()) {
> > > Rectangle nativeCrop = *scalerCrop;
> > > + CropParams &cropParams = cropParamsIt->second;
> > >
> > > if (!nativeCrop.width || !nativeCrop.height)
> > > nativeCrop = { 0, 0, 1, 1 };
> > > @@ -1308,12 +1315,12 @@ 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 = ispMinCropSize_.expandedToAspectRatio(nativeCrop.size());
> > > + Size minSize = cropParams.ispMinCropSize.expandedToAspectRatio(nativeCrop.size());
> > > Size size = ispCrop.size().expandedTo(minSize);
> > > ispCrop = size.centeredTo(ispCrop.center()).enclosedIn(Rectangle(sensorInfo_.outputSize));
> > >
> > > - if (ispCrop != ispCrop_) {
> > > - ispCrop_ = ispCrop;
> > > + if (ispCrop != cropParams.ispCrop) {
> > > + cropParams.ispCrop = ispCrop;
> > > platformSetIspCrop(ispCrop);
> > > }
> > > }
> > > @@ -1471,7 +1478,10 @@ void CameraData::fillRequestMetadata(const ControlList &bufferControls, Request
> > > request->metadata().set(controls::SensorTimestamp,
> > > bufferControls.get(controls::SensorTimestamp).value_or(0));
> > >
> > > - request->metadata().set(controls::ScalerCrop, scaleIspCrop(ispCrop_));
> > > + const auto cropParamsIt = cropParams_.find(0);
> > > + if (cropParamsIt != cropParams_.end())
> > > + request->metadata().set(controls::ScalerCrop,
> > > + scaleIspCrop(cropParamsIt->second.ispCrop));
> > > }
> > >
> > > } /* namespace libcamera */
> > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.h b/src/libcamera/pipeline/rpi/common/pipeline_base.h
> > > index d65b695c30b5..75babbe17f31 100644
> > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.h
> > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h
> > > @@ -133,8 +133,25 @@ public:
> > >
> > > /* For handling digital zoom. */
> > > IPACameraSensorInfo sensorInfo_;
> > > - Rectangle ispCrop_; /* crop in ISP (camera mode) pixels */
> > > - Size ispMinCropSize_;
> > > +
> > > + struct CropParams {
> > > + CropParams(Rectangle ispCrop_, Size ispMinCropSize_)
> > > + : ispCrop(ispCrop_), ispMinCropSize(ispMinCropSize_)
> > > + {
> > > + }
> > > +
> > > + CropParams()
> > > + {
> > > + }
> >
> > You probably don't need this ?
>
> Yes, I can get rid of this. I will have to change from cropParams_[i]
> to cropParams_.at(i) in the code since it's a std::map. But I think
> that's a good idea anyway!
Ah right
../src/libcamera/pipeline/rpi/common/pipeline_base.cpp:577:74: required from here
577 | Rectangle ispMinCrop = data->scaleIspCrop(Rectangle(data->cropParams_[0].ispMinCropSize));
| ^
/usr/include/c++/14/tuple:2888:9: error: no matching function for call to ‘libcamera::RPi::CameraData::CropParams::CropParams()’
2888 | second(std::forward<_Args2>(std::get<_Indexes2>(__tuple2))...)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
This will prevent erroneously creating non-initialized instances.
However std::vector::at() might throw an exception
Maybe
CropParams() = default;
it's not that bad after all :)
>
> Regards,
> Naush
>
> >
> > Thanks
> > j
> >
> > > +
> > > + /* Crop in ISP (camera mode) pixels */
> > > + Rectangle ispCrop;
> > > + /* Minimum crop size in ISP output pixels */
> > > + Size ispMinCropSize;
> > > + };
> > > +
> > > + /* Mapping of CropParams keyed by the output stream order in CameraConfiguration */
> > > + std::map<unsigned int, CropParams> cropParams_;
> > >
> > > unsigned int dropFrameCount_;
> > >
> > > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> > > index 0ea032293bc9..8080f55a1cf4 100644
> > > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> > > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> > > @@ -702,13 +702,19 @@ int Vc4CameraData::platformConfigure(const RPi::RPiCameraConfiguration *rpiConfi
> > > /* Figure out the smallest selection the ISP will allow. */
> > > Rectangle testCrop(0, 0, 1, 1);
> > > isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &testCrop);
> > > - ispMinCropSize_ = testCrop.size();
> > >
> > > /* Adjust aspect ratio by providing crops on the input image. */
> > > Size size = unicamFormat.size.boundedToAspectRatio(maxSize);
> > > - ispCrop_ = size.centeredTo(Rectangle(unicamFormat.size).center());
> > > + Rectangle ispCrop = size.centeredTo(Rectangle(unicamFormat.size).center());
> > >
> > > - platformSetIspCrop(ispCrop_);
> > > + platformSetIspCrop(ispCrop);
> > > + /*
> > > + * Set the scaler crop to the value we are using (scaled to native sensor
> > > + * coordinates).
> > > + */
> > > + cropParams_.emplace(std::piecewise_construct,
> > > + std::forward_as_tuple(0),
> > > + std::forward_as_tuple(ispCrop, testCrop.size()));
> > >
> > > return 0;
> > > }
> > > --
> > > 2.34.1
> > >
More information about the libcamera-devel
mailing list