[libcamera-devel,09/15] android: camera_stream: Add FrameBufferAllocator
diff mbox series

Message ID 20201005104649.10812-10-laurent.pinchart@ideasonboard.com
State Rejected
Delegated to: Laurent Pinchart
Headers show
Series
  • CameraStream refactoring
Related show

Commit Message

Laurent Pinchart Oct. 5, 2020, 10:46 a.m. UTC 
From: Jacopo Mondi <jacopo@jmondi.org>

Add a FrameBufferAllocator class member to the CameraStream class.
The allocator is constructed for CameraStream instances that needs
internal allocation and deleted at class destruction time.

The definition of a destructor for the class deletes the implicitly
defined copy constructor, required as the CameraDevice stores
CameraStream instances in a pre-reserved vector. In order to make the
CameraStream copy-constructable it is required to make the encoder_
pointer decay to a raw pointer, as unique_ptr<> are not
copy-constructable.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/android/camera_stream.cpp | 13 +++++++++++--
 src/android/camera_stream.h   |  6 +++++-
 2 files changed, 16 insertions(+), 3 deletions(-)

Comments

Laurent Pinchart Oct. 5, 2020, 12:25 p.m. UTC | #1 
Hi Jacopo,

Thank you for the patch.

On Mon, Oct 05, 2020 at 01:46:43PM +0300, Laurent Pinchart wrote:
> From: Jacopo Mondi <jacopo@jmondi.org>
> 
> Add a FrameBufferAllocator class member to the CameraStream class.
> The allocator is constructed for CameraStream instances that needs
> internal allocation and deleted at class destruction time.
> 
> The definition of a destructor for the class deletes the implicitly
> defined copy constructor, required as the CameraDevice stores
> CameraStream instances in a pre-reserved vector. In order to make the
> CameraStream copy-constructable it is required to make the encoder_
> pointer decay to a raw pointer, as unique_ptr<> are not
> copy-constructable.

How about defining a move constructor instead ? That's all that
std::vector<> should need if I'm not mistaken.

> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/android/camera_stream.cpp | 13 +++++++++++--
>  src/android/camera_stream.h   |  6 +++++-
>  2 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> index 6e7419ae31b8..9f3e7026b1a4 100644
> --- a/src/android/camera_stream.cpp
> +++ b/src/android/camera_stream.cpp
> @@ -19,12 +19,21 @@ LOG_DECLARE_CATEGORY(HAL);
>  CameraStream::CameraStream(CameraDevice *cameraDev, Type type,
>  			   camera3_stream_t *androidStream, unsigned int index)
>  	: cameraDevice_(cameraDev), type_(type), androidStream_(androidStream),
> -	  index_(index), encoder_(nullptr)
> +	  index_(index), encoder_(nullptr), allocator_(nullptr)
>  {
>  	config_ = cameraDevice_->cameraConfiguration();
>  
>  	if (type_ == Type::Internal || type_ == Type::Mapped)
> -		encoder_.reset(new EncoderLibJpeg);
> +		encoder_ = new EncoderLibJpeg();
> +
> +	if (type == Type::Internal)
> +		allocator_ = new FrameBufferAllocator(cameraDevice_->camera());
> +}
> +
> +CameraStream::~CameraStream()
> +{
> +	delete encoder_;
> +	delete allocator_;
>  }
>  
>  const StreamConfiguration &CameraStream::streamConfiguration() const
> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> index b67b4c0ca0b3..eaf4357ed096 100644
> --- a/src/android/camera_stream.h
> +++ b/src/android/camera_stream.h
> @@ -13,6 +13,7 @@
>  
>  #include <libcamera/buffer.h>
>  #include <libcamera/camera.h>
> +#include <libcamera/framebuffer_allocator.h>
>  #include <libcamera/geometry.h>
>  #include <libcamera/pixel_format.h>
>  
> @@ -109,6 +110,8 @@ public:
>  	};
>  	CameraStream(CameraDevice *cameraDev, Type type,
>  		     camera3_stream_t *androidStream, unsigned int index);
> +	CameraStream(const CameraStream &) = default;
> +	~CameraStream();

