[libcamera-devel,1/5] android: yuv: Use CameraBuffer::stride() in PostProcessorYuv
diff mbox series

Message ID 20210831063438.785767-2-hiroh@chromium.org
State New
Headers show
Series
  • Improve/Clean up post processors code in android
Related show

Commit Message

Hirokazu Honda Aug. 31, 2021, 6:34 a.m. UTC 
PostProcessorYuv computes strides for a CameraBuffer. CameraBuffer has
stride() function. This replaces the computation with the function.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
---
 src/android/yuv/post_processor_yuv.cpp | 13 +++++++------
 src/android/yuv/post_processor_yuv.h   |  1 -
 2 files changed, 7 insertions(+), 7 deletions(-)

Comments

Jacopo Mondi Aug. 31, 2021, 10:29 a.m. UTC | #1 
hi Hiro,

On Tue, Aug 31, 2021 at 03:34:34PM +0900, Hirokazu Honda wrote:
> PostProcessorYuv computes strides for a CameraBuffer. CameraBuffer has
> stride() function. This replaces the computation with the function.
>
> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> ---
>  src/android/yuv/post_processor_yuv.cpp | 13 +++++++------
>  src/android/yuv/post_processor_yuv.h   |  1 -
>  2 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp
> index 6952fc38..12d6297d 100644
> --- a/src/android/yuv/post_processor_yuv.cpp
> +++ b/src/android/yuv/post_processor_yuv.cpp
> @@ -69,9 +69,9 @@ int PostProcessorYuv::process(const FrameBuffer &source,
>  				    sourceStride_[1],
>  				    sourceSize_.width, sourceSize_.height,
>  				    destination->plane(0).data(),
> -				    destinationStride_[0],
> +				    destination->stride(0),
>  				    destination->plane(1).data(),
> -				    destinationStride_[1],
> +				    destination->stride(1),
>  				    destinationSize_.width,
>  				    destinationSize_.height,
>  				    libyuv::FilterMode::kFilterBilinear);
> @@ -115,8 +115,8 @@ bool PostProcessorYuv::isValidBuffers(const FrameBuffer &source,
>  			<< destination.plane(0).size() << ", "
>  			<< destination.plane(1).size()
>  			<< "}, expected size: {"
> -			<< sourceLength_[0] << ", "
> -			<< sourceLength_[1] << "}";
> +			<< destinationLength_[0] << ", "
> +			<< destinationLength_[1] << "}";

Is this intended ?

>  		return false;
>  	}
>
> @@ -132,13 +132,14 @@ void PostProcessorYuv::calculateLengths(const StreamConfiguration &inCfg,
>  	const PixelFormatInfo &nv12Info = PixelFormatInfo::info(formats::NV12);
>  	for (unsigned int i = 0; i < 2; i++) {
>  		sourceStride_[i] = inCfg.stride;
> -		destinationStride_[i] = nv12Info.stride(destinationSize_.width, i, 1);
> +		const unsigned int destinationStride =
> +			nv12Info.stride(destinationSize_.width, i, 1);
>
>  		const unsigned int vertSubSample =
>  			nv12Info.planes[i].verticalSubSampling;
>  		sourceLength_[i] = sourceStride_[i] *
>  			((sourceSize_.height + vertSubSample - 1) / vertSubSample);
> -		destinationLength_[i] = destinationStride_[i] *
> +		destinationLength_[i] = destinationStride *
>  			((destinationSize_.height + vertSubSample - 1) / vertSubSample);

A comment on the existing implementation:
destinationLength_[i]  is used in isValidBuffer() only for this
comparison:

	if (destination.plane(0).size() < destinationLength_[0] ||
	    destination.plane(1).size() < destinationLength_[1]) {

shouldn't we compare the source and dest ?

Also as destination is a CameraBuffer, shouldn't we use
CameraBuffer::size(i) there ?

Thanks
  j


>  	}
>  }
> diff --git a/src/android/yuv/post_processor_yuv.h b/src/android/yuv/post_processor_yuv.h
> index f8b1ba23..1a0b5e89 100644
> --- a/src/android/yuv/post_processor_yuv.h
> +++ b/src/android/yuv/post_processor_yuv.h
> @@ -36,7 +36,6 @@ private:
>  	unsigned int sourceLength_[2] = {};
>  	unsigned int destinationLength_[2] = {};
>  	unsigned int sourceStride_[2] = {};
> -	unsigned int destinationStride_[2] = {};
>  };
>
>  #endif /* __ANDROID_POST_PROCESSOR_YUV_H__ */
> --
> 2.33.0.259.gc128427fd7-goog
>
Hirokazu Honda Aug. 31, 2021, 6:11 p.m. UTC | #2 
Hi Jacopo,

