[libcamera-devel] [PATCH RFC 3/7] android: yuv: prepare support for other pixel formats

[Mattijs Korpershoek]

Hi Jacopo,

Thank you for your review.

On ven., sept. 22, 2023 at 12:58, Jacopo Mondi <jacopo.mondi at ideasonboard.com> wrote:

> Hi Mattijs
>
> On Fri, Sep 15, 2023 at 09:57:27AM +0200, Mattijs Korpershoek via libcamera-devel wrote:
>> PostProcessorYuv assumes that both source and destination pixel formats
>> will always be formats::NV12.
>> This might change in the future, for example when doing pixel format
>> conversion via libyuv.
>>
>> Add the necessary plumbing so that other formats can be added easily in
>> the future.
>>
>> Also increase plane number to 3, since some YUV format [1] are
>> fully planar (and thus have 3 planes).
>>
>> [1] https://docs.kernel.org/userspace-api/media/v4l/pixfmt-yuv-planar.html#fully-planar-yuv-formats
>> Signed-off-by: Mattijs Korpershoek <mkorpershoek at baylibre.com>
>> ---
>>  src/android/yuv/post_processor_yuv.cpp | 26 +++++++++++++++-----------
>>  src/android/yuv/post_processor_yuv.h   | 12 ++++++++----
>>  2 files changed, 23 insertions(+), 15 deletions(-)
>>
>> diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp
>> index d58090db14ee..734bb85b7351 100644
>> --- a/src/android/yuv/post_processor_yuv.cpp
>> +++ b/src/android/yuv/post_processor_yuv.cpp
>> @@ -90,18 +90,18 @@ void PostProcessorYuv::process(Camera3RequestDescriptor::StreamBuffer *streamBuf
>>  bool PostProcessorYuv::isValidBuffers(const FrameBuffer &source,
>>  				      const CameraBuffer &destination) const
>>  {
>> -	if (source.planes().size() != 2) {
>> +	if (source.planes().size() != sourceNumPlanes_) {
>>  		LOG(YUV, Error) << "Invalid number of source planes: "
>>  				<< source.planes().size();
>>  		return false;
>>  	}
>> -	if (destination.numPlanes() != 2) {
>> +	if (destination.numPlanes() != destinationNumPlanes_) {
>>  		LOG(YUV, Error) << "Invalid number of destination planes: "
>>  				<< destination.numPlanes();
>>  		return false;
>>  	}
>>
>> -	for (unsigned int i = 0; i < 2; i++) {
>> +	for (unsigned int i = 0; i < sourceNumPlanes_; i++) {
>>  		if (source.planes()[i].length < sourceLength_[i]) {
>>  			LOG(YUV, Error)
>>  				<< "The source planes lengths are too small, "
>> @@ -112,7 +112,7 @@ bool PostProcessorYuv::isValidBuffers(const FrameBuffer &source,
>>  			return false;
>>  		}
>>  	}
>> -	for (unsigned int i = 0; i < 2; i++) {
>> +	for (unsigned int i = 0; i < destinationNumPlanes_; i++) {
>>  		if (destination.plane(i).size() < destinationLength_[i]) {
>>  			LOG(YUV, Error)
>>  				<< "The destination planes lengths are too small, "
>> @@ -132,18 +132,22 @@ void PostProcessorYuv::calculateLengths(const StreamConfiguration &inCfg,
>>  {
>>  	sourceSize_ = inCfg.size;
>>  	destinationSize_ = outCfg.size;
>> +	sourceFormat_ = inCfg.pixelFormat;
>> +	destinationFormat_ = outCfg.pixelFormat;
>>
>> -	const PixelFormatInfo &sourceInfo = PixelFormatInfo::info(formats::NV12);
>> -	for (unsigned int i = 0; i < 2; i++) {
>> +	const PixelFormatInfo &sourceInfo = PixelFormatInfo::info(sourceFormat_);
>> +	sourceNumPlanes_ = sourceInfo.numPlanes();
>> +	for (unsigned int i = 0; i < sourceInfo.numPlanes(); i++) {
>
> Can't you use the just assigned sourceNumPlanes_ ?

Will do for v2

>
>>  		sourceStride_[i] = inCfg.stride;
>>  		sourceLength_[i] = sourceInfo.planeSize(sourceSize_.height, i,
>>  							sourceStride_[i]);
>>  	}
>>
>> -	const PixelFormatInfo &destinationInfo = PixelFormatInfo::info(formats::NV12);
>> -	for (unsigned int i = 0; i < 2; i++) {
>> -		destinationStride_[i] = sourceInfo.stride(destinationSize_.width, i, 1);
>> -		destinationLength_[i] = sourceInfo.planeSize(destinationSize_.height, i,
>> -							     destinationStride_[i]);
>> +	const PixelFormatInfo &destinationInfo = PixelFormatInfo::info(destinationFormat_);
>> +	destinationNumPlanes_ = destinationInfo.numPlanes();
>> +	for (unsigned int i = 0; i < destinationInfo.numPlanes(); i++) {
>
> ditto

Will do for v2

>
>> +		destinationStride_[i] = destinationInfo.stride(destinationSize_.width, i, 1);
>> +		destinationLength_[i] = destinationInfo.planeSize(destinationSize_.height, i,
>> +								  destinationStride_[i]);
>>  	}
>>  }
>> diff --git a/src/android/yuv/post_processor_yuv.h b/src/android/yuv/post_processor_yuv.h
>> index a7ac17c564b6..bfe35f46c6dc 100644
>> --- a/src/android/yuv/post_processor_yuv.h
>> +++ b/src/android/yuv/post_processor_yuv.h
>> @@ -28,8 +28,12 @@ private:
>>
>>  	libcamera::Size sourceSize_;
>>  	libcamera::Size destinationSize_;
>> -	unsigned int sourceLength_[2] = {};
>> -	unsigned int destinationLength_[2] = {};
>> -	unsigned int sourceStride_[2] = {};
>> -	unsigned int destinationStride_[2] = {};
>> +	libcamera::PixelFormat sourceFormat_;
>> +	libcamera::PixelFormat destinationFormat_;
>> +	unsigned int sourceLength_[3] = {};
>> +	unsigned int destinationLength_[3] = {};
>> +	unsigned int sourceStride_[3] = {};
>> +	unsigned int destinationStride_[3] = {};
>> +	unsigned int sourceNumPlanes_;
>> +	unsigned int destinationNumPlanes_;
>
> The rest looks good.
> Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>

Will apply your tag

>
>>  };
>>
>> --
>> 2.41.0
>>