If you declare a copy constructor, you should also declare a copy
assignement operator (but as stated above I think you need the move
constructor and assignment operators instead).

>  
>  	int outputFormat() const { return androidStream_->format; }
>  	Type type() const { return type_; }
> @@ -135,7 +138,8 @@ private:
>  	 */
>  	unsigned int index_;
>  	libcamera::CameraConfiguration *config_;
> -	std::unique_ptr<Encoder> encoder_;
> +	Encoder *encoder_;
> +	libcamera::FrameBufferAllocator *allocator_;
>  };
>  
>  #endif /* __ANDROID_CAMERA_STREAM__ */
Kieran Bingham Oct. 6, 2020, 10:35 a.m. UTC | #2 
Hi Jacopo

On 05/10/2020 13:25, Laurent Pinchart wrote:
> Hi Jacopo,
> 
> Thank you for the patch.
> 
> On Mon, Oct 05, 2020 at 01:46:43PM +0300, Laurent Pinchart wrote:
>> From: Jacopo Mondi <jacopo@jmondi.org>
>>
>> Add a FrameBufferAllocator class member to the CameraStream class.
>> The allocator is constructed for CameraStream instances that needs
>> internal allocation and deleted at class destruction time.
>>
>> The definition of a destructor for the class deletes the implicitly
>> defined copy constructor, required as the CameraDevice stores
>> CameraStream instances in a pre-reserved vector. In order to make the
>> CameraStream copy-constructable it is required to make the encoder_
>> pointer decay to a raw pointer, as unique_ptr<> are not
>> copy-constructable.
> 
> How about defining a move constructor instead ? That's all that
> std::vector<> should need if I'm not mistaken.


If that's all it needs that sounds like a better route indeed.

--
KB

>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>> ---
>>  src/android/camera_stream.cpp | 13 +++++++++++--
>>  src/android/camera_stream.h   |  6 +++++-
>>  2 files changed, 16 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
>> index 6e7419ae31b8..9f3e7026b1a4 100644
>> --- a/src/android/camera_stream.cpp
>> +++ b/src/android/camera_stream.cpp
>> @@ -19,12 +19,21 @@ LOG_DECLARE_CATEGORY(HAL);
>>  CameraStream::CameraStream(CameraDevice *cameraDev, Type type,
>>  			   camera3_stream_t *androidStream, unsigned int index)
>>  	: cameraDevice_(cameraDev), type_(type), androidStream_(androidStream),
>> -	  index_(index), encoder_(nullptr)
>> +	  index_(index), encoder_(nullptr), allocator_(nullptr)
>>  {
>>  	config_ = cameraDevice_->cameraConfiguration();
>>  
>>  	if (type_ == Type::Internal || type_ == Type::Mapped)
>> -		encoder_.reset(new EncoderLibJpeg);
>> +		encoder_ = new EncoderLibJpeg();
>> +
>> +	if (type == Type::Internal)
>> +		allocator_ = new FrameBufferAllocator(cameraDevice_->camera());
>> +}
>> +
>> +CameraStream::~CameraStream()
>> +{
>> +	delete encoder_;
>> +	delete allocator_;
>>  }
>>  
>>  const StreamConfiguration &CameraStream::streamConfiguration() const
>> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
>> index b67b4c0ca0b3..eaf4357ed096 100644
>> --- a/src/android/camera_stream.h
>> +++ b/src/android/camera_stream.h
>> @@ -13,6 +13,7 @@
>>  
>>  #include <libcamera/buffer.h>
>>  #include <libcamera/camera.h>
>> +#include <libcamera/framebuffer_allocator.h>
>>  #include <libcamera/geometry.h>
>>  #include <libcamera/pixel_format.h>
>>  
>> @@ -109,6 +110,8 @@ public:
>>  	};
>>  	CameraStream(CameraDevice *cameraDev, Type type,
>>  		     camera3_stream_t *androidStream, unsigned int index);
>> +	CameraStream(const CameraStream &) = default;
>> +	~CameraStream();
> 
> If you declare a copy constructor, you should also declare a copy
> assignement operator (but as stated above I think you need the move
> constructor and assignment operators instead).
> 
>>  
>>  	int outputFormat() const { return androidStream_->format; }
>>  	Type type() const { return type_; }
>> @@ -135,7 +138,8 @@ private:
>>  	 */
>>  	unsigned int index_;
>>  	libcamera::CameraConfiguration *config_;
>> -	std::unique_ptr<Encoder> encoder_;
>> +	Encoder *encoder_;
>> +	libcamera::FrameBufferAllocator *allocator_;
>>  };
>>  
>>  #endif /* __ANDROID_CAMERA_STREAM__ */
>
Jacopo Mondi Oct. 6, 2020, 11:19 a.m. UTC | #3 
Hi Kieran, Laurent,

