[libcamera-devel,v4,5/8] android: Set result metadata and EXIF fields based on request metadata
diff mbox series

Message ID 20210125071444.26252-6-paul.elder@ideasonboard.com
State Superseded
Delegated to: Paul Elder
Headers show
Series
  • Fill in android result metadata and EXIF tags
Related show

Commit Message

Paul Elder Jan. 25, 2021, 7:14 a.m. UTC 
Set the following android result metadata:
- ANDROID_LENS_FOCAL_LENGTH
- ANDROID_LENS_APERTURE
- ANDROID_JPEG_GPS_TIMESTAMP
- 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>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

---
Changes in v4:
- remove unused variable
- move android JPEG thumbnail tags allocation to the thumbnail patch
- move setting the timestamp with subsec to the patch that adds set
  subsec to timestamp ("android: jpeg: exif: Add functions for
  setting various values")
- group setting GPS-related tags

Changes in v3:
- fix metadata entries and byte count
- fix gps processing method string truncation
- move thumbnail handling to a different patch

Changes in v2:
- break out processControls and thumbnailer configuration into
  different
  patches
- fix android metadata entry number and size
- other small changes
---
 src/android/camera_device.cpp            | 19 +++++++-
 src/android/camera_stream.cpp            |  7 ++-
 src/android/camera_stream.h              |  4 +-
 src/android/jpeg/post_processor_jpeg.cpp | 62 +++++++++++++++++++-----
 src/android/jpeg/post_processor_jpeg.h   |  3 +-
 src/android/post_processor.h             |  3 +-
 6 files changed, 80 insertions(+), 18 deletions(-)

Comments

Laurent Pinchart Jan. 25, 2021, 9:54 a.m. UTC | #1 
Hi Paul,

Thank you for the patch.

On Mon, Jan 25, 2021 at 04:14:41PM +0900, Paul Elder wrote:
> Set the following android result metadata:
> - ANDROID_LENS_FOCAL_LENGTH
> - ANDROID_LENS_APERTURE
> - ANDROID_JPEG_GPS_TIMESTAMP
> - 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>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> ---
> Changes in v4:
> - remove unused variable
> - move android JPEG thumbnail tags allocation to the thumbnail patch
> - move setting the timestamp with subsec to the patch that adds set
>   subsec to timestamp ("android: jpeg: exif: Add functions for
>   setting various values")
> - group setting GPS-related tags
> 
> Changes in v3:
> - fix metadata entries and byte count
> - fix gps processing method string truncation
> - move thumbnail handling to a different patch
> 
> Changes in v2:
> - break out processControls and thumbnailer configuration into
>   different
>   patches
> - fix android metadata entry number and size
> - other small changes
> ---
>  src/android/camera_device.cpp            | 19 +++++++-
>  src/android/camera_stream.cpp            |  7 ++-
>  src/android/camera_stream.h              |  4 +-
>  src/android/jpeg/post_processor_jpeg.cpp | 62 +++++++++++++++++++-----
>  src/android/jpeg/post_processor_jpeg.h   |  3 +-
>  src/android/post_processor.h             |  3 +-
>  6 files changed, 80 insertions(+), 18 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 592e2d43..2cd3c8a1 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -1688,6 +1688,7 @@ 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 */
>  	Camera3RequestDescriptor *descriptor =
>  		new Camera3RequestDescriptor(camera_.get(), camera3Request);
>  
> @@ -1817,6 +1818,7 @@ void CameraDevice::requestComplete(Request *request)
>  		}
>  
>  		int ret = cameraStream->process(*buffer, &mapped,
> +						descriptor->settings_,
>  						resultMetadata.get());
>  		if (ret) {
>  			status = CAMERA3_BUFFER_STATUS_ERROR;
> @@ -1913,14 +1915,19 @@ CameraDevice::getResultMetadata(Camera3RequestDescriptor *descriptor,
>  
>  	/*
>  	 * \todo Keep this in sync with the actual number of entries.
> -	 * Currently: 33 entries, 75 bytes
> +	 * Currently: 38 entries, 147 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_GPS_COORDINATES (double x 3) = 24 bytes
> +	 * ANDROID_JPEG_GPS_PROCESSING_METHOD (byte x 32) = 32 bytes

Should this be 40, given the encoding prefix 

> +	 * ANDROID_JPEG_GPS_TIMESTAMP (int64) = 8 bytes
>  	 * ANDROID_JPEG_ORIENTATION (int32_t) = 3 entries, 9 bytes.
> +	 * ANDROID_LENS_APERTURE (float) = 4 bytes
> +	 * ANDROID_LENS_FOCAL_LENGTH (float) = 4 bytes
>  	 */
>  	std::unique_ptr<CameraMetadata> resultMetadata =
> -		std::make_unique<CameraMetadata>(33, 75);
> +		std::make_unique<CameraMetadata>(38, 147);
>  	if (!resultMetadata->isValid()) {
>  		LOG(HAL, Error) << "Failed to allocate static metadata";
>  		return nullptr;
> @@ -1985,6 +1992,14 @@ CameraDevice::getResultMetadata(Camera3RequestDescriptor *descriptor,
>  	value = ANDROID_FLASH_STATE_UNAVAILABLE;
>  	resultMetadata->addEntry(ANDROID_FLASH_STATE, &value, 1);
>  
> +	camera_metadata_ro_entry_t entry;
> +	int ret = descriptor->settings_.getEntry(ANDROID_LENS_APERTURE, &entry);
> +	if (ret)
> +		resultMetadata->addEntry(ANDROID_LENS_APERTURE, entry.data.f, 1);
> +
> +	float focal_length = 1.0;
> +	resultMetadata->addEntry(ANDROID_LENS_FOCAL_LENGTH, &focal_length, 1);
> +
>  	value = ANDROID_LENS_STATE_STATIONARY;
>  	resultMetadata->addEntry(ANDROID_LENS_STATE, &value, 1);
>  
> 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 c29cb352..10c71a47 100644
> --- a/src/android/jpeg/post_processor_jpeg.cpp
> +++ b/src/android/jpeg/post_processor_jpeg.cpp
> @@ -82,17 +82,26 @@ 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);
> +
> +	const uint32_t jpegOrientation = ret ? *entry.data.i32 : 0;
> +	resultMetadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpegOrientation, 1);
> +	exif.setOrientation(jpegOrientation);
> +
>  	exif.setSize(streamSize_);
>  	/*
>  	 * We set the frame's EXIF timestamp as the time of encode.
> @@ -101,6 +110,38 @@ int PostProcessorJpeg::process(const FrameBuffer &source,
>  	 */
>  	exif.setTimestamp(std::time(nullptr), std::chrono::milliseconds(0));
>  
> +	exif.setExposureTime(0);
> +	ret = requestMetadata.getEntry(ANDROID_LENS_APERTURE, &entry);
> +	if (ret)
> +		exif.setAperture(*entry.data.f);
> +	exif.setISO(100);
> +	exif.setFlash(Exif::Flash::FlashNotPresent);
> +	exif.setWhiteBalance(Exif::WhiteBalance::Auto);
> +
> +	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);
> +	}
> +
> +	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 + entry.count);
> +		exif.setGPSMethod(method);
> +		resultMetadata->addEntry(ANDROID_JPEG_GPS_PROCESSING_METHOD,
> +					 entry.data.u8, entry.count);
> +	}
> +
>  	std::vector<unsigned char> thumbnail;
>  	generateThumbnail(source, &thumbnail);
>  	if (!thumbnail.empty())
> @@ -135,13 +176,12 @@ int PostProcessorJpeg::process(const FrameBuffer &source,
>  	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);
> +	resultMetadata->addEntry(ANDROID_JPEG_SIZE, &jpeg_size, 1);
>  
> -	const uint32_t jpeg_orientation = 0;
> -	metadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpeg_orientation, 1);
> +	/* \todo Configure JPEG encoder with this */
> +	ret = requestMetadata.getEntry(ANDROID_JPEG_QUALITY, &entry);
> +	const uint32_t jpegQuality = ret ? *entry.data.u8 : 95;

