[libcamera-devel,6/6] android: Set result metadata and EXIF fields based on request metadata

10862 diff mbox series

Set the following android result metadata:
- ANDROID_LENS_FOCAL_LENGTH
- ANDROID_LENS_APERTURE
- ANDROID_JPEG_GPS_TIMESTAMP
- ANDROID_JPEG_THUMBNAIL_SIZE
- ANDROID_JPEG_THUMBNAIL_QUALITY
- ANDROID_JPEG_GPS_COORDINATES
- ANDROID_JPEG_GPS_PROCESSING_METHOD
- ANDROID_JPEG_SIZE
- ANDROID_JPEG_QUALITY
- ANDROID_JPEG_ORIENTATION

And the following EXIF fields:
- GPSDatestamp
- GPSTimestamp
- GPSLocation
  - GPSLatitudeRef
  - GPSLatitude
  - GPSLongitudeRef
  - GPSLongitude
  - GPSAltitudeRef
  - GPSAltitude
- GPSProcessingMethod
- FocalLength
- ExposureTime
- FNumber
- ISO
- Flash
- WhiteBalance
- SubsecTime
- SubsecTimeOriginal
- SubsecTimeDigitized

Based on android request metadata.

This allows the following CTS tests to pass:
- android.hardware.camera2.cts.StillCaptureTest#testFocalLengths
- android.hardware.camera2.cts.StillCaptureTest#testJpegExif

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
---
 src/android/camera_device.cpp            |  27 +++++-
 src/android/camera_device.h              |   5 +-
 src/android/camera_stream.cpp            |   7 +-
 src/android/camera_stream.h              |   4 +-
 src/android/jpeg/post_processor_jpeg.cpp | 116 +++++++++++++++++++----
 src/android/jpeg/post_processor_jpeg.h   |   5 +-
 src/android/jpeg/thumbnailer.h           |   1 +
 src/android/post_processor.h             |   3 +-
 8 files changed, 136 insertions(+), 32 deletions(-)

Hi Paul,

On Thu, Jan 14, 2021 at 07:40:35PM +0900, Paul Elder wrote:
> Set the following android result metadata:
> - ANDROID_LENS_FOCAL_LENGTH
> - ANDROID_LENS_APERTURE
> - ANDROID_JPEG_GPS_TIMESTAMP
> - ANDROID_JPEG_THUMBNAIL_SIZE
> - ANDROID_JPEG_THUMBNAIL_QUALITY
> - ANDROID_JPEG_GPS_COORDINATES
> - ANDROID_JPEG_GPS_PROCESSING_METHOD
> - ANDROID_JPEG_SIZE
> - ANDROID_JPEG_QUALITY
> - ANDROID_JPEG_ORIENTATION
>
> And the following EXIF fields:
> - GPSDatestamp
> - GPSTimestamp
> - GPSLocation
>   - GPSLatitudeRef
>   - GPSLatitude
>   - GPSLongitudeRef
>   - GPSLongitude
>   - GPSAltitudeRef
>   - GPSAltitude
> - GPSProcessingMethod
> - FocalLength
> - ExposureTime
> - FNumber
> - ISO
> - Flash
> - WhiteBalance
> - SubsecTime
> - SubsecTimeOriginal
> - SubsecTimeDigitized
>
> Based on android request metadata.
>
> This allows the following CTS tests to pass:
> - android.hardware.camera2.cts.StillCaptureTest#testFocalLengths
> - android.hardware.camera2.cts.StillCaptureTest#testJpegExif

\o/

>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
>  src/android/camera_device.cpp            |  27 +++++-
>  src/android/camera_device.h              |   5 +-
>  src/android/camera_stream.cpp            |   7 +-
>  src/android/camera_stream.h              |   4 +-
>  src/android/jpeg/post_processor_jpeg.cpp | 116 +++++++++++++++++++----
>  src/android/jpeg/post_processor_jpeg.h   |   5 +-
>  src/android/jpeg/thumbnailer.h           |   1 +
>  src/android/post_processor.h             |   3 +-
>  8 files changed, 136 insertions(+), 32 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index ed47c7cd..8d697080 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -296,8 +296,9 @@ MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,
>   */
>
>  CameraDevice::Camera3RequestDescriptor::Camera3RequestDescriptor(
> -	Camera *camera, unsigned int frameNumber, unsigned int numBuffers)
> -	: frameNumber_(frameNumber), numBuffers_(numBuffers)
> +	Camera *camera, unsigned int frameNumber, unsigned int numBuffers,
> +	CameraMetadata &settings)
> +	: frameNumber_(frameNumber), numBuffers_(numBuffers), settings_(settings)
>  {
>  	buffers_ = new camera3_stream_buffer_t[numBuffers];
>
> @@ -1700,9 +1701,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  	 * The descriptor and the associated memory reserved here are freed
>  	 * at request complete time.
>  	 */
> +	/* \todo Handle null request settings */
> +	CameraMetadata requestMetadata(camera3Request->settings);
>  	Camera3RequestDescriptor *descriptor =
>  		new Camera3RequestDescriptor(camera_.get(), camera3Request->frame_number,
> -					     camera3Request->num_output_buffers);
> +					     camera3Request->num_output_buffers,
> +					     requestMetadata);

As you use settings, I would name it settings here as well

>
>  	LOG(HAL, Debug) << "Queueing Request to libcamera with "
>  			<< descriptor->numBuffers_ << " HAL streams";
> @@ -1815,6 +1819,9 @@ void CameraDevice::requestComplete(Request *request)
>  		CameraStream *cameraStream =
>  			static_cast<CameraStream *>(descriptor->buffers_[i].stream->priv);
>
> +		float focal_length = 1.0;
> +		resultMetadata->addEntry(ANDROID_LENS_FOCAL_LENGTH, &focal_length, 1);
> +

Ups, result metadata are procesed in getResultMetadata() and if you
add an entry you need to expand the metadata pack size. Also.. see
below..

>  		if (cameraStream->camera3Stream().format != HAL_PIXEL_FORMAT_BLOB)
>  			continue;
>
> @@ -1837,6 +1844,7 @@ void CameraDevice::requestComplete(Request *request)
>  		}
>
>  		int ret = cameraStream->process(*buffer, &mapped,
> +						descriptor->settings_,

I would move:
1) Adding JPEG tags to EXIF
2) Filling in the result metadata

to 2 different patches

However, I'm a bit skeptic about this change.
You pass to the JPEG encode the settings the application has
requested. The actual process should go through the pipeline handler,
so that you should be able to find all the required information in
libcamera::Request::metadata()

This requires several parts:
1) Define a libcamera (draft) control to be able to translate the
Android metadata to a control the libcamera pipeline handler can
consume
2) Set the libcamera control in the Request's metadata
3) Parse the control value in the completion handler here.

You can see how (parts of) this process is handled for draft::PipelineDepth.

However, it's not trivial to make all the pieces fit together. I have
a series of patches I still have to send out that implement exactly
this for SCALER_CROP_REGION (except for the fact no ad-hoc property
needs to be defined, as we already have
libcamera::controls::ScalerCrop)

I'm not sure if we should either rush and take this big shortcut here
to provide the lens info to the post-processor, or rather hard-code it
there with a big \todo, or maybe wait until we close the loop.. I
would go for the second option, but I would like to know Laurent's
opinion here.

