[libcamera-devel] [PATCH 11/15] android: camera_device: Make CameraStream configuration nicer

[Kieran Bingham]

Hi Jacopo,

On 05/10/2020 13:30, Laurent Pinchart wrote:
> Hi Jacopo,
> 
> Thank you for the patch.
> 
> On Mon, Oct 05, 2020 at 01:46:45PM +0300, Laurent Pinchart wrote:
>> From: Jacopo Mondi <jacopo at jmondi.org>
>>
>> Loop over the CameraStream instances and use their interface to perform
>> CameraStream configuration after the Camera::configure().
>>
>> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
>> ---
>>  src/android/camera_device.cpp | 10 ++++------
>>  src/android/camera_stream.cpp |  4 ++--
>>  src/android/camera_stream.h   |  3 ++-
>>  3 files changed, 8 insertions(+), 9 deletions(-)
>>
>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
>> index 537883b63f15..62307726aea9 100644
>> --- a/src/android/camera_device.cpp
>> +++ b/src/android/camera_device.cpp
>> @@ -1298,17 +1298,15 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>>  	 * StreamConfiguration and set the number of required buffers in
>>  	 * the Android camera3_stream_t.
>>  	 */
>> -	for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
>> -		camera3_stream_t *stream = stream_list->streams[i];
>> -		CameraStream *cameraStream = static_cast<CameraStream *>(stream->priv);
>> -		const StreamConfiguration &cfg = cameraStream->streamConfiguration();
>> +	for (CameraStream &cameraStream : streams_) {
>> +		camera3_stream_t *stream = cameraStream.androidStream();

Oh that's nicer indeed.

>>  
>> -		int ret = cameraStream->configure(cfg);
>> +		int ret = cameraStream.configure();
>>  		if (ret)
>>  			return ret;
>>  
>>  		/* Use the bufferCount confirmed by the validation process. */
>> -		stream->max_buffers = cfg.bufferCount;
>> +		stream->max_buffers = cameraStream.streamConfiguration().bufferCount;
> 
> Should this move to the CameraStream class ?>
>>  	}
>>  
>>  	return 0;
>> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
>> index 76e7d240f4e7..dbde1e786300 100644
>> --- a/src/android/camera_stream.cpp
>> +++ b/src/android/camera_stream.cpp
>> @@ -56,10 +56,10 @@ const Size &CameraStream::size() const
>>  	return streamConfiguration().size;
>>  }
>>  
>> -int CameraStream::configure(const libcamera::StreamConfiguration &cfg)
>> +int CameraStream::configure()
>>  {
>>  	if (encoder_) {
>> -		int ret = encoder_->configure(cfg);
>> +		int ret = encoder_->configure(streamConfiguration());
>>  		if (ret)
>>  			return ret;
>>  	}
>> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
>> index e399e17b2a2f..c6ff79230b7e 100644
>> --- a/src/android/camera_stream.h
>> +++ b/src/android/camera_stream.h
>> @@ -116,13 +116,14 @@ public:
>>  
>>  	int outputFormat() const { return androidStream_->format; }
>>  	Type type() const { return type_; }
>> +	camera3_stream_t *androidStream() const { return androidStream_; }

If the max_buffers/bufferCount does move to CameraStream, this might not
be needed in this patch - but I think it's certainly better to be able
to iterate out HAL objects and get the camera3 types from our
representations - so I really like this androidStream() in it's
potential usage.

If it ends up dropped, it can always come in when required later.

I'll still add this :

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


>>  
>>  	const libcamera::StreamConfiguration &streamConfiguration() const;
>>  	libcamera::Stream *stream() const;
>>  	const libcamera::PixelFormat &format() const;
>>  	const libcamera::Size &size() const;
>>  
>> -	int configure(const libcamera::StreamConfiguration &cfg);
>> +	int configure();
>>  	int process(libcamera::FrameBuffer *source,
>>  		    MappedCamera3Buffer *dest,
>>  		    CameraMetadata *metadata);
> 

-- 
Regards
--
Kieran