s/uint32_t/uint8_t/ or the next call won't work on big endian platforms.

> +	resultMetadata->addEntry(ANDROID_JPEG_QUALITY, &jpegQuality, 1);
>  
>  	return 0;
>  }
> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h
> index 5afa831c..d721d1b9 100644
> --- a/src/android/jpeg/post_processor_jpeg.h
> +++ b/src/android/jpeg/post_processor_jpeg.h
> @@ -26,7 +26,8 @@ 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,
> 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__ */
Paul Elder Jan. 25, 2021, 10:11 a.m. UTC | #2 
Hi Laurent,

On Mon, Jan 25, 2021 at 11:54:07AM +0200, Laurent Pinchart wrote:
> Hi Paul,
> 
> Thank you for the patch.
> 
> On Mon, Jan 25, 2021 at 04:14:41PM +0900, Paul Elder wrote:
> > Set the following android result metadata:
> > - ANDROID_LENS_FOCAL_LENGTH
> > - ANDROID_LENS_APERTURE
> > - ANDROID_JPEG_GPS_TIMESTAMP
> > - 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>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > ---
> > Changes in v4:
> > - remove unused variable
> > - move android JPEG thumbnail tags allocation to the thumbnail patch
> > - move setting the timestamp with subsec to the patch that adds set
> >   subsec to timestamp ("android: jpeg: exif: Add functions for
> >   setting various values")
> > - group setting GPS-related tags
> > 
> > Changes in v3:
> > - fix metadata entries and byte count
> > - fix gps processing method string truncation
> > - move thumbnail handling to a different patch
> > 
> > Changes in v2:
> > - break out processControls and thumbnailer configuration into
> >   different
> >   patches
> > - fix android metadata entry number and size
> > - other small changes
> > ---
> >  src/android/camera_device.cpp            | 19 +++++++-
> >  src/android/camera_stream.cpp            |  7 ++-
> >  src/android/camera_stream.h              |  4 +-
> >  src/android/jpeg/post_processor_jpeg.cpp | 62 +++++++++++++++++++-----
> >  src/android/jpeg/post_processor_jpeg.h   |  3 +-
> >  src/android/post_processor.h             |  3 +-
> >  6 files changed, 80 insertions(+), 18 deletions(-)
> > 
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index 592e2d43..2cd3c8a1 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -1688,6 +1688,7 @@ 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 */
> >  	Camera3RequestDescriptor *descriptor =
> >  		new Camera3RequestDescriptor(camera_.get(), camera3Request);
> >  
> > @@ -1817,6 +1818,7 @@ void CameraDevice::requestComplete(Request *request)
> >  		}
> >  
> >  		int ret = cameraStream->process(*buffer, &mapped,
> > +						descriptor->settings_,
> >  						resultMetadata.get());
> >  		if (ret) {
> >  			status = CAMERA3_BUFFER_STATUS_ERROR;
> > @@ -1913,14 +1915,19 @@ CameraDevice::getResultMetadata(Camera3RequestDescriptor *descriptor,
> >  
> >  	/*
> >  	 * \todo Keep this in sync with the actual number of entries.
> > -	 * Currently: 33 entries, 75 bytes
> > +	 * Currently: 38 entries, 147 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_GPS_COORDINATES (double x 3) = 24 bytes
> > +	 * ANDROID_JPEG_GPS_PROCESSING_METHOD (byte x 32) = 32 bytes
> 
> Should this be 40, given the encoding prefix 

No, the encoding prefix is only for exif. Android specifically wants 32
bytes of null-terminated UTF-8.

> > +	 * ANDROID_JPEG_GPS_TIMESTAMP (int64) = 8 bytes
> >  	 * ANDROID_JPEG_ORIENTATION (int32_t) = 3 entries, 9 bytes.
> > +	 * ANDROID_LENS_APERTURE (float) = 4 bytes
> > +	 * ANDROID_LENS_FOCAL_LENGTH (float) = 4 bytes
> >  	 */
> >  	std::unique_ptr<CameraMetadata> resultMetadata =
> > -		std::make_unique<CameraMetadata>(33, 75);
> > +		std::make_unique<CameraMetadata>(38, 147);
> >  	if (!resultMetadata->isValid()) {
> >  		LOG(HAL, Error) << "Failed to allocate static metadata";
> >  		return nullptr;
> > @@ -1985,6 +1992,14 @@ CameraDevice::getResultMetadata(Camera3RequestDescriptor *descriptor,
> >  	value = ANDROID_FLASH_STATE_UNAVAILABLE;
> >  	resultMetadata->addEntry(ANDROID_FLASH_STATE, &value, 1);
> >  
> > +	camera_metadata_ro_entry_t entry;
> > +	int ret = descriptor->settings_.getEntry(ANDROID_LENS_APERTURE, &entry);
> > +	if (ret)
> > +		resultMetadata->addEntry(ANDROID_LENS_APERTURE, entry.data.f, 1);
> > +
> > +	float focal_length = 1.0;
> > +	resultMetadata->addEntry(ANDROID_LENS_FOCAL_LENGTH, &focal_length, 1);
> > +
> >  	value = ANDROID_LENS_STATE_STATIONARY;
> >  	resultMetadata->addEntry(ANDROID_LENS_STATE, &value, 1);
> >  
> > 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 c29cb352..10c71a47 100644
> > --- a/src/android/jpeg/post_processor_jpeg.cpp
> > +++ b/src/android/jpeg/post_processor_jpeg.cpp
> > @@ -82,17 +82,26 @@ 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);
> > +
> > +	const uint32_t jpegOrientation = ret ? *entry.data.i32 : 0;
> > +	resultMetadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpegOrientation, 1);
> > +	exif.setOrientation(jpegOrientation);
> > +
> >  	exif.setSize(streamSize_);
> >  	/*
> >  	 * We set the frame's EXIF timestamp as the time of encode.
> > @@ -101,6 +110,38 @@ int PostProcessorJpeg::process(const FrameBuffer &source,
> >  	 */
> >  	exif.setTimestamp(std::time(nullptr), std::chrono::milliseconds(0));
> >  
> > +	exif.setExposureTime(0);
> > +	ret = requestMetadata.getEntry(ANDROID_LENS_APERTURE, &entry);
> > +	if (ret)
> > +		exif.setAperture(*entry.data.f);
> > +	exif.setISO(100);
> > +	exif.setFlash(Exif::Flash::FlashNotPresent);
> > +	exif.setWhiteBalance(Exif::WhiteBalance::Auto);
> > +
> > +	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);
> > +	}
> > +
> > +	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 + entry.count);
> > +		exif.setGPSMethod(method);
> > +		resultMetadata->addEntry(ANDROID_JPEG_GPS_PROCESSING_METHOD,
> > +					 entry.data.u8, entry.count);
> > +	}
> > +
> >  	std::vector<unsigned char> thumbnail;
> >  	generateThumbnail(source, &thumbnail);
> >  	if (!thumbnail.empty())
> > @@ -135,13 +176,12 @@ int PostProcessorJpeg::process(const FrameBuffer &source,
> >  	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);
> > +	resultMetadata->addEntry(ANDROID_JPEG_SIZE, &jpeg_size, 1);
> >  
> > -	const uint32_t jpeg_orientation = 0;
> > -	metadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpeg_orientation, 1);
> > +	/* \todo Configure JPEG encoder with this */
> > +	ret = requestMetadata.getEntry(ANDROID_JPEG_QUALITY, &entry);
> > +	const uint32_t jpegQuality = ret ? *entry.data.u8 : 95;
> 
> s/uint32_t/uint8_t/ or the next call won't work on big endian platforms.

