[libcamera-devel] [PATCH 09/15] android: camera_stream: Add FrameBufferAllocator

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Mon, Oct 05, 2020 at 01:46:43PM +0300, Laurent Pinchart wrote:
> From: Jacopo Mondi <jacopo at 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 at 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,

Laurent Pinchart