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

Message ID 20210122092335.254626-1-hiroh@chromium.org
State Superseded
Headers show
Series
  • [libcamera-devel,1/2] android: post_processor: Change the type destination in process()
Related show

Commit Message

Hirokazu Honda Jan. 22, 2021, 9:23 a.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>
---
 src/android/camera_stream.cpp            | 5 +++--
 src/android/camera_stream.h              | 5 +++--
 src/android/jpeg/post_processor_jpeg.cpp | 7 ++++---
 src/android/jpeg/post_processor_jpeg.h   | 2 +-
 src/android/post_processor.h             | 3 ++-
 5 files changed, 13 insertions(+), 9 deletions(-)

--
2.30.0.280.ga3ce27912f-goog

Comments

Kieran Bingham Jan. 22, 2021, 12:51 p.m. UTC | #1 
Hi Hiro,

On 22/01/2021 09:23, 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.
> 
> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> ---
>  src/android/camera_stream.cpp            | 5 +++--
>  src/android/camera_stream.h              | 5 +++--
>  src/android/jpeg/post_processor_jpeg.cpp | 7 ++++---
>  src/android/jpeg/post_processor_jpeg.h   | 2 +-
>  src/android/post_processor.h             | 3 ++-
>  5 files changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> index dba351a4..33d0e005 100644
> --- a/src/android/camera_stream.cpp
> +++ b/src/android/camera_stream.cpp
> @@ -96,12 +96,13 @@ int CameraStream::configure()
>  }
> 
>  int CameraStream::process(const libcamera::FrameBuffer &source,
> -			  MappedCamera3Buffer *dest, CameraMetadata *metadata)
> +			  libcamera::MappedBuffer *destination,
> +			  CameraMetadata *metadata)
>  {
>  	if (!postProcessor_)
>  		return 0;
> 
> -	return postProcessor_->process(source, dest->maps()[0], metadata);
> +	return postProcessor_->process(source, destination, metadata);
>  }
> 
>  FrameBuffer *CameraStream::getBuffer()
> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> index cc9d5470..7d7b2b16 100644
> --- a/src/android/camera_stream.h
> +++ b/src/android/camera_stream.h
> @@ -17,11 +17,11 @@
>  #include <libcamera/camera.h>
>  #include <libcamera/framebuffer_allocator.h>
>  #include <libcamera/geometry.h>
> +#include <libcamera/internal/buffer.h>

internal includes should have their own include block separately, after
the other libcamera headers.


>  #include <libcamera/pixel_format.h>
> 

so it would go here. with a blank line each side.

>  class CameraDevice;
>  class CameraMetadata;
> -class MappedCamera3Buffer;
>  class PostProcessor;
> 
>  class CameraStream
> @@ -119,7 +119,8 @@ public:
> 
>  	int configure();
>  	int process(const libcamera::FrameBuffer &source,
> -		    MappedCamera3Buffer *dest, CameraMetadata *metadata);
> +		    libcamera::MappedBuffer *destination,
> +		    CameraMetadata *metadata);
>  	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 436a50f8..2374716d 100644
> --- a/src/android/jpeg/post_processor_jpeg.cpp
> +++ b/src/android/jpeg/post_processor_jpeg.cpp
> @@ -79,7 +79,7 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,
>  }
> 
>  int PostProcessorJpeg::process(const FrameBuffer &source,
> -			       Span<uint8_t> destination,
> +			       libcamera::MappedBuffer *destination,
>  			       CameraMetadata *metadata)
>  {
>  	if (!encoder_)
> @@ -107,7 +107,8 @@ int PostProcessorJpeg::process(const FrameBuffer &source,
>  	if (exif.generate() != 0)
>  		LOG(JPEG, Error) << "Failed to generate valid EXIF data";
> 
> -	int jpeg_size = encoder_->encode(source, destination, exif.data());
> +	int jpeg_size = encoder_->encode(source, destination->maps()[0],
> +					 exif.data());
>  	if (jpeg_size < 0) {
>  		LOG(JPEG, Error) << "Failed to encode stream image";
>  		return jpeg_size;
> @@ -125,7 +126,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 5afa831c..68a38410 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,
>  		    CameraMetadata *metadata) override;
> 
>  private:
> diff --git a/src/android/post_processor.h b/src/android/post_processor.h
> index e0f91880..368e0fe5 100644
> --- a/src/android/post_processor.h
> +++ b/src/android/post_processor.h
> @@ -7,6 +7,7 @@
>  #ifndef __ANDROID_POST_PROCESSOR_H__
>  #define __ANDROID_POST_PROCESSOR_H__
> 
> +#include <libcamera/internal/buffer.h>

Same here, this should go in it's own block after this one.

Except for the header locations, which is quite trivial, this looks ok
to me.

We might expect discussion of using internal headers in the Android
layer, but actually we are already using internal components there so I
don't think this is an issue.

In fact of course we're already using the MappedBuffers as part of the
MappedCamera3Buffer so I think this is fine all round.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

>  #include <libcamera/buffer.h>
>  #include <libcamera/span.h>
>  #include <libcamera/stream.h>> @@ -21,7 +22,7 @@ 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,
>  			    CameraMetadata *metadata) = 0;
>  };
> 
> --
> 2.30.0.280.ga3ce27912f-goog
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
>
Laurent Pinchart Jan. 26, 2021, 8:56 a.m. UTC | #2 
Hi Hiro,

