[PATCH 3/7] mali-c55: implement support for ScalerCrop
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Mon Jun 24 17:01:42 CEST 2024
Hi Laurent
On Mon, Jun 17, 2024 at 08:11:33AM GMT, Laurent Pinchart wrote:
> On Thu, Jun 13, 2024 at 08:33:11PM +0100, Kieran Bingham wrote:
> > Quoting Daniel Scally (2024-06-13 16:59:45)
> > > From: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> > >
> > > Implement support for the ScalerCrop control that allows to apply a
> > > digital zoom to the captured streams.
> > >
> > > Initialize the camera controls at camera registration time and update
> > > them at configure time as the sensor's analogue crop size might change
> > > depending on the desired Camera configuration.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> > > ---
> > > src/libcamera/pipeline/mali-c55/mali-c55.cpp | 138 ++++++++++++++++++-
> > > 1 file changed, 137 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> > > index c4f1afbc..2c34f3e9 100644
> > > --- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> > > +++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> > > @@ -100,6 +100,8 @@ public:
> > >
> > > PixelFormat bestRawFormat() const;
> > >
> > > + void updateControls();
> > > +
> > > PixelFormat adjustRawFormat(const PixelFormat &pixFmt) const;
> > > Size adjustRawSizes(const PixelFormat &pixFmt, const Size &rawSize) const;
> > >
> > > @@ -245,6 +247,27 @@ PixelFormat MaliC55CameraData::bestRawFormat() const
> > > return rawFormat;
> > > }
> > >
> > > +void MaliC55CameraData::updateControls()
> > > +{
> > > + if (!sensor_)
> > > + return;
> > > +
> > > + IPACameraSensorInfo sensorInfo;
> > > + int ret = sensor_->sensorInfo(&sensorInfo);
> > > + if (ret) {
> > > + LOG(MaliC55, Error) << "Failed to retrieve sensor info";
> > > + return;
> > > + }
> > > +
> > > + ControlInfoMap::Map controls;
> > > + Rectangle ispMinCrop{ 0, 0, 640, 480 };
> >
> > This looks like it should be a constant somewhere outside of here?
> > Perhaps class constant or just a constant at the top of the file.
> >
ack
> > > + controls[&controls::ScalerCrop] =
> > > + ControlInfo(ispMinCrop, sensorInfo.analogCrop,
> > > + sensorInfo.analogCrop);
> > > +
> > > + controlInfo_ = ControlInfoMap(std::move(controls), controls::controls);
> > > +}
> > > +
> > > /*
> > > * Make sure the provided raw pixel format is supported and adjust it to
> > > * one of the supported ones if it's not.
> > > @@ -515,6 +538,8 @@ private:
> > > const StreamConfiguration &config,
> > > V4L2SubdeviceFormat &subdevFormat);
> > >
> > > + void applyScalerCrop(Camera *camera, const ControlList &controls);
> > > +
> > > void registerMaliCamera(std::unique_ptr<MaliC55CameraData> data,
> > > const std::string &name);
> > > bool registerTPGCamera(MediaLink *link);
> > > @@ -828,6 +853,8 @@ int PipelineHandlerMaliC55::configure(Camera *camera,
> > > pipe->stream = stream;
> > > }
> > >
> > > + data->updateControls();
> > > +
> > > return 0;
> > > }
> > >
> > > @@ -875,6 +902,104 @@ void PipelineHandlerMaliC55::stopDevice([[maybe_unused]] Camera *camera)
> > > }
> > > }
> > >
> > > +void PipelineHandlerMaliC55::applyScalerCrop(Camera *camera,
> > > + const ControlList &controls)
> > > +{
> > > + MaliC55CameraData *data = cameraData(camera);
> > > +
> > > + const auto &scalerCrop = controls.get<Rectangle>(controls::ScalerCrop);
> > > + if (!scalerCrop)
> > > + return;
> > > +
> > > + if (!data->sensor_) {
> > > + LOG(MaliC55, Error) << "ScalerCrop not supported for TPG";
> > > + return;
> > > + }
> > > +
> > > + Rectangle nativeCrop = *scalerCrop;
> > > +
> > > + IPACameraSensorInfo sensorInfo;
> > > + int ret = data->sensor_->sensorInfo(&sensorInfo);
>
> Gathering the sensor info is a bit expensive, it should be cached.
>
I can cache it at configure() time, even if the pixel rate and
blanking values might be outdated as they can change during streaming
time.
In general, sensorInfo describes the current sensor configuration,
it's not desirable to cache it. However here I actually only need the
sensor's output size and the analog crop which shouldn't change during
streaming. Should I cache the two ? It seems a bit of an hack to have
them cached in CameraData.
> > > + if (ret) {
> > > + LOG(MaliC55, Error) << "Failed to retrieve sensor info";
> > > + return;
> > > + }
> > > +
> > > + /*
> > > + * The ScalerCrop rectangle re-scaling in the ISP crop rectangle
> > > + * comes straight from the RPi pipeline handler.
>
> Does this mean we should have a helper somewhere ? And does this comment
I'm not sure where should I put it. This combines properties of the
CameraSensor class with ISP's ones (the min crop size) and with the
value of an application provided control. Could go in CameraSensor but a
CameraSensor::rescaleScalerCrop() feels a bit out of place ?
> belong here, or to the commit message ?
>
Yes
> > > + *
> > > + * Create a version of the crop rectangle aligned to the analogue crop
> > > + * rectangle top-left coordinates and scaled in the [analogue crop to
> > > + * output frame] ratio to take into account binning/skipping on the
> > > + * sensor.
> > > + */
> > > + Rectangle ispCrop = nativeCrop.translatedBy(-sensorInfo.analogCrop
> > > + .topLeft());
> > > + ispCrop.scaleBy(sensorInfo.outputSize, sensorInfo.analogCrop.size());
> > > +
> > > + /*
> > > + * The crop rectangle should be:
> > > + * 1. At least as big as ispMinCropSize_, once that's been
> > > + * enlarged to the same aspect ratio.
>
> Does this mean the hardware doesn' support sensor resolutions smaller
> than 640x480 ?
>
Yes, the TRM reports:
Minimum supported resolution: 640 (w) x 480 (h)
Now I wonder if is this the minimum supported sensor input resolution or the
ISP minimum output size ?
If 640x480 represents the minimum allowed input size, then it
shouldn't be here. We crop on the resizer's input, not on the ISP
input, so (unless there's a minimum resizer's crop limit) we shouldn't
care about this here, but rather filter out all sensor modes smaller
than 640x480 during validate().
If 640x480 represnts the minimum output resolution, then we should fix
constexpr Size kMaliC55MinSize = { 128, 128 };
Dan do you have any idea here ?
> > > + * 2. With the same mid-point, if possible.
>
> Why so ? Isn't the user in control of that ?
It is, if you look at
Size minSize = ispMinCrop.size().expandedToAspectRatio(nativeCrop.size());
Size size = ispCrop.size().expandedTo(minSize);
ispCrop = size.centeredTo(ispCrop.center())
.enclosedIn(Rectangle(sensorInfo.outputSize));
'size' is the ScalerCrop size expanded to the minimum accepted ISP crop
size, and we're centering it inside 'ispCrop.center()' and
'ispCrop' already has a top-left corner set by the user relatively to
the native pixel array size (and re-scaled in the analogueCrop-to-outputSize
ratio).
So, if ispCrop.size() > minSize then
Size size = ispCrop.size().expandedTo(minSize);
will give you ispCrop.size() and the rectangle resulting from
centering 'size' inside 'ispCrop' will be 'ispCrop' again.
If ispCrop.size() < minSize then ispCrop will be expanded to minSize
and the resulting Rectangle will be centered over ispCrop.
Does this match your understanding ?
However, there is one thing missing in this patch:
* Create a version of the crop rectangle aligned to the analogue crop
* rectangle top-left coordinates
*/
Rectangle ispCrop = nativeCrop.translatedBy(-sensorInfo.analogCrop
.topLeft());
Assumes the ScalerCrop is contained in the analogueCrop rectangle,
otherwise the here above 'translatedBy' will give you a negative
top-left corner.
This is actually correct if we assume controls::ScalerCropMaximum
is set to be the AnalogueCrop (so all ScalerCrop will be contained in
AnalogueCrop). RPi does set ScalerCropMaximum, this patch doesn't.
I presume however we should actually make sure the actual value of
ScalerCrop is at least contained in the AnalogueCrop instead of
assuming the application does the right thing. In case ScalerCrop >
AnalogueCrop should we adjust it and return its value without applying
any crop ? An helper in CameraSensor could actually help addressing
this for all pipelines
>
> > > + * 3. But it can't go outside the sensor area.
> > > + */
> > > + Rectangle ispMinCrop{ 0, 0, 640, 480 };
> >
> > Especially if it's used multiple times.
> >
> > > + Size minSize = ispMinCrop.size().expandedToAspectRatio(nativeCrop.size());
> > > + Size size = ispCrop.size().expandedTo(minSize);
> > > + ispCrop = size.centeredTo(ispCrop.center())
> > > + .enclosedIn(Rectangle(sensorInfo.outputSize));
> > > +
> > > + /*
> > > + * As the resizer can't upscale, the crop rectangle has to be larger
> > > + * than the larger stream output size.
> > > + */
> > > + Size maxYuvSize;
> > > + for (MaliC55Pipe &pipe : pipes_) {
> > > +
> >
> > Probably delete that blank line.
> >
I like it! :)
> > > + if (!pipe.stream)
> > > + continue;
> > > +
> > > + const StreamConfiguration &config = pipe.stream->configuration();
> > > + if (isFormatRaw(config.pixelFormat)) {
> > > + LOG(MaliC55, Debug) << "Cannot crop with a RAW stream";
> > > + return;
> > > + }
> > > +
> > > + Size streamSize = config.size;
> > > + if (streamSize.width > maxYuvSize.width)
> > > + maxYuvSize.width = streamSize.width;
> > > + if (streamSize.height > maxYuvSize.height)
> > > + maxYuvSize.height = streamSize.height;
> > > + }
> > > +
> > > + ispCrop.size().expandTo(maxYuvSize);
> > > +
> > > + /*
> > > + * Now apply the scaler crop to each enabled output. This overrides the
> > > + * crop configuration performed at configure() time and can cause
> > > + * square pixels if the crop rectangle and scaler output FOV ratio are
> >
> > Can cause 'non square' pixels perhaps?
> >
Ah! I always said "square pixels" to indicate this situation,
but in my mind "square pixels" were good. So I thought it was an
expression in English I didn't fully understand :) I'll fix it.
> > > + * different.
> > > + */
> > > + for (MaliC55Pipe &pipe : pipes_) {
> > > +
> > > + if (!pipe.stream)
> > > + continue;
> > > +
> > > + /* Create a copy to avoid setSelection() to modify ispCrop. */
> >
> > 'to prevent setSelection() from modifying ispCrop'
> >
> > > + Rectangle pipeCrop = ispCrop;
> > > + ret = pipe.resizer->setSelection(0, V4L2_SEL_TGT_CROP, &pipeCrop);
> > > + if (ret) {
> > > + LOG(MaliC55, Error)
> > > + << "Failed to apply crop to "
> > > + << (pipe.stream == &data->frStream_ ?
> > > + "FR" : "DS") << " pipe";
> > > + return;
> > > + }
> > > + }
>
> The effective ScalerCrop value should be reported in metadata.
>
true that
> > > +}
> > > +
> > > int PipelineHandlerMaliC55::queueRequestDevice(Camera *camera, Request *request)
> > > {
> > > int ret;
> > > @@ -887,6 +1012,15 @@ int PipelineHandlerMaliC55::queueRequestDevice(Camera *camera, Request *request)
> > > return ret;
> > > }
> > >
> > > + /*
> > > + * Some controls need to be applied immediately, as in example,
> > > + * the ScalerCrop one.
> > > + *
> > > + * \todo Move it buffer queue time (likely after the IPA has filled in
> > > + * the parameters buffer) once we have plumbed the IPA loop in.
> > > + */
> > > + applyScalerCrop(camera, request->controls());
> > > +
> > > return 0;
> > > }
> > >
> > > @@ -965,7 +1099,9 @@ bool PipelineHandlerMaliC55::registerSensorCamera(MediaLink *ispLink)
> > > if (data->init())
> > > return false;
> > >
> > > - /* \todo: Init properties and controls. */
> > > + /* \todo: Init properties. */
> > > +
> > > + data->updateControls();
> > >
> > > registerMaliCamera(std::move(data), sensor->name());
> > > }
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list