| Message ID | 20200904064019.29869-1-email@uajain.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
On 9/4/20 12:10 PM, Umang Jain wrote: > Create a Exif object with various metadata tags set, just before > the encoder starts to encode the frame. The object is passed > directly as libcamera::Span<> to make sure EXIF tags can be set > in a single place i.e. in CameraDevice and the encoder only has > the job to write the data in the final output. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Signed-off-by: Umang Jain <email@uajain.com> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/android/camera_device.cpp | 19 ++++++++++- > src/android/jpeg/encoder.h | 3 +- > src/android/jpeg/encoder_libjpeg.cpp | 9 ++++- > src/android/jpeg/encoder_libjpeg.h | 3 +- > src/android/jpeg/exif.cpp | 50 ++++++++++++++++++++++++++++ > src/android/jpeg/exif.h | 8 +++++ > 6 files changed, 88 insertions(+), 4 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index de6f86f..0328ac6 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -24,6 +24,7 @@ > #include "system/graphics.h" > > #include "jpeg/encoder_libjpeg.h" > +#include "jpeg/exif.h" > > using namespace libcamera; > > @@ -1434,7 +1435,23 @@ void CameraDevice::requestComplete(Request *request) > continue; > } > > - int jpeg_size = encoder->encode(buffer, mapped.maps()[0]); > + /* 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(orientation_); > + exif.setSize(cameraStream->size); > + /* > + * We set the frame's EXIF timestamp as the time of encode. > + * Since the precision we need for EXIF timestamp is only one > + * second, it is good enough. > + */ > + exif.setTimestamp(std::time(nullptr)); > + if (exif.generate() != 0) > + LOG(HAL, Error) << "Failed to get valid EXIF data"; > + > + 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/jpeg/encoder.h b/src/android/jpeg/encoder.h > index f9eb88e..cf26d67 100644 > --- a/src/android/jpeg/encoder.h > +++ b/src/android/jpeg/encoder.h > @@ -18,7 +18,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<const uint8_t> &exifData) = 0; > }; > > #endif /* __ANDROID_JPEG_ENCODER_H__ */ > diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp > index 977538c..510613c 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<const uint8_t> &exifData) > { > MappedFrameBuffer frame(source, PROT_READ); > if (!frame.isValid()) { > @@ -204,6 +205,12 @@ int EncoderLibJpeg::encode(const FrameBuffer *source, > > jpeg_start_compress(&compress_, TRUE); > > + if (exifData.size()) > + /* Store Exif data in the JPEG_APP1 data block. */ > + jpeg_write_marker(&compress_, JPEG_APP0 + 1, > + static_cast<const JOCTET *>(exifData.data()), > + exifData.size()); > + > LOG(JPEG, Debug) << "JPEG Encode Starting:" << compress_.image_width > << "x" << compress_.image_height; > > diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h > index aed081a..1e8df05 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<const uint8_t> &exifData) override; > > private: > void compressRGB(const libcamera::MappedBuffer *frame); > diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp > index 41ddf48..36922ef 100644 > --- a/src/android/jpeg/exif.cpp > +++ b/src/android/jpeg/exif.cpp > @@ -168,6 +168,56 @@ void Exif::setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::str > exif_entry_unref(entry); > } > > +void Exif::setMake(const std::string &make) > +{ > + setString(EXIF_IFD_0, EXIF_TAG_MAKE, EXIF_FORMAT_ASCII, make); > +} > + > +void Exif::setModel(const std::string &model) > +{ > + setString(EXIF_IFD_0, EXIF_TAG_MODEL, EXIF_FORMAT_ASCII, model); > +} > + > +void Exif::setSize(const Size &size) > +{ > + setLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_Y_DIMENSION, size.height); > + setLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_X_DIMENSION, size.width); > +} > + > +void Exif::setTimestamp(time_t timestamp) > +{ > + char str[20]; > + std::strftime(str, sizeof(str), "%Y:%m:%d %H:%M:%S", > + std::localtime(×tamp)); > + std::string ts(str); > + > + setString(EXIF_IFD_0, EXIF_TAG_DATE_TIME, EXIF_FORMAT_ASCII, ts); > + setString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_ORIGINAL, EXIF_FORMAT_ASCII, ts); > + setString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_DIGITIZED, EXIF_FORMAT_ASCII, ts); > +} > + > +void Exif::setOrientation(int orientation) > +{ > + int value; > + switch (orientation) { > + case 0: > + default: > + value = 1; > + break; > + case 90: > + value = 6; > + break; > + case 180: > + value = 3; > + break; > + case 270: > + value = 8; > + break; > + } > + > + setShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, value); > +} > + > [[nodiscard]] int Exif::generate() > { > if (exifData_) { > diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h > index 8dfc324..622de4c 100644 > --- a/src/android/jpeg/exif.h > +++ b/src/android/jpeg/exif.h > @@ -12,6 +12,7 @@ > > #include <libexif/exif-data.h> > > +#include <libcamera/geometry.h> > #include <libcamera/span.h> > > class Exif > @@ -20,6 +21,13 @@ public: > Exif(); > ~Exif(); > > + void setMake(const std::string &make); > + void setModel(const std::string &model); > + > + void setOrientation(int orientation); > + void setSize(const libcamera::Size &size); > + void setTimestamp(time_t timestamp); > + > libcamera::Span<const uint8_t> data() const { return { exifData_, size_ }; } > [[nodiscard]] int generate(); > Stupid internet connection dropped while sending patch 2/2 and git send-email .. --in-reply-to="[libcamera-devel] [PATCH v6 0/2] Initial EXIF metadata support" didn't work as expected :(
Hi Umang, On 04/09/2020 07:43, Umang Jain wrote: <snip> >> + void setOrientation(int orientation); >> + void setSize(const libcamera::Size &size); >> + void setTimestamp(time_t timestamp); >> + >> libcamera::Span<const uint8_t> data() const { return { >> exifData_, size_ }; } >> [[nodiscard]] int generate(); >> > Stupid internet connection dropped while sending patch 2/2 > and git send-email .. --in-reply-to="[libcamera-devel] [PATCH v6 0/2] > Initial EXIF metadata support" > didn't work as expected :( To use git send-email --in-reply-to= you need to specify the 'message id' Something like this: git send-email \ --in-reply-to="<936f2cf3-80b4-e8a6-1708-ac03f8eca59a@uajain.com>"
Hi Umang, Thank you for the patch. On Fri, Sep 04, 2020 at 12:10:19PM +0530, Umang Jain wrote: > Create a Exif object with various metadata tags set, just before > the encoder starts to encode the frame. The object is passed > directly as libcamera::Span<> to make sure EXIF tags can be set > in a single place i.e. in CameraDevice and the encoder only has > the job to write the data in the final output. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Signed-off-by: Umang Jain <email@uajain.com> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/android/camera_device.cpp | 19 ++++++++++- > src/android/jpeg/encoder.h | 3 +- > src/android/jpeg/encoder_libjpeg.cpp | 9 ++++- > src/android/jpeg/encoder_libjpeg.h | 3 +- > src/android/jpeg/exif.cpp | 50 ++++++++++++++++++++++++++++ > src/android/jpeg/exif.h | 8 +++++ > 6 files changed, 88 insertions(+), 4 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index de6f86f..0328ac6 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -24,6 +24,7 @@ > #include "system/graphics.h" > > #include "jpeg/encoder_libjpeg.h" > +#include "jpeg/exif.h" > > using namespace libcamera; > > @@ -1434,7 +1435,23 @@ void CameraDevice::requestComplete(Request *request) > continue; > } > > - int jpeg_size = encoder->encode(buffer, mapped.maps()[0]); > + /* 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(orientation_); > + exif.setSize(cameraStream->size); > + /* > + * We set the frame's EXIF timestamp as the time of encode. > + * Since the precision we need for EXIF timestamp is only one > + * second, it is good enough. > + */ > + exif.setTimestamp(std::time(nullptr)); > + if (exif.generate() != 0) > + LOG(HAL, Error) << "Failed to get valid EXIF data"; s/get/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/jpeg/encoder.h b/src/android/jpeg/encoder.h > index f9eb88e..cf26d67 100644 > --- a/src/android/jpeg/encoder.h > +++ b/src/android/jpeg/encoder.h > @@ -18,7 +18,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<const uint8_t> &exifData) = 0; > }; > > #endif /* __ANDROID_JPEG_ENCODER_H__ */ > diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp > index 977538c..510613c 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<const uint8_t> &exifData) > { > MappedFrameBuffer frame(source, PROT_READ); > if (!frame.isValid()) { > @@ -204,6 +205,12 @@ int EncoderLibJpeg::encode(const FrameBuffer *source, > > jpeg_start_compress(&compress_, TRUE); > > + if (exifData.size()) > + /* Store Exif data in the JPEG_APP1 data block. */ > + jpeg_write_marker(&compress_, JPEG_APP0 + 1, > + static_cast<const JOCTET *>(exifData.data()), > + exifData.size()); > + > LOG(JPEG, Debug) << "JPEG Encode Starting:" << compress_.image_width > << "x" << compress_.image_height; > > diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h > index aed081a..1e8df05 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<const uint8_t> &exifData) override; > > private: > void compressRGB(const libcamera::MappedBuffer *frame); > diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp > index 41ddf48..36922ef 100644 > --- a/src/android/jpeg/exif.cpp > +++ b/src/android/jpeg/exif.cpp > @@ -168,6 +168,56 @@ void Exif::setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::str > exif_entry_unref(entry); > } > > +void Exif::setMake(const std::string &make) > +{ > + setString(EXIF_IFD_0, EXIF_TAG_MAKE, EXIF_FORMAT_ASCII, make); > +} > + > +void Exif::setModel(const std::string &model) > +{ > + setString(EXIF_IFD_0, EXIF_TAG_MODEL, EXIF_FORMAT_ASCII, model); > +} > + > +void Exif::setSize(const Size &size) > +{ > + setLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_Y_DIMENSION, size.height); > + setLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_X_DIMENSION, size.width); > +} > + > +void Exif::setTimestamp(time_t timestamp) > +{ > + char str[20]; > + std::strftime(str, sizeof(str), "%Y:%m:%d %H:%M:%S", > + std::localtime(×tamp)); > + std::string ts(str); > + > + setString(EXIF_IFD_0, EXIF_TAG_DATE_TIME, EXIF_FORMAT_ASCII, ts); > + setString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_ORIGINAL, EXIF_FORMAT_ASCII, ts); > + setString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_DIGITIZED, EXIF_FORMAT_ASCII, ts); > +} > + > +void Exif::setOrientation(int orientation) > +{ > + int value; > + switch (orientation) { > + case 0: > + default: > + value = 1; > + break; > + case 90: > + value = 6; > + break; > + case 180: > + value = 3; > + break; > + case 270: > + value = 8; > + break; Shouldn't the the 90 and 270 values be swapped ? 6 means "The 0th row is the visual right-hand side of the image, and the 0th column is the visual top", and 8 means "The 0th row is the visual left-hand side of the image, and the 0th column is the visual bottom". Looking at the rotation property definition, the 90° rotation examples shows +-------------------------------------+ | _ _ | | \ / | | | | | | | | | | | > | | < | | | | | | | . | | V | +-------------------------------------+ where the 0th row is the left side and the 0th column the bottom side. I can fix those two small issues when applying, but I'd appreciate if someone could double-check the rotation. > + } > + > + setShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, value); > +} > + > [[nodiscard]] int Exif::generate() > { > if (exifData_) { > diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h > index 8dfc324..622de4c 100644 > --- a/src/android/jpeg/exif.h > +++ b/src/android/jpeg/exif.h > @@ -12,6 +12,7 @@ > > #include <libexif/exif-data.h> > > +#include <libcamera/geometry.h> > #include <libcamera/span.h> > > class Exif > @@ -20,6 +21,13 @@ public: > Exif(); > ~Exif(); > > + void setMake(const std::string &make); > + void setModel(const std::string &model); > + > + void setOrientation(int orientation); > + void setSize(const libcamera::Size &size); > + void setTimestamp(time_t timestamp); > + > libcamera::Span<const uint8_t> data() const { return { exifData_, size_ }; } > [[nodiscard]] int generate(); >
Hi Laurent, On 9/4/20 9:04 PM, Laurent Pinchart wrote: > Hi Umang, > > Thank you for the patch. > > On Fri, Sep 04, 2020 at 12:10:19PM +0530, Umang Jain wrote: >> Create a Exif object with various metadata tags set, just before >> the encoder starts to encode the frame. The object is passed >> directly as libcamera::Span<> to make sure EXIF tags can be set >> in a single place i.e. in CameraDevice and the encoder only has >> the job to write the data in the final output. >> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> Signed-off-by: Umang Jain <email@uajain.com> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> --- >> src/android/camera_device.cpp | 19 ++++++++++- >> src/android/jpeg/encoder.h | 3 +- >> src/android/jpeg/encoder_libjpeg.cpp | 9 ++++- >> src/android/jpeg/encoder_libjpeg.h | 3 +- >> src/android/jpeg/exif.cpp | 50 ++++++++++++++++++++++++++++ >> src/android/jpeg/exif.h | 8 +++++ >> 6 files changed, 88 insertions(+), 4 deletions(-) >> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp >> index de6f86f..0328ac6 100644 >> --- a/src/android/camera_device.cpp >> +++ b/src/android/camera_device.cpp >> @@ -24,6 +24,7 @@ >> #include "system/graphics.h" >> >> #include "jpeg/encoder_libjpeg.h" >> +#include "jpeg/exif.h" >> >> using namespace libcamera; >> >> @@ -1434,7 +1435,23 @@ void CameraDevice::requestComplete(Request *request) >> continue; >> } >> >> - int jpeg_size = encoder->encode(buffer, mapped.maps()[0]); >> + /* 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(orientation_); >> + exif.setSize(cameraStream->size); >> + /* >> + * We set the frame's EXIF timestamp as the time of encode. >> + * Since the precision we need for EXIF timestamp is only one >> + * second, it is good enough. >> + */ >> + exif.setTimestamp(std::time(nullptr)); >> + if (exif.generate() != 0) >> + LOG(HAL, Error) << "Failed to get valid EXIF data"; > s/get/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/jpeg/encoder.h b/src/android/jpeg/encoder.h >> index f9eb88e..cf26d67 100644 >> --- a/src/android/jpeg/encoder.h >> +++ b/src/android/jpeg/encoder.h >> @@ -18,7 +18,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<const uint8_t> &exifData) = 0; >> }; >> >> #endif /* __ANDROID_JPEG_ENCODER_H__ */ >> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp >> index 977538c..510613c 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<const uint8_t> &exifData) >> { >> MappedFrameBuffer frame(source, PROT_READ); >> if (!frame.isValid()) { >> @@ -204,6 +205,12 @@ int EncoderLibJpeg::encode(const FrameBuffer *source, >> >> jpeg_start_compress(&compress_, TRUE); >> >> + if (exifData.size()) >> + /* Store Exif data in the JPEG_APP1 data block. */ >> + jpeg_write_marker(&compress_, JPEG_APP0 + 1, >> + static_cast<const JOCTET *>(exifData.data()), >> + exifData.size()); >> + >> LOG(JPEG, Debug) << "JPEG Encode Starting:" << compress_.image_width >> << "x" << compress_.image_height; >> >> diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h >> index aed081a..1e8df05 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<const uint8_t> &exifData) override; >> >> private: >> void compressRGB(const libcamera::MappedBuffer *frame); >> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp >> index 41ddf48..36922ef 100644 >> --- a/src/android/jpeg/exif.cpp >> +++ b/src/android/jpeg/exif.cpp >> @@ -168,6 +168,56 @@ void Exif::setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::str >> exif_entry_unref(entry); >> } >> >> +void Exif::setMake(const std::string &make) >> +{ >> + setString(EXIF_IFD_0, EXIF_TAG_MAKE, EXIF_FORMAT_ASCII, make); >> +} >> + >> +void Exif::setModel(const std::string &model) >> +{ >> + setString(EXIF_IFD_0, EXIF_TAG_MODEL, EXIF_FORMAT_ASCII, model); >> +} >> + >> +void Exif::setSize(const Size &size) >> +{ >> + setLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_Y_DIMENSION, size.height); >> + setLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_X_DIMENSION, size.width); >> +} >> + >> +void Exif::setTimestamp(time_t timestamp) >> +{ >> + char str[20]; >> + std::strftime(str, sizeof(str), "%Y:%m:%d %H:%M:%S", >> + std::localtime(×tamp)); >> + std::string ts(str); >> + >> + setString(EXIF_IFD_0, EXIF_TAG_DATE_TIME, EXIF_FORMAT_ASCII, ts); >> + setString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_ORIGINAL, EXIF_FORMAT_ASCII, ts); >> + setString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_DIGITIZED, EXIF_FORMAT_ASCII, ts); >> +} >> + >> +void Exif::setOrientation(int orientation) >> +{ >> + int value; >> + switch (orientation) { >> + case 0: >> + default: >> + value = 1; >> + break; >> + case 90: >> + value = 6; >> + break; >> + case 180: >> + value = 3; >> + break; >> + case 270: >> + value = 8; >> + break; > Shouldn't the the 90 and 270 values be swapped ? 6 means "The 0th row is > the visual right-hand side of the image, and the 0th column is the > visual top", and 8 means "The 0th row is the visual left-hand side of > the image, and the 0th column is the visual bottom". > > Looking at the rotation property definition, the 90° rotation examples > shows > > +-------------------------------------+ > | _ _ | > | \ / | > | | | | > | | | | > | | > | > | < | | > | | | | > | . | > | V | > +-------------------------------------+ > > where the 0th row is the left side and the 0th column the bottom side. > > I can fix those two small issues when applying, but I'd appreciate if > someone could double-check the rotation. Does the rotation property has to do anything with the fact that, camera sensor(webcam use-case) is intentionally mounted upside down to avoid rotation correction? A widely used example on the internet is a big 'F' image denoting the value according to it's orientation [1], which is the same as per the values used in the patch. On the other hand, I can also find the code which satisfies Laurent's review and the EXIF 'orientation' specification [2] [1]: https://chromium.googlesource.com/chromiumos/platform2/+/HEAD/camera/common/exif_utils.cc#376 [2]: https://piexif.readthedocs.io/en/latest/sample.html#rotate-image-by-exif-orientation ¯\_(ツ)_/¯ > >> + } >> + >> + setShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, value); >> +} >> + >> [[nodiscard]] int Exif::generate() >> { >> if (exifData_) { >> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h >> index 8dfc324..622de4c 100644 >> --- a/src/android/jpeg/exif.h >> +++ b/src/android/jpeg/exif.h >> @@ -12,6 +12,7 @@ >> >> #include <libexif/exif-data.h> >> >> +#include <libcamera/geometry.h> >> #include <libcamera/span.h> >> >> class Exif >> @@ -20,6 +21,13 @@ public: >> Exif(); >> ~Exif(); >> >> + void setMake(const std::string &make); >> + void setModel(const std::string &model); >> + >> + void setOrientation(int orientation); >> + void setSize(const libcamera::Size &size); >> + void setTimestamp(time_t timestamp); >> + >> libcamera::Span<const uint8_t> data() const { return { exifData_, size_ }; } >> [[nodiscard]] int generate(); >>
Hi Umang, On Tue, Sep 08, 2020 at 03:42:39PM +0530, Umang Jain wrote: > On 9/4/20 9:04 PM, Laurent Pinchart wrote: > > On Fri, Sep 04, 2020 at 12:10:19PM +0530, Umang Jain wrote: > >> Create a Exif object with various metadata tags set, just before > >> the encoder starts to encode the frame. The object is passed > >> directly as libcamera::Span<> to make sure EXIF tags can be set > >> in a single place i.e. in CameraDevice and the encoder only has > >> the job to write the data in the final output. > >> > >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> Signed-off-by: Umang Jain <email@uajain.com> > >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> --- > >> src/android/camera_device.cpp | 19 ++++++++++- > >> src/android/jpeg/encoder.h | 3 +- > >> src/android/jpeg/encoder_libjpeg.cpp | 9 ++++- > >> src/android/jpeg/encoder_libjpeg.h | 3 +- > >> src/android/jpeg/exif.cpp | 50 ++++++++++++++++++++++++++++ > >> src/android/jpeg/exif.h | 8 +++++ > >> 6 files changed, 88 insertions(+), 4 deletions(-) > >> > >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > >> index de6f86f..0328ac6 100644 > >> --- a/src/android/camera_device.cpp > >> +++ b/src/android/camera_device.cpp > >> @@ -24,6 +24,7 @@ > >> #include "system/graphics.h" > >> > >> #include "jpeg/encoder_libjpeg.h" > >> +#include "jpeg/exif.h" > >> > >> using namespace libcamera; > >> > >> @@ -1434,7 +1435,23 @@ void CameraDevice::requestComplete(Request *request) > >> continue; > >> } > >> > >> - int jpeg_size = encoder->encode(buffer, mapped.maps()[0]); > >> + /* 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(orientation_); > >> + exif.setSize(cameraStream->size); > >> + /* > >> + * We set the frame's EXIF timestamp as the time of encode. > >> + * Since the precision we need for EXIF timestamp is only one > >> + * second, it is good enough. > >> + */ > >> + exif.setTimestamp(std::time(nullptr)); > >> + if (exif.generate() != 0) > >> + LOG(HAL, Error) << "Failed to get valid EXIF data"; > > > > s/get/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/jpeg/encoder.h b/src/android/jpeg/encoder.h > >> index f9eb88e..cf26d67 100644 > >> --- a/src/android/jpeg/encoder.h > >> +++ b/src/android/jpeg/encoder.h > >> @@ -18,7 +18,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<const uint8_t> &exifData) = 0; > >> }; > >> > >> #endif /* __ANDROID_JPEG_ENCODER_H__ */ > >> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp > >> index 977538c..510613c 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<const uint8_t> &exifData) > >> { > >> MappedFrameBuffer frame(source, PROT_READ); > >> if (!frame.isValid()) { > >> @@ -204,6 +205,12 @@ int EncoderLibJpeg::encode(const FrameBuffer *source, > >> > >> jpeg_start_compress(&compress_, TRUE); > >> > >> + if (exifData.size()) > >> + /* Store Exif data in the JPEG_APP1 data block. */ > >> + jpeg_write_marker(&compress_, JPEG_APP0 + 1, > >> + static_cast<const JOCTET *>(exifData.data()), > >> + exifData.size()); > >> + > >> LOG(JPEG, Debug) << "JPEG Encode Starting:" << compress_.image_width > >> << "x" << compress_.image_height; > >> > >> diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h > >> index aed081a..1e8df05 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<const uint8_t> &exifData) override; > >> > >> private: > >> void compressRGB(const libcamera::MappedBuffer *frame); > >> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp > >> index 41ddf48..36922ef 100644 > >> --- a/src/android/jpeg/exif.cpp > >> +++ b/src/android/jpeg/exif.cpp > >> @@ -168,6 +168,56 @@ void Exif::setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::str > >> exif_entry_unref(entry); > >> } > >> > >> +void Exif::setMake(const std::string &make) > >> +{ > >> + setString(EXIF_IFD_0, EXIF_TAG_MAKE, EXIF_FORMAT_ASCII, make); > >> +} > >> + > >> +void Exif::setModel(const std::string &model) > >> +{ > >> + setString(EXIF_IFD_0, EXIF_TAG_MODEL, EXIF_FORMAT_ASCII, model); > >> +} > >> + > >> +void Exif::setSize(const Size &size) > >> +{ > >> + setLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_Y_DIMENSION, size.height); > >> + setLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_X_DIMENSION, size.width); > >> +} > >> + > >> +void Exif::setTimestamp(time_t timestamp) > >> +{ > >> + char str[20]; > >> + std::strftime(str, sizeof(str), "%Y:%m:%d %H:%M:%S", > >> + std::localtime(×tamp)); > >> + std::string ts(str); > >> + > >> + setString(EXIF_IFD_0, EXIF_TAG_DATE_TIME, EXIF_FORMAT_ASCII, ts); > >> + setString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_ORIGINAL, EXIF_FORMAT_ASCII, ts); > >> + setString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_DIGITIZED, EXIF_FORMAT_ASCII, ts); > >> +} > >> + > >> +void Exif::setOrientation(int orientation) > >> +{ > >> + int value; > >> + switch (orientation) { > >> + case 0: > >> + default: > >> + value = 1; > >> + break; > >> + case 90: > >> + value = 6; > >> + break; > >> + case 180: > >> + value = 3; > >> + break; > >> + case 270: > >> + value = 8; > >> + break; > > > > Shouldn't the the 90 and 270 values be swapped ? 6 means "The 0th row is > > the visual right-hand side of the image, and the 0th column is the > > visual top", and 8 means "The 0th row is the visual left-hand side of > > the image, and the 0th column is the visual bottom". > > > > Looking at the rotation property definition, the 90° rotation examples > > shows > > > > +-------------------------------------+ > > | _ _ | > > | \ / | > > | | | | > > | | | | > > | | > | > > | < | | > > | | | | > > | . | > > | V | > > +-------------------------------------+ > > > > where the 0th row is the left side and the 0th column the bottom side. > > > > I can fix those two small issues when applying, but I'd appreciate if > > someone could double-check the rotation. > > Does the rotation property has to do anything with the fact that, camera > sensor(webcam use-case) is intentionally mounted upside down to avoid > rotation correction? The rotation reported by libcamera exposes the optical rotation of the camera module, combining both the camera sensor and the lens. It is indeed common to mount the camera sensor upside-down to compensate for the lens inverting the image. In that case the reported rotation is 0°, as the camera sensor rotation and the lens inversion compensate each other. > A widely used example on the internet is a big 'F' > image denoting the value according to it's orientation [1], which is the > same as per the values used in the patch. On the other hand, I can also > find the code which satisfies Laurent's review and the EXIF > 'orientation' specification [2] > > [1]: > https://chromium.googlesource.com/chromiumos/platform2/+/HEAD/camera/common/exif_utils.cc#376 > [2]: > https://piexif.readthedocs.io/en/latest/sample.html#rotate-image-by-exif-orientation The 0° and 180° cases are fairly straightforward, they map to the EXIF values 1 and 3 respectively, there's usually no disagreement on that. The 90° and 270° cases are much more error prone as it's easy to get the rotation direction wrong. The mapping between the 'F' orientation and the EXIF values shown in [1] seems right to me. Android defines the rotation as the clockwise angle by which the image needs to be rotated to appear in the correct orientation on the device screen. For EXIF value 6, 88 88 88 8888888888 the image needs to be rotated by 90° clockwise, while for EXIF value 8, 8888888888 88 88 88 it needs to be rotated by 270°. The code in [2] thus appears correct. What however appears to be incorrect is our claim that the libcamera and Android numerical values are the same. Jacopo, any opinion on that ? > ¯\_(ツ)_/¯ > > >> + } > >> + > >> + setShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, value); > >> +} > >> + > >> [[nodiscard]] int Exif::generate() > >> { > >> if (exifData_) { > >> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h > >> index 8dfc324..622de4c 100644 > >> --- a/src/android/jpeg/exif.h > >> +++ b/src/android/jpeg/exif.h > >> @@ -12,6 +12,7 @@ > >> > >> #include <libexif/exif-data.h> > >> > >> +#include <libcamera/geometry.h> > >> #include <libcamera/span.h> > >> > >> class Exif > >> @@ -20,6 +21,13 @@ public: > >> Exif(); > >> ~Exif(); > >> > >> + void setMake(const std::string &make); > >> + void setModel(const std::string &model); > >> + > >> + void setOrientation(int orientation); > >> + void setSize(const libcamera::Size &size); > >> + void setTimestamp(time_t timestamp); > >> + > >> libcamera::Span<const uint8_t> data() const { return { exifData_, size_ }; } > >> [[nodiscard]] int generate(); > >>
Hi Laurent, Umang, On Tue, Sep 08, 2020 at 03:03:21PM +0300, Laurent Pinchart wrote: > Hi Umang, > > On Tue, Sep 08, 2020 at 03:42:39PM +0530, Umang Jain wrote: > > On 9/4/20 9:04 PM, Laurent Pinchart wrote: > > > On Fri, Sep 04, 2020 at 12:10:19PM +0530, Umang Jain wrote: > > >> Create a Exif object with various metadata tags set, just before > > >> the encoder starts to encode the frame. The object is passed > > >> directly as libcamera::Span<> to make sure EXIF tags can be set > > >> in a single place i.e. in CameraDevice and the encoder only has > > >> the job to write the data in the final output. > > >> > > >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > >> Signed-off-by: Umang Jain <email@uajain.com> > > >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > >> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > >> --- > > >> src/android/camera_device.cpp | 19 ++++++++++- > > >> src/android/jpeg/encoder.h | 3 +- > > >> src/android/jpeg/encoder_libjpeg.cpp | 9 ++++- > > >> src/android/jpeg/encoder_libjpeg.h | 3 +- > > >> src/android/jpeg/exif.cpp | 50 ++++++++++++++++++++++++++++ > > >> src/android/jpeg/exif.h | 8 +++++ > > >> 6 files changed, 88 insertions(+), 4 deletions(-) > > >> > > >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > >> index de6f86f..0328ac6 100644 > > >> --- a/src/android/camera_device.cpp > > >> +++ b/src/android/camera_device.cpp > > >> @@ -24,6 +24,7 @@ > > >> #include "system/graphics.h" > > >> > > >> #include "jpeg/encoder_libjpeg.h" > > >> +#include "jpeg/exif.h" > > >> > > >> using namespace libcamera; > > >> > > >> @@ -1434,7 +1435,23 @@ void CameraDevice::requestComplete(Request *request) > > >> continue; > > >> } > > >> > > >> - int jpeg_size = encoder->encode(buffer, mapped.maps()[0]); > > >> + /* 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(orientation_); > > >> + exif.setSize(cameraStream->size); > > >> + /* > > >> + * We set the frame's EXIF timestamp as the time of encode. > > >> + * Since the precision we need for EXIF timestamp is only one > > >> + * second, it is good enough. > > >> + */ > > >> + exif.setTimestamp(std::time(nullptr)); > > >> + if (exif.generate() != 0) > > >> + LOG(HAL, Error) << "Failed to get valid EXIF data"; > > > > > > s/get/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/jpeg/encoder.h b/src/android/jpeg/encoder.h > > >> index f9eb88e..cf26d67 100644 > > >> --- a/src/android/jpeg/encoder.h > > >> +++ b/src/android/jpeg/encoder.h > > >> @@ -18,7 +18,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<const uint8_t> &exifData) = 0; > > >> }; > > >> > > >> #endif /* __ANDROID_JPEG_ENCODER_H__ */ > > >> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp > > >> index 977538c..510613c 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<const uint8_t> &exifData) > > >> { > > >> MappedFrameBuffer frame(source, PROT_READ); > > >> if (!frame.isValid()) { > > >> @@ -204,6 +205,12 @@ int EncoderLibJpeg::encode(const FrameBuffer *source, > > >> > > >> jpeg_start_compress(&compress_, TRUE); > > >> > > >> + if (exifData.size()) > > >> + /* Store Exif data in the JPEG_APP1 data block. */ > > >> + jpeg_write_marker(&compress_, JPEG_APP0 + 1, > > >> + static_cast<const JOCTET *>(exifData.data()), > > >> + exifData.size()); > > >> + > > >> LOG(JPEG, Debug) << "JPEG Encode Starting:" << compress_.image_width > > >> << "x" << compress_.image_height; > > >> > > >> diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h > > >> index aed081a..1e8df05 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<const uint8_t> &exifData) override; > > >> > > >> private: > > >> void compressRGB(const libcamera::MappedBuffer *frame); > > >> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp > > >> index 41ddf48..36922ef 100644 > > >> --- a/src/android/jpeg/exif.cpp > > >> +++ b/src/android/jpeg/exif.cpp > > >> @@ -168,6 +168,56 @@ void Exif::setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::str > > >> exif_entry_unref(entry); > > >> } > > >> > > >> +void Exif::setMake(const std::string &make) > > >> +{ > > >> + setString(EXIF_IFD_0, EXIF_TAG_MAKE, EXIF_FORMAT_ASCII, make); > > >> +} > > >> + > > >> +void Exif::setModel(const std::string &model) > > >> +{ > > >> + setString(EXIF_IFD_0, EXIF_TAG_MODEL, EXIF_FORMAT_ASCII, model); > > >> +} > > >> + > > >> +void Exif::setSize(const Size &size) > > >> +{ > > >> + setLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_Y_DIMENSION, size.height); > > >> + setLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_X_DIMENSION, size.width); > > >> +} > > >> + > > >> +void Exif::setTimestamp(time_t timestamp) > > >> +{ > > >> + char str[20]; > > >> + std::strftime(str, sizeof(str), "%Y:%m:%d %H:%M:%S", > > >> + std::localtime(×tamp)); > > >> + std::string ts(str); > > >> + > > >> + setString(EXIF_IFD_0, EXIF_TAG_DATE_TIME, EXIF_FORMAT_ASCII, ts); > > >> + setString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_ORIGINAL, EXIF_FORMAT_ASCII, ts); > > >> + setString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_DIGITIZED, EXIF_FORMAT_ASCII, ts); > > >> +} > > >> + > > >> +void Exif::setOrientation(int orientation) > > >> +{ > > >> + int value; > > >> + switch (orientation) { > > >> + case 0: > > >> + default: > > >> + value = 1; > > >> + break; > > >> + case 90: > > >> + value = 6; > > >> + break; > > >> + case 180: > > >> + value = 3; > > >> + break; > > >> + case 270: > > >> + value = 8; > > >> + break; > > > > > > Shouldn't the the 90 and 270 values be swapped ? 6 means "The 0th row is > > > the visual right-hand side of the image, and the 0th column is the > > > visual top", and 8 means "The 0th row is the visual left-hand side of > > > the image, and the 0th column is the visual bottom". > > > > > > Looking at the rotation property definition, the 90° rotation examples > > > shows > > > > > > +-------------------------------------+ > > > | _ _ | > > > | \ / | > > > | | | | > > > | | | | > > > | | > | > > > | < | | > > > | | | | > > > | . | > > > | V | > > > +-------------------------------------+ > > > > > > where the 0th row is the left side and the 0th column the bottom side. > > > Isn't the 0-th row (the first row of the image) on the 'visual right-hand side' ? > > > I can fix those two small issues when applying, but I'd appreciate if > > > someone could double-check the rotation. > > > > Does the rotation property has to do anything with the fact that, camera > > sensor(webcam use-case) is intentionally mounted upside down to avoid > > rotation correction? > > The rotation reported by libcamera exposes the optical rotation of the > camera module, combining both the camera sensor and the lens. It is > indeed common to mount the camera sensor upside-down to compensate for > the lens inverting the image. In that case the reported rotation is 0°, > as the camera sensor rotation and the lens inversion compensate each > other. > The 'rotation' property definition takes into account the lens inversion, so you shouldn't have to care about it > > A widely used example on the internet is a big 'F' > > image denoting the value according to it's orientation [1], which is the > > same as per the values used in the patch. On the other hand, I can also > > find the code which satisfies Laurent's review and the EXIF > > 'orientation' specification [2] > > > > [1]: > > https://chromium.googlesource.com/chromiumos/platform2/+/HEAD/camera/common/exif_utils.cc#376 > > [2]: > > https://piexif.readthedocs.io/en/latest/sample.html#rotate-image-by-exif-orientation > > The 0° and 180° cases are fairly straightforward, they map to the EXIF > values 1 and 3 respectively, there's usually no disagreement on that. > > The 90° and 270° cases are much more error prone as it's easy to get the > rotation direction wrong. The mapping between the 'F' orientation and > the EXIF values shown in [1] seems right to me. > > Android defines the rotation as the clockwise angle by which the image > needs to be rotated to appear in the correct orientation on the device > screen. > > For EXIF value 6, > > 88 > 88 88 > 8888888888 > > the image needs to be rotated by 90° clockwise, while for EXIF value 8, > > 8888888888 > 88 88 > 88 > > it needs to be rotated by 270°. The code in [2] thus appears correct. > What however appears to be incorrect is our claim that the libcamera and > Android numerical values are the same. > > Jacopo, any opinion on that ? > Our definition differs from the Android one, as the rotation value we report is the rotation correction degrees in counter-clockwise direction. Android defines that in the clockwise direction. If I got the above discussion right and looking at the 'F' examples in [1], we should report: 90 degrees = 8 270 degrees = 6 However, this interpreation and the 'F' examples, don't match the textual description Laurent reported above: ------------------------------------------------------------------------------- Shouldn't the the 90 and 270 values be swapped ? 6 means "The 0th row is the visual right-hand side of the image, and the 0th column is the visual top", and 8 means "The 0th row is the visual left-hand side of the image, and the 0th column is the visual bottom". ------------------------------------------------------------------------------ For EXIF value 6, 88 88 88 8888888888 the image needs to be rotated by 90° clockwise, while for EXIF value 8, 8888888888 88 88 88 ------------------------------------------------------------------------------ The two do not match, and I wonder what's the authoritative source for the definition of the semantic associated with the '6' and '8' values. Thanks j > > ¯\_(ツ)_/¯ > > > > >> + } > > >> + > > >> + setShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, value); > > >> +} > > >> + > > >> [[nodiscard]] int Exif::generate() > > >> { > > >> if (exifData_) { > > >> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h > > >> index 8dfc324..622de4c 100644 > > >> --- a/src/android/jpeg/exif.h > > >> +++ b/src/android/jpeg/exif.h > > >> @@ -12,6 +12,7 @@ > > >> > > >> #include <libexif/exif-data.h> > > >> > > >> +#include <libcamera/geometry.h> > > >> #include <libcamera/span.h> > > >> > > >> class Exif > > >> @@ -20,6 +21,13 @@ public: > > >> Exif(); > > >> ~Exif(); > > >> > > >> + void setMake(const std::string &make); > > >> + void setModel(const std::string &model); > > >> + > > >> + void setOrientation(int orientation); > > >> + void setSize(const libcamera::Size &size); > > >> + void setTimestamp(time_t timestamp); > > >> + > > >> libcamera::Span<const uint8_t> data() const { return { exifData_, size_ }; } > > >> [[nodiscard]] int generate(); > > >> > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Tue, Sep 08, 2020 at 02:43:46PM +0200, Jacopo Mondi wrote: > On Tue, Sep 08, 2020 at 03:03:21PM +0300, Laurent Pinchart wrote: > > On Tue, Sep 08, 2020 at 03:42:39PM +0530, Umang Jain wrote: > >> On 9/4/20 9:04 PM, Laurent Pinchart wrote: > >>> On Fri, Sep 04, 2020 at 12:10:19PM +0530, Umang Jain wrote: > >>>> Create a Exif object with various metadata tags set, just before > >>>> the encoder starts to encode the frame. The object is passed > >>>> directly as libcamera::Span<> to make sure EXIF tags can be set > >>>> in a single place i.e. in CameraDevice and the encoder only has > >>>> the job to write the data in the final output. > >>>> > >>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >>>> Signed-off-by: Umang Jain <email@uajain.com> > >>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>>> --- > >>>> src/android/camera_device.cpp | 19 ++++++++++- > >>>> src/android/jpeg/encoder.h | 3 +- > >>>> src/android/jpeg/encoder_libjpeg.cpp | 9 ++++- > >>>> src/android/jpeg/encoder_libjpeg.h | 3 +- > >>>> src/android/jpeg/exif.cpp | 50 ++++++++++++++++++++++++++++ > >>>> src/android/jpeg/exif.h | 8 +++++ > >>>> 6 files changed, 88 insertions(+), 4 deletions(-) > >>>> > >>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > >>>> index de6f86f..0328ac6 100644 > >>>> --- a/src/android/camera_device.cpp > >>>> +++ b/src/android/camera_device.cpp > >>>> @@ -24,6 +24,7 @@ > >>>> #include "system/graphics.h" > >>>> > >>>> #include "jpeg/encoder_libjpeg.h" > >>>> +#include "jpeg/exif.h" > >>>> > >>>> using namespace libcamera; > >>>> > >>>> @@ -1434,7 +1435,23 @@ void CameraDevice::requestComplete(Request *request) > >>>> continue; > >>>> } > >>>> > >>>> - int jpeg_size = encoder->encode(buffer, mapped.maps()[0]); > >>>> + /* 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(orientation_); > >>>> + exif.setSize(cameraStream->size); > >>>> + /* > >>>> + * We set the frame's EXIF timestamp as the time of encode. > >>>> + * Since the precision we need for EXIF timestamp is only one > >>>> + * second, it is good enough. > >>>> + */ > >>>> + exif.setTimestamp(std::time(nullptr)); > >>>> + if (exif.generate() != 0) > >>>> + LOG(HAL, Error) << "Failed to get valid EXIF data"; > >>> > >>> s/get/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/jpeg/encoder.h b/src/android/jpeg/encoder.h > >>>> index f9eb88e..cf26d67 100644 > >>>> --- a/src/android/jpeg/encoder.h > >>>> +++ b/src/android/jpeg/encoder.h > >>>> @@ -18,7 +18,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<const uint8_t> &exifData) = 0; > >>>> }; > >>>> > >>>> #endif /* __ANDROID_JPEG_ENCODER_H__ */ > >>>> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp > >>>> index 977538c..510613c 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<const uint8_t> &exifData) > >>>> { > >>>> MappedFrameBuffer frame(source, PROT_READ); > >>>> if (!frame.isValid()) { > >>>> @@ -204,6 +205,12 @@ int EncoderLibJpeg::encode(const FrameBuffer *source, > >>>> > >>>> jpeg_start_compress(&compress_, TRUE); > >>>> > >>>> + if (exifData.size()) > >>>> + /* Store Exif data in the JPEG_APP1 data block. */ > >>>> + jpeg_write_marker(&compress_, JPEG_APP0 + 1, > >>>> + static_cast<const JOCTET *>(exifData.data()), > >>>> + exifData.size()); > >>>> + > >>>> LOG(JPEG, Debug) << "JPEG Encode Starting:" << compress_.image_width > >>>> << "x" << compress_.image_height; > >>>> > >>>> diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h > >>>> index aed081a..1e8df05 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<const uint8_t> &exifData) override; > >>>> > >>>> private: > >>>> void compressRGB(const libcamera::MappedBuffer *frame); > >>>> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp > >>>> index 41ddf48..36922ef 100644 > >>>> --- a/src/android/jpeg/exif.cpp > >>>> +++ b/src/android/jpeg/exif.cpp > >>>> @@ -168,6 +168,56 @@ void Exif::setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::str > >>>> exif_entry_unref(entry); > >>>> } > >>>> > >>>> +void Exif::setMake(const std::string &make) > >>>> +{ > >>>> + setString(EXIF_IFD_0, EXIF_TAG_MAKE, EXIF_FORMAT_ASCII, make); > >>>> +} > >>>> + > >>>> +void Exif::setModel(const std::string &model) > >>>> +{ > >>>> + setString(EXIF_IFD_0, EXIF_TAG_MODEL, EXIF_FORMAT_ASCII, model); > >>>> +} > >>>> + > >>>> +void Exif::setSize(const Size &size) > >>>> +{ > >>>> + setLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_Y_DIMENSION, size.height); > >>>> + setLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_X_DIMENSION, size.width); > >>>> +} > >>>> + > >>>> +void Exif::setTimestamp(time_t timestamp) > >>>> +{ > >>>> + char str[20]; > >>>> + std::strftime(str, sizeof(str), "%Y:%m:%d %H:%M:%S", > >>>> + std::localtime(×tamp)); > >>>> + std::string ts(str); > >>>> + > >>>> + setString(EXIF_IFD_0, EXIF_TAG_DATE_TIME, EXIF_FORMAT_ASCII, ts); > >>>> + setString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_ORIGINAL, EXIF_FORMAT_ASCII, ts); > >>>> + setString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_DIGITIZED, EXIF_FORMAT_ASCII, ts); > >>>> +} > >>>> + > >>>> +void Exif::setOrientation(int orientation) > >>>> +{ > >>>> + int value; > >>>> + switch (orientation) { > >>>> + case 0: > >>>> + default: > >>>> + value = 1; > >>>> + break; > >>>> + case 90: > >>>> + value = 6; > >>>> + break; > >>>> + case 180: > >>>> + value = 3; > >>>> + break; > >>>> + case 270: > >>>> + value = 8; > >>>> + break; > >>> > >>> Shouldn't the the 90 and 270 values be swapped ? 6 means "The 0th row is > >>> the visual right-hand side of the image, and the 0th column is the > >>> visual top", and 8 means "The 0th row is the visual left-hand side of > >>> the image, and the 0th column is the visual bottom". > >>> > >>> Looking at the rotation property definition, the 90° rotation examples > >>> shows > >>> > >>> +-------------------------------------+ > >>> | _ _ | > >>> | \ / | > >>> | | | | > >>> | | | | > >>> | | > | > >>> | < | | > >>> | | | | > >>> | . | > >>> | V | > >>> +-------------------------------------+ > >>> > >>> where the 0th row is the left side and the 0th column the bottom side. > > Isn't the 0-th row (the first row of the image) on the 'visual > right-hand side' ? Here's my interpretation: Looking at the fish in front of you (forgetting the camera), the visual right-hand side is the head, and the visual left-hand side is the tail. The 0th row of the image (now we're talking about the buffer, as numbering rows implies we're counting pixels, so it's the buffer, not the scene anymore). is the tail, thus the visual left-hand side of the image. > >>> I can fix those two small issues when applying, but I'd appreciate if > >>> someone could double-check the rotation. > >> > >> Does the rotation property has to do anything with the fact that, camera > >> sensor(webcam use-case) is intentionally mounted upside down to avoid > >> rotation correction? > > > > The rotation reported by libcamera exposes the optical rotation of the > > camera module, combining both the camera sensor and the lens. It is > > indeed common to mount the camera sensor upside-down to compensate for > > the lens inverting the image. In that case the reported rotation is 0°, > > as the camera sensor rotation and the lens inversion compensate each > > other. > > The 'rotation' property definition takes into account the lens > inversion, so you shouldn't have to care about it I'm not sure if this means you agree or disagree with my explanation :-) > >> A widely used example on the internet is a big 'F' > >> image denoting the value according to it's orientation [1], which is the > >> same as per the values used in the patch. On the other hand, I can also > >> find the code which satisfies Laurent's review and the EXIF > >> 'orientation' specification [2] > >> > >> [1]: > >> https://chromium.googlesource.com/chromiumos/platform2/+/HEAD/camera/common/exif_utils.cc#376 > >> [2]: > >> https://piexif.readthedocs.io/en/latest/sample.html#rotate-image-by-exif-orientation > > > > The 0° and 180° cases are fairly straightforward, they map to the EXIF > > values 1 and 3 respectively, there's usually no disagreement on that. > > > > The 90° and 270° cases are much more error prone as it's easy to get the > > rotation direction wrong. The mapping between the 'F' orientation and > > the EXIF values shown in [1] seems right to me. > > > > Android defines the rotation as the clockwise angle by which the image > > needs to be rotated to appear in the correct orientation on the device > > screen. > > > > For EXIF value 6, > > > > 88 > > 88 88 > > 8888888888 > > > > the image needs to be rotated by 90° clockwise, while for EXIF value 8, > > > > 8888888888 > > 88 88 > > 88 > > > > it needs to be rotated by 270°. The code in [2] thus appears correct. > > What however appears to be incorrect is our claim that the libcamera and > > Android numerical values are the same. > > > > Jacopo, any opinion on that ? > > Our definition differs from the Android one, as the rotation value we > report is the rotation correction degrees in counter-clockwise direction. > Android defines that in the clockwise direction. So there's a bug in our HAL implementation when reporting the rotation ? > If I got the above discussion right and looking at the 'F' examples in > [1], we should report: > 90 degrees = 8 > 270 degrees = 6 > > However, this interpreation and the 'F' examples, don't match the > textual description Laurent reported above: > > ------------------------------------------------------------------------------- > Shouldn't the the 90 and 270 values be swapped ? 6 means "The 0th row is > the visual right-hand side of the image, and the 0th column is the > visual top", and 8 means "The 0th row is the visual left-hand side of > the image, and the 0th column is the visual bottom". > ------------------------------------------------------------------------------ > > For EXIF value 6, > > 88 > 88 88 > 8888888888 > > the image needs to be rotated by 90° clockwise, while for EXIF value 8, > > 8888888888 > 88 88 > 88 > ------------------------------------------------------------------------------ > > The two do not match, and I wonder what's the authoritative source for > the definition of the semantic associated with the '6' and '8' > values. The EXIF specification is the authoritative source, and it's defined as I've quoted. > >> ¯\_(ツ)_/¯ > >> > >>>> + } > >>>> + > >>>> + setShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, value); > >>>> +} > >>>> + > >>>> [[nodiscard]] int Exif::generate() > >>>> { > >>>> if (exifData_) { > >>>> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h > >>>> index 8dfc324..622de4c 100644 > >>>> --- a/src/android/jpeg/exif.h > >>>> +++ b/src/android/jpeg/exif.h > >>>> @@ -12,6 +12,7 @@ > >>>> > >>>> #include <libexif/exif-data.h> > >>>> > >>>> +#include <libcamera/geometry.h> > >>>> #include <libcamera/span.h> > >>>> > >>>> class Exif > >>>> @@ -20,6 +21,13 @@ public: > >>>> Exif(); > >>>> ~Exif(); > >>>> > >>>> + void setMake(const std::string &make); > >>>> + void setModel(const std::string &model); > >>>> + > >>>> + void setOrientation(int orientation); > >>>> + void setSize(const libcamera::Size &size); > >>>> + void setTimestamp(time_t timestamp); > >>>> + > >>>> libcamera::Span<const uint8_t> data() const { return { exifData_, size_ }; } > >>>> [[nodiscard]] int generate(); > >>>>
Hi Laurent On Tue, Sep 08, 2020 at 03:46:14PM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > On Tue, Sep 08, 2020 at 02:43:46PM +0200, Jacopo Mondi wrote: > > On Tue, Sep 08, 2020 at 03:03:21PM +0300, Laurent Pinchart wrote: > > > On Tue, Sep 08, 2020 at 03:42:39PM +0530, Umang Jain wrote: > > >> On 9/4/20 9:04 PM, Laurent Pinchart wrote: > > >>> On Fri, Sep 04, 2020 at 12:10:19PM +0530, Umang Jain wrote: > > >>>> Create a Exif object with various metadata tags set, just before > > >>>> the encoder starts to encode the frame. The object is passed > > >>>> directly as libcamera::Span<> to make sure EXIF tags can be set > > >>>> in a single place i.e. in CameraDevice and the encoder only has > > >>>> the job to write the data in the final output. > > >>>> > > >>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > >>>> Signed-off-by: Umang Jain <email@uajain.com> > > >>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > >>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > >>>> --- > > >>>> src/android/camera_device.cpp | 19 ++++++++++- > > >>>> src/android/jpeg/encoder.h | 3 +- > > >>>> src/android/jpeg/encoder_libjpeg.cpp | 9 ++++- > > >>>> src/android/jpeg/encoder_libjpeg.h | 3 +- > > >>>> src/android/jpeg/exif.cpp | 50 ++++++++++++++++++++++++++++ > > >>>> src/android/jpeg/exif.h | 8 +++++ > > >>>> 6 files changed, 88 insertions(+), 4 deletions(-) > > >>>> > > >>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > >>>> index de6f86f..0328ac6 100644 > > >>>> --- a/src/android/camera_device.cpp > > >>>> +++ b/src/android/camera_device.cpp > > >>>> @@ -24,6 +24,7 @@ > > >>>> #include "system/graphics.h" > > >>>> > > >>>> #include "jpeg/encoder_libjpeg.h" > > >>>> +#include "jpeg/exif.h" > > >>>> > > >>>> using namespace libcamera; > > >>>> > > >>>> @@ -1434,7 +1435,23 @@ void CameraDevice::requestComplete(Request *request) > > >>>> continue; > > >>>> } > > >>>> > > >>>> - int jpeg_size = encoder->encode(buffer, mapped.maps()[0]); > > >>>> + /* 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(orientation_); > > >>>> + exif.setSize(cameraStream->size); > > >>>> + /* > > >>>> + * We set the frame's EXIF timestamp as the time of encode. > > >>>> + * Since the precision we need for EXIF timestamp is only one > > >>>> + * second, it is good enough. > > >>>> + */ > > >>>> + exif.setTimestamp(std::time(nullptr)); > > >>>> + if (exif.generate() != 0) > > >>>> + LOG(HAL, Error) << "Failed to get valid EXIF data"; > > >>> > > >>> s/get/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/jpeg/encoder.h b/src/android/jpeg/encoder.h > > >>>> index f9eb88e..cf26d67 100644 > > >>>> --- a/src/android/jpeg/encoder.h > > >>>> +++ b/src/android/jpeg/encoder.h > > >>>> @@ -18,7 +18,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<const uint8_t> &exifData) = 0; > > >>>> }; > > >>>> > > >>>> #endif /* __ANDROID_JPEG_ENCODER_H__ */ > > >>>> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp > > >>>> index 977538c..510613c 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<const uint8_t> &exifData) > > >>>> { > > >>>> MappedFrameBuffer frame(source, PROT_READ); > > >>>> if (!frame.isValid()) { > > >>>> @@ -204,6 +205,12 @@ int EncoderLibJpeg::encode(const FrameBuffer *source, > > >>>> > > >>>> jpeg_start_compress(&compress_, TRUE); > > >>>> > > >>>> + if (exifData.size()) > > >>>> + /* Store Exif data in the JPEG_APP1 data block. */ > > >>>> + jpeg_write_marker(&compress_, JPEG_APP0 + 1, > > >>>> + static_cast<const JOCTET *>(exifData.data()), > > >>>> + exifData.size()); > > >>>> + > > >>>> LOG(JPEG, Debug) << "JPEG Encode Starting:" << compress_.image_width > > >>>> << "x" << compress_.image_height; > > >>>> > > >>>> diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h > > >>>> index aed081a..1e8df05 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<const uint8_t> &exifData) override; > > >>>> > > >>>> private: > > >>>> void compressRGB(const libcamera::MappedBuffer *frame); > > >>>> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp > > >>>> index 41ddf48..36922ef 100644 > > >>>> --- a/src/android/jpeg/exif.cpp > > >>>> +++ b/src/android/jpeg/exif.cpp > > >>>> @@ -168,6 +168,56 @@ void Exif::setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::str > > >>>> exif_entry_unref(entry); > > >>>> } > > >>>> > > >>>> +void Exif::setMake(const std::string &make) > > >>>> +{ > > >>>> + setString(EXIF_IFD_0, EXIF_TAG_MAKE, EXIF_FORMAT_ASCII, make); > > >>>> +} > > >>>> + > > >>>> +void Exif::setModel(const std::string &model) > > >>>> +{ > > >>>> + setString(EXIF_IFD_0, EXIF_TAG_MODEL, EXIF_FORMAT_ASCII, model); > > >>>> +} > > >>>> + > > >>>> +void Exif::setSize(const Size &size) > > >>>> +{ > > >>>> + setLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_Y_DIMENSION, size.height); > > >>>> + setLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_X_DIMENSION, size.width); > > >>>> +} > > >>>> + > > >>>> +void Exif::setTimestamp(time_t timestamp) > > >>>> +{ > > >>>> + char str[20]; > > >>>> + std::strftime(str, sizeof(str), "%Y:%m:%d %H:%M:%S", > > >>>> + std::localtime(×tamp)); > > >>>> + std::string ts(str); > > >>>> + > > >>>> + setString(EXIF_IFD_0, EXIF_TAG_DATE_TIME, EXIF_FORMAT_ASCII, ts); > > >>>> + setString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_ORIGINAL, EXIF_FORMAT_ASCII, ts); > > >>>> + setString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_DIGITIZED, EXIF_FORMAT_ASCII, ts); > > >>>> +} > > >>>> + > > >>>> +void Exif::setOrientation(int orientation) > > >>>> +{ > > >>>> + int value; > > >>>> + switch (orientation) { > > >>>> + case 0: > > >>>> + default: > > >>>> + value = 1; > > >>>> + break; > > >>>> + case 90: > > >>>> + value = 6; > > >>>> + break; > > >>>> + case 180: > > >>>> + value = 3; > > >>>> + break; > > >>>> + case 270: > > >>>> + value = 8; > > >>>> + break; > > >>> > > >>> Shouldn't the the 90 and 270 values be swapped ? 6 means "The 0th row is > > >>> the visual right-hand side of the image, and the 0th column is the > > >>> visual top", and 8 means "The 0th row is the visual left-hand side of > > >>> the image, and the 0th column is the visual bottom". > > >>> > > >>> Looking at the rotation property definition, the 90° rotation examples > > >>> shows > > >>> > > >>> +-------------------------------------+ > > >>> | _ _ | > > >>> | \ / | > > >>> | | | | > > >>> | | | | > > >>> | | > | > > >>> | < | | > > >>> | | | | > > >>> | . | > > >>> | V | > > >>> +-------------------------------------+ > > >>> > > >>> where the 0th row is the left side and the 0th column the bottom side. > > > > Isn't the 0-th row (the first row of the image) on the 'visual > > right-hand side' ? > > Here's my interpretation: > > Looking at the fish in front of you (forgetting the camera), the visual > right-hand side is the head, and the visual left-hand side is the tail. > The 0th row of the image (now we're talking about the buffer, as > numbering rows implies we're counting pixels, so it's the buffer, not > the scene anymore). is the tail, thus the visual left-hand side of the > image. > I see. As I've interpreted this, the description was about where the intended first row is located. The 0-th row of the image (what would be the "top" if you look at the image displayed on the screen and correctly rotated) is placed, in the above imge, on the right side. That's how I read "the 0th row is the visual right-hand side", but you interepretation might be correct too, as otherwise they could have phrased it as "the 0th row is placed on the right-hand side of the non-rotated image" > > >>> I can fix those two small issues when applying, but I'd appreciate if > > >>> someone could double-check the rotation. > > >> > > >> Does the rotation property has to do anything with the fact that, camera > > >> sensor(webcam use-case) is intentionally mounted upside down to avoid > > >> rotation correction? > > > > > > The rotation reported by libcamera exposes the optical rotation of the > > > camera module, combining both the camera sensor and the lens. It is > > > indeed common to mount the camera sensor upside-down to compensate for > > > the lens inverting the image. In that case the reported rotation is 0°, > > > as the camera sensor rotation and the lens inversion compensate each > > > other. > > > > The 'rotation' property definition takes into account the lens > > inversion, so you shouldn't have to care about it > > I'm not sure if this means you agree or disagree with my explanation :-) > > > >> A widely used example on the internet is a big 'F' > > >> image denoting the value according to it's orientation [1], which is the > > >> same as per the values used in the patch. On the other hand, I can also > > >> find the code which satisfies Laurent's review and the EXIF > > >> 'orientation' specification [2] > > >> > > >> [1]: > > >> https://chromium.googlesource.com/chromiumos/platform2/+/HEAD/camera/common/exif_utils.cc#376 > > >> [2]: > > >> https://piexif.readthedocs.io/en/latest/sample.html#rotate-image-by-exif-orientation > > > > > > The 0° and 180° cases are fairly straightforward, they map to the EXIF > > > values 1 and 3 respectively, there's usually no disagreement on that. > > > > > > The 90° and 270° cases are much more error prone as it's easy to get the > > > rotation direction wrong. The mapping between the 'F' orientation and > > > the EXIF values shown in [1] seems right to me. > > > > > > Android defines the rotation as the clockwise angle by which the image > > > needs to be rotated to appear in the correct orientation on the device > > > screen. > > > > > > For EXIF value 6, > > > > > > 88 > > > 88 88 > > > 8888888888 > > > > > > the image needs to be rotated by 90° clockwise, while for EXIF value 8, > > > > > > 8888888888 > > > 88 88 > > > 88 > > > > > > it needs to be rotated by 270°. The code in [2] thus appears correct. > > > What however appears to be incorrect is our claim that the libcamera and > > > Android numerical values are the same. > > > > > > Jacopo, any opinion on that ? > > > > Our definition differs from the Android one, as the rotation value we > > report is the rotation correction degrees in counter-clockwise direction. > > Android defines that in the clockwise direction. > > So there's a bug in our HAL implementation when reporting the rotation ? > I'm pretty sure we discussed that when we addressed that. Have we missed it that badly ? Seems weird but seems it's the case... > > If I got the above discussion right and looking at the 'F' examples in > > [1], we should report: > > 90 degrees = 8 > > 270 degrees = 6 > > > > However, this interpreation and the 'F' examples, don't match the > > textual description Laurent reported above: > > > > ------------------------------------------------------------------------------- > > Shouldn't the the 90 and 270 values be swapped ? 6 means "The 0th row is > > the visual right-hand side of the image, and the 0th column is the > > visual top", and 8 means "The 0th row is the visual left-hand side of > > the image, and the 0th column is the visual bottom". > > ------------------------------------------------------------------------------ > > > > For EXIF value 6, > > > > 88 > > 88 88 > > 8888888888 > > > > the image needs to be rotated by 90° clockwise, while for EXIF value 8, > > > > 8888888888 > > 88 88 > > 88 > > ------------------------------------------------------------------------------ > > > > The two do not match, and I wonder what's the authoritative source for > > the definition of the semantic associated with the '6' and '8' > > values. > > The EXIF specification is the authoritative source, and it's defined as > I've quoted. > > > >> ¯\_(ツ)_/¯ > > >> > > >>>> + } > > >>>> + > > >>>> + setShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, value); > > >>>> +} > > >>>> + > > >>>> [[nodiscard]] int Exif::generate() > > >>>> { > > >>>> if (exifData_) { > > >>>> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h > > >>>> index 8dfc324..622de4c 100644 > > >>>> --- a/src/android/jpeg/exif.h > > >>>> +++ b/src/android/jpeg/exif.h > > >>>> @@ -12,6 +12,7 @@ > > >>>> > > >>>> #include <libexif/exif-data.h> > > >>>> > > >>>> +#include <libcamera/geometry.h> > > >>>> #include <libcamera/span.h> > > >>>> > > >>>> class Exif > > >>>> @@ -20,6 +21,13 @@ public: > > >>>> Exif(); > > >>>> ~Exif(); > > >>>> > > >>>> + void setMake(const std::string &make); > > >>>> + void setModel(const std::string &model); > > >>>> + > > >>>> + void setOrientation(int orientation); > > >>>> + void setSize(const libcamera::Size &size); > > >>>> + void setTimestamp(time_t timestamp); > > >>>> + > > >>>> libcamera::Span<const uint8_t> data() const { return { exifData_, size_ }; } > > >>>> [[nodiscard]] int generate(); > > >>>> > > -- > Regards, > > Laurent Pinchart
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index de6f86f..0328ac6 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -24,6 +24,7 @@ #include "system/graphics.h" #include "jpeg/encoder_libjpeg.h" +#include "jpeg/exif.h" using namespace libcamera; @@ -1434,7 +1435,23 @@ void CameraDevice::requestComplete(Request *request) continue; } - int jpeg_size = encoder->encode(buffer, mapped.maps()[0]); + /* 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(orientation_); + exif.setSize(cameraStream->size); + /* + * We set the frame's EXIF timestamp as the time of encode. + * Since the precision we need for EXIF timestamp is only one + * second, it is good enough. + */ + exif.setTimestamp(std::time(nullptr)); + if (exif.generate() != 0) + LOG(HAL, Error) << "Failed to get valid EXIF data"; + + 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/jpeg/encoder.h b/src/android/jpeg/encoder.h index f9eb88e..cf26d67 100644 --- a/src/android/jpeg/encoder.h +++ b/src/android/jpeg/encoder.h @@ -18,7 +18,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<const uint8_t> &exifData) = 0; }; #endif /* __ANDROID_JPEG_ENCODER_H__ */ diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp index 977538c..510613c 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<const uint8_t> &exifData) { MappedFrameBuffer frame(source, PROT_READ); if (!frame.isValid()) { @@ -204,6 +205,12 @@ int EncoderLibJpeg::encode(const FrameBuffer *source, jpeg_start_compress(&compress_, TRUE); + if (exifData.size()) + /* Store Exif data in the JPEG_APP1 data block. */ + jpeg_write_marker(&compress_, JPEG_APP0 + 1, + static_cast<const JOCTET *>(exifData.data()), + exifData.size()); + LOG(JPEG, Debug) << "JPEG Encode Starting:" << compress_.image_width << "x" << compress_.image_height; diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h index aed081a..1e8df05 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<const uint8_t> &exifData) override; private: void compressRGB(const libcamera::MappedBuffer *frame); diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp index 41ddf48..36922ef 100644 --- a/src/android/jpeg/exif.cpp +++ b/src/android/jpeg/exif.cpp @@ -168,6 +168,56 @@ void Exif::setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::str exif_entry_unref(entry); } +void Exif::setMake(const std::string &make) +{ + setString(EXIF_IFD_0, EXIF_TAG_MAKE, EXIF_FORMAT_ASCII, make); +} + +void Exif::setModel(const std::string &model) +{ + setString(EXIF_IFD_0, EXIF_TAG_MODEL, EXIF_FORMAT_ASCII, model); +} + +void Exif::setSize(const Size &size) +{ + setLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_Y_DIMENSION, size.height); + setLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_X_DIMENSION, size.width); +} + +void Exif::setTimestamp(time_t timestamp) +{ + char str[20]; + std::strftime(str, sizeof(str), "%Y:%m:%d %H:%M:%S", + std::localtime(×tamp)); + std::string ts(str); + + setString(EXIF_IFD_0, EXIF_TAG_DATE_TIME, EXIF_FORMAT_ASCII, ts); + setString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_ORIGINAL, EXIF_FORMAT_ASCII, ts); + setString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_DIGITIZED, EXIF_FORMAT_ASCII, ts); +} + +void Exif::setOrientation(int orientation) +{ + int value; + switch (orientation) { + case 0: + default: + value = 1; + break; + case 90: + value = 6; + break; + case 180: + value = 3; + break; + case 270: + value = 8; + break; + } + + setShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, value); +} + [[nodiscard]] int Exif::generate() { if (exifData_) { diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h index 8dfc324..622de4c 100644 --- a/src/android/jpeg/exif.h +++ b/src/android/jpeg/exif.h @@ -12,6 +12,7 @@ #include <libexif/exif-data.h> +#include <libcamera/geometry.h> #include <libcamera/span.h> class Exif @@ -20,6 +21,13 @@ public: Exif(); ~Exif(); + void setMake(const std::string &make); + void setModel(const std::string &model); + + void setOrientation(int orientation); + void setSize(const libcamera::Size &size); + void setTimestamp(time_t timestamp); + libcamera::Span<const uint8_t> data() const { return { exifData_, size_ }; } [[nodiscard]] int generate();