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

[Laurent Pinchart]

On Sun, Sep 24, 2023 at 02:18:17PM +0200, Mattijs Korpershoek via libcamera-devel wrote:
> On ven., sept. 22, 2023 at 12:58, Jacopo Mondi wrote:
> > 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

s/might/will/
s/for example //

> >> conversion via libyuv.

If you intend to have two separate paragraphs, you need a blank line
between the first and second sentence. Otherwise (which I think is what
you intended here), you shouldn't have a line break after the first
sentence.

> >>
> >> 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_;

You can use an std::vector<unsigned int> to store sourceLength_, and
drop the sourceNumPlanes_ variable. Same for the destination.

Actually, I would probably make it

	struct PlaneLayout {
		unsigned int length;
		unsigned int stride;
	};

	std::vector<Plane> sourcePlanes_;
	std::vector<Plane> destinationPlanes_;

(names to be bikeshedded)

> > The rest looks good.
> > Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> 
> Will apply your tag
> 
> >>  };
> >>

-- 
Regards,

Laurent Pinchart