[libcamera-devel] [PATCH v2 2/3] android: camera_buffer: Map buffer in the first plane() call
Jacopo Mondi
jacopo at jmondi.org
Wed Aug 25 10:46:35 CEST 2021
Hi Hiro,
On Wed, Aug 25, 2021 at 01:44:09PM +0900, Hirokazu Honda wrote:
> CameraBuffer implementation maps a given buffer_handle_t in
> constructor. Mapping is redundant to only know the plane info like
> stride and offset. Mapping should be executed rater in the first
> plane() call.
>
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> ---
> src/android/mm/cros_camera_buffer.cpp | 71 ++++++++++++--------
> src/android/mm/generic_camera_buffer.cpp | 85 ++++++++++++++++--------
> 2 files changed, 101 insertions(+), 55 deletions(-)
>
> diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp
> index 50732637..42546d87 100644
> --- a/src/android/mm/cros_camera_buffer.cpp
> +++ b/src/android/mm/cros_camera_buffer.cpp
> @@ -25,7 +25,7 @@ public:
> int flags);
> ~Private();
>
> - bool isValid() const { return valid_; }
> + bool isValid() const { return registered_; }
>
> unsigned int numPlanes() const;
>
> @@ -34,10 +34,12 @@ public:
> size_t jpegBufferSize(size_t maxJpegBufferSize) const;
>
> private:
> + bool map();
void map();
> +
> cros::CameraBufferManager *bufferManager_;
> buffer_handle_t handle_;
> unsigned int numPlanes_;
> - bool valid_;
> + bool mapped_;
> bool registered_;
> union {
> void *addr;
> @@ -50,7 +52,7 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
> [[maybe_unused]] libcamera::PixelFormat pixelFormat,
> [[maybe_unused]] const libcamera::Size &size,
> [[maybe_unused]] int flags)
> - : handle_(camera3Buffer), numPlanes_(0), valid_(false),
> + : handle_(camera3Buffer), numPlanes_(0), mapped_(false),
> registered_(false)
> {
> bufferManager_ = cros::CameraBufferManager::GetInstance();
> @@ -63,36 +65,11 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
>
> registered_ = true;
> numPlanes_ = bufferManager_->GetNumPlanes(camera3Buffer);
> - switch (numPlanes_) {
> - case 1: {
> - ret = bufferManager_->Lock(handle_, 0, 0, 0, 0, 0, &mem.addr);
> - if (ret) {
> - LOG(HAL, Error) << "Single plane buffer mapping failed";
> - return;
> - }
> - break;
> - }
> - case 2:
> - case 3: {
> - ret = bufferManager_->LockYCbCr(handle_, 0, 0, 0, 0, 0,
> - &mem.ycbcr);
> - if (ret) {
> - LOG(HAL, Error) << "YCbCr buffer mapping failed";
> - return;
> - }
> - break;
> - }
> - default:
> - LOG(HAL, Error) << "Invalid number of planes: " << numPlanes_;
> - return;
> - }
> -
> - valid_ = true;
> }
>
> CameraBuffer::Private::~Private()
> {
> - if (valid_)
> + if (mapped_)
> bufferManager_->Unlock(handle_);
> if (registered_)
> bufferManager_->Deregister(handle_);
> @@ -105,6 +82,12 @@ unsigned int CameraBuffer::Private::numPlanes() const
>
> Span<uint8_t> CameraBuffer::Private::plane(unsigned int plane)
> {
> + if (!mapped_) {
> + mapped_ = map();
> + if (!mapped_)
> + return {};
> + }
if (!mapped_)
map();
if (!mapped_);
return {};
> +
> void *addr;
>
> switch (numPlanes()) {
> @@ -134,4 +117,34 @@ size_t CameraBuffer::Private::jpegBufferSize([[maybe_unused]] size_t maxJpegBuff
> return bufferManager_->GetPlaneSize(handle_, 0);
> }
>
> +bool CameraBuffer::Private::map()
void
> +{
> + int ret;
> + switch (numPlanes_) {
> + case 1: {
> + ret = bufferManager_->Lock(handle_, 0, 0, 0, 0, 0, &mem.addr);
> + if (ret) {
> + LOG(HAL, Error) << "Single plane buffer mapping failed";
> + return false;
return;
> + }
> + break;
> + }
> + case 2:
> + case 3: {
> + ret = bufferManager_->LockYCbCr(handle_, 0, 0, 0, 0, 0,
> + &mem.ycbcr);
> + if (ret) {
> + LOG(HAL, Error) << "YCbCr buffer mapping failed";
> + return false;
Ditto
> + }
> + break;
> + }
> + default:
> + LOG(HAL, Error) << "Invalid number of planes: " << numPlanes_;
> + return false;
Ditto
> + }
> +
> + return true;
mapped_ = true;
> +}
> +
> PUBLIC_CAMERA_BUFFER_IMPLEMENTATION
> diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp
> index 0bfdc2ba..37868d26 100644
> --- a/src/android/mm/generic_camera_buffer.cpp
> +++ b/src/android/mm/generic_camera_buffer.cpp
> @@ -37,6 +37,17 @@ public:
> size_t jpegBufferSize(size_t maxJpegBufferSize) const;
>
> private:
> + struct PlaneInfo {
> + unsigned int offset;
> + unsigned int size;
> + };
> +
> + bool map();
> +
> + int fd_;
> + int flags_;
> + off_t bufferLength_;
> + std::vector<PlaneInfo> planeInfo_;
Can't you use a mapped_ like the cros implementation ?
> /* \todo Remove planes_ when it will be added to MappedBuffer */
> std::vector<Span<uint8_t>> planes_;
> };
> @@ -45,6 +56,7 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
> buffer_handle_t camera3Buffer,
> libcamera::PixelFormat pixelFormat,
> const libcamera::Size &size, int flags)
> + : fd_(-1), flags_(flags)
> {
> error_ = 0;
> /*
> @@ -52,19 +64,18 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
> * 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 ||
> - camera3Buffer->data[i] == fd) {
> + camera3Buffer->data[i] == fd_) {
> continue;
> }
>
> - if (fd != -1)
> + if (fd_ != -1)
> LOG(HAL, Fatal) << "Discontiguous planes are not supported";
> - fd = camera3Buffer->data[i];
> + fd_ = camera3Buffer->data[i];
> }
>
> - if (fd != -1) {
> + if (fd_ != -1) {
> error_ = -EINVAL;
> LOG(HAL, Error) << "No valid file descriptor";
> return;
> @@ -78,23 +89,16 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
> return;
> }
>
> - off_t bufferLength = lseek(fd, 0, SEEK_END);
> + size_t bufferLength = lseek(fd_, 0, SEEK_END);
> if (bufferLength < 0) {
> - error_ = -errno;
> + error_ = -EINVAL;
Why this change ?
> 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);
> + planeInfo_.resize(numPlanes);
> +
> unsigned int offset = 0;
> for (unsigned int i = 0; i < numPlanes; ++i) {
> /*
> @@ -106,17 +110,17 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
> const unsigned int planeSize =
> stride * ((size.height + vertSubSample - 1) / vertSubSample);
>
> - planes_[i] = libcamera::Span<uint8_t>(
> - static_cast<uint8_t *>(address) + offset, planeSize);
> + planeInfo_[i].offset = offset;
> + planeInfo_[i].size = planeSize;
>
> if (bufferLength < offset + planeSize) {
> - error_ = -EINVAL;
> - LOG(HAL, Error) << "Plane " << i << " is out of buffer"
> - << ", buffer length=" << bufferLength
> - << ", offset=" << offset
> - << ", size=" << planeSize;
> + LOG(HAL, Error) << "Plane " << i << " is out of buffer:"
> + << " plane offset=" << offset
> + << ", plane size=" << planeSize
> + << ", buffer length=" << bufferLength;
> return;
> }
> +
> offset += planeSize;
> }
> }
> @@ -127,11 +131,14 @@ CameraBuffer::Private::~Private()
>
> unsigned int CameraBuffer::Private::numPlanes() const
> {
> - return planes_.size();
> + return planeInfo_.size();
> }
>
> Span<uint8_t> CameraBuffer::Private::plane(unsigned int plane)
> {
> + if (planes_.empty() && !map())
> + return {};
> +
And repeat the pattern used in cros implementation using a mapped_
class variable to keep the two implementation similar ?
> if (plane >= planes_.size())
> return {};
>
> @@ -140,8 +147,34 @@ Span<uint8_t> CameraBuffer::Private::plane(unsigned int plane)
>
> size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const
> {
> - return std::min<unsigned int>(maps_[0].size(),
> - maxJpegBufferSize);
> + if (maps_.empty()) {
> + ASSERT(bufferLength_ >= 0);
> + return std::min<unsigned int>(bufferLength_, maxJpegBufferSize);
> + }
> +
> + return std::min<unsigned int>(maps_[0].size(), maxJpegBufferSize);
> +}
> +
> +bool CameraBuffer::Private::map()
> +{
> + ASSERT(fd_ != -1);
Shouldn't we fail earlier then ?
> + ASSERT(bufferLength_ >= 0);
> +
> + void *address = mmap(nullptr, bufferLength_, flags_, MAP_SHARED, fd_, 0);
> + if (address == MAP_FAILED) {
> + error_ = -errno;
> + LOG(HAL, Error) << "Failed to mmap plane";
> + return false;
> + }
> + maps_.emplace_back(static_cast<uint8_t *>(address), bufferLength_);
> +
> + planes_.reserve(planeInfo_.size());
> + for (const auto &info : planeInfo_) {
> + planes_.emplace_back(
> + static_cast<uint8_t *>(address) + info.offset, info.size);
> + }
Can't this be done on costruction as it just basically transfer the
information stored in planeInfo into planes_ rendering planeInfo_ just
a temporary storage ?
> +
> + return true;
> }
>
> PUBLIC_CAMERA_BUFFER_IMPLEMENTATION
> --
> 2.33.0.rc2.250.ged5fa647cd-goog
>
More information about the libcamera-devel
mailing list