Forgot about it here after I fixed it in a later patch.


Thanks,

Paul

> > +	resultMetadata->addEntry(ANDROID_JPEG_QUALITY, &jpegQuality, 1);
> >  
> >  	return 0;
> >  }
> > diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h
> > index 5afa831c..d721d1b9 100644
> > --- a/src/android/jpeg/post_processor_jpeg.h
> > +++ b/src/android/jpeg/post_processor_jpeg.h
> > @@ -26,7 +26,8 @@ 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,
> > 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__ */
Jacopo Mondi Jan. 25, 2021, 11:51 a.m. UTC | #3 
Hi Paul,

On Mon, Jan 25, 2021 at 04:14:41PM +0900, Paul Elder wrote:
> Set the following android result metadata:
> - ANDROID_LENS_FOCAL_LENGTH
> - ANDROID_LENS_APERTURE
> - ANDROID_JPEG_GPS_TIMESTAMP
> - 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>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> ---
> Changes in v4:
> - remove unused variable
> - move android JPEG thumbnail tags allocation to the thumbnail patch
> - move setting the timestamp with subsec to the patch that adds set
>   subsec to timestamp ("android: jpeg: exif: Add functions for
>   setting various values")
> - group setting GPS-related tags
>
> Changes in v3:
> - fix metadata entries and byte count
> - fix gps processing method string truncation
> - move thumbnail handling to a different patch
>
> Changes in v2:
> - break out processControls and thumbnailer configuration into
>   different
>   patches
> - fix android metadata entry number and size
> - other small changes
> ---
>  src/android/camera_device.cpp            | 19 +++++++-
>  src/android/camera_stream.cpp            |  7 ++-
>  src/android/camera_stream.h              |  4 +-
>  src/android/jpeg/post_processor_jpeg.cpp | 62 +++++++++++++++++++-----
>  src/android/jpeg/post_processor_jpeg.h   |  3 +-
>  src/android/post_processor.h             |  3 +-
>  6 files changed, 80 insertions(+), 18 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 592e2d43..2cd3c8a1 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -1688,6 +1688,7 @@ 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 */

Should you know drop this ?

>  	Camera3RequestDescriptor *descriptor =
>  		new Camera3RequestDescriptor(camera_.get(), camera3Request);
>
> @@ -1817,6 +1818,7 @@ void CameraDevice::requestComplete(Request *request)
>  		}
>
>  		int ret = cameraStream->process(*buffer, &mapped,
> +						descriptor->settings_,
>  						resultMetadata.get());
>  		if (ret) {
>  			status = CAMERA3_BUFFER_STATUS_ERROR;
> @@ -1913,14 +1915,19 @@ CameraDevice::getResultMetadata(Camera3RequestDescriptor *descriptor,
>
>  	/*
>  	 * \todo Keep this in sync with the actual number of entries.
> -	 * Currently: 33 entries, 75 bytes
> +	 * Currently: 38 entries, 147 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_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_JPEG_ORIENTATION (int32_t) = 3 entries, 9 bytes.

This comment ("= 3 entries, 9 bytes") referred to the three already
populated metadata  ANDROID_JPEG_SIZE, ANDROID_JPEG_QUALITY,
ANDROID_JPEG_ORIENTATION. As you're instead listing the size of each
metadata (and I think you should summarize the total size of the
JPEG-related metadata at the end of the comment, like it used to be)
this should just say '= 4 bytes'.

Also, on my previous question about the orientation and its
relationship with the camera rotation/location: as I read from the
description of the property this should not report the requested
orientation, but rather the correction that needs to be applied to
view the image upright. So it seems it really depends on the camera
rotation/orientation.

The property description also contain a code snippet that shows how
to combine the camera rotation and location to report the correct jpeg
orientation. Could you check if it matches your understanding ?

> +	 * ANDROID_LENS_APERTURE (float) = 4 bytes
> +	 * ANDROID_LENS_FOCAL_LENGTH (float) = 4 bytes

These are not metadata set by the jpeg post-processor, but they're set
here below. You should drop them from the comment, which serves for
the purpose of reporting what metadata (and how much space they
consume) are not set here but elsewhere.

>  	 */
>  	std::unique_ptr<CameraMetadata> resultMetadata =
> -		std::make_unique<CameraMetadata>(33, 75);
> +		std::make_unique<CameraMetadata>(38, 147);

Metadata size and number of entries looks correct now!

>  	if (!resultMetadata->isValid()) {
>  		LOG(HAL, Error) << "Failed to allocate static metadata";
>  		return nullptr;
> @@ -1985,6 +1992,14 @@ CameraDevice::getResultMetadata(Camera3RequestDescriptor *descriptor,
>  	value = ANDROID_FLASH_STATE_UNAVAILABLE;
>  	resultMetadata->addEntry(ANDROID_FLASH_STATE, &value, 1);
>
> +	camera_metadata_ro_entry_t entry;
> +	int ret = descriptor->settings_.getEntry(ANDROID_LENS_APERTURE, &entry);
> +	if (ret)
> +		resultMetadata->addEntry(ANDROID_LENS_APERTURE, entry.data.f, 1);
> +
> +	float focal_length = 1.0;
> +	resultMetadata->addEntry(ANDROID_LENS_FOCAL_LENGTH, &focal_length, 1);
> +

You're here adding 5 more metadata entries, they should be listed in
the availableResultKeys vector. Don't forget to add 20 more bytes to
the static metadata pack size.

>  	value = ANDROID_LENS_STATE_STATIONARY;
>  	resultMetadata->addEntry(ANDROID_LENS_STATE, &value, 1);
>
> 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 c29cb352..10c71a47 100644
> --- a/src/android/jpeg/post_processor_jpeg.cpp
> +++ b/src/android/jpeg/post_processor_jpeg.cpp
> @@ -82,17 +82,26 @@ 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);
> +
> +	const uint32_t jpegOrientation = ret ? *entry.data.i32 : 0;
> +	resultMetadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpegOrientation, 1);
> +	exif.setOrientation(jpegOrientation);
> +
>  	exif.setSize(streamSize_);
>  	/*
>  	 * We set the frame's EXIF timestamp as the time of encode.
> @@ -101,6 +110,38 @@ int PostProcessorJpeg::process(const FrameBuffer &source,
>  	 */
>  	exif.setTimestamp(std::time(nullptr), std::chrono::milliseconds(0));
>
> +	exif.setExposureTime(0);

You should record with a \todo that onces this information is
available from the libcamera::Request::metadata, it should be come
from there. Same from some of the fields below here.

> +	ret = requestMetadata.getEntry(ANDROID_LENS_APERTURE, &entry);
> +	if (ret)
> +		exif.setAperture(*entry.data.f);
> +	exif.setISO(100);
> +	exif.setFlash(Exif::Flash::FlashNotPresent);
> +	exif.setWhiteBalance(Exif::WhiteBalance::Auto);
> +
> +	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);
> +	}
> +
> +	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 + entry.count);
> +		exif.setGPSMethod(method);
> +		resultMetadata->addEntry(ANDROID_JPEG_GPS_PROCESSING_METHOD,
> +					 entry.data.u8, entry.count);
> +	}
> +

