[libcamera-devel] [PATCH v4 5/8] android: Set result metadata and EXIF fields based on request metadata
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Jan 25 13:55:49 CET 2021
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 at ideasonboard.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at 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__ */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list