On Tue, Aug 31, 2021 at 7:28 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> hi Hiro,
>
> On Tue, Aug 31, 2021 at 03:34:34PM +0900, Hirokazu Honda wrote:
> > PostProcessorYuv computes strides for a CameraBuffer. CameraBuffer has
> > stride() function. This replaces the computation with the function.
> >
> > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > ---
> >  src/android/yuv/post_processor_yuv.cpp | 13 +++++++------
> >  src/android/yuv/post_processor_yuv.h   |  1 -
> >  2 files changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp
> > index 6952fc38..12d6297d 100644
> > --- a/src/android/yuv/post_processor_yuv.cpp
> > +++ b/src/android/yuv/post_processor_yuv.cpp
> > @@ -69,9 +69,9 @@ int PostProcessorYuv::process(const FrameBuffer &source,
> >                                   sourceStride_[1],
> >                                   sourceSize_.width, sourceSize_.height,
> >                                   destination->plane(0).data(),
> > -                                 destinationStride_[0],
> > +                                 destination->stride(0),
> >                                   destination->plane(1).data(),
> > -                                 destinationStride_[1],
> > +                                 destination->stride(1),
> >                                   destinationSize_.width,
> >                                   destinationSize_.height,
> >                                   libyuv::FilterMode::kFilterBilinear);
> > @@ -115,8 +115,8 @@ bool PostProcessorYuv::isValidBuffers(const FrameBuffer &source,
> >                       << destination.plane(0).size() << ", "
> >                       << destination.plane(1).size()
> >                       << "}, expected size: {"
> > -                     << sourceLength_[0] << ", "
> > -                     << sourceLength_[1] << "}";
> > +                     << destinationLength_[0] << ", "
> > +                     << destinationLength_[1] << "}";
>
> Is this intended ?
>

Yes, I found this was wrong.

> >               return false;
> >       }
> >
> > @@ -132,13 +132,14 @@ void PostProcessorYuv::calculateLengths(const StreamConfiguration &inCfg,
> >       const PixelFormatInfo &nv12Info = PixelFormatInfo::info(formats::NV12);
> >       for (unsigned int i = 0; i < 2; i++) {
> >               sourceStride_[i] = inCfg.stride;
> > -             destinationStride_[i] = nv12Info.stride(destinationSize_.width, i, 1);
> > +             const unsigned int destinationStride =
> > +                     nv12Info.stride(destinationSize_.width, i, 1);
> >
> >               const unsigned int vertSubSample =
> >                       nv12Info.planes[i].verticalSubSampling;
> >               sourceLength_[i] = sourceStride_[i] *
> >                       ((sourceSize_.height + vertSubSample - 1) / vertSubSample);
> > -             destinationLength_[i] = destinationStride_[i] *
> > +             destinationLength_[i] = destinationStride *
> >                       ((destinationSize_.height + vertSubSample - 1) / vertSubSample);
>
> A comment on the existing implementation:
> destinationLength_[i]  is used in isValidBuffer() only for this
> comparison:
>
>         if (destination.plane(0).size() < destinationLength_[0] ||
>             destination.plane(1).size() < destinationLength_[1]) {
>
> shouldn't we compare the source and dest ?
>

Do you mean to add the comment? Where shall I add?

-Hiro
> Also as destination is a CameraBuffer, shouldn't we use
> CameraBuffer::size(i) there ?
>
> Thanks
>   j
>
>
> >       }
> >  }
> > diff --git a/src/android/yuv/post_processor_yuv.h b/src/android/yuv/post_processor_yuv.h
> > index f8b1ba23..1a0b5e89 100644
> > --- a/src/android/yuv/post_processor_yuv.h
> > +++ b/src/android/yuv/post_processor_yuv.h
> > @@ -36,7 +36,6 @@ private:
> >       unsigned int sourceLength_[2] = {};
> >       unsigned int destinationLength_[2] = {};
> >       unsigned int sourceStride_[2] = {};
> > -     unsigned int destinationStride_[2] = {};
> >  };
> >
> >  #endif /* __ANDROID_POST_PROCESSOR_YUV_H__ */
> > --
> > 2.33.0.259.gc128427fd7-goog
> >
Laurent Pinchart Aug. 31, 2021, 9:30 p.m. UTC | #3 
Hi Hiro,

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

