[libcamera-devel] [PATCH 7/8] android: camera_device: Use libcamera buffer pool

[Jacopo Mondi]

Hi Niklas,

On Thu, Sep 10, 2020 at 01:35:05PM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your work.
>
> On 2020-09-09 17:54:56 +0200, Jacopo Mondi wrote:
> > Now that we have reserved and made available to the camera HAL a
> > pool of libcamera allocated buffers, use them when a CameraStream
> > instance that requires internal allocation is processed.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/android/camera_device.cpp | 47 +++++++++++++++++++++++++----------
> >  1 file changed, 34 insertions(+), 13 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index a7cb1be03b9a..f94b313581e7 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -1422,6 +1422,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >  	for (unsigned int i = 0; i < descriptor->numBuffers; ++i) {
> >  		CameraStream *cameraStream =
> >  			static_cast<CameraStream *>(camera3Buffers[i].stream->priv);
> > +		const StreamConfiguration &config = config_->at(cameraStream->index());
> > +		Stream *stream = config.stream();
> >
> >  		/*
> >  		 * Keep track of which stream the request belongs to and store
> > @@ -1430,27 +1432,39 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >  		descriptor->buffers[i].stream = camera3Buffers[i].stream;
> >  		descriptor->buffers[i].buffer = camera3Buffers[i].buffer;
> >
> > -		/* Software streams are handled after hardware streams complete. */
> > -		if (cameraStream->format() == formats::MJPEG)
> > +		/* Mapped streams don't need to be added to the Request. */
> > +		if (cameraStream->type() == CameraStream::Type::Mapped)
> >  			continue;
> >
> > -		/*
> > -		 * Create a libcamera buffer using the dmabuf descriptors of
> > -		 * the camera3Buffer for each stream. The FrameBuffer is
> > -		 * directly associated with the Camera3RequestDescriptor for
> > -		 * lifetime management only.
> > -		 */
> > -		FrameBuffer *buffer = createFrameBuffer(*camera3Buffers[i].buffer);
> > +		FrameBuffer *buffer;
> > +		if (cameraStream->type() == CameraStream::Type::Direct) {
> > +			/*
> > +			 * Create a libcamera buffer using the dmabuf
> > +			 * descriptors of the camera3Buffer for each stream and
> > +			 * associate it with the Camera3RequestDescriptor for
> > +			 * lifetime management only.
> > +			 */
> > +			buffer = createFrameBuffer(*camera3Buffers[i].buffer);
> > +			descriptor->frameBuffers.emplace_back(buffer);
> > +
> > +		} else {
> > +			/*
> > +			 * Get the frame buffer from the CameraStream internal
> > +			 * buffer pool. The lifetime management of internal
> > +			 * buffers is connected to the one of the
> > +			 * FrameBufferAllocator instance.
> > +			 *
> > +			 * The retrieved buffer has to be returned to the
> > +			 * allocator once it has been processed.
> > +			 */
> > +			buffer = allocator_.getBuffer(stream);
>
> Should we add a LOG(Error) if there are no free buffers to retrieve form
> the allocator?
>
> > +		}
> >  		if (!buffer) {

It's catched here

> >  			LOG(HAL, Error) << "Failed to create buffer";
> >  			delete request;
> >  			delete descriptor;
> >  			return -ENOMEM;
> >  		}
> > -		descriptor->frameBuffers.emplace_back(buffer);
> > -
> > -		StreamConfiguration *streamConfiguration = &config_->at(cameraStream->index());
> > -		Stream *stream = streamConfiguration->stream();
> >
> >  		request->addBuffer(stream, buffer);
> >  	}
> > @@ -1561,6 +1575,13 @@ void CameraDevice::requestComplete(Request *request)
> >  		const uint32_t jpeg_orientation = 0;
> >  		resultMetadata->addEntry(ANDROID_JPEG_ORIENTATION,
> >  					 &jpeg_orientation, 1);
> > +
> > +		/*
> > +		 * Return the FrameBuffer to the CameraStream now that we're
> > +		 * done processing it.
> > +		 */
> > +		if (cameraStream->type() == CameraStream::Type::Internal)
> > +			allocator_.returnBuffer(stream, buffer);
>
> Seeing how the getBuffer() and returnBuffer() is used here could it not
> be replaced with a  std::queue in a similar fashion we have in
> pipeline handlers?
>

That's what I've done initially, but that's basically repeting the
same pattern in several parts of libcamera.

When I saw

void CIO2Device::tryReturnBuffer(FrameBuffer *buffer)
{
	/*
	 * \todo Once more pipelines deal with buffers that may be allocated
	 * internally or externally this pattern might become a common need. At
	 * that point this check should be moved to something clever in
	 * FrameBuffer.
	 */
	for (const std::unique_ptr<FrameBuffer> &buf : buffers_) {
		if (buf.get() == buffer) {
			availableBuffers_.push(buffer);
			break;
		}
	}
}

I'm not saying using an std::queue<> is "clever" as it's surely
efficient in insertion/removal but occupies quite some memory, but I
considered this worth to be brought to be to be part of the
FrameBufferAllocator interface.

My intention was to replace the custom implementations in the
pipelines by using the newly defined getBuffer()/returnBuffer()
methods calls.


> >  	}
> >
> >  	/* Prepare to call back the Android camera stack. */
> > --
> > 2.28.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund