[libcamera-devel,1/2] android: CameraBuffer: Add option of not mapping a buffer
diff mbox series

Message ID 20210405040424.1929737-2-hiroh@chromium.org
State Superseded
Headers show
Series
  • Check a buffer validness by CameraBuffer::IsValid()
Related show

Commit Message

Hirokazu Honda April 5, 2021, 4:04 a.m. UTC 
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.

Signed-off-by: Hirokazu Honda <hiroh@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(-)

Comments

Jacopo Mondi April 6, 2021, 12:47 p.m. UTC | #1 
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@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@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart Aug. 20, 2021, 11:25 a.m. UTC | #2 
Hello,

On Tue, Apr 06, 2021 at 02:47:34PM +0200, Jacopo Mondi wrote:
> 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)

I agree, I think that the constructor should not map the buffer, this
should instead be done in plane() the first time it's called.

> > Signed-off-by: Hirokazu Honda <hiroh@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),

Patch
diff mbox series 

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),