[libcamera-devel] [PATCH 03/15] android: camera_stream: Delegate Encoder construction

[Kieran Bingham]

Hi Jacopo

On 05/10/2020 12:26, Laurent Pinchart wrote:
> Hi Jacopo,
> 
> Thank you for the patch.
> 
> On Mon, Oct 05, 2020 at 01:46:37PM +0300, Laurent Pinchart wrote:
>> From: Jacopo Mondi <jacopo at jmondi.org>
>>
>> Delegate the construction of the encoder to the CameraStream class
>> for streams that need post-processing.
>>

Very happy to see this get pushed down to CameraStream ...
This series is getting me far too excited - and I'm only on patch 3!

:-)


>> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
>> ---
>>  src/android/camera_device.cpp | 23 ++++++++++-------------
>>  src/android/camera_stream.cpp | 17 ++++++++++++++---
>>  src/android/camera_stream.h   |  4 +++-
>>  3 files changed, 27 insertions(+), 17 deletions(-)
>>
>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
>> index 0600ebc81c64..9c9a5cfa3c2f 100644
>> --- a/src/android/camera_device.cpp
>> +++ b/src/android/camera_device.cpp
>> @@ -1273,19 +1273,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>>  
>>  		StreamConfiguration &cfg = config_->at(index);
>>  
>> -		/*
>> -		 * Construct a software encoder for the MJPEG streams from the
>> -		 * chosen libcamera source stream.
>> -		 */
>> -		Encoder *encoder = new EncoderLibJpeg();
>> -		int ret = encoder->configure(cfg);
>> -		if (ret) {
>> -			LOG(HAL, Error) << "Failed to configure encoder";
>> -			delete encoder;
>> -			return ret;
>> -		}
>> -
>> -		streams_.emplace_back(formats::MJPEG, cfg.size, type, index, encoder);
>> +		streams_.emplace_back(formats::MJPEG, cfg.size, type, index);
>>  		jpegStream->priv = static_cast<void *>(&streams_.back());
>>  	}
>>  
>> @@ -1306,11 +1294,20 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>>  		return -EINVAL;
>>  	}
>>  
>> +	/*
>> +	 * Configure the HAL CameraStream instances using the associated
>> +	 * 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);
>>  		StreamConfiguration &cfg = config_->at(cameraStream->index());
>>  
>> +		int ret = cameraStream->configure(cfg);
>> +		if (ret)
>> +			return ret;
>> +
>>  		/* Use the bufferCount confirmed by the validation process. */
>>  		stream->max_buffers = cfg.bufferCount;
>>  	}
>> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
>> index 585bf2b68f4f..5b2b625c563b 100644
>> --- a/src/android/camera_stream.cpp
>> +++ b/src/android/camera_stream.cpp
>> @@ -7,10 +7,21 @@
>>  
>>  #include "camera_stream.h"
>>  
>> +#include "jpeg/encoder_libjpeg.h"
>> +
>>  using namespace libcamera;
>>  
>> -CameraStream::CameraStream(PixelFormat format, Size size,
>> -			   Type type, unsigned int index, Encoder *encoder)
>> -	: format_(format), size_(size), type_(type), index_(index), encoder_(encoder)
>> +CameraStream::CameraStream(PixelFormat format, Size size, Type type, unsigned int index)
>> +	: format_(format), size_(size), type_(type), index_(index), encoder(nullptr)
>>  {
>> +	if (type_ == Type::Internal || type_ == Type::Mapped)
>> +		encoder_.reset(new EncoderLibJpeg);
> 
> 		encoder_ = std::make_unique<EncoderLibJpeg>();
> 
> This is functionally equivalent but more "C++".
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
>> +}
>> +
>> +int CameraStream::configure(const libcamera::StreamConfiguration &cfg)
>> +{
>> +	if (encoder_)
>> +		return encoder_->configure(cfg);

I wonder if in the near future, the creation of the Encoder might happen
on demand here depending on what is needed to fulfil the
streamConfiguration, rather than being in the constructor.

But I'm not worried about that now, and I'm very pleased to see the
CameraStream specific code moving into here, so lets proceed!

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 07c714e3a365..d0dc40d81151 100644
>> --- a/src/android/camera_stream.h
>> +++ b/src/android/camera_stream.h
>> @@ -100,7 +100,7 @@ public:
>>  		Mapped,
>>  	};
>>  	CameraStream(libcamera::PixelFormat format, libcamera::Size size,
>> -		     Type type, unsigned int index, Encoder *encoder = nullptr);
>> +		     Type type, unsigned int index);
>>  
>>  	const libcamera::PixelFormat &format() const { return format_; }
>>  	const libcamera::Size &size() const { return size_; }
>> @@ -108,6 +108,8 @@ public:
>>  	unsigned int index() const { return index_; }
>>  	Encoder *encoder() const { return encoder_.get(); }
>>  
>> +	int configure(const libcamera::StreamConfiguration &cfg);
>> +
>>  private:
>>  	libcamera::PixelFormat format_;
>>  	libcamera::Size size_;
> 

-- 
Regards
--
Kieran