Thank you for the patch.

On Fri, Jan 22, 2021 at 12:51:27PM +0000, Kieran Bingham wrote:
> On 22/01/2021 09:23, 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.
> > 
> > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > ---
> >  src/android/camera_stream.cpp            | 5 +++--
> >  src/android/camera_stream.h              | 5 +++--
> >  src/android/jpeg/post_processor_jpeg.cpp | 7 ++++---
> >  src/android/jpeg/post_processor_jpeg.h   | 2 +-
> >  src/android/post_processor.h             | 3 ++-
> >  5 files changed, 13 insertions(+), 9 deletions(-)
> > 
> > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> > index dba351a4..33d0e005 100644
> > --- a/src/android/camera_stream.cpp
> > +++ b/src/android/camera_stream.cpp
> > @@ -96,12 +96,13 @@ int CameraStream::configure()
> >  }
> > 
> >  int CameraStream::process(const libcamera::FrameBuffer &source,
> > -			  MappedCamera3Buffer *dest, CameraMetadata *metadata)
> > +			  libcamera::MappedBuffer *destination,
> > +			  CameraMetadata *metadata)
> >  {
> >  	if (!postProcessor_)
> >  		return 0;
> > 
> > -	return postProcessor_->process(source, dest->maps()[0], metadata);
> > +	return postProcessor_->process(source, destination, metadata);
> >  }
> > 
> >  FrameBuffer *CameraStream::getBuffer()
> > diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> > index cc9d5470..7d7b2b16 100644
> > --- a/src/android/camera_stream.h
> > +++ b/src/android/camera_stream.h
> > @@ -17,11 +17,11 @@
> >  #include <libcamera/camera.h>
> >  #include <libcamera/framebuffer_allocator.h>
> >  #include <libcamera/geometry.h>
> > +#include <libcamera/internal/buffer.h>
> 
> internal includes should have their own include block separately, after
> the other libcamera headers.
> 
> >  #include <libcamera/pixel_format.h>
> > 
> 
> so it would go here. with a blank line each side.
> 
> >  class CameraDevice;
> >  class CameraMetadata;
> > -class MappedCamera3Buffer;
> >  class PostProcessor;
> > 
> >  class CameraStream
> > @@ -119,7 +119,8 @@ public:
> > 
> >  	int configure();
> >  	int process(const libcamera::FrameBuffer &source,
> > -		    MappedCamera3Buffer *dest, CameraMetadata *metadata);
> > +		    libcamera::MappedBuffer *destination,
> > +		    CameraMetadata *metadata);
> >  	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 436a50f8..2374716d 100644
> > --- a/src/android/jpeg/post_processor_jpeg.cpp
> > +++ b/src/android/jpeg/post_processor_jpeg.cpp
> > @@ -79,7 +79,7 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,
> >  }
> > 
> >  int PostProcessorJpeg::process(const FrameBuffer &source,
> > -			       Span<uint8_t> destination,
> > +			       libcamera::MappedBuffer *destination,
> >  			       CameraMetadata *metadata)
> >  {
> >  	if (!encoder_)
> > @@ -107,7 +107,8 @@ int PostProcessorJpeg::process(const FrameBuffer &source,
> >  	if (exif.generate() != 0)
> >  		LOG(JPEG, Error) << "Failed to generate valid EXIF data";
> > 
> > -	int jpeg_size = encoder_->encode(source, destination, exif.data());
> > +	int jpeg_size = encoder_->encode(source, destination->maps()[0],
> > +					 exif.data());
> >  	if (jpeg_size < 0) {
> >  		LOG(JPEG, Error) << "Failed to encode stream image";
> >  		return jpeg_size;
> > @@ -125,7 +126,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 5afa831c..68a38410 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,
> >  		    CameraMetadata *metadata) override;
> > 
> >  private:
> > diff --git a/src/android/post_processor.h b/src/android/post_processor.h
> > index e0f91880..368e0fe5 100644
> > --- a/src/android/post_processor.h
> > +++ b/src/android/post_processor.h
> > @@ -7,6 +7,7 @@
> >  #ifndef __ANDROID_POST_PROCESSOR_H__
> >  #define __ANDROID_POST_PROCESSOR_H__
> > 
> > +#include <libcamera/internal/buffer.h>
> 
> Same here, this should go in it's own block after this one.
> 
> Except for the header locations, which is quite trivial, this looks ok
> to me.
> 
> We might expect discussion of using internal headers in the Android
> layer, but actually we are already using internal components there so I
> don't think this is an issue.

Given that the HAL is compiled in the same shared object, I don't think
it's a big issue :-) We may split it to a separate library in the
future, but it would still be developed in lockstep with the libcamera
core, so I don't have any problem using the internal API. The only issue
I can think about now would be if we wanted to hide the internal API
from the symbols exported by the libcamera core, using
https://gcc.gnu.org/wiki/Visibility.

> In fact of course we're already using the MappedBuffers as part of the
> MappedCamera3Buffer so I think this is fine all round.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
> >  #include <libcamera/buffer.h>
> >  #include <libcamera/span.h>

You can drop span.h.

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

> >  #include <libcamera/stream.h>> @@ -21,7 +22,7 @@ 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,
> >  			    CameraMetadata *metadata) = 0;
> >  };
> >

