[libcamera-devel] [PATCH v3 6/8] android: camera_device: Add methods to get and return buffers

[Kieran Bingham]

Hi Jacopo,

On 22/09/2020 10:47, Jacopo Mondi wrote:
> Add two methods to the CameraDevice class to retrieve and return
> frame buffers associated to a stream from the memory pool reserved
> in libcamera.
> 
> Protect accessing the vector of FrameBufer pointers with a

s/FrameBufer/FrameBuffer/

> per-pool mutex in the get and return buffer methods.
> 
> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/android/camera_device.cpp | 34 +++++++++++++++++++++++++++++++++-
>  src/android/camera_device.h   | 11 +++++++++--
>  2 files changed, 42 insertions(+), 3 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index f96ea7321a67..6ed56ff57dab 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -8,6 +8,7 @@
>  #include "camera_device.h"
>  #include "camera_ops.h"
>  
> +#include <mutex>
>  #include <sys/mman.h>
>  #include <tuple>
>  #include <vector>
> @@ -1400,11 +1401,42 @@ int CameraDevice::allocateBuffersPool(Stream *stream)
>  	 * the HAL.
>  	 */
>  	for (const auto &frameBuffer : allocator_.buffers(stream))
> -		bufferPool_[stream].push_back(frameBuffer.get());
> +		bufferPool_[stream].buffers.push_back(frameBuffer.get());
>  
>  	return 0;
>  }
>  
> +libcamera::FrameBuffer *CameraDevice::getBuffer(libcamera::Stream *stream)
> +{
> +	if (bufferPool_.find(stream) == bufferPool_.end())
> +		return nullptr;
> +
> +	BufferPool *pool = &bufferPool_[stream];
> +	std::lock_guard<std::mutex> locker(pool->mutex);
> +
> +	if (pool->buffers.empty()) {
> +		LOG(HAL, Error) << "Buffer underrun";
> +		return nullptr;
> +	}
> +
> +	FrameBuffer *buffer = pool->buffers.front();
> +	pool->buffers.erase(pool->buffers.begin());
> +

This feels quite complicated

A buffer pool with multiple sets of buffers which are specific to each
stream

As I've already suggested moving this stuff to be in a CameraStream:

How about making the BufferPool (with it's key attribute being the
locked container) a class with a getBuffer() and a putBuffer() which
ensures the lock is held in those calls.

Then the individual buffer pools keep the lock (I guess the locking is
required because this can all happen asynchronously in events).

> +	return buffer;
> +}
> +
> +void CameraDevice::returnBuffer(libcamera::Stream *stream,
> +				libcamera::FrameBuffer *buffer)
> +{
> +	if (bufferPool_.find(stream) == bufferPool_.end())
> +		return;
> +
> +	BufferPool *pool = &bufferPool_[stream];

Oh I misread earlier - it's a map of pools by stream, not a pool mapping
multiple streams of buffers.


For this series in general, I'd say - define the buffer /pool storage
you want, add it - then allocate the buffers and put them there.

This series has added them, then changed the storage along the way which
has been confusing to follow I think.



> +	std::lock_guard<std::mutex> locker(pool->mutex);
> +
> +	pool->buffers.push_back(buffer);
> +}
> +
>  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 4cef34c01a49..5addffdc070a 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -9,6 +9,7 @@
>  
>  #include <map>
>  #include <memory>
> +#include <mutex>
>  #include <tuple>
>  #include <vector>
>  
> @@ -166,8 +167,11 @@ protected:
>  	std::string logPrefix() const override;
>  
>  private:
> -	using FrameBufferPool = std::map<libcamera::Stream *,
> -					 std::vector<libcamera::FrameBuffer *>>;
> +	struct BufferPool {

Aha - my beloved BufferPool is coming back ;-)

> +		std::mutex mutex;
> +		std::vector<libcamera::FrameBuffer *> buffers;
> +	};
> +	using FrameBufferPool = std::map<libcamera::Stream *, BufferPool>;

I still see this (a BufferPool if required) as better handled inside the
CameraStream object.

I'm trying to picture in my head how the iterations might change, but I
guess when filling a request, you would cycle each android stream and
deliver all given buffers to the CameraStreams.

Then cycle all CameraStreams and ask if they have any buffer to add to
the current request.

Send request...

And on request complete, let the magic* complete everything.


* Insert magic pixie dust where required.

Hrm ... but all of that is possibly quite different to your
'internal/mapped/direct' concepts ... so I'm not quite sure yet anyway.



>  
>  	CameraDevice(unsigned int id, const std::shared_ptr<libcamera::Camera> &camera);
>  
> @@ -198,6 +202,9 @@ private:
>  	std::tuple<uint32_t, uint32_t> calculateStaticMetadataSize();
>  	libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer);
>  	int allocateBuffersPool(libcamera::Stream *stream);
> +	libcamera::FrameBuffer *getBuffer(libcamera::Stream *stream);
> +	void returnBuffer(libcamera::Stream *stream,
> +			  libcamera::FrameBuffer *buffer);
>  
>  	void notifyShutter(uint32_t frameNumber, uint64_t timestamp);
>  	void notifyError(uint32_t frameNumber, camera3_stream_t *stream);
> 

-- 
Regards
--
Kieran