[v4,1/3] pipeline: simple: Allow buffer counts from 1 to 32

24575 diff mbox series

While a default value of 4 buffers appears to be a good default that is
used by other pipelines as well, allowing both higher and lower values
can be desirable, notably for:
1. Video encoding, e.g. encoding multiple buffers in parallel.
2. Clients requesting a single buffer - e.g. in multi-stream scenarios.

Thus allow buffer counts between 1 and 32 buffers - following the default
maximum from vb2 core - while keeping the default to the previous 4.

While on it mark the config as adjusted when appropriate.

Signed-off-by: Robert Mader <robert.mader@collabora.com>
Reviewed-by: Milan Zamazal <mzamazal@redhat.com>

---

Changes in v4:
1. Limit the number of queued requests to 4

Changes in v3:
1. Adopeted code cleanup suggestion - no change in behavior.
2. Split out change of kNumInternalBuffers.
3. Minor commit message changes.

Changes since v1 with title "pipeline: simple: Allow buffer counts from 1 to 16 for swISP"
1: Cover all cases, not just the swISP one.
2: Increase maximum to 32 to match vb2 core.
3: Change constant naming to better match similar ones.
4: Bump kNumInternalBuffers to 4.
---
 src/libcamera/pipeline/simple/simple.cpp | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

Quoting Robert Mader (2025-10-10 10:22:24)
> While a default value of 4 buffers appears to be a good default that is
> used by other pipelines as well, allowing both higher and lower values
> can be desirable, notably for:
> 1. Video encoding, e.g. encoding multiple buffers in parallel.
> 2. Clients requesting a single buffer - e.g. in multi-stream scenarios.
> 
> Thus allow buffer counts between 1 and 32 buffers - following the default
> maximum from vb2 core - while keeping the default to the previous 4.
> 
> While on it mark the config as adjusted when appropriate.
> 
> Signed-off-by: Robert Mader <robert.mader@collabora.com>
> Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
> 
> ---
> 
> Changes in v4:
> 1. Limit the number of queued requests to 4
> 
> Changes in v3:
> 1. Adopeted code cleanup suggestion - no change in behavior.
> 2. Split out change of kNumInternalBuffers.
> 3. Minor commit message changes.
> 
> Changes since v1 with title "pipeline: simple: Allow buffer counts from 1 to 16 for swISP"
> 1: Cover all cases, not just the swISP one.
> 2: Increase maximum to 32 to match vb2 core.
> 3: Change constant naming to better match similar ones.
> 4: Bump kNumInternalBuffers to 4.
> ---
>  src/libcamera/pipeline/simple/simple.cpp | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index c816cffc9..2dcba04ec 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -378,6 +378,9 @@ public:
>         const Transform &combinedTransform() const { return combinedTransform_; }
>  
>  private:
> +       static constexpr unsigned int kNumBuffersDefault = 4;
> +       static constexpr unsigned int kNumBuffersMax = 32;
> +
>         /*
>          * The SimpleCameraData instance is guaranteed to be valid as long as
>          * the corresponding Camera instance is valid. In order to borrow a
> @@ -1239,7 +1242,7 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()
>                     cfg.size != pipeConfig_->captureSize)
>                         needConversion_ = true;
>  
> -               /* Set the stride, frameSize and bufferCount. */
> +               /* Set the stride and frameSize. */
>                 if (needConversion_) {
>                         std::tie(cfg.stride, cfg.frameSize) =
>                                 data_->converter_
> @@ -1262,7 +1265,18 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()
>                         cfg.frameSize = format.planes[0].size;
>                 }
>  
> -               cfg.bufferCount = 4;
> +               const auto bufferCount = cfg.bufferCount;
> +               if (bufferCount <= 0)

bufferCount is an unsigned int.

> +                       cfg.bufferCount = kNumBuffersDefault;
> +               else if (bufferCount > kNumBuffersMax)
> +                       cfg.bufferCount = kNumBuffersMax;



Do we really expect to support 1,2,3 ?