Patch
diff mbox series 

diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
index dba351a4..33d0e005 100644
--- a/src/android/camera_stream.cpp
+++ b/src/android/camera_stream.cpp
@@ -96,12 +96,13 @@  int CameraStream::configure()
 }

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

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

 FrameBuffer *CameraStream::getBuffer()
diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
index cc9d5470..7d7b2b16 100644
--- a/src/android/camera_stream.h
+++ b/src/android/camera_stream.h
@@ -17,11 +17,11 @@ 
 #include <libcamera/camera.h>
 #include <libcamera/framebuffer_allocator.h>
 #include <libcamera/geometry.h>
+#include <libcamera/internal/buffer.h>
 #include <libcamera/pixel_format.h>

 class CameraDevice;
 class CameraMetadata;
-class MappedCamera3Buffer;
 class PostProcessor;

 class CameraStream
@@ -119,7 +119,8 @@  public:

 	int configure();
 	int process(const libcamera::FrameBuffer &source,
-		    MappedCamera3Buffer *dest, CameraMetadata *metadata);
+		    libcamera::MappedBuffer *destination,
+		    CameraMetadata *metadata);
 	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 436a50f8..2374716d 100644
--- a/src/android/jpeg/post_processor_jpeg.cpp
+++ b/src/android/jpeg/post_processor_jpeg.cpp
@@ -79,7 +79,7 @@  void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,
 }

 int PostProcessorJpeg::process(const FrameBuffer &source,
-			       Span<uint8_t> destination,
+			       libcamera::MappedBuffer *destination,
 			       CameraMetadata *metadata)
 {
 	if (!encoder_)
@@ -107,7 +107,8 @@  int PostProcessorJpeg::process(const FrameBuffer &source,
 	if (exif.generate() != 0)
 		LOG(JPEG, Error) << "Failed to generate valid EXIF data";

-	int jpeg_size = encoder_->encode(source, destination, exif.data());
+	int jpeg_size = encoder_->encode(source, destination->maps()[0],
+					 exif.data());
 	if (jpeg_size < 0) {
 		LOG(JPEG, Error) << "Failed to encode stream image";
 		return jpeg_size;
@@ -125,7 +126,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 5afa831c..68a38410 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,
 		    CameraMetadata *metadata) override;

 private:
diff --git a/src/android/post_processor.h b/src/android/post_processor.h
index e0f91880..368e0fe5 100644
--- a/src/android/post_processor.h
+++ b/src/android/post_processor.h
@@ -7,6 +7,7 @@ 
 #ifndef __ANDROID_POST_PROCESSOR_H__
 #define __ANDROID_POST_PROCESSOR_H__

+#include <libcamera/internal/buffer.h>
 #include <libcamera/buffer.h>
 #include <libcamera/span.h>
 #include <libcamera/stream.h>
@@ -21,7 +22,7 @@  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,
 			    CameraMetadata *metadata) = 0;
 };