[libcamera-devel] [PATCH 10/15] android: camera_stream: Allocate FrameBuffers

[Jacopo Mondi]

Hi Kieran

On Tue, Oct 06, 2020 at 01:29:30PM +0100, Kieran Bingham wrote:
> Hi Jacopo
>
> On 05/10/2020 13:28, Laurent Pinchart wrote:
> > Hi Jacopo,
> >
> > Thank you for the patch.
> >
> > On Mon, Oct 05, 2020 at 01:46:44PM +0300, Laurent Pinchart wrote:
> >> From: Jacopo Mondi <jacopo at jmondi.org>
> >>
> >> Allocate FrameBuffers using the allocator_ class member in the
> >> CameraStream class at CameraStream::configure() time.
> >>
> >> As FrameBuffer allocation can only happen after the Camera has been
> >> correctly configured, move the CameraStream configuration loop
> >> after the Camera::configure() call in CameraDevice.
>
> If we know that now, should we update the patch earlier to put it there
> in the first place?
>
> (Or is that more effort than it's worth, or possible not correct at that
> point in the series).

It is not required before CameraStream::configure() has to allocate
buffers, so I think the change should happen at the same time.

>
>
> >>
> >> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> >> ---
> >>  src/android/camera_device.cpp | 22 +++++++++++-----------
> >>  src/android/camera_stream.cpp | 17 +++++++++++++++--
> >>  src/android/camera_stream.h   |  2 ++
> >>  3 files changed, 28 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> >> index adaec54dbfdb..537883b63f15 100644
> >> --- a/src/android/camera_device.cpp
> >> +++ b/src/android/camera_device.cpp
> >> @@ -1282,6 +1282,17 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >>  		return -EINVAL;
> >>  	}
> >>
> >> +	/*
> >> +	 * Once the CameraConfiguration has been adjusted/validated
> >> +	 * it can be applied to the camera.
> >> +	 */
> >> +	int ret = camera_->configure(config_.get());
> >> +	if (ret) {
> >> +		LOG(HAL, Error) << "Failed to configure camera '"
> >> +				<< camera_->id() << "'";
> >> +		return ret;
> >> +	}
> >> +
> >>  	/*
> >>  	 * Configure the HAL CameraStream instances using the associated
> >>  	 * StreamConfiguration and set the number of required buffers in
> >> @@ -1300,17 +1311,6 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >>  		stream->max_buffers = cfg.bufferCount;
> >>  	}
> >>
> >> -	/*
> >> -	 * Once the CameraConfiguration has been adjusted/validated
> >> -	 * it can be applied to the camera.
> >> -	 */
> >> -	int ret = camera_->configure(config_.get());
> >> -	if (ret) {
> >> -		LOG(HAL, Error) << "Failed to configure camera '"
> >> -				<< camera_->id() << "'";
> >> -		return ret;
> >> -	}
> >> -
> >>  	return 0;
> >>  }
> >>
> >> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> >> index 9f3e7026b1a4..76e7d240f4e7 100644
> >> --- a/src/android/camera_stream.cpp
> >> +++ b/src/android/camera_stream.cpp
> >> @@ -58,8 +58,21 @@ const Size &CameraStream::size() const
> >>
> >>  int CameraStream::configure(const libcamera::StreamConfiguration &cfg)
> >>  {
> >> -	if (encoder_)
> >> -		return encoder_->configure(cfg);
> >> +	if (encoder_) {
> >> +		int ret = encoder_->configure(cfg);
> >> +		if (ret)
> >> +			return ret;
> >> +	}
> >> +
> >> +	if (allocator_) {
> >> +		int ret = allocator_->allocate(stream());
> >> +		if (ret < 0)
> >> +			return ret;
> >> +
> >> +		/* Save a pointer to the reserved frame buffers */
> >> +		for (const auto &frameBuffer : allocator_->buffers(stream()))
> >> +			buffers_.push_back(frameBuffer.get());
> >
> > I'd move this to patch 12/15, it's not clear in this patch why you need
> > it.
>
>
> If the configure gets moved in the patch when it is originally added,
> and this buffers_.push_back gets moved to patch 12/15 - the only thing
> left here might be the allocate - which at that piont might also be
> easily squashed into 12/15... but it's not a requirement - just what
> might happen if the other blocks reduce this patch to very little ;-)
>

I've moved everything to patch 12/15

> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> If it stays otherwise:
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>

Thanks
  j

>
> >
> >> +	}
> >>
> >>  	return 0;
> >>  }
> >> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> >> index eaf4357ed096..e399e17b2a2f 100644
> >> --- a/src/android/camera_stream.h
> >> +++ b/src/android/camera_stream.h
> >> @@ -8,6 +8,7 @@
> >>  #define __ANDROID_CAMERA_STREAM_H__
> >>
> >>  #include <memory>
> >> +#include <vector>
> >>
> >>  #include <hardware/camera3.h>
> >>
> >> @@ -140,6 +141,7 @@ private:
> >>  	libcamera::CameraConfiguration *config_;
> >>  	Encoder *encoder_;
> >>  	libcamera::FrameBufferAllocator *allocator_;
> >> +	std::vector<libcamera::FrameBuffer *> buffers_;
> >>  };
> >>
> >>  #endif /* __ANDROID_CAMERA_STREAM__ */
> >
>
> --
> Regards
> --
> Kieran