[libcamera-devel] [PATCH v2 7/9] android: camera_device: Add buffers for each stream to Requests

[Kieran Bingham]

Hi Jacopo,

On 03/07/2020 10:47, Jacopo Mondi wrote:
> Hi Kieran,
> 
> On Thu, Jul 02, 2020 at 10:36:52PM +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 at ideasonboard.com>
>> ---
>>  src/android/camera_device.cpp | 38 ++++++++++++++---------------------
>>  1 file changed, 15 insertions(+), 23 deletions(-)
>>
>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
>> index fc3962dac230..eea8c8c50352 100644
>> --- a/src/android/camera_device.cpp
>> +++ b/src/android/camera_device.cpp
>> @@ -977,6 +977,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>>
>>  		/* Maintain internal state of all stream mappings. */
>>  		streams_[i].androidStream = stream;
>> +		stream->priv = static_cast<void*>(&streams_[i]);
>                                                ^ space ?

Checkstyle hasn't complained. Where would you have a space? After the '>' ?

I'm not sure it needs it, I don't think we've done that anywhere else...

> 
>>
>>  		StreamConfiguration streamConfiguration;
>>
>> @@ -1045,9 +1046,6 @@ static FrameBuffer *newFrameBuffer(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;
>> @@ -1085,30 +1083,28 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>>  		camera_->createRequest(reinterpret_cast<uint64_t>(descriptor));

I.e. there's no space in the casting above....?

Oh - now looking up - did you mean between void and * ?

>>
>>  	for (unsigned int i = 0; i < descriptor->numBuffers; ++i) {
>> +		CameraStream *cameraStream = static_cast<CameraStream*>(camera3Buffers[i].stream->priv);
>                                                                      ^
>                                                                      space ?

Is this before the '*' ?

> 
> This makes me think that you could store a StreamConfiguration *
> in stream->priv.

I'll need to store more data regarding an internal HAL only stream very
soon, so I'd like to keep a specific structure.



>> +
>>  		/*
>>  		 * 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 = newFrameBuffer(*camera3Buffers[0].buffer);
>> -	if (!buffer) {
>> -		LOG(HAL, Error) << "Failed to create buffer";
>> -		delete request;
>> -		delete descriptor;
>> -		return -ENOMEM;
>> -	}
>> +		FrameBuffer *buffer = newFrameBuffer(*camera3Buffers[0].buffer);
>> +		if (!buffer) {
>> +			LOG(HAL, Error) << "Failed to create buffer";
>> +			delete request;
>> +			delete descriptor;
>> +			return -ENOMEM;
>> +		}
>> +
>> +		StreamConfiguration *streamConfiguration = &config_->at(cameraStream->libcameraIndex);
>> +		Stream *stream = streamConfiguration->stream();
>>
>> -	request->addBuffer(stream, buffer);
>> +		request->addBuffer(stream, buffer);
>> +	}
>>
>>  	int ret = camera_->queueRequest(request);
>>  	if (ret) {
>> @@ -1142,10 +1138,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 at lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards
--
Kieran