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

[Laurent Pinchart]

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.
> 
> 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);
> +
> +	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,

Laurent Pinchart