>  						resultMetadata.get());
>  		if (ret) {
>  			status = CAMERA3_BUFFER_STATUS_ERROR;
> @@ -1933,14 +1941,23 @@ CameraDevice::getResultMetadata(Camera3RequestDescriptor *descriptor,
>
>  	/*
>  	 * \todo Keep this in sync with the actual number of entries.
> -	 * Currently: 33 entries, 75 bytes
> +	 * Currently: 41 entries, 187 bytes
>  	 *
>  	 * Reserve more space for the JPEG metadata set by the post-processor.
>  	 * Currently: ANDROID_JPEG_SIZE (int32_t), ANDROID_JPEG_QUALITY (byte),
>  	 * ANDROID_JPEG_ORIENTATION (int32_t) = 3 entries, 9 bytes.

Can you move the additional JPEG sizes to the end ?

        = 10 entries, 92 bytes

However I count:
        41 - 33 = +8
        We have 10 JPEG metadata so I assume the additional one is for
        LENS_FOCAL_LENGTH you add here above

        187 - 75 = 112
        I count 92 bytes for JPEG + 4 for LENS_FOCAL_LENGHT

        Where do the additional 16 bytes come from ?

> +	 * ANDROID_JPEG_THUMBNAIL_SIZE (int32 x 2) = 8 bytes
> +	 * ANDROID_JPEG_THUMBNAIL_QUALITY (int32 x 2) = 8 bytes
> +	 * ANDROID_LENS_FOCAL_LENGTH (float) = 4 bytes
> +	 * ANDROID_JPEG_GPS_COORDINATES (double x 3) = 24 bytes
> +	 * ANDROID_JPEG_GPS_PROCESSING_METHOD (byte x 32) = 32 bytes
> +	 * ANDROID_JPEG_GPS_TIMESTAMP (int64) = 8 bytes
> +	 * ANDROID_LENS_APERTURE (float) = 4 bytes
> +	 *
> +	 * add coordinates again?


Not sure I get this todo :)

>  	 */
>  	std::unique_ptr<CameraMetadata> resultMetadata =
> -		std::make_unique<CameraMetadata>(33, 75);
> +		std::make_unique<CameraMetadata>(41, 187);
>  	if (!resultMetadata->isValid()) {
>  		LOG(HAL, Error) << "Failed to allocate static metadata";
>  		return nullptr;
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index a285d0a1..111a7d8f 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -24,6 +24,7 @@
>  #include "libcamera/internal/log.h"
>  #include "libcamera/internal/message.h"
>
> +#include "camera_metadata.h"
>  #include "camera_stream.h"
>  #include "camera_worker.h"
>  #include "jpeg/encoder.h"
> @@ -78,7 +79,8 @@ private:
>  	struct Camera3RequestDescriptor {
>  		Camera3RequestDescriptor(libcamera::Camera *camera,
>  					 unsigned int frameNumber,
> -					 unsigned int numBuffers);
> +					 unsigned int numBuffers,
> +					 CameraMetadata &settings);
>  		~Camera3RequestDescriptor();
>
>  		uint32_t frameNumber_;
> @@ -86,6 +88,7 @@ private:
>  		camera3_stream_buffer_t *buffers_;
>  		std::vector<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers_;
>  		std::unique_ptr<CaptureRequest> request_;
> +		CameraMetadata settings_;
>  	};
>
>  	struct Camera3StreamConfiguration {

The part below should go to a separate patch imho

> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> index dba351a4..4c8020e5 100644
> --- a/src/android/camera_stream.cpp
> +++ b/src/android/camera_stream.cpp
> @@ -96,12 +96,15 @@ int CameraStream::configure()
>  }
>
>  int CameraStream::process(const libcamera::FrameBuffer &source,
> -			  MappedCamera3Buffer *dest, CameraMetadata *metadata)
> +			  MappedCamera3Buffer *dest,
> +			  const CameraMetadata &requestMetadata,
> +			  CameraMetadata *resultMetadata)
>  {
>  	if (!postProcessor_)
>  		return 0;
>
> -	return postProcessor_->process(source, dest->maps()[0], metadata);
> +	return postProcessor_->process(source, dest->maps()[0],
> +				       requestMetadata, resultMetadata);
>  }
>
>  FrameBuffer *CameraStream::getBuffer()
> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> index cc9d5470..298ffbf6 100644
> --- a/src/android/camera_stream.h
> +++ b/src/android/camera_stream.h
> @@ -119,7 +119,9 @@ public:
>
>  	int configure();
>  	int process(const libcamera::FrameBuffer &source,
> -		    MappedCamera3Buffer *dest, CameraMetadata *metadata);
> +		    MappedCamera3Buffer *dest,
> +		    const CameraMetadata &requestMetadata,
> +		    CameraMetadata *resultMetadata);
>  	libcamera::FrameBuffer *getBuffer();
>  	void putBuffer(libcamera::FrameBuffer *buffer);
>
> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> index 436a50f8..13ad3777 100644
> --- a/src/android/jpeg/post_processor_jpeg.cpp
> +++ b/src/android/jpeg/post_processor_jpeg.cpp
> @@ -25,6 +25,21 @@ PostProcessorJpeg::PostProcessorJpeg(CameraDevice *const device)
>  {
>  }
>
> +int PostProcessorJpeg::configureThumbnailer(const Size &size,
> +					    const PixelFormat &pixelFormat)
> +{
> +	thumbnailer_.configure(size, pixelFormat);
> +	StreamConfiguration thCfg;
> +	thCfg.size = thumbnailer_.size();
> +	thCfg.pixelFormat = pixelFormat;
> +	if (thumbnailEncoder_.configure(thCfg) != 0) {
> +		LOG(JPEG, Error) << "Failed to configure thumbnail encoder";
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,
>  				 const StreamConfiguration &outCfg)
>  {
> @@ -40,16 +55,11 @@ int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,
>
>  	streamSize_ = outCfg.size;
>
> -	thumbnailer_.configure(inCfg.size, inCfg.pixelFormat);
> -	StreamConfiguration thCfg = inCfg;
> -	thCfg.size = thumbnailer_.size();
> -	if (thumbnailEncoder_.configure(thCfg) != 0) {
> -		LOG(JPEG, Error) << "Failed to configure thumbnail encoder";
> -		return -EINVAL;
> -	}
> +	int ret = configureThumbnailer(inCfg.size, inCfg.pixelFormat);
> +	if (ret)
> +		return ret;
>
>  	encoder_ = std::make_unique<EncoderLibJpeg>();
> -
>  	return encoder_->configure(inCfg);

Unrelated changes ? Should they go to a third patch ?

I'll review the rest once separated to a new patch

Thanks
   j

>  }
>
> @@ -80,17 +90,22 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,
>
>  int PostProcessorJpeg::process(const FrameBuffer &source,
>  			       Span<uint8_t> destination,
> -			       CameraMetadata *metadata)
> +			       const CameraMetadata &requestMetadata,
> +			       CameraMetadata *resultMetadata)
>  {
>  	if (!encoder_)
>  		return 0;
>
> +	camera_metadata_ro_entry_t entry;
> +	int ret;
> +
>  	/* 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.setMake(cameraDevice_->cameraMake());
> +	exif.setModel(cameraDevice_->cameraModel());
> +
> +	ret = requestMetadata.getEntry(ANDROID_JPEG_ORIENTATION, &entry);

If you intend to handle the ANDROID_JPEG_ORIENTATION control (do you
need this?) I think you should combine it with the camera orientation.

> +	exif.setOrientation(ret ? cameraDevice_->orientation() : *entry.data.i32);
>  	exif.setSize(streamSize_);
>  	/*
>  	 * We set the frame's EXIF timestamp as the time of encode.
> @@ -99,11 +114,68 @@ int PostProcessorJpeg::process(const FrameBuffer &source,
>  	 */
>  	exif.setTimestamp(std::time(nullptr));
>
> -	std::vector<unsigned char> thumbnail;
> -	generateThumbnail(source, &thumbnail);
> -	if (!thumbnail.empty())
> +	exif.setExposureTime(0);
> +	ret = requestMetadata.getEntry(ANDROID_LENS_APERTURE, &entry);
> +	if (!ret) {
> +		exif.setAperture(*entry.data.f);
> +		resultMetadata->addEntry(ANDROID_LENS_APERTURE, entry.data.f, 1);
> +	}
> +	exif.setISO(100);
> +	exif.setFlash(Exif::Flash::FlashNotPresent);
> +	exif.setWhiteBalance(Exif::WhiteBalance::Auto);
> +
> +	exif.setSubsecTime(0);
> +	exif.setSubsecTimeOriginal(0);
> +	exif.setSubsecTimeDigitized(0);
> +
> +	float focal_length = 1.0;
> +	exif.setFocalLength(focal_length);
> +
> +	ret = requestMetadata.getEntry(ANDROID_JPEG_GPS_TIMESTAMP, &entry);
> +	if (!ret) {
> +		exif.setGPSDateTimestamp(*entry.data.i64);
> +		resultMetadata->addEntry(ANDROID_JPEG_GPS_TIMESTAMP,
> +					 entry.data.i64, 1);
> +	}
> +
> +	ret = requestMetadata.getEntry(ANDROID_JPEG_THUMBNAIL_SIZE, &entry);
> +	if (!ret) {
> +		/* \todo Handle non-matching aspect ratios */
> +		/* \todo Handle non-zero orientations */
> +		const int32_t *data = entry.data.i32;
> +		Size thumbnailSize = { static_cast<uint32_t>(data[0]),
> +				       static_cast<uint32_t>(data[1]) };
> +
> +		std::vector<unsigned char> thumbnail;
> +		if (thumbnailSize != Size(0, 0)) {
> +			configureThumbnailer(thumbnailSize, thumbnailer_.pixelFormat());
> +			generateThumbnail(source, &thumbnail);
> +		}
>  		exif.setThumbnail(thumbnail, Exif::Compression::JPEG);
>
> +		resultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE, data, 2);
> +
> +		/* \todo Use this quality as a parameter to the encoder */
> +		ret = requestMetadata.getEntry(ANDROID_JPEG_THUMBNAIL_QUALITY, &entry);
> +		if (!ret)
> +			resultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_QUALITY, entry.data.u8, 1);
> +	}
> +
> +	ret = requestMetadata.getEntry(ANDROID_JPEG_GPS_COORDINATES, &entry);
> +	if (!ret) {
> +		exif.setGPSLocation(entry.data.d);
> +		resultMetadata->addEntry(ANDROID_JPEG_GPS_COORDINATES,
> +					 entry.data.d, 3);
> +	}
> +
> +	ret = requestMetadata.getEntry(ANDROID_JPEG_GPS_PROCESSING_METHOD, &entry);
> +	if (!ret) {
> +		std::string method(entry.data.u8, entry.data.u8 + 32);
> +		exif.setGPSMethod(method);
> +		resultMetadata->addEntry(ANDROID_JPEG_GPS_PROCESSING_METHOD,
> +					 entry.data.u8, 32);
> +	}
> +
>  	if (exif.generate() != 0)
>  		LOG(JPEG, Error) << "Failed to generate valid EXIF data";
>
> @@ -133,13 +205,15 @@ int PostProcessorJpeg::process(const FrameBuffer &source,
>  	blob->jpeg_size = jpeg_size;
>
>  	/* Update the JPEG result Metadata. */
> -	metadata->addEntry(ANDROID_JPEG_SIZE, &jpeg_size, 1);
> +	resultMetadata->addEntry(ANDROID_JPEG_SIZE, &jpeg_size, 1);
>
> -	const uint32_t jpeg_quality = 95;
> -	metadata->addEntry(ANDROID_JPEG_QUALITY, &jpeg_quality, 1);
> +	ret = requestMetadata.getEntry(ANDROID_JPEG_QUALITY, &entry);
> +	const uint32_t jpeg_quality = ret ? 95 : *entry.data.u8;
> +	resultMetadata->addEntry(ANDROID_JPEG_QUALITY, &jpeg_quality, 1);
>
> -	const uint32_t jpeg_orientation = 0;
> -	metadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpeg_orientation, 1);
> +	ret = requestMetadata.getEntry(ANDROID_JPEG_ORIENTATION, &entry);
> +	const uint32_t jpeg_orientation = ret ? 0 : *entry.data.i32;
> +	resultMetadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpeg_orientation, 1);
>
>  	return 0;
>  }
> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h
> index 5afa831c..d07c9fe5 100644
> --- a/src/android/jpeg/post_processor_jpeg.h
> +++ b/src/android/jpeg/post_processor_jpeg.h
> @@ -26,11 +26,14 @@ public:
>  		      const libcamera::StreamConfiguration &outcfg) override;
>  	int process(const libcamera::FrameBuffer &source,
>  		    libcamera::Span<uint8_t> destination,
> -		    CameraMetadata *metadata) override;
> +		    const CameraMetadata &requestMetadata,
> +		    CameraMetadata *resultMetadata) override;
>
>  private:
>  	void generateThumbnail(const libcamera::FrameBuffer &source,
>  			       std::vector<unsigned char> *thumbnail);
> +	int configureThumbnailer(const libcamera::Size &size,
> +				 const libcamera::PixelFormat &pixelFormat);
>
>  	CameraDevice *const cameraDevice_;
>  	std::unique_ptr<Encoder> encoder_;
> diff --git a/src/android/jpeg/thumbnailer.h b/src/android/jpeg/thumbnailer.h
> index 98f11833..f393db47 100644
> --- a/src/android/jpeg/thumbnailer.h
> +++ b/src/android/jpeg/thumbnailer.h
> @@ -22,6 +22,7 @@ public:
>  	void createThumbnail(const libcamera::FrameBuffer &source,
>  			     std::vector<unsigned char> *dest);
>  	const libcamera::Size &size() const { return targetSize_; }
> +	const libcamera::PixelFormat &pixelFormat() const { return pixelFormat_; }
>
>  private:
>  	libcamera::Size computeThumbnailSize() const;
> diff --git a/src/android/post_processor.h b/src/android/post_processor.h
> index e0f91880..bda93bb4 100644
> --- a/src/android/post_processor.h
> +++ b/src/android/post_processor.h
> @@ -22,7 +22,8 @@ public:
>  			      const libcamera::StreamConfiguration &outCfg) = 0;
>  	virtual int process(const libcamera::FrameBuffer &source,
>  			    libcamera::Span<uint8_t> destination,
> -			    CameraMetadata *metadata) = 0;
> +			    const CameraMetadata &requestMetadata,
> +			    CameraMetadata *resultMetadata) = 0;
>  };
>
>  #endif /* __ANDROID_POST_PROCESSOR_H__ */
> --
> 2.27.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Paul,

Thank you for the patch.

On Fri, Jan 15, 2021 at 04:48:12PM +0100, Jacopo Mondi wrote:
> On Thu, Jan 14, 2021 at 07:40:35PM +0900, Paul Elder wrote:
> > Set the following android result metadata:
> > - ANDROID_LENS_FOCAL_LENGTH
> > - ANDROID_LENS_APERTURE
> > - ANDROID_JPEG_GPS_TIMESTAMP
> > - ANDROID_JPEG_THUMBNAIL_SIZE
> > - ANDROID_JPEG_THUMBNAIL_QUALITY
> > - ANDROID_JPEG_GPS_COORDINATES
> > - ANDROID_JPEG_GPS_PROCESSING_METHOD
> > - ANDROID_JPEG_SIZE
> > - ANDROID_JPEG_QUALITY
> > - ANDROID_JPEG_ORIENTATION
> >
> > And the following EXIF fields:
> > - GPSDatestamp
> > - GPSTimestamp
> > - GPSLocation
> >   - GPSLatitudeRef
> >   - GPSLatitude
> >   - GPSLongitudeRef
> >   - GPSLongitude
> >   - GPSAltitudeRef
> >   - GPSAltitude
> > - GPSProcessingMethod
> > - FocalLength
> > - ExposureTime
> > - FNumber
> > - ISO
> > - Flash
> > - WhiteBalance
> > - SubsecTime
> > - SubsecTimeOriginal
> > - SubsecTimeDigitized
> >
> > Based on android request metadata.