On Tue, Oct 06, 2020 at 11:35:46AM +0100, Kieran Bingham wrote:
> Hi Jacopo
>
> On 05/10/2020 13:25, Laurent Pinchart wrote:
> > Hi Jacopo,
> >
> > Thank you for the patch.
> >
> > On Mon, Oct 05, 2020 at 01:46:43PM +0300, Laurent Pinchart wrote:
> >> From: Jacopo Mondi <jacopo@jmondi.org>
> >>
> >> Add a FrameBufferAllocator class member to the CameraStream class.
> >> The allocator is constructed for CameraStream instances that needs
> >> internal allocation and deleted at class destruction time.
> >>
> >> The definition of a destructor for the class deletes the implicitly
> >> defined copy constructor, required as the CameraDevice stores
> >> CameraStream instances in a pre-reserved vector. In order to make the
> >> CameraStream copy-constructable it is required to make the encoder_
> >> pointer decay to a raw pointer, as unique_ptr<> are not
> >> copy-constructable.
> >
> > How about defining a move constructor instead ? That's all that
> > std::vector<> should need if I'm not mistaken.
>
>
> If that's all it needs that sounds like a better route indeed.
>

Even better, if I use unique_ptr for every field (encoder_,
allocator_, mutex_) I don't need a custom destructor, so the
constructor implicitly generated by the compiler stays valid and I
don't need to force it.

Thanks
  j

