[libcamera-devel] [RFC PATCH 1/3] android: generic_camera_buffer: Correct buffer mapping
Tomasz Figa
tfiga at chromium.org
Fri Aug 27 11:33:25 CEST 2021
On Tue, Aug 24, 2021 at 6:23 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Hiro,
>
> Thank you for the patch.
>
> On Mon, Aug 23, 2021 at 09:43:19PM +0900, Hirokazu Honda wrote:
> > buffer_hanlde_t doesn't provide sufficient info to map a buffer
>
> s/buffer_hanlde_t/buffer_handle_t/
>
> > 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>
> > ---
> > 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 | 82 +++++++++++++++++-------
> > 4 files changed, 78 insertions(+), 29 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);
>
> I'm not thrilled by the idea of having to pass a format and size, but as
> you correctly explained, buffer_handle_t isn't enough, so I don't see
> any other option :-(
>
> > ~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..3127069f 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,69 @@ public:
> > Span<uint8_t> plane(unsigned int plane);
> >
> > size_t jpegBufferSize(size_t maxJpegBufferSize) const;
> > +
> > +private:
> > + /* \todo remove planes_ is added to MappedBuffer. */
>
> I'm not sure to follow you. Do you mean "Remove planes_ when it will be
> added to MappedBuffer" maybe ?
>
> > + 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;
> >
>
> I'd add a comment here:
>
> /*
> * 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)
> > - continue;
> > -
> > - off_t length = lseek(camera3Buffer->data[i], 0, SEEK_END);
> > - if (length < 0) {
> > - error_ = -errno;
> > - LOG(HAL, Error) << "Failed to query plane length";
> > + if (camera3Buffer->data[i] != -1) {
> > + fd = camera3Buffer->data[i];
> > break;
> > }
> > + }
>
> Could we verify the assumption ? Maybe something like
>
> int fd = -1;
> for (int i = 0; i < camera3Buffer->numFds; i++) {
> if (camera3Buffer->data[i] == -1 ||
> camera3Buffer->data[i] == fd)
> continue;
>
> if (fd != -1)
> LOG(HAL, Fatal)
> << "Discontiguous planes are not supported";
> fd = camera3Buffer->data[i];
> }
>
If numFds > 1, generally the FDs will be different, because the handle
is sent over Binder, which would create new FDs for each array element
when unflattening the handle on the receiver side.
> >
> > - 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;
> > - }
> > + if (fd != -1) {
>
> That should be
>
> if (fd == -1) {
>
> > + error_ = EINVAL;
>
> -EINVAL
>
> > + LOG(HAL, Error) << "No valid file descriptor";
> > + return;
> > + }
> > +
> > + const auto &info = libcamera::PixelFormatInfo::info(pixelFormat);
> > + if (!info.isValid()) {
> > + error_ = EINVAL;
>
> -EINVAL
>
> > + LOG(HAL, Error) << "Invalid pixel format: "
> > + << pixelFormat.toString();
> > + return;
> > + }
> > +
> > + size_t bufferLength = lseek(fd, 0, SEEK_END);
> > + if (bufferLength < 0) {
>
> This comparison is always false, as size_t is unsigned. The correct data
> type if off_t for the return value of lseek.
Is perhaps the auto type the right thing to use to avoid issues like this?
Best regards,
Tomasz
>
> > + 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) {
> > + 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);
>
> It may make sense to move this calculation to a new helper function in
> libcamera::PixelFormatInfo at some point, but it's fine for now.
>
> > +
> > + planes_[i] = libcamera::Span<uint8_t>(
> > + static_cast<uint8_t *>(address) + offset, planeSize);
> >
> > - maps_.emplace_back(static_cast<uint8_t *>(address),
> > - static_cast<size_t>(length));
> > + offset += planeSize;
> > }
> > }
> >
> > @@ -71,15 +107,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
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list