Can you split this in multiple patches ? Review is difficult. You can
have one patch that sets all the result metadata and Exif tags that do
not interact with any other component, and one patch per change for all
other changes. For instance ANDROID_JPEG_THUMBNAIL_SIZE requires
configuring the thumbnailer accordingly, and should be in a patch of its
own.

> >
> > This allows the following CTS tests to pass:
> > - android.hardware.camera2.cts.StillCaptureTest#testFocalLengths
> > - android.hardware.camera2.cts.StillCaptureTest#testJpegExif
>
> \o/
>
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > ---
> >  src/android/camera_device.cpp            |  27 +++++-
> >  src/android/camera_device.h              |   5 +-
> >  src/android/camera_stream.cpp            |   7 +-
> >  src/android/camera_stream.h              |   4 +-
> >  src/android/jpeg/post_processor_jpeg.cpp | 116 +++++++++++++++++++----
> >  src/android/jpeg/post_processor_jpeg.h   |   5 +-
> >  src/android/jpeg/thumbnailer.h           |   1 +
> >  src/android/post_processor.h             |   3 +-
> >  8 files changed, 136 insertions(+), 32 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index ed47c7cd..8d697080 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -296,8 +296,9 @@ MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,
> >   */
> >
> >  CameraDevice::Camera3RequestDescriptor::Camera3RequestDescriptor(
> > -	Camera *camera, unsigned int frameNumber, unsigned int numBuffers)
> > -	: frameNumber_(frameNumber), numBuffers_(numBuffers)
> > +	Camera *camera, unsigned int frameNumber, unsigned int numBuffers,
> > +	CameraMetadata &settings)
> > +	: frameNumber_(frameNumber), numBuffers_(numBuffers), settings_(settings)
> >  {
> >  	buffers_ = new camera3_stream_buffer_t[numBuffers];
> >
> > @@ -1700,9 +1701,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >  	 * The descriptor and the associated memory reserved here are freed
> >  	 * at request complete time.
> >  	 */
> > +	/* \todo Handle null request settings */

Can it happen ? If so it should be handled as part of this series or
immediately on top, not left for later.

> > +	CameraMetadata requestMetadata(camera3Request->settings);
> >  	Camera3RequestDescriptor *descriptor =
> >  		new Camera3RequestDescriptor(camera_.get(), camera3Request->frame_number,
> > -					     camera3Request->num_output_buffers);
> > +					     camera3Request->num_output_buffers,
> > +					     requestMetadata);
>
> As you use settings, I would name it settings here as well
>
> >
> >  	LOG(HAL, Debug) << "Queueing Request to libcamera with "
> >  			<< descriptor->numBuffers_ << " HAL streams";
> > @@ -1815,6 +1819,9 @@ void CameraDevice::requestComplete(Request *request)
> >  		CameraStream *cameraStream =
> >  			static_cast<CameraStream *>(descriptor->buffers_[i].stream->priv);
> >
> > +		float focal_length = 1.0;
> > +		resultMetadata->addEntry(ANDROID_LENS_FOCAL_LENGTH, &focal_length, 1);
> > +
>
> Ups, result metadata are procesed in getResultMetadata() and if you
> add an entry you need to expand the metadata pack size. Also.. see
> below..
>
> >  		if (cameraStream->camera3Stream().format != HAL_PIXEL_FORMAT_BLOB)
> >  			continue;
> >
> > @@ -1837,6 +1844,7 @@ void CameraDevice::requestComplete(Request *request)
> >  		}
> >
> >  		int ret = cameraStream->process(*buffer, &mapped,
> > +						descriptor->settings_,
>
> I would move:
> 1) Adding JPEG tags to EXIF
> 2) Filling in the result metadata
>
> to 2 different patches
>
> However, I'm a bit skeptic about this change.
> You pass to the JPEG encode the settings the application has
> requested. The actual process should go through the pipeline handler,
> so that you should be able to find all the required information in
> libcamera::Request::metadata()
>
> This requires several parts:
> 1) Define a libcamera (draft) control to be able to translate the
> Android metadata to a control the libcamera pipeline handler can
> consume
> 2) Set the libcamera control in the Request's metadata
> 3) Parse the control value in the completion handler here.
>
> You can see how (parts of) this process is handled for draft::PipelineDepth.
>
> However, it's not trivial to make all the pieces fit together. I have
> a series of patches I still have to send out that implement exactly
> this for SCALER_CROP_REGION (except for the fact no ad-hoc property
> needs to be defined, as we already have
> libcamera::controls::ScalerCrop)
>
> I'm not sure if we should either rush and take this big shortcut here
> to provide the lens info to the post-processor, or rather hard-code it
> there with a big \todo, or maybe wait until we close the loop.. I
> would go for the second option, but I would like to know Laurent's
> opinion here.

There are Android metadata that are meant only for the HAL, such as all
the JPEG-related information as we don't handle JPEG encoding in
libcamera. Passing the request settings to the post-processor does make
sense.

Looking at the code below, the only metadata entry that needs to be
handled by libcamera is the lens aperture. Given that we support a
single aperture at the moment (we report a single entry in
ANDROID_LENS_INFO_AVAILABLE_APERTURES), hardcoding ANDROID_LENS_APERTURE
makes sense, we don't need to define libcamera controls yet. However,
setting ANDROID_LENS_APERTURE doesn't belong to this patch (see below
for more comments).

Same for ANDROID_LENS_FOCAL_LENGTH, we can hardcode it for now as we
have a fixed focus lens, but it should go to getResultMetadata() as
you've mentioned.

