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

[Kieran Bingham]

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 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.


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

--
KB

>> 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
--
Kieran