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

[Kieran Bingham]

Hi Jacopo,

On 23/09/2020 14:21, Jacopo Mondi wrote:
> 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.

Not necessarily, The code here could push the buffers directly into the
CameraStream buffer pool or buffer vector or such (transferring ownership).

Or the future bufferPool could also store a pointer to the Allocator
indeed, to be able to return / free them when destroyed etc..

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

-- 
Regards
--
Kieran