[libcamera-devel] [PATCH v3 1/3] android: generic_camera_buffer: Correct buffer mapping
Jacopo Mondi
jacopo at jmondi.org
Thu Aug 26 23:43:14 CEST 2021
Hi Hiro,
On Thu, Aug 26, 2021 at 05:00:19PM +0900, Hirokazu Honda wrote:
> buffer_handle_t doesn't provide sufficient info to map a buffer
> properly. cros::CameraBufferManager enables handling the buffer on
> ChromeOS, but no way is provided for other platforms.
>
> Therefore, we put the assumption that planes are in the same buffer
> and they are consecutive. This modifies the way of mapping in
> generic_camera_buffer with the assumption.
>
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
Thanks
j
> ---
> src/android/camera_buffer.h | 14 ++-
> src/android/camera_stream.cpp | 4 +-
> src/android/mm/cros_camera_buffer.cpp | 7 +-
> src/android/mm/generic_camera_buffer.cpp | 103 ++++++++++++++++++-----
> 4 files changed, 100 insertions(+), 28 deletions(-)
>
> diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h
> index c4e3a9e7..87df2570 100644
> --- a/src/android/camera_buffer.h
> +++ b/src/android/camera_buffer.h
> @@ -11,13 +11,17 @@
>
> #include <libcamera/base/class.h>
> #include <libcamera/base/span.h>
> +#include <libcamera/geometry.h>
> +#include <libcamera/pixel_format.h>
>
> class CameraBuffer final : public libcamera::Extensible
> {
> LIBCAMERA_DECLARE_PRIVATE()
>
> public:
> - CameraBuffer(buffer_handle_t camera3Buffer, int flags);
> + CameraBuffer(buffer_handle_t camera3Buffer,
> + libcamera::PixelFormat pixelFormat,
> + const libcamera::Size &size, int flags);
> ~CameraBuffer();
>
> bool isValid() const;
> @@ -31,8 +35,12 @@ public:
> };
>
> #define PUBLIC_CAMERA_BUFFER_IMPLEMENTATION \
> -CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags) \
> - : Extensible(std::make_unique<Private>(this, camera3Buffer, flags)) \
> +CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, \
> + libcamera::PixelFormat pixelFormat, \
> + const libcamera::Size &size, int flags) \
> + : Extensible(std::make_unique<Private>(this, camera3Buffer, \
> + pixelFormat, size, \
> + flags)) \
> { \
> } \
> CameraBuffer::~CameraBuffer() \
> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> index 61b44183..01909ec7 100644
> --- a/src/android/camera_stream.cpp
> +++ b/src/android/camera_stream.cpp
> @@ -110,7 +110,9 @@ int CameraStream::process(const libcamera::FrameBuffer &source,
> * \todo Buffer mapping and processing should be moved to a
> * separate thread.
> */
> - CameraBuffer dest(camera3Dest, PROT_READ | PROT_WRITE);
> + const StreamConfiguration &output = configuration();
> + CameraBuffer dest(camera3Dest, formats::MJPEG, output.size,
> + PROT_READ | PROT_WRITE);
> if (!dest.isValid()) {
> LOG(HAL, Error) << "Failed to map android blob buffer";
> return -EINVAL;
> diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp
> index e8783ff8..50732637 100644
> --- a/src/android/mm/cros_camera_buffer.cpp
> +++ b/src/android/mm/cros_camera_buffer.cpp
> @@ -20,8 +20,9 @@ class CameraBuffer::Private : public Extensible::Private
> LIBCAMERA_DECLARE_PUBLIC(CameraBuffer)
>
> public:
> - Private(CameraBuffer *cameraBuffer,
> - buffer_handle_t camera3Buffer, int flags);
> + Private(CameraBuffer *cameraBuffer, buffer_handle_t camera3Buffer,
> + libcamera::PixelFormat pixelFormat, const libcamera::Size &size,
> + int flags);
> ~Private();
>
> bool isValid() const { return valid_; }
> @@ -46,6 +47,8 @@ private:
>
> CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
> buffer_handle_t camera3Buffer,
> + [[maybe_unused]] libcamera::PixelFormat pixelFormat,
> + [[maybe_unused]] const libcamera::Size &size,
> [[maybe_unused]] int flags)
> : handle_(camera3Buffer), numPlanes_(0), valid_(false),
> registered_(false)
> diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp
> index b3af194c..22753490 100644
> --- a/src/android/mm/generic_camera_buffer.cpp
> +++ b/src/android/mm/generic_camera_buffer.cpp
> @@ -12,6 +12,7 @@
>
> #include <libcamera/base/log.h>
>
> +#include "libcamera/internal/formats.h"
> #include "libcamera/internal/mapped_framebuffer.h"
>
> using namespace libcamera;
> @@ -24,8 +25,9 @@ class CameraBuffer::Private : public Extensible::Private,
> LIBCAMERA_DECLARE_PUBLIC(CameraBuffer)
>
> public:
> - Private(CameraBuffer *cameraBuffer,
> - buffer_handle_t camera3Buffer, int flags);
> + Private(CameraBuffer *cameraBuffer, buffer_handle_t camera3Buffer,
> + libcamera::PixelFormat pixelFormat, const libcamera::Size &size,
> + int flags);
> ~Private();
>
> unsigned int numPlanes() const;
> @@ -33,35 +35,92 @@ public:
> Span<uint8_t> plane(unsigned int plane);
>
> size_t jpegBufferSize(size_t maxJpegBufferSize) const;
> +
> +private:
> + /* \todo Remove planes_ when it will be added to MappedBuffer */
> + std::vector<Span<uint8_t>> planes_;
> };
>
> CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
> - buffer_handle_t camera3Buffer, int flags)
> + buffer_handle_t camera3Buffer,
> + libcamera::PixelFormat pixelFormat,
> + const libcamera::Size &size, int flags)
> {
> - maps_.reserve(camera3Buffer->numFds);
> error_ = 0;
>
> + const auto &info = libcamera::PixelFormatInfo::info(pixelFormat);
> + if (!info.isValid()) {
> + error_ = -EINVAL;
> + LOG(HAL, Error) << "Invalid pixel format: "
> + << pixelFormat.toString();
> + return;
> + }
> +
> + /*
> + * As Android doesn't offer an API to query buffer layouts, assume for
> + * now that the buffer is backed by a single dmabuf, with planes being
> + * stored contiguously.
> + */
> + int fd = -1;
> for (int i = 0; i < camera3Buffer->numFds; i++) {
> - if (camera3Buffer->data[i] == -1)
> + if (camera3Buffer->data[i] == -1 || camera3Buffer->data[i] == fd)
> continue;
>
> - off_t length = lseek(camera3Buffer->data[i], 0, SEEK_END);
> - if (length < 0) {
> - error_ = -errno;
> - LOG(HAL, Error) << "Failed to query plane length";
> - break;
> + if (fd != -1) {
> + error_ = -EINVAL;
> + LOG(HAL, Error) << "Discontiguous planes are not supported";
> + return;
> }
>
> - void *address = mmap(nullptr, length, flags, MAP_SHARED,
> - camera3Buffer->data[i], 0);
> - if (address == MAP_FAILED) {
> - error_ = -errno;
> - LOG(HAL, Error) << "Failed to mmap plane";
> - break;
> - }
> + fd = camera3Buffer->data[i];
> + }
>
> - maps_.emplace_back(static_cast<uint8_t *>(address),
> - static_cast<size_t>(length));
> + if (fd == -1) {
> + error_ = -EINVAL;
> + LOG(HAL, Error) << "No valid file descriptor";
> + return;
> + }
> +
> + off_t bufferLength = lseek(fd, 0, SEEK_END);
> + if (bufferLength < 0) {
> + error_ = -errno;
> + LOG(HAL, Error) << "Failed to get buffer length";
> + return;
> + }
> +
> + void *address = mmap(nullptr, bufferLength, flags, MAP_SHARED, fd, 0);
> + if (address == MAP_FAILED) {
> + error_ = -errno;
> + LOG(HAL, Error) << "Failed to mmap plane";
> + return;
> + }
> + maps_.emplace_back(static_cast<uint8_t *>(address), bufferLength);
> +
> + const unsigned int numPlanes = info.numPlanes();
> + planes_.resize(numPlanes);
> + unsigned int offset = 0;
> + for (unsigned int i = 0; i < numPlanes; ++i) {
> + /*
> + * \todo Remove if this plane size computation function is
> + * added to PixelFormatInfo.
> + */
> + const unsigned int vertSubSample = info.planes[i].verticalSubSampling;
> + const unsigned int stride = info.stride(size.width, i, 1u);
> + const unsigned int planeSize =
> + stride * ((size.height + vertSubSample - 1) / vertSubSample);
> +
> + planes_[i] = libcamera::Span<uint8_t>(
> + static_cast<uint8_t *>(address) + offset, planeSize);
> +
> + if (bufferLength < offset + planeSize) {
> + error_ = -EINVAL;
> + LOG(HAL, Error) << "Plane " << i << " is out of buffer"
> + << ", buffer length=" << bufferLength
> + << ", offset=" << offset
> + << ", size=" << planeSize;
> + return;
> + }
> + offset += planeSize;
> }
> }
>
> @@ -71,15 +130,15 @@ CameraBuffer::Private::~Private()
>
> unsigned int CameraBuffer::Private::numPlanes() const
> {
> - return maps_.size();
> + return planes_.size();
> }
>
> Span<uint8_t> CameraBuffer::Private::plane(unsigned int plane)
> {
> - if (plane >= maps_.size())
> + if (plane >= planes_.size())
> return {};
>
> - return maps_[plane];
> + return planes_[plane];
> }
>
> size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const
> --
> 2.33.0.rc2.250.ged5fa647cd-goog
>
More information about the libcamera-devel
mailing list