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

[Jacopo Mondi]

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

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 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
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel