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

8667 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, and
FrameBuffers have their lifetime tracked in the Camera3RequestDescriptor
to ensure they are released when the Request is completed.

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>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/android/camera_device.cpp | 54 +++++++++++++++++++----------------
 src/android/camera_device.h   |  1 +
 2 files changed, 31 insertions(+), 24 deletions(-)

Hello all,

And due to it being late in the evening, I've missed one indent during
the rebase fixups :(

One minor indentation fixup to apply to this patch, but if there's
nothing else on this series I'll handle it while integrating.

On 07/07/2020 00:02, 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, and
> FrameBuffers have their lifetime tracked in the Camera3RequestDescriptor
> to ensure they are released when the Request is completed.
> 
> 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>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/android/camera_device.cpp | 54 +++++++++++++++++++----------------
>  src/android/camera_device.h   |  1 +
>  2 files changed, 31 insertions(+), 24 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 6d060e0c40e5..b13729e84abf 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -98,6 +98,7 @@ CameraDevice::Camera3RequestDescriptor::Camera3RequestDescriptor(
>  	: frameNumber(frameNumber), numBuffers(numBuffers)
>  {
>  	buffers = new camera3_stream_buffer_t[numBuffers];
> +	frameBuffers.reserve(numBuffers);
>  }
>  
>  CameraDevice::Camera3RequestDescriptor::~Camera3RequestDescriptor()
> @@ -990,6 +991,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  		config_->addConfiguration(streamConfiguration);
>  
>  		streams_[i].index = streamIndex++;
> +		stream->priv = static_cast<void *>(&streams_[i]);
>  	}
>  
>  	switch (config_->validate()) {
> @@ -1048,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;
> @@ -1088,30 +1087,35 @@ 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";
> +		/*
> +		 * Create a libcamera buffer using the dmabuf descriptors of the
> +		 * first and (currently) only supported request buffer.
> +		 * The FrameBuffer is directly associated with the
> +		 * Camera3RequestDescriptor for lifetime management only.
> +		 */
> +		FrameBuffer *buffer = createFrameBuffer(*camera3Buffers[i].buffer);
> +		if (!buffer) {
> +			LOG(HAL, Error) << "Failed to create buffer";
>  		delete request;

This "delete request" is now indented correctly one tab to the right.

--
Kieran



> -		delete descriptor;
> -		return -ENOMEM;
> -	}
> +			delete descriptor;
> +			return -ENOMEM;
> +		}
> +		descriptor->frameBuffers.emplace_back(buffer);
>  
> -	request->addBuffer(stream, buffer);
> +		StreamConfiguration *streamConfiguration = &config_->at(cameraStream->index);
> +		Stream *stream = streamConfiguration->stream();
> +
> +		request->addBuffer(stream, buffer);
> +	}
>  
>  	int ret = camera_->queueRequest(request);
>  	if (ret) {
> @@ -1127,7 +1131,6 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  void CameraDevice::requestComplete(Request *request)
>  {
>  	const std::map<Stream *, FrameBuffer *> &buffers = request->buffers();
> -	FrameBuffer *buffer = buffers.begin()->second;
>  	camera3_buffer_status status = CAMERA3_BUFFER_STATUS_OK;
>  	std::unique_ptr<CameraMetadata> resultMetadata;
>  
> @@ -1145,10 +1148,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;
> @@ -1156,6 +1155,14 @@ void CameraDevice::requestComplete(Request *request)
>  	captureResult.output_buffers =
>  		const_cast<const camera3_stream_buffer_t *>(descriptor->buffers);
>  
> +	/*
> +	 * \todo The timestamp used for the metadata is currently always taken
> +	 * from the first buffer (which may be the first stream) in the Request.
> +	 * It might be appropriate to return a 'correct' (as determined by
> +	 * pipeline handlers) timestamp in the Request itself.
> +	 */
> +	FrameBuffer *buffer = buffers.begin()->second;
> +
>  	if (status == CAMERA3_BUFFER_STATUS_OK) {
>  		notifyShutter(descriptor->frameNumber,
>  			      buffer->metadata().timestamp);
> @@ -1180,7 +1187,6 @@ void CameraDevice::requestComplete(Request *request)
>  	callbacks_->process_capture_result(callbacks_, &captureResult);
>  
>  	delete descriptor;
> -	delete buffer;
>  }
>  
>  std::string CameraDevice::logPrefix() const
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index d00f617b09a6..5b8b9c3e26e2 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -70,6 +70,7 @@ private:
>  		uint32_t frameNumber;
>  		uint32_t numBuffers;
>  		camera3_stream_buffer_t *buffers;
> +		std::vector<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers;
>  	};
>  
>  	struct Camera3StreamConfiguration {
>

Hi Kieran,
  two nits you can fix when applying

On Tue, Jul 07, 2020 at 12:02:39AM +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, and
> FrameBuffers have their lifetime tracked in the Camera3RequestDescriptor
> to ensure they are released when the Request is completed.
>
> 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>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/android/camera_device.cpp | 54 +++++++++++++++++++----------------
>  src/android/camera_device.h   |  1 +
>  2 files changed, 31 insertions(+), 24 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 6d060e0c40e5..b13729e84abf 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -98,6 +98,7 @@ CameraDevice::Camera3RequestDescriptor::Camera3RequestDescriptor(
>  	: frameNumber(frameNumber), numBuffers(numBuffers)
>  {
>  	buffers = new camera3_stream_buffer_t[numBuffers];
> +	frameBuffers.reserve(numBuffers);
>  }
>
>  CameraDevice::Camera3RequestDescriptor::~Camera3RequestDescriptor()
> @@ -990,6 +991,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  		config_->addConfiguration(streamConfiguration);
>
>  		streams_[i].index = streamIndex++;
> +		stream->priv = static_cast<void *>(&streams_[i]);
>  	}
>
>  	switch (config_->validate()) {
> @@ -1048,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;
> @@ -1088,30 +1087,35 @@ 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);

This is really long. I would break it after =

> +
>  		/*
>  		 * 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";
> +		/*
> +		 * Create a libcamera buffer using the dmabuf descriptors of the
> +		 * first and (currently) only supported request buffer.

This is not true anymore, isn't it ?

The rest looks good as the whole series does!
Thanks
  j

> +		 * The FrameBuffer is directly associated with the
> +		 * Camera3RequestDescriptor for lifetime management only.
> +		 */
> +		FrameBuffer *buffer = createFrameBuffer(*camera3Buffers[i].buffer);
> +		if (!buffer) {
> +			LOG(HAL, Error) << "Failed to create buffer";
>  		delete request;
> -		delete descriptor;
> -		return -ENOMEM;
> -	}
> +			delete descriptor;
> +			return -ENOMEM;
> +		}
> +		descriptor->frameBuffers.emplace_back(buffer);
>
> -	request->addBuffer(stream, buffer);
> +		StreamConfiguration *streamConfiguration = &config_->at(cameraStream->index);
> +		Stream *stream = streamConfiguration->stream();
> +
> +		request->addBuffer(stream, buffer);
> +	}
>
>  	int ret = camera_->queueRequest(request);
>  	if (ret) {
> @@ -1127,7 +1131,6 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  void CameraDevice::requestComplete(Request *request)
>  {
>  	const std::map<Stream *, FrameBuffer *> &buffers = request->buffers();
> -	FrameBuffer *buffer = buffers.begin()->second;
>  	camera3_buffer_status status = CAMERA3_BUFFER_STATUS_OK;
>  	std::unique_ptr<CameraMetadata> resultMetadata;
>
> @@ -1145,10 +1148,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;
> @@ -1156,6 +1155,14 @@ void CameraDevice::requestComplete(Request *request)
>  	captureResult.output_buffers =
>  		const_cast<const camera3_stream_buffer_t *>(descriptor->buffers);
>
> +	/*
> +	 * \todo The timestamp used for the metadata is currently always taken
> +	 * from the first buffer (which may be the first stream) in the Request.
> +	 * It might be appropriate to return a 'correct' (as determined by
> +	 * pipeline handlers) timestamp in the Request itself.
> +	 */
> +	FrameBuffer *buffer = buffers.begin()->second;
> +
>  	if (status == CAMERA3_BUFFER_STATUS_OK) {
>  		notifyShutter(descriptor->frameNumber,
>  			      buffer->metadata().timestamp);
> @@ -1180,7 +1187,6 @@ void CameraDevice::requestComplete(Request *request)
>  	callbacks_->process_capture_result(callbacks_, &captureResult);
>
>  	delete descriptor;
> -	delete buffer;
>  }
>
>  std::string CameraDevice::logPrefix() const
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index d00f617b09a6..5b8b9c3e26e2 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -70,6 +70,7 @@ private:
>  		uint32_t frameNumber;
>  		uint32_t numBuffers;
>  		camera3_stream_buffer_t *buffers;
> +		std::vector<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers;
>  	};
>
>  	struct Camera3StreamConfiguration {
> --
> 2.25.1
>

Hi Jacopo,

On 07/07/2020 08:47, Jacopo Mondi wrote:
> Hi Kieran,
>   two nits you can fix when applying
> 
> On Tue, Jul 07, 2020 at 12:02:39AM +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, and
>> FrameBuffers have their lifetime tracked in the Camera3RequestDescriptor
>> to ensure they are released when the Request is completed.
>>
>> 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>
>> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>> ---
>>  src/android/camera_device.cpp | 54 +++++++++++++++++++----------------
>>  src/android/camera_device.h   |  1 +
>>  2 files changed, 31 insertions(+), 24 deletions(-)
>>
>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
>> index 6d060e0c40e5..b13729e84abf 100644
>> --- a/src/android/camera_device.cpp
>> +++ b/src/android/camera_device.cpp
>> @@ -98,6 +98,7 @@ CameraDevice::Camera3RequestDescriptor::Camera3RequestDescriptor(
>>  	: frameNumber(frameNumber), numBuffers(numBuffers)
>>  {
>>  	buffers = new camera3_stream_buffer_t[numBuffers];
>> +	frameBuffers.reserve(numBuffers);
>>  }
>>
>>  CameraDevice::Camera3RequestDescriptor::~Camera3RequestDescriptor()
>> @@ -990,6 +991,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>>  		config_->addConfiguration(streamConfiguration);
>>
>>  		streams_[i].index = streamIndex++;
>> +		stream->priv = static_cast<void *>(&streams_[i]);
>>  	}
>>
>>  	switch (config_->validate()) {
>> @@ -1048,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;
>> @@ -1088,30 +1087,35 @@ 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);
> 
> This is really long. I would break it after =
> 

:( I think I prefer the single line, but I can do so.


Even formatting as:


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

goes up to 85 chars.

Do you think the line break really helps readability ?

For me adding new lines looks awkward, as it turns a single line
statement into ... well ... two lines ;-)


Anyway, I don't want to spend time worrying about line lengths.

I'll apply this with the newline as shown above.

>> +
>>  		/*
>>  		 * 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";
>> +		/*
>> +		 * Create a libcamera buffer using the dmabuf descriptors of the
>> +		 * first and (currently) only supported request buffer.
> 
> This is not true anymore, isn't it ?

Good point, I'll fix this as: (combining both the original statement and
my addition):

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




> The rest looks good as the whole series does!
> Thanks
>   j
> 
>> +		 * The FrameBuffer is directly associated with the
>> +		 * Camera3RequestDescriptor for lifetime management only.
>> +		 */
>> +		FrameBuffer *buffer = createFrameBuffer(*camera3Buffers[i].buffer);
>> +		if (!buffer) {
>> +			LOG(HAL, Error) << "Failed to create buffer";
>>  		delete request;
>> -		delete descriptor;
>> -		return -ENOMEM;
>> -	}
>> +			delete descriptor;
>> +			return -ENOMEM;
>> +		}
>> +		descriptor->frameBuffers.emplace_back(buffer);
>>
>> -	request->addBuffer(stream, buffer);
>> +		StreamConfiguration *streamConfiguration = &config_->at(cameraStream->index);
>> +		Stream *stream = streamConfiguration->stream();
>> +
>> +		request->addBuffer(stream, buffer);
>> +	}
>>
>>  	int ret = camera_->queueRequest(request);
>>  	if (ret) {
>> @@ -1127,7 +1131,6 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>>  void CameraDevice::requestComplete(Request *request)
>>  {
>>  	const std::map<Stream *, FrameBuffer *> &buffers = request->buffers();
>> -	FrameBuffer *buffer = buffers.begin()->second;
>>  	camera3_buffer_status status = CAMERA3_BUFFER_STATUS_OK;
>>  	std::unique_ptr<CameraMetadata> resultMetadata;
>>
>> @@ -1145,10 +1148,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;
>> @@ -1156,6 +1155,14 @@ void CameraDevice::requestComplete(Request *request)
>>  	captureResult.output_buffers =
>>  		const_cast<const camera3_stream_buffer_t *>(descriptor->buffers);
>>
>> +	/*
>> +	 * \todo The timestamp used for the metadata is currently always taken
>> +	 * from the first buffer (which may be the first stream) in the Request.
>> +	 * It might be appropriate to return a 'correct' (as determined by
>> +	 * pipeline handlers) timestamp in the Request itself.
>> +	 */
>> +	FrameBuffer *buffer = buffers.begin()->second;
>> +
>>  	if (status == CAMERA3_BUFFER_STATUS_OK) {
>>  		notifyShutter(descriptor->frameNumber,
>>  			      buffer->metadata().timestamp);
>> @@ -1180,7 +1187,6 @@ void CameraDevice::requestComplete(Request *request)
>>  	callbacks_->process_capture_result(callbacks_, &captureResult);
>>
>>  	delete descriptor;
>> -	delete buffer;
>>  }
>>
>>  std::string CameraDevice::logPrefix() const
>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
>> index d00f617b09a6..5b8b9c3e26e2 100644
>> --- a/src/android/camera_device.h
>> +++ b/src/android/camera_device.h
>> @@ -70,6 +70,7 @@ private:
>>  		uint32_t frameNumber;
>>  		uint32_t numBuffers;
>>  		camera3_stream_buffer_t *buffers;
>> +		std::vector<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers;
>>  	};
>>
>>  	struct Camera3StreamConfiguration {
>> --
>> 2.25.1
>>

Hi Kieran

On Tue, Jul 07, 2020 at 10:14:48AM +0100, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 07/07/2020 08:47, Jacopo Mondi wrote:
> > Hi Kieran,
> >   two nits you can fix when applying
> >
> > On Tue, Jul 07, 2020 at 12:02:39AM +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, and
> >> FrameBuffers have their lifetime tracked in the Camera3RequestDescriptor
> >> to ensure they are released when the Request is completed.
> >>
> >> 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>
> >> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> >> ---
> >>  src/android/camera_device.cpp | 54 +++++++++++++++++++----------------
> >>  src/android/camera_device.h   |  1 +
> >>  2 files changed, 31 insertions(+), 24 deletions(-)
> >>
> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> >> index 6d060e0c40e5..b13729e84abf 100644
> >> --- a/src/android/camera_device.cpp
> >> +++ b/src/android/camera_device.cpp
> >> @@ -98,6 +98,7 @@ CameraDevice::Camera3RequestDescriptor::Camera3RequestDescriptor(
> >>  	: frameNumber(frameNumber), numBuffers(numBuffers)
> >>  {
> >>  	buffers = new camera3_stream_buffer_t[numBuffers];
> >> +	frameBuffers.reserve(numBuffers);
> >>  }
> >>
> >>  CameraDevice::Camera3RequestDescriptor::~Camera3RequestDescriptor()
> >> @@ -990,6 +991,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >>  		config_->addConfiguration(streamConfiguration);
> >>
> >>  		streams_[i].index = streamIndex++;
> >> +		stream->priv = static_cast<void *>(&streams_[i]);
> >>  	}
> >>
> >>  	switch (config_->validate()) {
> >> @@ -1048,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;
> >> @@ -1088,30 +1087,35 @@ 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);
> >
> > This is really long. I would break it after =
> >
>
> :( I think I prefer the single line, but I can do so.
>
>
> Even formatting as:
>
>
> > 	for (unsigned int i = 0; i < descriptor->numBuffers; ++i) {
> > 		CameraStream *cameraStream =
> > 			static_cast<CameraStream *>(camera3Buffers[i].stream->priv);
>
> goes up to 85 chars.
>
> Do you think the line break really helps readability ?
>
> For me adding new lines looks awkward, as it turns a single line
> statement into ... well ... two lines ;-)
>
>
> Anyway, I don't want to spend time worrying about line lengths.
>
> I'll apply this with the newline as shown above.

As you wish, it's really only a suggestion

>
> >> +
> >>  		/*
> >>  		 * 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";
> >> +		/*
> >> +		 * Create a libcamera buffer using the dmabuf descriptors of the
> >> +		 * first and (currently) only supported request buffer.
> >
> > This is not true anymore, isn't it ?
>
> Good point, I'll fix this as: (combining both the original statement and
> my addition):
>
> """
> 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.
> """

Good, thanks!

>
>
>
>
> > The rest looks good as the whole series does!
> > Thanks
> >   j
> >
> >> +		 * The FrameBuffer is directly associated with the
> >> +		 * Camera3RequestDescriptor for lifetime management only.
> >> +		 */
> >> +		FrameBuffer *buffer = createFrameBuffer(*camera3Buffers[i].buffer);
> >> +		if (!buffer) {
> >> +			LOG(HAL, Error) << "Failed to create buffer";
> >>  		delete request;
> >> -		delete descriptor;
> >> -		return -ENOMEM;
> >> -	}
> >> +			delete descriptor;
> >> +			return -ENOMEM;
> >> +		}
> >> +		descriptor->frameBuffers.emplace_back(buffer);
> >>
> >> -	request->addBuffer(stream, buffer);
> >> +		StreamConfiguration *streamConfiguration = &config_->at(cameraStream->index);
> >> +		Stream *stream = streamConfiguration->stream();
> >> +
> >> +		request->addBuffer(stream, buffer);
> >> +	}
> >>
> >>  	int ret = camera_->queueRequest(request);
> >>  	if (ret) {
> >> @@ -1127,7 +1131,6 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >>  void CameraDevice::requestComplete(Request *request)
> >>  {
> >>  	const std::map<Stream *, FrameBuffer *> &buffers = request->buffers();
> >> -	FrameBuffer *buffer = buffers.begin()->second;
> >>  	camera3_buffer_status status = CAMERA3_BUFFER_STATUS_OK;
> >>  	std::unique_ptr<CameraMetadata> resultMetadata;
> >>
> >> @@ -1145,10 +1148,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;
> >> @@ -1156,6 +1155,14 @@ void CameraDevice::requestComplete(Request *request)
> >>  	captureResult.output_buffers =
> >>  		const_cast<const camera3_stream_buffer_t *>(descriptor->buffers);
> >>
> >> +	/*
> >> +	 * \todo The timestamp used for the metadata is currently always taken
> >> +	 * from the first buffer (which may be the first stream) in the Request.
> >> +	 * It might be appropriate to return a 'correct' (as determined by
> >> +	 * pipeline handlers) timestamp in the Request itself.
> >> +	 */
> >> +	FrameBuffer *buffer = buffers.begin()->second;
> >> +
> >>  	if (status == CAMERA3_BUFFER_STATUS_OK) {
> >>  		notifyShutter(descriptor->frameNumber,
> >>  			      buffer->metadata().timestamp);
> >> @@ -1180,7 +1187,6 @@ void CameraDevice::requestComplete(Request *request)
> >>  	callbacks_->process_capture_result(callbacks_, &captureResult);
> >>
> >>  	delete descriptor;
> >> -	delete buffer;
> >>  }
> >>
> >>  std::string CameraDevice::logPrefix() const
> >> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> >> index d00f617b09a6..5b8b9c3e26e2 100644
> >> --- a/src/android/camera_device.h
> >> +++ b/src/android/camera_device.h
> >> @@ -70,6 +70,7 @@ private:
> >>  		uint32_t frameNumber;
> >>  		uint32_t numBuffers;
> >>  		camera3_stream_buffer_t *buffers;
> >> +		std::vector<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers;
> >>  	};
> >>
> >>  	struct Camera3StreamConfiguration {
> >> --
> >> 2.25.1
> >>
>
> --
> Regards
> --
> Kieran