[libcamera-devel,RFC,2/7] android: yuv: loop over each plane for size check
diff mbox series

Message ID 20230915-libyuv-convert-v1-2-1e5bcf68adac@baylibre.com
State New
Headers show
Series
  • android: add YUYV->NV12 conversion via libyuv
Related show

Commit Message

Mattijs Korpershoek Sept. 15, 2023, 7:57 a.m. UTC 
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@baylibre.com>
---
 src/android/yuv/post_processor_yuv.cpp | 40 +++++++++++++++++-----------------
 1 file changed, 20 insertions(+), 20 deletions(-)

Comments

Jacopo Mondi Sept. 22, 2023, 10:56 a.m. UTC | #1 
Hi Mattijs

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@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++) {
> +		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];
> +			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;
> +	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@ideasonboard.com>

> +			return false;
> +		}
>  	}
>
>  	return true;
>
> --
> 2.41.0
>
Mattijs Korpershoek Sept. 24, 2023, 12:17 p.m. UTC | #2 
Hi Jacopo,

Thank you for your review.

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

> Hi Mattijs
>
> 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@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++) {
>> +		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];
>> +			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;
>> +	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@ideasonboard.com>

Will fix for v2 and apply your reviewed tag.

>
>> +			return false;
>> +		}
>>  	}
>>
>>  	return true;
>>
>> --
>> 2.41.0
>>
Laurent Pinchart Sept. 24, 2023, 1:27 p.m. UTC | #3 
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@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@ideasonboard.com>
> 
> Will fix for v2 and apply your reviewed tag.
> 
> >> +			return false;
> >> +		}
> >>  	}

Similar comments here as above.

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

Patch
diff mbox series 

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++) {
+		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];
+			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;
+	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];
+			return false;
+		}
 	}
 
 	return true;