[libcamera-devel,RFC,2/3] android: jpeg: Port to PostProcessor interface

10023 diff mbox series

Remove the existing Encoder interface completely and use the
PostProcessor interface instead.

Now the ::encode() function will be called by PostProcessor::process()
internally and will not be directly exposed in CameraStream. Similarly,
other operations can be introduced internally in the PostProcessorJpeg,
and can be called through its process() interface.

Signed-off-by: Umang Jain <email@uajain.com>
---
 src/android/camera_device.h                   |  1 -
 src/android/camera_stream.cpp                 | 74 +++------------
 src/android/camera_stream.h                   |  9 +-
 src/android/jpeg/encoder.h                    | 25 -----
 ...er_libjpeg.cpp => post_processor_jpeg.cpp} | 92 +++++++++++++++++--
 ...ncoder_libjpeg.h => post_processor_jpeg.h} | 27 ++++--
 src/android/meson.build                       |  2 +-
 7 files changed, 122 insertions(+), 108 deletions(-)
 delete mode 100644 src/android/jpeg/encoder.h
 rename src/android/jpeg/{encoder_libjpeg.cpp => post_processor_jpeg.cpp} (67%)
 rename src/android/jpeg/{encoder_libjpeg.h => post_processor_jpeg.h} (55%)

Hi Umang,

Thank you for the patch.

On Thu, Oct 08, 2020 at 07:40:37PM +0530, Umang Jain wrote:
> Remove the existing Encoder interface completely and use the
> PostProcessor interface instead.

I think we need to keep the Encoder interface. We want to support other
JPEG encoders than the libjpeg-based implementation. Creating a JPEG
post-processor is the right thing to do, but it should still rely on the
existing encoder interface for the actual JPEG encoding. From a
CameraDevice point of view, only the PostProcessor interface will be
visible.