> >  						resultMetadata.get());
> >  		if (ret) {
> >  			status = CAMERA3_BUFFER_STATUS_ERROR;
> > @@ -1933,14 +1941,23 @@ CameraDevice::getResultMetadata(Camera3RequestDescriptor *descriptor,
> >
> >  	/*
> >  	 * \todo Keep this in sync with the actual number of entries.
> > -	 * Currently: 33 entries, 75 bytes
> > +	 * Currently: 41 entries, 187 bytes
> >  	 *
> >  	 * Reserve more space for the JPEG metadata set by the post-processor.
> >  	 * Currently: ANDROID_JPEG_SIZE (int32_t), ANDROID_JPEG_QUALITY (byte),
> >  	 * ANDROID_JPEG_ORIENTATION (int32_t) = 3 entries, 9 bytes.
>
> Can you move the additional JPEG sizes to the end ?
>
>         = 10 entries, 92 bytes
>
> However I count:
>         41 - 33 = +8
>         We have 10 JPEG metadata so I assume the additional one is for
>         LENS_FOCAL_LENGTH you add here above
>
>         187 - 75 = 112
>         I count 92 bytes for JPEG + 4 for LENS_FOCAL_LENGHT
>
>         Where do the additional 16 bytes come from ?
>
> > +	 * ANDROID_JPEG_THUMBNAIL_SIZE (int32 x 2) = 8 bytes
> > +	 * ANDROID_JPEG_THUMBNAIL_QUALITY (int32 x 2) = 8 bytes
> > +	 * ANDROID_LENS_FOCAL_LENGTH (float) = 4 bytes
> > +	 * ANDROID_JPEG_GPS_COORDINATES (double x 3) = 24 bytes
> > +	 * ANDROID_JPEG_GPS_PROCESSING_METHOD (byte x 32) = 32 bytes
> > +	 * ANDROID_JPEG_GPS_TIMESTAMP (int64) = 8 bytes
> > +	 * ANDROID_LENS_APERTURE (float) = 4 bytes
> > +	 *
> > +	 * add coordinates again?
>
> Not sure I get this todo :)
>
> >  	 */
> >  	std::unique_ptr<CameraMetadata> resultMetadata =
> > -		std::make_unique<CameraMetadata>(33, 75);
> > +		std::make_unique<CameraMetadata>(41, 187);
> >  	if (!resultMetadata->isValid()) {
> >  		LOG(HAL, Error) << "Failed to allocate static metadata";
> >  		return nullptr;
> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > index a285d0a1..111a7d8f 100644
> > --- a/src/android/camera_device.h
> > +++ b/src/android/camera_device.h
> > @@ -24,6 +24,7 @@
> >  #include "libcamera/internal/log.h"
> >  #include "libcamera/internal/message.h"
> >
> > +#include "camera_metadata.h"
> >  #include "camera_stream.h"
> >  #include "camera_worker.h"
> >  #include "jpeg/encoder.h"
> > @@ -78,7 +79,8 @@ private:
> >  	struct Camera3RequestDescriptor {
> >  		Camera3RequestDescriptor(libcamera::Camera *camera,
> >  					 unsigned int frameNumber,
> > -					 unsigned int numBuffers);
> > +					 unsigned int numBuffers,
> > +					 CameraMetadata &settings);
> >  		~Camera3RequestDescriptor();
> >
> >  		uint32_t frameNumber_;
> > @@ -86,6 +88,7 @@ private:
> >  		camera3_stream_buffer_t *buffers_;
> >  		std::vector<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers_;
> >  		std::unique_ptr<CaptureRequest> request_;
> > +		CameraMetadata settings_;
> >  	};
> >
> >  	struct Camera3StreamConfiguration {
>
> The part below should go to a separate patch imho
>
> > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> > index dba351a4..4c8020e5 100644
> > --- a/src/android/camera_stream.cpp
> > +++ b/src/android/camera_stream.cpp
> > @@ -96,12 +96,15 @@ int CameraStream::configure()
> >  }
> >
> >  int CameraStream::process(const libcamera::FrameBuffer &source,
> > -			  MappedCamera3Buffer *dest, CameraMetadata *metadata)
> > +			  MappedCamera3Buffer *dest,
> > +			  const CameraMetadata &requestMetadata,
> > +			  CameraMetadata *resultMetadata)
> >  {
> >  	if (!postProcessor_)
> >  		return 0;
> >
> > -	return postProcessor_->process(source, dest->maps()[0], metadata);
> > +	return postProcessor_->process(source, dest->maps()[0],
> > +				       requestMetadata, resultMetadata);
> >  }
> >
> >  FrameBuffer *CameraStream::getBuffer()
> > diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> > index cc9d5470..298ffbf6 100644
> > --- a/src/android/camera_stream.h
> > +++ b/src/android/camera_stream.h
> > @@ -119,7 +119,9 @@ public:
> >
> >  	int configure();
> >  	int process(const libcamera::FrameBuffer &source,
> > -		    MappedCamera3Buffer *dest, CameraMetadata *metadata);
> > +		    MappedCamera3Buffer *dest,
> > +		    const CameraMetadata &requestMetadata,
> > +		    CameraMetadata *resultMetadata);
> >  	libcamera::FrameBuffer *getBuffer();
> >  	void putBuffer(libcamera::FrameBuffer *buffer);
> >
> > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> > index 436a50f8..13ad3777 100644
> > --- a/src/android/jpeg/post_processor_jpeg.cpp
> > +++ b/src/android/jpeg/post_processor_jpeg.cpp
> > @@ -25,6 +25,21 @@ PostProcessorJpeg::PostProcessorJpeg(CameraDevice *const device)
> >  {
> >  }
> >
> > +int PostProcessorJpeg::configureThumbnailer(const Size &size,
> > +					    const PixelFormat &pixelFormat)
> > +{
> > +	thumbnailer_.configure(size, pixelFormat);
> > +	StreamConfiguration thCfg;
> > +	thCfg.size = thumbnailer_.size();
> > +	thCfg.pixelFormat = pixelFormat;
> > +	if (thumbnailEncoder_.configure(thCfg) != 0) {
> > +		LOG(JPEG, Error) << "Failed to configure thumbnail encoder";
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,
> >  				 const StreamConfiguration &outCfg)
> >  {
> > @@ -40,16 +55,11 @@ int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,
> >
> >  	streamSize_ = outCfg.size;
> >
> > -	thumbnailer_.configure(inCfg.size, inCfg.pixelFormat);
> > -	StreamConfiguration thCfg = inCfg;
> > -	thCfg.size = thumbnailer_.size();
> > -	if (thumbnailEncoder_.configure(thCfg) != 0) {
> > -		LOG(JPEG, Error) << "Failed to configure thumbnail encoder";
> > -		return -EINVAL;
> > -	}
> > +	int ret = configureThumbnailer(inCfg.size, inCfg.pixelFormat);
> > +	if (ret)
> > +		return ret;
> >
> >  	encoder_ = std::make_unique<EncoderLibJpeg>();
> > -
> >  	return encoder_->configure(inCfg);
>
> Unrelated changes ? Should they go to a third patch ?

The introduction of PostProcessorJpeg::configureThumbnailer(), if
desired, should be split to a patch of its own, yes.

> I'll review the rest once separated to a new patch
>
> >  }
> >
> > @@ -80,17 +90,22 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,
> >
> >  int PostProcessorJpeg::process(const FrameBuffer &source,
> >  			       Span<uint8_t> destination,
> > -			       CameraMetadata *metadata)
> > +			       const CameraMetadata &requestMetadata,
> > +			       CameraMetadata *resultMetadata)
> >  {
> >  	if (!encoder_)
> >  		return 0;
> >
> > +	camera_metadata_ro_entry_t entry;
> > +	int ret;
> > +
> >  	/* 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.setMake(cameraDevice_->cameraMake());
> > +	exif.setModel(cameraDevice_->cameraModel());
> > +
> > +	ret = requestMetadata.getEntry(ANDROID_JPEG_ORIENTATION, &entry);
>
> If you intend to handle the ANDROID_JPEG_ORIENTATION control (do you
> need this?) I think you should combine it with the camera orientation.
>
> > +	exif.setOrientation(ret ? cameraDevice_->orientation() : *entry.data.i32);
> >  	exif.setSize(streamSize_);
> >  	/*
> >  	 * We set the frame's EXIF timestamp as the time of encode.
> > @@ -99,11 +114,68 @@ int PostProcessorJpeg::process(const FrameBuffer &source,
> >  	 */
> >  	exif.setTimestamp(std::time(nullptr));
> >
> > -	std::vector<unsigned char> thumbnail;
> > -	generateThumbnail(source, &thumbnail);
> > -	if (!thumbnail.empty())
> > +	exif.setExposureTime(0);
> > +	ret = requestMetadata.getEntry(ANDROID_LENS_APERTURE, &entry);
> > +	if (!ret) {
> > +		exif.setAperture(*entry.data.f);
> > +		resultMetadata->addEntry(ANDROID_LENS_APERTURE, entry.data.f, 1);

This doesn't belong to PostProcessorJpeg::process(), only
exif.setAperture() belongs here. ANDROID_LENS_APERTURE should be set in
CameraDevice::getResultMetadata().

> > +	}
> > +	exif.setISO(100);
> > +	exif.setFlash(Exif::Flash::FlashNotPresent);
> > +	exif.setWhiteBalance(Exif::WhiteBalance::Auto);
> > +
> > +	exif.setSubsecTime(0);
> > +	exif.setSubsecTimeOriginal(0);
> > +	exif.setSubsecTimeDigitized(0);
> > +
> > +	float focal_length = 1.0;
> > +	exif.setFocalLength(focal_length);

	exif.setFocalLength(1.0);

> > +
> > +	ret = requestMetadata.getEntry(ANDROID_JPEG_GPS_TIMESTAMP, &entry);
> > +	if (!ret) {
> > +		exif.setGPSDateTimestamp(*entry.data.i64);
> > +		resultMetadata->addEntry(ANDROID_JPEG_GPS_TIMESTAMP,
> > +					 entry.data.i64, 1);
> > +	}
> > +

As part as the rework of the thumbnail generation, you need to report
additional sizes in ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES.

> > +	ret = requestMetadata.getEntry(ANDROID_JPEG_THUMBNAIL_SIZE, &entry);
> > +	if (!ret) {
> > +		/* \todo Handle non-matching aspect ratios */
> > +		/* \todo Handle non-zero orientations */

Is this required ? As we handle the orientation by setting it in the
Exif tags, we don't need to rotate the thumbnail, is there anything else
to handle ?

> > +		const int32_t *data = entry.data.i32;
> > +		Size thumbnailSize = { static_cast<uint32_t>(data[0]),
> > +				       static_cast<uint32_t>(data[1]) };
> > +
> > +		std::vector<unsigned char> thumbnail;
> > +		if (thumbnailSize != Size(0, 0)) {
> > +			configureThumbnailer(thumbnailSize, thumbnailer_.pixelFormat());
> > +			generateThumbnail(source, &thumbnail);
> > +		}
> >  		exif.setThumbnail(thumbnail, Exif::Compression::JPEG);

You shouldn't set a thumbnail at all if the size is { 0, 0 }.

> >
> > +		resultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE, data, 2);
> > +
> > +		/* \todo Use this quality as a parameter to the encoder */
> > +		ret = requestMetadata.getEntry(ANDROID_JPEG_THUMBNAIL_QUALITY, &entry);
> > +		if (!ret)
> > +			resultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_QUALITY, entry.data.u8, 1);
> > +	}
> > +
> > +	ret = requestMetadata.getEntry(ANDROID_JPEG_GPS_COORDINATES, &entry);
> > +	if (!ret) {
> > +		exif.setGPSLocation(entry.data.d);
> > +		resultMetadata->addEntry(ANDROID_JPEG_GPS_COORDINATES,
> > +					 entry.data.d, 3);
> > +	}
> > +
> > +	ret = requestMetadata.getEntry(ANDROID_JPEG_GPS_PROCESSING_METHOD, &entry);
> > +	if (!ret) {
> > +		std::string method(entry.data.u8, entry.data.u8 + 32);
> > +		exif.setGPSMethod(method);
> > +		resultMetadata->addEntry(ANDROID_JPEG_GPS_PROCESSING_METHOD,
> > +					 entry.data.u8, 32);
> > +	}
> > +
> >  	if (exif.generate() != 0)
> >  		LOG(JPEG, Error) << "Failed to generate valid EXIF data";
> >
> > @@ -133,13 +205,15 @@ int PostProcessorJpeg::process(const FrameBuffer &source,
> >  	blob->jpeg_size = jpeg_size;
> >
> >  	/* Update the JPEG result Metadata. */
> > -	metadata->addEntry(ANDROID_JPEG_SIZE, &jpeg_size, 1);
> > +	resultMetadata->addEntry(ANDROID_JPEG_SIZE, &jpeg_size, 1);
> >
> > -	const uint32_t jpeg_quality = 95;
> > -	metadata->addEntry(ANDROID_JPEG_QUALITY, &jpeg_quality, 1);
> > +	ret = requestMetadata.getEntry(ANDROID_JPEG_QUALITY, &entry);
> > +	const uint32_t jpeg_quality = ret ? 95 : *entry.data.u8;
> > +	resultMetadata->addEntry(ANDROID_JPEG_QUALITY, &jpeg_quality, 1);

If we start acting on ANDROID_JPEG_QUALITY, we should really take it
into account and configure the JPEG encoder accordingly. This can be
done on top.

> >
> > -	const uint32_t jpeg_orientation = 0;
> > -	metadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpeg_orientation, 1);
> > +	ret = requestMetadata.getEntry(ANDROID_JPEG_ORIENTATION, &entry);
> > +	const uint32_t jpeg_orientation = ret ? 0 : *entry.data.i32;
> > +	resultMetadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpeg_orientation, 1);
> >
> >  	return 0;
> >  }
> > diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h
> > index 5afa831c..d07c9fe5 100644
> > --- a/src/android/jpeg/post_processor_jpeg.h
> > +++ b/src/android/jpeg/post_processor_jpeg.h
> > @@ -26,11 +26,14 @@ public:
> >  		      const libcamera::StreamConfiguration &outcfg) override;
> >  	int process(const libcamera::FrameBuffer &source,
> >  		    libcamera::Span<uint8_t> destination,
> > -		    CameraMetadata *metadata) override;
> > +		    const CameraMetadata &requestMetadata,
> > +		    CameraMetadata *resultMetadata) override;

I'd split this change and the similar one below to a separate patch, it
will make this patch easier to review.

> >
> >  private:
> >  	void generateThumbnail(const libcamera::FrameBuffer &source,
> >  			       std::vector<unsigned char> *thumbnail);
> > +	int configureThumbnailer(const libcamera::Size &size,
> > +				 const libcamera::PixelFormat &pixelFormat);
> >
> >  	CameraDevice *const cameraDevice_;
> >  	std::unique_ptr<Encoder> encoder_;
> > diff --git a/src/android/jpeg/thumbnailer.h b/src/android/jpeg/thumbnailer.h
> > index 98f11833..f393db47 100644
> > --- a/src/android/jpeg/thumbnailer.h
> > +++ b/src/android/jpeg/thumbnailer.h
> > @@ -22,6 +22,7 @@ public:
> >  	void createThumbnail(const libcamera::FrameBuffer &source,
> >  			     std::vector<unsigned char> *dest);
> >  	const libcamera::Size &size() const { return targetSize_; }
> > +	const libcamera::PixelFormat &pixelFormat() const { return pixelFormat_; }
> >
> >  private:
> >  	libcamera::Size computeThumbnailSize() const;
> > diff --git a/src/android/post_processor.h b/src/android/post_processor.h
> > index e0f91880..bda93bb4 100644
> > --- a/src/android/post_processor.h
> > +++ b/src/android/post_processor.h
> > @@ -22,7 +22,8 @@ public:
> >  			      const libcamera::StreamConfiguration &outCfg) = 0;
> >  	virtual int process(const libcamera::FrameBuffer &source,
> >  			    libcamera::Span<uint8_t> destination,
> > -			    CameraMetadata *metadata) = 0;
> > +			    const CameraMetadata &requestMetadata,
> > +			    CameraMetadata *resultMetadata) = 0;
> >  };
> >
> >  #endif /* __ANDROID_POST_PROCESSOR_H__ */

--
Regards,

Laurent Pinchart

Hi Laurent,

