[libcamera-devel,v3,7/8] android: camera_device: Add buffers for each stream to Requests

8587 diff mbox series

Construct a FrameBuffer for every buffer given in the camera3Request
and add it to the libcamera Request on the appropriate stream.

The correct stream is obtained from the private data of the camera3_stream
associated with the camera3_buffer.

Comments regarding supporting only one buffer are now removed.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/android/camera_device.cpp | 42 ++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 23 deletions(-)

Hi Jacopo,

On 03/07/2020 14:24, Jacopo Mondi wrote:
> Hi Kieran,
> 
> On Fri, Jul 03, 2020 at 01:39:18PM +0100, Kieran Bingham wrote:
>> Construct a FrameBuffer for every buffer given in the camera3Request
>> and add it to the libcamera Request on the appropriate stream.
>>
>> The correct stream is obtained from the private data of the camera3_stream
>> associated with the camera3_buffer.
>>
>> Comments regarding supporting only one buffer are now removed.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> ---
>>  src/android/camera_device.cpp | 42 ++++++++++++++++-------------------
>>  1 file changed, 19 insertions(+), 23 deletions(-)
>>
>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
>> index 28334751a26e..3d21e59af258 100644
>> --- a/src/android/camera_device.cpp
>> +++ b/src/android/camera_device.cpp
>> @@ -991,6 +991,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>>
>>  		/* Maintain internal state of all stream mappings. */
>>  		streams_[i].index = streamIndex++;
>> +		stream->priv = static_cast<void *>(&streams_[i]);
>>  	}
>>
>>  	switch (config_->validate()) {
>> @@ -1049,9 +1050,6 @@ FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer
>>
>>  int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request)
>>  {
>> -	StreamConfiguration *streamConfiguration = &config_->at(0);
>> -	Stream *stream = streamConfiguration->stream();
>> -
>>  	if (camera3Request->num_output_buffers != 1) {
>>  		LOG(HAL, Error) << "Invalid number of output buffers: "
>>  				<< camera3Request->num_output_buffers;
>> @@ -1089,30 +1087,32 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>>  		camera_->createRequest(reinterpret_cast<uint64_t>(descriptor));
>>
>>  	for (unsigned int i = 0; i < descriptor->numBuffers; ++i) {
>> +		CameraStream *cameraStream = static_cast<CameraStream *>(camera3Buffers[i].stream->priv);
>> +
>>  		/*
>>  		 * Keep track of which stream the request belongs to and store
>>  		 * the native buffer handles.
>> -		 *
>> -		 * \todo Currently we only support one capture buffer. Copy
>> -		 * all of them to be ready once we'll support more.
>>  		 */
>>  		descriptor->buffers[i].stream = camera3Buffers[i].stream;
>>  		descriptor->buffers[i].buffer = camera3Buffers[i].buffer;
>> -	}
>>
>> -	/*
>> -	 * Create a libcamera buffer using the dmabuf descriptors of the first
>> -	 * and (currently) only supported request buffer.
>> -	 */
>> -	FrameBuffer *buffer = createFrameBuffer(*camera3Buffers[0].buffer);
>> -	if (!buffer) {
>> -		LOG(HAL, Error) << "Failed to create buffer";
>> -		delete request;
>> -		delete descriptor;
>> -		return -ENOMEM;
>> -	}
>> +		/*
>> +		* Create a libcamera buffer using the dmabuf descriptors of the
>> +		* first and (currently) only supported request buffer.
>> +		*/
>> +		FrameBuffer *buffer = createFrameBuffer(*camera3Buffers[0].buffer);
>> +		if (!buffer) {
>> +			LOG(HAL, Error) << "Failed to create buffer";
>> +			delete request;
>> +			delete descriptor;
>> +			return -ENOMEM;
>> +		}
>>
>> -	request->addBuffer(stream, buffer);
>> +		StreamConfiguration *streamConfiguration = &config_->at(cameraStream->index);
>> +		Stream *stream = streamConfiguration->stream();
>> +
>> +		request->addBuffer(stream, buffer);
> 
> As we support multiple streams/buffers now, they should be handled in
> CameraDevice::requestComplete(), as of now only the first frame buffer
> is correctly deleted.

Indeed,

> 
>> +	}
>>
>>  	int ret = camera_->queueRequest(request);
>>  	if (ret) {
> 
> Looking at the error path here, should we free the just allocated
> FrameBuffers if queueing a request fails ?
> 
> Would it make sense to track all the allocated FrameBuffers as part of
> the Camera3RequestDescriptor, and free all of them here and at the end
> of requestComplete() ?

We do certainly need to delete them all, but in requestComplete, we have
all the FrameBuffers as part of the request, so I don't think we need to
track them in the Camera3RequestDescriptor, unless that's the best place
to keep them until we know they have been queued succesfully.

I'll try to take a look.

--
Kieran



>> @@ -1146,10 +1146,6 @@ void CameraDevice::requestComplete(Request *request)
>>  	captureResult.frame_number = descriptor->frameNumber;
>>  	captureResult.num_output_buffers = descriptor->numBuffers;
>>  	for (unsigned int i = 0; i < descriptor->numBuffers; ++i) {
>> -		/*
>> -		 * \todo Currently we only support one capture buffer. Prepare
>> -		 * all of them to be ready once we'll support more.
>> -		 */
>>  		descriptor->buffers[i].acquire_fence = -1;
>>  		descriptor->buffers[i].release_fence = -1;
>>  		descriptor->buffers[i].status = status;
>> --
>> 2.25.1
>>
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel@lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Kieran,

On Fri, Jul 03, 2020 at 01:39:18PM +0100, Kieran Bingham wrote:
> Construct a FrameBuffer for every buffer given in the camera3Request
> and add it to the libcamera Request on the appropriate stream.
>
> The correct stream is obtained from the private data of the camera3_stream
> associated with the camera3_buffer.
>
> Comments regarding supporting only one buffer are now removed.
>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/android/camera_device.cpp | 42 ++++++++++++++++-------------------
>  1 file changed, 19 insertions(+), 23 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 28334751a26e..3d21e59af258 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -991,6 +991,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>
>  		/* Maintain internal state of all stream mappings. */
>  		streams_[i].index = streamIndex++;
> +		stream->priv = static_cast<void *>(&streams_[i]);
>  	}
>
>  	switch (config_->validate()) {
> @@ -1049,9 +1050,6 @@ FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer
>
>  int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request)
>  {
> -	StreamConfiguration *streamConfiguration = &config_->at(0);
> -	Stream *stream = streamConfiguration->stream();
> -
>  	if (camera3Request->num_output_buffers != 1) {
>  		LOG(HAL, Error) << "Invalid number of output buffers: "
>  				<< camera3Request->num_output_buffers;
> @@ -1089,30 +1087,32 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  		camera_->createRequest(reinterpret_cast<uint64_t>(descriptor));
>
>  	for (unsigned int i = 0; i < descriptor->numBuffers; ++i) {
> +		CameraStream *cameraStream = static_cast<CameraStream *>(camera3Buffers[i].stream->priv);
> +
>  		/*
>  		 * Keep track of which stream the request belongs to and store
>  		 * the native buffer handles.
> -		 *
> -		 * \todo Currently we only support one capture buffer. Copy
> -		 * all of them to be ready once we'll support more.
>  		 */
>  		descriptor->buffers[i].stream = camera3Buffers[i].stream;
>  		descriptor->buffers[i].buffer = camera3Buffers[i].buffer;
> -	}
>
> -	/*
> -	 * Create a libcamera buffer using the dmabuf descriptors of the first
> -	 * and (currently) only supported request buffer.
> -	 */
> -	FrameBuffer *buffer = createFrameBuffer(*camera3Buffers[0].buffer);
> -	if (!buffer) {
> -		LOG(HAL, Error) << "Failed to create buffer";
> -		delete request;
> -		delete descriptor;
> -		return -ENOMEM;
> -	}
> +		/*
> +		* Create a libcamera buffer using the dmabuf descriptors of the
> +		* first and (currently) only supported request buffer.
> +		*/
> +		FrameBuffer *buffer = createFrameBuffer(*camera3Buffers[0].buffer);
> +		if (!buffer) {
> +			LOG(HAL, Error) << "Failed to create buffer";
> +			delete request;
> +			delete descriptor;
> +			return -ENOMEM;
> +		}
>
> -	request->addBuffer(stream, buffer);
> +		StreamConfiguration *streamConfiguration = &config_->at(cameraStream->index);
> +		Stream *stream = streamConfiguration->stream();
> +
> +		request->addBuffer(stream, buffer);

As we support multiple streams/buffers now, they should be handled in
CameraDevice::requestComplete(), as of now only the first frame buffer
is correctly deleted.

> +	}
>
>  	int ret = camera_->queueRequest(request);
>  	if (ret) {

Looking at the error path here, should we free the just allocated
FrameBuffers if queueing a request fails ?

Would it make sense to track all the allocated FrameBuffers as part of
the Camera3RequestDescriptor, and free all of them here and at the end
of requestComplete() ?

> @@ -1146,10 +1146,6 @@ void CameraDevice::requestComplete(Request *request)
>  	captureResult.frame_number = descriptor->frameNumber;
>  	captureResult.num_output_buffers = descriptor->numBuffers;
>  	for (unsigned int i = 0; i < descriptor->numBuffers; ++i) {
> -		/*
> -		 * \todo Currently we only support one capture buffer. Prepare
> -		 * all of them to be ready once we'll support more.
> -		 */
>  		descriptor->buffers[i].acquire_fence = -1;
>  		descriptor->buffers[i].release_fence = -1;
>  		descriptor->buffers[i].status = status;
> --
> 2.25.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel