[libcamera-devel,v3,1/2] android: post_processor: Change the type destination in process()
diff mbox series

Message ID 20210128224217.2919790-2-hiroh@chromium.org
State Accepted
Headers show
Series
  • Introduce post processor using libyuv
Related show

Commit Message

Hirokazu Honda Jan. 28, 2021, 10:42 p.m. UTC 
The type of the destination buffer in PostProcessor::process() is
libcamera::Span. libcamera::Span is used for one dimension buffer
(e.g. blob buffer). The destination can be multiple dimensions
buffer (e.g. yuv frame). Therefore, this changes the type of the
destination buffer to MappedFrameBuffer.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

---
 src/android/camera_stream.cpp            | 4 ++--
 src/android/camera_stream.h              | 6 ++++--
 src/android/jpeg/post_processor_jpeg.cpp | 6 +++---
 src/android/jpeg/post_processor_jpeg.h   | 2 +-
 src/android/post_processor.h             | 7 ++++---
 5 files changed, 14 insertions(+), 11 deletions(-)

--
2.30.0.365.g02bc693789-goog

Comments

Jacopo Mondi Feb. 1, 2021, 11 a.m. UTC | #1 
Hi Hiro,

On Thu, Jan 28, 2021 at 10:42:15PM +0000, Hirokazu Honda wrote:
> The type of the destination buffer in PostProcessor::process() is
> libcamera::Span. libcamera::Span is used for one dimension buffer
> (e.g. blob buffer). The destination can be multiple dimensions
> buffer (e.g. yuv frame). Therefore, this changes the type of the
> destination buffer to MappedFrameBuffer.

Thanks, this will make the recalculation of the JPEG buffer size
easier.

>
> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> ---
>  src/android/camera_stream.cpp            | 4 ++--
>  src/android/camera_stream.h              | 6 ++++--
>  src/android/jpeg/post_processor_jpeg.cpp | 6 +++---
>  src/android/jpeg/post_processor_jpeg.h   | 2 +-
>  src/android/post_processor.h             | 7 ++++---
>  5 files changed, 14 insertions(+), 11 deletions(-)
>
> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> index 4c8020e5..611ec0d1 100644
> --- a/src/android/camera_stream.cpp
> +++ b/src/android/camera_stream.cpp
> @@ -96,14 +96,14 @@ int CameraStream::configure()
>  }
>
>  int CameraStream::process(const libcamera::FrameBuffer &source,
> -			  MappedCamera3Buffer *dest,
> +			  libcamera::MappedBuffer *destination,
>  			  const CameraMetadata &requestMetadata,
>  			  CameraMetadata *resultMetadata)
>  {
>  	if (!postProcessor_)
>  		return 0;
>
> -	return postProcessor_->process(source, dest->maps()[0],
> +	return postProcessor_->process(source, destination,
>  				       requestMetadata, resultMetadata);
>  }
>
> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> index 298ffbf6..73bac0ba 100644
> --- a/src/android/camera_stream.h
> +++ b/src/android/camera_stream.h
> @@ -19,9 +19,10 @@
>  #include <libcamera/geometry.h>
>  #include <libcamera/pixel_format.h>
>
> +#include <libcamera/internal/buffer.h>
> +
>  class CameraDevice;
>  class CameraMetadata;
> -class MappedCamera3Buffer;
>  class PostProcessor;
>
>  class CameraStream
> @@ -119,9 +120,10 @@ public:
>
>  	int configure();
>  	int process(const libcamera::FrameBuffer &source,
> -		    MappedCamera3Buffer *dest,
> +		    libcamera::MappedBuffer *destination,
>  		    const CameraMetadata &requestMetadata,
>  		    CameraMetadata *resultMetadata);
> +
>  	libcamera::FrameBuffer *getBuffer();
>  	void putBuffer(libcamera::FrameBuffer *buffer);
>
> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> index cac0087b..74e81c6f 100644
> --- a/src/android/jpeg/post_processor_jpeg.cpp
> +++ b/src/android/jpeg/post_processor_jpeg.cpp
> @@ -83,7 +83,7 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,
>  }
>
>  int PostProcessorJpeg::process(const FrameBuffer &source,
> -			       Span<uint8_t> destination,
> +			       libcamera::MappedBuffer *destination,
>  			       const CameraMetadata &requestMetadata,
>  			       CameraMetadata *resultMetadata)
>  {
> @@ -171,8 +171,8 @@ int PostProcessorJpeg::process(const FrameBuffer &source,
>  	ret = requestMetadata.getEntry(ANDROID_JPEG_QUALITY, &entry);
>  	const uint8_t quality = ret ? *entry.data.u8 : 95;
>  	resultMetadata->addEntry(ANDROID_JPEG_QUALITY, &quality, 1);
> +	int jpeg_size = encoder_->encode(source, destination->maps()[0], exif.data(), quality);

I would not move this

>
> -	int jpeg_size = encoder_->encode(source, destination, exif.data(), quality);
>  	if (jpeg_size < 0) {
>  		LOG(JPEG, Error) << "Failed to encode stream image";
>  		return jpeg_size;
> @@ -190,7 +190,7 @@ int PostProcessorJpeg::process(const FrameBuffer &source,
>  	 * \todo Investigate if the buffer size mismatch is an issue or
>  	 * expected behaviour.
>  	 */
> -	uint8_t *resultPtr = destination.data() +
> +	uint8_t *resultPtr = destination->maps()[0].data() +
>  			     cameraDevice_->maxJpegBufferSize() -
>  			     sizeof(struct camera3_jpeg_blob);
>  	auto *blob = reinterpret_cast<struct camera3_jpeg_blob *>(resultPtr);
> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h
> index d2dfa450..7689de73 100644
> --- a/src/android/jpeg/post_processor_jpeg.h
> +++ b/src/android/jpeg/post_processor_jpeg.h
> @@ -25,7 +25,7 @@ public:
>  	int configure(const libcamera::StreamConfiguration &incfg,
>  		      const libcamera::StreamConfiguration &outcfg) override;
>  	int process(const libcamera::FrameBuffer &source,
> -		    libcamera::Span<uint8_t> destination,
> +		    libcamera::MappedBuffer *destination,
>  		    const CameraMetadata &requestMetadata,
>  		    CameraMetadata *resultMetadata) override;
>
> diff --git a/src/android/post_processor.h b/src/android/post_processor.h
> index bda93bb4..f5c99f03 100644
> --- a/src/android/post_processor.h
> +++ b/src/android/post_processor.h
> @@ -8,9 +8,10 @@
>  #define __ANDROID_POST_PROCESSOR_H__
>
>  #include <libcamera/buffer.h>
> -#include <libcamera/span.h>
>  #include <libcamera/stream.h>
>
> +#include <libcamera/internal/buffer.h>
> +
>  class CameraMetadata;
>
>  class PostProcessor
> @@ -21,9 +22,9 @@ public:
>  	virtual int configure(const libcamera::StreamConfiguration &inCfg,
>  			      const libcamera::StreamConfiguration &outCfg) = 0;
>  	virtual int process(const libcamera::FrameBuffer &source,
> -			    libcamera::Span<uint8_t> destination,
> +			    libcamera::MappedBuffer *destination,
>  			    const CameraMetadata &requestMetadata,
> -			    CameraMetadata *resultMetadata) = 0;
> +			    CameraMetadata *metadata) = 0;

Unrelated and not updated in the sub-classes. I would drop.

I think both can be fixed when applying, the rest looks good
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

>  };
>
>  #endif /* __ANDROID_POST_PROCESSOR_H__ */
> --
> 2.30.0.365.g02bc693789-goog
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Jacopo Mondi Feb. 2, 2021, 6:18 p.m. UTC | #2 
Hello,

On Mon, Feb 01, 2021 at 12:00:31PM +0100, Jacopo Mondi wrote:
> Hi Hiro,
>
> On Thu, Jan 28, 2021 at 10:42:15PM +0000, Hirokazu Honda wrote:
> > The type of the destination buffer in PostProcessor::process() is
> > libcamera::Span. libcamera::Span is used for one dimension buffer
> > (e.g. blob buffer). The destination can be multiple dimensions
> > buffer (e.g. yuv frame). Therefore, this changes the type of the
> > destination buffer to MappedFrameBuffer.
>
> Thanks, this will make the recalculation of the JPEG buffer size
> easier.
>
> >
> > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > ---
> >  src/android/camera_stream.cpp            | 4 ++--
> >  src/android/camera_stream.h              | 6 ++++--
> >  src/android/jpeg/post_processor_jpeg.cpp | 6 +++---
> >  src/android/jpeg/post_processor_jpeg.h   | 2 +-
> >  src/android/post_processor.h             | 7 ++++---
> >  5 files changed, 14 insertions(+), 11 deletions(-)
> >
> > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> > index 4c8020e5..611ec0d1 100644
> > --- a/src/android/camera_stream.cpp
> > +++ b/src/android/camera_stream.cpp
> > @@ -96,14 +96,14 @@ int CameraStream::configure()
> >  }
> >
> >  int CameraStream::process(const libcamera::FrameBuffer &source,
> > -			  MappedCamera3Buffer *dest,
> > +			  libcamera::MappedBuffer *destination,
> >  			  const CameraMetadata &requestMetadata,
> >  			  CameraMetadata *resultMetadata)
> >  {
> >  	if (!postProcessor_)
> >  		return 0;
> >
> > -	return postProcessor_->process(source, dest->maps()[0],
> > +	return postProcessor_->process(source, destination,
> >  				       requestMetadata, resultMetadata);
> >  }
> >
> > diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> > index 298ffbf6..73bac0ba 100644
> > --- a/src/android/camera_stream.h
> > +++ b/src/android/camera_stream.h
> > @@ -19,9 +19,10 @@
> >  #include <libcamera/geometry.h>
> >  #include <libcamera/pixel_format.h>
> >
> > +#include <libcamera/internal/buffer.h>
> > +
> >  class CameraDevice;
> >  class CameraMetadata;
> > -class MappedCamera3Buffer;
> >  class PostProcessor;
> >
> >  class CameraStream
> > @@ -119,9 +120,10 @@ public:
> >
> >  	int configure();
> >  	int process(const libcamera::FrameBuffer &source,
> > -		    MappedCamera3Buffer *dest,
> > +		    libcamera::MappedBuffer *destination,
> >  		    const CameraMetadata &requestMetadata,
> >  		    CameraMetadata *resultMetadata);
> > +
> >  	libcamera::FrameBuffer *getBuffer();
> >  	void putBuffer(libcamera::FrameBuffer *buffer);
> >
> > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> > index cac0087b..74e81c6f 100644
> > --- a/src/android/jpeg/post_processor_jpeg.cpp
> > +++ b/src/android/jpeg/post_processor_jpeg.cpp
> > @@ -83,7 +83,7 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,
> >  }
> >
> >  int PostProcessorJpeg::process(const FrameBuffer &source,
> > -			       Span<uint8_t> destination,
> > +			       libcamera::MappedBuffer *destination,
> >  			       const CameraMetadata &requestMetadata,
> >  			       CameraMetadata *resultMetadata)
> >  {
> > @@ -171,8 +171,8 @@ int PostProcessorJpeg::process(const FrameBuffer &source,
> >  	ret = requestMetadata.getEntry(ANDROID_JPEG_QUALITY, &entry);
> >  	const uint8_t quality = ret ? *entry.data.u8 : 95;
> >  	resultMetadata->addEntry(ANDROID_JPEG_QUALITY, &quality, 1);
> > +	int jpeg_size = encoder_->encode(source, destination->maps()[0], exif.data(), quality);
>
> I would not move this
>
> >
> > -	int jpeg_size = encoder_->encode(source, destination, exif.data(), quality);
> >  	if (jpeg_size < 0) {
> >  		LOG(JPEG, Error) << "Failed to encode stream image";
> >  		return jpeg_size;
> > @@ -190,7 +190,7 @@ int PostProcessorJpeg::process(const FrameBuffer &source,
> >  	 * \todo Investigate if the buffer size mismatch is an issue or
> >  	 * expected behaviour.
> >  	 */
> > -	uint8_t *resultPtr = destination.data() +
> > +	uint8_t *resultPtr = destination->maps()[0].data() +
> >  			     cameraDevice_->maxJpegBufferSize() -
> >  			     sizeof(struct camera3_jpeg_blob);
> >  	auto *blob = reinterpret_cast<struct camera3_jpeg_blob *>(resultPtr);
> > diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h
> > index d2dfa450..7689de73 100644
> > --- a/src/android/jpeg/post_processor_jpeg.h
> > +++ b/src/android/jpeg/post_processor_jpeg.h
> > @@ -25,7 +25,7 @@ public:
> >  	int configure(const libcamera::StreamConfiguration &incfg,
> >  		      const libcamera::StreamConfiguration &outcfg) override;
> >  	int process(const libcamera::FrameBuffer &source,
> > -		    libcamera::Span<uint8_t> destination,
> > +		    libcamera::MappedBuffer *destination,
> >  		    const CameraMetadata &requestMetadata,
> >  		    CameraMetadata *resultMetadata) override;
> >
> > diff --git a/src/android/post_processor.h b/src/android/post_processor.h
> > index bda93bb4..f5c99f03 100644
> > --- a/src/android/post_processor.h
> > +++ b/src/android/post_processor.h
> > @@ -8,9 +8,10 @@
> >  #define __ANDROID_POST_PROCESSOR_H__
> >
> >  #include <libcamera/buffer.h>
> > -#include <libcamera/span.h>
> >  #include <libcamera/stream.h>
> >
> > +#include <libcamera/internal/buffer.h>
> > +
> >  class CameraMetadata;
> >
> >  class PostProcessor
> > @@ -21,9 +22,9 @@ public:
> >  	virtual int configure(const libcamera::StreamConfiguration &inCfg,
> >  			      const libcamera::StreamConfiguration &outCfg) = 0;
> >  	virtual int process(const libcamera::FrameBuffer &source,
> > -			    libcamera::Span<uint8_t> destination,
> > +			    libcamera::MappedBuffer *destination,
> >  			    const CameraMetadata &requestMetadata,
> > -			    CameraMetadata *resultMetadata) = 0;
> > +			    CameraMetadata *metadata) = 0;
>
> Unrelated and not updated in the sub-classes. I would drop.
>
> I think both can be fixed when applying, the rest looks good
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

I've applied this one with the minors pointed out here above.

Thanks
  j

>
> Thanks
>   j
>
> >  };
> >
> >  #endif /* __ANDROID_POST_PROCESSOR_H__ */
> > --
> > 2.30.0.365.g02bc693789-goog
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Patch
diff mbox series 

diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
index 4c8020e5..611ec0d1 100644
--- a/src/android/camera_stream.cpp
+++ b/src/android/camera_stream.cpp
@@ -96,14 +96,14 @@  int CameraStream::configure()
 }

 int CameraStream::process(const libcamera::FrameBuffer &source,
-			  MappedCamera3Buffer *dest,
+			  libcamera::MappedBuffer *destination,
 			  const CameraMetadata &requestMetadata,
 			  CameraMetadata *resultMetadata)
 {
 	if (!postProcessor_)
 		return 0;

-	return postProcessor_->process(source, dest->maps()[0],
+	return postProcessor_->process(source, destination,
 				       requestMetadata, resultMetadata);
 }

diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
index 298ffbf6..73bac0ba 100644
--- a/src/android/camera_stream.h
+++ b/src/android/camera_stream.h
@@ -19,9 +19,10 @@ 
 #include <libcamera/geometry.h>
 #include <libcamera/pixel_format.h>

+#include <libcamera/internal/buffer.h>
+
 class CameraDevice;
 class CameraMetadata;
-class MappedCamera3Buffer;
 class PostProcessor;

 class CameraStream
@@ -119,9 +120,10 @@  public:

 	int configure();
 	int process(const libcamera::FrameBuffer &source,
-		    MappedCamera3Buffer *dest,
+		    libcamera::MappedBuffer *destination,
 		    const CameraMetadata &requestMetadata,
 		    CameraMetadata *resultMetadata);
+
 	libcamera::FrameBuffer *getBuffer();
 	void putBuffer(libcamera::FrameBuffer *buffer);

diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
index cac0087b..74e81c6f 100644
--- a/src/android/jpeg/post_processor_jpeg.cpp
+++ b/src/android/jpeg/post_processor_jpeg.cpp
@@ -83,7 +83,7 @@  void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,
 }

 int PostProcessorJpeg::process(const FrameBuffer &source,
-			       Span<uint8_t> destination,
+			       libcamera::MappedBuffer *destination,
 			       const CameraMetadata &requestMetadata,
 			       CameraMetadata *resultMetadata)
 {
@@ -171,8 +171,8 @@  int PostProcessorJpeg::process(const FrameBuffer &source,
 	ret = requestMetadata.getEntry(ANDROID_JPEG_QUALITY, &entry);
 	const uint8_t quality = ret ? *entry.data.u8 : 95;
 	resultMetadata->addEntry(ANDROID_JPEG_QUALITY, &quality, 1);
+	int jpeg_size = encoder_->encode(source, destination->maps()[0], exif.data(), quality);

-	int jpeg_size = encoder_->encode(source, destination, exif.data(), quality);
 	if (jpeg_size < 0) {
 		LOG(JPEG, Error) << "Failed to encode stream image";
 		return jpeg_size;
@@ -190,7 +190,7 @@  int PostProcessorJpeg::process(const FrameBuffer &source,
 	 * \todo Investigate if the buffer size mismatch is an issue or
 	 * expected behaviour.
 	 */
-	uint8_t *resultPtr = destination.data() +
+	uint8_t *resultPtr = destination->maps()[0].data() +
 			     cameraDevice_->maxJpegBufferSize() -
 			     sizeof(struct camera3_jpeg_blob);
 	auto *blob = reinterpret_cast<struct camera3_jpeg_blob *>(resultPtr);
diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h
index d2dfa450..7689de73 100644
--- a/src/android/jpeg/post_processor_jpeg.h
+++ b/src/android/jpeg/post_processor_jpeg.h
@@ -25,7 +25,7 @@  public:
 	int configure(const libcamera::StreamConfiguration &incfg,
 		      const libcamera::StreamConfiguration &outcfg) override;
 	int process(const libcamera::FrameBuffer &source,
-		    libcamera::Span<uint8_t> destination,
+		    libcamera::MappedBuffer *destination,
 		    const CameraMetadata &requestMetadata,
 		    CameraMetadata *resultMetadata) override;

diff --git a/src/android/post_processor.h b/src/android/post_processor.h
index bda93bb4..f5c99f03 100644
--- a/src/android/post_processor.h
+++ b/src/android/post_processor.h
@@ -8,9 +8,10 @@ 
 #define __ANDROID_POST_PROCESSOR_H__

 #include <libcamera/buffer.h>
-#include <libcamera/span.h>
 #include <libcamera/stream.h>

+#include <libcamera/internal/buffer.h>
+
 class CameraMetadata;

 class PostProcessor
@@ -21,9 +22,9 @@  public:
 	virtual int configure(const libcamera::StreamConfiguration &inCfg,
 			      const libcamera::StreamConfiguration &outCfg) = 0;
 	virtual int process(const libcamera::FrameBuffer &source,
-			    libcamera::Span<uint8_t> destination,
+			    libcamera::MappedBuffer *destination,
 			    const CameraMetadata &requestMetadata,
-			    CameraMetadata *resultMetadata) = 0;
+			    CameraMetadata *metadata) = 0;
 };

 #endif /* __ANDROID_POST_PROCESSOR_H__ */