On Fri, Jan 15, 2021 at 09:38:24PM +0200, Laurent Pinchart wrote:
> Hi Paul,
>
> Thank you for the patch.
>
> > >  		int ret = cameraStream->process(*buffer, &mapped,
> > > +						descriptor->settings_,
> >
> > I would move:
> > 1) Adding JPEG tags to EXIF
> > 2) Filling in the result metadata
> >
> > to 2 different patches
> >
> > However, I'm a bit skeptic about this change.
> > You pass to the JPEG encode the settings the application has
> > requested. The actual process should go through the pipeline handler,
> > so that you should be able to find all the required information in
> > libcamera::Request::metadata()
> >
> > This requires several parts:
> > 1) Define a libcamera (draft) control to be able to translate the
> > Android metadata to a control the libcamera pipeline handler can
> > consume
> > 2) Set the libcamera control in the Request's metadata
> > 3) Parse the control value in the completion handler here.
> >
> > You can see how (parts of) this process is handled for draft::PipelineDepth.
> >
> > However, it's not trivial to make all the pieces fit together. I have
> > a series of patches I still have to send out that implement exactly
> > this for SCALER_CROP_REGION (except for the fact no ad-hoc property
> > needs to be defined, as we already have
> > libcamera::controls::ScalerCrop)
> >
> > I'm not sure if we should either rush and take this big shortcut here
> > to provide the lens info to the post-processor, or rather hard-code it
> > there with a big \todo, or maybe wait until we close the loop.. I
> > would go for the second option, but I would like to know Laurent's
> > opinion here.
>
> There are Android metadata that are meant only for the HAL, such as all
> the JPEG-related information as we don't handle JPEG encoding in
> libcamera. Passing the request settings to the post-processor does make
> sense.
>
> Looking at the code below, the only metadata entry that needs to be
> handled by libcamera is the lens aperture. Given that we support a
> single aperture at the moment (we report a single entry in
> ANDROID_LENS_INFO_AVAILABLE_APERTURES), hardcoding ANDROID_LENS_APERTURE
> makes sense, we don't need to define libcamera controls yet. However,
> setting ANDROID_LENS_APERTURE doesn't belong to this patch (see below
> for more comments).
>
> Same for ANDROID_LENS_FOCAL_LENGTH, we can hardcode it for now as we
> have a fixed focus lens, but it should go to getResultMetadata() as
> you've mentioned.
>

Agreed with the fixed values.

> > >  						resultMetadata.get());
> > >  		if (ret) {
> > >  			status = CAMERA3_BUFFER_STATUS_ERROR;
> > > @@ -1933,14 +1941,23 @@ CameraDevice::getResultMetadata(Camera3RequestDescriptor *descriptor,
> > >
> > >  	/*
> > >  	 * \todo Keep this in sync with the actual number of entries.
> > > -	 * Currently: 33 entries, 75 bytes
> > > +	 * Currently: 41 entries, 187 bytes
> > >  	 *
> > >  	 * Reserve more space for the JPEG metadata set by the post-processor.
> > >  	 * Currently: ANDROID_JPEG_SIZE (int32_t), ANDROID_JPEG_QUALITY (byte),
> > >  	 * ANDROID_JPEG_ORIENTATION (int32_t) = 3 entries, 9 bytes.
> >
> > Can you move the additional JPEG sizes to the end ?
> >
> >         = 10 entries, 92 bytes
> >
> > However I count:
> >         41 - 33 = +8
> >         We have 10 JPEG metadata so I assume the additional one is for
> >         LENS_FOCAL_LENGTH you add here above
> >
> >         187 - 75 = 112
> >         I count 92 bytes for JPEG + 4 for LENS_FOCAL_LENGHT
> >
> >         Where do the additional 16 bytes come from ?
> >
> > > +	 * ANDROID_JPEG_THUMBNAIL_SIZE (int32 x 2) = 8 bytes
> > > +	 * ANDROID_JPEG_THUMBNAIL_QUALITY (int32 x 2) = 8 bytes
> > > +	 * ANDROID_LENS_FOCAL_LENGTH (float) = 4 bytes
> > > +	 * ANDROID_JPEG_GPS_COORDINATES (double x 3) = 24 bytes
> > > +	 * ANDROID_JPEG_GPS_PROCESSING_METHOD (byte x 32) = 32 bytes
> > > +	 * ANDROID_JPEG_GPS_TIMESTAMP (int64) = 8 bytes
> > > +	 * ANDROID_LENS_APERTURE (float) = 4 bytes
> > > +	 *
> > > +	 * add coordinates again?
> >
> > Not sure I get this todo :)
> >
> > >  	 */
> > >  	std::unique_ptr<CameraMetadata> resultMetadata =
> > > -		std::make_unique<CameraMetadata>(33, 75);
> > > +		std::make_unique<CameraMetadata>(41, 187);
> > >  	if (!resultMetadata->isValid()) {
> > >  		LOG(HAL, Error) << "Failed to allocate static metadata";
> > >  		return nullptr;
> > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > > index a285d0a1..111a7d8f 100644
> > > --- a/src/android/camera_device.h
> > > +++ b/src/android/camera_device.h
> > > @@ -24,6 +24,7 @@
> > >  #include "libcamera/internal/log.h"
> > >  #include "libcamera/internal/message.h"
> > >
> > > +#include "camera_metadata.h"
> > >  #include "camera_stream.h"
> > >  #include "camera_worker.h"
> > >  #include "jpeg/encoder.h"
> > > @@ -78,7 +79,8 @@ private:
> > >  	struct Camera3RequestDescriptor {
> > >  		Camera3RequestDescriptor(libcamera::Camera *camera,
> > >  					 unsigned int frameNumber,
> > > -					 unsigned int numBuffers);
> > > +					 unsigned int numBuffers,
> > > +					 CameraMetadata &settings);
> > >  		~Camera3RequestDescriptor();
> > >
> > >  		uint32_t frameNumber_;
> > > @@ -86,6 +88,7 @@ private:
> > >  		camera3_stream_buffer_t *buffers_;
> > >  		std::vector<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers_;
> > >  		std::unique_ptr<CaptureRequest> request_;
> > > +		CameraMetadata settings_;
> > >  	};
> > >
> > >  	struct Camera3StreamConfiguration {
> >
> > The part below should go to a separate patch imho
> >
> > > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> > > index dba351a4..4c8020e5 100644
> > > --- a/src/android/camera_stream.cpp
> > > +++ b/src/android/camera_stream.cpp
> > > @@ -96,12 +96,15 @@ int CameraStream::configure()
> > >  }
> > >
> > >  int CameraStream::process(const libcamera::FrameBuffer &source,
> > > -			  MappedCamera3Buffer *dest, CameraMetadata *metadata)
> > > +			  MappedCamera3Buffer *dest,
> > > +			  const CameraMetadata &requestMetadata,
> > > +			  CameraMetadata *resultMetadata)
> > >  {
> > >  	if (!postProcessor_)
> > >  		return 0;
> > >
> > > -	return postProcessor_->process(source, dest->maps()[0], metadata);
> > > +	return postProcessor_->process(source, dest->maps()[0],
> > > +				       requestMetadata, resultMetadata);
> > >  }
> > >
> > >  FrameBuffer *CameraStream::getBuffer()
> > > diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> > > index cc9d5470..298ffbf6 100644
> > > --- a/src/android/camera_stream.h
> > > +++ b/src/android/camera_stream.h
> > > @@ -119,7 +119,9 @@ public:
> > >
> > >  	int configure();
> > >  	int process(const libcamera::FrameBuffer &source,
> > > -		    MappedCamera3Buffer *dest, CameraMetadata *metadata);
> > > +		    MappedCamera3Buffer *dest,
> > > +		    const CameraMetadata &requestMetadata,
> > > +		    CameraMetadata *resultMetadata);
> > >  	libcamera::FrameBuffer *getBuffer();
> > >  	void putBuffer(libcamera::FrameBuffer *buffer);
> > >
> > > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> > > index 436a50f8..13ad3777 100644
> > > --- a/src/android/jpeg/post_processor_jpeg.cpp
> > > +++ b/src/android/jpeg/post_processor_jpeg.cpp
> > > @@ -25,6 +25,21 @@ PostProcessorJpeg::PostProcessorJpeg(CameraDevice *const device)
> > >  {
> > >  }
> > >
> > > +int PostProcessorJpeg::configureThumbnailer(const Size &size,
> > > +					    const PixelFormat &pixelFormat)
> > > +{
> > > +	thumbnailer_.configure(size, pixelFormat);
> > > +	StreamConfiguration thCfg;
> > > +	thCfg.size = thumbnailer_.size();
> > > +	thCfg.pixelFormat = pixelFormat;
> > > +	if (thumbnailEncoder_.configure(thCfg) != 0) {
> > > +		LOG(JPEG, Error) << "Failed to configure thumbnail encoder";
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,
> > >  				 const StreamConfiguration &outCfg)
> > >  {
> > > @@ -40,16 +55,11 @@ int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,
> > >
> > >  	streamSize_ = outCfg.size;
> > >
> > > -	thumbnailer_.configure(inCfg.size, inCfg.pixelFormat);
> > > -	StreamConfiguration thCfg = inCfg;
> > > -	thCfg.size = thumbnailer_.size();
> > > -	if (thumbnailEncoder_.configure(thCfg) != 0) {
> > > -		LOG(JPEG, Error) << "Failed to configure thumbnail encoder";
> > > -		return -EINVAL;
> > > -	}
> > > +	int ret = configureThumbnailer(inCfg.size, inCfg.pixelFormat);
> > > +	if (ret)
> > > +		return ret;
> > >
> > >  	encoder_ = std::make_unique<EncoderLibJpeg>();
> > > -
> > >  	return encoder_->configure(inCfg);
> >
> > Unrelated changes ? Should they go to a third patch ?
>
> The introduction of PostProcessorJpeg::configureThumbnailer(), if
> desired, should be split to a patch of its own, yes.
>

I mean the empy line, but yeah, the newly introduce function even more

> > I'll review the rest once separated to a new patch
> >
> > >  }
> > >
> > > @@ -80,17 +90,22 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,
> > >
> > >  int PostProcessorJpeg::process(const FrameBuffer &source,
> > >  			       Span<uint8_t> destination,
> > > -			       CameraMetadata *metadata)
> > > +			       const CameraMetadata &requestMetadata,
> > > +			       CameraMetadata *resultMetadata)
> > >  {
> > >  	if (!encoder_)
> > >  		return 0;
> > >
> > > +	camera_metadata_ro_entry_t entry;
> > > +	int ret;
> > > +
> > >  	/* 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.setMake(cameraDevice_->cameraMake());
> > > +	exif.setModel(cameraDevice_->cameraModel());
> > > +
> > > +	ret = requestMetadata.getEntry(ANDROID_JPEG_ORIENTATION, &entry);
> >
> > If you intend to handle the ANDROID_JPEG_ORIENTATION control (do you
> > need this?) I think you should combine it with the camera orientation.
> >
> > > +	exif.setOrientation(ret ? cameraDevice_->orientation() : *entry.data.i32);
> > >  	exif.setSize(streamSize_);
> > >  	/*
> > >  	 * We set the frame's EXIF timestamp as the time of encode.
> > > @@ -99,11 +114,68 @@ int PostProcessorJpeg::process(const FrameBuffer &source,
> > >  	 */
> > >  	exif.setTimestamp(std::time(nullptr));
> > >
> > > -	std::vector<unsigned char> thumbnail;
> > > -	generateThumbnail(source, &thumbnail);
> > > -	if (!thumbnail.empty())
> > > +	exif.setExposureTime(0);
> > > +	ret = requestMetadata.getEntry(ANDROID_LENS_APERTURE, &entry);
> > > +	if (!ret) {
> > > +		exif.setAperture(*entry.data.f);
> > > +		resultMetadata->addEntry(ANDROID_LENS_APERTURE, entry.data.f, 1);
>
> This doesn't belong to PostProcessorJpeg::process(), only
> exif.setAperture() belongs here. ANDROID_LENS_APERTURE should be set in
> CameraDevice::getResultMetadata().
>
> > > +	}
> > > +	exif.setISO(100);
> > > +	exif.setFlash(Exif::Flash::FlashNotPresent);
> > > +	exif.setWhiteBalance(Exif::WhiteBalance::Auto);
> > > +
> > > +	exif.setSubsecTime(0);
> > > +	exif.setSubsecTimeOriginal(0);
> > > +	exif.setSubsecTimeDigitized(0);
> > > +
> > > +	float focal_length = 1.0;
> > > +	exif.setFocalLength(focal_length);
>
> 	exif.setFocalLength(1.0);
>
> > > +
> > > +	ret = requestMetadata.getEntry(ANDROID_JPEG_GPS_TIMESTAMP, &entry);
> > > +	if (!ret) {
> > > +		exif.setGPSDateTimestamp(*entry.data.i64);
> > > +		resultMetadata->addEntry(ANDROID_JPEG_GPS_TIMESTAMP,
> > > +					 entry.data.i64, 1);
> > > +	}
> > > +
>
> As part as the rework of the thumbnail generation, you need to report
> additional sizes in ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES.
>

That's not that linear, you know.. I mean, are the newly sizes fixed ?
in that case it's trivial, but going forward we need to design an
interface to query the post-processor derived class to query it
capabilities at static metadata intialization time.. maybe a few static
parameters would do.

> > > +	ret = requestMetadata.getEntry(ANDROID_JPEG_THUMBNAIL_SIZE, &entry);
> > > +	if (!ret) {
> > > +		/* \todo Handle non-matching aspect ratios */
> > > +		/* \todo Handle non-zero orientations */
>
> Is this required ? As we handle the orientation by setting it in the
> Exif tags, we don't need to rotate the thumbnail, is there anything else
> to handle ?
>
> > > +		const int32_t *data = entry.data.i32;
> > > +		Size thumbnailSize = { static_cast<uint32_t>(data[0]),
> > > +				       static_cast<uint32_t>(data[1]) };
> > > +
> > > +		std::vector<unsigned char> thumbnail;
> > > +		if (thumbnailSize != Size(0, 0)) {
> > > +			configureThumbnailer(thumbnailSize, thumbnailer_.pixelFormat());
> > > +			generateThumbnail(source, &thumbnail);
> > > +		}
> > >  		exif.setThumbnail(thumbnail, Exif::Compression::JPEG);
>
> You shouldn't set a thumbnail at all if the size is { 0, 0 }.
>
> > >
> > > +		resultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE, data, 2);
> > > +
> > > +		/* \todo Use this quality as a parameter to the encoder */
> > > +		ret = requestMetadata.getEntry(ANDROID_JPEG_THUMBNAIL_QUALITY, &entry);
> > > +		if (!ret)
> > > +			resultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_QUALITY, entry.data.u8, 1);
> > > +	}
> > > +
> > > +	ret = requestMetadata.getEntry(ANDROID_JPEG_GPS_COORDINATES, &entry);
> > > +	if (!ret) {
> > > +		exif.setGPSLocation(entry.data.d);
> > > +		resultMetadata->addEntry(ANDROID_JPEG_GPS_COORDINATES,
> > > +					 entry.data.d, 3);
> > > +	}
> > > +
> > > +	ret = requestMetadata.getEntry(ANDROID_JPEG_GPS_PROCESSING_METHOD, &entry);
> > > +	if (!ret) {
> > > +		std::string method(entry.data.u8, entry.data.u8 + 32);
> > > +		exif.setGPSMethod(method);
> > > +		resultMetadata->addEntry(ANDROID_JPEG_GPS_PROCESSING_METHOD,
> > > +					 entry.data.u8, 32);
> > > +	}
> > > +
> > >  	if (exif.generate() != 0)
> > >  		LOG(JPEG, Error) << "Failed to generate valid EXIF data";
> > >
> > > @@ -133,13 +205,15 @@ int PostProcessorJpeg::process(const FrameBuffer &source,
> > >  	blob->jpeg_size = jpeg_size;
> > >
> > >  	/* Update the JPEG result Metadata. */
> > > -	metadata->addEntry(ANDROID_JPEG_SIZE, &jpeg_size, 1);
> > > +	resultMetadata->addEntry(ANDROID_JPEG_SIZE, &jpeg_size, 1);
> > >
> > > -	const uint32_t jpeg_quality = 95;
> > > -	metadata->addEntry(ANDROID_JPEG_QUALITY, &jpeg_quality, 1);
> > > +	ret = requestMetadata.getEntry(ANDROID_JPEG_QUALITY, &entry);
> > > +	const uint32_t jpeg_quality = ret ? 95 : *entry.data.u8;
> > > +	resultMetadata->addEntry(ANDROID_JPEG_QUALITY, &jpeg_quality, 1);
>
> If we start acting on ANDROID_JPEG_QUALITY, we should really take it
> into account and configure the JPEG encoder accordingly. This can be
> done on top.
>
> > >
> > > -	const uint32_t jpeg_orientation = 0;
> > > -	metadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpeg_orientation, 1);
> > > +	ret = requestMetadata.getEntry(ANDROID_JPEG_ORIENTATION, &entry);
> > > +	const uint32_t jpeg_orientation = ret ? 0 : *entry.data.i32;
> > > +	resultMetadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpeg_orientation, 1);
> > >
> > >  	return 0;
> > >  }
> > > diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h
> > > index 5afa831c..d07c9fe5 100644
> > > --- a/src/android/jpeg/post_processor_jpeg.h
> > > +++ b/src/android/jpeg/post_processor_jpeg.h
> > > @@ -26,11 +26,14 @@ public:
> > >  		      const libcamera::StreamConfiguration &outcfg) override;
> > >  	int process(const libcamera::FrameBuffer &source,
> > >  		    libcamera::Span<uint8_t> destination,
> > > -		    CameraMetadata *metadata) override;
> > > +		    const CameraMetadata &requestMetadata,
> > > +		    CameraMetadata *resultMetadata) override;
>
> I'd split this change and the similar one below to a separate patch, it
> will make this patch easier to review.
>
> > >
> > >  private:
> > >  	void generateThumbnail(const libcamera::FrameBuffer &source,
> > >  			       std::vector<unsigned char> *thumbnail);
> > > +	int configureThumbnailer(const libcamera::Size &size,
> > > +				 const libcamera::PixelFormat &pixelFormat);
> > >
> > >  	CameraDevice *const cameraDevice_;
> > >  	std::unique_ptr<Encoder> encoder_;
> > > diff --git a/src/android/jpeg/thumbnailer.h b/src/android/jpeg/thumbnailer.h
> > > index 98f11833..f393db47 100644
> > > --- a/src/android/jpeg/thumbnailer.h
> > > +++ b/src/android/jpeg/thumbnailer.h
> > > @@ -22,6 +22,7 @@ public:
> > >  	void createThumbnail(const libcamera::FrameBuffer &source,
> > >  			     std::vector<unsigned char> *dest);
> > >  	const libcamera::Size &size() const { return targetSize_; }
> > > +	const libcamera::PixelFormat &pixelFormat() const { return pixelFormat_; }
> > >
> > >  private:
> > >  	libcamera::Size computeThumbnailSize() const;
> > > diff --git a/src/android/post_processor.h b/src/android/post_processor.h
> > > index e0f91880..bda93bb4 100644
> > > --- a/src/android/post_processor.h
> > > +++ b/src/android/post_processor.h
> > > @@ -22,7 +22,8 @@ public:
> > >  			      const libcamera::StreamConfiguration &outCfg) = 0;
> > >  	virtual int process(const libcamera::FrameBuffer &source,
> > >  			    libcamera::Span<uint8_t> destination,
> > > -			    CameraMetadata *metadata) = 0;
> > > +			    const CameraMetadata &requestMetadata,
> > > +			    CameraMetadata *resultMetadata) = 0;
> > >  };
> > >
> > >  #endif /* __ANDROID_POST_PROCESSOR_H__ */
>
> --
> Regards,
>
> Laurent Pinchart

