[libcamera-devel] [PATCH 1/2] android: post_processor: Change the type destination in process()
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Jan 26 09:56:03 CET 2021
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 at 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 at ideasonboard.com>
>
> > #include <libcamera/buffer.h>
> > #include <libcamera/span.h>
You can drop span.h.
Reviewed-by: Laurent Pinchart <laurent.pinchart at 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;
> > };
> >
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list