[PATCH v2 8/9] android: Add JpegExifMetadata to store tags setting into Exif

[Jacopo Mondi]

Hi Harvey

On Wed, Nov 27, 2024 at 09:25:58AM +0000, Harvey Yang wrote:
> From: Han-Lin Chen <hanlinchen at chromium.org>
>
> With partial result, some metadata, which needs to be added into Exif,
> may be sent back to framework earlier before Jpeg post-processing.
> Add a type JpegExifMetadata associated with StreamBuffer to store the values,
> so Jpeg post-processing doesn't need to reference to current metadata.
>
> Signed-off-by: Han-Lin Chen <hanlinchen at chromium.org>
> Co-developed-by: Harvey Yang <chenghaoyang at chromium.org>
> Signed-off-by: Harvey Yang <chenghaoyang at chromium.org>
> ---
>  src/android/camera_device.cpp            | 26 ++++++++++++++++++++++++
>  src/android/camera_device.h              |  2 ++
>  src/android/camera_request.h             |  6 ++++++
>  src/android/camera_stream.h              |  4 ++++
>  src/android/jpeg/post_processor_jpeg.cpp | 12 ++++++-----
>  5 files changed, 45 insertions(+), 5 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 9fd851bc8..e085e18b2 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -1250,6 +1250,10 @@ void CameraDevice::requestComplete(Request *request)
>  		CameraStream *stream = iter->first;
>  		StreamBuffer *buffer = iter->second;
>
> +		if (stream->isJpegStream()) {
> +			generateJpegExifMetadata(descriptor, buffer);
> +		}
> +

no {} for single line statements

>  		FrameBuffer *src = request->findBuffer(stream->stream());
>  		if (!src) {
>  			LOG(HAL, Error) << "Failed to find a source stream buffer";
> @@ -1443,6 +1447,28 @@ void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream,
>  	callbacks_->notify(callbacks_, &notify);
>  }
>
> +/*
> + * Set jpeg metadata used to generate EXIF in the JPEG post processing.
> + */
> +void CameraDevice::generateJpegExifMetadata(Camera3RequestDescriptor *request,
> +					    StreamBuffer *buffer) const
> +{
> +	const ControlList &metadata = request->request_->metadata();
> +	auto &jpegExifMetadata = buffer->jpegExifMetadata;
> +	jpegExifMetadata.emplace(StreamBuffer::JpegExifMetadata());
> +
> +	const int64_t exposureTime = metadata.get(controls::ExposureTime).value_or(0);
> +	jpegExifMetadata->sensorExposureTime = exposureTime;
> +
> +	/*
> +	 * todo: Android Sensitivity should only include analog gain X digital

\todo

> +	 * gain from sensor. Digital gain on ISP shouldn't be included.

mmm, I guess how the gain is split between analogue and digital on the
sensor is up to the IPA implementation, and currently I only see vc4
handling it and it sets it on the ISP.

I wonder if you couldn't simply use AnalogueGain here

> +	 * Calculate sensitivity accordingly when we can differentiate
> +	 * the source of digital gains.
> +	 */
> +	jpegExifMetadata->sensorSensitivityISO = 100;
> +}
> +
>  /*
>   * Produce a set of fixed result metadata.
>   */
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index 815a695d1..3c46ff918 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -102,6 +102,8 @@ private:
>  	void sendCaptureResult(Camera3RequestDescriptor *request) const;
>  	void setBufferStatus(StreamBuffer &buffer,
>  			     StreamBuffer::Status status);
> +	void generateJpegExifMetadata(Camera3RequestDescriptor *request,
> +				      StreamBuffer *buffer) const;
>  	std::unique_ptr<CameraMetadata> getResultMetadata(
>  		const Camera3RequestDescriptor &descriptor) const;
>
> diff --git a/src/android/camera_request.h b/src/android/camera_request.h
> index bd75d4595..bd87b36fd 100644
> --- a/src/android/camera_request.h
> +++ b/src/android/camera_request.h
> @@ -44,6 +44,11 @@ public:
>  	StreamBuffer(StreamBuffer &&);
>  	StreamBuffer &operator=(StreamBuffer &&);
>
> +	struct JpegExifMetadata {
> +		int64_t sensorExposureTime;
> +		int32_t sensorSensitivityISO;
> +	};
> +
>  	CameraStream *stream;
>  	buffer_handle_t *camera3Buffer;
>  	std::unique_ptr<HALFrameBuffer> frameBuffer;
> @@ -51,6 +56,7 @@ public:
>  	Status status = Status::Success;
>  	const libcamera::FrameBuffer *srcBuffer = nullptr;
>  	std::unique_ptr<CameraBuffer> dstBuffer;
> +	std::optional<JpegExifMetadata> jpegExifMetadata;
>  	Camera3RequestDescriptor *request;
>
>  private:
> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> index 30f64f690..47cd7ab85 100644
> --- a/src/android/camera_stream.h
> +++ b/src/android/camera_stream.h
> @@ -125,6 +125,10 @@ public:
>  	const libcamera::StreamConfiguration &configuration() const;
>  	libcamera::Stream *stream() const;
>  	CameraStream *sourceStream() const { return sourceStream_; }
> +	bool isJpegStream() const
> +	{
> +		return camera3Stream_->format == HAL_PIXEL_FORMAT_BLOB;
> +	}
>
>  	int configure();
>  	int process(StreamBuffer *streamBuffer);
> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> index f5a90785d..48782b574 100644
> --- a/src/android/jpeg/post_processor_jpeg.cpp
> +++ b/src/android/jpeg/post_processor_jpeg.cpp
> @@ -112,8 +112,11 @@ void PostProcessorJpeg::process(StreamBuffer *streamBuffer)
>
>  	const FrameBuffer &source = *streamBuffer->srcBuffer;
>  	CameraBuffer *destination = streamBuffer->dstBuffer.get();
> +	const std::optional<StreamBuffer::JpegExifMetadata> &jpegExifMetadata =
> +		streamBuffer->jpegExifMetadata;
>
>  	ASSERT(destination->numPlanes() == 1);
> +	ASSERT(jpegExifMetadata.has_value());