I'm a bit puzzled the only thing we have to do is record what the
camera framework tells us to. It might be possible that Android
collects this information from the GPS sub-system and pass it down to
us to record it in the Exif header, so it's not that strange after
all...

>  	std::vector<unsigned char> thumbnail;
>  	generateThumbnail(source, &thumbnail);
>  	if (!thumbnail.empty())
> @@ -135,13 +176,12 @@ int PostProcessorJpeg::process(const FrameBuffer &source,
>  	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);
> +	resultMetadata->addEntry(ANDROID_JPEG_SIZE, &jpeg_size, 1);
>
> -	const uint32_t jpeg_orientation = 0;
> -	metadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpeg_orientation, 1);
> +	/* \todo Configure JPEG encoder with this */
> +	ret = requestMetadata.getEntry(ANDROID_JPEG_QUALITY, &entry);
> +	const uint32_t jpegQuality = ret ? *entry.data.u8 : 95;
> +	resultMetadata->addEntry(ANDROID_JPEG_QUALITY, &jpegQuality, 1);
>
>  	return 0;
>  }
> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h
> index 5afa831c..d721d1b9 100644
> --- a/src/android/jpeg/post_processor_jpeg.h
> +++ b/src/android/jpeg/post_processor_jpeg.h
> @@ -26,7 +26,8 @@ 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,
> 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
Laurent Pinchart Jan. 25, 2021, 12:55 p.m. UTC | #4 
Hi Jacopo,