> Now the ::encode() function will be called by PostProcessor::process()
> internally and will not be directly exposed in CameraStream. Similarly,
> other operations can be introduced internally in the PostProcessorJpeg,
> and can be called through its process() interface.
> 
> Signed-off-by: Umang Jain <email@uajain.com>
> ---
>  src/android/camera_device.h                   |  1 -
>  src/android/camera_stream.cpp                 | 74 +++------------
>  src/android/camera_stream.h                   |  9 +-
>  src/android/jpeg/encoder.h                    | 25 -----
>  ...er_libjpeg.cpp => post_processor_jpeg.cpp} | 92 +++++++++++++++++--
>  ...ncoder_libjpeg.h => post_processor_jpeg.h} | 27 ++++--
>  src/android/meson.build                       |  2 +-
>  7 files changed, 122 insertions(+), 108 deletions(-)
>  delete mode 100644 src/android/jpeg/encoder.h
>  rename src/android/jpeg/{encoder_libjpeg.cpp => post_processor_jpeg.cpp} (67%)
>  rename src/android/jpeg/{encoder_libjpeg.h => post_processor_jpeg.h} (55%)
> 
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index 777d1a3..25de12e 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -25,7 +25,6 @@
>  #include "libcamera/internal/message.h"
>  
>  #include "camera_stream.h"
> -#include "jpeg/encoder.h"
>  
>  class CameraMetadata;
>  
> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> index eac1480..ed3bb41 100644
> --- a/src/android/camera_stream.cpp
> +++ b/src/android/camera_stream.cpp
> @@ -9,9 +9,7 @@
>  
>  #include "camera_device.h"
>  #include "camera_metadata.h"
> -#include "jpeg/encoder.h"
> -#include "jpeg/encoder_libjpeg.h"
> -#include "jpeg/exif.h"
> +#include "jpeg/post_processor_jpeg.h"
>  
>  using namespace libcamera;
>  
> @@ -24,8 +22,15 @@ CameraStream::CameraStream(CameraDevice *cameraDevice, Type type,
>  {
>  	config_ = cameraDevice_->cameraConfiguration();
>  
> -	if (type_ == Type::Internal || type_ == Type::Mapped)
> -		encoder_ = std::make_unique<EncoderLibJpeg>();
> +	if (type_ == Type::Internal || type_ == Type::Mapped) {
> +		/*
> +		 * \todo There might be multiple post-processors. The logic
> +		 * which should be instantiated here, is deferred for future.
> +		 * For now, we only have PostProcessJpeg and that is what we

s/PostProcessJpeg/PostProcessorJpeg/

> +		 * will instantiate here.
> +		 */
> +		postProcessor_ = std::make_unique<PostProcessorJpeg>();
> +	}
>  
>  	if (type == Type::Internal) {
>  		allocator_ = std::make_unique<FrameBufferAllocator>(cameraDevice_->camera());
> @@ -45,8 +50,8 @@ Stream *CameraStream::stream() const
>  
>  int CameraStream::configure()
>  {
> -	if (encoder_) {
> -		int ret = encoder_->configure(configuration());
> +	if (postProcessor_) {
> +		int ret = postProcessor_->configure(configuration());
>  		if (ret)
>  			return ret;
>  	}
> @@ -69,60 +74,11 @@ int CameraStream::configure()
>  int CameraStream::process(const libcamera::FrameBuffer &source,
>  			  MappedCamera3Buffer *dest, CameraMetadata *metadata)
>  {
> -	if (!encoder_)
> +	if (!postProcessor_)
>  		return 0;
>  
> -	/* Set EXIF metadata for various tags. */
> -	Exif exif;
> -	/* \todo Set Make and Model from external vendor tags. */
> -	exif.setMake("libcamera");
> -	exif.setModel("cameraModel");
> -	exif.setOrientation(cameraDevice_->orientation());
> -	exif.setSize(configuration().size);
> -	/*
> -	 * We set the frame's EXIF timestamp as the time of encode.
> -	 * Since the precision we need for EXIF timestamp is only one
> -	 * second, it is good enough.
> -	 */
> -	exif.setTimestamp(std::time(nullptr));
> -	if (exif.generate() != 0)
> -		LOG(HAL, Error) << "Failed to generate valid EXIF data";
> -
> -	int jpeg_size = encoder_->encode(&source, dest->maps()[0], exif.data());
> -	if (jpeg_size < 0) {
> -		LOG(HAL, Error) << "Failed to encode stream image";
> -		return jpeg_size;
> -	}
> -
> -	/*
> -	 * Fill in the JPEG blob header.
> -	 *
> -	 * The mapped size of the buffer is being returned as
> -	 * substantially larger than the requested JPEG_MAX_SIZE
> -	 * (which is referenced from maxJpegBufferSize_). Utilise
> -	 * this static size to ensure the correct offset of the blob is
> -	 * determined.
> -	 *
> -	 * \todo Investigate if the buffer size mismatch is an issue or
> -	 * expected behaviour.
> -	 */
> -	uint8_t *resultPtr = dest->maps()[0].data() +
> -			     cameraDevice_->maxJpegBufferSize() -
> -			     sizeof(struct camera3_jpeg_blob);
> -	auto *blob = reinterpret_cast<struct camera3_jpeg_blob *>(resultPtr);
> -	blob->jpeg_blob_id = CAMERA3_JPEG_BLOB_ID;
> -	blob->jpeg_size = jpeg_size;
> -
> -	/* Update the JPEG result Metadata. */
> -	metadata->addEntry(ANDROID_JPEG_SIZE, &jpeg_size, 1);
> -
> -	const uint32_t jpeg_quality = 95;
> -	metadata->addEntry(ANDROID_JPEG_QUALITY, &jpeg_quality, 1);
> -
> -	const uint32_t jpeg_orientation = 0;
> -	metadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpeg_orientation, 1);
> -
> -	return 0;
> +	return postProcessor_->process(&source, dest->maps()[0],
> +				       metadata, cameraDevice_);

There are at least two options to keep the interface generic, avoiding
variadic arguments. One of them is to explicitly pass the the camera
device to the process function. Another option is to pass it to the
PostProcessorJpeg constructor, and store it internally. I'd go for the
latter.

>  }
>  
>  FrameBuffer *CameraStream::getBuffer()
> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> index 8df0101..8d0f2e3 100644
> --- a/src/android/camera_stream.h
> +++ b/src/android/camera_stream.h
> @@ -19,7 +19,12 @@
>  #include <libcamera/geometry.h>
>  #include <libcamera/pixel_format.h>
>  
> -class Encoder;
> +/*
> + * \todo Ideally we want to replace this include with forward-declaration.
> + * If we do that. currently we get a compile error.
> + */

Let's fix them :-) What compilation error do you get ?

> +#include "post_processor.h"
> +
>  class CameraDevice;
>  class CameraMetadata;
>  class MappedCamera3Buffer;
> @@ -135,7 +140,7 @@ private:
>  	 */
>  	unsigned int index_;
>  
> -	std::unique_ptr<Encoder> encoder_;
> +	std::unique_ptr<PostProcessor> postProcessor_;
>  	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
> deleted file mode 100644
> index cf26d67..0000000
> --- a/src/android/jpeg/encoder.h
> +++ /dev/null
> @@ -1,25 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0-or-later */
> -/*
> - * Copyright (C) 2020, Google Inc.
> - *
> - * encoder.h - Image encoding interface
> - */
> -#ifndef __ANDROID_JPEG_ENCODER_H__
> -#define __ANDROID_JPEG_ENCODER_H__
> -
> -#include <libcamera/buffer.h>
> -#include <libcamera/span.h>
> -#include <libcamera/stream.h>
> -
> -class Encoder
> -{
> -public:
> -	virtual ~Encoder() {};
> -
> -	virtual int configure(const libcamera::StreamConfiguration &cfg) = 0;
> -	virtual int encode(const libcamera::FrameBuffer *source,
> -			   const libcamera::Span<uint8_t> &destination,
> -			   const libcamera::Span<const uint8_t> &exifData) = 0;
> -};
> -
> -#endif /* __ANDROID_JPEG_ENCODER_H__ */

As mentioned above, this should be kept, and used by the JPEG
post-processor.

> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> similarity index 67%
> rename from src/android/jpeg/encoder_libjpeg.cpp
> rename to src/android/jpeg/post_processor_jpeg.cpp
> index 510613c..eeb4e95 100644
> --- a/src/android/jpeg/encoder_libjpeg.cpp
> +++ b/src/android/jpeg/post_processor_jpeg.cpp
> @@ -2,10 +2,14 @@
>  /*
>   * Copyright (C) 2020, Google Inc.
>   *
> - * encoder_libjpeg.cpp - JPEG encoding using libjpeg native API
> + * post_processor_jpeg.cpp - JPEG Post Processor
>   */
>  
> -#include "encoder_libjpeg.h"
> +#include "post_processor_jpeg.h"
> +
> +#include "exif.h"
> +
> +#include "../camera_device.h"
>  
>  #include <fcntl.h>
>  #include <iomanip>
> @@ -25,6 +29,14 @@
>  
>  using namespace libcamera;
>  
> +#define extract_va_arg(type, arg, last) \
> +{                                       \
> +        va_list ap;                     \
> +        va_start(ap, last);             \
> +        arg = va_arg(ap, type);         \
> +        va_end(ap);                     \
> +}
> +
>  LOG_DEFINE_CATEGORY(JPEG)
>  
>  namespace {
> @@ -67,7 +79,7 @@ const struct JPEGPixelFormatInfo &findPixelInfo(const PixelFormat &format)
>  
>  } /* namespace */
>  
> -EncoderLibJpeg::EncoderLibJpeg()
> +PostProcessorJpeg::PostProcessorJpeg()
>  	: quality_(95)
>  {
>  	/* \todo Expand error handling coverage with a custom handler. */
> @@ -76,12 +88,12 @@ EncoderLibJpeg::EncoderLibJpeg()
>  	jpeg_create_compress(&compress_);
>  }
>  
> -EncoderLibJpeg::~EncoderLibJpeg()
> +PostProcessorJpeg::~PostProcessorJpeg()
>  {
>  	jpeg_destroy_compress(&compress_);
>  }
>  
> -int EncoderLibJpeg::configure(const StreamConfiguration &cfg)
> +int PostProcessorJpeg::configure(const StreamConfiguration &cfg)
>  {
>  	const struct JPEGPixelFormatInfo info = findPixelInfo(cfg.pixelFormat);
>  	if (info.colorSpace == JCS_UNKNOWN)
> @@ -104,7 +116,67 @@ int EncoderLibJpeg::configure(const StreamConfiguration &cfg)
>  	return 0;
>  }
>  
> -void EncoderLibJpeg::compressRGB(const libcamera::MappedBuffer *frame)
> +int PostProcessorJpeg::process(const libcamera::FrameBuffer *source,
> +			       const libcamera::Span<uint8_t> &destination,
> +			       CameraMetadata *metadata, ...)
> +{
> +	CameraDevice *device = nullptr;
> +	extract_va_arg(CameraDevice *, device, metadata);
> +
> +	/* Set EXIF metadata for various tags. */
> +	Exif exif;
> +	/* \todo Set Make and Model from external vendor tags. */
> +	exif.setMake("libcamera");
> +	exif.setModel("cameraModel");
> +	exif.setOrientation(device->orientation());
> +	exif.setSize(Size {compress_.image_width, compress_.image_height});
> +	/*
> +	 * We set the frame's EXIF timestamp as the time of encode.
> +	 * Since the precision we need for EXIF timestamp is only one
> +	 * second, it is good enough.
> +	 */
> +	exif.setTimestamp(std::time(nullptr));
> +	if (exif.generate() != 0)
> +		LOG(JPEG, Error) << "Failed to generate valid EXIF data";
> +
> +	int jpeg_size = encode(source, destination, exif.data());
> +	if (jpeg_size < 0) {
> +		LOG(JPEG, Error) << "Failed to encode stream image";
> +		return jpeg_size;
> +	}
> +
> +	/*
> +	 * Fill in the JPEG blob header.
> +	 *
> +	 * The mapped size of the buffer is being returned as
> +	 * substantially larger than the requested JPEG_MAX_SIZE
> +	 * (which is referenced from maxJpegBufferSize_). Utilise
> +	 * this static size to ensure the correct offset of the blob is
> +	 * determined.
> +	 *
> +	 * \todo Investigate if the buffer size mismatch is an issue or
> +	 * expected behaviour.
> +	 */
> +	uint8_t *resultPtr = destination.data() +
> +			     device->maxJpegBufferSize() -
> +			     sizeof(struct camera3_jpeg_blob);
> +	auto *blob = reinterpret_cast<struct camera3_jpeg_blob *>(resultPtr);
> +	blob->jpeg_blob_id = CAMERA3_JPEG_BLOB_ID;
> +	blob->jpeg_size = jpeg_size;
> +
> +	/* Update the JPEG result Metadata. */
> +	metadata->addEntry(ANDROID_JPEG_SIZE, &jpeg_size, 1);
> +
> +	const uint32_t jpeg_quality = 95;
> +	metadata->addEntry(ANDROID_JPEG_QUALITY, &jpeg_quality, 1);
> +
> +	const uint32_t jpeg_orientation = 0;
> +	metadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpeg_orientation, 1);
> +
> +	return 0;
> +}
> +
> +void PostProcessorJpeg::compressRGB(const libcamera::MappedBuffer *frame)
>  {
>  	unsigned char *src = static_cast<unsigned char *>(frame->maps()[0].data());
>  	/* \todo Stride information should come from buffer configuration. */
> @@ -122,7 +194,7 @@ void EncoderLibJpeg::compressRGB(const libcamera::MappedBuffer *frame)
>   * Compress the incoming buffer from a supported NV format.
>   * This naively unpacks the semi-planar NV12 to a YUV888 format for libjpeg.
>   */
> -void EncoderLibJpeg::compressNV(const libcamera::MappedBuffer *frame)
> +void PostProcessorJpeg::compressNV(const libcamera::MappedBuffer *frame)
>  {
>  	uint8_t tmprowbuf[compress_.image_width * 3];
>  
> @@ -179,9 +251,9 @@ void EncoderLibJpeg::compressNV(const libcamera::MappedBuffer *frame)
>  	}
>  }
>  
> -int EncoderLibJpeg::encode(const FrameBuffer *source,
> -			   const libcamera::Span<uint8_t> &dest,
> -			   const libcamera::Span<const uint8_t> &exifData)
> +int PostProcessorJpeg::encode(const FrameBuffer *source,
> +			      const libcamera::Span<uint8_t> &dest,
> +			      const libcamera::Span<const uint8_t> &exifData)
>  {
>  	MappedFrameBuffer frame(source, PROT_READ);
>  	if (!frame.isValid()) {
> diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/post_processor_jpeg.h
> similarity index 55%
> rename from src/android/jpeg/encoder_libjpeg.h
> rename to src/android/jpeg/post_processor_jpeg.h
> index 1e8df05..7f9ce70 100644
> --- a/src/android/jpeg/encoder_libjpeg.h
> +++ b/src/android/jpeg/post_processor_jpeg.h
> @@ -2,30 +2,37 @@
>  /*
>   * Copyright (C) 2020, Google Inc.
>   *
> - * encoder_libjpeg.h - JPEG encoding using libjpeg
> + * post_processor_jpeg.h - JPEG Post Processor
>   */
> -#ifndef __ANDROID_JPEG_ENCODER_LIBJPEG_H__
> -#define __ANDROID_JPEG_ENCODER_LIBJPEG_H__
> +#ifndef __ANDROID_POST_PROCESSOR_JPEG_H__
> +#define __ANDROID_POST_PROCESSOR_JPEG_H__
>  
> -#include "encoder.h"
> +#include "../post_processor.h"
> +#include "../camera_metadata.h"
>  
>  #include "libcamera/internal/buffer.h"
>  #include "libcamera/internal/formats.h"
>  
>  #include <jpeglib.h>
>  
> -class EncoderLibJpeg : public Encoder
> +class PostProcessorJpeg : public PostProcessor
>  {
>  public:
> -	EncoderLibJpeg();
> -	~EncoderLibJpeg();
> +	PostProcessorJpeg();
> +	~PostProcessorJpeg();
>  
>  	int configure(const libcamera::StreamConfiguration &cfg) override;
> +	int process(const libcamera::FrameBuffer *source,
> +		    const libcamera::Span<uint8_t> &destination,
> +		    CameraMetadata *metadata,
> +		    ...) override;
> +
> +
> +private:
>  	int encode(const libcamera::FrameBuffer *source,
>  		   const libcamera::Span<uint8_t> &destination,
> -		   const libcamera::Span<const uint8_t> &exifData) override;
> +		   const libcamera::Span<const uint8_t> &exifData);
>  
> -private:
>  	void compressRGB(const libcamera::MappedBuffer *frame);
>  	void compressNV(const libcamera::MappedBuffer *frame);
>  
> @@ -40,4 +47,4 @@ private:
>  	bool nvSwap_;
>  };
>  
> -#endif /* __ANDROID_JPEG_ENCODER_LIBJPEG_H__ */
> +#endif /* __ANDROID_POST_PROCESSOR_JPEG_H__ */
> diff --git a/src/android/meson.build b/src/android/meson.build
> index 802bb89..02b3b47 100644
> --- a/src/android/meson.build
> +++ b/src/android/meson.build
> @@ -21,8 +21,8 @@ android_hal_sources = files([
>      'camera_metadata.cpp',
>      'camera_ops.cpp',
>      'camera_stream.cpp',
> -    'jpeg/encoder_libjpeg.cpp',
>      'jpeg/exif.cpp',
> +    'jpeg/post_processor_jpeg.cpp',
>  ])
>  
>  android_camera_metadata_sources = files([

Hi Laurent,

Thanks for the review.

On 10/9/20 12:52 AM, Laurent Pinchart wrote:
> Hi Umang,
>
> Thank you for the patch.
>
> On Thu, Oct 08, 2020 at 07:40:37PM +0530, Umang Jain wrote:
>> Remove the existing Encoder interface completely and use the
>> PostProcessor interface instead.
> I think we need to keep the Encoder interface. We want to support other
> JPEG encoders than the libjpeg-based implementation. Creating a JPEG
> post-processor is the right thing to do, but it should still rely on the
> existing encoder interface for the actual JPEG encoding. From a
> CameraDevice point of view, only the PostProcessor interface will be
> visible.
Oh. I didn't see that. I assumed we will have only one? encoder and even 
if we had more, it would simply encapsulated within a private function 
as PostProcessorJpeg::encodeViaX() or whatever. I will bring back the 
Encoder interface if you say so.

Indeed, CameraDevice point-of-view, we only want to expose the 
PostProcessor interface.
>
>> Now the ::encode() function will be called by PostProcessor::process()
>> internally and will not be directly exposed in CameraStream. Similarly,
>> other operations can be introduced internally in the PostProcessorJpeg,
>> and can be called through its process() interface.
>>
>> Signed-off-by: Umang Jain <email@uajain.com>
>> ---
>>   src/android/camera_device.h                   |  1 -
>>   src/android/camera_stream.cpp                 | 74 +++------------
>>   src/android/camera_stream.h                   |  9 +-
>>   src/android/jpeg/encoder.h                    | 25 -----
>>   ...er_libjpeg.cpp => post_processor_jpeg.cpp} | 92 +++++++++++++++++--
>>   ...ncoder_libjpeg.h => post_processor_jpeg.h} | 27 ++++--
>>   src/android/meson.build                       |  2 +-
>>   7 files changed, 122 insertions(+), 108 deletions(-)
>>   delete mode 100644 src/android/jpeg/encoder.h
>>   rename src/android/jpeg/{encoder_libjpeg.cpp => post_processor_jpeg.cpp} (67%)
>>   rename src/android/jpeg/{encoder_libjpeg.h => post_processor_jpeg.h} (55%)
>>
>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
>> index 777d1a3..25de12e 100644
>> --- a/src/android/camera_device.h
>> +++ b/src/android/camera_device.h
>> @@ -25,7 +25,6 @@
>>   #include "libcamera/internal/message.h"
>>   
>>   #include "camera_stream.h"
>> -#include "jpeg/encoder.h"
>>   
>>   class CameraMetadata;
>>   
>> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
>> index eac1480..ed3bb41 100644
>> --- a/src/android/camera_stream.cpp
>> +++ b/src/android/camera_stream.cpp
>> @@ -9,9 +9,7 @@
>>   
>>   #include "camera_device.h"
>>   #include "camera_metadata.h"
>> -#include "jpeg/encoder.h"
>> -#include "jpeg/encoder_libjpeg.h"
>> -#include "jpeg/exif.h"
>> +#include "jpeg/post_processor_jpeg.h"
>>   
>>   using namespace libcamera;
>>   
>> @@ -24,8 +22,15 @@ CameraStream::CameraStream(CameraDevice *cameraDevice, Type type,
>>   {
>>   	config_ = cameraDevice_->cameraConfiguration();
>>   
>> -	if (type_ == Type::Internal || type_ == Type::Mapped)
>> -		encoder_ = std::make_unique<EncoderLibJpeg>();
>> +	if (type_ == Type::Internal || type_ == Type::Mapped) {
>> +		/*
>> +		 * \todo There might be multiple post-processors. The logic
>> +		 * which should be instantiated here, is deferred for future.
>> +		 * For now, we only have PostProcessJpeg and that is what we
> s/PostProcessJpeg/PostProcessorJpeg/
>
>> +		 * will instantiate here.
>> +		 */
>> +		postProcessor_ = std::make_unique<PostProcessorJpeg>();
>> +	}
>>   
>>   	if (type == Type::Internal) {
>>   		allocator_ = std::make_unique<FrameBufferAllocator>(cameraDevice_->camera());
>> @@ -45,8 +50,8 @@ Stream *CameraStream::stream() const
>>   
>>   int CameraStream::configure()
>>   {
>> -	if (encoder_) {
>> -		int ret = encoder_->configure(configuration());
>> +	if (postProcessor_) {
>> +		int ret = postProcessor_->configure(configuration());
>>   		if (ret)
>>   			return ret;
>>   	}
>> @@ -69,60 +74,11 @@ int CameraStream::configure()
>>   int CameraStream::process(const libcamera::FrameBuffer &source,
>>   			  MappedCamera3Buffer *dest, CameraMetadata *metadata)
>>   {
>> -	if (!encoder_)
>> +	if (!postProcessor_)
>>   		return 0;
>>   
>> -	/* Set EXIF metadata for various tags. */
>> -	Exif exif;
>> -	/* \todo Set Make and Model from external vendor tags. */
>> -	exif.setMake("libcamera");
>> -	exif.setModel("cameraModel");
>> -	exif.setOrientation(cameraDevice_->orientation());
>> -	exif.setSize(configuration().size);
>> -	/*
>> -	 * We set the frame's EXIF timestamp as the time of encode.
>> -	 * Since the precision we need for EXIF timestamp is only one
>> -	 * second, it is good enough.
>> -	 */
>> -	exif.setTimestamp(std::time(nullptr));
>> -	if (exif.generate() != 0)
>> -		LOG(HAL, Error) << "Failed to generate valid EXIF data";
>> -
>> -	int jpeg_size = encoder_->encode(&source, dest->maps()[0], exif.data());
>> -	if (jpeg_size < 0) {
>> -		LOG(HAL, Error) << "Failed to encode stream image";
>> -		return jpeg_size;
>> -	}
>> -
>> -	/*
>> -	 * Fill in the JPEG blob header.
>> -	 *
>> -	 * The mapped size of the buffer is being returned as
>> -	 * substantially larger than the requested JPEG_MAX_SIZE
>> -	 * (which is referenced from maxJpegBufferSize_). Utilise
>> -	 * this static size to ensure the correct offset of the blob is
>> -	 * determined.
>> -	 *
>> -	 * \todo Investigate if the buffer size mismatch is an issue or
>> -	 * expected behaviour.
>> -	 */
>> -	uint8_t *resultPtr = dest->maps()[0].data() +
>> -			     cameraDevice_->maxJpegBufferSize() -
>> -			     sizeof(struct camera3_jpeg_blob);
>> -	auto *blob = reinterpret_cast<struct camera3_jpeg_blob *>(resultPtr);
>> -	blob->jpeg_blob_id = CAMERA3_JPEG_BLOB_ID;
>> -	blob->jpeg_size = jpeg_size;
>> -
>> -	/* Update the JPEG result Metadata. */
>> -	metadata->addEntry(ANDROID_JPEG_SIZE, &jpeg_size, 1);
>> -
>> -	const uint32_t jpeg_quality = 95;
>> -	metadata->addEntry(ANDROID_JPEG_QUALITY, &jpeg_quality, 1);
>> -
>> -	const uint32_t jpeg_orientation = 0;
>> -	metadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpeg_orientation, 1);
>> -
>> -	return 0;
>> +	return postProcessor_->process(&source, dest->maps()[0],
>> +				       metadata, cameraDevice_);
> There are at least two options to keep the interface generic, avoiding
> variadic arguments. One of them is to explicitly pass the the camera
> device to the process function. Another option is to pass it to the
> PostProcessorJpeg constructor, and store it internally. I'd go for the
> latter.
I see. I thought vardiac arguments can be helpful for 
(future)PostProcessors to have the flexibility of parameters they might 
need when they process(). But I do get your point that they might defeat 
the entire purpose of keeping the interface generic. Asking for 
curiousity, is passing the specifics parameters via respective 
PostProcessors' constructors (instead of vardiac args) is the way you 
would suggest in general to achieve this goal?
>
>>   }
>>   
>>   FrameBuffer *CameraStream::getBuffer()
>> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
>> index 8df0101..8d0f2e3 100644
>> --- a/src/android/camera_stream.h
>> +++ b/src/android/camera_stream.h
>> @@ -19,7 +19,12 @@
>>   #include <libcamera/geometry.h>
>>   #include <libcamera/pixel_format.h>
>>   
>> -class Encoder;
>> +/*
>> + * \todo Ideally we want to replace this include with forward-declaration.
>> + * If we do that. currently we get a compile error.
>> + */
> Let's fix them :-) What compilation error do you get ?
>
>> +#include "post_processor.h"
>> +
>>   class CameraDevice;
>>   class CameraMetadata;
>>   class MappedCamera3Buffer;
>> @@ -135,7 +140,7 @@ private:
>>   	 */
>>   	unsigned int index_;
>>   
>> -	std::unique_ptr<Encoder> encoder_;
>> +	std::unique_ptr<PostProcessor> postProcessor_;
>>   	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
>> deleted file mode 100644
>> index cf26d67..0000000
>> --- a/src/android/jpeg/encoder.h
>> +++ /dev/null
>> @@ -1,25 +0,0 @@
>> -/* SPDX-License-Identifier: GPL-2.0-or-later */
>> -/*
>> - * Copyright (C) 2020, Google Inc.
>> - *
>> - * encoder.h - Image encoding interface
>> - */
>> -#ifndef __ANDROID_JPEG_ENCODER_H__
>> -#define __ANDROID_JPEG_ENCODER_H__
>> -
>> -#include <libcamera/buffer.h>
>> -#include <libcamera/span.h>
>> -#include <libcamera/stream.h>
>> -
>> -class Encoder
>> -{
>> -public:
>> -	virtual ~Encoder() {};
>> -
>> -	virtual int configure(const libcamera::StreamConfiguration &cfg) = 0;
>> -	virtual int encode(const libcamera::FrameBuffer *source,
>> -			   const libcamera::Span<uint8_t> &destination,
>> -			   const libcamera::Span<const uint8_t> &exifData) = 0;
>> -};
>> -
>> -#endif /* __ANDROID_JPEG_ENCODER_H__ */
> As mentioned above, this should be kept, and used by the JPEG
> post-processor.
>
>> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
>> similarity index 67%
>> rename from src/android/jpeg/encoder_libjpeg.cpp
>> rename to src/android/jpeg/post_processor_jpeg.cpp
>> index 510613c..eeb4e95 100644
>> --- a/src/android/jpeg/encoder_libjpeg.cpp
>> +++ b/src/android/jpeg/post_processor_jpeg.cpp
>> @@ -2,10 +2,14 @@
>>   /*
>>    * Copyright (C) 2020, Google Inc.
>>    *
>> - * encoder_libjpeg.cpp - JPEG encoding using libjpeg native API
>> + * post_processor_jpeg.cpp - JPEG Post Processor
>>    */
>>   
>> -#include "encoder_libjpeg.h"
>> +#include "post_processor_jpeg.h"
>> +
>> +#include "exif.h"
>> +
>> +#include "../camera_device.h"
>>   
>>   #include <fcntl.h>
>>   #include <iomanip>
>> @@ -25,6 +29,14 @@
>>   
>>   using namespace libcamera;
>>   
>> +#define extract_va_arg(type, arg, last) \
>> +{                                       \
>> +        va_list ap;                     \
>> +        va_start(ap, last);             \
>> +        arg = va_arg(ap, type);         \
>> +        va_end(ap);                     \
>> +}
>> +
>>   LOG_DEFINE_CATEGORY(JPEG)
>>   
>>   namespace {
>> @@ -67,7 +79,7 @@ const struct JPEGPixelFormatInfo &findPixelInfo(const PixelFormat &format)
>>   
>>   } /* namespace */
>>   
>> -EncoderLibJpeg::EncoderLibJpeg()
>> +PostProcessorJpeg::PostProcessorJpeg()
>>   	: quality_(95)
>>   {
>>   	/* \todo Expand error handling coverage with a custom handler. */
>> @@ -76,12 +88,12 @@ EncoderLibJpeg::EncoderLibJpeg()
>>   	jpeg_create_compress(&compress_);
>>   }
>>   
>> -EncoderLibJpeg::~EncoderLibJpeg()
>> +PostProcessorJpeg::~PostProcessorJpeg()
>>   {
>>   	jpeg_destroy_compress(&compress_);
>>   }
>>   
>> -int EncoderLibJpeg::configure(const StreamConfiguration &cfg)
>> +int PostProcessorJpeg::configure(const StreamConfiguration &cfg)
>>   {
>>   	const struct JPEGPixelFormatInfo info = findPixelInfo(cfg.pixelFormat);
>>   	if (info.colorSpace == JCS_UNKNOWN)
>> @@ -104,7 +116,67 @@ int EncoderLibJpeg::configure(const StreamConfiguration &cfg)
>>   	return 0;
>>   }
>>   
>> -void EncoderLibJpeg::compressRGB(const libcamera::MappedBuffer *frame)
>> +int PostProcessorJpeg::process(const libcamera::FrameBuffer *source,
>> +			       const libcamera::Span<uint8_t> &destination,
>> +			       CameraMetadata *metadata, ...)
>> +{
>> +	CameraDevice *device = nullptr;
>> +	extract_va_arg(CameraDevice *, device, metadata);
>> +
>> +	/* Set EXIF metadata for various tags. */
>> +	Exif exif;
>> +	/* \todo Set Make and Model from external vendor tags. */
>> +	exif.setMake("libcamera");
>> +	exif.setModel("cameraModel");
>> +	exif.setOrientation(device->orientation());
>> +	exif.setSize(Size {compress_.image_width, compress_.image_height});
>> +	/*
>> +	 * We set the frame's EXIF timestamp as the time of encode.
>> +	 * Since the precision we need for EXIF timestamp is only one
>> +	 * second, it is good enough.
>> +	 */
>> +	exif.setTimestamp(std::time(nullptr));
>> +	if (exif.generate() != 0)
>> +		LOG(JPEG, Error) << "Failed to generate valid EXIF data";
>> +
>> +	int jpeg_size = encode(source, destination, exif.data());
>> +	if (jpeg_size < 0) {
>> +		LOG(JPEG, Error) << "Failed to encode stream image";
>> +		return jpeg_size;
>> +	}
>> +
>> +	/*
>> +	 * Fill in the JPEG blob header.
>> +	 *
>> +	 * The mapped size of the buffer is being returned as
>> +	 * substantially larger than the requested JPEG_MAX_SIZE
>> +	 * (which is referenced from maxJpegBufferSize_). Utilise
>> +	 * this static size to ensure the correct offset of the blob is
>> +	 * determined.
>> +	 *
>> +	 * \todo Investigate if the buffer size mismatch is an issue or
>> +	 * expected behaviour.
>> +	 */
>> +	uint8_t *resultPtr = destination.data() +
>> +			     device->maxJpegBufferSize() -
>> +			     sizeof(struct camera3_jpeg_blob);
>> +	auto *blob = reinterpret_cast<struct camera3_jpeg_blob *>(resultPtr);
>> +	blob->jpeg_blob_id = CAMERA3_JPEG_BLOB_ID;
>> +	blob->jpeg_size = jpeg_size;
>> +
>> +	/* Update the JPEG result Metadata. */
>> +	metadata->addEntry(ANDROID_JPEG_SIZE, &jpeg_size, 1);
>> +
>> +	const uint32_t jpeg_quality = 95;
>> +	metadata->addEntry(ANDROID_JPEG_QUALITY, &jpeg_quality, 1);
>> +
>> +	const uint32_t jpeg_orientation = 0;
>> +	metadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpeg_orientation, 1);
>> +
>> +	return 0;
>> +}
>> +
>> +void PostProcessorJpeg::compressRGB(const libcamera::MappedBuffer *frame)
>>   {
>>   	unsigned char *src = static_cast<unsigned char *>(frame->maps()[0].data());
>>   	/* \todo Stride information should come from buffer configuration. */
>> @@ -122,7 +194,7 @@ void EncoderLibJpeg::compressRGB(const libcamera::MappedBuffer *frame)
>>    * Compress the incoming buffer from a supported NV format.
>>    * This naively unpacks the semi-planar NV12 to a YUV888 format for libjpeg.
>>    */
>> -void EncoderLibJpeg::compressNV(const libcamera::MappedBuffer *frame)
>> +void PostProcessorJpeg::compressNV(const libcamera::MappedBuffer *frame)
>>   {
>>   	uint8_t tmprowbuf[compress_.image_width * 3];
>>   
>> @@ -179,9 +251,9 @@ void EncoderLibJpeg::compressNV(const libcamera::MappedBuffer *frame)
>>   	}
>>   }
>>   
>> -int EncoderLibJpeg::encode(const FrameBuffer *source,
>> -			   const libcamera::Span<uint8_t> &dest,
>> -			   const libcamera::Span<const uint8_t> &exifData)
>> +int PostProcessorJpeg::encode(const FrameBuffer *source,
>> +			      const libcamera::Span<uint8_t> &dest,
>> +			      const libcamera::Span<const uint8_t> &exifData)
>>   {
>>   	MappedFrameBuffer frame(source, PROT_READ);
>>   	if (!frame.isValid()) {
>> diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/post_processor_jpeg.h
>> similarity index 55%
>> rename from src/android/jpeg/encoder_libjpeg.h
>> rename to src/android/jpeg/post_processor_jpeg.h
>> index 1e8df05..7f9ce70 100644
>> --- a/src/android/jpeg/encoder_libjpeg.h
>> +++ b/src/android/jpeg/post_processor_jpeg.h
>> @@ -2,30 +2,37 @@
>>   /*
>>    * Copyright (C) 2020, Google Inc.
>>    *
>> - * encoder_libjpeg.h - JPEG encoding using libjpeg
>> + * post_processor_jpeg.h - JPEG Post Processor
>>    */
>> -#ifndef __ANDROID_JPEG_ENCODER_LIBJPEG_H__
>> -#define __ANDROID_JPEG_ENCODER_LIBJPEG_H__
>> +#ifndef __ANDROID_POST_PROCESSOR_JPEG_H__
>> +#define __ANDROID_POST_PROCESSOR_JPEG_H__
>>   
>> -#include "encoder.h"
>> +#include "../post_processor.h"
>> +#include "../camera_metadata.h"
>>   
>>   #include "libcamera/internal/buffer.h"
>>   #include "libcamera/internal/formats.h"
>>   
>>   #include <jpeglib.h>
>>   
>> -class EncoderLibJpeg : public Encoder
>> +class PostProcessorJpeg : public PostProcessor
>>   {
>>   public:
>> -	EncoderLibJpeg();
>> -	~EncoderLibJpeg();
>> +	PostProcessorJpeg();
>> +	~PostProcessorJpeg();
>>   
>>   	int configure(const libcamera::StreamConfiguration &cfg) override;
>> +	int process(const libcamera::FrameBuffer *source,
>> +		    const libcamera::Span<uint8_t> &destination,
>> +		    CameraMetadata *metadata,
>> +		    ...) override;
>> +
>> +
>> +private:
>>   	int encode(const libcamera::FrameBuffer *source,
>>   		   const libcamera::Span<uint8_t> &destination,
>> -		   const libcamera::Span<const uint8_t> &exifData) override;
>> +		   const libcamera::Span<const uint8_t> &exifData);
>>   
>> -private:
>>   	void compressRGB(const libcamera::MappedBuffer *frame);
>>   	void compressNV(const libcamera::MappedBuffer *frame);
>>   
>> @@ -40,4 +47,4 @@ private:
>>   	bool nvSwap_;
>>   };
>>   
>> -#endif /* __ANDROID_JPEG_ENCODER_LIBJPEG_H__ */
>> +#endif /* __ANDROID_POST_PROCESSOR_JPEG_H__ */
>> diff --git a/src/android/meson.build b/src/android/meson.build
>> index 802bb89..02b3b47 100644
>> --- a/src/android/meson.build
>> +++ b/src/android/meson.build
>> @@ -21,8 +21,8 @@ android_hal_sources = files([
>>       'camera_metadata.cpp',
>>       'camera_ops.cpp',
>>       'camera_stream.cpp',
>> -    'jpeg/encoder_libjpeg.cpp',
>>       'jpeg/exif.cpp',
>> +    'jpeg/post_processor_jpeg.cpp',
>>   ])
>>   
>>   android_camera_metadata_sources = files([

Hi Laurent,

Forgot to follow up at one more point.

On 10/9/20 12:52 AM, Laurent Pinchart wrote:
> Hi Umang,
>
> Thank you for the patch.
>
> On Thu, Oct 08, 2020 at 07:40:37PM +0530, Umang Jain wrote:
>> Remove the existing Encoder interface completely and use the
>> PostProcessor interface instead.
> I think we need to keep the Encoder interface. We want to support other
> JPEG encoders than the libjpeg-based implementation. Creating a JPEG
> post-processor is the right thing to do, but it should still rely on the
> existing encoder interface for the actual JPEG encoding. From a
> CameraDevice point of view, only the PostProcessor interface will be
> visible.
>
>> Now the ::encode() function will be called by PostProcessor::process()
>> internally and will not be directly exposed in CameraStream. Similarly,
>> other operations can be introduced internally in the PostProcessorJpeg,
>> and can be called through its process() interface.
>>
>> Signed-off-by: Umang Jain <email@uajain.com>
>> ---
>>   src/android/camera_device.h                   |  1 -
>>   src/android/camera_stream.cpp                 | 74 +++------------
>>   src/android/camera_stream.h                   |  9 +-
>>   src/android/jpeg/encoder.h                    | 25 -----
>>   ...er_libjpeg.cpp => post_processor_jpeg.cpp} | 92 +++++++++++++++++--
>>   ...ncoder_libjpeg.h => post_processor_jpeg.h} | 27 ++++--
>>   src/android/meson.build                       |  2 +-
>>   7 files changed, 122 insertions(+), 108 deletions(-)
>>   delete mode 100644 src/android/jpeg/encoder.h
>>   rename src/android/jpeg/{encoder_libjpeg.cpp => post_processor_jpeg.cpp} (67%)
>>   rename src/android/jpeg/{encoder_libjpeg.h => post_processor_jpeg.h} (55%)
>>
>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
>> index 777d1a3..25de12e 100644
>> --- a/src/android/camera_device.h
>> +++ b/src/android/camera_device.h
>> @@ -25,7 +25,6 @@
>>   #include "libcamera/internal/message.h"
>>   
>>   #include "camera_stream.h"
>> -#include "jpeg/encoder.h"
>>   
>>   class CameraMetadata;
>>   
>> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
>> index eac1480..ed3bb41 100644
>> --- a/src/android/camera_stream.cpp
>> +++ b/src/android/camera_stream.cpp
>> @@ -9,9 +9,7 @@
>>   
>>   #include "camera_device.h"
>>   #include "camera_metadata.h"
>> -#include "jpeg/encoder.h"
>> -#include "jpeg/encoder_libjpeg.h"
>> -#include "jpeg/exif.h"
>> +#include "jpeg/post_processor_jpeg.h"
>>   
>>   using namespace libcamera;
>>   
>> @@ -24,8 +22,15 @@ CameraStream::CameraStream(CameraDevice *cameraDevice, Type type,
>>   {
>>   	config_ = cameraDevice_->cameraConfiguration();
>>   
>> -	if (type_ == Type::Internal || type_ == Type::Mapped)
>> -		encoder_ = std::make_unique<EncoderLibJpeg>();
>> +	if (type_ == Type::Internal || type_ == Type::Mapped) {
>> +		/*
>> +		 * \todo There might be multiple post-processors. The logic
>> +		 * which should be instantiated here, is deferred for future.
>> +		 * For now, we only have PostProcessJpeg and that is what we
> s/PostProcessJpeg/PostProcessorJpeg/
>
>> +		 * will instantiate here.
>> +		 */
>> +		postProcessor_ = std::make_unique<PostProcessorJpeg>();
>> +	}
>>   
>>   	if (type == Type::Internal) {
>>   		allocator_ = std::make_unique<FrameBufferAllocator>(cameraDevice_->camera());
>> @@ -45,8 +50,8 @@ Stream *CameraStream::stream() const
>>   
>>   int CameraStream::configure()
>>   {
>> -	if (encoder_) {
>> -		int ret = encoder_->configure(configuration());
>> +	if (postProcessor_) {
>> +		int ret = postProcessor_->configure(configuration());
>>   		if (ret)
>>   			return ret;
>>   	}
>> @@ -69,60 +74,11 @@ int CameraStream::configure()
>>   int CameraStream::process(const libcamera::FrameBuffer &source,
>>   			  MappedCamera3Buffer *dest, CameraMetadata *metadata)
>>   {
>> -	if (!encoder_)
>> +	if (!postProcessor_)
>>   		return 0;
>>   
>> -	/* Set EXIF metadata for various tags. */
>> -	Exif exif;
>> -	/* \todo Set Make and Model from external vendor tags. */
>> -	exif.setMake("libcamera");
>> -	exif.setModel("cameraModel");
>> -	exif.setOrientation(cameraDevice_->orientation());
>> -	exif.setSize(configuration().size);
>> -	/*
>> -	 * We set the frame's EXIF timestamp as the time of encode.
>> -	 * Since the precision we need for EXIF timestamp is only one
>> -	 * second, it is good enough.
>> -	 */
>> -	exif.setTimestamp(std::time(nullptr));
>> -	if (exif.generate() != 0)
>> -		LOG(HAL, Error) << "Failed to generate valid EXIF data";
>> -
>> -	int jpeg_size = encoder_->encode(&source, dest->maps()[0], exif.data());
>> -	if (jpeg_size < 0) {
>> -		LOG(HAL, Error) << "Failed to encode stream image";
>> -		return jpeg_size;
>> -	}
>> -
>> -	/*
>> -	 * Fill in the JPEG blob header.
>> -	 *
>> -	 * The mapped size of the buffer is being returned as
>> -	 * substantially larger than the requested JPEG_MAX_SIZE
>> -	 * (which is referenced from maxJpegBufferSize_). Utilise
>> -	 * this static size to ensure the correct offset of the blob is
>> -	 * determined.
>> -	 *
>> -	 * \todo Investigate if the buffer size mismatch is an issue or
>> -	 * expected behaviour.
>> -	 */
>> -	uint8_t *resultPtr = dest->maps()[0].data() +
>> -			     cameraDevice_->maxJpegBufferSize() -
>> -			     sizeof(struct camera3_jpeg_blob);
>> -	auto *blob = reinterpret_cast<struct camera3_jpeg_blob *>(resultPtr);
>> -	blob->jpeg_blob_id = CAMERA3_JPEG_BLOB_ID;
>> -	blob->jpeg_size = jpeg_size;
>> -
>> -	/* Update the JPEG result Metadata. */
>> -	metadata->addEntry(ANDROID_JPEG_SIZE, &jpeg_size, 1);
>> -
>> -	const uint32_t jpeg_quality = 95;
>> -	metadata->addEntry(ANDROID_JPEG_QUALITY, &jpeg_quality, 1);
>> -
>> -	const uint32_t jpeg_orientation = 0;
>> -	metadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpeg_orientation, 1);
>> -
>> -	return 0;
>> +	return postProcessor_->process(&source, dest->maps()[0],
>> +				       metadata, cameraDevice_);
> There are at least two options to keep the interface generic, avoiding
> variadic arguments. One of them is to explicitly pass the the camera
> device to the process function. Another option is to pass it to the
> PostProcessorJpeg constructor, and store it internally. I'd go for the
> latter.
>
>>   }
>>   
>>   FrameBuffer *CameraStream::getBuffer()
>> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
>> index 8df0101..8d0f2e3 100644
>> --- a/src/android/camera_stream.h
>> +++ b/src/android/camera_stream.h
>> @@ -19,7 +19,12 @@
>>   #include <libcamera/geometry.h>
>>   #include <libcamera/pixel_format.h>
>>   
>> -class Encoder;
>> +/*
>> + * \todo Ideally we want to replace this include with forward-declaration.
>> + * If we do that. currently we get a compile error.
>> + */
> Let's fix them :-) What compilation error do you get ?
The error I see:

[89/298] Compiling C++ object 
src/libcamera/libcamera.so.p/.._android_camera_device.cpp.o
FAILED: src/libcamera/libcamera.so.p/.._android_camera_device.cpp.o
c++ -Isrc/libcamera/libcamera.so.p -Isrc/libcamera -I../src/libcamera 
-Iinclude -I../include -I../include/android/hardware/libhardware/include 
-I../include/android/metadata -I../include/android/system/core/include 
-Iinclude/libcamera -fdiagnostics-color=always -pipe 
-D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wnon-virtual-dtor -Wextra 
-Werror -std=c++17 -g -include config.h -fPIC -pthread -MD -MQ 
src/libcamera/libcamera.so.p/.._android_camera_device.cpp.o -MF 
src/libcamera/libcamera.so.p/.._android_camera_device.cpp.o.d -o 
src/libcamera/libcamera.so.p/.._android_camera_device.cpp.o -c 
../src/android/camera_device.cpp
In file included from /usr/include/c++/10/memory:83,
                  from ../src/android/camera_device.h:11,
                  from ../src/android/camera_device.cpp:8:
/usr/include/c++/10/bits/unique_ptr.h: In instantiation of ‘void 
std::default_delete<_Tp>::operator()(_Tp*) const [with _Tp = 
PostProcessor]’:
/usr/include/c++/10/bits/unique_ptr.h:360:17:   required from 
‘std::unique_ptr<_Tp, _Dp>::~unique_ptr() [with _Tp = PostProcessor; _Dp 
= std::default_delete<PostProcessor>]’
../src/android/camera_stream.h:32:7:   required from ‘void 
__gnu_cxx::new_allocator<_Tp>::destroy(_Up*) [with _Up = CameraStream; 
_Tp = CameraStream]’
/usr/include/c++/10/bits/alloc_traits.h:531:15:   required from ‘static 
void std::allocator_traits<std::allocator<_Tp1> 
 >::destroy(std::allocator_traits<std::allocator<_Tp1> 
 >::allocator_type&, _Up*) [with _Up = CameraStream; _Tp = CameraStream; 
std::allocator_traits<std::allocator<_Tp1> >::allocator_type = 
std::allocator<CameraStream>]’
/usr/include/c++/10/bits/vector.tcc:488:28:   required from ‘void 
std::vector<_Tp, _Alloc>::_M_realloc_insert(std::vector<_Tp, 
_Alloc>::iterator, _Args&& ...) [with _Args = {CameraDevice*, 
CameraStream::Type, camera3_stream*&, long unsigned int}; _Tp = 
CameraStream; _Alloc = std::allocator<CameraStream>; std::vector<_Tp, 
_Alloc>::iterator = std::vector<CameraStream>::iterator]’
/usr/include/c++/10/bits/vector.tcc:121:21:   required from 
‘std::vector<_Tp, _Alloc>::reference std::vector<_Tp, 
_Alloc>::emplace_back(_Args&& ...) [with _Args = {CameraDevice*, 
CameraStream::Type, camera3_stream*&, long unsigned int}; _Tp = 
CameraStream; _Alloc = std::allocator<CameraStream>; std::vector<_Tp, 
_Alloc>::reference = CameraStream&]’
../src/android/camera_device.cpp:1212:38:   required from here
/usr/include/c++/10/bits/unique_ptr.h:82:16: error: invalid application 
of ‘sizeof’ to incomplete type ‘PostProcessor’
    82 |  static_assert(sizeof(_Tp)>0,
       |                ^~~~~~~~~~~
[98/298] Compiling C++ object 
src/libcamera/libcamera.so.p/.._android_camera_stream.cpp.o
ninja: build stopped: subcommand failed.

Not sure why it cannot work with the incomplete type `PostProcessor` 
interface
(well, it used to work well with Encoder's interface, right there!)
>
>> +#include "post_processor.h"
>> +
>>   class CameraDevice;
>>   class CameraMetadata;
>>   class MappedCamera3Buffer;
>> @@ -135,7 +140,7 @@ private:
>>   	 */
>>   	unsigned int index_;
>>   
>> -	std::unique_ptr<Encoder> encoder_;
>> +	std::unique_ptr<PostProcessor> postProcessor_;
>>   	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
>> deleted file mode 100644
>> index cf26d67..0000000
>> --- a/src/android/jpeg/encoder.h
>> +++ /dev/null
>> @@ -1,25 +0,0 @@
>> -/* SPDX-License-Identifier: GPL-2.0-or-later */
>> -/*
>> - * Copyright (C) 2020, Google Inc.
>> - *
>> - * encoder.h - Image encoding interface
>> - */
>> -#ifndef __ANDROID_JPEG_ENCODER_H__
>> -#define __ANDROID_JPEG_ENCODER_H__
>> -
>> -#include <libcamera/buffer.h>
>> -#include <libcamera/span.h>
>> -#include <libcamera/stream.h>
>> -
>> -class Encoder
>> -{
>> -public:
>> -	virtual ~Encoder() {};
>> -
>> -	virtual int configure(const libcamera::StreamConfiguration &cfg) = 0;
>> -	virtual int encode(const libcamera::FrameBuffer *source,
>> -			   const libcamera::Span<uint8_t> &destination,
>> -			   const libcamera::Span<const uint8_t> &exifData) = 0;
>> -};
>> -
>> -#endif /* __ANDROID_JPEG_ENCODER_H__ */
> As mentioned above, this should be kept, and used by the JPEG
> post-processor.
>
>> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
>> similarity index 67%
>> rename from src/android/jpeg/encoder_libjpeg.cpp
>> rename to src/android/jpeg/post_processor_jpeg.cpp
>> index 510613c..eeb4e95 100644
>> --- a/src/android/jpeg/encoder_libjpeg.cpp
>> +++ b/src/android/jpeg/post_processor_jpeg.cpp
>> @@ -2,10 +2,14 @@
>>   /*
>>    * Copyright (C) 2020, Google Inc.
>>    *
>> - * encoder_libjpeg.cpp - JPEG encoding using libjpeg native API
>> + * post_processor_jpeg.cpp - JPEG Post Processor
>>    */
>>   
>> -#include "encoder_libjpeg.h"
>> +#include "post_processor_jpeg.h"
>> +
>> +#include "exif.h"
>> +
>> +#include "../camera_device.h"
>>   
>>   #include <fcntl.h>
>>   #include <iomanip>
>> @@ -25,6 +29,14 @@
>>   
>>   using namespace libcamera;
>>   
>> +#define extract_va_arg(type, arg, last) \
>> +{                                       \
>> +        va_list ap;                     \
>> +        va_start(ap, last);             \
>> +        arg = va_arg(ap, type);         \
>> +        va_end(ap);                     \
>> +}
>> +
>>   LOG_DEFINE_CATEGORY(JPEG)
>>   
>>   namespace {
>> @@ -67,7 +79,7 @@ const struct JPEGPixelFormatInfo &findPixelInfo(const PixelFormat &format)
>>   
>>   } /* namespace */
>>   
>> -EncoderLibJpeg::EncoderLibJpeg()
>> +PostProcessorJpeg::PostProcessorJpeg()
>>   	: quality_(95)
>>   {
>>   	/* \todo Expand error handling coverage with a custom handler. */
>> @@ -76,12 +88,12 @@ EncoderLibJpeg::EncoderLibJpeg()
>>   	jpeg_create_compress(&compress_);
>>   }
>>   
>> -EncoderLibJpeg::~EncoderLibJpeg()
>> +PostProcessorJpeg::~PostProcessorJpeg()
>>   {
>>   	jpeg_destroy_compress(&compress_);
>>   }
>>   
>> -int EncoderLibJpeg::configure(const StreamConfiguration &cfg)
>> +int PostProcessorJpeg::configure(const StreamConfiguration &cfg)
>>   {
>>   	const struct JPEGPixelFormatInfo info = findPixelInfo(cfg.pixelFormat);
>>   	if (info.colorSpace == JCS_UNKNOWN)
>> @@ -104,7 +116,67 @@ int EncoderLibJpeg::configure(const StreamConfiguration &cfg)
>>   	return 0;
>>   }
>>   
>> -void EncoderLibJpeg::compressRGB(const libcamera::MappedBuffer *frame)
>> +int PostProcessorJpeg::process(const libcamera::FrameBuffer *source,
>> +			       const libcamera::Span<uint8_t> &destination,
>> +			       CameraMetadata *metadata, ...)
>> +{
>> +	CameraDevice *device = nullptr;
>> +	extract_va_arg(CameraDevice *, device, metadata);
>> +
>> +	/* Set EXIF metadata for various tags. */
>> +	Exif exif;
>> +	/* \todo Set Make and Model from external vendor tags. */
>> +	exif.setMake("libcamera");
>> +	exif.setModel("cameraModel");
>> +	exif.setOrientation(device->orientation());
>> +	exif.setSize(Size {compress_.image_width, compress_.image_height});
>> +	/*
>> +	 * We set the frame's EXIF timestamp as the time of encode.
>> +	 * Since the precision we need for EXIF timestamp is only one
>> +	 * second, it is good enough.
>> +	 */
>> +	exif.setTimestamp(std::time(nullptr));
>> +	if (exif.generate() != 0)
>> +		LOG(JPEG, Error) << "Failed to generate valid EXIF data";
>> +
>> +	int jpeg_size = encode(source, destination, exif.data());
>> +	if (jpeg_size < 0) {
>> +		LOG(JPEG, Error) << "Failed to encode stream image";
>> +		return jpeg_size;
>> +	}
>> +
>> +	/*
>> +	 * Fill in the JPEG blob header.
>> +	 *
>> +	 * The mapped size of the buffer is being returned as
>> +	 * substantially larger than the requested JPEG_MAX_SIZE
>> +	 * (which is referenced from maxJpegBufferSize_). Utilise
>> +	 * this static size to ensure the correct offset of the blob is
>> +	 * determined.
>> +	 *
>> +	 * \todo Investigate if the buffer size mismatch is an issue or
>> +	 * expected behaviour.
>> +	 */
>> +	uint8_t *resultPtr = destination.data() +
>> +			     device->maxJpegBufferSize() -
>> +			     sizeof(struct camera3_jpeg_blob);
>> +	auto *blob = reinterpret_cast<struct camera3_jpeg_blob *>(resultPtr);
>> +	blob->jpeg_blob_id = CAMERA3_JPEG_BLOB_ID;
>> +	blob->jpeg_size = jpeg_size;
>> +
>> +	/* Update the JPEG result Metadata. */
>> +	metadata->addEntry(ANDROID_JPEG_SIZE, &jpeg_size, 1);
>> +
>> +	const uint32_t jpeg_quality = 95;
>> +	metadata->addEntry(ANDROID_JPEG_QUALITY, &jpeg_quality, 1);
>> +
>> +	const uint32_t jpeg_orientation = 0;
>> +	metadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpeg_orientation, 1);
>> +
>> +	return 0;
>> +}
>> +
>> +void PostProcessorJpeg::compressRGB(const libcamera::MappedBuffer *frame)
>>   {
>>   	unsigned char *src = static_cast<unsigned char *>(frame->maps()[0].data());
>>   	/* \todo Stride information should come from buffer configuration. */
>> @@ -122,7 +194,7 @@ void EncoderLibJpeg::compressRGB(const libcamera::MappedBuffer *frame)
>>    * Compress the incoming buffer from a supported NV format.
>>    * This naively unpacks the semi-planar NV12 to a YUV888 format for libjpeg.
>>    */
>> -void EncoderLibJpeg::compressNV(const libcamera::MappedBuffer *frame)
>> +void PostProcessorJpeg::compressNV(const libcamera::MappedBuffer *frame)
>>   {
>>   	uint8_t tmprowbuf[compress_.image_width * 3];
>>   
>> @@ -179,9 +251,9 @@ void EncoderLibJpeg::compressNV(const libcamera::MappedBuffer *frame)
>>   	}
>>   }
>>   
>> -int EncoderLibJpeg::encode(const FrameBuffer *source,
>> -			   const libcamera::Span<uint8_t> &dest,
>> -			   const libcamera::Span<const uint8_t> &exifData)
>> +int PostProcessorJpeg::encode(const FrameBuffer *source,
>> +			      const libcamera::Span<uint8_t> &dest,
>> +			      const libcamera::Span<const uint8_t> &exifData)
>>   {
>>   	MappedFrameBuffer frame(source, PROT_READ);
>>   	if (!frame.isValid()) {
>> diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/post_processor_jpeg.h
>> similarity index 55%
>> rename from src/android/jpeg/encoder_libjpeg.h
>> rename to src/android/jpeg/post_processor_jpeg.h
>> index 1e8df05..7f9ce70 100644
>> --- a/src/android/jpeg/encoder_libjpeg.h
>> +++ b/src/android/jpeg/post_processor_jpeg.h
>> @@ -2,30 +2,37 @@
>>   /*
>>    * Copyright (C) 2020, Google Inc.
>>    *
>> - * encoder_libjpeg.h - JPEG encoding using libjpeg
>> + * post_processor_jpeg.h - JPEG Post Processor
>>    */
>> -#ifndef __ANDROID_JPEG_ENCODER_LIBJPEG_H__
>> -#define __ANDROID_JPEG_ENCODER_LIBJPEG_H__
>> +#ifndef __ANDROID_POST_PROCESSOR_JPEG_H__
>> +#define __ANDROID_POST_PROCESSOR_JPEG_H__
>>   
>> -#include "encoder.h"
>> +#include "../post_processor.h"
>> +#include "../camera_metadata.h"
>>   
>>   #include "libcamera/internal/buffer.h"
>>   #include "libcamera/internal/formats.h"
>>   
>>   #include <jpeglib.h>
>>   
>> -class EncoderLibJpeg : public Encoder
>> +class PostProcessorJpeg : public PostProcessor
>>   {
>>   public:
>> -	EncoderLibJpeg();
>> -	~EncoderLibJpeg();
>> +	PostProcessorJpeg();
>> +	~PostProcessorJpeg();
>>   
>>   	int configure(const libcamera::StreamConfiguration &cfg) override;
>> +	int process(const libcamera::FrameBuffer *source,
>> +		    const libcamera::Span<uint8_t> &destination,
>> +		    CameraMetadata *metadata,
>> +		    ...) override;
>> +
>> +
>> +private:
>>   	int encode(const libcamera::FrameBuffer *source,
>>   		   const libcamera::Span<uint8_t> &destination,
>> -		   const libcamera::Span<const uint8_t> &exifData) override;
>> +		   const libcamera::Span<const uint8_t> &exifData);
>>   
>> -private:
>>   	void compressRGB(const libcamera::MappedBuffer *frame);
>>   	void compressNV(const libcamera::MappedBuffer *frame);
>>   
>> @@ -40,4 +47,4 @@ private:
>>   	bool nvSwap_;
>>   };
>>   
>> -#endif /* __ANDROID_JPEG_ENCODER_LIBJPEG_H__ */
>> +#endif /* __ANDROID_POST_PROCESSOR_JPEG_H__ */
>> diff --git a/src/android/meson.build b/src/android/meson.build
>> index 802bb89..02b3b47 100644
>> --- a/src/android/meson.build
>> +++ b/src/android/meson.build
>> @@ -21,8 +21,8 @@ android_hal_sources = files([
>>       'camera_metadata.cpp',
>>       'camera_ops.cpp',
>>       'camera_stream.cpp',
>> -    'jpeg/encoder_libjpeg.cpp',
>>       'jpeg/exif.cpp',
>> +    'jpeg/post_processor_jpeg.cpp',
>>   ])
>>   
>>   android_camera_metadata_sources = files([

Hi Umang,

On 09/10/2020 15:12, Umang Jain wrote:
> Hi Laurent,
> 
> Forgot to follow up at one more point.
> 
> On 10/9/20 12:52 AM, Laurent Pinchart wrote:
>> Hi Umang,
>>
>> Thank you for the patch.
>>
>> On Thu, Oct 08, 2020 at 07:40:37PM +0530, Umang Jain wrote:
>>> Remove the existing Encoder interface completely and use the
>>> PostProcessor interface instead.
>> I think we need to keep the Encoder interface. We want to support other
>> JPEG encoders than the libjpeg-based implementation. Creating a JPEG
>> post-processor is the right thing to do, but it should still rely on the
>> existing encoder interface for the actual JPEG encoding. From a
>> CameraDevice point of view, only the PostProcessor interface will be
>> visible.
>>
>>> Now the ::encode() function will be called by PostProcessor::process()
>>> internally and will not be directly exposed in CameraStream. Similarly,
>>> other operations can be introduced internally in the PostProcessorJpeg,
>>> and can be called through its process() interface.
>>>
>>> Signed-off-by: Umang Jain <email@uajain.com>
>>> ---
>>>   src/android/camera_device.h                   |  1 -
>>>   src/android/camera_stream.cpp                 | 74 +++------------
>>>   src/android/camera_stream.h                   |  9 +-
>>>   src/android/jpeg/encoder.h                    | 25 -----
>>>   ...er_libjpeg.cpp => post_processor_jpeg.cpp} | 92 +++++++++++++++++--
>>>   ...ncoder_libjpeg.h => post_processor_jpeg.h} | 27 ++++--
>>>   src/android/meson.build                       |  2 +-
>>>   7 files changed, 122 insertions(+), 108 deletions(-)
>>>   delete mode 100644 src/android/jpeg/encoder.h
>>>   rename src/android/jpeg/{encoder_libjpeg.cpp =>
>>> post_processor_jpeg.cpp} (67%)
>>>   rename src/android/jpeg/{encoder_libjpeg.h =>
>>> post_processor_jpeg.h} (55%)
>>>
>>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
>>> index 777d1a3..25de12e 100644
>>> --- a/src/android/camera_device.h
>>> +++ b/src/android/camera_device.h
>>> @@ -25,7 +25,6 @@
>>>   #include "libcamera/internal/message.h"
>>>     #include "camera_stream.h"
>>> -#include "jpeg/encoder.h"
>>>     class CameraMetadata;
>>>   diff --git a/src/android/camera_stream.cpp
>>> b/src/android/camera_stream.cpp
>>> index eac1480..ed3bb41 100644
>>> --- a/src/android/camera_stream.cpp
>>> +++ b/src/android/camera_stream.cpp
>>> @@ -9,9 +9,7 @@
>>>     #include "camera_device.h"
>>>   #include "camera_metadata.h"
>>> -#include "jpeg/encoder.h"
>>> -#include "jpeg/encoder_libjpeg.h"
>>> -#include "jpeg/exif.h"
>>> +#include "jpeg/post_processor_jpeg.h"
>>>     using namespace libcamera;
>>>   @@ -24,8 +22,15 @@ CameraStream::CameraStream(CameraDevice
>>> *cameraDevice, Type type,
>>>   {
>>>       config_ = cameraDevice_->cameraConfiguration();
>>>   -    if (type_ == Type::Internal || type_ == Type::Mapped)
>>> -        encoder_ = std::make_unique<EncoderLibJpeg>();
>>> +    if (type_ == Type::Internal || type_ == Type::Mapped) {
>>> +        /*
>>> +         * \todo There might be multiple post-processors. The logic
>>> +         * which should be instantiated here, is deferred for future.
>>> +         * For now, we only have PostProcessJpeg and that is what we
>> s/PostProcessJpeg/PostProcessorJpeg/
>>
>>> +         * will instantiate here.
>>> +         */
>>> +        postProcessor_ = std::make_unique<PostProcessorJpeg>();
>>> +    }
>>>         if (type == Type::Internal) {
>>>           allocator_ =
>>> std::make_unique<FrameBufferAllocator>(cameraDevice_->camera());
>>> @@ -45,8 +50,8 @@ Stream *CameraStream::stream() const
>>>     int CameraStream::configure()
>>>   {
>>> -    if (encoder_) {
>>> -        int ret = encoder_->configure(configuration());
>>> +    if (postProcessor_) {
>>> +        int ret = postProcessor_->configure(configuration());
>>>           if (ret)
>>>               return ret;
>>>       }
>>> @@ -69,60 +74,11 @@ int CameraStream::configure()
>>>   int CameraStream::process(const libcamera::FrameBuffer &source,
>>>                 MappedCamera3Buffer *dest, CameraMetadata *metadata)
>>>   {
>>> -    if (!encoder_)
>>> +    if (!postProcessor_)
>>>           return 0;
>>>   -    /* Set EXIF metadata for various tags. */
>>> -    Exif exif;
>>> -    /* \todo Set Make and Model from external vendor tags. */
>>> -    exif.setMake("libcamera");
>>> -    exif.setModel("cameraModel");
>>> -    exif.setOrientation(cameraDevice_->orientation());
>>> -    exif.setSize(configuration().size);
>>> -    /*
>>> -     * We set the frame's EXIF timestamp as the time of encode.
>>> -     * Since the precision we need for EXIF timestamp is only one
>>> -     * second, it is good enough.
>>> -     */
>>> -    exif.setTimestamp(std::time(nullptr));
>>> -    if (exif.generate() != 0)
>>> -        LOG(HAL, Error) << "Failed to generate valid EXIF data";
>>> -
>>> -    int jpeg_size = encoder_->encode(&source, dest->maps()[0],
>>> exif.data());
>>> -    if (jpeg_size < 0) {
>>> -        LOG(HAL, Error) << "Failed to encode stream image";
>>> -        return jpeg_size;
>>> -    }
>>> -
>>> -    /*
>>> -     * Fill in the JPEG blob header.
>>> -     *
>>> -     * The mapped size of the buffer is being returned as
>>> -     * substantially larger than the requested JPEG_MAX_SIZE
>>> -     * (which is referenced from maxJpegBufferSize_). Utilise
>>> -     * this static size to ensure the correct offset of the blob is
>>> -     * determined.
>>> -     *
>>> -     * \todo Investigate if the buffer size mismatch is an issue or
>>> -     * expected behaviour.
>>> -     */
>>> -    uint8_t *resultPtr = dest->maps()[0].data() +
>>> -                 cameraDevice_->maxJpegBufferSize() -
>>> -                 sizeof(struct camera3_jpeg_blob);
>>> -    auto *blob = reinterpret_cast<struct camera3_jpeg_blob
>>> *>(resultPtr);
>>> -    blob->jpeg_blob_id = CAMERA3_JPEG_BLOB_ID;
>>> -    blob->jpeg_size = jpeg_size;
>>> -
>>> -    /* Update the JPEG result Metadata. */
>>> -    metadata->addEntry(ANDROID_JPEG_SIZE, &jpeg_size, 1);
>>> -
>>> -    const uint32_t jpeg_quality = 95;
>>> -    metadata->addEntry(ANDROID_JPEG_QUALITY, &jpeg_quality, 1);
>>> -
>>> -    const uint32_t jpeg_orientation = 0;
>>> -    metadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpeg_orientation, 1);
>>> -
>>> -    return 0;
>>> +    return postProcessor_->process(&source, dest->maps()[0],
>>> +                       metadata, cameraDevice_);
>> There are at least two options to keep the interface generic, avoiding
>> variadic arguments. One of them is to explicitly pass the the camera
>> device to the process function. Another option is to pass it to the
>> PostProcessorJpeg constructor, and store it internally. I'd go for the
>> latter.
>>
>>>   }
>>>     FrameBuffer *CameraStream::getBuffer()
>>> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
>>> index 8df0101..8d0f2e3 100644
>>> --- a/src/android/camera_stream.h
>>> +++ b/src/android/camera_stream.h
>>> @@ -19,7 +19,12 @@
>>>   #include <libcamera/geometry.h>
>>>   #include <libcamera/pixel_format.h>
>>>   -class Encoder;
>>> +/*
>>> + * \todo Ideally we want to replace this include with
>>> forward-declaration.
>>> + * If we do that. currently we get a compile error.
>>> + */
>> Let's fix them :-) What compilation error do you get ?
> The error I see:
> 
> [89/298] Compiling C++ object
> src/libcamera/libcamera.so.p/.._android_camera_device.cpp.o
> FAILED: src/libcamera/libcamera.so.p/.._android_camera_device.cpp.o
> c++ -Isrc/libcamera/libcamera.so.p -Isrc/libcamera -I../src/libcamera
> -Iinclude -I../include -I../include/android/hardware/libhardware/include
> -I../include/android/metadata -I../include/android/system/core/include
> -Iinclude/libcamera -fdiagnostics-color=always -pipe
> -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wnon-virtual-dtor -Wextra
> -Werror -std=c++17 -g -include config.h -fPIC -pthread -MD -MQ
> src/libcamera/libcamera.so.p/.._android_camera_device.cpp.o -MF
> src/libcamera/libcamera.so.p/.._android_camera_device.cpp.o.d -o
> src/libcamera/libcamera.so.p/.._android_camera_device.cpp.o -c
> ../src/android/camera_device.cpp
> In file included from /usr/include/c++/10/memory:83,
>                  from ../src/android/camera_device.h:11,
>                  from ../src/android/camera_device.cpp:8:
> /usr/include/c++/10/bits/unique_ptr.h: In instantiation of ‘void
> std::default_delete<_Tp>::operator()(_Tp*) const [with _Tp =
> PostProcessor]’:
> /usr/include/c++/10/bits/unique_ptr.h:360:17:   required from
> ‘std::unique_ptr<_Tp, _Dp>::~unique_ptr() [with _Tp = PostProcessor; _Dp
> = std::default_delete<PostProcessor>]’
> ../src/android/camera_stream.h:32:7:   required from ‘void
> __gnu_cxx::new_allocator<_Tp>::destroy(_Up*) [with _Up = CameraStream;
> _Tp = CameraStream]’
> /usr/include/c++/10/bits/alloc_traits.h:531:15:   required from ‘static
> void std::allocator_traits<std::allocator<_Tp1>
>>::destroy(std::allocator_traits<std::allocator<_Tp1>
>>::allocator_type&, _Up*) [with _Up = CameraStream; _Tp = CameraStream;
> std::allocator_traits<std::allocator<_Tp1> >::allocator_type =
> std::allocator<CameraStream>]’
> /usr/include/c++/10/bits/vector.tcc:488:28:   required from ‘void
> std::vector<_Tp, _Alloc>::_M_realloc_insert(std::vector<_Tp,
> _Alloc>::iterator, _Args&& ...) [with _Args = {CameraDevice*,
> CameraStream::Type, camera3_stream*&, long unsigned int}; _Tp =
> CameraStream; _Alloc = std::allocator<CameraStream>; std::vector<_Tp,
> _Alloc>::iterator = std::vector<CameraStream>::iterator]’
> /usr/include/c++/10/bits/vector.tcc:121:21:   required from
> ‘std::vector<_Tp, _Alloc>::reference std::vector<_Tp,
> _Alloc>::emplace_back(_Args&& ...) [with _Args = {CameraDevice*,
> CameraStream::Type, camera3_stream*&, long unsigned int}; _Tp =
> CameraStream; _Alloc = std::allocator<CameraStream>; std::vector<_Tp,
> _Alloc>::reference = CameraStream&]’
> ../src/android/camera_device.cpp:1212:38:   required from here
> /usr/include/c++/10/bits/unique_ptr.h:82:16: error: invalid application
> of ‘sizeof’ to incomplete type ‘PostProcessor’
>    82 |  static_assert(sizeof(_Tp)>0,
>       |                ^~~~~~~~~~~
> [98/298] Compiling C++ object
> src/libcamera/libcamera.so.p/.._android_camera_stream.cpp.o
> ninja: build stopped: subcommand failed.
> 
> Not sure why it cannot work with the incomplete type `PostProcessor`
> interface
> (well, it used to work well with Encoder's interface, right there!)
>>
>>> +#include "post_processor.h"

Does this file get included in ./src/android/camera_device.cpp ?
--
Kieran


>>> +
>>>   class CameraDevice;
>>>   class CameraMetadata;
>>>   class MappedCamera3Buffer;
>>> @@ -135,7 +140,7 @@ private:
>>>        */
>>>       unsigned int index_;
>>>   -    std::unique_ptr<Encoder> encoder_;
>>> +    std::unique_ptr<PostProcessor> postProcessor_;
>>>       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
>>> deleted file mode 100644
>>> index cf26d67..0000000
>>> --- a/src/android/jpeg/encoder.h
>>> +++ /dev/null
>>> @@ -1,25 +0,0 @@
>>> -/* SPDX-License-Identifier: GPL-2.0-or-later */
>>> -/*
>>> - * Copyright (C) 2020, Google Inc.
>>> - *
>>> - * encoder.h - Image encoding interface
>>> - */
>>> -#ifndef __ANDROID_JPEG_ENCODER_H__
>>> -#define __ANDROID_JPEG_ENCODER_H__
>>> -
>>> -#include <libcamera/buffer.h>
>>> -#include <libcamera/span.h>
>>> -#include <libcamera/stream.h>
>>> -
>>> -class Encoder
>>> -{
>>> -public:
>>> -    virtual ~Encoder() {};
>>> -
>>> -    virtual int configure(const libcamera::StreamConfiguration &cfg)
>>> = 0;
>>> -    virtual int encode(const libcamera::FrameBuffer *source,
>>> -               const libcamera::Span<uint8_t> &destination,
>>> -               const libcamera::Span<const uint8_t> &exifData) = 0;
>>> -};
>>> -
>>> -#endif /* __ANDROID_JPEG_ENCODER_H__ */
>> As mentioned above, this should be kept, and used by the JPEG
>> post-processor.
>>
>>> diff --git a/src/android/jpeg/encoder_libjpeg.cpp
>>> b/src/android/jpeg/post_processor_jpeg.cpp
>>> similarity index 67%
>>> rename from src/android/jpeg/encoder_libjpeg.cpp
>>> rename to src/android/jpeg/post_processor_jpeg.cpp
>>> index 510613c..eeb4e95 100644
>>> --- a/src/android/jpeg/encoder_libjpeg.cpp
>>> +++ b/src/android/jpeg/post_processor_jpeg.cpp
>>> @@ -2,10 +2,14 @@
>>>   /*
>>>    * Copyright (C) 2020, Google Inc.
>>>    *
>>> - * encoder_libjpeg.cpp - JPEG encoding using libjpeg native API
>>> + * post_processor_jpeg.cpp - JPEG Post Processor
>>>    */
>>>   -#include "encoder_libjpeg.h"
>>> +#include "post_processor_jpeg.h"
>>> +
>>> +#include "exif.h"
>>> +
>>> +#include "../camera_device.h"
>>>     #include <fcntl.h>
>>>   #include <iomanip>
>>> @@ -25,6 +29,14 @@
>>>     using namespace libcamera;
>>>   +#define extract_va_arg(type, arg, last) \
>>> +{                                       \
>>> +        va_list ap;                     \
>>> +        va_start(ap, last);             \
>>> +        arg = va_arg(ap, type);         \
>>> +        va_end(ap);                     \
>>> +}
>>> +
>>>   LOG_DEFINE_CATEGORY(JPEG)
>>>     namespace {
>>> @@ -67,7 +79,7 @@ const struct JPEGPixelFormatInfo
>>> &findPixelInfo(const PixelFormat &format)
>>>     } /* namespace */
>>>   -EncoderLibJpeg::EncoderLibJpeg()
>>> +PostProcessorJpeg::PostProcessorJpeg()
>>>       : quality_(95)
>>>   {
>>>       /* \todo Expand error handling coverage with a custom handler. */
>>> @@ -76,12 +88,12 @@ EncoderLibJpeg::EncoderLibJpeg()
>>>       jpeg_create_compress(&compress_);
>>>   }
>>>   -EncoderLibJpeg::~EncoderLibJpeg()
>>> +PostProcessorJpeg::~PostProcessorJpeg()
>>>   {
>>>       jpeg_destroy_compress(&compress_);
>>>   }
>>>   -int EncoderLibJpeg::configure(const StreamConfiguration &cfg)
>>> +int PostProcessorJpeg::configure(const StreamConfiguration &cfg)
>>>   {
>>>       const struct JPEGPixelFormatInfo info =
>>> findPixelInfo(cfg.pixelFormat);
>>>       if (info.colorSpace == JCS_UNKNOWN)
>>> @@ -104,7 +116,67 @@ int EncoderLibJpeg::configure(const
>>> StreamConfiguration &cfg)
>>>       return 0;
>>>   }
>>>   -void EncoderLibJpeg::compressRGB(const libcamera::MappedBuffer
>>> *frame)
>>> +int PostProcessorJpeg::process(const libcamera::FrameBuffer *source,
>>> +                   const libcamera::Span<uint8_t> &destination,
>>> +                   CameraMetadata *metadata, ...)
>>> +{
>>> +    CameraDevice *device = nullptr;
>>> +    extract_va_arg(CameraDevice *, device, metadata);
>>> +
>>> +    /* Set EXIF metadata for various tags. */
>>> +    Exif exif;
>>> +    /* \todo Set Make and Model from external vendor tags. */
>>> +    exif.setMake("libcamera");
>>> +    exif.setModel("cameraModel");
>>> +    exif.setOrientation(device->orientation());
>>> +    exif.setSize(Size {compress_.image_width, compress_.image_height});
>>> +    /*
>>> +     * We set the frame's EXIF timestamp as the time of encode.
>>> +     * Since the precision we need for EXIF timestamp is only one
>>> +     * second, it is good enough.
>>> +     */
>>> +    exif.setTimestamp(std::time(nullptr));
>>> +    if (exif.generate() != 0)
>>> +        LOG(JPEG, Error) << "Failed to generate valid EXIF data";
>>> +
>>> +    int jpeg_size = encode(source, destination, exif.data());
>>> +    if (jpeg_size < 0) {
>>> +        LOG(JPEG, Error) << "Failed to encode stream image";
>>> +        return jpeg_size;
>>> +    }
>>> +
>>> +    /*
>>> +     * Fill in the JPEG blob header.
>>> +     *
>>> +     * The mapped size of the buffer is being returned as
>>> +     * substantially larger than the requested JPEG_MAX_SIZE
>>> +     * (which is referenced from maxJpegBufferSize_). Utilise
>>> +     * this static size to ensure the correct offset of the blob is
>>> +     * determined.
>>> +     *
>>> +     * \todo Investigate if the buffer size mismatch is an issue or
>>> +     * expected behaviour.
>>> +     */
>>> +    uint8_t *resultPtr = destination.data() +
>>> +                 device->maxJpegBufferSize() -
>>> +                 sizeof(struct camera3_jpeg_blob);
>>> +    auto *blob = reinterpret_cast<struct camera3_jpeg_blob
>>> *>(resultPtr);
>>> +    blob->jpeg_blob_id = CAMERA3_JPEG_BLOB_ID;
>>> +    blob->jpeg_size = jpeg_size;
>>> +
>>> +    /* Update the JPEG result Metadata. */
>>> +    metadata->addEntry(ANDROID_JPEG_SIZE, &jpeg_size, 1);
>>> +
>>> +    const uint32_t jpeg_quality = 95;
>>> +    metadata->addEntry(ANDROID_JPEG_QUALITY, &jpeg_quality, 1);
>>> +
>>> +    const uint32_t jpeg_orientation = 0;
>>> +    metadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpeg_orientation, 1);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +void PostProcessorJpeg::compressRGB(const libcamera::MappedBuffer
>>> *frame)
>>>   {
>>>       unsigned char *src = static_cast<unsigned char
>>> *>(frame->maps()[0].data());
>>>       /* \todo Stride information should come from buffer
>>> configuration. */
>>> @@ -122,7 +194,7 @@ void EncoderLibJpeg::compressRGB(const
>>> libcamera::MappedBuffer *frame)
>>>    * Compress the incoming buffer from a supported NV format.
>>>    * This naively unpacks the semi-planar NV12 to a YUV888 format for
>>> libjpeg.
>>>    */
>>> -void EncoderLibJpeg::compressNV(const libcamera::MappedBuffer *frame)
>>> +void PostProcessorJpeg::compressNV(const libcamera::MappedBuffer
>>> *frame)
>>>   {
>>>       uint8_t tmprowbuf[compress_.image_width * 3];
>>>   @@ -179,9 +251,9 @@ void EncoderLibJpeg::compressNV(const
>>> libcamera::MappedBuffer *frame)
>>>       }
>>>   }
>>>   -int EncoderLibJpeg::encode(const FrameBuffer *source,
>>> -               const libcamera::Span<uint8_t> &dest,
>>> -               const libcamera::Span<const uint8_t> &exifData)
>>> +int PostProcessorJpeg::encode(const FrameBuffer *source,
>>> +                  const libcamera::Span<uint8_t> &dest,
>>> +                  const libcamera::Span<const uint8_t> &exifData)
>>>   {
>>>       MappedFrameBuffer frame(source, PROT_READ);
>>>       if (!frame.isValid()) {
>>> diff --git a/src/android/jpeg/encoder_libjpeg.h
>>> b/src/android/jpeg/post_processor_jpeg.h
>>> similarity index 55%
>>> rename from src/android/jpeg/encoder_libjpeg.h
>>> rename to src/android/jpeg/post_processor_jpeg.h
>>> index 1e8df05..7f9ce70 100644
>>> --- a/src/android/jpeg/encoder_libjpeg.h
>>> +++ b/src/android/jpeg/post_processor_jpeg.h
>>> @@ -2,30 +2,37 @@
>>>   /*
>>>    * Copyright (C) 2020, Google Inc.
>>>    *
>>> - * encoder_libjpeg.h - JPEG encoding using libjpeg
>>> + * post_processor_jpeg.h - JPEG Post Processor
>>>    */
>>> -#ifndef __ANDROID_JPEG_ENCODER_LIBJPEG_H__
>>> -#define __ANDROID_JPEG_ENCODER_LIBJPEG_H__
>>> +#ifndef __ANDROID_POST_PROCESSOR_JPEG_H__
>>> +#define __ANDROID_POST_PROCESSOR_JPEG_H__
>>>   -#include "encoder.h"
>>> +#include "../post_processor.h"
>>> +#include "../camera_metadata.h"
>>>     #include "libcamera/internal/buffer.h"
>>>   #include "libcamera/internal/formats.h"
>>>     #include <jpeglib.h>
>>>   -class EncoderLibJpeg : public Encoder
>>> +class PostProcessorJpeg : public PostProcessor
>>>   {
>>>   public:
>>> -    EncoderLibJpeg();
>>> -    ~EncoderLibJpeg();
>>> +    PostProcessorJpeg();
>>> +    ~PostProcessorJpeg();
>>>         int configure(const libcamera::StreamConfiguration &cfg)
>>> override;
>>> +    int process(const libcamera::FrameBuffer *source,
>>> +            const libcamera::Span<uint8_t> &destination,
>>> +            CameraMetadata *metadata,
>>> +            ...) override;
>>> +
>>> +
>>> +private:
>>>       int encode(const libcamera::FrameBuffer *source,
>>>              const libcamera::Span<uint8_t> &destination,
>>> -           const libcamera::Span<const uint8_t> &exifData) override;
>>> +           const libcamera::Span<const uint8_t> &exifData);
>>>   -private:
>>>       void compressRGB(const libcamera::MappedBuffer *frame);
>>>       void compressNV(const libcamera::MappedBuffer *frame);
>>>   @@ -40,4 +47,4 @@ private:
>>>       bool nvSwap_;
>>>   };
>>>   -#endif /* __ANDROID_JPEG_ENCODER_LIBJPEG_H__ */
>>> +#endif /* __ANDROID_POST_PROCESSOR_JPEG_H__ */
>>> diff --git a/src/android/meson.build b/src/android/meson.build
>>> index 802bb89..02b3b47 100644
>>> --- a/src/android/meson.build
>>> +++ b/src/android/meson.build
>>> @@ -21,8 +21,8 @@ android_hal_sources = files([
>>>       'camera_metadata.cpp',
>>>       'camera_ops.cpp',
>>>       'camera_stream.cpp',
>>> -    'jpeg/encoder_libjpeg.cpp',
>>>       'jpeg/exif.cpp',
>>> +    'jpeg/post_processor_jpeg.cpp',
>>>   ])
>>>     android_camera_metadata_sources = files([
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Umang,

On Fri, Oct 09, 2020 at 10:04:18AM +0530, Umang Jain wrote:
> On 10/9/20 12:52 AM, Laurent Pinchart wrote:
> > On Thu, Oct 08, 2020 at 07:40:37PM +0530, Umang Jain wrote:
> >> Remove the existing Encoder interface completely and use the
> >> PostProcessor interface instead.
> >
> > I think we need to keep the Encoder interface. We want to support other
> > JPEG encoders than the libjpeg-based implementation. Creating a JPEG
> > post-processor is the right thing to do, but it should still rely on the
> > existing encoder interface for the actual JPEG encoding. From a
> > CameraDevice point of view, only the PostProcessor interface will be
> > visible.
>
> Oh. I didn't see that. I assumed we will have only one? encoder and even 
> if we had more, it would simply encapsulated within a private function 
> as PostProcessorJpeg::encodeViaX() or whatever. I will bring back the 
> Encoder interface if you say so.
> 
> Indeed, CameraDevice point-of-view, we only want to expose the 
> PostProcessor interface.
>
> >> Now the ::encode() function will be called by PostProcessor::process()
> >> internally and will not be directly exposed in CameraStream. Similarly,
> >> other operations can be introduced internally in the PostProcessorJpeg,
> >> and can be called through its process() interface.
> >>
> >> Signed-off-by: Umang Jain <email@uajain.com>
> >> ---
> >>   src/android/camera_device.h                   |  1 -
> >>   src/android/camera_stream.cpp                 | 74 +++------------
> >>   src/android/camera_stream.h                   |  9 +-
> >>   src/android/jpeg/encoder.h                    | 25 -----
> >>   ...er_libjpeg.cpp => post_processor_jpeg.cpp} | 92 +++++++++++++++++--
> >>   ...ncoder_libjpeg.h => post_processor_jpeg.h} | 27 ++++--
> >>   src/android/meson.build                       |  2 +-
> >>   7 files changed, 122 insertions(+), 108 deletions(-)
> >>   delete mode 100644 src/android/jpeg/encoder.h
> >>   rename src/android/jpeg/{encoder_libjpeg.cpp => post_processor_jpeg.cpp} (67%)
> >>   rename src/android/jpeg/{encoder_libjpeg.h => post_processor_jpeg.h} (55%)
> >>
> >> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> >> index 777d1a3..25de12e 100644
> >> --- a/src/android/camera_device.h
> >> +++ b/src/android/camera_device.h
> >> @@ -25,7 +25,6 @@
> >>   #include "libcamera/internal/message.h"
> >>   
> >>   #include "camera_stream.h"
> >> -#include "jpeg/encoder.h"
> >>   
> >>   class CameraMetadata;
> >>   
> >> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> >> index eac1480..ed3bb41 100644
> >> --- a/src/android/camera_stream.cpp
> >> +++ b/src/android/camera_stream.cpp
> >> @@ -9,9 +9,7 @@
> >>   
> >>   #include "camera_device.h"
> >>   #include "camera_metadata.h"
> >> -#include "jpeg/encoder.h"
> >> -#include "jpeg/encoder_libjpeg.h"
> >> -#include "jpeg/exif.h"
> >> +#include "jpeg/post_processor_jpeg.h"
> >>   
> >>   using namespace libcamera;
> >>   
> >> @@ -24,8 +22,15 @@ CameraStream::CameraStream(CameraDevice *cameraDevice, Type type,
> >>   {
> >>   	config_ = cameraDevice_->cameraConfiguration();
> >>   
> >> -	if (type_ == Type::Internal || type_ == Type::Mapped)
> >> -		encoder_ = std::make_unique<EncoderLibJpeg>();
> >> +	if (type_ == Type::Internal || type_ == Type::Mapped) {
> >> +		/*
> >> +		 * \todo There might be multiple post-processors. The logic
> >> +		 * which should be instantiated here, is deferred for future.
> >> +		 * For now, we only have PostProcessJpeg and that is what we
> >
> > s/PostProcessJpeg/PostProcessorJpeg/
> >
> >> +		 * will instantiate here.
> >> +		 */
> >> +		postProcessor_ = std::make_unique<PostProcessorJpeg>();
> >> +	}
> >>   
> >>   	if (type == Type::Internal) {
> >>   		allocator_ = std::make_unique<FrameBufferAllocator>(cameraDevice_->camera());
> >> @@ -45,8 +50,8 @@ Stream *CameraStream::stream() const
> >>   
> >>   int CameraStream::configure()
> >>   {
> >> -	if (encoder_) {
> >> -		int ret = encoder_->configure(configuration());
> >> +	if (postProcessor_) {
> >> +		int ret = postProcessor_->configure(configuration());
> >>   		if (ret)
> >>   			return ret;
> >>   	}
> >> @@ -69,60 +74,11 @@ int CameraStream::configure()
> >>   int CameraStream::process(const libcamera::FrameBuffer &source,
> >>   			  MappedCamera3Buffer *dest, CameraMetadata *metadata)
> >>   {
> >> -	if (!encoder_)
> >> +	if (!postProcessor_)
> >>   		return 0;
> >>   
> >> -	/* Set EXIF metadata for various tags. */
> >> -	Exif exif;
> >> -	/* \todo Set Make and Model from external vendor tags. */
> >> -	exif.setMake("libcamera");
> >> -	exif.setModel("cameraModel");
> >> -	exif.setOrientation(cameraDevice_->orientation());
> >> -	exif.setSize(configuration().size);
> >> -	/*
> >> -	 * We set the frame's EXIF timestamp as the time of encode.
> >> -	 * Since the precision we need for EXIF timestamp is only one
> >> -	 * second, it is good enough.
> >> -	 */
> >> -	exif.setTimestamp(std::time(nullptr));
> >> -	if (exif.generate() != 0)
> >> -		LOG(HAL, Error) << "Failed to generate valid EXIF data";
> >> -
> >> -	int jpeg_size = encoder_->encode(&source, dest->maps()[0], exif.data());
> >> -	if (jpeg_size < 0) {
> >> -		LOG(HAL, Error) << "Failed to encode stream image";
> >> -		return jpeg_size;
> >> -	}
> >> -
> >> -	/*
> >> -	 * Fill in the JPEG blob header.
> >> -	 *
> >> -	 * The mapped size of the buffer is being returned as
> >> -	 * substantially larger than the requested JPEG_MAX_SIZE
> >> -	 * (which is referenced from maxJpegBufferSize_). Utilise
> >> -	 * this static size to ensure the correct offset of the blob is
> >> -	 * determined.
> >> -	 *
> >> -	 * \todo Investigate if the buffer size mismatch is an issue or
> >> -	 * expected behaviour.
> >> -	 */
> >> -	uint8_t *resultPtr = dest->maps()[0].data() +
> >> -			     cameraDevice_->maxJpegBufferSize() -
> >> -			     sizeof(struct camera3_jpeg_blob);
> >> -	auto *blob = reinterpret_cast<struct camera3_jpeg_blob *>(resultPtr);
> >> -	blob->jpeg_blob_id = CAMERA3_JPEG_BLOB_ID;
> >> -	blob->jpeg_size = jpeg_size;
> >> -
> >> -	/* Update the JPEG result Metadata. */
> >> -	metadata->addEntry(ANDROID_JPEG_SIZE, &jpeg_size, 1);
> >> -
> >> -	const uint32_t jpeg_quality = 95;
> >> -	metadata->addEntry(ANDROID_JPEG_QUALITY, &jpeg_quality, 1);
> >> -
> >> -	const uint32_t jpeg_orientation = 0;
> >> -	metadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpeg_orientation, 1);
> >> -
> >> -	return 0;
> >> +	return postProcessor_->process(&source, dest->maps()[0],
> >> +				       metadata, cameraDevice_);
> >
> > There are at least two options to keep the interface generic, avoiding
> > variadic arguments. One of them is to explicitly pass the the camera
> > device to the process function. Another option is to pass it to the
> > PostProcessorJpeg constructor, and store it internally. I'd go for the
> > latter.
>
> I see. I thought vardiac arguments can be helpful for 
> (future)PostProcessors to have the flexibility of parameters they might 
> need when they process(). But I do get your point that they might defeat 
> the entire purpose of keeping the interface generic. Asking for 
> curiousity, is passing the specifics parameters via respective 
> PostProcessors' constructors (instead of vardiac args) is the way you 
> would suggest in general to achieve this goal?

Yes, that would be the best option I think. The code that creates the
post-processors will need to be post-processor-aware, so that's where we
should handle any specifics. The rest of the code should be generic.

We can also extend the process() and configure() functions with extra
parameters that are always passed to the functions, and only used by
some post-processors. This should however be limited to fairly generic
parameters (CameraDevice would qualify), not data that would be very
post-processor-specific.

> >>   }
> >>   
> >>   FrameBuffer *CameraStream::getBuffer()
> >> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> >> index 8df0101..8d0f2e3 100644
> >> --- a/src/android/camera_stream.h
> >> +++ b/src/android/camera_stream.h
> >> @@ -19,7 +19,12 @@
> >>   #include <libcamera/geometry.h>
> >>   #include <libcamera/pixel_format.h>
> >>   
> >> -class Encoder;
> >> +/*
> >> + * \todo Ideally we want to replace this include with forward-declaration.
> >> + * If we do that. currently we get a compile error.
> >> + */
> >
> > Let's fix them :-) What compilation error do you get ?
> >
> >> +#include "post_processor.h"
> >> +
> >>   class CameraDevice;
> >>   class CameraMetadata;
> >>   class MappedCamera3Buffer;
> >> @@ -135,7 +140,7 @@ private:
> >>   	 */
> >>   	unsigned int index_;
> >>   
> >> -	std::unique_ptr<Encoder> encoder_;
> >> +	std::unique_ptr<PostProcessor> postProcessor_;
> >>   	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
> >> deleted file mode 100644
> >> index cf26d67..0000000
> >> --- a/src/android/jpeg/encoder.h
> >> +++ /dev/null
> >> @@ -1,25 +0,0 @@
> >> -/* SPDX-License-Identifier: GPL-2.0-or-later */
> >> -/*
> >> - * Copyright (C) 2020, Google Inc.
> >> - *
> >> - * encoder.h - Image encoding interface
> >> - */
> >> -#ifndef __ANDROID_JPEG_ENCODER_H__
> >> -#define __ANDROID_JPEG_ENCODER_H__
> >> -
> >> -#include <libcamera/buffer.h>
> >> -#include <libcamera/span.h>
> >> -#include <libcamera/stream.h>
> >> -
> >> -class Encoder
> >> -{
> >> -public:
> >> -	virtual ~Encoder() {};
> >> -
> >> -	virtual int configure(const libcamera::StreamConfiguration &cfg) = 0;
> >> -	virtual int encode(const libcamera::FrameBuffer *source,
> >> -			   const libcamera::Span<uint8_t> &destination,
> >> -			   const libcamera::Span<const uint8_t> &exifData) = 0;
> >> -};
> >> -
> >> -#endif /* __ANDROID_JPEG_ENCODER_H__ */
> >
> > As mentioned above, this should be kept, and used by the JPEG
> > post-processor.
> >
> >> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> >> similarity index 67%
> >> rename from src/android/jpeg/encoder_libjpeg.cpp
> >> rename to src/android/jpeg/post_processor_jpeg.cpp
> >> index 510613c..eeb4e95 100644
> >> --- a/src/android/jpeg/encoder_libjpeg.cpp
> >> +++ b/src/android/jpeg/post_processor_jpeg.cpp
> >> @@ -2,10 +2,14 @@
> >>   /*
> >>    * Copyright (C) 2020, Google Inc.
> >>    *
> >> - * encoder_libjpeg.cpp - JPEG encoding using libjpeg native API
> >> + * post_processor_jpeg.cpp - JPEG Post Processor
> >>    */
> >>   
> >> -#include "encoder_libjpeg.h"
> >> +#include "post_processor_jpeg.h"
> >> +
> >> +#include "exif.h"
> >> +
> >> +#include "../camera_device.h"
> >>   
> >>   #include <fcntl.h>
> >>   #include <iomanip>
> >> @@ -25,6 +29,14 @@
> >>   
> >>   using namespace libcamera;
> >>   
> >> +#define extract_va_arg(type, arg, last) \
> >> +{                                       \
> >> +        va_list ap;                     \
> >> +        va_start(ap, last);             \
> >> +        arg = va_arg(ap, type);         \
> >> +        va_end(ap);                     \
> >> +}
> >> +
> >>   LOG_DEFINE_CATEGORY(JPEG)
> >>   
> >>   namespace {
> >> @@ -67,7 +79,7 @@ const struct JPEGPixelFormatInfo &findPixelInfo(const PixelFormat &format)
> >>   
> >>   } /* namespace */
> >>   
> >> -EncoderLibJpeg::EncoderLibJpeg()
> >> +PostProcessorJpeg::PostProcessorJpeg()
> >>   	: quality_(95)
> >>   {
> >>   	/* \todo Expand error handling coverage with a custom handler. */
> >> @@ -76,12 +88,12 @@ EncoderLibJpeg::EncoderLibJpeg()
> >>   	jpeg_create_compress(&compress_);
> >>   }
> >>   
> >> -EncoderLibJpeg::~EncoderLibJpeg()
> >> +PostProcessorJpeg::~PostProcessorJpeg()
> >>   {
> >>   	jpeg_destroy_compress(&compress_);
> >>   }
> >>   
> >> -int EncoderLibJpeg::configure(const StreamConfiguration &cfg)
> >> +int PostProcessorJpeg::configure(const StreamConfiguration &cfg)
> >>   {
> >>   	const struct JPEGPixelFormatInfo info = findPixelInfo(cfg.pixelFormat);
> >>   	if (info.colorSpace == JCS_UNKNOWN)
> >> @@ -104,7 +116,67 @@ int EncoderLibJpeg::configure(const StreamConfiguration &cfg)
> >>   	return 0;
> >>   }
> >>   
> >> -void EncoderLibJpeg::compressRGB(const libcamera::MappedBuffer *frame)
> >> +int PostProcessorJpeg::process(const libcamera::FrameBuffer *source,
> >> +			       const libcamera::Span<uint8_t> &destination,
> >> +			       CameraMetadata *metadata, ...)
> >> +{
> >> +	CameraDevice *device = nullptr;
> >> +	extract_va_arg(CameraDevice *, device, metadata);
> >> +
> >> +	/* Set EXIF metadata for various tags. */
> >> +	Exif exif;
> >> +	/* \todo Set Make and Model from external vendor tags. */
> >> +	exif.setMake("libcamera");
> >> +	exif.setModel("cameraModel");
> >> +	exif.setOrientation(device->orientation());
> >> +	exif.setSize(Size {compress_.image_width, compress_.image_height});
> >> +	/*
> >> +	 * We set the frame's EXIF timestamp as the time of encode.
> >> +	 * Since the precision we need for EXIF timestamp is only one
> >> +	 * second, it is good enough.
> >> +	 */
> >> +	exif.setTimestamp(std::time(nullptr));
> >> +	if (exif.generate() != 0)
> >> +		LOG(JPEG, Error) << "Failed to generate valid EXIF data";
> >> +
> >> +	int jpeg_size = encode(source, destination, exif.data());
> >> +	if (jpeg_size < 0) {
> >> +		LOG(JPEG, Error) << "Failed to encode stream image";
> >> +		return jpeg_size;
> >> +	}
> >> +
> >> +	/*
> >> +	 * Fill in the JPEG blob header.
> >> +	 *
> >> +	 * The mapped size of the buffer is being returned as
> >> +	 * substantially larger than the requested JPEG_MAX_SIZE
> >> +	 * (which is referenced from maxJpegBufferSize_). Utilise
> >> +	 * this static size to ensure the correct offset of the blob is
> >> +	 * determined.
> >> +	 *
> >> +	 * \todo Investigate if the buffer size mismatch is an issue or
> >> +	 * expected behaviour.
> >> +	 */
> >> +	uint8_t *resultPtr = destination.data() +
> >> +			     device->maxJpegBufferSize() -
> >> +			     sizeof(struct camera3_jpeg_blob);
> >> +	auto *blob = reinterpret_cast<struct camera3_jpeg_blob *>(resultPtr);
> >> +	blob->jpeg_blob_id = CAMERA3_JPEG_BLOB_ID;
> >> +	blob->jpeg_size = jpeg_size;
> >> +
> >> +	/* Update the JPEG result Metadata. */
> >> +	metadata->addEntry(ANDROID_JPEG_SIZE, &jpeg_size, 1);
> >> +
> >> +	const uint32_t jpeg_quality = 95;
> >> +	metadata->addEntry(ANDROID_JPEG_QUALITY, &jpeg_quality, 1);
> >> +
> >> +	const uint32_t jpeg_orientation = 0;
> >> +	metadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpeg_orientation, 1);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +void PostProcessorJpeg::compressRGB(const libcamera::MappedBuffer *frame)
> >>   {
> >>   	unsigned char *src = static_cast<unsigned char *>(frame->maps()[0].data());
> >>   	/* \todo Stride information should come from buffer configuration. */
> >> @@ -122,7 +194,7 @@ void EncoderLibJpeg::compressRGB(const libcamera::MappedBuffer *frame)
> >>    * Compress the incoming buffer from a supported NV format.
> >>    * This naively unpacks the semi-planar NV12 to a YUV888 format for libjpeg.
> >>    */
> >> -void EncoderLibJpeg::compressNV(const libcamera::MappedBuffer *frame)
> >> +void PostProcessorJpeg::compressNV(const libcamera::MappedBuffer *frame)
> >>   {
> >>   	uint8_t tmprowbuf[compress_.image_width * 3];
> >>   
> >> @@ -179,9 +251,9 @@ void EncoderLibJpeg::compressNV(const libcamera::MappedBuffer *frame)
> >>   	}
> >>   }
> >>   
> >> -int EncoderLibJpeg::encode(const FrameBuffer *source,
> >> -			   const libcamera::Span<uint8_t> &dest,
> >> -			   const libcamera::Span<const uint8_t> &exifData)
> >> +int PostProcessorJpeg::encode(const FrameBuffer *source,
> >> +			      const libcamera::Span<uint8_t> &dest,
> >> +			      const libcamera::Span<const uint8_t> &exifData)
> >>   {
> >>   	MappedFrameBuffer frame(source, PROT_READ);
> >>   	if (!frame.isValid()) {
> >> diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/post_processor_jpeg.h
> >> similarity index 55%
> >> rename from src/android/jpeg/encoder_libjpeg.h
> >> rename to src/android/jpeg/post_processor_jpeg.h
> >> index 1e8df05..7f9ce70 100644
> >> --- a/src/android/jpeg/encoder_libjpeg.h
> >> +++ b/src/android/jpeg/post_processor_jpeg.h
> >> @@ -2,30 +2,37 @@
> >>   /*
> >>    * Copyright (C) 2020, Google Inc.
> >>    *
> >> - * encoder_libjpeg.h - JPEG encoding using libjpeg
> >> + * post_processor_jpeg.h - JPEG Post Processor
> >>    */
> >> -#ifndef __ANDROID_JPEG_ENCODER_LIBJPEG_H__
> >> -#define __ANDROID_JPEG_ENCODER_LIBJPEG_H__
> >> +#ifndef __ANDROID_POST_PROCESSOR_JPEG_H__
> >> +#define __ANDROID_POST_PROCESSOR_JPEG_H__
> >>   
> >> -#include "encoder.h"
> >> +#include "../post_processor.h"
> >> +#include "../camera_metadata.h"
> >>   
> >>   #include "libcamera/internal/buffer.h"
> >>   #include "libcamera/internal/formats.h"
> >>   
> >>   #include <jpeglib.h>
> >>   
> >> -class EncoderLibJpeg : public Encoder
> >> +class PostProcessorJpeg : public PostProcessor
> >>   {
> >>   public:
> >> -	EncoderLibJpeg();
> >> -	~EncoderLibJpeg();
> >> +	PostProcessorJpeg();
> >> +	~PostProcessorJpeg();
> >>   
> >>   	int configure(const libcamera::StreamConfiguration &cfg) override;
> >> +	int process(const libcamera::FrameBuffer *source,
> >> +		    const libcamera::Span<uint8_t> &destination,
> >> +		    CameraMetadata *metadata,
> >> +		    ...) override;
> >> +
> >> +
> >> +private:
> >>   	int encode(const libcamera::FrameBuffer *source,
> >>   		   const libcamera::Span<uint8_t> &destination,
> >> -		   const libcamera::Span<const uint8_t> &exifData) override;
> >> +		   const libcamera::Span<const uint8_t> &exifData);
> >>   
> >> -private:
> >>   	void compressRGB(const libcamera::MappedBuffer *frame);
> >>   	void compressNV(const libcamera::MappedBuffer *frame);
> >>   
> >> @@ -40,4 +47,4 @@ private:
> >>   	bool nvSwap_;
> >>   };
> >>   
> >> -#endif /* __ANDROID_JPEG_ENCODER_LIBJPEG_H__ */
> >> +#endif /* __ANDROID_POST_PROCESSOR_JPEG_H__ */
> >> diff --git a/src/android/meson.build b/src/android/meson.build
> >> index 802bb89..02b3b47 100644
> >> --- a/src/android/meson.build
> >> +++ b/src/android/meson.build
> >> @@ -21,8 +21,8 @@ android_hal_sources = files([
> >>       'camera_metadata.cpp',
> >>       'camera_ops.cpp',
> >>       'camera_stream.cpp',
> >> -    'jpeg/encoder_libjpeg.cpp',
> >>       'jpeg/exif.cpp',
> >> +    'jpeg/post_processor_jpeg.cpp',
> >>   ])
> >>   
> >>   android_camera_metadata_sources = files([