[libcamera-devel] [PATCH v3 1/3] android: camera_stream: Create post processor in configure()

[Laurent Pinchart]

Hi Hiro,

Thank you for the patch.

On Tue, Aug 31, 2021 at 06:34:37PM +0900, Hirokazu Honda wrote:
> CameraStream creates PostProcessor and FrameBufferAllocator in
> the constructor. CameraStream assumes that a used post processor
> is JPEG post processor. Since we need to support various post
> processors, we would rather move the creation to configure() so
> as to return an error code if no proper post processor is found.
> This also moves FrameBufferAllocator and Mutex creation for
> consistency.
> 
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/android/camera_device.h   |  1 +
>  src/android/camera_stream.cpp | 36 ++++++++++++++++++-----------------
>  2 files changed, 20 insertions(+), 17 deletions(-)
> 
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index a5576927..296c2f18 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -48,6 +48,7 @@ public:
>  
>  	unsigned int id() const { return id_; }
>  	camera3_device_t *camera3Device() { return &camera3Device_; }
> +	const CameraCapabilities *capabilities() const { return &capabilities_; }
>  	const std::shared_ptr<libcamera::Camera> &camera() const { return camera_; }
>  
>  	const std::string &maker() const { return maker_; }
> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> index 01909ec7..fb10bf06 100644
> --- a/src/android/camera_stream.cpp
> +++ b/src/android/camera_stream.cpp
> @@ -10,6 +10,7 @@
>  #include <sys/mman.h>
>  
>  #include "camera_buffer.h"
> +#include "camera_capabilities.h"
>  #include "camera_device.h"
>  #include "camera_metadata.h"
>  #include "jpeg/post_processor_jpeg.h"
> @@ -47,20 +48,6 @@ CameraStream::CameraStream(CameraDevice *const cameraDevice,
>  	: cameraDevice_(cameraDevice), config_(config), type_(type),
>  	  camera3Stream_(camera3Stream), index_(index)
>  {
> -	if (type_ == Type::Internal || type_ == Type::Mapped) {
> -		/*
> -		 * \todo There might be multiple post-processors. The logic
> -		 * which should be instantiated here, is deferred for the
> -		 * future. For now, we only have PostProcessorJpeg and that
> -		 * is what we instantiate here.
> -		 */
> -		postProcessor_ = std::make_unique<PostProcessorJpeg>(cameraDevice_);
> -	}
> -
> -	if (type == Type::Internal) {
> -		allocator_ = std::make_unique<FrameBufferAllocator>(cameraDevice_->camera());
> -		mutex_ = std::make_unique<std::mutex>();
> -	}
>  }
>  
>  const StreamConfiguration &CameraStream::configuration() const
> @@ -75,15 +62,30 @@ Stream *CameraStream::stream() const
>  
>  int CameraStream::configure()
>  {
> -	if (postProcessor_) {
> +	if (type_ == Type::Internal || type_ == Type::Mapped) {
> +		const PixelFormat outFormat =
> +			cameraDevice_->capabilities()->toPixelFormat(camera3Stream_->format);
>  		StreamConfiguration output = configuration();
> -		output.pixelFormat = formats::MJPEG;
> +		output.pixelFormat = outFormat;

I'd add a blank line here. Please let me know what you prefer, and I'll
handle it when pushing.

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

> +		switch (outFormat) {
> +		case formats::MJPEG:
> +			postProcessor_ = std::make_unique<PostProcessorJpeg>(cameraDevice_);
> +			break;
> +
> +		default:
> +			LOG(HAL, Error) << "Unsupported format: " << outFormat;
> +			return -EINVAL;
> +		}
> +
>  		int ret = postProcessor_->configure(configuration(), output);
>  		if (ret)
>  			return ret;
>  	}
>  
> -	if (allocator_) {
> +	if (type_ == Type::Internal) {
> +		allocator_ = std::make_unique<FrameBufferAllocator>(cameraDevice_->camera());
> +		mutex_ = std::make_unique<std::mutex>();
> +
>  		int ret = allocator_->allocate(stream());
>  		if (ret < 0)
>  			return ret;

-- 
Regards,

Laurent Pinchart