Or should we do : 
cfg.bufferCount = std::clamp(cfg.bufferCount, kNumBuffersDefault, kNumBuffersMax);

Hrm, I think we could so:

cfg.bufferCount = std::clamp(cfg.bufferCount, 1, kNumBuffersMax);

But that doesn't handle the case you have with <= 0 = default.... so
maybe it's

	unsigned int bufferCount = cfg.bufferCount;
	if (!bufferCount)
		cfg.bufferCount = kNumBuffersDefault;
	
	cfg.bufferCount = std::min(cfg.bufferCount, kNumBuffersMax);

That's not specifically any much better or readable than your existing
patch so for your patch:


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>


> +
> +               if (cfg.bufferCount != bufferCount) {
> +                       LOG(SimplePipeline, Debug)
> +                               << "Adjusting bufferCount from " << bufferCount
> +                               << " to " << cfg.bufferCount;
> +                       status = Adjusted;
> +               }
>         }
>  
>         return status;
> -- 
> 2.51.0
>

Hi

2025. 10. 10. 14:44 keltezéssel, Kieran Bingham írta:
> Quoting Robert Mader (2025-10-10 10:22:24)
>> While a default value of 4 buffers appears to be a good default that is
>> used by other pipelines as well, allowing both higher and lower values
>> can be desirable, notably for:
>> 1. Video encoding, e.g. encoding multiple buffers in parallel.
>> 2. Clients requesting a single buffer - e.g. in multi-stream scenarios.
>>
>> Thus allow buffer counts between 1 and 32 buffers - following the default
>> maximum from vb2 core - while keeping the default to the previous 4.
>>
>> While on it mark the config as adjusted when appropriate.
>>
>> Signed-off-by: Robert Mader <robert.mader@collabora.com>
>> Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
>>
>> ---
> [...]
>> ---
>>   src/libcamera/pipeline/simple/simple.cpp | 18 ++++++++++++++++--
>>   1 file changed, 16 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
>> index c816cffc9..2dcba04ec 100644
>> --- a/src/libcamera/pipeline/simple/simple.cpp
>> +++ b/src/libcamera/pipeline/simple/simple.cpp
>> @@ -378,6 +378,9 @@ public:
>>          const Transform &combinedTransform() const { return combinedTransform_; }
>>   
>>   private:
>> +       static constexpr unsigned int kNumBuffersDefault = 4;
>> +       static constexpr unsigned int kNumBuffersMax = 32;
>> +
> [...]
>> -               cfg.bufferCount = 4;
>> +               const auto bufferCount = cfg.bufferCount;
>> +               if (bufferCount <= 0)
> 
> bufferCount is an unsigned int.
> 
>> +                       cfg.bufferCount = kNumBuffersDefault;
>> +               else if (bufferCount > kNumBuffersMax)
>> +                       cfg.bufferCount = kNumBuffersMax;
> 
> 
> 
> Do we really expect to support 1,2,3 ?

Seems to work on this one ipu6 laptop with software isp. And the default
is 4, so someone has to set it deliberately if they want 1, 2, or 3.
I think it is probably fine to allow it. Or maybe there are other
downsides that I cannot see.


Regards,
Barnabás Pőcze

> [...]

On 10/10/25 15:09, Barnabás Pőcze wrote:
>>
>>> +                       cfg.bufferCount = kNumBuffersDefault;
>>> +               else if (bufferCount > kNumBuffersMax)
>>> +                       cfg.bufferCount = kNumBuffersMax;
>>
>>
>>
>> Do we really expect to support 1,2,3 ?
>
> Seems to work on this one ipu6 laptop with software isp. And the default
> is 4, so someone has to set it deliberately if they want 1, 2, or 3.
> I think it is probably fine to allow it. Or maybe there are other
> downsides that I cannot see.

For the record, the main intention to reduce overhead for clients 
wanting to capture just a single frame - let's say in multi-stream or 
low-power environments. My hope is that such applications are rare 
enough that, in case something breaks, we'll be likely to get bug 
reports from whoever tries it.