Hi Laurent and Jacopo,

Thank you for the review.

On Fri, Jan 15, 2021 at 09:38:24PM +0200, Laurent Pinchart wrote:
> Hi Paul,
> 
> Thank you for the patch.
> 
> On Fri, Jan 15, 2021 at 04:48:12PM +0100, Jacopo Mondi wrote:
> > On Thu, Jan 14, 2021 at 07:40:35PM +0900, Paul Elder wrote:
> > > Set the following android result metadata:
> > > - ANDROID_LENS_FOCAL_LENGTH
> > > - ANDROID_LENS_APERTURE
> > > - ANDROID_JPEG_GPS_TIMESTAMP
> > > - ANDROID_JPEG_THUMBNAIL_SIZE
> > > - ANDROID_JPEG_THUMBNAIL_QUALITY
> > > - ANDROID_JPEG_GPS_COORDINATES
> > > - ANDROID_JPEG_GPS_PROCESSING_METHOD
> > > - ANDROID_JPEG_SIZE
> > > - ANDROID_JPEG_QUALITY
> > > - ANDROID_JPEG_ORIENTATION
> > >
> > > And the following EXIF fields:
> > > - GPSDatestamp
> > > - GPSTimestamp
> > > - GPSLocation
> > >   - GPSLatitudeRef
> > >   - GPSLatitude
> > >   - GPSLongitudeRef
> > >   - GPSLongitude
> > >   - GPSAltitudeRef
> > >   - GPSAltitude
> > > - GPSProcessingMethod
> > > - FocalLength
> > > - ExposureTime
> > > - FNumber
> > > - ISO
> > > - Flash
> > > - WhiteBalance
> > > - SubsecTime
> > > - SubsecTimeOriginal
> > > - SubsecTimeDigitized
> > >
> > > Based on android request metadata.
> 
> Can you split this in multiple patches ? Review is difficult. You can
> have one patch that sets all the result metadata and Exif tags that do
> not interact with any other component, and one patch per change for all
> other changes. For instance ANDROID_JPEG_THUMBNAIL_SIZE requires
> configuring the thumbnailer accordingly, and should be in a patch of its
> own.

Okay :/

