[PATCH 3/7] mali-c55: implement support for ScalerCrop
Dan Scally
dan.scally at ideasonboard.com
Mon Jun 24 17:08:17 CEST 2024
Hello both
On 24/06/2024 16:01, Jacopo Mondi wrote:
> 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 ?
I think it's both minimum supported input and output; I discussed with Arm smaller input framesizes
(in the context of the TPG at the time) and the conclusion from the discussion was not to support
smaller than 640x480.
>
>>>> + * 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