[libcamera-devel] [PATCH 10/15] android: camera_stream: Allocate FrameBuffers

[Kieran Bingham]

Hi Jacopo

On 05/10/2020 13:28, Laurent Pinchart wrote:
> Hi Jacopo,
> 
> Thank you for the patch.
> 
> On Mon, Oct 05, 2020 at 01:46:44PM +0300, Laurent Pinchart wrote:
>> From: Jacopo Mondi <jacopo at jmondi.org>
>>
>> Allocate FrameBuffers using the allocator_ class member in the
>> CameraStream class at CameraStream::configure() time.
>>
>> As FrameBuffer allocation can only happen after the Camera has been
>> correctly configured, move the CameraStream configuration loop
>> after the Camera::configure() call in CameraDevice.

If we know that now, should we update the patch earlier to put it there
in the first place?

(Or is that more effort than it's worth, or possible not correct at that
point in the series).


>>
>> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
>> ---
>>  src/android/camera_device.cpp | 22 +++++++++++-----------
>>  src/android/camera_stream.cpp | 17 +++++++++++++++--
>>  src/android/camera_stream.h   |  2 ++
>>  3 files changed, 28 insertions(+), 13 deletions(-)
>>
>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
>> index adaec54dbfdb..537883b63f15 100644
>> --- a/src/android/camera_device.cpp
>> +++ b/src/android/camera_device.cpp
>> @@ -1282,6 +1282,17 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>>  		return -EINVAL;
>>  	}
>>  
>> +	/*
>> +	 * Once the CameraConfiguration has been adjusted/validated
>> +	 * it can be applied to the camera.
>> +	 */
>> +	int ret = camera_->configure(config_.get());
>> +	if (ret) {
>> +		LOG(HAL, Error) << "Failed to configure camera '"
>> +				<< camera_->id() << "'";
>> +		return ret;
>> +	}
>> +
>>  	/*
>>  	 * Configure the HAL CameraStream instances using the associated
>>  	 * StreamConfiguration and set the number of required buffers in
>> @@ -1300,17 +1311,6 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>>  		stream->max_buffers = cfg.bufferCount;
>>  	}
>>  
>> -	/*
>> -	 * Once the CameraConfiguration has been adjusted/validated
>> -	 * it can be applied to the camera.
>> -	 */
>> -	int ret = camera_->configure(config_.get());
>> -	if (ret) {
>> -		LOG(HAL, Error) << "Failed to configure camera '"
>> -				<< camera_->id() << "'";
>> -		return ret;
>> -	}
>> -
>>  	return 0;
>>  }
>>  
>> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
>> index 9f3e7026b1a4..76e7d240f4e7 100644
>> --- a/src/android/camera_stream.cpp
>> +++ b/src/android/camera_stream.cpp
>> @@ -58,8 +58,21 @@ const Size &CameraStream::size() const
>>  
>>  int CameraStream::configure(const libcamera::StreamConfiguration &cfg)
>>  {
>> -	if (encoder_)
>> -		return encoder_->configure(cfg);
>> +	if (encoder_) {
>> +		int ret = encoder_->configure(cfg);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	if (allocator_) {
>> +		int ret = allocator_->allocate(stream());
>> +		if (ret < 0)
>> +			return ret;
>> +
>> +		/* Save a pointer to the reserved frame buffers */
>> +		for (const auto &frameBuffer : allocator_->buffers(stream()))
>> +			buffers_.push_back(frameBuffer.get());
> 
> I'd move this to patch 12/15, it's not clear in this patch why you need
> it.


If the configure gets moved in the patch when it is originally added,
and this buffers_.push_back gets moved to patch 12/15 - the only thing
left here might be the allocate - which at that piont might also be
easily squashed into 12/15... but it's not a requirement - just what
might happen if the other blocks reduce this patch to very little ;-)

> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

If it stays otherwise:

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>


> 
>> +	}
>>  
>>  	return 0;
>>  }
>> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
>> index eaf4357ed096..e399e17b2a2f 100644
>> --- a/src/android/camera_stream.h
>> +++ b/src/android/camera_stream.h
>> @@ -8,6 +8,7 @@
>>  #define __ANDROID_CAMERA_STREAM_H__
>>  
>>  #include <memory>
>> +#include <vector>
>>  
>>  #include <hardware/camera3.h>
>>  
>> @@ -140,6 +141,7 @@ private:
>>  	libcamera::CameraConfiguration *config_;
>>  	Encoder *encoder_;
>>  	libcamera::FrameBufferAllocator *allocator_;
>> +	std::vector<libcamera::FrameBuffer *> buffers_;
>>  };
>>  
>>  #endif /* __ANDROID_CAMERA_STREAM__ */
> 

-- 
Regards
--
Kieran