| Message ID | 20210825044410.2787433-3-hiroh@chromium.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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@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 >
Hi Jacopo, On Wed, Aug 25, 2021 at 5:45 PM Jacopo Mondi <jacopo@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@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 > >
Hi Hiro, Thank you for the patch. On Wed, Aug 25, 2021 at 08:02:45PM +0900, Hirokazu Honda wrote: > On Wed, Aug 25, 2021 at 5:45 PM Jacopo Mondi wrote: > > 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@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); s/size_t/off_t/ or you'll have a warning with the next line. > > > 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); Given that maps_[0].size() == bufferLength_, I think this whole function can be simplified to return std::min<unsigned int>(bufferLength_, 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. Another option would be to store the mmap()ed address in a single void *address_ class member, drop maps_, and change the plane() function to Span<uint8_t> CameraBuffer::Private::plane(unsigned int plane) { if (plane >= planeInfo_.size()) return {}; const PlaneInfo &info = planeInfo_[plane]; return { static_cast<uint8_t *>(address_) + info.offset, info.size }; } Yet another option would be to store { info.offset, info.size } in planes_ in the constructor, and then add address to the offset in this function. > > > + > > > + return true; > > > } > > > > > > PUBLIC_CAMERA_BUFFER_IMPLEMENTATION
On Wed, Aug 25, 2021 at 11:33:24PM +0300, Laurent Pinchart wrote: > Hi Hiro, > > Thank you for the patch. > > On Wed, Aug 25, 2021 at 08:02:45PM +0900, Hirokazu Honda wrote: > > On Wed, Aug 25, 2021 at 5:45 PM Jacopo Mondi wrote: > > > 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@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); > > s/size_t/off_t/ or you'll have a warning with the next line. > > > > > 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); > > Given that maps_[0].size() == bufferLength_, I think this whole function > can be simplified to > > return std::min<unsigned int>(bufferLength_, 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. > > Another option would be to store the mmap()ed address in a single void > *address_ class member, drop maps_, and change the plane() function to > > Span<uint8_t> CameraBuffer::Private::plane(unsigned int plane) > { > if (plane >= planeInfo_.size()) > return {}; > > const PlaneInfo &info = planeInfo_[plane]; > return { static_cast<uint8_t *>(address_) + info.offset, info.size }; > } > > Yet another option would be to store { info.offset, info.size } in planes_ > in the constructor, and then add address to the offset in this function. Now that I've read patch 3/3, I think there will be a bit of additional refactoring of the MappedBuffer class that I'd like to do on top of this, so I'm fine with any of the above options. > > > > + > > > > + return true; > > > > } > > > > > > > > PUBLIC_CAMERA_BUFFER_IMPLEMENTATION
Hi Laurent, On Thu, Aug 26, 2021 at 5:39 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > On Wed, Aug 25, 2021 at 11:33:24PM +0300, Laurent Pinchart wrote: > > Hi Hiro, > > > > Thank you for the patch. > > > > On Wed, Aug 25, 2021 at 08:02:45PM +0900, Hirokazu Honda wrote: > > > On Wed, Aug 25, 2021 at 5:45 PM Jacopo Mondi wrote: > > > > 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@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); > > > > s/size_t/off_t/ or you'll have a warning with the next line. > > > > > > > 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); > > > > Given that maps_[0].size() == bufferLength_, I think this whole function > > can be simplified to > > > > return std::min<unsigned int>(bufferLength_, 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. > > > > Another option would be to store the mmap()ed address in a single void > > *address_ class member, drop maps_, and change the plane() function to > > > > Span<uint8_t> CameraBuffer::Private::plane(unsigned int plane) > > { > > if (plane >= planeInfo_.size()) > > return {}; > > > > const PlaneInfo &info = planeInfo_[plane]; > > return { static_cast<uint8_t *>(address_) + info.offset, info.size }; > > } > > > > Yet another option would be to store { info.offset, info.size } in planes_ > > in the constructor, and then add address to the offset in this function. > > Now that I've read patch 3/3, I think there will be a bit of additional > refactoring of the MappedBuffer class that I'd like to do on top of > this, so I'm fine with any of the above options. > CameraBuffer::Private in generic_camera_buffer.cpp inherits MappedBuffer. It will have planes for address in my another patch series. I think it is better to temporarily have planes_ here and store it addresses. -Hiro > > > > > + > > > > > + return true; > > > > > } > > > > > > > > > > PUBLIC_CAMERA_BUFFER_IMPLEMENTATION > > -- > Regards, > > Laurent Pinchart
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(); + 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 {}; + } + 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() +{ + 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; + } + 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; + } + break; + } + default: + LOG(HAL, Error) << "Invalid number of planes: " << numPlanes_; + return false; + } + + return 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_; /* \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; 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 {}; + 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); + 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); + } + + return true; } PUBLIC_CAMERA_BUFFER_IMPLEMENTATION
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@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(-)