[libcamera-devel] [PATCH v3 2/3] android: camera_buffer: Map buffer in the first plane() call
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Aug 30 17:00:55 CEST 2021
Hi Hiro,
On Fri, Aug 27, 2021 at 04:00:10PM +0900, Hirokazu Honda wrote:
> On Fri, Aug 27, 2021 at 3:57 PM Hirokazu Honda wrote:
> > On Thu, Aug 26, 2021 at 9:06 PM Laurent Pinchart wrote:
> > > On Thu, Aug 26, 2021 at 05:00:20PM +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 | 86 ++++++++++++++++--------
> > > > 2 files changed, 100 insertions(+), 57 deletions(-)
> > > >
> > > > diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp
> > > > index 50732637..ba6650cf 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:
> > > > + 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,11 @@ unsigned int CameraBuffer::Private::numPlanes() const
> > > >
> > > > Span<uint8_t> CameraBuffer::Private::plane(unsigned int plane)
> > > > {
> > > > + if (!mapped_)
> > > > + map();
> > > > + if (!mapped_)
> > > > + return {};
> > > > +
> > > > void *addr;
> > > >
> > > > switch (numPlanes()) {
> > > > @@ -134,4 +116,35 @@ size_t CameraBuffer::Private::jpegBufferSize([[maybe_unused]] size_t maxJpegBuff
> > > > return bufferManager_->GetPlaneSize(handle_, 0);
> > > > }
> > > >
> > > > +void CameraBuffer::Private::map()
> > > > +{
> > > > + 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;
> > > > + }
> > > > + 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;
> > > > + }
> > > > +
> > > > + mapped_ = true;
> > > > + return;
> > > > +}
> > > > +
> > > > PUBLIC_CAMERA_BUFFER_IMPLEMENTATION
> > > > diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp
> > > > index 22753490..cfe55b69 100644
> > > > --- a/src/android/mm/generic_camera_buffer.cpp
> > > > +++ b/src/android/mm/generic_camera_buffer.cpp
> > > > @@ -37,6 +37,18 @@ public:
> > > > size_t jpegBufferSize(size_t maxJpegBufferSize) const;
> > > >
> > > > private:
> > > > + struct PlaneInfo {
> > > > + unsigned int offset;
> > > > + unsigned int size;
> > > > + };
> > > > +
> > > > + void map();
> > > > +
> > > > + int fd_;
> > > > + int flags_;
> > > > + off_t bufferLength_;
> > > > + bool mapped_;
> > > > + std::vector<PlaneInfo> planeInfo_;
> > > > /* \todo Remove planes_ when it will be added to MappedBuffer */
> > > > std::vector<Span<uint8_t>> planes_;
> > > > };
> > > > @@ -45,6 +57,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), mapped_(false)
> > >
> > > This should initialize bufferLength_ to 0, otherwise the ASSERT()s that
> > > checks the variable will be pointless if the constructor returns an
> > > error before setting bufferLength_.
> >
> > Shall I set bufferLength to -1 and then ASSERT(bufferLength_ >= 0), or
> > set to 0 and ASSERT(bufferLength > 0)?
>
> Uploaded with the former. If you think the latter is correct, please
> fix it upon applying.
> Thanks in advance.
I've mistakenly pushed this patch already. I'll turn yours into a fix
and will send it to the list. Sorry about the mistake.
> > > And it will also avoid a Coverity warning :-)
> > >
> > > I can handle this when pushing the series.
> > >
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > >
> > > > {
> > > > error_ = 0;
> > > >
> > > > @@ -61,43 +74,35 @@ 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)
> > > > + if (camera3Buffer->data[i] == -1 || camera3Buffer->data[i] == fd_)
> > > > continue;
> > > >
> > > > - if (fd != -1) {
> > > > + if (fd_ != -1) {
> > > > error_ = -EINVAL;
> > > > LOG(HAL, Error) << "Discontiguous planes are not supported";
> > > > return;
> > > > }
> > > >
> > > > - fd = camera3Buffer->data[i];
> > > > + fd_ = camera3Buffer->data[i];
> > > > }
> > > >
> > > > - if (fd == -1) {
> > > > + if (fd_ == -1) {
> > > > error_ = -EINVAL;
> > > > LOG(HAL, Error) << "No valid file descriptor";
> > > > return;
> > > > }
> > > >
> > > > - off_t bufferLength = lseek(fd, 0, SEEK_END);
> > > > - if (bufferLength < 0) {
> > > > + 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);
> > > > + planeInfo_.resize(numPlanes);
> > > > +
> > > > unsigned int offset = 0;
> > > > for (unsigned int i = 0; i < numPlanes; ++i) {
> > > > /*
> > > > @@ -109,17 +114,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;
> > > > + if (bufferLength_ < offset + planeSize) {
> > > > + LOG(HAL, Error) << "Plane " << i << " is out of buffer:"
> > > > + << " plane offset=" << offset
> > > > + << ", plane size=" << planeSize
> > > > + << ", buffer length=" << bufferLength_;
> > > > return;
> > > > }
> > > > +
> > > > offset += planeSize;
> > > > }
> > > > }
> > > > @@ -130,12 +135,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 (plane >= planes_.size())
> > > > + if (!mapped_)
> > > > + map();
> > > > + if (!mapped_)
> > > > return {};
> > > >
> > > > return planes_[plane];
> > > > @@ -143,8 +150,31 @@ 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);
> > > > + ASSERT(bufferLength_ >= 0);
> > > > +
> > > > + return std::min<unsigned int>(bufferLength_, maxJpegBufferSize);
> > > > +}
> > > > +
> > > > +void CameraBuffer::Private::map()
> > > > +{
> > > > + ASSERT(fd_ != -1);
> > > > + 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;
> > > > + }
> > > > + 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);
> > > > + }
> > > > +
> > > > + mapped_ = true;
> > > > }
> > > >
> > > > PUBLIC_CAMERA_BUFFER_IMPLEMENTATION
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list