[libcamera-devel] [PATCH v2 2/3] android: camera_buffer: Map buffer in the first plane() call

Hirokazu Honda hiroh at chromium.org
Wed Aug 25 13:02:45 CEST 2021


Hi Jacopo,

On Wed, Aug 25, 2021 at 5:45 PM Jacopo Mondi <jacopo at jmondi.org> wrote:
>
> 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 ?
>

address is needed to fill planes_ correctly.
planeInfo_ is used in stride/offset/size() in the next patch. Since it
is useful even in this patch, I introduced it from this patch.

-Hiro
> > +
> > +     return true;
> >  }
> >
> >  PUBLIC_CAMERA_BUFFER_IMPLEMENTATION
> > --
> > 2.33.0.rc2.250.ged5fa647cd-goog
> >


More information about the libcamera-devel mailing list