> > >
> > > This allows the following CTS tests to pass:
> > > - android.hardware.camera2.cts.StillCaptureTest#testFocalLengths
> > > - android.hardware.camera2.cts.StillCaptureTest#testJpegExif
> >
> > \o/
> >
> > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > ---
> > >  src/android/camera_device.cpp            |  27 +++++-
> > >  src/android/camera_device.h              |   5 +-
> > >  src/android/camera_stream.cpp            |   7 +-
> > >  src/android/camera_stream.h              |   4 +-
> > >  src/android/jpeg/post_processor_jpeg.cpp | 116 +++++++++++++++++++----
> > >  src/android/jpeg/post_processor_jpeg.h   |   5 +-
> > >  src/android/jpeg/thumbnailer.h           |   1 +
> > >  src/android/post_processor.h             |   3 +-
> > >  8 files changed, 136 insertions(+), 32 deletions(-)
> > >
> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > index ed47c7cd..8d697080 100644
> > > --- a/src/android/camera_device.cpp
> > > +++ b/src/android/camera_device.cpp
> > > @@ -296,8 +296,9 @@ MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,
> > >   */
> > >
> > >  CameraDevice::Camera3RequestDescriptor::Camera3RequestDescriptor(
> > > -	Camera *camera, unsigned int frameNumber, unsigned int numBuffers)
> > > -	: frameNumber_(frameNumber), numBuffers_(numBuffers)
> > > +	Camera *camera, unsigned int frameNumber, unsigned int numBuffers,
> > > +	CameraMetadata &settings)
> > > +	: frameNumber_(frameNumber), numBuffers_(numBuffers), settings_(settings)
> > >  {
> > >  	buffers_ = new camera3_stream_buffer_t[numBuffers];
> > >
> > > @@ -1700,9 +1701,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> > >  	 * The descriptor and the associated memory reserved here are freed
> > >  	 * at request complete time.
> > >  	 */
> > > +	/* \todo Handle null request settings */
> 
> Can it happen ? If so it should be handled as part of this series or
> immediately on top, not left for later.

According to the android docs, yes. In that case the last request
settings should be applied, so I think some small plumbing is necessary
to get that working.

I guess I'll do it for v2 then. idk if there are any tests to confirm
it though.

> > > +	CameraMetadata requestMetadata(camera3Request->settings);
> > >  	Camera3RequestDescriptor *descriptor =
> > >  		new Camera3RequestDescriptor(camera_.get(), camera3Request->frame_number,
> > > -					     camera3Request->num_output_buffers);
> > > +					     camera3Request->num_output_buffers,
> > > +					     requestMetadata);
> >
> > As you use settings, I would name it settings here as well
> >
> > >
> > >  	LOG(HAL, Debug) << "Queueing Request to libcamera with "
> > >  			<< descriptor->numBuffers_ << " HAL streams";
> > > @@ -1815,6 +1819,9 @@ void CameraDevice::requestComplete(Request *request)
> > >  		CameraStream *cameraStream =
> > >  			static_cast<CameraStream *>(descriptor->buffers_[i].stream->priv);
> > >
> > > +		float focal_length = 1.0;
> > > +		resultMetadata->addEntry(ANDROID_LENS_FOCAL_LENGTH, &focal_length, 1);
> > > +
> >
> > Ups, result metadata are procesed in getResultMetadata() and if you
> > add an entry you need to expand the metadata pack size. Also.. see
> > below..
> >
> > >  		if (cameraStream->camera3Stream().format != HAL_PIXEL_FORMAT_BLOB)
> > >  			continue;
> > >
> > > @@ -1837,6 +1844,7 @@ void CameraDevice::requestComplete(Request *request)
> > >  		}
> > >
> > >  		int ret = cameraStream->process(*buffer, &mapped,
> > > +						descriptor->settings_,
> >
> > I would move:
> > 1) Adding JPEG tags to EXIF
> > 2) Filling in the result metadata
> >
> > to 2 different patches
> >
> > However, I'm a bit skeptic about this change.
> > You pass to the JPEG encode the settings the application has
> > requested. The actual process should go through the pipeline handler,
> > so that you should be able to find all the required information in
> > libcamera::Request::metadata()
> >
> > This requires several parts:
> > 1) Define a libcamera (draft) control to be able to translate the
> > Android metadata to a control the libcamera pipeline handler can
> > consume
> > 2) Set the libcamera control in the Request's metadata
> > 3) Parse the control value in the completion handler here.
> >
> > You can see how (parts of) this process is handled for draft::PipelineDepth.
> >
> > However, it's not trivial to make all the pieces fit together. I have
> > a series of patches I still have to send out that implement exactly
> > this for SCALER_CROP_REGION (except for the fact no ad-hoc property
> > needs to be defined, as we already have
> > libcamera::controls::ScalerCrop)
> >
> > I'm not sure if we should either rush and take this big shortcut here
> > to provide the lens info to the post-processor, or rather hard-code it
> > there with a big \todo, or maybe wait until we close the loop.. I
> > would go for the second option, but I would like to know Laurent's
> > opinion here.
> 
> There are Android metadata that are meant only for the HAL, such as all
> the JPEG-related information as we don't handle JPEG encoding in
> libcamera. Passing the request settings to the post-processor does make
> sense.
> 
> Looking at the code below, the only metadata entry that needs to be
> handled by libcamera is the lens aperture. Given that we support a
> single aperture at the moment (we report a single entry in
> ANDROID_LENS_INFO_AVAILABLE_APERTURES), hardcoding ANDROID_LENS_APERTURE
> makes sense, we don't need to define libcamera controls yet. However,
> setting ANDROID_LENS_APERTURE doesn't belong to this patch (see below
> for more comments).
> 
> Same for ANDROID_LENS_FOCAL_LENGTH, we can hardcode it for now as we
> have a fixed focus lens, but it should go to getResultMetadata() as
> you've mentioned.

I'll leave a big todo that some settings need to be translated to
libcamera controls.