On Mon, Jan 25, 2021 at 12:51:34PM +0100, Jacopo Mondi wrote:
> On Mon, Jan 25, 2021 at 04:14:41PM +0900, Paul Elder wrote:
> > Set the following android result metadata:
> > - ANDROID_LENS_FOCAL_LENGTH
> > - ANDROID_LENS_APERTURE
> > - ANDROID_JPEG_GPS_TIMESTAMP
> > - 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>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > ---
> > Changes in v4:
> > - remove unused variable
> > - move android JPEG thumbnail tags allocation to the thumbnail patch
> > - move setting the timestamp with subsec to the patch that adds set
> >   subsec to timestamp ("android: jpeg: exif: Add functions for
> >   setting various values")
> > - group setting GPS-related tags
> >
> > Changes in v3:
> > - fix metadata entries and byte count
> > - fix gps processing method string truncation
> > - move thumbnail handling to a different patch
> >
> > Changes in v2:
> > - break out processControls and thumbnailer configuration into
> >   different
> >   patches
> > - fix android metadata entry number and size
> > - other small changes
> > ---
> >  src/android/camera_device.cpp            | 19 +++++++-
> >  src/android/camera_stream.cpp            |  7 ++-
> >  src/android/camera_stream.h              |  4 +-
> >  src/android/jpeg/post_processor_jpeg.cpp | 62 +++++++++++++++++++-----
> >  src/android/jpeg/post_processor_jpeg.h   |  3 +-
> >  src/android/post_processor.h             |  3 +-
> >  6 files changed, 80 insertions(+), 18 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index 592e2d43..2cd3c8a1 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -1688,6 +1688,7 @@ 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 */
> 
> Should you know drop this ?
> 
> >  	Camera3RequestDescriptor *descriptor =
> >  		new Camera3RequestDescriptor(camera_.get(), camera3Request);
> >
> > @@ -1817,6 +1818,7 @@ void CameraDevice::requestComplete(Request *request)
> >  		}
> >
> >  		int ret = cameraStream->process(*buffer, &mapped,
> > +						descriptor->settings_,
> >  						resultMetadata.get());
> >  		if (ret) {
> >  			status = CAMERA3_BUFFER_STATUS_ERROR;
> > @@ -1913,14 +1915,19 @@ CameraDevice::getResultMetadata(Camera3RequestDescriptor *descriptor,
> >
> >  	/*
> >  	 * \todo Keep this in sync with the actual number of entries.
> > -	 * Currently: 33 entries, 75 bytes
> > +	 * Currently: 38 entries, 147 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_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_JPEG_ORIENTATION (int32_t) = 3 entries, 9 bytes.
> 
> This comment ("= 3 entries, 9 bytes") referred to the three already
> populated metadata  ANDROID_JPEG_SIZE, ANDROID_JPEG_QUALITY,
> ANDROID_JPEG_ORIENTATION. As you're instead listing the size of each
> metadata (and I think you should summarize the total size of the
> JPEG-related metadata at the end of the comment, like it used to be)
> this should just say '= 4 bytes'.
> 
> Also, on my previous question about the orientation and its
> relationship with the camera rotation/location: as I read from the
> description of the property this should not report the requested
> orientation, but rather the correction that needs to be applied to
> view the image upright. So it seems it really depends on the camera
> rotation/orientation.
> 
> The property description also contain a code snippet that shows how
> to combine the camera rotation and location to report the correct jpeg
> orientation. Could you check if it matches your understanding ?

As far as I understand, that code should be used by the camera service
to compute the JPEG orientation to pass to the HAL. From a HAL point of
view, we get an orientation that we need to apply to the JPEG image,
either by rotating the image, or by recording it in Exif. I'm not sure
why we have to report it back in dynamic metadata as it should always
match the value we get, but Android an I have no always agreed on design
choices :-)

> > +	 * ANDROID_LENS_APERTURE (float) = 4 bytes
> > +	 * ANDROID_LENS_FOCAL_LENGTH (float) = 4 bytes
> 
> These are not metadata set by the jpeg post-processor, but they're set
> here below. You should drop them from the comment, which serves for
> the purpose of reporting what metadata (and how much space they
> consume) are not set here but elsewhere.
> 
> >  	 */
> >  	std::unique_ptr<CameraMetadata> resultMetadata =
> > -		std::make_unique<CameraMetadata>(33, 75);
> > +		std::make_unique<CameraMetadata>(38, 147);
> 
> Metadata size and number of entries looks correct now!
> 
> >  	if (!resultMetadata->isValid()) {
> >  		LOG(HAL, Error) << "Failed to allocate static metadata";
> >  		return nullptr;
> > @@ -1985,6 +1992,14 @@ CameraDevice::getResultMetadata(Camera3RequestDescriptor *descriptor,
> >  	value = ANDROID_FLASH_STATE_UNAVAILABLE;
> >  	resultMetadata->addEntry(ANDROID_FLASH_STATE, &value, 1);
> >
> > +	camera_metadata_ro_entry_t entry;
> > +	int ret = descriptor->settings_.getEntry(ANDROID_LENS_APERTURE, &entry);
> > +	if (ret)
> > +		resultMetadata->addEntry(ANDROID_LENS_APERTURE, entry.data.f, 1);
> > +
> > +	float focal_length = 1.0;
> > +	resultMetadata->addEntry(ANDROID_LENS_FOCAL_LENGTH, &focal_length, 1);
> > +
> 
> You're here adding 5 more metadata entries, they should be listed in
> the availableResultKeys vector. Don't forget to add 20 more bytes to
> the static metadata pack size.
> 
> >  	value = ANDROID_LENS_STATE_STATIONARY;
> >  	resultMetadata->addEntry(ANDROID_LENS_STATE, &value, 1);
> >
> > 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 c29cb352..10c71a47 100644
> > --- a/src/android/jpeg/post_processor_jpeg.cpp
> > +++ b/src/android/jpeg/post_processor_jpeg.cpp
> > @@ -82,17 +82,26 @@ 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);
> > +
> > +	const uint32_t jpegOrientation = ret ? *entry.data.i32 : 0;
> > +	resultMetadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpegOrientation, 1);
> > +	exif.setOrientation(jpegOrientation);
> > +
> >  	exif.setSize(streamSize_);
> >  	/*
> >  	 * We set the frame's EXIF timestamp as the time of encode.
> > @@ -101,6 +110,38 @@ int PostProcessorJpeg::process(const FrameBuffer &source,
> >  	 */
> >  	exif.setTimestamp(std::time(nullptr), std::chrono::milliseconds(0));
> >
> > +	exif.setExposureTime(0);
> 
> You should record with a \todo that onces this information is
> available from the libcamera::Request::metadata, it should be come
> from there. Same from some of the fields below here.
> 
> > +	ret = requestMetadata.getEntry(ANDROID_LENS_APERTURE, &entry);
> > +	if (ret)
> > +		exif.setAperture(*entry.data.f);
> > +	exif.setISO(100);
> > +	exif.setFlash(Exif::Flash::FlashNotPresent);
> > +	exif.setWhiteBalance(Exif::WhiteBalance::Auto);
> > +
> > +	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);
> > +	}
> > +
> > +	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 + entry.count);
> > +		exif.setGPSMethod(method);
> > +		resultMetadata->addEntry(ANDROID_JPEG_GPS_PROCESSING_METHOD,
> > +					 entry.data.u8, entry.count);
> > +	}
> > +
> 
> I'm a bit puzzled the only thing we have to do is record what the
> camera framework tells us to. It might be possible that Android
> collects this information from the GPS sub-system and pass it down to
> us to record it in the Exif header, so it's not that strange after
> all...

