[libcamera-devel] [PATCH 6/6] android: Set result metadata and EXIF fields based on request metadata
Jacopo Mondi
jacopo at jmondi.org
Fri Jan 15 16:48:12 CET 2021
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 at 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 at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list