[libcamera-devel,v4,7/9] android: camera_device: Add methods to get and return buffers

9879 diff mbox series

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 FrameBuffer pointers with a
per-pool mutex in the get and return buffer methods.

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/android/camera_device.cpp | 34 +++++++++++++++++++++++++++++++++-
 src/android/camera_device.h   | 11 +++++++++--
 2 files changed, 42 insertions(+), 3 deletions(-)

Hi Jacopo,

Thank you for the patch.

On Wed, Sep 30, 2020 at 03:27:05PM +0200, 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 FrameBuffer pointers with a
> per-pool mutex in the get and return buffer methods.
> 
> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> Signed-off-by: Jacopo Mondi <jacopo@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 07041dd916d5..2ebc3fcc336e 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>

You have that in camera_device.h already.

>  #include <sys/mman.h>
>  #include <tuple>
>  #include <vector>
> @@ -1402,11 +1403,42 @@ int CameraDevice::allocateBufferPool(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];

	auto iter = bufferPool_.find(stream);
	if (iter == bufferPool_.end())
		return nullptr;

	BufferPool *pool = &iter->second;

to avoid a double lookup. Same below.

> +	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());

Erasing an element at the front isn't nice, as all the elements will
have to move. Wouldn't it be better to use a std::queue ? Alternatively
you can use use back() and pop_back(), as we don't need to cycle through
buffers in order.

> +
> +	return buffer;
> +}
> +
> +void CameraDevice::returnBuffer(libcamera::Stream *stream,
> +				libcamera::FrameBuffer *buffer)
> +{
> +	if (bufferPool_.find(stream) == bufferPool_.end())
> +		return;
> +
> +	BufferPool *pool = &bufferPool_[stream];
> +	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 c46610725e12..c91de367d7bd 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 {
> +		std::mutex mutex;
> +		std::vector<libcamera::FrameBuffer *> buffers;
> +	};
> +	using FrameBufferPool = std::map<libcamera::Stream *, BufferPool>;

I think I'd squash this patch with the previous one.

And wouldn't it be best to store the pool in the CameraStream class ?
The callers of allocateBufferPool(), getBuffer() and returnBuffer() all
have a pointer to the CameraStream.

>  
>  	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 allocateBufferPool(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);

Hi Laurent,

On Fri, Oct 02, 2020 at 03:35:25AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Wed, Sep 30, 2020 at 03:27:05PM +0200, 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 FrameBuffer pointers with a
> > per-pool mutex in the get and return buffer methods.
> >
> > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > Signed-off-by: Jacopo Mondi <jacopo@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 07041dd916d5..2ebc3fcc336e 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>
>
> You have that in camera_device.h already.
>

For sake of discussion: I tend to include all the headers I need in
the .cpp files too, mostly for clarity, as that's after all a no-op as
all headers are guarded. Is this a bad practice I should avoid ?

> >  #include <sys/mman.h>
> >  #include <tuple>
> >  #include <vector>
> > @@ -1402,11 +1403,42 @@ int CameraDevice::allocateBufferPool(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];
>
> 	auto iter = bufferPool_.find(stream);
> 	if (iter == bufferPool_.end())
> 		return nullptr;
>
> 	BufferPool *pool = &iter->second;
>
> to avoid a double lookup. Same below.
>

Ack!

> > +	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());
>
> Erasing an element at the front isn't nice, as all the elements will
> have to move. Wouldn't it be better to use a std::queue ? Alternatively
> you can use use back() and pop_back(), as we don't need to cycle through
> buffers in order.
>

I started with a queue, but seems to be quite expensive in terms of
memory. Hiro suggested to use the last entry of the vector, but we
might end up re-using the same buffer, potentially only a single one,
and this seems to be a bad idea even if it doesn't have any practical
effect.

What's best? A queue, or potentially re-use the same buffer over and over ?