As far as I understand, yes, that's what happens. I wonder why the
Android camera service doesn't update the Exif itself...

> >  	std::vector<unsigned char> thumbnail;
> >  	generateThumbnail(source, &thumbnail);
> >  	if (!thumbnail.empty())
> > @@ -135,13 +176,12 @@ int PostProcessorJpeg::process(const FrameBuffer &source,
> >  	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);
> > +	resultMetadata->addEntry(ANDROID_JPEG_SIZE, &jpeg_size, 1);
> >
> > -	const uint32_t jpeg_orientation = 0;
> > -	metadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpeg_orientation, 1);
> > +	/* \todo Configure JPEG encoder with this */
> > +	ret = requestMetadata.getEntry(ANDROID_JPEG_QUALITY, &entry);
> > +	const uint32_t jpegQuality = ret ? *entry.data.u8 : 95;
> > +	resultMetadata->addEntry(ANDROID_JPEG_QUALITY, &jpegQuality, 1);
> >
> >  	return 0;
> >  }
> > diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h
> > index 5afa831c..d721d1b9 100644
> > --- a/src/android/jpeg/post_processor_jpeg.h
> > +++ b/src/android/jpeg/post_processor_jpeg.h
> > @@ -26,7 +26,8 @@ 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,
> > 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__ */