> --
> KB
>
> >> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >> ---
> >>  src/android/camera_stream.cpp | 13 +++++++++++--
> >>  src/android/camera_stream.h   |  6 +++++-
> >>  2 files changed, 16 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> >> index 6e7419ae31b8..9f3e7026b1a4 100644
> >> --- a/src/android/camera_stream.cpp
> >> +++ b/src/android/camera_stream.cpp
> >> @@ -19,12 +19,21 @@ LOG_DECLARE_CATEGORY(HAL);
> >>  CameraStream::CameraStream(CameraDevice *cameraDev, Type type,
> >>  			   camera3_stream_t *androidStream, unsigned int index)
> >>  	: cameraDevice_(cameraDev), type_(type), androidStream_(androidStream),
> >> -	  index_(index), encoder_(nullptr)
> >> +	  index_(index), encoder_(nullptr), allocator_(nullptr)
> >>  {
> >>  	config_ = cameraDevice_->cameraConfiguration();
> >>
> >>  	if (type_ == Type::Internal || type_ == Type::Mapped)
> >> -		encoder_.reset(new EncoderLibJpeg);
> >> +		encoder_ = new EncoderLibJpeg();
> >> +
> >> +	if (type == Type::Internal)
> >> +		allocator_ = new FrameBufferAllocator(cameraDevice_->camera());
> >> +}
> >> +
> >> +CameraStream::~CameraStream()
> >> +{
> >> +	delete encoder_;
> >> +	delete allocator_;
> >>  }
> >>
> >>  const StreamConfiguration &CameraStream::streamConfiguration() const
> >> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> >> index b67b4c0ca0b3..eaf4357ed096 100644
> >> --- a/src/android/camera_stream.h
> >> +++ b/src/android/camera_stream.h
> >> @@ -13,6 +13,7 @@
> >>
> >>  #include <libcamera/buffer.h>
> >>  #include <libcamera/camera.h>
> >> +#include <libcamera/framebuffer_allocator.h>
> >>  #include <libcamera/geometry.h>
> >>  #include <libcamera/pixel_format.h>
> >>
> >> @@ -109,6 +110,8 @@ public:
> >>  	};
> >>  	CameraStream(CameraDevice *cameraDev, Type type,
> >>  		     camera3_stream_t *androidStream, unsigned int index);
> >> +	CameraStream(const CameraStream &) = default;
> >> +	~CameraStream();
> >
> > If you declare a copy constructor, you should also declare a copy
> > assignement operator (but as stated above I think you need the move
> > constructor and assignment operators instead).
> >
> >>
> >>  	int outputFormat() const { return androidStream_->format; }
> >>  	Type type() const { return type_; }
> >> @@ -135,7 +138,8 @@ private:
> >>  	 */
> >>  	unsigned int index_;
> >>  	libcamera::CameraConfiguration *config_;
> >> -	std::unique_ptr<Encoder> encoder_;
> >> +	Encoder *encoder_;
> >> +	libcamera::FrameBufferAllocator *allocator_;
> >>  };
> >>
> >>  #endif /* __ANDROID_CAMERA_STREAM__ */
> >
>
> --
> Regards
> --
> Kieran
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Patch
diff mbox series 

diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
index 6e7419ae31b8..9f3e7026b1a4 100644
--- a/src/android/camera_stream.cpp
+++ b/src/android/camera_stream.cpp
@@ -19,12 +19,21 @@  LOG_DECLARE_CATEGORY(HAL);
 CameraStream::CameraStream(CameraDevice *cameraDev, Type type,
 			   camera3_stream_t *androidStream, unsigned int index)
 	: cameraDevice_(cameraDev), type_(type), androidStream_(androidStream),
-	  index_(index), encoder_(nullptr)
+	  index_(index), encoder_(nullptr), allocator_(nullptr)
 {
 	config_ = cameraDevice_->cameraConfiguration();
 
 	if (type_ == Type::Internal || type_ == Type::Mapped)
-		encoder_.reset(new EncoderLibJpeg);
+		encoder_ = new EncoderLibJpeg();
+
+	if (type == Type::Internal)
+		allocator_ = new FrameBufferAllocator(cameraDevice_->camera());
+}
+
+CameraStream::~CameraStream()
+{
+	delete encoder_;
+	delete allocator_;
 }
 
 const StreamConfiguration &CameraStream::streamConfiguration() const
diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
index b67b4c0ca0b3..eaf4357ed096 100644
--- a/src/android/camera_stream.h
+++ b/src/android/camera_stream.h
@@ -13,6 +13,7 @@ 
 
 #include <libcamera/buffer.h>
 #include <libcamera/camera.h>
+#include <libcamera/framebuffer_allocator.h>
 #include <libcamera/geometry.h>
 #include <libcamera/pixel_format.h>
 
@@ -109,6 +110,8 @@  public:
 	};
 	CameraStream(CameraDevice *cameraDev, Type type,
 		     camera3_stream_t *androidStream, unsigned int index);
+	CameraStream(const CameraStream &) = default;
+	~CameraStream();
 
 	int outputFormat() const { return androidStream_->format; }
 	Type type() const { return type_; }
@@ -135,7 +138,8 @@  private:
 	 */
 	unsigned int index_;
 	libcamera::CameraConfiguration *config_;
-	std::unique_ptr<Encoder> encoder_;
+	Encoder *encoder_;
+	libcamera::FrameBufferAllocator *allocator_;
 };
 
 #endif /* __ANDROID_CAMERA_STREAM__ */