[libcamera-devel] [PATCH v2 11/12] android: camera_device: Set Encoder at construction

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wed, Sep 02, 2020 at 05:22:35PM +0200, Jacopo Mondi wrote:
> Make the CameraStream Encoder * a private struct member and require its
> initialization at construction time.
> 
> This change dis-allow creating a CameraStream and set the Encoder later,

s/dis-allow/disallows/

> which shall not happen now that we create CameraStream once we have all
> the required information in place.
> 
> No functional changes intended but this change aims to make the code
> more robust enforcing a stricter CameraStream interface.

Thinking ahead in the future, do you think this will be a good match
when we'll add other post-processing than JPEG encoding (I'm thinking
about scaling, rotation and/or format conversion) ? I don't see any
particular issue, but a second opinion would be useful.

> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>

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

> ---
>  src/android/camera_device.cpp | 23 ++++++++++++-----------
>  src/android/camera_device.h   |  7 ++++---
>  2 files changed, 16 insertions(+), 14 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 9bcd1d993c17..28d8e081eab0 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -168,14 +168,14 @@ MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,
>  	}
>  }
>  
> -CameraStream::CameraStream(PixelFormat f, Size s, unsigned int i)
> -	: format(f), size(s), jpeg(nullptr), index_(i)
> +CameraStream::CameraStream(PixelFormat f, Size s, unsigned int i, Encoder *encoder)
> +	: format(f), size(s), index_(i), encoder_(encoder)
>  {
>  }
>  
>  CameraStream::~CameraStream()
>  {
> -	delete jpeg;
> +	delete encoder_;
>  };
>  
>  /*
> @@ -1279,20 +1279,21 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  		PixelFormat format = formats::MJPEG;
>  		Size size = cfg.size;
>  
> -		CameraStream &cameraStream = streams_.emplace_back(format, size, index);
> -		stream->priv = static_cast<void *>(&cameraStream);
> -
>  		/*
>  		 * Construct a software encoder for MJPEG streams from the
>  		 * chosen libcamera source stream.
>  		 */
> -		cameraStream.jpeg = new EncoderLibJpeg();
> -		int ret = cameraStream.jpeg->configure(cfg);
> +		Encoder *encoder = new EncoderLibJpeg();
> +		int ret = encoder->configure(cfg);
>  		if (ret) {
> -			LOG(HAL, Error)
> -				<< "Failed to configure encoder";
> +			LOG(HAL, Error) << "Failed to configure encoder";
> +			delete encoder;
>  			return ret;
>  		}
> +
> +		CameraStream &cameraStream = streams_.emplace_back(format, size,
> +								   index, encoder);
> +		stream->priv = static_cast<void *>(&cameraStream);
>  	}
>  
>  	switch (config_->validate()) {
> @@ -1481,7 +1482,7 @@ void CameraDevice::requestComplete(Request *request)
>  		if (cameraStream->format != formats::MJPEG)
>  			continue;
>  
> -		Encoder *encoder = cameraStream->jpeg;
> +		Encoder *encoder = cameraStream->encoder();
>  		if (!encoder) {
>  			LOG(HAL, Error) << "Failed to identify encoder";
>  			continue;
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index f8f237203ce9..3c57ffec265d 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -29,16 +29,16 @@ class CameraMetadata;
>  
>  struct CameraStream {
>  public:
> -	CameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i);
> +	CameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i,
> +		     Encoder *encoder = nullptr);
>  	~CameraStream();
>  
>  	unsigned int index() const { return index_; }
> +	Encoder *encoder() const { return encoder_; }
>  
>  	libcamera::PixelFormat format;
>  	libcamera::Size size;
>  
> -	Encoder *jpeg;
> -
>  private:
>  	/*
>  	 * The index of the libcamera StreamConfiguration as added during
> @@ -46,6 +46,7 @@ private:
>  	 * one or more streams to the Android framework.
>  	 */
>  	unsigned int index_;
> +	Encoder *encoder_;
>  };
>  
>  class CameraDevice : protected libcamera::Loggable

-- 
Regards,

Laurent Pinchart