[libcamera-devel] [PATCH v1 2/8] pipeline: raspberrypi: Handle MandatoryStream hints for Unicam Image

[Jacopo Mondi]

Hi Naush

On Fri, Feb 03, 2023 at 09:44:18AM +0000, Naushir Patuck via libcamera-devel wrote:
> Look for MandatoryStream flag in the hints field of the Unicam Image
> StreamConfiguration structure. If this flag is set, it guarantees that the
> application will provide buffers for Unicam Image, so override the
> minUnicamBuffers and minTotalUnicamBuffers config parameters in the following
> way:
>
> - If startup drop frames are required, allocate at least 1 internal buffer.
> - If no startup drop frames are required, do not allocate any internal buffers.
>
> All other buffer allocations remain unchanged.
>
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> ---
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 34 +++++++++++++++----
>  1 file changed, 27 insertions(+), 7 deletions(-)
>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index d752911ddfff..7d786fe839f7 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -1520,12 +1520,33 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera)
>  int PipelineHandlerRPi::prepareBuffers(Camera *camera)
>  {
>  	RPiCameraData *data = cameraData(camera);
> +	unsigned int minUnicamBuffers = data->config_.minUnicamBuffers;
> +	unsigned int minTotalUnicamBuffers = data->config_.minTotalUnicamBuffers;
>  	unsigned int numRawBuffers = 0;
>  	int ret;
>
>  	for (Stream *s : camera->streams()) {
> -		if (isRaw(s->configuration().pixelFormat)) {
> +		if (s == &data->unicam_[Unicam::Image]) {

What if the sensor produces YUV ? I guess it doesn't really make a
difference, we'll have to drop frames or not regardless of the image
format ?

>  			numRawBuffers = s->configuration().bufferCount;
> +			/*
> +			 * If the application provides a guarantees that Unicam
> +			 * image buffers will always be provided for the RAW stream
> +			 * in a Request, we need:
> +			 * - at least 1 internal Unicam buffer to handle startup frame drops,
> +			 * - no internal Unicam buffers if there are no startup frame drops.


> +			 */
> +			if (s->configuration().hints &
> +			    StreamConfiguration::Hint::MandatoryStream) {
> +				if (data->dropFrameCount_) {
> +					minUnicamBuffers = std::max(1u, minUnicamBuffers);
> +					minTotalUnicamBuffers =
> +						std::max(minUnicamBuffers, minTotalUnicamBuffers);
> +				} else {
> +					minUnicamBuffers = 0;
> +					minTotalUnicamBuffers = 0;
> +				}
> +			}
> +
>  			break;
>  		}
>  	}
> @@ -1537,7 +1558,6 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
>  		 * For Unicam, allocate a minimum number of buffers for internal
>  		 * use as we want to avoid any frame drops.
>  		 */
> -		const unsigned int minBuffers = data->config_.minTotalUnicamBuffers;
>  		if (stream == &data->unicam_[Unicam::Image]) {
>  			/*
>  			 * If an application has configured a RAW stream, allocate
> @@ -1545,8 +1565,8 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
>  			 * we have at least minUnicamBuffers of internal buffers
>  			 * to use to minimise frame drops.
>  			 */
> -			numBuffers = std::max<int>(data->config_.minUnicamBuffers,
> -						   minBuffers - numRawBuffers);
> +			numBuffers = std::max<int>(minUnicamBuffers,
> +						   minTotalUnicamBuffers - numRawBuffers);

if minTotalUnicamBuffers == 0, is there a risk that
(minTotalUnicamBuffers - numRawBuffers) becomes negative ? as
std::max<int> do we risk any wrong type conversion ?

>  		} else if (stream == &data->isp_[Isp::Input]) {
>  			/*
>  			 * ISP input buffers are imported from Unicam, so follow
> @@ -1554,15 +1574,15 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
>  			 * available.
>  			 */
>  			numBuffers = numRawBuffers +
> -				     std::max<int>(data->config_.minUnicamBuffers,
> -						   minBuffers - numRawBuffers);
> +				     std::max<int>(minUnicamBuffers,
> +						   minTotalUnicamBuffers - numRawBuffers);

ditto

>
>  		} else if (stream == &data->unicam_[Unicam::Embedded]) {
>  			/*
>  			 * Embedded data buffers are (currently) for internal use,
>  			 * so allocate the minimum required to avoid frame drops.
>  			 */
> -			numBuffers = minBuffers;
> +			numBuffers = data->config_.minTotalUnicamBuffers;
>  		} else {
>  			/*
>  			 * Since the ISP runs synchronous with the IPA and requests,
> --
> 2.25.1
>