On Wed, Sep 01, 2021 at 03:11:00AM +0900, Hirokazu Honda wrote:
> On Tue, Aug 31, 2021 at 7:28 PM Jacopo Mondi wrote:
> > On Tue, Aug 31, 2021 at 03:34:34PM +0900, Hirokazu Honda wrote:
> > > PostProcessorYuv computes strides for a CameraBuffer. CameraBuffer has
> > > stride() function. This replaces the computation with the function.
> > >
> > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > > ---
> > >  src/android/yuv/post_processor_yuv.cpp | 13 +++++++------
> > >  src/android/yuv/post_processor_yuv.h   |  1 -
> > >  2 files changed, 7 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp
> > > index 6952fc38..12d6297d 100644
> > > --- a/src/android/yuv/post_processor_yuv.cpp
> > > +++ b/src/android/yuv/post_processor_yuv.cpp
> > > @@ -69,9 +69,9 @@ int PostProcessorYuv::process(const FrameBuffer &source,
> > >                                   sourceStride_[1],
> > >                                   sourceSize_.width, sourceSize_.height,
> > >                                   destination->plane(0).data(),
> > > -                                 destinationStride_[0],
> > > +                                 destination->stride(0),
> > >                                   destination->plane(1).data(),
> > > -                                 destinationStride_[1],
> > > +                                 destination->stride(1),
> > >                                   destinationSize_.width,
> > >                                   destinationSize_.height,
> > >                                   libyuv::FilterMode::kFilterBilinear);
> > > @@ -115,8 +115,8 @@ bool PostProcessorYuv::isValidBuffers(const FrameBuffer &source,
> > >                       << destination.plane(0).size() << ", "
> > >                       << destination.plane(1).size()
> > >                       << "}, expected size: {"
> > > -                     << sourceLength_[0] << ", "
> > > -                     << sourceLength_[1] << "}";
> > > +                     << destinationLength_[0] << ", "
> > > +                     << destinationLength_[1] << "}";
> >
> > Is this intended ?
> 
> Yes, I found this was wrong.

Could you please mention it in the commit message ?

> > >               return false;
> > >       }
> > >
> > > @@ -132,13 +132,14 @@ void PostProcessorYuv::calculateLengths(const StreamConfiguration &inCfg,
> > >       const PixelFormatInfo &nv12Info = PixelFormatInfo::info(formats::NV12);
> > >       for (unsigned int i = 0; i < 2; i++) {
> > >               sourceStride_[i] = inCfg.stride;
> > > -             destinationStride_[i] = nv12Info.stride(destinationSize_.width, i, 1);
> > > +             const unsigned int destinationStride =
> > > +                     nv12Info.stride(destinationSize_.width, i, 1);
> > >
> > >               const unsigned int vertSubSample =
> > >                       nv12Info.planes[i].verticalSubSampling;
> > >               sourceLength_[i] = sourceStride_[i] *
> > >                       ((sourceSize_.height + vertSubSample - 1) / vertSubSample);
> > > -             destinationLength_[i] = destinationStride_[i] *
> > > +             destinationLength_[i] = destinationStride *
> > >                       ((destinationSize_.height + vertSubSample - 1) / vertSubSample);
> >
> > A comment on the existing implementation:
> > destinationLength_[i]  is used in isValidBuffer() only for this
> > comparison:
> >
> >         if (destination.plane(0).size() < destinationLength_[0] ||
> >             destination.plane(1).size() < destinationLength_[1]) {
> >
> > shouldn't we compare the source and dest ?
> 
> Do you mean to add the comment? Where shall I add?
> 
> > Also as destination is a CameraBuffer, shouldn't we use
> > CameraBuffer::size(i) there ?

I thought about that but then realized that the check is meant to ensure
that the buffer has a size compatible with the size that was set at
configuration time.

I think this should be reworked to use outCfg.stride and move the
destination stride calculation to the caller, but maybe a todo comment
would be enough for now.

	/* \todo Move stride calculation to the caller and use outCfg.stride */

> > >       }
> > >  }
> > > diff --git a/src/android/yuv/post_processor_yuv.h b/src/android/yuv/post_processor_yuv.h
> > > index f8b1ba23..1a0b5e89 100644
> > > --- a/src/android/yuv/post_processor_yuv.h
> > > +++ b/src/android/yuv/post_processor_yuv.h
> > > @@ -36,7 +36,6 @@ private:
> > >       unsigned int sourceLength_[2] = {};
> > >       unsigned int destinationLength_[2] = {};
> > >       unsigned int sourceStride_[2] = {};
> > > -     unsigned int destinationStride_[2] = {};
> > >  };
> > >
> > >  #endif /* __ANDROID_POST_PROCESSOR_YUV_H__ */

