[libcamera-devel] [PATCH] libcamera: formats: add planeCount helper

[Kieran Bingham]

Hi Laurent,

On 29/07/2020 14:36, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Wed, Jul 29, 2020 at 12:25:24PM +0100, Kieran Bingham wrote:
>> Determine the number of planes used by a format by counting the number
>> of PxielFormatPlaneInfo entries with a valid entry.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>> ---
>>  include/libcamera/internal/formats.h |  2 ++
>>  src/libcamera/formats.cpp            | 22 ++++++++++++++++++++++
>>  2 files changed, 24 insertions(+)
>>
>> diff --git a/include/libcamera/internal/formats.h b/include/libcamera/internal/formats.h
>> index 0bb151044294..beeaab1ae2f5 100644
>> --- a/include/libcamera/internal/formats.h
>> +++ b/include/libcamera/internal/formats.h
>> @@ -45,6 +45,8 @@ public:
>>  	unsigned int frameSize(const Size &size,
>>  			       const std::array<unsigned int, 3> &strides) const;
>>  
>> +	unsigned int planeCount() const;
>> +
>>  	/* \todo Add support for non-contiguous memory planes */
>>  	const char *name;
>>  	PixelFormat format;
>> diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
>> index cd63c15cb926..70d31fb36c37 100644
>> --- a/src/libcamera/formats.cpp
>> +++ b/src/libcamera/formats.cpp
>> @@ -805,4 +805,26 @@ PixelFormatInfo::frameSize(const Size &size,
>>  	return sum;
>>  }
>>  
>> +/**
>> + * \brief Identify the number of planes represented by the format
>> + *
>> + * This function counts the number of planes which have a valid configuration
>> + * and uses that to determine the number of planes used by the format.
> 
> I think this is just an internal implementation detail.
> 
>  * \brief Retrieve the number of planes used by the format
>  * \return The number of planes used by the format
> 
> should be enough.

Sure, shorter is sweeter ...

> 
> And if we hide the fact that we count them, maybe
> s/planeCount/numPlanes/ ? I think that's a bit more consistent with how
> we name fields (haven't checked though).

numPlanes is indeed better, and doesn't define how it's implemented.


>> + *
>> + * \return The number of planes used by the format
>> + */
>> +unsigned int PixelFormatInfo::planeCount() const
>> +{
>> +	unsigned int count = 0;
>> +
>> +	for (const PixelFormatPlaneInfo &p : planes) {
>> +		if (p.bytesPerGroup == 0)
>> +			break;
>> +
>> +		count++;
>> +	}
>> +
>> +	return count;
> 
> 	return std::count_if(planes.begin(), planes.end(),
> 			     [](const PixelFormatPlaneInfo &p) {
> 				     return p.bytesPerGroup != 0;
> 			     });
> 
> would also work, entirely up to you.
> 

I find the lambda's are often less readable. Yes, more C++'ified. but
... not very friendly at all on the eye.

I assume there would be no performance gain or improvement, so I'd
prefer to keep the visible count ...

Maybe I'll change my mind in the future ...

> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

Thanks,



>> +}
>> +
>>  } /* namespace libcamera */
> 

-- 
Regards
--
Kieran