This means it's not optional, isn't it ?

>
>  	const CameraMetadata &requestMetadata = streamBuffer->request->settings_;
>  	CameraMetadata *resultMetadata = streamBuffer->request->resultMetadata_.get();
> @@ -139,15 +142,14 @@ void PostProcessorJpeg::process(StreamBuffer *streamBuffer)
>  	 */
>  	exif.setTimestamp(std::time(nullptr), 0ms);
>
> -	ret = resultMetadata->getEntry(ANDROID_SENSOR_EXPOSURE_TIME, &entry);
> -	exif.setExposureTime(ret ? *entry.data.i64 : 0);
> +	/* Exif requires nsec for exposure time */
> +	exif.setExposureTime(jpegExifMetadata->sensorExposureTime * 1000);
> +	exif.setISO(jpegExifMetadata->sensorSensitivityISO);
> +

StreamBuffer has a pointer to the Camera3RequestDescriptor it belongs
to. From there you could get the Request metadata as you currently do
in CameraDevice::generateJpegExifMetadata().

What is the advantage of caching the jpegExifMetadata at
CameraDevice::requestComplete() time ?

Thanks
  j

>  	ret = requestMetadata.getEntry(ANDROID_LENS_APERTURE, &entry);
>  	if (ret)
>  		exif.setAperture(*entry.data.f);
>
> -	ret = resultMetadata->getEntry(ANDROID_SENSOR_SENSITIVITY, &entry);
> -	exif.setISO(ret ? *entry.data.i32 : 100);
> -
>  	exif.setFlash(Exif::Flash::FlashNotPresent);
>  	exif.setWhiteBalance(Exif::WhiteBalance::Auto);
>
> --
> 2.47.0.338.g60cca15819-goog
>