Patch
diff mbox series 

diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp
index 6952fc38..12d6297d 100644
--- a/src/android/yuv/post_processor_yuv.cpp
+++ b/src/android/yuv/post_processor_yuv.cpp
@@ -69,9 +69,9 @@  int PostProcessorYuv::process(const FrameBuffer &source,
 				    sourceStride_[1],
 				    sourceSize_.width, sourceSize_.height,
 				    destination->plane(0).data(),
-				    destinationStride_[0],
+				    destination->stride(0),
 				    destination->plane(1).data(),
-				    destinationStride_[1],
+				    destination->stride(1),
 				    destinationSize_.width,
 				    destinationSize_.height,
 				    libyuv::FilterMode::kFilterBilinear);
@@ -115,8 +115,8 @@  bool PostProcessorYuv::isValidBuffers(const FrameBuffer &source,
 			<< destination.plane(0).size() << ", "
 			<< destination.plane(1).size()
 			<< "}, expected size: {"
-			<< sourceLength_[0] << ", "
-			<< sourceLength_[1] << "}";
+			<< destinationLength_[0] << ", "
+			<< destinationLength_[1] << "}";
 		return false;
 	}
 
@@ -132,13 +132,14 @@  void PostProcessorYuv::calculateLengths(const StreamConfiguration &inCfg,
 	const PixelFormatInfo &nv12Info = PixelFormatInfo::info(formats::NV12);
 	for (unsigned int i = 0; i < 2; i++) {
 		sourceStride_[i] = inCfg.stride;
-		destinationStride_[i] = nv12Info.stride(destinationSize_.width, i, 1);
+		const unsigned int destinationStride =
+			nv12Info.stride(destinationSize_.width, i, 1);
 
 		const unsigned int vertSubSample =
 			nv12Info.planes[i].verticalSubSampling;
 		sourceLength_[i] = sourceStride_[i] *
 			((sourceSize_.height + vertSubSample - 1) / vertSubSample);
-		destinationLength_[i] = destinationStride_[i] *
+		destinationLength_[i] = destinationStride *
 			((destinationSize_.height + vertSubSample - 1) / vertSubSample);
 	}
 }
diff --git a/src/android/yuv/post_processor_yuv.h b/src/android/yuv/post_processor_yuv.h
index f8b1ba23..1a0b5e89 100644
--- a/src/android/yuv/post_processor_yuv.h
+++ b/src/android/yuv/post_processor_yuv.h
@@ -36,7 +36,6 @@  private:
 	unsigned int sourceLength_[2] = {};
 	unsigned int destinationLength_[2] = {};
 	unsigned int sourceStride_[2] = {};
-	unsigned int destinationStride_[2] = {};
 };
 
 #endif /* __ANDROID_POST_PROCESSOR_YUV_H__ */