[libcamera-devel] [PATCH 1/2] android: CameraBuffer: Add option of not mapping a buffer

[Jacopo Mondi]

Hi Hiro,

On Mon, Apr 05, 2021 at 01:04:23PM +0900, Hirokazu Honda wrote:
> CameraBuffer always maps a buffer in constructor. This adds an
> option to not map a buffer within CameraBuffer. The option is
> useful for a caller to only validate a buffer and not have to
> map the buffer.
>

Although this requires CameraDevice to create a CameraBuffer in
processCaptureRequest() only to validate. Would a static method work
better ? After all if the validation shall happen before mapping, it
means it is based on static attribute of buffer_handle_t

Also, I don't really like the 'map' flag, as it seems to me if only
construction is required, but not mapping, we should rather split the
two operations in two methods (ie adding an explicit
CameraBuffer::map() method)

Thanks
  j


> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> ---
>  src/android/camera_buffer.h              |  7 ++++---
>  src/android/mm/cros_camera_buffer.cpp    | 17 +++++++++++++----
>  src/android/mm/generic_camera_buffer.cpp | 20 ++++++++++++--------
>  3 files changed, 29 insertions(+), 15 deletions(-)
>
> diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h
> index 7e8970b4..1fdb76a6 100644
> --- a/src/android/camera_buffer.h
> +++ b/src/android/camera_buffer.h
> @@ -17,7 +17,7 @@ class CameraBuffer final : public libcamera::Extensible
>  	LIBCAMERA_DECLARE_PRIVATE(CameraBuffer)
>
>  public:
> -	CameraBuffer(buffer_handle_t camera3Buffer, int flags);
> +	CameraBuffer(buffer_handle_t camera3Buffer, int flags, bool map = true);
>  	~CameraBuffer();
>
>  	bool isValid() const;
> @@ -31,8 +31,9 @@ public:
>  };
>
>  #define PUBLIC_CAMERA_BUFFER_IMPLEMENTATION				\
> -CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags)	\
> -	: Extensible(new Private(this, camera3Buffer, flags))		\
> +	CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags, \
> +				   bool map)				\
> +		: Extensible(new Private(this, camera3Buffer, flags, map))\
>  {									\
>  }									\
>  CameraBuffer::~CameraBuffer()						\
> diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp
> index 1a4fd5d1..71e03766 100644
> --- a/src/android/mm/cros_camera_buffer.cpp
> +++ b/src/android/mm/cros_camera_buffer.cpp
> @@ -21,7 +21,7 @@ class CameraBuffer::Private : public Extensible::Private
>
>  public:
>  	Private(CameraBuffer *cameraBuffer,
> -		buffer_handle_t camera3Buffer, int flags);
> +		buffer_handle_t camera3Buffer, int flags, bool map);
>  	~Private();
>
>  	bool isValid() const { return valid_; }
> @@ -38,6 +38,7 @@ private:
>  	unsigned int numPlanes_;
>  	bool valid_;
>  	bool registered_;
> +	bool map_;
>  	union {
>  		void *addr;
>  		android_ycbcr ycbcr;
> @@ -48,9 +49,10 @@ private:
>  };
>
>  CameraBuffer::Private::Private(CameraBuffer *cameraBuffer,
> -			       buffer_handle_t camera3Buffer, int flags)
> +			       buffer_handle_t camera3Buffer, int flags,
> +			       bool map)
>  	: Extensible::Private(cameraBuffer), handle_(camera3Buffer),
> -	  numPlanes_(0), valid_(false), registered_(false)
> +	  numPlanes_(0), valid_(false), registered_(false), map_(map)
>  {
>  	bufferManager_ = cros::CameraBufferManager::GetInstance();
>
> @@ -62,6 +64,13 @@ CameraBuffer::Private::Private(CameraBuffer *cameraBuffer,
>
>  	registered_ = true;
>  	numPlanes_ = bufferManager_->GetNumPlanes(camera3Buffer);
> +
> +	memset(&mem, 0, sizeof(mem));
> +	if (!map_) {
> +		valid_ = true;
> +		return;
> +	}
> +
>  	switch (numPlanes_) {
>  	case 1: {
>  		ret = bufferManager_->Lock(handle_, 0, 0, 0, 0, 0, &mem.addr);
> @@ -91,7 +100,7 @@ CameraBuffer::Private::Private(CameraBuffer *cameraBuffer,
>
>  CameraBuffer::Private::~Private()
>  {
> -	if (valid_)
> +	if (map_ && valid_)
>  		bufferManager_->Unlock(handle_);
>  	if (registered_)
>  		bufferManager_->Deregister(handle_);
> diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp
> index 929e078a..8fa79889 100644
> --- a/src/android/mm/generic_camera_buffer.cpp
> +++ b/src/android/mm/generic_camera_buffer.cpp
> @@ -21,7 +21,7 @@ class CameraBuffer::Private : public Extensible::Private,
>
>  public:
>  	Private(CameraBuffer *cameraBuffer,
> -		buffer_handle_t camera3Buffer, int flags);
> +		buffer_handle_t camera3Buffer, int flags, bool map);
>  	~Private();
>
>  	unsigned int numPlanes() const;
> @@ -32,7 +32,8 @@ public:
>  };
>
>  CameraBuffer::Private::Private(CameraBuffer *cameraBuffer,
> -			       buffer_handle_t camera3Buffer, int flags)
> +			       buffer_handle_t camera3Buffer,
> +			       int flags, bool map)
>  	: Extensible::Private(cameraBuffer)
>  {
>  	maps_.reserve(camera3Buffer->numFds);
> @@ -49,12 +50,15 @@ CameraBuffer::Private::Private(CameraBuffer *cameraBuffer,
>  			break;
>  		}
>
> -		void *address = mmap(nullptr, length, flags, MAP_SHARED,
> -				     camera3Buffer->data[i], 0);
> -		if (address == MAP_FAILED) {
> -			error_ = -errno;
> -			LOG(HAL, Error) << "Failed to mmap plane";
> -			break;
> +		void *address = nullptr;
> +		if (map) {
> +			mmap(nullptr, length, flags, MAP_SHARED,
> +			     camera3Buffer->data[i], 0);
> +			if (address == MAP_FAILED) {
> +				error_ = -errno;
> +				LOG(HAL, Error) << "Failed to mmap plane";
> +				break;
> +			}
>  		}
>
>  		maps_.emplace_back(static_cast<uint8_t *>(address),
> --
> 2.31.0.208.g409f899ff0-goog
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel