[libcamera-devel] [PATCH v2 2/2] android: jpeg: Support a initial set of EXIF metadata tags
Kieran Bingham
kieran.bingham at ideasonboard.com
Wed Aug 26 10:11:55 CEST 2020
Hi Umang, Laurent,
On 26/08/2020 01:40, Laurent Pinchart wrote:
> Hi Umang,
>
> Thank you for the patch.
>
> On Tue, Aug 25, 2020 at 08:10:53PM +0000, Umang Jain wrote:
>> From: Kieran Bingham <kieran.bingham at ideasonboard.com>
>>
>> Add a Exif data placeholder inside CameraStream, to populate it
>> accordingly to the stream properties (size, orientation etc).
>> Static EXIF properties (such as make, model, size, orientation)
>> can be configured once when the stream is being configured.
>> Per-frame related metadata (such as timestamp) needs to set
>> just before encoding preferably in CameraDevice::requestComplete().
>
> As explained in the review of 1/2, I don't think we can reuse the EXIF
> generator. Sorry :-(
I'm not yet convinced, but if you really want it to be a new object each
time that can be done too, and I guess it keeps all the code setting the
exif in camera_device in one place anyway.
>
>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>> Signed-off-by: Umang Jain <email at uajain.com>
>> ---
>> src/android/camera_device.cpp | 22 +++++++++-
>> src/android/camera_device.h | 2 +
>> src/android/jpeg/encoder.h | 5 ++-
>> src/android/jpeg/encoder_libjpeg.cpp | 9 +++-
>> src/android/jpeg/encoder_libjpeg.h | 3 +-
>> src/android/jpeg/exif.cpp | 66 ++++++++++++++++++++++++++++
>> src/android/jpeg/exif.h | 9 ++++
>> 7 files changed, 112 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
>> index de6f86f..8789a9e 100644
>> --- a/src/android/camera_device.cpp
>> +++ b/src/android/camera_device.cpp
>> @@ -1245,6 +1245,17 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>> << "Failed to configure encoder";
>> return ret;
>> }
>> +
>> + /*
>> + * Set EXIF metadata for various tags.
>> + * \todo Discuss setMake String, maybe vendor ID?
>> + * KB suggested to leave it to "libcamera" for now.
>> + * setModel should use the 'model' property of the camera.
>
> Actually this should use the vendor and model of the end-product. We
> will need to load this from a platform config/data file. It can stay
> as-is for now.
Hrm, indeed I was overthinking the 'model' property from the pipeline
handler.
I think we can adjust this to
\todo Set Make and Model from external vendor tags.
>
>> + */
>> + cameraStream->exif.setMake("libcamera");
>> + cameraStream->exif.setModel("cameraModel");
>> + cameraStream->exif.setOrientation(orientation_);
>> + cameraStream->exif.setSize(cfg.size);
>> }
>> }
>>
>> @@ -1434,7 +1445,16 @@ void CameraDevice::requestComplete(Request *request)
>> continue;
>> }
>>
>> - int jpeg_size = encoder->encode(buffer, mapped.maps()[0]);
>> + /*
>> + * We set the frame's EXIF timestamp as the time of encode. Since the
>
> "the" ? the precision ?
>
>> + * we need for EXIF is only one second, it is good enough.
>> + */
>> + struct timespec tp;
>> + clock_gettime(CLOCK_REALTIME, &tp);
>> + cameraStream->exif.setTimestamp(tp.tv_sec);
>
> This can be simplified to
>
> cameraStream->exif.setTimestamp(std::time(nullptr));
>
>> + Span<uint8_t> exif_data = cameraStream->exif.generate();
>> +
>> + int jpeg_size = encoder->encode(buffer, mapped.maps()[0], exif_data);
>> if (jpeg_size < 0) {
>> LOG(HAL, Error) << "Failed to encode stream image";
>> status = CAMERA3_BUFFER_STATUS_ERROR;
>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
>> index 3934f19..3e8a119 100644
>> --- a/src/android/camera_device.h
>> +++ b/src/android/camera_device.h
>> @@ -42,6 +42,8 @@ struct CameraStream {
>> libcamera::Size size;
>>
>> Encoder *jpeg;
>> +
>> + Exif exif;
>> };
>>
>> class CameraDevice : protected libcamera::Loggable
>> diff --git a/src/android/jpeg/encoder.h b/src/android/jpeg/encoder.h
>> index f9eb88e..969f291 100644
>> --- a/src/android/jpeg/encoder.h
>> +++ b/src/android/jpeg/encoder.h
>> @@ -7,6 +7,8 @@
>> #ifndef __ANDROID_JPEG_ENCODER_H__
>> #define __ANDROID_JPEG_ENCODER_H__
>>
>> +#include "exif.h"
>> +
>
> I don't think you need to include exif.h here.
Indeed, the benefit of using the Span :-)
>
>> #include <libcamera/buffer.h>
>> #include <libcamera/span.h>
>> #include <libcamera/stream.h>
>> @@ -18,7 +20,8 @@ public:
>>
>> virtual int configure(const libcamera::StreamConfiguration &cfg) = 0;
>> virtual int encode(const libcamera::FrameBuffer *source,
>> - const libcamera::Span<uint8_t> &destination) = 0;
>> + const libcamera::Span<uint8_t> &destination,
>> + const libcamera::Span<uint8_t> &exif_data) = 0;
>
> The variable should be named exitData, or just exif.
>
>> };
>>
>> #endif /* __ANDROID_JPEG_ENCODER_H__ */
>> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp
>> index 977538c..87464a4 100644
>> --- a/src/android/jpeg/encoder_libjpeg.cpp
>> +++ b/src/android/jpeg/encoder_libjpeg.cpp
>> @@ -180,7 +180,8 @@ void EncoderLibJpeg::compressNV(const libcamera::MappedBuffer *frame)
>> }
>>
>> int EncoderLibJpeg::encode(const FrameBuffer *source,
>> - const libcamera::Span<uint8_t> &dest)
>> + const libcamera::Span<uint8_t> &dest,
>> + const libcamera::Span<uint8_t> &exif_data)
>> {
>> MappedFrameBuffer frame(source, PROT_READ);
>> if (!frame.isValid()) {
>> @@ -214,5 +215,11 @@ int EncoderLibJpeg::encode(const FrameBuffer *source,
>>
>> jpeg_finish_compress(&compress_);
>>
>> + if (exif_data.size())
>> + /* Store Exif data in the JPEG_APP1 data block. */
>> + jpeg_write_marker(&compress_, JPEG_APP0 + 1,
>> + static_cast<const JOCTET *>(exif_data.data()),
>> + exif_data.size());
>> +
>> return size;
>> }
>> diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h
>> index aed081a..88bc6e0 100644
>> --- a/src/android/jpeg/encoder_libjpeg.h
>> +++ b/src/android/jpeg/encoder_libjpeg.h
>> @@ -22,7 +22,8 @@ public:
>>
>> int configure(const libcamera::StreamConfiguration &cfg) override;
>> int encode(const libcamera::FrameBuffer *source,
>> - const libcamera::Span<uint8_t> &destination) override;
>> + const libcamera::Span<uint8_t> &destination,
>> + const libcamera::Span<uint8_t> &exif_data) override;
>>
>> private:
>> void compressRGB(const libcamera::MappedBuffer *frame);
>> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp
>> index f6a9f5c..d2ef14e 100644
>> --- a/src/android/jpeg/exif.cpp
>> +++ b/src/android/jpeg/exif.cpp
>> @@ -160,6 +160,72 @@ int Exif::setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::stri
>> return 0;
>> }
>>
>> +int Exif::setMake(const std::string &make)
>> +{
>> + return setString(EXIF_IFD_0, EXIF_TAG_MAKE, EXIF_FORMAT_ASCII, make);
>> +}
>> +
>> +int Exif::setModel(const std::string &model)
>> +{
>> + return setString(EXIF_IFD_0, EXIF_TAG_MODEL, EXIF_FORMAT_ASCII, model);
>> +}
>> +
>> +int Exif::setSize(Size size)
>> +{
>> + setShort(EXIF_IFD_0, EXIF_TAG_IMAGE_LENGTH, size.height);
>> + setLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_Y_DIMENSION, size.height);
>> +
>> + setShort(EXIF_IFD_0, EXIF_TAG_IMAGE_WIDTH, size.width);
>> + setLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_X_DIMENSION, size.width);
>> +
>> + return 0;
>> +}
>> +
>> +int Exif::setTimestamp(const time_t timestamp)
>> +{
>> + char str[40];
>> + size_t len = std::strftime(str, sizeof(str), "%c %Z",
>> + std::localtime(×tamp));
>
> The EXIF specification requires dates to be in the "YYYY:MM:DD HH:MM:SS"
> format. str can be shortened to 20.
>
>> + std::string ts(str);
>> + int ret = -1;
>> +
>> + if (len > 0) {
>
> This can't happen if str is large enough, so I'd skip this check.
>
>> + ret = setString(EXIF_IFD_0, EXIF_TAG_DATE_TIME, EXIF_FORMAT_ASCII, ts);
>> + if (ret < 0)
>> + return ret;
>> +
>> + ret = setString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_ORIGINAL, EXIF_FORMAT_ASCII, ts);
>> + if (ret < 0)
>> + return ret;
>> +
>> + ret = setString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_DIGITIZED, EXIF_FORMAT_ASCII, ts);
>> + if (ret < 0)
>> + return ret;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +
>
> Extra blank line.
>
>> +int Exif::setOrientation(int orientation)
>> +{
>> + /* Orientation's tag value computed similar to Chrome HAL. */
>
> I don't think the Chrome OS HAL is relevant :-) What's relevant is
> turning the libcamera orientation into an EXIF orientation.
>
>> + int value = 1;
>> + switch (orientation) {
>> + case 90:
>> + value = 6;
>> + break;
>> + case 180:
>> + value = 3;
>> + break;
>> + case 270:
>> + value = 8;
>> + break;
>> + }
>> +
>> + return setShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, value);
>> +}
>> +
>> Span<uint8_t> Exif::generate()
>> {
>> if (exif_data_) {
>> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h
>> index 7df83c7..6cca578 100644
>> --- a/src/android/jpeg/exif.h
>> +++ b/src/android/jpeg/exif.h
>> @@ -9,8 +9,10 @@
>>
>> #include <libexif/exif-data.h>
>>
>> +#include <libcamera/geometry.h>
>> #include <libcamera/span.h>
>>
>> +#include <ctime>
>> #include <string>
>>
>> class Exif
>> @@ -24,6 +26,13 @@ public:
>> int setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::string &item);
>> int setRational(ExifIfd ifd, ExifTag tag, uint32_t numerator, uint32_t denominator);
>>
>> + int setMake(const std::string &make);
>> + int setModel(const std::string &model);
>> +
>> + int setOrientation(int orientation);
>> + int setSize(libcamera::Size size);
>> + int setTimestamp(const time_t timestamp);
>> +
>> libcamera::Span<uint8_t> generate();
>> unsigned char *data() const { return exif_data_; }
>> unsigned int size() const { return size_; }
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list