[libcamera-devel] [PATCH v3 5/8] android: camera_device: Allocate buffer pools

[Jacopo Mondi]

Hi Kieran

On Wed, Sep 23, 2020 at 01:28:26PM +0100, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 22/09/2020 10:47, Jacopo Mondi wrote:
> > After the Camera has been configured, walk the list of collected
> > CameraStream instances and allocate memory for the ones that needs
> > intermediate buffers reserved by the libcamera FrameBufferAllocator.
> >
> > Maintain a map between each Stream and a vector of pointers to the
> > associated buffers.
> >
> > Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/android/camera_device.cpp | 36 +++++++++++++++++++++++++++++++++++
> >  src/android/camera_device.h   |  6 ++++++
> >  2 files changed, 42 insertions(+)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index 42fb9ea4e113..f96ea7321a67 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -1189,6 +1189,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >  	streams_.clear();
> >  	streams_.reserve(stream_list->num_streams);
> >  	allocator_.clear();
> > +	bufferPool_.clear();
> >
> >  	/* First handle all non-MJPEG streams. */
> >  	camera3_stream_t *jpegStream = nullptr;
> > @@ -1336,6 +1337,25 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >  		return ret;
> >  	}
> >
> > +	/*
> > +	 * Now that the camera has been configured allocate buffers for
> > +	 * the streams that need memory reserved by libcamera.
> > +	 */
> > +	for (const CameraStream &cameraStream : streams_) {
> > +		const StreamConfiguration &cfg = config_->at(cameraStream.index());
> > +		Stream *stream = cfg.stream();
> > +
> > +		if (cameraStream.type() != CameraStream::Type::Internal)
> > +			continue;
> > +
> > +		ret = allocateBuffersPool(stream);
> > +		if (ret) {
> > +			LOG(HAL, Error) << "Failed to allocate buffers for stream "
> > +					<< cameraStream.index();
> > +			return ret;
> > +		}
> > +	}
> > +
>
> Some how I thought I envisaged the ownership of these buffers being 'in'
> the CameraStream class ... but I think I can see that it might be
> difficult to map that.
>
> I'll try to think some more, and I'm sure things will be more clear as I
> go through the other patches.

I considered that. However, the allocator is global to the camera
device which manages its creation and celeanup, so it kind of made
sense to me to make the pool a camera device feature. I also would
have had to pass the allocator around, but that's probably not that
bad, as it is required for allocation only.

>
>
> >  	return 0;
> >  }
> >
> > @@ -1369,6 +1389,22 @@ FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer
> >  	return new FrameBuffer(std::move(planes));
> >  }
> >
> > +int CameraDevice::allocateBuffersPool(Stream *stream)
>
> s/Buffers/Buffer/ ... Pool is already plural, so it sounds really odd to
> have a Buffers Pool.
>
> (you can have a pool of buffers, but not a buffers pool - It's just a
> buffer pool).
>

Ah thanks, I was actually not sure about this

>
> > +{
> > +	int ret = allocator_.allocate(stream);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	/*
> > +	 * Save a pointer to the reserved frame buffer for usage in
> > +	 * the HAL.
> > +	 */
> > +	for (const auto &frameBuffer : allocator_.buffers(stream))
> > +		bufferPool_[stream].push_back(frameBuffer.get());
> > +
> > +	return 0;
> > +}
> > +
> >  int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request)
> >  {
> >  	if (!camera3Request->num_output_buffers) {
> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > index 84f636f7a93c..4cef34c01a49 100644
> > --- a/src/android/camera_device.h
> > +++ b/src/android/camera_device.h
> > @@ -166,6 +166,9 @@ protected:
> >  	std::string logPrefix() const override;
> >
> >  private:
> > +	using FrameBufferPool = std::map<libcamera::Stream *,
> > +					 std::vector<libcamera::FrameBuffer *>>;
> > +
>
> Putting these 'in' the CameraStream would mean we don't need to keep a
> mapping of it?

We would save a map and store the vectors in each CameraStream, yes.

>
> Hrm ... time to read further forwards...
>
>
> >  	CameraDevice(unsigned int id, const std::shared_ptr<libcamera::Camera> &camera);
> >
> >  	struct Camera3RequestDescriptor {
> > @@ -194,6 +197,8 @@ private:
> >
> >  	std::tuple<uint32_t, uint32_t> calculateStaticMetadataSize();
> >  	libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer);
> > +	int allocateBuffersPool(libcamera::Stream *stream);
> > +
> >  	void notifyShutter(uint32_t frameNumber, uint64_t timestamp);
> >  	void notifyError(uint32_t frameNumber, camera3_stream_t *stream);
> >  	CameraMetadata *requestTemplatePreview();
> > @@ -208,6 +213,7 @@ private:
> >  	std::shared_ptr<libcamera::Camera> camera_;
> >  	std::unique_ptr<libcamera::CameraConfiguration> config_;
> >  	libcamera::FrameBufferAllocator allocator_;
> > +	FrameBufferPool bufferPool_;
> >
> >  	CameraMetadata *staticMetadata_;
> >  	std::map<unsigned int, const CameraMetadata *> requestTemplates_;
> >
>
> --
> Regards
> --
> Kieran