[libcamera-devel] [PATCH 08/12] android: post_processor: Use CameraBuffer API
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sun Feb 28 19:42:17 CET 2021
Hi Jacopo,
Thank you for the patch.
On Fri, Feb 26, 2021 at 02:29:28PM +0100, Jacopo Mondi wrote:
> Use the newly introduced CameraBuffer class as the type for the
> destination buffer in the PostProcessor class hierarchy in place of the
> libcamera::MappedFrameBuffer one and use its API to retrieve the length
> and the location of the CameraBuffer plane allocated for JPEG
> post-processing.
>
> Remove all the assumption on the underlying memory storage and only go
> through the CameraBuffer API when dealing with memory buffers. To do so
> rework the Encoder interface to use a raw pointer and an explicit size
> to remove access to the Span<uint8_t> maps that serve as memory storage
> for the current implementation but might not be ideal for other memory
> backend.
>
> Now that the whole PostProcessor hierarchy has been converted to use
> the CameraBuffer API remove libcamera::MappedBuffer as base class
> of the CameraBuffer interface and only reply on its interface.
>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
> src/android/camera_buffer.h | 2 +-
> src/android/jpeg/encoder.h | 4 +++-
> src/android/jpeg/encoder_libjpeg.cpp | 19 ++++++++--------
> src/android/jpeg/encoder_libjpeg.h | 4 ++--
> src/android/jpeg/post_processor_jpeg.cpp | 28 ++++++++++--------------
> src/android/jpeg/post_processor_jpeg.h | 2 +-
> src/android/post_processor.h | 4 +++-
> src/android/yuv/post_processor_yuv.cpp | 20 ++++++++---------
> src/android/yuv/post_processor_yuv.h | 4 ++--
> 9 files changed, 42 insertions(+), 45 deletions(-)
>
> diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h
> index 2a91e6a3c9c1..b251e4514864 100644
> --- a/src/android/camera_buffer.h
> +++ b/src/android/camera_buffer.h
> @@ -11,7 +11,7 @@
>
> #include <libcamera/internal/buffer.h>
>
> -class CameraBuffer : public libcamera::MappedBuffer
> +class CameraBuffer
> {
> public:
> CameraBuffer(buffer_handle_t camera3Buffer, int flags);
> diff --git a/src/android/jpeg/encoder.h b/src/android/jpeg/encoder.h
> index 8d449369869f..a3c47c09a06e 100644
> --- a/src/android/jpeg/encoder.h
> +++ b/src/android/jpeg/encoder.h
> @@ -11,6 +11,8 @@
> #include <libcamera/span.h>
> #include <libcamera/stream.h>
>
> +#include "../camera_buffer.h"
This doesn't seem to be needed.
> +
> class Encoder
> {
> public:
> @@ -18,7 +20,7 @@ public:
>
> virtual int configure(const libcamera::StreamConfiguration &cfg) = 0;
> virtual int encode(const libcamera::FrameBuffer &source,
> - libcamera::Span<uint8_t> destination,
> + uint8_t *destination, size_t destinationSize,
Whenever we need to pass a pointer and a size, a span is the right tool.
It's its core purpose, grouping pointer and size together. This will not
support multi-planar buffers, but neither will a pointer and a size, so
the interface will need to be reworked later.
> libcamera::Span<const uint8_t> exifData,
> unsigned int quality) = 0;
> };
> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp
> index f006e1d1999a..a965620703df 100644
> --- a/src/android/jpeg/encoder_libjpeg.cpp
> +++ b/src/android/jpeg/encoder_libjpeg.cpp
> @@ -177,8 +177,9 @@ void EncoderLibJpeg::compressNV(Span<const uint8_t> frame)
> }
> }
>
> -int EncoderLibJpeg::encode(const FrameBuffer &source, Span<uint8_t> dest,
> - Span<const uint8_t> exifData, unsigned int quality)
> +int EncoderLibJpeg::encode(const FrameBuffer &source, uint8_t *destination,
> + size_t destinationSize, Span<const uint8_t> exifData,
> + unsigned int quality)
> {
> MappedFrameBuffer frame(&source, PROT_READ);
> if (!frame.isValid()) {
> @@ -187,15 +188,13 @@ int EncoderLibJpeg::encode(const FrameBuffer &source, Span<uint8_t> dest,
> return frame.error();
> }
>
> - return encode(frame.maps()[0], dest, exifData, quality);
> + return encode(frame.maps()[0], destination, destinationSize, exifData, quality);
> }
>
> -int EncoderLibJpeg::encode(Span<const uint8_t> src, Span<uint8_t> dest,
> - Span<const uint8_t> exifData, unsigned int quality)
> +int EncoderLibJpeg::encode(Span<const uint8_t> src, uint8_t *destination,
> + size_t destinationSize, Span<const uint8_t> exifData,
> + unsigned int quality)
> {
> - unsigned char *destination = dest.data();
> - unsigned long size = dest.size();
> -
> jpeg_set_quality(&compress_, quality, TRUE);
>
> /*
> @@ -206,7 +205,7 @@ int EncoderLibJpeg::encode(Span<const uint8_t> src, Span<uint8_t> dest,
> * \todo Implement our own custom memory destination to prevent
> * reallocation and prefer failure with correct reporting.
> */
> - jpeg_mem_dest(&compress_, &destination, &size);
> + jpeg_mem_dest(&compress_, &destination, &destinationSize);
>
> jpeg_start_compress(&compress_, TRUE);
>
> @@ -226,5 +225,5 @@ int EncoderLibJpeg::encode(Span<const uint8_t> src, Span<uint8_t> dest,
>
> jpeg_finish_compress(&compress_);
>
> - return size;
> + return destinationSize;
> }
> diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h
> index 838da7728382..bda5c16089df 100644
> --- a/src/android/jpeg/encoder_libjpeg.h
> +++ b/src/android/jpeg/encoder_libjpeg.h
> @@ -22,11 +22,11 @@ public:
>
> int configure(const libcamera::StreamConfiguration &cfg) override;
> int encode(const libcamera::FrameBuffer &source,
> - libcamera::Span<uint8_t> destination,
> + uint8_t *destination, size_t destinationSize,
> libcamera::Span<const uint8_t> exifData,
> unsigned int quality) override;
> int encode(libcamera::Span<const uint8_t> source,
> - libcamera::Span<uint8_t> destination,
> + uint8_t *destination, size_t destinationSize,
> libcamera::Span<const uint8_t> exifData,
> unsigned int quality);
>
> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> index ab5b63844067..d6eeb962e81d 100644
> --- a/src/android/jpeg/post_processor_jpeg.cpp
> +++ b/src/android/jpeg/post_processor_jpeg.cpp
> @@ -73,7 +73,9 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,
> thumbnail->resize(rawThumbnail.size());
>
> int jpeg_size = thumbnailEncoder_.encode(rawThumbnail,
> - *thumbnail, {}, quality);
> + thumbnail->data(),
> + thumbnail->size(),
> + {}, quality);
> thumbnail->resize(jpeg_size);
>
> LOG(JPEG, Debug)
> @@ -83,13 +85,15 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,
> }
>
> int PostProcessorJpeg::process(const FrameBuffer &source,
> - libcamera::MappedBuffer *destination,
> + CameraBuffer *destination,
> const CameraMetadata &requestMetadata,
> CameraMetadata *resultMetadata)
> {
> if (!encoder_)
> return 0;
>
> + ASSERT(destination->numPlanes() == 1);
> +
> camera_metadata_ro_entry_t entry;
> int ret;
>
> @@ -172,27 +176,17 @@ int PostProcessorJpeg::process(const FrameBuffer &source,
> 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],
> + int jpeg_size = encoder_->encode(source, destination->plane(0),
> + destination->planeSize(0),
The CameraBuffer::plane() function should return a span, and I'd then
drop CameraBuffer::planeSize().
> exif.data(), quality);
> if (jpeg_size < 0) {
> LOG(JPEG, Error) << "Failed to encode stream image";
> return jpeg_size;
> }
>
> - /*
> - * Fill in the JPEG blob header.
> - *
> - * The mapped size of the buffer is being returned as
> - * substantially larger than the requested JPEG_MAX_SIZE
> - * (which is referenced from maxJpegBufferSize_). Utilise
> - * this static size to ensure the correct offset of the blob is
> - * determined.
> - *
> - * \todo Investigate if the buffer size mismatch is an issue or
> - * expected behaviour.
> - */
> - uint8_t *resultPtr = destination->maps()[0].data() +
> - cameraDevice_->maxJpegBufferSize() -
> + /* Fill in the JPEG blob header. */
> + uint8_t *resultPtr = destination->plane(0) +
> + destination->planeSize(0) -
> sizeof(struct camera3_jpeg_blob);
> auto *blob = reinterpret_cast<struct camera3_jpeg_blob *>(resultPtr);
> blob->jpeg_blob_id = CAMERA3_JPEG_BLOB_ID;
> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h
> index 7689de73c664..5d2d4ab224b1 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::MappedBuffer *destination,
> + CameraBuffer *destination,
> const CameraMetadata &requestMetadata,
> CameraMetadata *resultMetadata) override;
>
> diff --git a/src/android/post_processor.h b/src/android/post_processor.h
> index ac40d3414892..4944078b490c 100644
> --- a/src/android/post_processor.h
> +++ b/src/android/post_processor.h
> @@ -12,6 +12,8 @@
>
> #include <libcamera/internal/buffer.h>
>
> +#include "camera_buffer.h"
> +
Maybe a forward declaration of CameraBuffer would be enough here ?
The rest of the patch looks good to me. With these issues addressed,
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> class CameraMetadata;
>
> class PostProcessor
> @@ -22,7 +24,7 @@ public:
> virtual int configure(const libcamera::StreamConfiguration &inCfg,
> const libcamera::StreamConfiguration &outCfg) = 0;
> virtual int process(const libcamera::FrameBuffer &source,
> - libcamera::MappedBuffer *destination,
> + CameraBuffer *destination,
> const CameraMetadata &requestMetadata,
> CameraMetadata *resultMetadata) = 0;
> };
> diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp
> index 1318349ad66b..f1487185a95a 100644
> --- a/src/android/yuv/post_processor_yuv.cpp
> +++ b/src/android/yuv/post_processor_yuv.cpp
> @@ -48,7 +48,7 @@ int PostProcessorYuv::configure(const StreamConfiguration &inCfg,
> }
>
> int PostProcessorYuv::process(const FrameBuffer &source,
> - libcamera::MappedBuffer *destination,
> + CameraBuffer *destination,
> [[maybe_unused]] const CameraMetadata &requestMetadata,
> [[maybe_unused]] CameraMetadata *metadata)
> {
> @@ -66,9 +66,9 @@ int PostProcessorYuv::process(const FrameBuffer &source,
> sourceMapped.maps()[1].data(),
> sourceStride_[1],
> sourceSize_.width, sourceSize_.height,
> - destination->maps()[0].data(),
> + destination->plane(0),
> destinationStride_[0],
> - destination->maps()[1].data(),
> + destination->plane(1),
> destinationStride_[1],
> destinationSize_.width,
> destinationSize_.height,
> @@ -82,16 +82,16 @@ int PostProcessorYuv::process(const FrameBuffer &source,
> }
>
> bool PostProcessorYuv::isValidBuffers(const FrameBuffer &source,
> - const libcamera::MappedBuffer &destination) const
> + const CameraBuffer &destination) const
> {
> if (source.planes().size() != 2) {
> LOG(YUV, Error) << "Invalid number of source planes: "
> << source.planes().size();
> return false;
> }
> - if (destination.maps().size() != 2) {
> + if (destination.numPlanes() != 2) {
> LOG(YUV, Error) << "Invalid number of destination planes: "
> - << destination.maps().size();
> + << destination.numPlanes();
> return false;
> }
>
> @@ -106,12 +106,12 @@ bool PostProcessorYuv::isValidBuffers(const FrameBuffer &source,
> << sourceLength_[1] << "}";
> return false;
> }
> - if (destination.maps()[0].size() < destinationLength_[0] ||
> - destination.maps()[1].size() < destinationLength_[1]) {
> + if (destination.planeSize(0) < destinationLength_[0] ||
> + destination.planeSize(1) < destinationLength_[1]) {
> LOG(YUV, Error)
> << "The destination planes lengths are too small, actual size: {"
> - << destination.maps()[0].size() << ", "
> - << destination.maps()[1].size()
> + << destination.planeSize(0) << ", "
> + << destination.planeSize(1)
> << "}, expected size: {"
> << sourceLength_[0] << ", "
> << sourceLength_[1] << "}";
> diff --git a/src/android/yuv/post_processor_yuv.h b/src/android/yuv/post_processor_yuv.h
> index c58b4cf790fc..f8b1ba23fa6c 100644
> --- a/src/android/yuv/post_processor_yuv.h
> +++ b/src/android/yuv/post_processor_yuv.h
> @@ -21,13 +21,13 @@ public:
> int configure(const libcamera::StreamConfiguration &incfg,
> const libcamera::StreamConfiguration &outcfg) override;
> int process(const libcamera::FrameBuffer &source,
> - libcamera::MappedBuffer *destination,
> + CameraBuffer *destination,
> const CameraMetadata &requestMetadata,
> CameraMetadata *metadata) override;
>
> private:
> bool isValidBuffers(const libcamera::FrameBuffer &source,
> - const libcamera::MappedBuffer &destination) const;
> + const CameraBuffer &destination) const;
> void calculateLengths(const libcamera::StreamConfiguration &inCfg,
> const libcamera::StreamConfiguration &outCfg);
>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list