> > >  						resultMetadata.get());
> > >  		if (ret) {
> > >  			status = CAMERA3_BUFFER_STATUS_ERROR;
> > > @@ -1933,14 +1941,23 @@ CameraDevice::getResultMetadata(Camera3RequestDescriptor *descriptor,
> > >
> > >  	/*
> > >  	 * \todo Keep this in sync with the actual number of entries.
> > > -	 * Currently: 33 entries, 75 bytes
> > > +	 * Currently: 41 entries, 187 bytes
> > >  	 *
> > >  	 * Reserve more space for the JPEG metadata set by the post-processor.
> > >  	 * Currently: ANDROID_JPEG_SIZE (int32_t), ANDROID_JPEG_QUALITY (byte),
> > >  	 * ANDROID_JPEG_ORIENTATION (int32_t) = 3 entries, 9 bytes.
> >
> > Can you move the additional JPEG sizes to the end ?
> >
> >         = 10 entries, 92 bytes
> >
> > However I count:
> >         41 - 33 = +8
> >         We have 10 JPEG metadata so I assume the additional one is for
> >         LENS_FOCAL_LENGTH you add here above
> >
> >         187 - 75 = 112
> >         I count 92 bytes for JPEG + 4 for LENS_FOCAL_LENGHT
> >
> >         Where do the additional 16 bytes come from ?

That's what I counted too, so I'm not sure why CameraMetadata complained
when I had that number. I'll double-check.

> > > +	 * ANDROID_JPEG_THUMBNAIL_SIZE (int32 x 2) = 8 bytes
> > > +	 * ANDROID_JPEG_THUMBNAIL_QUALITY (int32 x 2) = 8 bytes
> > > +	 * ANDROID_LENS_FOCAL_LENGTH (float) = 4 bytes
> > > +	 * ANDROID_JPEG_GPS_COORDINATES (double x 3) = 24 bytes
> > > +	 * ANDROID_JPEG_GPS_PROCESSING_METHOD (byte x 32) = 32 bytes
> > > +	 * ANDROID_JPEG_GPS_TIMESTAMP (int64) = 8 bytes
> > > +	 * ANDROID_LENS_APERTURE (float) = 4 bytes
> > > +	 *
> > > +	 * add coordinates again?
> >
> > Not sure I get this todo :)

Personal note that I forgot to remove :)

> > >  	 */
> > >  	std::unique_ptr<CameraMetadata> resultMetadata =
> > > -		std::make_unique<CameraMetadata>(33, 75);
> > > +		std::make_unique<CameraMetadata>(41, 187);
> > >  	if (!resultMetadata->isValid()) {
> > >  		LOG(HAL, Error) << "Failed to allocate static metadata";
> > >  		return nullptr;
> > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > > index a285d0a1..111a7d8f 100644
> > > --- a/src/android/camera_device.h
> > > +++ b/src/android/camera_device.h
> > > @@ -24,6 +24,7 @@
> > >  #include "libcamera/internal/log.h"
> > >  #include "libcamera/internal/message.h"
> > >
> > > +#include "camera_metadata.h"
> > >  #include "camera_stream.h"
> > >  #include "camera_worker.h"
> > >  #include "jpeg/encoder.h"
> > > @@ -78,7 +79,8 @@ private:
> > >  	struct Camera3RequestDescriptor {
> > >  		Camera3RequestDescriptor(libcamera::Camera *camera,
> > >  					 unsigned int frameNumber,
> > > -					 unsigned int numBuffers);
> > > +					 unsigned int numBuffers,
> > > +					 CameraMetadata &settings);
> > >  		~Camera3RequestDescriptor();
> > >
> > >  		uint32_t frameNumber_;
> > > @@ -86,6 +88,7 @@ private:
> > >  		camera3_stream_buffer_t *buffers_;
> > >  		std::vector<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers_;
> > >  		std::unique_ptr<CaptureRequest> request_;
> > > +		CameraMetadata settings_;
> > >  	};
> > >
> > >  	struct Camera3StreamConfiguration {
> >
> > The part below should go to a separate patch imho

Yeah you're right.

> > > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> > > index dba351a4..4c8020e5 100644
> > > --- a/src/android/camera_stream.cpp
> > > +++ b/src/android/camera_stream.cpp
> > > @@ -96,12 +96,15 @@ int CameraStream::configure()
> > >  }
> > >
> > >  int CameraStream::process(const libcamera::FrameBuffer &source,
> > > -			  MappedCamera3Buffer *dest, CameraMetadata *metadata)
> > > +			  MappedCamera3Buffer *dest,
> > > +			  const CameraMetadata &requestMetadata,
> > > +			  CameraMetadata *resultMetadata)
> > >  {
> > >  	if (!postProcessor_)
> > >  		return 0;
> > >
> > > -	return postProcessor_->process(source, dest->maps()[0], metadata);
> > > +	return postProcessor_->process(source, dest->maps()[0],
> > > +				       requestMetadata, resultMetadata);
> > >  }
> > >
> > >  FrameBuffer *CameraStream::getBuffer()
> > > diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> > > index cc9d5470..298ffbf6 100644
> > > --- a/src/android/camera_stream.h
> > > +++ b/src/android/camera_stream.h
> > > @@ -119,7 +119,9 @@ public:
> > >
> > >  	int configure();
> > >  	int process(const libcamera::FrameBuffer &source,
> > > -		    MappedCamera3Buffer *dest, CameraMetadata *metadata);
> > > +		    MappedCamera3Buffer *dest,
> > > +		    const CameraMetadata &requestMetadata,
> > > +		    CameraMetadata *resultMetadata);
> > >  	libcamera::FrameBuffer *getBuffer();
> > >  	void putBuffer(libcamera::FrameBuffer *buffer);
> > >
> > > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> > > index 436a50f8..13ad3777 100644
> > > --- a/src/android/jpeg/post_processor_jpeg.cpp
> > > +++ b/src/android/jpeg/post_processor_jpeg.cpp
> > > @@ -25,6 +25,21 @@ PostProcessorJpeg::PostProcessorJpeg(CameraDevice *const device)
> > >  {
> > >  }
> > >
> > > +int PostProcessorJpeg::configureThumbnailer(const Size &size,
> > > +					    const PixelFormat &pixelFormat)
> > > +{
> > > +	thumbnailer_.configure(size, pixelFormat);
> > > +	StreamConfiguration thCfg;
> > > +	thCfg.size = thumbnailer_.size();
> > > +	thCfg.pixelFormat = pixelFormat;
> > > +	if (thumbnailEncoder_.configure(thCfg) != 0) {
> > > +		LOG(JPEG, Error) << "Failed to configure thumbnail encoder";
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,
> > >  				 const StreamConfiguration &outCfg)
> > >  {
> > > @@ -40,16 +55,11 @@ int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,
> > >
> > >  	streamSize_ = outCfg.size;
> > >
> > > -	thumbnailer_.configure(inCfg.size, inCfg.pixelFormat);
> > > -	StreamConfiguration thCfg = inCfg;
> > > -	thCfg.size = thumbnailer_.size();
> > > -	if (thumbnailEncoder_.configure(thCfg) != 0) {
> > > -		LOG(JPEG, Error) << "Failed to configure thumbnail encoder";
> > > -		return -EINVAL;
> > > -	}
> > > +	int ret = configureThumbnailer(inCfg.size, inCfg.pixelFormat);
> > > +	if (ret)
> > > +		return ret;
> > >
> > >  	encoder_ = std::make_unique<EncoderLibJpeg>();
> > > -
> > >  	return encoder_->configure(inCfg);
> >
> > Unrelated changes ? Should they go to a third patch ?
> 
> The introduction of PostProcessorJpeg::configureThumbnailer(), if
> desired, should be split to a patch of its own, yes.

Yeah you're right.

> > I'll review the rest once separated to a new patch
> >
> > >  }
> > >
> > > @@ -80,17 +90,22 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,
> > >
> > >  int PostProcessorJpeg::process(const FrameBuffer &source,
> > >  			       Span<uint8_t> destination,
> > > -			       CameraMetadata *metadata)
> > > +			       const CameraMetadata &requestMetadata,
> > > +			       CameraMetadata *resultMetadata)
> > >  {
> > >  	if (!encoder_)
> > >  		return 0;
> > >
> > > +	camera_metadata_ro_entry_t entry;
> > > +	int ret;
> > > +
> > >  	/* 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.setMake(cameraDevice_->cameraMake());
> > > +	exif.setModel(cameraDevice_->cameraModel());
> > > +
> > > +	ret = requestMetadata.getEntry(ANDROID_JPEG_ORIENTATION, &entry);
> >
> > If you intend to handle the ANDROID_JPEG_ORIENTATION control (do you
> > need this?) I think you should combine it with the camera orientation.
> >
> > > +	exif.setOrientation(ret ? cameraDevice_->orientation() : *entry.data.i32);
> > >  	exif.setSize(streamSize_);
> > >  	/*
> > >  	 * We set the frame's EXIF timestamp as the time of encode.
> > > @@ -99,11 +114,68 @@ int PostProcessorJpeg::process(const FrameBuffer &source,
> > >  	 */
> > >  	exif.setTimestamp(std::time(nullptr));
> > >
> > > -	std::vector<unsigned char> thumbnail;
> > > -	generateThumbnail(source, &thumbnail);
> > > -	if (!thumbnail.empty())
> > > +	exif.setExposureTime(0);
> > > +	ret = requestMetadata.getEntry(ANDROID_LENS_APERTURE, &entry);
> > > +	if (!ret) {
> > > +		exif.setAperture(*entry.data.f);
> > > +		resultMetadata->addEntry(ANDROID_LENS_APERTURE, entry.data.f, 1);
> 
> This doesn't belong to PostProcessorJpeg::process(), only
> exif.setAperture() belongs here. ANDROID_LENS_APERTURE should be set in
> CameraDevice::getResultMetadata().
> 
> > > +	}
> > > +	exif.setISO(100);
> > > +	exif.setFlash(Exif::Flash::FlashNotPresent);
> > > +	exif.setWhiteBalance(Exif::WhiteBalance::Auto);
> > > +
> > > +	exif.setSubsecTime(0);
> > > +	exif.setSubsecTimeOriginal(0);
> > > +	exif.setSubsecTimeDigitized(0);
> > > +
> > > +	float focal_length = 1.0;
> > > +	exif.setFocalLength(focal_length);
> 
> 	exif.setFocalLength(1.0);
> 
> > > +
> > > +	ret = requestMetadata.getEntry(ANDROID_JPEG_GPS_TIMESTAMP, &entry);
> > > +	if (!ret) {
> > > +		exif.setGPSDateTimestamp(*entry.data.i64);
> > > +		resultMetadata->addEntry(ANDROID_JPEG_GPS_TIMESTAMP,
> > > +					 entry.data.i64, 1);
> > > +	}
> > > +
> 
> As part as the rework of the thumbnail generation, you need to report
> additional sizes in ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES.
> 
> > > +	ret = requestMetadata.getEntry(ANDROID_JPEG_THUMBNAIL_SIZE, &entry);
> > > +	if (!ret) {
> > > +		/* \todo Handle non-matching aspect ratios */
> > > +		/* \todo Handle non-zero orientations */
> 
> Is this required ? As we handle the orientation by setting it in the
> Exif tags, we don't need to rotate the thumbnail, is there anything else
> to handle ?

That's what I saw in the android properties docs.

> > > +		const int32_t *data = entry.data.i32;
> > > +		Size thumbnailSize = { static_cast<uint32_t>(data[0]),
> > > +				       static_cast<uint32_t>(data[1]) };
> > > +
> > > +		std::vector<unsigned char> thumbnail;
> > > +		if (thumbnailSize != Size(0, 0)) {
> > > +			configureThumbnailer(thumbnailSize, thumbnailer_.pixelFormat());
> > > +			generateThumbnail(source, &thumbnail);
> > > +		}
> > >  		exif.setThumbnail(thumbnail, Exif::Compression::JPEG);
> 
> You shouldn't set a thumbnail at all if the size is { 0, 0 }.
> 
> > >
> > > +		resultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE, data, 2);
> > > +
> > > +		/* \todo Use this quality as a parameter to the encoder */
> > > +		ret = requestMetadata.getEntry(ANDROID_JPEG_THUMBNAIL_QUALITY, &entry);
> > > +		if (!ret)
> > > +			resultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_QUALITY, entry.data.u8, 1);
> > > +	}
> > > +
> > > +	ret = requestMetadata.getEntry(ANDROID_JPEG_GPS_COORDINATES, &entry);
> > > +	if (!ret) {
> > > +		exif.setGPSLocation(entry.data.d);
> > > +		resultMetadata->addEntry(ANDROID_JPEG_GPS_COORDINATES,
> > > +					 entry.data.d, 3);
> > > +	}
> > > +
> > > +	ret = requestMetadata.getEntry(ANDROID_JPEG_GPS_PROCESSING_METHOD, &entry);
> > > +	if (!ret) {
> > > +		std::string method(entry.data.u8, entry.data.u8 + 32);
> > > +		exif.setGPSMethod(method);
> > > +		resultMetadata->addEntry(ANDROID_JPEG_GPS_PROCESSING_METHOD,
> > > +					 entry.data.u8, 32);
> > > +	}
> > > +
> > >  	if (exif.generate() != 0)
> > >  		LOG(JPEG, Error) << "Failed to generate valid EXIF data";
> > >
> > > @@ -133,13 +205,15 @@ int PostProcessorJpeg::process(const FrameBuffer &source,
> > >  	blob->jpeg_size = jpeg_size;
> > >
> > >  	/* Update the JPEG result Metadata. */
> > > -	metadata->addEntry(ANDROID_JPEG_SIZE, &jpeg_size, 1);
> > > +	resultMetadata->addEntry(ANDROID_JPEG_SIZE, &jpeg_size, 1);
> > >
> > > -	const uint32_t jpeg_quality = 95;
> > > -	metadata->addEntry(ANDROID_JPEG_QUALITY, &jpeg_quality, 1);
> > > +	ret = requestMetadata.getEntry(ANDROID_JPEG_QUALITY, &entry);
> > > +	const uint32_t jpeg_quality = ret ? 95 : *entry.data.u8;
> > > +	resultMetadata->addEntry(ANDROID_JPEG_QUALITY, &jpeg_quality, 1);
> 
> If we start acting on ANDROID_JPEG_QUALITY, we should really take it
> into account and configure the JPEG encoder accordingly. This can be
> done on top.

I noted that and forgot to slap on a \todo.

> > >
> > > -	const uint32_t jpeg_orientation = 0;
> > > -	metadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpeg_orientation, 1);
> > > +	ret = requestMetadata.getEntry(ANDROID_JPEG_ORIENTATION, &entry);
> > > +	const uint32_t jpeg_orientation = ret ? 0 : *entry.data.i32;
> > > +	resultMetadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpeg_orientation, 1);
> > >
> > >  	return 0;
> > >  }
> > > diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h
> > > index 5afa831c..d07c9fe5 100644
> > > --- a/src/android/jpeg/post_processor_jpeg.h
> > > +++ b/src/android/jpeg/post_processor_jpeg.h
> > > @@ -26,11 +26,14 @@ public:
> > >  		      const libcamera::StreamConfiguration &outcfg) override;
> > >  	int process(const libcamera::FrameBuffer &source,
> > >  		    libcamera::Span<uint8_t> destination,
> > > -		    CameraMetadata *metadata) override;
> > > +		    const CameraMetadata &requestMetadata,
> > > +		    CameraMetadata *resultMetadata) override;
> 
> I'd split this change and the similar one below to a separate patch, it
> will make this patch easier to review.
> 
> > >
> > >  private:
> > >  	void generateThumbnail(const libcamera::FrameBuffer &source,
> > >  			       std::vector<unsigned char> *thumbnail);
> > > +	int configureThumbnailer(const libcamera::Size &size,
> > > +				 const libcamera::PixelFormat &pixelFormat);
> > >
> > >  	CameraDevice *const cameraDevice_;
> > >  	std::unique_ptr<Encoder> encoder_;
> > > diff --git a/src/android/jpeg/thumbnailer.h b/src/android/jpeg/thumbnailer.h
> > > index 98f11833..f393db47 100644
> > > --- a/src/android/jpeg/thumbnailer.h
> > > +++ b/src/android/jpeg/thumbnailer.h
> > > @@ -22,6 +22,7 @@ public:
> > >  	void createThumbnail(const libcamera::FrameBuffer &source,
> > >  			     std::vector<unsigned char> *dest);
> > >  	const libcamera::Size &size() const { return targetSize_; }
> > > +	const libcamera::PixelFormat &pixelFormat() const { return pixelFormat_; }
> > >
> > >  private:
> > >  	libcamera::Size computeThumbnailSize() const;
> > > diff --git a/src/android/post_processor.h b/src/android/post_processor.h
> > > index e0f91880..bda93bb4 100644
> > > --- a/src/android/post_processor.h
> > > +++ b/src/android/post_processor.h
> > > @@ -22,7 +22,8 @@ public:
> > >  			      const libcamera::StreamConfiguration &outCfg) = 0;
> > >  	virtual int process(const libcamera::FrameBuffer &source,
> > >  			    libcamera::Span<uint8_t> destination,
> > > -			    CameraMetadata *metadata) = 0;
> > > +			    const CameraMetadata &requestMetadata,
> > > +			    CameraMetadata *resultMetadata) = 0;
> > >  };
> > >
> > >  #endif /* __ANDROID_POST_PROCESSOR_H__ */


Thanks,

Paul