[libcamera-devel] [PATCH RFC 2/7] android: yuv: loop over each plane for size check

[Laurent Pinchart]

On Sun, Sep 24, 2023 at 02:17:22PM +0200, Mattijs Korpershoek via libcamera-devel wrote:
> On ven., sept. 22, 2023 at 12:56, Jacopo Mondi wrote:
> > On Fri, Sep 15, 2023 at 09:57:26AM +0200, Mattijs Korpershoek via libcamera-devel wrote:
> >> PostProcessorYuv assumption is that only formats::NV12 is supported
> >> for source and destination pixelFormat.
> >>
> >> Because of this, we don't loop on each plane when size checking, but we
> >> always assume a 2 plane buffer.
> >>
> >> To prepare for adding more YUV formats such as YUYV, loop over the
> >> planes instead of using an if.
> >>
> >> No functional change, besides the logs only printing the first faulty
> >> plane length.
> >>
> >> Signed-off-by: Mattijs Korpershoek <mkorpershoek at baylibre.com>
> >> ---
> >>  src/android/yuv/post_processor_yuv.cpp | 40 +++++++++++++++++-----------------
> >>  1 file changed, 20 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp
> >> index 9631c9617154..d58090db14ee 100644
> >> --- a/src/android/yuv/post_processor_yuv.cpp
> >> +++ b/src/android/yuv/post_processor_yuv.cpp
> >> @@ -101,27 +101,27 @@ bool PostProcessorYuv::isValidBuffers(const FrameBuffer &source,
> >>  		return false;
> >>  	}
> >>
> >> -	if (source.planes()[0].length < sourceLength_[0] ||
> >> -	    source.planes()[1].length < sourceLength_[1]) {
> >> -		LOG(YUV, Error)
> >> -			<< "The source planes lengths are too small, actual size: {"
> >> -			<< source.planes()[0].length << ", "
> >> -			<< source.planes()[1].length
> >> -			<< "}, expected size: {"
> >> -			<< sourceLength_[0] << ", "
> >> -			<< sourceLength_[1] << "}";
> >> -		return false;
> >> +	for (unsigned int i = 0; i < 2; i++) {

I would replace 2 with source.planes().size() here.

> >> +		if (source.planes()[i].length < sourceLength_[i]) {
> >> +			LOG(YUV, Error)
> >> +				<< "The source planes lengths are too small, "
> >> +				<< "actual size[" << i << "]="
> >> +				<< source.planes()[i].length
> >> +				<< ", expected size[" << i << "]="
> >> +				<< sourceLength_[i];

You can avoid printing i twice:

			LOG(YUV, Error)
				<< "Source plane " << i << " is too small, "
				<< "actual size=" << source.planes()[i].length
				<< ", expected size=" << sourceLength_[i];

> >> +			return false;
> >> +		}
> >>  	}

To do it the C++ way,

	for (const auto &[i, plane] : utils::enumerate(source.planes())) {
		if (plane.length < sourceLength_[i]) {
			LOG(YUV, Error)
				<< "Source plane " << i << " is too small, "
				<< "actual size=" << source.planes()[i].length
				<< ", expected size=" << sourceLength_[i];
			return false;
		}
	}

> >> -	if (destination.plane(0).size() < destinationLength_[0] ||
> >> -	    destination.plane(1).size() < destinationLength_[1]) {
> >> -		LOG(YUV, Error)
> >> -			<< "The destination planes lengths are too small, actual size: {"
> >> -			<< destination.plane(0).size() << ", "
> >> -			<< destination.plane(1).size()
> >> -			<< "}, expected size: {"
> >> -			<< sourceLength_[0] << ", "
> >> -			<< sourceLength_[1] << "}";
> >> -		return false;

Blank line please.

> >> +	for (unsigned int i = 0; i < 2; i++) {
> >> +		if (destination.plane(i).size() < destinationLength_[i]) {
> >> +			LOG(YUV, Error)
> >> +				<< "The destination planes lengths are too small, "
> >> +				<< "actual size[" << i << "]="
> >> +				<< destination.plane(i).size()
> >> +				<< ", expected size[" << i << "]="
> >> +				<< sourceLength_[i];
> >
> > This should be destinationLength_[i]
> >
> > With this fixed
> > Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> 
> Will fix for v2 and apply your reviewed tag.
> 
> >> +			return false;
> >> +		}
> >>  	}

Similar comments here as above.

> >>
> >>  	return true;
> >>

-- 
Regards,

Laurent Pinchart