> > +
> > +	return buffer;
> > +}
> > +
> > +void CameraDevice::returnBuffer(libcamera::Stream *stream,
> > +				libcamera::FrameBuffer *buffer)
> > +{
> > +	if (bufferPool_.find(stream) == bufferPool_.end())
> > +		return;
> > +
> > +	BufferPool *pool = &bufferPool_[stream];
> > +	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 c46610725e12..c91de367d7bd 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 {
> > +		std::mutex mutex;
> > +		std::vector<libcamera::FrameBuffer *> buffers;
> > +	};
> > +	using FrameBufferPool = std::map<libcamera::Stream *, BufferPool>;
>
> I think I'd squash this patch with the previous one.
>

I was afraid it would grow very large.. but Kieran had the same
comment on this type changing in two consecutive patches...

> And wouldn't it be best to store the pool in the CameraStream class ?
> The callers of allocateBufferPool(), getBuffer() and returnBuffer() all
> have a pointer to the CameraStream.

That's the long discussion I had with Kieran. I agree it would be
better placed in the CameraStream, but I think this would require a
major re-design of that class, that needs to be broken out from
the camera_device.cpp file and made copy-constructable. Also adding
yet another parameter to the constructor is not nice, it already has 5
of them.

If I could tie a CameraConfiguration to the Camera that created it,
from there get the StreamConfiguration at the right index, I could
save a few parameters now that I look at that again.

I will have another try maybe, but I already think this is piling too
many things on top of this series and CameraStream will require a
major re-design anyway. In case it gets too complex, I'll record with
a todo that we have to move allocation and framebuffer access there.

Thanks
  j

>
> >
> >  	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 allocateBufferPool(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,
>
> Laurent Pinchart

Hi Jacopo,

On Fri, Oct 02, 2020 at 04:46:22PM +0200, Jacopo Mondi wrote:
> On Fri, Oct 02, 2020 at 03:35:25AM +0300, Laurent Pinchart wrote:
> > On Wed, Sep 30, 2020 at 03:27:05PM +0200, 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 FrameBuffer pointers with a
> > > per-pool mutex in the get and return buffer methods.
> > >
> > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > > Signed-off-by: Jacopo Mondi <jacopo@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 07041dd916d5..2ebc3fcc336e 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>
> >
> > You have that in camera_device.h already.
> 
> For sake of discussion: I tend to include all the headers I need in
> the .cpp files too, mostly for clarity, as that's after all a no-op as
> all headers are guarded. Is this a bad practice I should avoid ?

Making sure we don't depend on implicit includes is a very good
practice. I usually don't include the headers that are included by the
header corresponding to a source file though. The rationale is that if
class Foo embeds a Bar, foo.h will have to include bar.h, so usage of
Bar in foo.cpp comes for free.

> > >  #include <sys/mman.h>
> > >  #include <tuple>
> > >  #include <vector>
> > > @@ -1402,11 +1403,42 @@ int CameraDevice::allocateBufferPool(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];
> >
> > 	auto iter = bufferPool_.find(stream);
> > 	if (iter == bufferPool_.end())
> > 		return nullptr;
> >
> > 	BufferPool *pool = &iter->second;
> >
> > to avoid a double lookup. Same below.
> 
> Ack!
> 
> > > +	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());
> >
> > Erasing an element at the front isn't nice, as all the elements will
> > have to move. Wouldn't it be better to use a std::queue ? Alternatively
> > you can use use back() and pop_back(), as we don't need to cycle through
> > buffers in order.
> 
> I started with a queue, but seems to be quite expensive in terms of
> memory. Hiro suggested to use the last entry of the vector, but we
> might end up re-using the same buffer, potentially only a single one,
> and this seems to be a bad idea even if it doesn't have any practical
> effect.
> 
> What's best? A queue, or potentially re-use the same buffer over and over ?

Reusing same buffer shouldn't be an issue. I don't think it will cause
that particular piece of RAM to wear out faster :-)

> > > +
> > > +	return buffer;
> > > +}
> > > +
> > > +void CameraDevice::returnBuffer(libcamera::Stream *stream,
> > > +				libcamera::FrameBuffer *buffer)
> > > +{
> > > +	if (bufferPool_.find(stream) == bufferPool_.end())
> > > +		return;
> > > +
> > > +	BufferPool *pool = &bufferPool_[stream];
> > > +	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 c46610725e12..c91de367d7bd 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 {
> > > +		std::mutex mutex;
> > > +		std::vector<libcamera::FrameBuffer *> buffers;
> > > +	};
> > > +	using FrameBufferPool = std::map<libcamera::Stream *, BufferPool>;
> >
> > I think I'd squash this patch with the previous one.
> 
> I was afraid it would grow very large.. but Kieran had the same
> comment on this type changing in two consecutive patches...
> 
> > And wouldn't it be best to store the pool in the CameraStream class ?
> > The callers of allocateBufferPool(), getBuffer() and returnBuffer() all
> > have a pointer to the CameraStream.
> 
> That's the long discussion I had with Kieran. I agree it would be
> better placed in the CameraStream, but I think this would require a
> major re-design of that class, that needs to be broken out from
> the camera_device.cpp file and made copy-constructable. Also adding
> yet another parameter to the constructor is not nice, it already has 5
> of them.
> 
> If I could tie a CameraConfiguration to the Camera that created it,
> from there get the StreamConfiguration at the right index, I could
> save a few parameters now that I look at that again.
> 
> I will have another try maybe, but I already think this is piling too
> many things on top of this series and CameraStream will require a
> major re-design anyway. In case it gets too complex, I'll record with
> a todo that we have to move allocation and framebuffer access there.

Works for me, thanks.

> > >
> > >  	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 allocateBufferPool(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);