[libcamera-devel] [PATCH] android: Modify PostProcessor interface

[Umang Jain]

Hi Hiro,

On 10/19/20 1:33 PM, Hirokazu Honda wrote:
> This modifies PostProcessor interface and polishes the code
> around the PostProcessor.
>
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
Changes looks good, albeit a few comments. Also, I think the patch could 
also be split down further to address the per Span pass-by-value,  per 
*-type reference to &-type, etc.
> ---
>   src/android/camera_stream.cpp            | 9 ++++-----
>   src/android/camera_stream.h              | 8 ++++----
>   src/android/jpeg/encoder.h               | 6 +++---
>   src/android/jpeg/encoder_libjpeg.cpp     | 6 +++---
>   src/android/jpeg/encoder_libjpeg.h       | 4 ++--
>   src/android/jpeg/post_processor_jpeg.cpp | 8 +++++---
>   src/android/jpeg/post_processor_jpeg.h   | 6 +++---
>   src/android/post_processor.h             | 4 ++--
>   8 files changed, 26 insertions(+), 25 deletions(-)
>
> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> index 1b8afa8..cc8691d 100644
> --- a/src/android/camera_stream.cpp
> +++ b/src/android/camera_stream.cpp
> @@ -40,11 +40,10 @@ LOG_DECLARE_CATEGORY(HAL);
>   
>   CameraStream::CameraStream(CameraDevice *cameraDevice, Type type,
>   			   camera3_stream_t *camera3Stream, unsigned int index)
> -	: cameraDevice_(cameraDevice), type_(type),
> +	: cameraDevice_(cameraDevice),
> +	  config_(cameraDevice_->cameraConfiguration()), type_(type),
Will cameraDevice_ will be initialized by this point? I am not sure.
>   	  camera3Stream_(camera3Stream), index_(index)
>   {
> -	config_ = cameraDevice_->cameraConfiguration();
> -
>   	if (type_ == Type::Internal || type_ == Type::Mapped) {
>   		/*
>   		 * \todo There might be multiple post-processors. The logic
> @@ -52,7 +51,7 @@ CameraStream::CameraStream(CameraDevice *cameraDevice, Type type,
>   		 * future. For now, we only have PostProcessorJpeg and that
>   		 * is what we instantiate here.
>   		 */
> -		postProcessor_ = std::make_unique<PostProcessorJpeg>(cameraDevice_);
> +		postProcessor_ = std::make_unique<PostProcessorJpeg>(cameraDevice);
What does this change achieve?
>   	}
>   
>   	if (type == Type::Internal) {
> @@ -102,7 +101,7 @@ int CameraStream::process(const libcamera::FrameBuffer &source,
>   	if (!postProcessor_)
>   		return 0;
>   
> -	return postProcessor_->process(&source, dest->maps()[0], metadata);
> +	return postProcessor_->process(source, dest->maps()[0], metadata);
>   }
>   
>   FrameBuffer *CameraStream::getBuffer()
> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> index c367a5f..24e66bb 100644
> --- a/src/android/camera_stream.h
> +++ b/src/android/camera_stream.h
> @@ -124,11 +124,11 @@ public:
>   	void putBuffer(libcamera::FrameBuffer *buffer);
>   
>   private:
> -	CameraDevice *cameraDevice_;
> -	libcamera::CameraConfiguration *config_;
> -	Type type_;
> +	CameraDevice const *cameraDevice_;
> +	const libcamera::CameraConfiguration *config_;
> +	const Type type_;
>   	camera3_stream_t *camera3Stream_;
> -	unsigned int index_;
> +	const unsigned int index_;
>   
>   	std::unique_ptr<libcamera::FrameBufferAllocator> allocator_;
>   	std::vector<libcamera::FrameBuffer *> buffers_;
> diff --git a/src/android/jpeg/encoder.h b/src/android/jpeg/encoder.h
> index 4483153..270ea60 100644
> --- a/src/android/jpeg/encoder.h
> +++ b/src/android/jpeg/encoder.h
> @@ -14,11 +14,11 @@
>   class Encoder
>   {
>   public:
> -	virtual ~Encoder() {};
> +	virtual ~Encoder() {}
>   
>   	virtual int configure(const libcamera::StreamConfiguration &cfg) = 0;
> -	virtual int encode(const libcamera::FrameBuffer *source,
> -			   const libcamera::Span<uint8_t> &destination,
> +	virtual int encode(const libcamera::FrameBuffer &source,
> +			   libcamera::Span<uint8_t> destination,
>   			   const libcamera::Span<const uint8_t> &exifData) = 0;
>   };
>   
> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp
> index 8995ba5..4236c9d 100644
> --- a/src/android/jpeg/encoder_libjpeg.cpp
> +++ b/src/android/jpeg/encoder_libjpeg.cpp
> @@ -179,11 +179,11 @@ void EncoderLibJpeg::compressNV(const libcamera::MappedBuffer *frame)
>   	}
>   }
>   
> -int EncoderLibJpeg::encode(const FrameBuffer *source,
> -			   const libcamera::Span<uint8_t> &dest,
> +int EncoderLibJpeg::encode(const FrameBuffer &source,
> +			   libcamera::Span<uint8_t> dest,
>   			   const libcamera::Span<const uint8_t> &exifData)
>   {
> -	MappedFrameBuffer frame(source, PROT_READ);
> +	MappedFrameBuffer frame(&source, PROT_READ);
>   	if (!frame.isValid()) {
>   		LOG(JPEG, Error) << "Failed to map FrameBuffer : "
>   				 << strerror(frame.error());
> diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h
> index 934caef..391a53c 100644
> --- a/src/android/jpeg/encoder_libjpeg.h
> +++ b/src/android/jpeg/encoder_libjpeg.h
> @@ -21,8 +21,8 @@ public:
>   	~EncoderLibJpeg();
>   
>   	int configure(const libcamera::StreamConfiguration &cfg) override;
> -	int encode(const libcamera::FrameBuffer *source,
> -		   const libcamera::Span<uint8_t> &destination,
> +	int encode(const libcamera::FrameBuffer &source,
> +		   libcamera::Span<uint8_t> destination,
>   		   const libcamera::Span<const uint8_t> &exifData) override;
>   
>   private:
> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> index 753c28e..4ded3e3 100644
> --- a/src/android/jpeg/post_processor_jpeg.cpp
> +++ b/src/android/jpeg/post_processor_jpeg.cpp
> @@ -44,8 +44,8 @@ int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,
>   	return encoder_->configure(inCfg);
>   }
>   
> -int PostProcessorJpeg::process(const libcamera::FrameBuffer *source,
> -			       const libcamera::Span<uint8_t> &destination,
> +int PostProcessorJpeg::process(const libcamera::FrameBuffer &source,
> +			       libcamera::Span<uint8_t> destination,
>   			       CameraMetadata *metadata)
>   {
>   	if (!encoder_)
> @@ -64,8 +64,10 @@ int PostProcessorJpeg::process(const libcamera::FrameBuffer *source,
>   	 * second, it is good enough.
>   	 */
>   	exif.setTimestamp(std::time(nullptr));
> -	if (exif.generate() != 0)
> +	if (exif.generate() != 0) {
>   		LOG(JPEG, Error) << "Failed to generate valid EXIF data";
> +		return -EINVAL;
> +	}
>   
Personally speaking, I don't think we should return FAIL here. So my 
preference is to skip this change.
>   	int jpeg_size = encoder_->encode(source, destination, exif.data());
>   	if (jpeg_size < 0) {
> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h
> index 62c8650..4d8edf5 100644
> --- a/src/android/jpeg/post_processor_jpeg.h
> +++ b/src/android/jpeg/post_processor_jpeg.h
> @@ -23,12 +23,12 @@ public:
>   
>   	int configure(const libcamera::StreamConfiguration &incfg,
>   		      const libcamera::StreamConfiguration &outcfg) override;
> -	int process(const libcamera::FrameBuffer *source,
> -		    const libcamera::Span<uint8_t> &destination,
> +	int process(const libcamera::FrameBuffer &source,
> +		    libcamera::Span<uint8_t> destination,
>   		    CameraMetadata *metadata) override;
>   
>   private:
> -	CameraDevice *cameraDevice_;
> +	CameraDevice const *cameraDevice_;
>   	std::unique_ptr<Encoder> encoder_;
>   	libcamera::Size streamSize_;
>   };
> diff --git a/src/android/post_processor.h b/src/android/post_processor.h
> index a891c43..5f87a5d 100644
> --- a/src/android/post_processor.h
> +++ b/src/android/post_processor.h
> @@ -20,8 +20,8 @@ public:
>   
>   	virtual int configure(const libcamera::StreamConfiguration &inCfg,
>   			      const libcamera::StreamConfiguration &outCfg) = 0;
> -	virtual int process(const libcamera::FrameBuffer *source,
> -			    const libcamera::Span<uint8_t> &destination,
> +	virtual int process(const libcamera::FrameBuffer &source,
> +			    libcamera::Span<uint8_t> destination,
>   			    CameraMetadata *metadata) = 0;
>   };
>