| Message ID | 20210823124321.980847-3-hiroh@chromium.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Hiro, Thank you for the patch. On Mon, Aug 23, 2021 at 09:43: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@chromium.org> > --- > src/android/mm/cros_camera_buffer.cpp | 71 ++++++++++-------- > src/android/mm/generic_camera_buffer.cpp | 93 +++++++++++++++--------- > 2 files changed, 100 insertions(+), 64 deletions(-) > > diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp > index 50732637..85ef6480 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(); s/Map/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 3127069f..6c1e4611 100644 > --- a/src/android/mm/generic_camera_buffer.cpp > +++ b/src/android/mm/generic_camera_buffer.cpp > @@ -37,6 +37,12 @@ public: > size_t jpegBufferSize(size_t maxJpegBufferSize) const; > > private: > + bool Map(); s/Map/map/ > + > + int fd_; > + int flags_; > + libcamera::Size size_; > + libcamera::PixelFormatInfo info_; Maybe a const pointer, to avoid a copy ? Or a const reference, initialized in the constructor's initializer list. > /* \todo remove planes_ is added to MappedBuffer. */ > std::vector<Span<uint8_t>> planes_; > }; > @@ -45,83 +51,100 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer, > buffer_handle_t camera3Buffer, > libcamera::PixelFormat pixelFormat, > const libcamera::Size &size, int flags) > + : fd_(-1), flags_(flags), size_(size) > { > error_ = 0; > > - int fd = -1; > for (int i = 0; i < camera3Buffer->numFds; i++) { > if (camera3Buffer->data[i] != -1) { > - fd = camera3Buffer->data[i]; > + fd_ = camera3Buffer->data[i]; > break; > } > } > > - if (fd != -1) { > + if (fd_ != -1) { > error_ = EINVAL; > LOG(HAL, Error) << "No valid file descriptor"; > return; > } > > - const auto &info = libcamera::PixelFormatInfo::info(pixelFormat); > - if (!info.isValid()) { > + info_ = libcamera::PixelFormatInfo::info(pixelFormat); > + if (!info_.isValid()) { > error_ = EINVAL; > LOG(HAL, Error) << "Invalid pixel format: " > << pixelFormat.toString(); > return; > } > +} > + > +CameraBuffer::Private::~Private() > +{ > +} > + > +unsigned int CameraBuffer::Private::numPlanes() const > +{ > + return info_.numPlanes(); > +} > + > +Span<uint8_t> CameraBuffer::Private::plane(unsigned int plane) > +{ > + if (planes_.empty() && !Map()) > + return {}; > > - size_t bufferLength = lseek(fd, 0, SEEK_END); > + if (plane >= planes_.size()) > + return {}; > + > + return planes_[plane]; > +} > + > +size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const > +{ > + if (maps_.empty()) { > + size_t bufferLength = lseek(fd_, 0, SEEK_END); > + if (bufferLength < 0) > + LOG(HAL, Error) << "Failed to get buffer length"; > + return 0; > + } > + > + return std::min<unsigned int>(maps_[0].size(), > + maxJpegBufferSize); > +} > + > +bool CameraBuffer::Private::Map() > +{ > + ASSERT(fd_ != -1); > + > + size_t bufferLength = lseek(fd_, 0, SEEK_END); > if (bufferLength < 0) { > error_ = -errno; > LOG(HAL, Error) << "Failed to get buffer length"; > - return; > + return false; > } I would keep this in the constructor, to avoid the lseek() call in CameraBuffer::Private::jpegBufferSize(), as lseek() isn't a very expensive operation. It would also be useful to check, in the constructor, that the buffer size as reported by lseek() is equal to or larger than the total size computed from the format, as a sanity check. I think that should go in patch 1/3. > > - void *address = mmap(nullptr, bufferLength, flags, MAP_SHARED, fd, 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; > + return false; > } > maps_.emplace_back(static_cast<uint8_t *>(address), bufferLength); > > - const unsigned int numPlanes = info.numPlanes(); > + const unsigned int numPlanes = info_.numPlanes(); > planes_.resize(numPlanes); > unsigned int offset = 0; > for (unsigned int i = 0; i < numPlanes; ++i) { > - const unsigned int vertSubSample = info.planes[i].verticalSubSampling; > - const unsigned int stride = info.stride(size.width, i, 1u); > + const unsigned int vertSubSample = info_.planes[i].verticalSubSampling; > + const unsigned int stride = info_.stride(size_.width, i, 1u); > const unsigned int planeSize = > - stride * ((size.height + vertSubSample - 1) / vertSubSample); > + stride * ((size_.height + vertSubSample - 1) / vertSubSample); > > planes_[i] = libcamera::Span<uint8_t>( > static_cast<uint8_t *>(address) + offset, planeSize); > > offset += planeSize; > } This should also be kept in the constructor, as otherwise calling CameraBuffer::size() will be valid on an unmapped before on CrOS, but not on Android. > -} > - > -CameraBuffer::Private::~Private() > -{ > -} > - > -unsigned int CameraBuffer::Private::numPlanes() const > -{ > - return planes_.size(); > -} > > -Span<uint8_t> CameraBuffer::Private::plane(unsigned int plane) > -{ > - if (plane >= planes_.size()) > - return {}; > - > - return planes_[plane]; > -} > - > -size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const > -{ > - return std::min<unsigned int>(maps_[0].size(), > - maxJpegBufferSize); > + return true; > } > > PUBLIC_CAMERA_BUFFER_IMPLEMENTATION
diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp index 50732637..85ef6480 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 3127069f..6c1e4611 100644 --- a/src/android/mm/generic_camera_buffer.cpp +++ b/src/android/mm/generic_camera_buffer.cpp @@ -37,6 +37,12 @@ public: size_t jpegBufferSize(size_t maxJpegBufferSize) const; private: + bool Map(); + + int fd_; + int flags_; + libcamera::Size size_; + libcamera::PixelFormatInfo info_; /* \todo remove planes_ is added to MappedBuffer. */ std::vector<Span<uint8_t>> planes_; }; @@ -45,83 +51,100 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer, buffer_handle_t camera3Buffer, libcamera::PixelFormat pixelFormat, const libcamera::Size &size, int flags) + : fd_(-1), flags_(flags), size_(size) { error_ = 0; - int fd = -1; for (int i = 0; i < camera3Buffer->numFds; i++) { if (camera3Buffer->data[i] != -1) { - fd = camera3Buffer->data[i]; + fd_ = camera3Buffer->data[i]; break; } } - if (fd != -1) { + if (fd_ != -1) { error_ = EINVAL; LOG(HAL, Error) << "No valid file descriptor"; return; } - const auto &info = libcamera::PixelFormatInfo::info(pixelFormat); - if (!info.isValid()) { + info_ = libcamera::PixelFormatInfo::info(pixelFormat); + if (!info_.isValid()) { error_ = EINVAL; LOG(HAL, Error) << "Invalid pixel format: " << pixelFormat.toString(); return; } +} + +CameraBuffer::Private::~Private() +{ +} + +unsigned int CameraBuffer::Private::numPlanes() const +{ + return info_.numPlanes(); +} + +Span<uint8_t> CameraBuffer::Private::plane(unsigned int plane) +{ + if (planes_.empty() && !Map()) + return {}; - size_t bufferLength = lseek(fd, 0, SEEK_END); + if (plane >= planes_.size()) + return {}; + + return planes_[plane]; +} + +size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const +{ + if (maps_.empty()) { + size_t bufferLength = lseek(fd_, 0, SEEK_END); + if (bufferLength < 0) + LOG(HAL, Error) << "Failed to get buffer length"; + return 0; + } + + return std::min<unsigned int>(maps_[0].size(), + maxJpegBufferSize); +} + +bool CameraBuffer::Private::Map() +{ + ASSERT(fd_ != -1); + + size_t bufferLength = lseek(fd_, 0, SEEK_END); if (bufferLength < 0) { error_ = -errno; LOG(HAL, Error) << "Failed to get buffer length"; - return; + return false; } - void *address = mmap(nullptr, bufferLength, flags, MAP_SHARED, fd, 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; + return false; } maps_.emplace_back(static_cast<uint8_t *>(address), bufferLength); - const unsigned int numPlanes = info.numPlanes(); + const unsigned int numPlanes = info_.numPlanes(); planes_.resize(numPlanes); unsigned int offset = 0; for (unsigned int i = 0; i < numPlanes; ++i) { - const unsigned int vertSubSample = info.planes[i].verticalSubSampling; - const unsigned int stride = info.stride(size.width, i, 1u); + const unsigned int vertSubSample = info_.planes[i].verticalSubSampling; + const unsigned int stride = info_.stride(size_.width, i, 1u); const unsigned int planeSize = - stride * ((size.height + vertSubSample - 1) / vertSubSample); + stride * ((size_.height + vertSubSample - 1) / vertSubSample); planes_[i] = libcamera::Span<uint8_t>( static_cast<uint8_t *>(address) + offset, planeSize); offset += planeSize; } -} - -CameraBuffer::Private::~Private() -{ -} - -unsigned int CameraBuffer::Private::numPlanes() const -{ - return planes_.size(); -} -Span<uint8_t> CameraBuffer::Private::plane(unsigned int plane) -{ - if (plane >= planes_.size()) - return {}; - - return planes_[plane]; -} - -size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const -{ - return std::min<unsigned int>(maps_[0].size(), - maxJpegBufferSize); + 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 | 93 +++++++++++++++--------- 2 files changed, 100 insertions(+), 64 deletions(-)