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

[Jacopo Mondi]

Hi Kieran,

On Thu, Sep 24, 2020 at 05:27:14PM +0100, Kieran Bingham wrote:
> 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

Just to note that if you move this to CameraStream, you will save the
first 3 lines only. And the first two are already a paranoid check as
that condition shall never happen.

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

Locking is require because you can have a request to complete while
another one is queued. Might this happen from different threads of
execution ? I am not a 100% sure but it mostly depends on the
threading model of the camera stack, and we have no guarantees that
it's single threaded.

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

When it was added there was no lock required. As soon as it's need I
added it. Adding before using it makes any sense ?

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


So I tried moving the buffer pool to the CameraStream, but to me there
are two constraints here:

- the FrameBufferAllocator is constructed with a Camera and indexes
  buffers by Stream -> it belongs to the CameraDevice which also
  manages the lifetime of the allocated FrameBuffer
- I don't want resource allocation/free in one class and the logic to
  get/return buffers in one other as it seems a layer violation to me

I tried moving allocation/release of buffers as well as get/retrurn as
you suggested to the CameraStream class. What happens is that:
- The CameraStream constructor has now 7 parameters -> it's a signal
  something is wrong imho
- I want to release buffers for the Stream handled by the CameraStream
  in the CameraStream::~CameraStream() destructor. If I add a
  destructor the class implicit copy contructor gets deleted. If I add
  it as a default it fails as the Encoder virtual base class is not
  copy constructible. If I add a copy constructor I should rework how
  the encoder is handled, as right now is a unique_ptr<> and moving it
  from 'other' to 'this' would invalidate 'other'. If the CameraStream
  class is not copy constructable, we cannot streams_.reserve() and
  all the house of cards built on the assumption streams_ doesn't get
  relocated falls down.

All in all, I don't think it's worth at this point. CameraStream will
be reworked, currently there's no clear separation between what's
Android and what's libcamera (I'm referring to sizes and format) and
the constructor has already too many parameters sign that we're
duplicating information between CameraDevice and CameraStream.

For this series, I won't go to that extent. Maybe there's some easy
route I haven't identified yet.

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