[PATCH v6 01/18] libcamera: pipeline: simple: fix size adjustment in validate()
Andrei Konovalov
andrey.konovalov.ynk at gmail.com
Wed Mar 20 13:53:21 CET 2024
Hi Laurent and Milan,
On 20.03.2024 15:17, Laurent Pinchart wrote:
> Hi Milan,
>
> Thank you for the patch.
>
> On Tue, Mar 19, 2024 at 01:35:48PM +0100, Milan Zamazal wrote:
>> From: Andrey Konovalov <andrey.konovalov at linaro.org>
>>
>> SimpleCameraConfiguration::validate() adjusts the configuration of its
>> streams (if the size is not in the outputSizes) to the captureSize. But
>> the captureSize itself can be not in the outputSizes, and then the
>> adjusted configuration won't be valid resulting in camera configuration
>> failure.
>>
>> Tested-by: Bryan O'Donoghue <bryan.odonoghue at linaro.org> # sc8280xp Lenovo x13s
>> Tested-by: Pavel Machek <pavel at ucw.cz>
>> Reviewed-by: Milan Zamazal <mzamazal at redhat.com>
>> Reviewed-by: Pavel Machek <pavel at ucw.cz>
>> Signed-off-by: Andrey Konovalov <andrey.konovalov at linaro.org>
>> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
>> ---
>> src/libcamera/pipeline/simple/simple.cpp | 37 ++++++++++++++++++++++--
>> 1 file changed, 35 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
>> index 01f2a977..04e77f7e 100644
>> --- a/src/libcamera/pipeline/simple/simple.cpp
>> +++ b/src/libcamera/pipeline/simple/simple.cpp
>> @@ -882,6 +882,30 @@ SimpleCameraConfiguration::SimpleCameraConfiguration(Camera *camera,
>> {
>> }
>>
>> +namespace {
>> +
>> +static Size adjustSize(const Size &requestedSize, const SizeRange &supportedSizes)
>> +{
>> + ASSERT(supportedSizes.min <= supportedSizes.max);
>> +
>> + if (supportedSizes.min == supportedSizes.max)
>> + return supportedSizes.max;
>> +
>> + unsigned int hStep = supportedSizes.hStep;
>> + unsigned int vStep = supportedSizes.vStep;
>> +
>> + if (hStep == 0)
>> + hStep = supportedSizes.max.width - supportedSizes.min.width;
>> + if (vStep == 0)
>> + vStep = supportedSizes.max.height - supportedSizes.min.height;
>> +
>> + Size adjusted = requestedSize.boundedTo(supportedSizes.max).expandedTo(supportedSizes.min);
>> +
>> + return adjusted.shrunkBy(supportedSizes.min).alignedDownTo(hStep, vStep).grownBy(supportedSizes.min);
>
> Those lines are getting long. You can write
>
> Size adjusted = requestedSize.boundedTo(supportedSizes.max)
> .expandedTo(supportedSizes.min);
> return adjusted.shrunkBy(supportedSizes.min)
> .alignedDownTo(hStep, vStep)
> .grownBy(supportedSizes.min);
>
> or even
>
> return requestedSize.boundedTo(supportedSizes.max)
> .expandedTo(supportedSizes.min)
> .shrunkBy(supportedSizes.min)
> .alignedDownTo(hStep, vStep)
> .grownBy(supportedSizes.min);
>
> I can change this when applying if you tell me which version you like
> best (if any).
As for me, the both versions look good.
The first version matching my taste a tiny bit better probably.
Thanks,
Andrei
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
>> +}
>> +
>> +} /* namespace */
>> +
>> CameraConfiguration::Status SimpleCameraConfiguration::validate()
>> {
>> const CameraSensor *sensor = data_->sensor_.get();
>> @@ -998,10 +1022,19 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()
>> }
>>
>> if (!pipeConfig_->outputSizes.contains(cfg.size)) {
>> + Size adjustedSize = pipeConfig_->captureSize;
>> + /*
>> + * The converter (when present) may not be able to output
>> + * a size identical to its input size. The capture size is thus
>> + * not guaranteed to be a valid output size. In such cases, use
>> + * the smaller valid output size closest to the requested.
>> + */
>> + if (!pipeConfig_->outputSizes.contains(adjustedSize))
>> + adjustedSize = adjustSize(cfg.size, pipeConfig_->outputSizes);
>> LOG(SimplePipeline, Debug)
>> << "Adjusting size from " << cfg.size
>> - << " to " << pipeConfig_->captureSize;
>> - cfg.size = pipeConfig_->captureSize;
>> + << " to " << adjustedSize;
>> + cfg.size = adjustedSize;
>> status = Adjusted;
>> }
>>
>
More information about the libcamera-devel
mailing list