[libcamera-devel] [PATCH v2 1/3] android: generic_camera_buffer: Correct buffer mapping
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Aug 30 18:18:19 CEST 2021
Hi Tomasz,
On Fri, Aug 27, 2021 at 09:09:37PM +0900, Tomasz Figa wrote:
> On Fri, Aug 27, 2021 at 9:07 PM Laurent Pinchart wrote:
> > On Fri, Aug 27, 2021 at 09:01:07PM +0900, Tomasz Figa wrote:
> > > On Fri, Aug 27, 2021 at 8:36 PM Laurent Pinchart wrote:
> > > > On Fri, Aug 27, 2021 at 07:08:30PM +0900, Tomasz Figa wrote:
> > > > > On Wed, Aug 25, 2021 at 5:34 PM Jacopo Mondi wrote:
> > > > > > On Wed, Aug 25, 2021 at 01:44:08PM +0900, Hirokazu Honda wrote:
> > > > > > > buffer_hanlde_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>
> > > > > > > ---
> > > > > > > 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 | 104 +++++++++++++++++------
> > > > > > > 4 files changed, 99 insertions(+), 30 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..0bfdc2ba 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,89 @@ 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;
> > > > > > > -
> > > > > > > + /*
> > > > > > > + * 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++) {
> > > > > >
> > > > > > unsigned int 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;
> > > > > > > }
> > > > > > >
> > > > > > > - 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)
> > > > > > > + LOG(HAL, Fatal) << "Discontiguous planes are not supported";
> > > > > >
> > > > > > Do we just warn ? Or exit with error ? See the below question
> > > > > >
> > > > > > > + fd = camera3Buffer->data[i];
> > > > > >
> > > > > > This assigns fd to the 'next' valid data[i].fd. I might have missed
> > > > > > what is the policy here if we have more than one valid dmabuf fds.
> > > > > >
> > > > > > Currently you warn and use the last valid one. I would instead error out,
> > > > > > or warn but use the first available one (even if that would result in
> > > > > > undefined behaviour, or most probably an access to non valid memory when
> > > > > > creating planes below).
> > > > > >
> > > > > > Do you have a use case for formats with planes allocated in different
> > > > > > buffers that you need to support ? Or can we error out without causing
> > > > > > regressions ?
> > > > > >
> > > > > > > + }
> > > > > >
> > > > > > The loop is pretty convoluted. If I'm not mistaken what you want to
> > > > > > verify is that
> > > > > > 1) Either all data[i].fd are the same
> > > > >
> > > > > This is actually not something that would happen in practice, because
> > > > > the handles are passed via Binder and even if the source puts the same
> > > > > FD for all the planes, the receiver would see all different FDs,
> > > > > because Binder would create a new FD for each array element.
> > > >
> > > > Sounds like there's room for improvement in binder then. Nevertheless,
> > > > we certainly want to support the case where the fds will be different
> > > > but all refer to the same dmabuf. The main blocker for this is the
> > > > ability to actually test it on Android, which I'm working on.
> > >
> > > Do we really need that strict validation? I think we could just take
> > > the first DMA-buf if the offsets match the non-M format layout. There
> > > is no practical case when libcamera would get more than 1 DMA-buf to
> > > be used with hardware that only supports non-M formats.
> >
> > It's an easy check to implement, so I'd rather do it now that my memory
> > is fresh than running into a problem in a year and spend days debugging
> > it :-)
>
> Is the check cheap and compatible with older kernel versions?
As far as I can tell, yes, but I'll double-check of course.
> Generally I don't think it would ever be possible to run into issues
> with this in real life, but agreed that if the check is simple it's
> better to have it.
>
> > > > > > 2) All but the data[0].fd are -1
> > > > > >
> > > > > > Wouldn't it be nicer as
> > > > > >
> > > > > > int fd = camera3Buffer->data[0].fd;
> > > > > > for (unsigned int i = 1; i < camera3Buffer->numFds; ++i) {
> > > > > > if (camera3Buffer->data[i].fd != -1)
> > > > > > continue;
> > > > > >
> > > > > > if (camera3Buffer->data[i].fd != fd)
> > > > > > /* Here I would error out */
> > > > > > }
> > > > > > if (fd == -1) {
> > > > > > /* No valid fds, error out */
> > > > > > }
> > > > > >
> > > > > > > +
> > > > > > > + if (fd != -1) {
> > > > > >
> > > > > > Shouldn't this be fd == -1 ? As in "no valid fd found ?"
> > > > > > If I'm not mistaken checking for fd != -1 should fail all the times as
> > > > > > at least one valid fd should be found. On which platform have you
> > > > > > tested this patch ?
> > > > > >
> > > > > > > + error_ = -EINVAL;
> > > > > > > + LOG(HAL, Error) << "No valid file descriptor";
> > > > > > > + return;
> > > > > > > + }
> > > > > > >
> > > > > > > - maps_.emplace_back(static_cast<uint8_t *>(address),
> > > > > > > - static_cast<size_t>(length));
> > > > > > > + const auto &info = libcamera::PixelFormatInfo::info(pixelFormat);
> > > > > > > + if (!info.isValid()) {
> > > > > > > + error_ = -EINVAL;
> > > > > > > + LOG(HAL, Error) << "Invalid pixel format: "
> > > > > > > + << pixelFormat.toString();
> > > > > > > + return;
> > > > > > > + }
> > > > > >
> > > > > > We could check this at the very beginning of the function and avoid
> > > > > > looping on fds if the format is invalid.
> > > > > >
> > > > > > > +
> > > > > > > + 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 +127,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