Patch
diff mbox series 

diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index 592e2d43..2cd3c8a1 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -1688,6 +1688,7 @@  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 */
 	Camera3RequestDescriptor *descriptor =
 		new Camera3RequestDescriptor(camera_.get(), camera3Request);
 
@@ -1817,6 +1818,7 @@  void CameraDevice::requestComplete(Request *request)
 		}
 
 		int ret = cameraStream->process(*buffer, &mapped,
+						descriptor->settings_,
 						resultMetadata.get());
 		if (ret) {
 			status = CAMERA3_BUFFER_STATUS_ERROR;
@@ -1913,14 +1915,19 @@  CameraDevice::getResultMetadata(Camera3RequestDescriptor *descriptor,
 
 	/*
 	 * \todo Keep this in sync with the actual number of entries.
-	 * Currently: 33 entries, 75 bytes
+	 * Currently: 38 entries, 147 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_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_JPEG_ORIENTATION (int32_t) = 3 entries, 9 bytes.
+	 * ANDROID_LENS_APERTURE (float) = 4 bytes
+	 * ANDROID_LENS_FOCAL_LENGTH (float) = 4 bytes
 	 */
 	std::unique_ptr<CameraMetadata> resultMetadata =
-		std::make_unique<CameraMetadata>(33, 75);
+		std::make_unique<CameraMetadata>(38, 147);
 	if (!resultMetadata->isValid()) {
 		LOG(HAL, Error) << "Failed to allocate static metadata";
 		return nullptr;
@@ -1985,6 +1992,14 @@  CameraDevice::getResultMetadata(Camera3RequestDescriptor *descriptor,
 	value = ANDROID_FLASH_STATE_UNAVAILABLE;
 	resultMetadata->addEntry(ANDROID_FLASH_STATE, &value, 1);
 
+	camera_metadata_ro_entry_t entry;
+	int ret = descriptor->settings_.getEntry(ANDROID_LENS_APERTURE, &entry);
+	if (ret)
+		resultMetadata->addEntry(ANDROID_LENS_APERTURE, entry.data.f, 1);
+
+	float focal_length = 1.0;
+	resultMetadata->addEntry(ANDROID_LENS_FOCAL_LENGTH, &focal_length, 1);
+
 	value = ANDROID_LENS_STATE_STATIONARY;
 	resultMetadata->addEntry(ANDROID_LENS_STATE, &value, 1);
 
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 c29cb352..10c71a47 100644
--- a/src/android/jpeg/post_processor_jpeg.cpp
+++ b/src/android/jpeg/post_processor_jpeg.cpp
@@ -82,17 +82,26 @@  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);
+
+	const uint32_t jpegOrientation = ret ? *entry.data.i32 : 0;
+	resultMetadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpegOrientation, 1);
+	exif.setOrientation(jpegOrientation);
+
 	exif.setSize(streamSize_);
 	/*
 	 * We set the frame's EXIF timestamp as the time of encode.
@@ -101,6 +110,38 @@  int PostProcessorJpeg::process(const FrameBuffer &source,
 	 */
 	exif.setTimestamp(std::time(nullptr), std::chrono::milliseconds(0));
 
+	exif.setExposureTime(0);
+	ret = requestMetadata.getEntry(ANDROID_LENS_APERTURE, &entry);
+	if (ret)
+		exif.setAperture(*entry.data.f);
+	exif.setISO(100);
+	exif.setFlash(Exif::Flash::FlashNotPresent);
+	exif.setWhiteBalance(Exif::WhiteBalance::Auto);
+
+	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);
+	}
+
+	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 + entry.count);
+		exif.setGPSMethod(method);
+		resultMetadata->addEntry(ANDROID_JPEG_GPS_PROCESSING_METHOD,
+					 entry.data.u8, entry.count);
+	}
+
 	std::vector<unsigned char> thumbnail;
 	generateThumbnail(source, &thumbnail);
 	if (!thumbnail.empty())
@@ -135,13 +176,12 @@  int PostProcessorJpeg::process(const FrameBuffer &source,
 	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);
+	resultMetadata->addEntry(ANDROID_JPEG_SIZE, &jpeg_size, 1);
 
-	const uint32_t jpeg_orientation = 0;
-	metadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpeg_orientation, 1);
+	/* \todo Configure JPEG encoder with this */
+	ret = requestMetadata.getEntry(ANDROID_JPEG_QUALITY, &entry);
+	const uint32_t jpegQuality = ret ? *entry.data.u8 : 95;
+	resultMetadata->addEntry(ANDROID_JPEG_QUALITY, &jpegQuality, 1);
 
 	return 0;
 }
diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h
index 5afa831c..d721d1b9 100644
--- a/src/android/jpeg/post_processor_jpeg.h
+++ b/src/android/jpeg/post_processor_jpeg.h
@@ -26,7 +26,8 @@  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,
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__ */